On Tue, Nov 5, 2024 at 11:06 PM Philippe Mathieu-Daudé <philmd@linaro.org> wrote:

Rename the 'endian' property as 'little-endian' because the 'ENDI' bit is set when the endianness is in little order, and unset in big order.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair

--- hw/microblaze/petalogix_ml605_mmu.c | 2 +- hw/microblaze/xlnx-zynqmp-pmu.c | 2 +- target/microblaze/cpu.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/microblaze/petalogix_ml605_mmu.c b/hw/microblaze/petalogix_ml605_mmu.c index b4183c5267d..df808ac323e 100644 --- a/hw/microblaze/petalogix_ml605_mmu.c +++ b/hw/microblaze/petalogix_ml605_mmu.c @@ -90,7 +90,7 @@ petalogix_ml605_init(MachineState *machine) object_property_set_int(OBJECT(cpu), "use-fpu", 1, &error_abort); object_property_set_bool(OBJECT(cpu), "dcache-writeback", true, &error_abort); - object_property_set_bool(OBJECT(cpu), "endianness", true, &error_abort); + object_property_set_bool(OBJECT(cpu), "little-endian", true, &error_abort); qdev_realize(DEVICE(cpu), NULL, &error_abort);

/* Attach emulated BRAM through the LMB. */ diff --git a/hw/microblaze/xlnx-zynqmp-pmu.c b/hw/microblaze/xlnx-zynqmp-pmu.c index 1bfc9641d29..43608c2dca4 100644 --- a/hw/microblaze/xlnx-zynqmp-pmu.c +++ b/hw/microblaze/xlnx-zynqmp-pmu.c @@ -90,7 +90,7 @@ static void xlnx_zynqmp_pmu_soc_realize(DeviceState *dev, Error **errp) object_property_set_bool(OBJECT(&s->cpu), "use-pcmp-instr", true, &error_abort); object_property_set_bool(OBJECT(&s->cpu), "use-mmu", false, &error_abort); - object_property_set_bool(OBJECT(&s->cpu), "endianness", true, + object_property_set_bool(OBJECT(&s->cpu), "little-endian", true, &error_abort); object_property_set_str(OBJECT(&s->cpu), "version", "8.40.b", &error_abort); diff --git a/target/microblaze/cpu.c b/target/microblaze/cpu.c index 135947ee800..e9f98806274 100644 --- a/target/microblaze/cpu.c +++ b/target/microblaze/cpu.c @@ -368,7 +368,7 @@ static Property mb_properties[] = { DEFINE_PROP_UINT8("use-non-secure", MicroBlazeCPU, cfg.use_non_secure, 0), DEFINE_PROP_BOOL("dcache-writeback", MicroBlazeCPU, cfg.dcache_writeback, false), - DEFINE_PROP_BOOL("endianness", MicroBlazeCPU, cfg.endi, false), + DEFINE_PROP_BOOL("little-endian", MicroBlazeCPU, cfg.endi, false), /* Enables bus exceptions on failed data accesses (load/stores). */ DEFINE_PROP_BOOL("dopb-bus-exception", MicroBlazeCPU, cfg.dopb_bus_exception, false), -- 2.45.2

Hi Edgar, On 5/11/24 23:54, Edgar E. Iglesias wrote:

On Tue, Nov 05, 2024 at 02:04:13PM +0100, Philippe Mathieu-Daudé wrote:

...

Rename the 'endian' property as 'little-endian' because the 'ENDI' bit is set when the endianness is in little order, and unset in big order.

Hi Phil,

Unfortunately, these properties are not only QEMU internal these got named from the bindings Xilinx choose way back in time.

This will likely break many of the Xilinx flows with automatic dts to qemu property conversions so I don't think it's a good idea to rename it. If you like to clarify things perhaps we could keep an alias for the old one?

Adding an alias is the safest way, I'll respin this patch. Note however I'm worried about this fragile disconnect between Xilinx dts conversion which isn't exercised on mainstream (in particular if you get busy and can't review).

For example: https://github.com/torvalds/linux/blob/master/arch/microblaze/boot/dts/syste...

Cheers, Edgar

...

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- hw/microblaze/petalogix_ml605_mmu.c | 2 +- hw/microblaze/xlnx-zynqmp-pmu.c | 2 +- target/microblaze/cpu.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/microblaze/petalogix_ml605_mmu.c b/hw/microblaze/petalogix_ml605_mmu.c index b4183c5267d..df808ac323e 100644 --- a/hw/microblaze/petalogix_ml605_mmu.c +++ b/hw/microblaze/petalogix_ml605_mmu.c @@ -90,7 +90,7 @@ petalogix_ml605_init(MachineState *machine) object_property_set_int(OBJECT(cpu), "use-fpu", 1, &error_abort); object_property_set_bool(OBJECT(cpu), "dcache-writeback", true, &error_abort); - object_property_set_bool(OBJECT(cpu), "endianness", true, &error_abort); + object_property_set_bool(OBJECT(cpu), "little-endian", true, &error_abort); qdev_realize(DEVICE(cpu), NULL, &error_abort);

/* Attach emulated BRAM through the LMB. */ diff --git a/hw/microblaze/xlnx-zynqmp-pmu.c b/hw/microblaze/xlnx-zynqmp-pmu.c index 1bfc9641d29..43608c2dca4 100644 --- a/hw/microblaze/xlnx-zynqmp-pmu.c +++ b/hw/microblaze/xlnx-zynqmp-pmu.c @@ -90,7 +90,7 @@ static void xlnx_zynqmp_pmu_soc_realize(DeviceState *dev, Error **errp) object_property_set_bool(OBJECT(&s->cpu), "use-pcmp-instr", true, &error_abort); object_property_set_bool(OBJECT(&s->cpu), "use-mmu", false, &error_abort); - object_property_set_bool(OBJECT(&s->cpu), "endianness", true, + object_property_set_bool(OBJECT(&s->cpu), "little-endian", true, &error_abort); object_property_set_str(OBJECT(&s->cpu), "version", "8.40.b", &error_abort); diff --git a/target/microblaze/cpu.c b/target/microblaze/cpu.c index 135947ee800..e9f98806274 100644 --- a/target/microblaze/cpu.c +++ b/target/microblaze/cpu.c @@ -368,7 +368,7 @@ static Property mb_properties[] = { DEFINE_PROP_UINT8("use-non-secure", MicroBlazeCPU, cfg.use_non_secure, 0), DEFINE_PROP_BOOL("dcache-writeback", MicroBlazeCPU, cfg.dcache_writeback, false), - DEFINE_PROP_BOOL("endianness", MicroBlazeCPU, cfg.endi, false), + DEFINE_PROP_BOOL("little-endian", MicroBlazeCPU, cfg.endi, false), /* Enables bus exceptions on failed data accesses (load/stores). */ DEFINE_PROP_BOOL("dopb-bus-exception", MicroBlazeCPU, cfg.dopb_bus_exception, false), -- 2.45.2

On Tue, Nov 05, 2024 at 11:18:31PM +0000, Philippe Mathieu-Daudé wrote:

On 5/11/24 23:01, Philippe Mathieu-Daudé wrote:

...

Hi Edgar,

On 5/11/24 23:54, Edgar E. Iglesias wrote:

...

On Tue, Nov 05, 2024 at 02:04:13PM +0100, Philippe Mathieu-Daudé wrote:

...

Rename the 'endian' property as 'little-endian' because the 'ENDI' bit is set when the endianness is in little order, and unset in big order.

Hi Phil,

Unfortunately, these properties are not only QEMU internal these got named from the bindings Xilinx choose way back in time.

This will likely break many of the Xilinx flows with automatic dts to qemu property conversions so I don't think it's a good idea to rename it. If you like to clarify things perhaps we could keep an alias for the old one?

Adding an alias is the safest way, I'll respin this patch.

Note however I'm worried about this fragile disconnect between Xilinx dts conversion which isn't exercised on mainstream (in particular if you get busy and can't review).

...

For example: https://github.com/torvalds/linux/blob/master/arch/microblaze/boot/dts/syste...

Cheers, Edgar

...

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> ---   hw/microblaze/petalogix_ml605_mmu.c | 2 +-   hw/microblaze/xlnx-zynqmp-pmu.c     | 2 +-   target/microblaze/cpu.c             | 2 +-   3 files changed, 3 insertions(+), 3 deletions(-)

...

...

...

diff --git a/target/microblaze/cpu.c b/target/microblaze/cpu.c index 135947ee800..e9f98806274 100644 --- a/target/microblaze/cpu.c +++ b/target/microblaze/cpu.c @@ -368,7 +368,7 @@ static Property mb_properties[] = {       DEFINE_PROP_UINT8("use-non-secure", MicroBlazeCPU, cfg.use_non_secure, 0),       DEFINE_PROP_BOOL("dcache-writeback", MicroBlazeCPU, cfg.dcache_writeback,                        false), -    DEFINE_PROP_BOOL("endianness", MicroBlazeCPU, cfg.endi, false), +    DEFINE_PROP_BOOL("little-endian", MicroBlazeCPU, cfg.endi, false),       /* Enables bus exceptions on failed data accesses (load/stores).  */       DEFINE_PROP_BOOL("dopb-bus-exception", MicroBlazeCPU,                        cfg.dopb_bus_exception, false), --

OK if I squash the following?

Looks good! Thanks! Reviewed-by: Edgar E. Iglesias <edgar.iglesias@amd.com>

-- >8 -- diff --git a/target/microblaze/cpu.c b/target/microblaze/cpu.c index e9f98806274..b322f060777 100644 --- a/target/microblaze/cpu.c +++ b/target/microblaze/cpu.c @@ -328,9 +328,16 @@ static void mb_cpu_initfn(Object *obj) qdev_init_gpio_in_named(DEVICE(cpu), mb_cpu_ns_axi_dc, "ns_axi_dc", 1); qdev_init_gpio_in_named(DEVICE(cpu), mb_cpu_ns_axi_ic, "ns_axi_ic", 1); #endif + + /* Restricted 'endianness' property is equivalent of 'little-endian' */ + object_property_add_alias(obj, "little-endian", obj, "endianness"); }

static Property mb_properties[] = { + /* + * Following properties are used by Xilinx DTS conversion tool + * do not rename them. + */ DEFINE_PROP_UINT32("base-vectors", MicroBlazeCPU, cfg.base_vectors, 0), DEFINE_PROP_BOOL("use-stack-protection", MicroBlazeCPU, cfg.stackprot, false), @@ -368,7 +375,7 @@ static Property mb_properties[] = { DEFINE_PROP_UINT8("use-non-secure", MicroBlazeCPU, cfg.use_non_secure, 0), DEFINE_PROP_BOOL("dcache-writeback", MicroBlazeCPU, cfg.dcache_writeback, false), - DEFINE_PROP_BOOL("little-endian", MicroBlazeCPU, cfg.endi, false), + DEFINE_PROP_BOOL("endianness", MicroBlazeCPU, cfg.endi, false), /* Enables bus exceptions on failed data accesses (load/stores). */ DEFINE_PROP_BOOL("dopb-bus-exception", MicroBlazeCPU, cfg.dopb_bus_exception, false), @@ -387,6 +394,9 @@ static Property mb_properties[] = { DEFINE_PROP_UINT8("pvr", MicroBlazeCPU, cfg.pvr, C_PVR_FULL), DEFINE_PROP_UINT8("pvr-user1", MicroBlazeCPU, cfg.pvr_user1, 0), DEFINE_PROP_UINT32("pvr-user2", MicroBlazeCPU, cfg.pvr_user2, 0), + /* + * End of properties reserved by Xilinx DTS conversion tool. + */ DEFINE_PROP_END_OF_LIST(), };

---

On Tue, Nov 05, 2024 at 02:04:14PM +0100, Philippe Mathieu-Daudé wrote:

The petalogix-ml605 machine was explicitly added as little-endian only machine in commit 00914b7d970 ("microblaze: Add PetaLogix ml605 MMU little-endian ref design"). Mark the big-endian version as deprecated.

When the xlnx-zynqmp-pmu machine's CPU was added in commit 133d23b3ad1 ("xlnx-zynqmp-pmu: Add the CPU and memory"), its 'endianness' property was set to %true, thus wired in little endianness.

Both machine are included in the big-endian system binary, while their CPU is working in little-endian. Unlikely to work as it. Deprecate now as broken config so we can remove soon.

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@amd.com>

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- docs/about/deprecated.rst | 6 ++++++ configs/devices/microblaze-softmmu/default.mak | 2 -- configs/devices/microblazeel-softmmu/default.mak | 5 ++++- hw/microblaze/petalogix_ml605_mmu.c | 7 ++++++- hw/microblaze/xlnx-zynqmp-pmu.c | 8 ++++++-- 5 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index ff404d44f85..e1c8829e1a4 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -279,6 +279,12 @@ BMC and a witherspoon like OpenPOWER system. It was used for bring up of the AST2600 SoC in labs. It can be easily replaced by the ``rainier-bmc`` machine which is a real product.

+Big-Endian variants of MicroBlaze ``petalogix-ml605`` and ``xlnx-zynqmp-pmu`` machines (since 9.2) +'''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''' + +Both ``petalogix-ml605`` and ``xlnx-zynqmp-pmu`` were added for little endian +CPUs. Big endian support is not tested. + Backend options ---------------

diff --git a/configs/devices/microblaze-softmmu/default.mak b/configs/devices/microblaze-softmmu/default.mak index 583e3959bb7..78941064655 100644 --- a/configs/devices/microblaze-softmmu/default.mak +++ b/configs/devices/microblaze-softmmu/default.mak @@ -2,5 +2,3 @@

# Boards are selected by default, uncomment to keep out of the build. # CONFIG_PETALOGIX_S3ADSP1800=n -# CONFIG_PETALOGIX_ML605=n -# CONFIG_XLNX_ZYNQMP_PMU=n diff --git a/configs/devices/microblazeel-softmmu/default.mak b/configs/devices/microblazeel-softmmu/default.mak index 29f7f13816c..4c1086435bf 100644 --- a/configs/devices/microblazeel-softmmu/default.mak +++ b/configs/devices/microblazeel-softmmu/default.mak @@ -1,3 +1,6 @@ # Default configuration for microblazeel-softmmu

-include ../microblaze-softmmu/default.mak +# Boards are selected by default, uncomment to keep out of the build. +# CONFIG_PETALOGIX_S3ADSP1800=n +# CONFIG_PETALOGIX_ML605=n +# CONFIG_XLNX_ZYNQMP_PMU=n diff --git a/hw/microblaze/petalogix_ml605_mmu.c b/hw/microblaze/petalogix_ml605_mmu.c index df808ac323e..61e47d83988 100644 --- a/hw/microblaze/petalogix_ml605_mmu.c +++ b/hw/microblaze/petalogix_ml605_mmu.c @@ -213,7 +213,12 @@ petalogix_ml605_init(MachineState *machine)

static void petalogix_ml605_machine_init(MachineClass *mc) { - mc->desc = "PetaLogix linux refdesign for xilinx ml605 little endian"; +#if TARGET_BIG_ENDIAN + mc->desc = "PetaLogix linux refdesign for xilinx ml605 (big endian)"; + mc->deprecation_reason = "big endian support is not tested"; +#else + mc->desc = "PetaLogix linux refdesign for xilinx ml605 (little endian)"; +#endif mc->init = petalogix_ml605_init; }

diff --git a/hw/microblaze/xlnx-zynqmp-pmu.c b/hw/microblaze/xlnx-zynqmp-pmu.c index 43608c2dca4..567aad47bfc 100644 --- a/hw/microblaze/xlnx-zynqmp-pmu.c +++ b/hw/microblaze/xlnx-zynqmp-pmu.c @@ -181,9 +181,13 @@ static void xlnx_zynqmp_pmu_init(MachineState *machine)

static void xlnx_zynqmp_pmu_machine_init(MachineClass *mc) { - mc->desc = "Xilinx ZynqMP PMU machine"; +#if TARGET_BIG_ENDIAN + mc->desc = "Xilinx ZynqMP PMU machine (big endian)"; + mc->deprecation_reason = "big endian support is not tested"; +#else + mc->desc = "Xilinx ZynqMP PMU machine (little endian)"; +#endif mc->init = xlnx_zynqmp_pmu_init; }

DEFINE_MACHINE("xlnx-zynqmp-pmu", xlnx_zynqmp_pmu_machine_init) - -- 2.45.2

On Tue, Nov 05, 2024 at 02:04:15PM +0100, Philippe Mathieu-Daudé wrote:

By default the machine's CPU endianness is 'big' order ('little-endian' property set to %false).

This corresponds to the default when this machine was added; see commits 6a8b1ae2020 "microblaze: Add petalogix s3a1800dsp MMU linux ref-design." and 72b675caacf "microblaze: Hook into the build-system." which added:

[ "$target_cpu" = "microblaze" ] && target_bigendian=yes

Later commit 877fdc12b1a ("microblaze: Allow targeting little-endian mb") added little-endian support, forgetting to set the CPU endianness to little-endian. Not an issue since this property was never used, but we will use it soon, so explicit the endianness to get the expected behavior.

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@amd.com>

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- hw/microblaze/petalogix_s3adsp1800_mmu.c | 2 ++ 1 file changed, 2 insertions(+)

diff --git a/hw/microblaze/petalogix_s3adsp1800_mmu.c b/hw/microblaze/petalogix_s3adsp1800_mmu.c index dad46bd7f98..37e9a05a62a 100644 --- a/hw/microblaze/petalogix_s3adsp1800_mmu.c +++ b/hw/microblaze/petalogix_s3adsp1800_mmu.c @@ -71,6 +71,8 @@ petalogix_s3adsp1800_init(MachineState *machine)

cpu = MICROBLAZE_CPU(object_new(TYPE_MICROBLAZE_CPU)); object_property_set_str(OBJECT(cpu), "version", "7.10.d", &error_abort); + object_property_set_bool(OBJECT(cpu), "little-endian", + !TARGET_BIG_ENDIAN, &error_abort); qdev_realize(DEVICE(cpu), NULL, &error_abort);

/* Attach emulated BRAM through the LMB. */ -- 2.45.2

On Tue, Nov 05, 2024 at 02:04:16PM +0100, Philippe Mathieu-Daudé wrote:

The machine datasheet mentions the GPIO device as 'xps_gpio'. Rename it accordingly to easily find its documentation.

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@amd.com>

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- hw/microblaze/petalogix_s3adsp1800_mmu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/microblaze/petalogix_s3adsp1800_mmu.c b/hw/microblaze/petalogix_s3adsp1800_mmu.c index 37e9a05a62a..581b0411e29 100644 --- a/hw/microblaze/petalogix_s3adsp1800_mmu.c +++ b/hw/microblaze/petalogix_s3adsp1800_mmu.c @@ -124,7 +124,7 @@ petalogix_s3adsp1800_init(MachineState *machine) sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, ETHLITE_BASEADDR); sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, irq[ETHLITE_IRQ]);

- create_unimplemented_device("gpio", GPIO_BASEADDR, 0x10000); + create_unimplemented_device("xps_gpio", GPIO_BASEADDR, 0x10000);

microblaze_load_kernel(cpu, ddr_base, ram_size, machine->initrd_filename, -- 2.45.2

On Tue, Nov 05, 2024 at 02:04:17PM +0100, Philippe Mathieu-Daudé wrote:

Replace DEFINE_MACHINE() by DEFINE_TYPES(), converting the class_init() handler.

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@amd.com>

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- hw/microblaze/petalogix_s3adsp1800_mmu.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/hw/microblaze/petalogix_s3adsp1800_mmu.c b/hw/microblaze/petalogix_s3adsp1800_mmu.c index 581b0411e29..6c0f5c6c651 100644 --- a/hw/microblaze/petalogix_s3adsp1800_mmu.c +++ b/hw/microblaze/petalogix_s3adsp1800_mmu.c @@ -55,6 +55,9 @@ #define ETHLITE_IRQ 1 #define UARTLITE_IRQ 3

+#define TYPE_PETALOGIX_S3ADSP1800_MACHINE \ + MACHINE_TYPE_NAME("petalogix-s3adsp1800") + static void petalogix_s3adsp1800_init(MachineState *machine) { @@ -132,11 +135,21 @@ petalogix_s3adsp1800_init(MachineState *machine) NULL); }

-static void petalogix_s3adsp1800_machine_init(MachineClass *mc) +static void petalogix_s3adsp1800_machine_class_init(ObjectClass *oc, void *data) { + MachineClass *mc = MACHINE_CLASS(oc); + mc->desc = "PetaLogix linux refdesign for xilinx Spartan 3ADSP1800"; mc->init = petalogix_s3adsp1800_init; mc->is_default = true; }

-DEFINE_MACHINE("petalogix-s3adsp1800", petalogix_s3adsp1800_machine_init) +static const TypeInfo petalogix_s3adsp1800_machine_types[] = { + { + .name = TYPE_PETALOGIX_S3ADSP1800_MACHINE, + .parent = TYPE_MACHINE, + .class_init = petalogix_s3adsp1800_machine_class_init, + }, +}; + +DEFINE_TYPES(petalogix_s3adsp1800_machine_types) -- 2.45.2

On Tue, Nov 05, 2024 at 02:04:18PM +0100, Philippe Mathieu-Daudé wrote:

Fix few MemoryRegionOps style before adding new fields in the following commits.

Wasn't aware of this style rule :-) Reviewed-by: Edgar E. Iglesias <edgar.iglesias@amd.com>

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- hw/char/xilinx_uartlite.c | 4 ++-- hw/intc/xilinx_intc.c | 4 ++-- hw/net/xilinx_ethlite.c | 4 ++-- hw/timer/xilinx_timer.c | 4 ++-- 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/hw/char/xilinx_uartlite.c b/hw/char/xilinx_uartlite.c index f325084f8b9..a69ad769cc4 100644 --- a/hw/char/xilinx_uartlite.c +++ b/hw/char/xilinx_uartlite.c @@ -172,8 +172,8 @@ static const MemoryRegionOps uart_ops = { .endianness = DEVICE_NATIVE_ENDIAN, .valid = { .min_access_size = 1, - .max_access_size = 4 - } + .max_access_size = 4, + }, };

static Property xilinx_uartlite_properties[] = { diff --git a/hw/intc/xilinx_intc.c b/hw/intc/xilinx_intc.c index 6e5012e66eb..2b8246f6206 100644 --- a/hw/intc/xilinx_intc.c +++ b/hw/intc/xilinx_intc.c @@ -146,8 +146,8 @@ static const MemoryRegionOps pic_ops = { .endianness = DEVICE_NATIVE_ENDIAN, .valid = { .min_access_size = 4, - .max_access_size = 4 - } + .max_access_size = 4, + }, };

static void irq_handler(void *opaque, int irq, int level) diff --git a/hw/net/xilinx_ethlite.c b/hw/net/xilinx_ethlite.c index bd812908085..11eb53c4d60 100644 --- a/hw/net/xilinx_ethlite.c +++ b/hw/net/xilinx_ethlite.c @@ -172,8 +172,8 @@ static const MemoryRegionOps eth_ops = { .endianness = DEVICE_NATIVE_ENDIAN, .valid = { .min_access_size = 4, - .max_access_size = 4 - } + .max_access_size = 4, + }, };

static bool eth_can_rx(NetClientState *nc) diff --git a/hw/timer/xilinx_timer.c b/hw/timer/xilinx_timer.c index 32a9df69e0b..0822345779c 100644 --- a/hw/timer/xilinx_timer.c +++ b/hw/timer/xilinx_timer.c @@ -195,8 +195,8 @@ static const MemoryRegionOps timer_ops = { .endianness = DEVICE_NATIVE_ENDIAN, .valid = { .min_access_size = 4, - .max_access_size = 4 - } + .max_access_size = 4, + }, };

static void timer_hit(void *opaque) -- 2.45.2

On 5/11/24 23:24, Philippe Mathieu-Daudé wrote:

On 5/11/24 14:04, Philippe Mathieu-Daudé wrote:

...

All these MemoryRegionOps read() and write() handlers are implemented expecting 32-bit accesses. Clarify that setting .impl.min/max_access_size fields.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> ---   hw/char/xilinx_uartlite.c | 4 ++++   hw/intc/xilinx_intc.c     | 4 ++++   hw/net/xilinx_ethlite.c   | 4 ++++   hw/timer/xilinx_timer.c   | 4 ++++   4 files changed, 16 insertions(+)

diff --git a/hw/char/xilinx_uartlite.c b/hw/char/xilinx_uartlite.c index a69ad769cc4..892efe81fee 100644 --- a/hw/char/xilinx_uartlite.c +++ b/hw/char/xilinx_uartlite.c @@ -170,6 +170,10 @@ static const MemoryRegionOps uart_ops = {       .read = uart_read,       .write = uart_write,       .endianness = DEVICE_NATIVE_ENDIAN, +    .impl = { +        .min_access_size = 4,

Odd. The change makes the qtests pass, but here I'm modifying .impl, not .valid... Since .valid.min_access_size = 1, SBI is a valid opcode, no need to use SWI.

...

+        .max_access_size = 4, +    },       .valid = {           .min_access_size = 1,           .max_access_size = 4,

To have qtests working I need to squash:

-- >8 -- diff --git a/tests/qtest/boot-serial-test.c b/tests/qtest/boot-serial-test.c index 3b92fa5d506..6d9291c8ae2 100644 --- a/tests/qtest/boot-serial-test.c +++ b/tests/qtest/boot-serial-test.c @@ -57,7 +57,7 @@ static const uint8_t kernel_pls3adsp1800[] = {      0xb0, 0x00, 0x84, 0x00,                 /* imm   0x8400 */      0x30, 0x60, 0x00, 0x04,                 /* addik r3,r0,4 */      0x30, 0x80, 0x00, 0x54,                 /* addik r4,r0,'T' */ -    0xf0, 0x83, 0x00, 0x00,                 /* sbi   r4,r3,0 */ +    0xf8, 0x83, 0x00, 0x00,                 /* swi   r4,r3,0 */      0xb8, 0x00, 0xff, 0xfc                  /* bri   -4  loop */  };

@@ -65,7 +65,7 @@ static const uint8_t kernel_plml605[] = {      0xe0, 0x83, 0x00, 0xb0,                 /* imm   0x83e0 */      0x00, 0x10, 0x60, 0x30,                 /* addik r3,r0,0x1000 */      0x54, 0x00, 0x80, 0x30,                 /* addik r4,r0,'T' */ -    0x00, 0x00, 0x83, 0xf0,                 /* sbi   r4,r3,0 */ +    0x00, 0x00, 0x83, 0xf8,                 /* swi   r4,r3,0 */      0xfc, 0xff, 0x00, 0xb8                  /* bri   -4  loop */  }; ---

to access the uart by 32-bit instead of 8-bit.

On Tue, Nov 5, 2024 at 11:06 PM Philippe Mathieu-Daudé <philmd@linaro.org> wrote:

Pass vCPU endianness as argument so we can load kernels with different endianness (different from the qemu-system-binary builtin one).

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair

--- hw/microblaze/boot.h | 4 ++-- hw/microblaze/boot.c | 8 ++++---- hw/microblaze/petalogix_ml605_mmu.c | 2 +- hw/microblaze/petalogix_s3adsp1800_mmu.c | 2 +- hw/microblaze/xlnx-zynqmp-pmu.c | 2 +- 5 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/hw/microblaze/boot.h b/hw/microblaze/boot.h index 5a8c2f79750..d179a551a69 100644 --- a/hw/microblaze/boot.h +++ b/hw/microblaze/boot.h @@ -2,8 +2,8 @@ #define MICROBLAZE_BOOT_H

-void microblaze_load_kernel(MicroBlazeCPU *cpu, hwaddr ddr_base, - uint32_t ramsize, +void microblaze_load_kernel(MicroBlazeCPU *cpu, bool is_little_endian, + hwaddr ddr_base, uint32_t ramsize, const char *initrd_filename, const char *dtb_filename, void (*machine_cpu_reset)(MicroBlazeCPU *)); diff --git a/hw/microblaze/boot.c b/hw/microblaze/boot.c index ed61e483ee8..3675489fa5b 100644 --- a/hw/microblaze/boot.c +++ b/hw/microblaze/boot.c @@ -114,8 +114,8 @@ static uint64_t translate_kernel_address(void *opaque, uint64_t addr) return addr - 0x30000000LL; }

-void microblaze_load_kernel(MicroBlazeCPU *cpu, hwaddr ddr_base, - uint32_t ramsize, +void microblaze_load_kernel(MicroBlazeCPU *cpu, bool is_little_endian, + hwaddr ddr_base, uint32_t ramsize, const char *initrd_filename, const char *dtb_filename, void (*machine_cpu_reset)(MicroBlazeCPU *)) @@ -144,13 +144,13 @@ void microblaze_load_kernel(MicroBlazeCPU *cpu, hwaddr ddr_base, /* Boots a kernel elf binary. */ kernel_size = load_elf(kernel_filename, NULL, NULL, NULL, &entry, NULL, &high, NULL, - TARGET_BIG_ENDIAN, EM_MICROBLAZE, 0, 0); + !is_little_endian, EM_MICROBLAZE, 0, 0); base32 = entry; if (base32 == 0xc0000000) { kernel_size = load_elf(kernel_filename, NULL, translate_kernel_address, NULL, &entry, NULL, NULL, NULL, - TARGET_BIG_ENDIAN, EM_MICROBLAZE, 0, 0); + !is_little_endian, EM_MICROBLAZE, 0, 0); } /* Always boot into physical ram. */ boot_info.bootstrap_pc = (uint32_t)entry; diff --git a/hw/microblaze/petalogix_ml605_mmu.c b/hw/microblaze/petalogix_ml605_mmu.c index 61e47d83988..d2b2109065d 100644 --- a/hw/microblaze/petalogix_ml605_mmu.c +++ b/hw/microblaze/petalogix_ml605_mmu.c @@ -204,7 +204,7 @@ petalogix_ml605_init(MachineState *machine) cpu->cfg.pvr_regs[5] = 0xc56be000; cpu->cfg.pvr_regs[10] = 0x0e000000; /* virtex 6 */

- microblaze_load_kernel(cpu, MEMORY_BASEADDR, ram_size, + microblaze_load_kernel(cpu, true, MEMORY_BASEADDR, ram_size, machine->initrd_filename, BINARY_DEVICE_TREE_FILE, NULL); diff --git a/hw/microblaze/petalogix_s3adsp1800_mmu.c b/hw/microblaze/petalogix_s3adsp1800_mmu.c index 6c0f5c6c651..8110be83715 100644 --- a/hw/microblaze/petalogix_s3adsp1800_mmu.c +++ b/hw/microblaze/petalogix_s3adsp1800_mmu.c @@ -129,7 +129,7 @@ petalogix_s3adsp1800_init(MachineState *machine)

create_unimplemented_device("xps_gpio", GPIO_BASEADDR, 0x10000);

- microblaze_load_kernel(cpu, ddr_base, ram_size, + microblaze_load_kernel(cpu, !TARGET_BIG_ENDIAN, ddr_base, ram_size, machine->initrd_filename, BINARY_DEVICE_TREE_FILE, NULL); diff --git a/hw/microblaze/xlnx-zynqmp-pmu.c b/hw/microblaze/xlnx-zynqmp-pmu.c index 567aad47bfc..bdbf7328bf4 100644 --- a/hw/microblaze/xlnx-zynqmp-pmu.c +++ b/hw/microblaze/xlnx-zynqmp-pmu.c @@ -172,7 +172,7 @@ static void xlnx_zynqmp_pmu_init(MachineState *machine) qdev_realize(DEVICE(pmu), NULL, &error_fatal);

/* Load the kernel */ - microblaze_load_kernel(&pmu->cpu, XLNX_ZYNQMP_PMU_RAM_ADDR, + microblaze_load_kernel(&pmu->cpu, true, XLNX_ZYNQMP_PMU_RAM_ADDR, machine->ram_size, machine->initrd_filename, machine->dtb, -- 2.45.2

On Tue, Nov 5, 2024 at 11:08 PM Philippe Mathieu-Daudé <philmd@linaro.org> wrote:

Per the datasheet (reference added in file header, p.9) 'Programming Model' -> 'Register Data Types and Organization':

"The XPS INTC registers are read as big-endian data"

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Acked-by: Alistair Francis <alistair.francis@wdc.com> Alistair

--- hw/intc/xilinx_intc.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/hw/intc/xilinx_intc.c b/hw/intc/xilinx_intc.c index 1762b34564e..71f743a1f14 100644 --- a/hw/intc/xilinx_intc.c +++ b/hw/intc/xilinx_intc.c @@ -3,6 +3,9 @@ * * Copyright (c) 2009 Edgar E. Iglesias. * + * https://docs.amd.com/v/u/en-US/xps_intc + * DS572: LogiCORE IP XPS Interrupt Controller (v2.01a) + * * 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 @@ -143,12 +146,20 @@ static void pic_write(void *opaque, hwaddr addr, static const MemoryRegionOps pic_ops = { .read = pic_read, .write = pic_write, - .endianness = DEVICE_NATIVE_ENDIAN, + /* The XPS INTC registers are read as big-endian data. */ + .endianness = DEVICE_BIG_ENDIAN, .impl = { .min_access_size = 4, .max_access_size = 4, }, .valid = { + /* + * All XPS INTC registers are accessed through the PLB interface. + * The base address for these registers is provided by the + * configuration parameter, C_BASEADDR. Each register is 32 bits + * although some bits may be unused and is accessed on a 4-byte + * boundary offset from the base address. + */ .min_access_size = 4, .max_access_size = 4, }, -- 2.45.2

+Michal for Linux driver On 5/11/24 23:08, Edgar E. Iglesias wrote:

On Tue, Nov 05, 2024 at 02:04:21PM +0100, Philippe Mathieu-Daudé wrote:

...

Per the datasheet (reference added in file header, p.9) 'Programming Model' -> 'Register Data Types and Organization':

"The XPS INTC registers are read as big-endian data"

Hi Phil,

Some of these devices exist in both big and little endian versions. So far we've reused the same model by using DEVICE_NATIVE_ENDIAN.

Here's the little endian version: https://docs.amd.com/v/u/en-US/ds747_axi_intc

This model is specified as: - https://docs.amd.com/v/u/en-US/xps_intc LogiCORE IP XPS Interrupt Controller (v2.01a) DS572 April 19, 2010 Spec is from 2010, you added it in 2009: commit 17628bc642260df3a07b9df8b8a9ca7da2e7e87c Author: Edgar E. Iglesias <edgar.iglesias@gmail.com> Date: Wed May 20 20:11:30 2009 +0200 xilinx: Add interrupt controller. The spec is explicit: "The XPS INTC registers are read as big-endian data" The other model you mention is: - https://docs.amd.com/v/u/en-US/ds747_axi_intc LogiCORE IP AXI INTC (v1.04a) DS747 June 19, 2013 The spec is more recent than your addition, and contains the Interrupt Vector Address Register (IVAR) which is not present in our model. Indeed the latter seems to extend the former, but they are not the same and we need some work to model the latter. The endianness explicit for each model (and is not listed in the "IP Configurable Design Parameters" tables). That said, let's look at Linux use. Driver was added in: commit eedbdab99fffb8ed71cac75a722088b8ace2583c Author: Michal Simek <monstr@monstr.eu> Date: Fri Mar 27 14:25:49 2009 +0100 microblaze_v8: Interrupt handling and timer support Using explicit big-endian API: +void __init init_IRQ(void) +{ ... + /* + * Disable all external interrupts until they are + * explicity requested. + */ + out_be32(intc_baseaddr + IER, 0); + + /* Acknowledge any pending interrupts just in case. */ + out_be32(intc_baseaddr + IAR, 0xffffffff); + + /* Turn on the Master Enable. */ + out_be32(intc_baseaddr + MER, MER_HIE | MER_ME); Then the driver became clever in: commit 1aa1243c339d4c902c0f9c1ced45742729a86e6a Author: Michal Simek <michal.simek@amd.com> Date: Mon Feb 24 14:56:32 2014 +0100 microblaze: Make intc driver endian aware Detect endianess directly on the hardware and use ioread/iowrite functions. +static void intc_write32(u32 val, void __iomem *addr) +{ + iowrite32(val, addr); +} + +static void intc_write32_be(u32 val, void __iomem *addr) +{ + iowrite32be(val, addr); +} @@ -140,17 +163,25 @@ static int __init xilinx_intc_of_init(struct device_node *intc, + write_fn = intc_write32; + read_fn = intc_read32; + /* * Disable all external interrupts until they are * explicity requested. */ - out_be32(intc_baseaddr + IER, 0); + write_fn(0, intc_baseaddr + IER); /* Acknowledge any pending interrupts just in case. */ - out_be32(intc_baseaddr + IAR, 0xffffffff); + write_fn(0xffffffff, intc_baseaddr + IAR); /* Turn on the Master Enable. */ - out_be32(intc_baseaddr + MER, MER_HIE | MER_ME); + write_fn(MER_HIE | MER_ME, intc_baseaddr + MER); + if (!(read_fn(intc_baseaddr + MER) & (MER_HIE | MER_ME))) { + write_fn = intc_write32_be; + read_fn = intc_read32_be; + write_fn(MER_HIE | MER_ME, intc_baseaddr + MER); + } Interestingly little endianness became the default, although the driver detect it on init. This is from 2014, maybe to work with the "LogiCORE IP AXI INTC" you mentioned, which spec date is 2013. Indeed when forcing different endianness [*], the Linux kernel used in our tests (tests/functional/test_microblaze*) does the check and ends up using the correct INTC endianness: LE CPU, LE INTC model pic_write addr=8 val=0 pic_write addr=c val=ffffffff pic_write addr=1c val=3 <- LE pic_read 1c=3 pic_write addr=c val=1 pic_write addr=10 val=1 pic_read 18=0 LE CPU, BE INTC model pic_write addr=8 val=0 pic_write addr=c val=ffffffff pic_write addr=1c val=3000000 <- LE test pic_read 1c=0 pic_write addr=1c val=3 <- BE pic_write addr=c val=1 pic_write addr=10 val=1 pic_read 18=0 BE CPU, BE INTC model pic_write addr=8 val=0 pic_write addr=c val=ffffffff pic_write addr=1c val=3000000 <- LE test pic_read 1c=0 pic_write addr=1c val=3 <- BE pic_write addr=c val=1 pic_write addr=10 val=1 pic_read 18=0 BE CPU, LE INTC model pic_write addr=8 val=0 pic_write addr=c val=ffffffff pic_write addr=1c val=3 <- LE pic_read 1c=3 pic_write addr=c val=1 pic_write addr=10 val=1 pic_read 18=0 IMHO this patch behavior is correct. Besides, I don't expect firmwares to be as clever as Linux.

Can we have add property to select the endianess? For the Xilinx use-cases I think it may be a good idea to default it to little endian and have the big-endian machines explicitly set it.

What you suggested is implemented in v3: https://lore.kernel.org/qemu-devel/20241108154317.12129-4-philmd@linaro.org/ but after the analysis, I wonder if it isn't safer to not make the endianness configurable, but expose as 2 models: - xlnx,xps_intc (2010) in BE - xlnx,axi_intc (2013) in LE

Cheers, Edgar

...

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- hw/intc/xilinx_intc.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/hw/intc/xilinx_intc.c b/hw/intc/xilinx_intc.c index 1762b34564e..71f743a1f14 100644 --- a/hw/intc/xilinx_intc.c +++ b/hw/intc/xilinx_intc.c @@ -3,6 +3,9 @@ * * Copyright (c) 2009 Edgar E. Iglesias. * + * https://docs.amd.com/v/u/en-US/xps_intc + * DS572: LogiCORE IP XPS Interrupt Controller (v2.01a) + * * 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 @@ -143,12 +146,20 @@ static void pic_write(void *opaque, hwaddr addr, static const MemoryRegionOps pic_ops = { .read = pic_read, .write = pic_write, - .endianness = DEVICE_NATIVE_ENDIAN, + /* The XPS INTC registers are read as big-endian data. */ + .endianness = DEVICE_BIG_ENDIAN, .impl = { .min_access_size = 4, .max_access_size = 4, }, .valid = { + /* + * All XPS INTC registers are accessed through the PLB interface. + * The base address for these registers is provided by the + * configuration parameter, C_BASEADDR. Each register is 32 bits + * although some bits may be unused and is accessed on a 4-byte + * boundary offset from the base address. + */ .min_access_size = 4, .max_access_size = 4, }, -- 2.45.2

[*] diff used: -- >8 -- @@ -32,7 +45,7 @@ diff --git a/hw/intc/xilinx_intc.c b/hw/intc/xilinx_intc.c index fc55c8afca..883ec685f4 100644 --- a/hw/intc/xilinx_intc.c +++ b/hw/intc/xilinx_intc.c #include "hw/qdev-properties.h" #include "qom/object.h" -#define D(x) +#define D(x) x #define R_ISR 0 #define R_IPR 1 @@ -105,7 +122,7 @@ static uint64_t pic_read(void *opaque, hwaddr addr, unsigned int size) break; } - D(printf("%s %x=%x\n", __func__, addr * 4, r)); + D(printf("%s %llx=%x\n", __func__, addr * 4, r)); return r; } @@ -116,7 +133,7 @@ static void pic_write(void *opaque, hwaddr addr, uint32_t value = val64; addr >>= 2; - D(qemu_log("%s addr=%x val=%x\n", __func__, addr * 4, value)); + D(printf("%s addr=%llx val=%x\n", __func__, addr * 4, value)); switch (addr) { case R_IAR: ---

On Tue, Nov 05, 2024 at 02:04:22PM +0100, Philippe Mathieu-Daudé wrote:

Per the datasheet (reference added in file header, p.10): 'Register Data Types and Organization':

"The XPS Timer/Counter registers are organized as big-endian data."

Haven't checked but pretty sure this will break things the same way as for the intc. Cheers, Edgar

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- hw/timer/xilinx_timer.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/timer/xilinx_timer.c b/hw/timer/xilinx_timer.c index 28ac95edea1..3e272c8bb39 100644 --- a/hw/timer/xilinx_timer.c +++ b/hw/timer/xilinx_timer.c @@ -3,6 +3,9 @@ * * Copyright (c) 2009 Edgar E. Iglesias. * + * DS573: https://docs.amd.com/v/u/en-US/xps_timer + * LogiCORE IP XPS Timer/Counter (v1.02a) + * * 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 @@ -192,7 +195,7 @@ timer_write(void *opaque, hwaddr addr, static const MemoryRegionOps timer_ops = { .read = timer_read, .write = timer_write, - .endianness = DEVICE_NATIVE_ENDIAN, + .endianness = DEVICE_BIG_ENDIAN, .impl = { .min_access_size = 4, .max_access_size = 4, -- 2.45.2

On Tue, Nov 05, 2024 at 02:04:23PM +0100, Philippe Mathieu-Daudé wrote:

Allow down to 8-bit access, per the datasheet (reference added in previous commit):

"Timer Counter registers are accessed as one of the following types: • Byte (8 bits) • Half word (2 bytes) • Word (4 bytes)"

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@amd.com>

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- hw/timer/xilinx_timer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/timer/xilinx_timer.c b/hw/timer/xilinx_timer.c index 3e272c8bb39..e9498fc7eec 100644 --- a/hw/timer/xilinx_timer.c +++ b/hw/timer/xilinx_timer.c @@ -201,7 +201,7 @@ static const MemoryRegionOps timer_ops = { .max_access_size = 4, }, .valid = { - .min_access_size = 4, + .min_access_size = 1, .max_access_size = 4, }, }; -- 2.45.2

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@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),

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. r~

On Tue, Nov 5, 2024 at 11:07 PM Philippe Mathieu-Daudé <philmd@linaro.org> wrote:

Extract the implicit MO_TE definition in order to replace it by runtime variable in the next commit.

Mechanical change using:

$ for n in UW UL UQ UO SW SL SQ; do \ sed -i -e "s/MO_TE$n/MO_TE | MO_$n/" \ $(git grep -l MO_TE$n target/microblaze); \ done

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair

--- target/microblaze/translate.c | 36 +++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/target/microblaze/translate.c b/target/microblaze/translate.c index 4beaf69e76a..4c25b1e4383 100644 --- a/target/microblaze/translate.c +++ b/target/microblaze/translate.c @@ -779,13 +779,13 @@ static bool trans_lbui(DisasContext *dc, arg_typeb *arg) static bool trans_lhu(DisasContext *dc, arg_typea *arg) { TCGv addr = compute_ldst_addr_typea(dc, arg->ra, arg->rb); - return do_load(dc, arg->rd, addr, MO_TEUW, dc->mem_index, false); + return do_load(dc, arg->rd, addr, MO_TE | MO_UW, dc->mem_index, false); }

static bool trans_lhur(DisasContext *dc, arg_typea *arg) { TCGv addr = compute_ldst_addr_typea(dc, arg->ra, arg->rb); - return do_load(dc, arg->rd, addr, MO_TEUW, dc->mem_index, true); + return do_load(dc, arg->rd, addr, MO_TE | MO_UW, dc->mem_index, true); }

static bool trans_lhuea(DisasContext *dc, arg_typea *arg) @@ -797,26 +797,26 @@ static bool trans_lhuea(DisasContext *dc, arg_typea *arg) return true; #else TCGv addr = compute_ldst_addr_ea(dc, arg->ra, arg->rb); - return do_load(dc, arg->rd, addr, MO_TEUW, MMU_NOMMU_IDX, false); + return do_load(dc, arg->rd, addr, MO_TE | MO_UW, MMU_NOMMU_IDX, false); #endif }

static bool trans_lhui(DisasContext *dc, arg_typeb *arg) { TCGv addr = compute_ldst_addr_typeb(dc, arg->ra, arg->imm); - return do_load(dc, arg->rd, addr, MO_TEUW, dc->mem_index, false); + return do_load(dc, arg->rd, addr, MO_TE | MO_UW, dc->mem_index, false); }

static bool trans_lw(DisasContext *dc, arg_typea *arg) { TCGv addr = compute_ldst_addr_typea(dc, arg->ra, arg->rb); - return do_load(dc, arg->rd, addr, MO_TEUL, dc->mem_index, false); + return do_load(dc, arg->rd, addr, MO_TE | MO_UL, dc->mem_index, false); }

static bool trans_lwr(DisasContext *dc, arg_typea *arg) { TCGv addr = compute_ldst_addr_typea(dc, arg->ra, arg->rb); - return do_load(dc, arg->rd, addr, MO_TEUL, dc->mem_index, true); + return do_load(dc, arg->rd, addr, MO_TE | MO_UL, dc->mem_index, true); }

static bool trans_lwea(DisasContext *dc, arg_typea *arg) @@ -828,14 +828,14 @@ static bool trans_lwea(DisasContext *dc, arg_typea *arg) return true; #else TCGv addr = compute_ldst_addr_ea(dc, arg->ra, arg->rb); - return do_load(dc, arg->rd, addr, MO_TEUL, MMU_NOMMU_IDX, false); + return do_load(dc, arg->rd, addr, MO_TE | MO_UL, MMU_NOMMU_IDX, false); #endif }

static bool trans_lwi(DisasContext *dc, arg_typeb *arg) { TCGv addr = compute_ldst_addr_typeb(dc, arg->ra, arg->imm); - return do_load(dc, arg->rd, addr, MO_TEUL, dc->mem_index, false); + return do_load(dc, arg->rd, addr, MO_TE | MO_UL, dc->mem_index, false); }

static bool trans_lwx(DisasContext *dc, arg_typea *arg) @@ -845,7 +845,7 @@ static bool trans_lwx(DisasContext *dc, arg_typea *arg) /* lwx does not throw unaligned access errors, so force alignment */ tcg_gen_andi_tl(addr, addr, ~3);

- tcg_gen_qemu_ld_i32(cpu_res_val, addr, dc->mem_index, MO_TEUL); + tcg_gen_qemu_ld_i32(cpu_res_val, addr, dc->mem_index, MO_TE | MO_UL); tcg_gen_mov_tl(cpu_res_addr, addr);

if (arg->rd) { @@ -929,13 +929,13 @@ static bool trans_sbi(DisasContext *dc, arg_typeb *arg) static bool trans_sh(DisasContext *dc, arg_typea *arg) { TCGv addr = compute_ldst_addr_typea(dc, arg->ra, arg->rb); - return do_store(dc, arg->rd, addr, MO_TEUW, dc->mem_index, false); + return do_store(dc, arg->rd, addr, MO_TE | MO_UW, dc->mem_index, false); }

static bool trans_shr(DisasContext *dc, arg_typea *arg) { TCGv addr = compute_ldst_addr_typea(dc, arg->ra, arg->rb); - return do_store(dc, arg->rd, addr, MO_TEUW, dc->mem_index, true); + return do_store(dc, arg->rd, addr, MO_TE | MO_UW, dc->mem_index, true); }

static bool trans_shea(DisasContext *dc, arg_typea *arg) @@ -947,26 +947,26 @@ static bool trans_shea(DisasContext *dc, arg_typea *arg) return true; #else TCGv addr = compute_ldst_addr_ea(dc, arg->ra, arg->rb); - return do_store(dc, arg->rd, addr, MO_TEUW, MMU_NOMMU_IDX, false); + return do_store(dc, arg->rd, addr, MO_TE | MO_UW, MMU_NOMMU_IDX, false); #endif }

static bool trans_shi(DisasContext *dc, arg_typeb *arg) { TCGv addr = compute_ldst_addr_typeb(dc, arg->ra, arg->imm); - return do_store(dc, arg->rd, addr, MO_TEUW, dc->mem_index, false); + return do_store(dc, arg->rd, addr, MO_TE | MO_UW, dc->mem_index, false); }

static bool trans_sw(DisasContext *dc, arg_typea *arg) { TCGv addr = compute_ldst_addr_typea(dc, arg->ra, arg->rb); - return do_store(dc, arg->rd, addr, MO_TEUL, dc->mem_index, false); + return do_store(dc, arg->rd, addr, MO_TE | MO_UL, dc->mem_index, false); }

static bool trans_swr(DisasContext *dc, arg_typea *arg) { TCGv addr = compute_ldst_addr_typea(dc, arg->ra, arg->rb); - return do_store(dc, arg->rd, addr, MO_TEUL, dc->mem_index, true); + return do_store(dc, arg->rd, addr, MO_TE | MO_UL, dc->mem_index, true); }

static bool trans_swea(DisasContext *dc, arg_typea *arg) @@ -978,14 +978,14 @@ static bool trans_swea(DisasContext *dc, arg_typea *arg) return true; #else TCGv addr = compute_ldst_addr_ea(dc, arg->ra, arg->rb); - return do_store(dc, arg->rd, addr, MO_TEUL, MMU_NOMMU_IDX, false); + return do_store(dc, arg->rd, addr, MO_TE | MO_UL, MMU_NOMMU_IDX, false); #endif }

static bool trans_swi(DisasContext *dc, arg_typeb *arg) { TCGv addr = compute_ldst_addr_typeb(dc, arg->ra, arg->imm); - return do_store(dc, arg->rd, addr, MO_TEUL, dc->mem_index, false); + return do_store(dc, arg->rd, addr, MO_TE | MO_UL, dc->mem_index, false); }

static bool trans_swx(DisasContext *dc, arg_typea *arg) @@ -1014,7 +1014,7 @@ static bool trans_swx(DisasContext *dc, arg_typea *arg)

tcg_gen_atomic_cmpxchg_i32(tval, cpu_res_addr, cpu_res_val, reg_for_write(dc, arg->rd), - dc->mem_index, MO_TEUL); + dc->mem_index, MO_TE | MO_UL);

tcg_gen_brcond_i32(TCG_COND_NE, cpu_res_val, tval, swx_fail);

-- 2.45.2