Re: [Qemu-devel] [PATCH v8 03/10] dp8393x: manage big endian bus

[Philippe Mathieu-Daudé]

On 6/25/19 7:09 PM, Laurent Vivier wrote:
> Le 25/06/2019 à 17:57, Philippe Mathieu-Daudé a écrit :
>> On 6/24/19 10:07 PM, Laurent Vivier wrote:
>>> Hi,
>>>
>>> Jason, Can I have an Acked-by from you (as network devices maintainer)?
>>
>> Hmm something seems odd here indeed...
>>
>> What a stable model! This file has no logical modification since its
>> introduction, a65f56eeba "Implement sonic netcard (MIPS Jazz)"
>>
>> Here we had:
>>
>> static void dp8393x_writeb(void *opaque, hwaddr addr, uint32_t val)
>> {
>>     uint16_t old_val = dp8393x_readw(opaque, addr & ~0x1);
>>
>>     switch (addr & 3) {
>>     case 0:
>>         val = val | (old_val & 0xff00);
>>         break;
>>     case 1:
>>         val = (val << 8) | (old_val & 0x00ff);
>>         break;
>>     }
>>     dp8393x_writew(opaque, addr & ~0x1, val);
>> }
>>
>> So we had 16-bit endian shifting there.
>>
>> And few lines below:
>>
>>     /* XXX: Check byte ordering */
>>     ...
>>     /* Calculate the ethernet checksum */
>>     #ifdef SONIC_CALCULATE_RXCRC
>>         checksum = cpu_to_le32(crc32(0, buf, rx_len));
>>     #else
>>         checksum = 0;
>>     #endif
>>
>> After various housekeeping, we get:
>>
>> 84689cbb97 "net/dp8393x: do not use old_mmio accesses"
>>
>> The MIPS Jazz is known to run in both endianess, but I haven't checked
>> if at that time both were available.
>>
>> Have you tried this patch?
>>
>> -- >8 --
>> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
>> index bdb0b3b2c2..646e11206f 100644
>> @@ -651,7 +651,7 @@ static const MemoryRegionOps dp8393x_ops = {
>>      .write = dp8393x_write,
>>      .impl.min_access_size = 2,
>>      .impl.max_access_size = 2,
>> -    .endianness = DEVICE_NATIVE_ENDIAN,
>> +    .endianness = DEVICE_LITTLE_ENDIAN,
>>  };
>> ---
>>
>> (but then mips64-softmmu Jazz would have networking broken).
>>
> 
> I doesn't help, the endianness is a MemoryRegion property (see
> memory_region_wrong_endianness()) so it is used when the CPU writes to
> the device MMIO, not when the device accesses the other memory.
> In this case, it reads from system_memory. Perhaps we can create the
> address_space with a system_memory in big endian mode?

Ah I missed that...

What about not using address_space_rw(data) but directly use
address_space_lduw_le() and address_space_stw_le() instead?