Re: [PATCH 12/19] hw/net/xilinx_ethlite: Only expect big-endian accesses

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 5/11/24 14:18, Paolo Bonzini wrote:
> On 11/5/24 14:04, Philippe Mathieu-Daudé wrote:
>> The Xilinx 'ethlite' device was added in commit b43848a100
>> ("xilinx: Add ethlite emulation"), being only built back
>> then for a big-endian MicroBlaze target (see commit 72b675caac
>> "microblaze: Hook into the build-system").
>>
>> I/O endianness access was then clarified in commit d48751ed4f
>> ("xilinx-ethlite: Simplify byteswapping to/from brams"). Here
>> the 'fix' was to use tswap32(). Since the machine was built as
>> big-endian target, tswap32() use means the fix was for a little
>> endian host. While the datasheet (reference added in file header)
>> is not precise about it, we interpret such change as the device
>> expects accesses in big-endian order. Besides, this is what other
>> Xilinx/MicroBlaze devices use (see the 3 previous commits).
>>
>> Correct the MemoryRegionOps endianness. Add a 'access-little-endian'
>> property, so if the bus access expect little-endian order we swap
>> the values. Replace the tswap32() calls accordingly.
>>
>> Set the property on the single machine using this device.
> 
> I don't understand.  This machine type is little-endian only and 
> expecting inverted accesses, isn't it?  Therefore, all that you need is
> 
>> -    .endianness = DEVICE_NATIVE_ENDIAN,
>> +    .endianness = DEVICE_BIG_ENDIAN,
> 
> And removing the tswap altogether.  The big-endian petalogix board will 
> start getting "correct" values (not swapped anymore).  That's a feature, 
> not a bug.

The feature is memory.c swapping MemoryRegionOps depending on the
*qemu-system binary* target endianness.

We assumed most guest vCPUs run with the same endianness of the binary.

Now we want to swap wrt the vCPU, not the binary. So indeed this patch
effectively undo the memory.c swapping (feature).

I suppose the better way is to modify memory.c, possibly passing MemOp
all over. For HW accel where vCPU endianness is forced to host one,
this would become a no-op. Lot of rework in perspective.