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

["Edgar E. Iglesias" <edgar.iglesias@gmail.com>]

On Tue, Nov 05, 2024 at 02:04:24PM +0100, 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 think you're partially correct but not fully. This buffer area is
really a RAM and has no endianess. Problem is back then I don't think
I was a ware of a way to map RAM memory sub regions so we hacked in
byteswaps to swap from host (which usually was little endian) to
big endian. This is because register accesses from CPU to device model
are kept in host endianess. I think the right way to solve this issue
is to map a RAM memory region to represent the BRAM.

Cheers,
Edgar


> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/microblaze/petalogix_s3adsp1800_mmu.c |  1 +
>  hw/net/xilinx_ethlite.c                  | 20 ++++++++++++++++----
>  2 files changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/microblaze/petalogix_s3adsp1800_mmu.c b/hw/microblaze/petalogix_s3adsp1800_mmu.c
> index 8110be83715..8407dbee12a 100644
> --- a/hw/microblaze/petalogix_s3adsp1800_mmu.c
> +++ b/hw/microblaze/petalogix_s3adsp1800_mmu.c
> @@ -123,6 +123,7 @@ petalogix_s3adsp1800_init(MachineState *machine)
>      qemu_configure_nic_device(dev, true, NULL);
>      qdev_prop_set_uint32(dev, "tx-ping-pong", 0);
>      qdev_prop_set_uint32(dev, "rx-ping-pong", 0);
> +    qdev_prop_set_bit(dev, "access-little-endian", !TARGET_BIG_ENDIAN);
>      sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>      sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, ETHLITE_BASEADDR);
>      sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, irq[ETHLITE_IRQ]);
> diff --git a/hw/net/xilinx_ethlite.c b/hw/net/xilinx_ethlite.c
> index ede7c172748..44ef11ebf89 100644
> --- a/hw/net/xilinx_ethlite.c
> +++ b/hw/net/xilinx_ethlite.c
> @@ -3,6 +3,9 @@
>   *
>   * Copyright (c) 2009 Edgar E. Iglesias.
>   *
> + * DS580: https://docs.amd.com/v/u/en-US/xps_ethernetlite
> + * LogiCORE IP XPS Ethernet Lite Media Access Controller
> + *
>   * Permission is hereby granted, free of charge, to any person obtaining a copy
>   * of this software and associated documentation files (the "Software"), to deal
>   * in the Software without restriction, including without limitation the rights
> @@ -25,7 +28,6 @@
>  #include "qemu/osdep.h"
>  #include "qemu/module.h"
>  #include "qom/object.h"
> -#include "exec/tswap.h"
>  #include "hw/sysbus.h"
>  #include "hw/irq.h"
>  #include "hw/qdev-properties.h"
> @@ -65,6 +67,7 @@ struct xlx_ethlite
>      NICState *nic;
>      NICConf conf;
>  
> +    bool access_little_endian;
>      uint32_t c_tx_pingpong;
>      uint32_t c_rx_pingpong;
>      unsigned int txbuf;
> @@ -103,9 +106,12 @@ eth_read(void *opaque, hwaddr addr, unsigned int size)
>              break;
>  
>          default:
> -            r = tswap32(s->regs[addr]);
> +            r = s->regs[addr];
>              break;
>      }
> +    if (s->access_little_endian) {
> +        bswap32s(&r);
> +    }
>      return r;
>  }
>  
> @@ -117,6 +123,10 @@ eth_write(void *opaque, hwaddr addr,
>      unsigned int base = 0;
>      uint32_t value = val64;
>  
> +    if (s->access_little_endian) {
> +        bswap32s(&value);
> +    }
> +
>      addr >>= 2;
>      switch (addr) 
>      {
> @@ -161,7 +171,7 @@ eth_write(void *opaque, hwaddr addr,
>              break;
>  
>          default:
> -            s->regs[addr] = tswap32(value);
> +            s->regs[addr] = value;
>              break;
>      }
>  }
> @@ -169,7 +179,7 @@ eth_write(void *opaque, hwaddr addr,
>  static const MemoryRegionOps eth_ops = {
>      .read = eth_read,
>      .write = eth_write,
> -    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .endianness = DEVICE_BIG_ENDIAN,
>      .impl = {
>          .min_access_size = 4,
>          .max_access_size = 4,
> @@ -256,6 +266,8 @@ static void xilinx_ethlite_init(Object *obj)
>  }
>  
>  static Property xilinx_ethlite_properties[] = {
> +    DEFINE_PROP_BOOL("access-little-endian", struct xlx_ethlite,
> +                     access_little_endian, false),
>      DEFINE_PROP_UINT32("tx-ping-pong", struct xlx_ethlite, c_tx_pingpong, 1),
>      DEFINE_PROP_UINT32("rx-ping-pong", struct xlx_ethlite, c_rx_pingpong, 1),
>      DEFINE_NIC_PROPERTIES(struct xlx_ethlite, conf),
> -- 
> 2.45.2
>