Re: [RFC PATCH v3 04/17] hw/net/xilinx_ethlite: Simplify by having configurable endianness

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

On 8/11/24 16:05, Paolo Bonzini wrote:
> On 11/8/24 16:43, 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.
>>
>> Instead of having a double swapping, one in the core memory layer
>> due to DEVICE_NATIVE_ENDIAN and a second one with the tswap calls,
>> allow the machine code to select the proper endianness desired,
>> removing the need of tswap().
>>
>> Replace the DEVICE_NATIVE_ENDIAN MemoryRegionOps by a pair of
>> DEVICE_LITTLE_ENDIAN / DEVICE_BIG_ENDIAN.
>> Add the "little-endian" property to select the device endianness,
>> defaulting to little endian.
>> Set the proper endianness on the single machine using the device.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>> RFC until I digest Paolo's review from v1:
>> https://lore.kernel.org/qemu-devel/34f6fe2f-06e0-4e2a-a361-2d662f6814b5@redhat.com/
> 
> tl;dr: this works but would break migration compatibility with the 
> previous version.  If you want to keep that, you need to add
> 
>> -            r = tswap32(s->regs[addr]);
>> +            r = s->regs[addr];
> 
> if (s->little_endian_model)
>      r = cpu_to_le32(r);
> else
>      r = cpu_to_be32(r);
> 
> 
>> @@ -161,23 +165,26 @@ eth_write(void *opaque, hwaddr addr,
>>               break;
>>           default:
> 
> if (s->little_endian_model)
>      r = le32_to_cpu(r);
> else
>      r = be32_to_cpu(r);
> 
>> -            s->regs[addr] = tswap32(value);
>> +            s->regs[addr] = value;
>>               break;
> 
> These pairs ensure that RAM goes through an even number of swaps.  I 
> don't think they are needed but you decide.

Indeed; I didn't realize it was RAM.

> However, I am wondering if the double MemoryRegionOps are needed *at 
> all*.  Since petalogix is arguably a little-endian only machine, can you 
> just use DEVICE_LITTLE_ENDIAN?

1/ This petalogix machine is actually built in the big-endian binary
2/ As Edgar mentioned elsewhere, Petalogic IP can be synthetized as
    big-endian
3/ This machine is used to prove we can remove the TARGET_BIG_ENDIAN
    definition and unify big/little endian binaries in our build system.