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

[Paolo Bonzini]

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.

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?

Paolo