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

[Laurent Vivier]

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?

Thanks,
Laurent