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

[Laurent Vivier]

Le 26/06/2019 à 10:57, Philippe Mathieu-Daudé a écrit :
> 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?
> 

It's more complicated than that, because access size depends on a
register value:

static uint16_t dp8393x_get(dp8393xState *s, int width, uint16_t *base,
                            int offset)
{
    uint16_t val;

    if (s->big_endian) {
        val = be16_to_cpu(base[offset * width + width - 1]);
    } else {
        val = le16_to_cpu(base[offset * width]);
    }
    return val;
}

and width is:

width = (s->regs[SONIC_DCR] & SONIC_DCR_DW) ? 2 : 1;

So in the end we always need the big_endian flag to know how to read the
memory. I think it's simpler to read/write the memory (like a real DMA
access), and then to swap data internally.

Moreover, the big-endian/little-endian is a real feature of the
controller (see  1.3 DATA WIDTH AND BYTE ORDERING,
http://pccomponents.com/datasheets/NSC83932.PDF )

Thanks,
Laurent