diff mbox series

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

Message ID 20241108154317.12129-5-philmd@linaro.org
State New
Headers show
Series hw/microblaze: Allow running cross-endian vCPUs | expand

Commit Message

Philippe Mathieu-Daudé Nov. 8, 2024, 3:43 p.m. UTC 
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/
---
 hw/microblaze/petalogix_s3adsp1800_mmu.c |  1 +
 hw/net/xilinx_ethlite.c                  | 44 +++++++++++++++---------
 2 files changed, 28 insertions(+), 17 deletions(-)

Comments

Paolo Bonzini Nov. 8, 2024, 4:05 p.m. UTC | #1 
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
diff mbox series

Patch

diff --git a/hw/microblaze/petalogix_s3adsp1800_mmu.c b/hw/microblaze/petalogix_s3adsp1800_mmu.c
index af949196d3..f02d343e29 100644
--- a/hw/microblaze/petalogix_s3adsp1800_mmu.c
+++ b/hw/microblaze/petalogix_s3adsp1800_mmu.c
@@ -121,6 +121,7 @@  petalogix_s3adsp1800_init(MachineState *machine)
     sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, irq[TIMER_IRQ]);
 
     dev = qdev_new("xlnx.xps-ethernetlite");
+    qdev_prop_set_bit(dev, "little-endian", !TARGET_BIG_ENDIAN);
     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);
diff --git a/hw/net/xilinx_ethlite.c b/hw/net/xilinx_ethlite.c
index e84b4cdd35..5c27f1250d 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"
@@ -60,6 +62,7 @@  struct xlx_ethlite
 {
     SysBusDevice parent_obj;
 
+    bool little_endian_model;
     MemoryRegion mmio;
     qemu_irq irq;
     NICState *nic;
@@ -103,9 +106,10 @@  eth_read(void *opaque, hwaddr addr, unsigned int size)
             break;
 
         default:
-            r = tswap32(s->regs[addr]);
+            r = s->regs[addr];
             break;
     }
+
     return r;
 }
 
@@ -161,23 +165,26 @@  eth_write(void *opaque, hwaddr addr,
             break;
 
         default:
-            s->regs[addr] = tswap32(value);
+            s->regs[addr] = value;
             break;
     }
 }
 
-static const MemoryRegionOps eth_ops = {
-    .read = eth_read,
-    .write = eth_write,
-    .endianness = DEVICE_NATIVE_ENDIAN,
-    .impl = {
-        .min_access_size = 4,
-        .max_access_size = 4,
+static const MemoryRegionOps eth_ops[2] = {
+    [0 ... 1] = {
+        .read = eth_read,
+        .write = eth_write,
+        .impl = {
+            .min_access_size = 4,
+            .max_access_size = 4,
+        },
+        .valid = {
+            .min_access_size = 4,
+            .max_access_size = 4,
+        },
     },
-    .valid = {
-        .min_access_size = 4,
-        .max_access_size = 4
-    }
+    [0].endianness = DEVICE_BIG_ENDIAN,
+    [1].endianness = DEVICE_LITTLE_ENDIAN,
 };
 
 static bool eth_can_rx(NetClientState *nc)
@@ -237,6 +244,10 @@  static void xilinx_ethlite_realize(DeviceState *dev, Error **errp)
 {
     struct xlx_ethlite *s = XILINX_ETHLITE(dev);
 
+    memory_region_init_io(&s->mmio, OBJECT(dev),
+                          &eth_ops[s->little_endian_model], s,
+                          "xlnx.xps-ethernetlite", R_MAX * 4);
+
     qemu_macaddr_default_if_unset(&s->conf.macaddr);
     s->nic = qemu_new_nic(&net_xilinx_ethlite_info, &s->conf,
                           object_get_typename(OBJECT(dev)), dev->id,
@@ -249,13 +260,12 @@  static void xilinx_ethlite_init(Object *obj)
     struct xlx_ethlite *s = XILINX_ETHLITE(obj);
 
     sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->irq);
-
-    memory_region_init_io(&s->mmio, obj, &eth_ops, s,
-                          "xlnx.xps-ethernetlite", R_MAX * 4);
     sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mmio);
 }
 
 static Property xilinx_ethlite_properties[] = {
+    DEFINE_PROP_BOOL("little-endian", struct xlx_ethlite,
+                     little_endian_model, true),
     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),