On 5/11/24 13:30, Richard Henderson wrote:
On 11/5/24 13: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.
Signed-off-by: Philippe Mathieu-Daudé <philmd@xxxxxxxxxx>
---
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),
I'm not a fan of performing any swapping within device code.
The memory subsystem should do all of it.
A better solution is two tables, eth_ops_{be,le}, which differ only in
the setting of .endianness. Handle the property by registering the
correct MemoryRegionOps during init.
Squashing this on top to have the two tables, but we still need to
swap back the effect of memory.c swapping for the binary, so I don't
think this is what you want:
-- >8 --
diff --git a/hw/net/xilinx_ethlite.c b/hw/net/xilinx_ethlite.c
index 44ef11ebf89..d58757eb919 100644
--- a/hw/net/xilinx_ethlite.c
+++ b/hw/net/xilinx_ethlite.c
@@ -67,6 +67,7 @@ struct xlx_ethlite
NICState *nic;
NICConf conf;
+ bool model_little_endian;
bool access_little_endian;
uint32_t c_tx_pingpong;
uint32_t c_rx_pingpong;
@@ -109,7 +110,7 @@ eth_read(void *opaque, hwaddr addr, unsigned int size)
r = s->regs[addr];
break;
}
- if (s->access_little_endian) {
+ if (s->access_little_endian ^ s->model_little_endian) {
bswap32s(&r);
}
return r;
@@ -123,7 +124,7 @@ eth_write(void *opaque, hwaddr addr,
unsigned int base = 0;
uint32_t value = val64;
- if (s->access_little_endian) {
+ if (s->access_little_endian ^ s->model_little_endian) {
bswap32s(&value);
}
@@ -176,7 +177,7 @@ eth_write(void *opaque, hwaddr addr,
}
}
-static const MemoryRegionOps eth_ops = {
+static const MemoryRegionOps eth_ops_be = {
.read = eth_read,
.write = eth_write,
.endianness = DEVICE_BIG_ENDIAN,
@@ -190,6 +191,20 @@ static const MemoryRegionOps eth_ops = {
},
};
+static const MemoryRegionOps eth_ops_le = {
+ .read = eth_read,
+ .write = eth_write,
+ .endianness = DEVICE_LITTLE_ENDIAN,
+ .impl = {
+ .min_access_size = 4,
+ .max_access_size = 4,
+ },
+ .valid = {
+ .min_access_size = 4,
+ .max_access_size = 4,
+ },
+};
+
static bool eth_can_rx(NetClientState *nc)
{
struct xlx_ethlite *s = qemu_get_nic_opaque(nc);
@@ -247,6 +262,11 @@ static void xilinx_ethlite_realize(DeviceState
*dev, Error **errp)
{
struct xlx_ethlite *s = XILINX_ETHLITE(dev);
+ memory_region_init_io(&s->mmio, OBJECT(dev),
+ s->model_little_endian ? ð_ops_le :
ð_ops_be, s,
+ "xlnx.xps-ethernetlite", R_MAX * 4);
+ sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->mmio);
+
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,
@@ -259,15 +279,13 @@ 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, ð_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("access-little-endian", struct xlx_ethlite,
access_little_endian, false),
+ DEFINE_PROP_BOOL("model-little-endian", struct xlx_ethlite,
+ model_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),
---
