[PATCH v6 00/11] hw/microblaze: Allow running cross-endian vCPUs

[PATCH v6 00/11] hw/microblaze: Allow running cross-endian vCPUs

[Philippe Mathieu-Daudé]

Since v5:
- Introduce QAPI EndianMode
- Update RISCV machine while rebasing
- Fixed INTC use on PPC (Thomas)
- Dropped patch adding more machines (Daniel)

Since v4 & v3:
- Addressed Thomas review comments

Since v2:
- Addressed Richard's review comments

Since v1:
- Make device endianness configurable (Edgar)
- Convert more Xilinx devices
- Avoid preprocessor #if (Richard)
- Add R-b tags

Philippe Mathieu-Daudé (11):
  hw/qdev-properties-system: Introduce EndianMode QAPI enum
  hw/intc/xilinx_intc: Make device endianness configurable
  hw/net/xilinx_ethlite: Make device endianness configurable
  hw/timer/xilinx_timer: Make device endianness configurable
  hw/char/xilinx_uartlite: Make device endianness configurable
  hw/ssi/xilinx_spi: Make device endianness configurable
  tests/functional: Avoid using www.qemu-advent-calendar.org URL
  tests/functional: Explicit endianness of microblaze assets
  tests/functional: Allow microblaze tests to take a machine name
    argument
  tests/functional: Remove sleep() kludges from microblaze tests
  tests/functional: Have microblaze tests inherit common parent class

 qapi/common.json                              | 16 +++++
 include/hw/qdev-properties-system.h           |  7 +++
 hw/char/xilinx_uartlite.c                     | 34 +++++++----
 hw/core/qdev-properties-system.c              | 11 ++++
 hw/intc/xilinx_intc.c                         | 60 ++++++++++++++-----
 hw/microblaze/petalogix_ml605_mmu.c           |  3 +
 hw/microblaze/petalogix_s3adsp1800_mmu.c      |  6 ++
 hw/net/xilinx_ethlite.c                       | 27 +++++++--
 hw/ppc/virtex_ml507.c                         |  2 +
 hw/riscv/microblaze-v-generic.c               |  5 ++
 hw/ssi/xilinx_spi.c                           | 32 +++++++---
 hw/timer/xilinx_timer.c                       | 43 +++++++++----
 .../functional/test_microblaze_s3adsp1800.py  | 35 +++++++++--
 .../test_microblazeel_s3adsp1800.py           | 32 ++--------
 14 files changed, 229 insertions(+), 84 deletions(-)

-- 
2.47.1

[PATCH v6 01/11] hw/qdev-properties-system: Introduce EndianMode QAPI enum

[Philippe Mathieu-Daudé]

Introduce the EndianMode type and the DEFINE_PROP_ENDIAN() macros.
Endianness can be BIG, LITTLE or unspecified (default).

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 qapi/common.json                    | 16 ++++++++++++++++
 include/hw/qdev-properties-system.h |  7 +++++++
 hw/core/qdev-properties-system.c    | 11 +++++++++++
 3 files changed, 34 insertions(+)

diff --git a/qapi/common.json b/qapi/common.json
index 6ffc7a37890..217feaaf683 100644
--- a/qapi/common.json
+++ b/qapi/common.json
@@ -212,3 +212,19 @@
 ##
 { 'struct': 'HumanReadableText',
   'data': { 'human-readable-text': 'str' } }
+
+##
+# @EndianMode:
+#
+# An enumeration of three options: little, big, and unspecified
+#
+# @little: Little endianness
+#
+# @big: Big endianness
+#
+# @unspecified: Endianness not specified
+#
+# Since: 10.0
+##
+{ 'enum': 'EndianMode',
+  'data': [ 'little', 'big', 'unspecified' ] }
diff --git a/include/hw/qdev-properties-system.h b/include/hw/qdev-properties-system.h
index 7ec37f6316c..ead4dfc2f02 100644
--- a/include/hw/qdev-properties-system.h
+++ b/include/hw/qdev-properties-system.h
@@ -30,6 +30,7 @@ extern const PropertyInfo qdev_prop_pcie_link_speed;
 extern const PropertyInfo qdev_prop_pcie_link_width;
 extern const PropertyInfo qdev_prop_cpus390entitlement;
 extern const PropertyInfo qdev_prop_iothread_vq_mapping_list;
+extern const PropertyInfo qdev_prop_endian_mode;
 
 #define DEFINE_PROP_PCI_DEVFN(_n, _s, _f, _d)                   \
     DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_pci_devfn, int32_t)
@@ -97,4 +98,10 @@ extern const PropertyInfo qdev_prop_iothread_vq_mapping_list;
     DEFINE_PROP(_name, _state, _field, qdev_prop_iothread_vq_mapping_list, \
                 IOThreadVirtQueueMappingList *)
 
+#define DEFINE_PROP_ENDIAN(_name, _state, _field, _default) \
+    DEFINE_PROP_UNSIGNED(_name, _state, _field, _default, \
+                         qdev_prop_endian_mode, EndianMode)
+#define DEFINE_PROP_ENDIAN_NODEFAULT(_name, _state, _field) \
+    DEFINE_PROP_ENDIAN(_name, _state, _field, ENDIAN_MODE_UNSPECIFIED)
+
 #endif
diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index a96675beb0d..89f954f569e 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -1283,3 +1283,14 @@ const PropertyInfo qdev_prop_iothread_vq_mapping_list = {
     .set = set_iothread_vq_mapping_list,
     .release = release_iothread_vq_mapping_list,
 };
+
+/* --- Endian modes */
+
+const PropertyInfo qdev_prop_endian_mode = {
+    .name = "EndianMode",
+    .description = "Endian mode, big/little/unspecified",
+    .enum_table = &EndianMode_lookup,
+    .get = qdev_propinfo_get_enum,
+    .set = qdev_propinfo_set_enum,
+    .set_default_value = qdev_propinfo_set_default_value_enum,
+};
-- 
2.47.1

[PATCH v6 02/11] hw/intc/xilinx_intc: Make device endianness configurable

[Philippe Mathieu-Daudé]

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 for each machine using the device.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/intc/xilinx_intc.c                    | 60 ++++++++++++++++++------
 hw/microblaze/petalogix_ml605_mmu.c      |  1 +
 hw/microblaze/petalogix_s3adsp1800_mmu.c |  3 ++
 hw/ppc/virtex_ml507.c                    |  1 +
 hw/riscv/microblaze-v-generic.c          |  1 +
 5 files changed, 52 insertions(+), 14 deletions(-)

diff --git a/hw/intc/xilinx_intc.c b/hw/intc/xilinx_intc.c
index 6930f83907a..523402b688c 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
@@ -23,10 +26,12 @@
  */
 
 #include "qemu/osdep.h"
+#include "qapi/error.h"
 #include "hw/sysbus.h"
 #include "qemu/module.h"
 #include "hw/irq.h"
 #include "hw/qdev-properties.h"
+#include "hw/qdev-properties-system.h"
 #include "qom/object.h"
 
 #define D(x)
@@ -49,6 +54,7 @@ struct XpsIntc
 {
     SysBusDevice parent_obj;
 
+    EndianMode model_endianness;
     MemoryRegion mmio;
     qemu_irq parent_irq;
 
@@ -140,18 +146,29 @@ static void pic_write(void *opaque, hwaddr addr,
     update_irq(p);
 }
 
-static const MemoryRegionOps pic_ops = {
-    .read = pic_read,
-    .write = pic_write,
-    .endianness = DEVICE_NATIVE_ENDIAN,
-    .impl = {
-        .min_access_size = 4,
-        .max_access_size = 4,
+static const MemoryRegionOps pic_ops[2] = {
+    [0 ... 1] = {
+        .read = pic_read,
+        .write = pic_write,
+        .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,
+        },
     },
-    .valid = {
-        .min_access_size = 4,
-        .max_access_size = 4
-    }
+    [ENDIAN_MODE_BIG].endianness = DEVICE_BIG_ENDIAN,
+    [ENDIAN_MODE_LITTLE].endianness = DEVICE_LITTLE_ENDIAN,
 };
 
 static void irq_handler(void *opaque, int irq, int level)
@@ -174,13 +191,27 @@ static void xilinx_intc_init(Object *obj)
 
     qdev_init_gpio_in(DEVICE(obj), irq_handler, 32);
     sysbus_init_irq(SYS_BUS_DEVICE(obj), &p->parent_irq);
-
-    memory_region_init_io(&p->mmio, obj, &pic_ops, p, "xlnx.xps-intc",
-                          R_MAX * 4);
     sysbus_init_mmio(SYS_BUS_DEVICE(obj), &p->mmio);
 }
 
+static void xilinx_intc_realize(DeviceState *dev, Error **errp)
+{
+    XpsIntc *p = XILINX_INTC(dev);
+
+    if (p->model_endianness == ENDIAN_MODE_UNSPECIFIED) {
+        error_setg(errp, TYPE_XILINX_INTC " property 'endianness'"
+                         " must be set to 'big' or 'little'");
+        return;
+    }
+
+    memory_region_init_io(&p->mmio, OBJECT(dev),
+                          &pic_ops[p->model_endianness],
+                          p, "xlnx.xps-intc",
+                          R_MAX * 4);
+}
+
 static const Property xilinx_intc_properties[] = {
+    DEFINE_PROP_ENDIAN_NODEFAULT("endianness", XpsIntc, model_endianness),
     DEFINE_PROP_UINT32("kind-of-intr", XpsIntc, c_kind_of_intr, 0),
 };
 
@@ -188,6 +219,7 @@ static void xilinx_intc_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
+    dc->realize = xilinx_intc_realize;
     device_class_set_props(dc, xilinx_intc_properties);
 }
 
diff --git a/hw/microblaze/petalogix_ml605_mmu.c b/hw/microblaze/petalogix_ml605_mmu.c
index 8b44be75a22..55398cc67d1 100644
--- a/hw/microblaze/petalogix_ml605_mmu.c
+++ b/hw/microblaze/petalogix_ml605_mmu.c
@@ -111,6 +111,7 @@ petalogix_ml605_init(MachineState *machine)
 
 
     dev = qdev_new("xlnx.xps-intc");
+    qdev_prop_set_enum(dev, "endianness", ENDIAN_MODE_LITTLE);
     qdev_prop_set_uint32(dev, "kind-of-intr", 1 << TIMER_IRQ);
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
     sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, INTC_BASEADDR);
diff --git a/hw/microblaze/petalogix_s3adsp1800_mmu.c b/hw/microblaze/petalogix_s3adsp1800_mmu.c
index 2c0d8c34cd2..15cabe11777 100644
--- a/hw/microblaze/petalogix_s3adsp1800_mmu.c
+++ b/hw/microblaze/petalogix_s3adsp1800_mmu.c
@@ -71,6 +71,8 @@ petalogix_s3adsp1800_init(MachineState *machine)
     MemoryRegion *phys_ram = g_new(MemoryRegion, 1);
     qemu_irq irq[32];
     MemoryRegion *sysmem = get_system_memory();
+    EndianMode endianness = TARGET_BIG_ENDIAN ? ENDIAN_MODE_BIG
+                                              : ENDIAN_MODE_LITTLE;
 
     cpu = MICROBLAZE_CPU(object_new(TYPE_MICROBLAZE_CPU));
     object_property_set_str(OBJECT(cpu), "version", "7.10.d", &error_abort);
@@ -95,6 +97,7 @@ petalogix_s3adsp1800_init(MachineState *machine)
                           64 * KiB, 1, 0x89, 0x18, 0x0000, 0x0, 1);
 
     dev = qdev_new("xlnx.xps-intc");
+    qdev_prop_set_enum(dev, "endianness", endianness);
     qdev_prop_set_uint32(dev, "kind-of-intr",
                          1 << ETHLITE_IRQ | 1 << UARTLITE_IRQ);
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
diff --git a/hw/ppc/virtex_ml507.c b/hw/ppc/virtex_ml507.c
index 23238119273..df8f9644829 100644
--- a/hw/ppc/virtex_ml507.c
+++ b/hw/ppc/virtex_ml507.c
@@ -217,6 +217,7 @@ static void virtex_init(MachineState *machine)
 
     cpu_irq = qdev_get_gpio_in(DEVICE(cpu), PPC40x_INPUT_INT);
     dev = qdev_new("xlnx.xps-intc");
+    qdev_prop_set_enum(dev, "endianness", ENDIAN_MODE_BIG);
     qdev_prop_set_uint32(dev, "kind-of-intr", 0);
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
     sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, INTC_BASEADDR);
diff --git a/hw/riscv/microblaze-v-generic.c b/hw/riscv/microblaze-v-generic.c
index 26788a1824a..ebdd461ae98 100644
--- a/hw/riscv/microblaze-v-generic.c
+++ b/hw/riscv/microblaze-v-generic.c
@@ -79,6 +79,7 @@ static void mb_v_generic_init(MachineState *machine)
     memory_region_add_subregion(sysmem, ddr_base, phys_ram);
 
     dev = qdev_new("xlnx.xps-intc");
+    qdev_prop_set_enum(dev, "endianness", ENDIAN_MODE_LITTLE);
     qdev_prop_set_uint32(dev, "kind-of-intr",
                          1 << UARTLITE_IRQ);
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
-- 
2.47.1

[PATCH v6 03/11] hw/net/xilinx_ethlite: Make device endianness configurable

[Philippe Mathieu-Daudé]

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>
---
 hw/microblaze/petalogix_s3adsp1800_mmu.c |  1 +
 hw/net/xilinx_ethlite.c                  | 27 ++++++++++++++++++------
 hw/riscv/microblaze-v-generic.c          |  1 +
 3 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/hw/microblaze/petalogix_s3adsp1800_mmu.c b/hw/microblaze/petalogix_s3adsp1800_mmu.c
index 15cabe11777..d419dc49a25 100644
--- a/hw/microblaze/petalogix_s3adsp1800_mmu.c
+++ b/hw/microblaze/petalogix_s3adsp1800_mmu.c
@@ -123,6 +123,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_enum(dev, "endianness", endianness);
     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 14bf2b2e17a..c1cb54a27af 100644
--- a/hw/net/xilinx_ethlite.c
+++ b/hw/net/xilinx_ethlite.c
@@ -34,6 +34,7 @@
 #include "hw/sysbus.h"
 #include "hw/irq.h"
 #include "hw/qdev-properties.h"
+#include "hw/qdev-properties-system.h"
 #include "hw/misc/unimp.h"
 #include "net/net.h"
 #include "trace.h"
@@ -85,6 +86,7 @@ struct XlnxXpsEthLite
 {
     SysBusDevice parent_obj;
 
+    EndianMode model_endianness;
     MemoryRegion container;
     qemu_irq irq;
     NICState *nic;
@@ -183,10 +185,10 @@ static void port_tx_write(void *opaque, hwaddr addr, uint64_t value,
     }
 }
 
-static const MemoryRegionOps eth_porttx_ops = {
+static const MemoryRegionOps eth_porttx_ops[2] = {
+    [0 ... 1] = {
         .read = port_tx_read,
         .write = port_tx_write,
-        .endianness = DEVICE_NATIVE_ENDIAN,
         .impl = {
             .min_access_size = 4,
             .max_access_size = 4,
@@ -195,6 +197,9 @@ static const MemoryRegionOps eth_porttx_ops = {
             .min_access_size = 4,
             .max_access_size = 4,
         },
+    },
+    [ENDIAN_MODE_BIG].endianness = DEVICE_BIG_ENDIAN,
+    [ENDIAN_MODE_LITTLE].endianness = DEVICE_LITTLE_ENDIAN,
 };
 
 static uint64_t port_rx_read(void *opaque, hwaddr addr, unsigned int size)
@@ -232,10 +237,10 @@ static void port_rx_write(void *opaque, hwaddr addr, uint64_t value,
     }
 }
 
-static const MemoryRegionOps eth_portrx_ops = {
+static const MemoryRegionOps eth_portrx_ops[2] = {
+    [0 ... 1] = {
         .read = port_rx_read,
         .write = port_rx_write,
-        .endianness = DEVICE_NATIVE_ENDIAN,
         .impl = {
             .min_access_size = 4,
             .max_access_size = 4,
@@ -244,6 +249,9 @@ static const MemoryRegionOps eth_portrx_ops = {
             .min_access_size = 4,
             .max_access_size = 4,
         },
+    },
+    [ENDIAN_MODE_BIG].endianness = DEVICE_BIG_ENDIAN,
+    [ENDIAN_MODE_LITTLE].endianness = DEVICE_LITTLE_ENDIAN,
 };
 
 static bool eth_can_rx(NetClientState *nc)
@@ -301,6 +309,12 @@ static void xilinx_ethlite_realize(DeviceState *dev, Error **errp)
 {
     XlnxXpsEthLite *s = XILINX_ETHLITE(dev);
 
+    if (s->model_endianness == ENDIAN_MODE_UNSPECIFIED) {
+        error_setg(errp, TYPE_XILINX_ETHLITE " property 'endianness'"
+                         " must be set to 'big' or 'little'");
+        return;
+    }
+
     memory_region_init(&s->container, OBJECT(dev),
                        "xlnx.xps-ethernetlite", 0x2000);
 
@@ -328,7 +342,7 @@ static void xilinx_ethlite_realize(DeviceState *dev, Error **errp)
                                BUFSZ_MAX, &error_abort);
         memory_region_add_subregion(&s->container, 0x0800 * i, &s->port[i].txbuf);
         memory_region_init_io(&s->port[i].txio, OBJECT(dev),
-                              &eth_porttx_ops, s,
+                              &eth_porttx_ops[s->model_endianness], s,
                               i ? "ethlite.tx[1]io" : "ethlite.tx[0]io",
                               4 * TX_MAX);
         memory_region_add_subregion(&s->container, i ? A_TX_BASE1 : A_TX_BASE0,
@@ -340,7 +354,7 @@ static void xilinx_ethlite_realize(DeviceState *dev, Error **errp)
         memory_region_add_subregion(&s->container, 0x1000 + 0x0800 * i,
                                     &s->port[i].rxbuf);
         memory_region_init_io(&s->port[i].rxio, OBJECT(dev),
-                              &eth_portrx_ops, s,
+                              &eth_portrx_ops[s->model_endianness], s,
                               i ? "ethlite.rx[1]io" : "ethlite.rx[0]io",
                               4 * RX_MAX);
         memory_region_add_subregion(&s->container, i ? A_RX_BASE1 : A_RX_BASE0,
@@ -363,6 +377,7 @@ static void xilinx_ethlite_init(Object *obj)
 }
 
 static const Property xilinx_ethlite_properties[] = {
+    DEFINE_PROP_ENDIAN_NODEFAULT("endianness", XlnxXpsEthLite, model_endianness),
     DEFINE_PROP_UINT32("tx-ping-pong", XlnxXpsEthLite, c_tx_pingpong, 1),
     DEFINE_PROP_UINT32("rx-ping-pong", XlnxXpsEthLite, c_rx_pingpong, 1),
     DEFINE_NIC_PROPERTIES(XlnxXpsEthLite, conf),
diff --git a/hw/riscv/microblaze-v-generic.c b/hw/riscv/microblaze-v-generic.c
index ebdd461ae98..a21fdfbe6db 100644
--- a/hw/riscv/microblaze-v-generic.c
+++ b/hw/riscv/microblaze-v-generic.c
@@ -120,6 +120,7 @@ static void mb_v_generic_init(MachineState *machine)
 
     /* Emaclite */
     dev = qdev_new("xlnx.xps-ethernetlite");
+    qdev_prop_set_enum(dev, "endianness", ENDIAN_MODE_LITTLE);
     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);
-- 
2.47.1

[PATCH v6 04/11] hw/timer/xilinx_timer: Make device endianness configurable

[Philippe Mathieu-Daudé]

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 for each machine using the device.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/microblaze/petalogix_ml605_mmu.c      |  1 +
 hw/microblaze/petalogix_s3adsp1800_mmu.c |  1 +
 hw/ppc/virtex_ml507.c                    |  1 +
 hw/riscv/microblaze-v-generic.c          |  2 ++
 hw/timer/xilinx_timer.c                  | 43 +++++++++++++++++-------
 5 files changed, 35 insertions(+), 13 deletions(-)

diff --git a/hw/microblaze/petalogix_ml605_mmu.c b/hw/microblaze/petalogix_ml605_mmu.c
index 55398cc67d1..490640e9428 100644
--- a/hw/microblaze/petalogix_ml605_mmu.c
+++ b/hw/microblaze/petalogix_ml605_mmu.c
@@ -127,6 +127,7 @@ petalogix_ml605_init(MachineState *machine)
 
     /* 2 timers at irq 2 @ 100 Mhz.  */
     dev = qdev_new("xlnx.xps-timer");
+    qdev_prop_set_enum(dev, "endianness", ENDIAN_MODE_LITTLE);
     qdev_prop_set_uint32(dev, "one-timer-only", 0);
     qdev_prop_set_uint32(dev, "clock-frequency", 100 * 1000000);
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
diff --git a/hw/microblaze/petalogix_s3adsp1800_mmu.c b/hw/microblaze/petalogix_s3adsp1800_mmu.c
index d419dc49a25..caaea222a8c 100644
--- a/hw/microblaze/petalogix_s3adsp1800_mmu.c
+++ b/hw/microblaze/petalogix_s3adsp1800_mmu.c
@@ -116,6 +116,7 @@ petalogix_s3adsp1800_init(MachineState *machine)
 
     /* 2 timers at irq 2 @ 62 Mhz.  */
     dev = qdev_new("xlnx.xps-timer");
+    qdev_prop_set_enum(dev, "endianness", endianness);
     qdev_prop_set_uint32(dev, "one-timer-only", 0);
     qdev_prop_set_uint32(dev, "clock-frequency", 62 * 1000000);
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
diff --git a/hw/ppc/virtex_ml507.c b/hw/ppc/virtex_ml507.c
index df8f9644829..a01354d991d 100644
--- a/hw/ppc/virtex_ml507.c
+++ b/hw/ppc/virtex_ml507.c
@@ -231,6 +231,7 @@ static void virtex_init(MachineState *machine)
 
     /* 2 timers at irq 2 @ 62 Mhz.  */
     dev = qdev_new("xlnx.xps-timer");
+    qdev_prop_set_enum(dev, "endianness", ENDIAN_MODE_BIG);
     qdev_prop_set_uint32(dev, "one-timer-only", 0);
     qdev_prop_set_uint32(dev, "clock-frequency", 62 * 1000000);
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
diff --git a/hw/riscv/microblaze-v-generic.c b/hw/riscv/microblaze-v-generic.c
index a21fdfbe6db..3c79f5733b2 100644
--- a/hw/riscv/microblaze-v-generic.c
+++ b/hw/riscv/microblaze-v-generic.c
@@ -104,6 +104,7 @@ static void mb_v_generic_init(MachineState *machine)
 
     /* 2 timers at irq 0 @ 100 Mhz.  */
     dev = qdev_new("xlnx.xps-timer");
+    qdev_prop_set_enum(dev, "endianness", ENDIAN_MODE_LITTLE);
     qdev_prop_set_uint32(dev, "one-timer-only", 0);
     qdev_prop_set_uint32(dev, "clock-frequency", 100000000);
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
@@ -112,6 +113,7 @@ static void mb_v_generic_init(MachineState *machine)
 
     /* 2 timers at irq 3 @ 100 Mhz.  */
     dev = qdev_new("xlnx.xps-timer");
+    qdev_prop_set_enum(dev, "endianness", ENDIAN_MODE_LITTLE);
     qdev_prop_set_uint32(dev, "one-timer-only", 0);
     qdev_prop_set_uint32(dev, "clock-frequency", 100000000);
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
diff --git a/hw/timer/xilinx_timer.c b/hw/timer/xilinx_timer.c
index 6595cf5f517..8cd44fd6925 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
@@ -23,10 +26,12 @@
  */
 
 #include "qemu/osdep.h"
+#include "qapi/error.h"
 #include "hw/sysbus.h"
 #include "hw/irq.h"
 #include "hw/ptimer.h"
 #include "hw/qdev-properties.h"
+#include "hw/qdev-properties-system.h"
 #include "qemu/log.h"
 #include "qemu/module.h"
 #include "qom/object.h"
@@ -69,6 +74,7 @@ struct XpsTimerState
 {
     SysBusDevice parent_obj;
 
+    EndianMode model_endianness;
     MemoryRegion mmio;
     qemu_irq irq;
     uint8_t one_timer_only;
@@ -189,18 +195,21 @@ timer_write(void *opaque, hwaddr addr,
     timer_update_irq(t);
 }
 
-static const MemoryRegionOps timer_ops = {
-    .read = timer_read,
-    .write = timer_write,
-    .endianness = DEVICE_NATIVE_ENDIAN,
-    .impl = {
-        .min_access_size = 4,
-        .max_access_size = 4,
+static const MemoryRegionOps timer_ops[2] = {
+    [0 ... 1] = {
+        .read = timer_read,
+        .write = timer_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
-    }
+    [ENDIAN_MODE_BIG].endianness = DEVICE_BIG_ENDIAN,
+    [ENDIAN_MODE_LITTLE].endianness = DEVICE_LITTLE_ENDIAN,
 };
 
 static void timer_hit(void *opaque)
@@ -220,6 +229,12 @@ static void xilinx_timer_realize(DeviceState *dev, Error **errp)
     XpsTimerState *t = XILINX_TIMER(dev);
     unsigned int i;
 
+    if (t->model_endianness == ENDIAN_MODE_UNSPECIFIED) {
+        error_setg(errp, TYPE_XILINX_TIMER " property 'endianness'"
+                         " must be set to 'big' or 'little'");
+        return;
+    }
+
     /* Init all the ptimers.  */
     t->timers = g_malloc0(sizeof t->timers[0] * num_timers(t));
     for (i = 0; i < num_timers(t); i++) {
@@ -233,8 +248,9 @@ static void xilinx_timer_realize(DeviceState *dev, Error **errp)
         ptimer_transaction_commit(xt->ptimer);
     }
 
-    memory_region_init_io(&t->mmio, OBJECT(t), &timer_ops, t, "xlnx.xps-timer",
-                          R_MAX * 4 * num_timers(t));
+    memory_region_init_io(&t->mmio, OBJECT(t),
+                          &timer_ops[t->model_endianness], t,
+                          "xlnx.xps-timer", R_MAX * 4 * num_timers(t));
     sysbus_init_mmio(SYS_BUS_DEVICE(dev), &t->mmio);
 }
 
@@ -247,6 +263,7 @@ static void xilinx_timer_init(Object *obj)
 }
 
 static const Property xilinx_timer_properties[] = {
+    DEFINE_PROP_ENDIAN_NODEFAULT("endianness", XpsTimerState, model_endianness),
     DEFINE_PROP_UINT32("clock-frequency", XpsTimerState, freq_hz, 62 * 1000000),
     DEFINE_PROP_UINT8("one-timer-only", XpsTimerState, one_timer_only, 0),
 };
-- 
2.47.1

[PATCH v6 05/11] hw/char/xilinx_uartlite: Make device endianness configurable

[Philippe Mathieu-Daudé]

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>
---
 hw/char/xilinx_uartlite.c                | 34 ++++++++++++++++--------
 hw/microblaze/petalogix_s3adsp1800_mmu.c |  1 +
 hw/riscv/microblaze-v-generic.c          |  1 +
 3 files changed, 25 insertions(+), 11 deletions(-)

diff --git a/hw/char/xilinx_uartlite.c b/hw/char/xilinx_uartlite.c
index 56955e0d74a..907fb33c6c0 100644
--- a/hw/char/xilinx_uartlite.c
+++ b/hw/char/xilinx_uartlite.c
@@ -24,6 +24,7 @@
 
 #include "qemu/osdep.h"
 #include "qemu/log.h"
+#include "qapi/error.h"
 #include "hw/char/xilinx_uartlite.h"
 #include "hw/irq.h"
 #include "hw/qdev-properties.h"
@@ -57,6 +58,7 @@
 struct XilinxUARTLite {
     SysBusDevice parent_obj;
 
+    EndianMode model_endianness;
     MemoryRegion mmio;
     CharBackend chr;
     qemu_irq irq;
@@ -166,17 +168,21 @@ uart_write(void *opaque, hwaddr addr,
     uart_update_irq(s);
 }
 
-static const MemoryRegionOps uart_ops = {
-    .read = uart_read,
-    .write = uart_write,
-    .endianness = DEVICE_NATIVE_ENDIAN,
-    .valid = {
-        .min_access_size = 1,
-        .max_access_size = 4
-    }
+static const MemoryRegionOps uart_ops[2] = {
+    [0 ... 1] = {
+        .read = uart_read,
+        .write = uart_write,
+        .valid = {
+            .min_access_size = 1,
+            .max_access_size = 4,
+        },
+    },
+    [ENDIAN_MODE_BIG].endianness = DEVICE_BIG_ENDIAN,
+    [ENDIAN_MODE_LITTLE].endianness = DEVICE_LITTLE_ENDIAN,
 };
 
 static const Property xilinx_uartlite_properties[] = {
+    DEFINE_PROP_ENDIAN_NODEFAULT("endianness", XilinxUARTLite, model_endianness),
     DEFINE_PROP_CHR("chardev", XilinxUARTLite, chr),
 };
 
@@ -214,6 +220,15 @@ static void xilinx_uartlite_realize(DeviceState *dev, Error **errp)
 {
     XilinxUARTLite *s = XILINX_UARTLITE(dev);
 
+    if (s->model_endianness == ENDIAN_MODE_UNSPECIFIED) {
+        error_setg(errp, TYPE_XILINX_UARTLITE " property 'endianness'"
+                         " must be set to 'big' or 'little'");
+        return;
+    }
+
+    memory_region_init_io(&s->mmio, OBJECT(dev),
+                          &uart_ops[s->model_endianness],
+                          s, "xlnx.xps-uartlite", R_MAX * 4);
     qemu_chr_fe_set_handlers(&s->chr, uart_can_rx, uart_rx,
                              uart_event, NULL, s, NULL, true);
 }
@@ -223,9 +238,6 @@ static void xilinx_uartlite_init(Object *obj)
     XilinxUARTLite *s = XILINX_UARTLITE(obj);
 
     sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->irq);
-
-    memory_region_init_io(&s->mmio, obj, &uart_ops, s,
-                          "xlnx.xps-uartlite", R_MAX * 4);
     sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mmio);
 }
 
diff --git a/hw/microblaze/petalogix_s3adsp1800_mmu.c b/hw/microblaze/petalogix_s3adsp1800_mmu.c
index caaea222a8c..bdba2006b72 100644
--- a/hw/microblaze/petalogix_s3adsp1800_mmu.c
+++ b/hw/microblaze/petalogix_s3adsp1800_mmu.c
@@ -109,6 +109,7 @@ petalogix_s3adsp1800_init(MachineState *machine)
     }
 
     dev = qdev_new(TYPE_XILINX_UARTLITE);
+    qdev_prop_set_enum(dev, "endianness", endianness);
     qdev_prop_set_chr(dev, "chardev", serial_hd(0));
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
     sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, UARTLITE_BASEADDR);
diff --git a/hw/riscv/microblaze-v-generic.c b/hw/riscv/microblaze-v-generic.c
index 3c79f5733b2..d8e67906d26 100644
--- a/hw/riscv/microblaze-v-generic.c
+++ b/hw/riscv/microblaze-v-generic.c
@@ -92,6 +92,7 @@ static void mb_v_generic_init(MachineState *machine)
 
     /* Uartlite */
     dev = qdev_new(TYPE_XILINX_UARTLITE);
+    qdev_prop_set_enum(dev, "endianness", ENDIAN_MODE_LITTLE);
     qdev_prop_set_chr(dev, "chardev", serial_hd(0));
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
     sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, UARTLITE_BASEADDR);
-- 
2.47.1

[PATCH v6 06/11] hw/ssi/xilinx_spi: Make device endianness configurable

[Philippe Mathieu-Daudé]

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>
---
 hw/microblaze/petalogix_ml605_mmu.c |  1 +
 hw/ssi/xilinx_spi.c                 | 32 +++++++++++++++++++++--------
 2 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/hw/microblaze/petalogix_ml605_mmu.c b/hw/microblaze/petalogix_ml605_mmu.c
index 490640e9428..b34edf13796 100644
--- a/hw/microblaze/petalogix_ml605_mmu.c
+++ b/hw/microblaze/petalogix_ml605_mmu.c
@@ -175,6 +175,7 @@ petalogix_ml605_init(MachineState *machine)
         SSIBus *spi;
 
         dev = qdev_new("xlnx.xps-spi");
+        qdev_prop_set_enum(dev, "endianness", ENDIAN_MODE_LITTLE);
         qdev_prop_set_uint8(dev, "num-ss-bits", NUM_SPI_FLASHES);
         busdev = SYS_BUS_DEVICE(dev);
         sysbus_realize_and_unref(busdev, &error_fatal);
diff --git a/hw/ssi/xilinx_spi.c b/hw/ssi/xilinx_spi.c
index fd1ff12eb1d..0cedfb526a0 100644
--- a/hw/ssi/xilinx_spi.c
+++ b/hw/ssi/xilinx_spi.c
@@ -25,6 +25,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qapi/error.h"
 #include "hw/sysbus.h"
 #include "migration/vmstate.h"
 #include "qemu/module.h"
@@ -32,6 +33,7 @@
 
 #include "hw/irq.h"
 #include "hw/qdev-properties.h"
+#include "hw/qdev-properties-system.h"
 #include "hw/ssi/ssi.h"
 #include "qom/object.h"
 
@@ -83,6 +85,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(XilinxSPI, XILINX_SPI)
 struct XilinxSPI {
     SysBusDevice parent_obj;
 
+    EndianMode model_endianness;
     MemoryRegion mmio;
 
     qemu_irq irq;
@@ -313,14 +316,17 @@ done:
     xlx_spi_update_irq(s);
 }
 
-static const MemoryRegionOps spi_ops = {
-    .read = spi_read,
-    .write = spi_write,
-    .endianness = DEVICE_NATIVE_ENDIAN,
-    .valid = {
-        .min_access_size = 4,
-        .max_access_size = 4
-    }
+static const MemoryRegionOps spi_ops[2] = {
+    [0 ... 1] = {
+        .read = spi_read,
+        .write = spi_write,
+        .valid = {
+            .min_access_size = 4,
+            .max_access_size = 4,
+        },
+    },
+    [ENDIAN_MODE_BIG].endianness = DEVICE_BIG_ENDIAN,
+    [ENDIAN_MODE_LITTLE].endianness = DEVICE_LITTLE_ENDIAN,
 };
 
 static void xilinx_spi_realize(DeviceState *dev, Error **errp)
@@ -329,6 +335,12 @@ static void xilinx_spi_realize(DeviceState *dev, Error **errp)
     XilinxSPI *s = XILINX_SPI(dev);
     int i;
 
+    if (s->model_endianness == ENDIAN_MODE_UNSPECIFIED) {
+        error_setg(errp, TYPE_XILINX_SPI " property 'endianness'"
+                         " must be set to 'big' or 'little'");
+        return;
+    }
+
     DB_PRINT("\n");
 
     s->spi = ssi_create_bus(dev, "spi");
@@ -339,7 +351,8 @@ static void xilinx_spi_realize(DeviceState *dev, Error **errp)
         sysbus_init_irq(sbd, &s->cs_lines[i]);
     }
 
-    memory_region_init_io(&s->mmio, OBJECT(s), &spi_ops, s,
+    memory_region_init_io(&s->mmio, OBJECT(s),
+                          &spi_ops[s->model_endianness], s,
                           "xilinx-spi", R_MAX * 4);
     sysbus_init_mmio(sbd, &s->mmio);
 
@@ -362,6 +375,7 @@ static const VMStateDescription vmstate_xilinx_spi = {
 };
 
 static const Property xilinx_spi_properties[] = {
+    DEFINE_PROP_ENDIAN_NODEFAULT("endianness", XilinxSPI, model_endianness),
     DEFINE_PROP_UINT8("num-ss-bits", XilinxSPI, num_cs, 1),
 };
 
-- 
2.47.1

[PATCH v6 07/11] tests/functional: Avoid using www.qemu-advent-calendar.org URL

[Philippe Mathieu-Daudé]

Avoid fetching assets from www.qemu-advent-calendar.org
website, prefer fetching microblaze assets from GitLab servers.

Suggested-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 tests/functional/test_microblazeel_s3adsp1800.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/functional/test_microblazeel_s3adsp1800.py b/tests/functional/test_microblazeel_s3adsp1800.py
index c382afe6bfa..5bf94d88dd8 100755
--- a/tests/functional/test_microblazeel_s3adsp1800.py
+++ b/tests/functional/test_microblazeel_s3adsp1800.py
@@ -18,7 +18,8 @@ class MicroblazeelMachine(QemuSystemTest):
     timeout = 90
 
     ASSET_IMAGE = Asset(
-        ('http://www.qemu-advent-calendar.org/2023/download/day13.tar.gz'),
+        ('https://qemu-advcal.gitlab.io/qac-best-of-multiarch/download/'
+         'day05.tar.xz'),
         'b9b3d43c5dd79db88ada495cc6e0d1f591153fe41355e925d791fbf44de50c22')
 
     def test_microblazeel_s3adsp1800(self):
-- 
2.47.1

[PATCH v6 08/11] tests/functional: Explicit endianness of microblaze assets

[Philippe Mathieu-Daudé]

The archive used in test_microblaze_s3adsp1800.py (testing a
big-endian target) contains a big-endian kernel. Rename using
the _BE suffix.

Similarly, the archive in test_microblazeel_s3adsp1800 (testing
a little-endian target) contains a little-endian kernel. Rename
using _LE suffix.

These changes will help when adding cross-endian kernel tests.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Message-Id: <20250206131052.30207-13-philmd@linaro.org>
---
 tests/functional/test_microblaze_s3adsp1800.py   | 6 +++---
 tests/functional/test_microblazeel_s3adsp1800.py | 6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/tests/functional/test_microblaze_s3adsp1800.py b/tests/functional/test_microblaze_s3adsp1800.py
index 2c4464bd05a..fac364b1ea9 100755
--- a/tests/functional/test_microblaze_s3adsp1800.py
+++ b/tests/functional/test_microblaze_s3adsp1800.py
@@ -15,14 +15,14 @@ class MicroblazeMachine(QemuSystemTest):
 
     timeout = 90
 
-    ASSET_IMAGE = Asset(
+    ASSET_IMAGE_BE = Asset(
         ('https://qemu-advcal.gitlab.io/qac-best-of-multiarch/download/'
          'day17.tar.xz'),
         '3ba7439dfbea7af4876662c97f8e1f0cdad9231fc166e4861d17042489270057')
 
-    def test_microblaze_s3adsp1800(self):
+    def test_microblaze_s3adsp1800_be(self):
         self.set_machine('petalogix-s3adsp1800')
-        self.archive_extract(self.ASSET_IMAGE)
+        self.archive_extract(self.ASSET_IMAGE_BE)
         self.vm.set_console()
         self.vm.add_args('-kernel',
                          self.scratch_file('day17', 'ballerina.bin'))
diff --git a/tests/functional/test_microblazeel_s3adsp1800.py b/tests/functional/test_microblazeel_s3adsp1800.py
index 5bf94d88dd8..ccebf5c2ce1 100755
--- a/tests/functional/test_microblazeel_s3adsp1800.py
+++ b/tests/functional/test_microblazeel_s3adsp1800.py
@@ -17,15 +17,15 @@ class MicroblazeelMachine(QemuSystemTest):
 
     timeout = 90
 
-    ASSET_IMAGE = Asset(
+    ASSET_IMAGE_LE = Asset(
         ('https://qemu-advcal.gitlab.io/qac-best-of-multiarch/download/'
          'day05.tar.xz'),
         'b9b3d43c5dd79db88ada495cc6e0d1f591153fe41355e925d791fbf44de50c22')
 
-    def test_microblazeel_s3adsp1800(self):
+    def test_microblazeel_s3adsp1800_le(self):
         self.require_netdev('user')
         self.set_machine('petalogix-s3adsp1800')
-        self.archive_extract(self.ASSET_IMAGE)
+        self.archive_extract(self.ASSET_IMAGE_LE)
         self.vm.set_console()
         self.vm.add_args('-kernel', self.scratch_file('day13', 'xmaton.bin'))
         tftproot = self.scratch_file('day13')
-- 
2.47.1

[PATCH v6 09/11] tests/functional: Allow microblaze tests to take a machine name argument

[Philippe Mathieu-Daudé]

Make microblaze tests a bit more generic.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Message-Id: <20250206131052.30207-14-philmd@linaro.org>
---
 tests/functional/test_microblaze_s3adsp1800.py   | 7 +++++--
 tests/functional/test_microblazeel_s3adsp1800.py | 7 +++++--
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/tests/functional/test_microblaze_s3adsp1800.py b/tests/functional/test_microblaze_s3adsp1800.py
index fac364b1ea9..c4226f49cf3 100755
--- a/tests/functional/test_microblaze_s3adsp1800.py
+++ b/tests/functional/test_microblaze_s3adsp1800.py
@@ -20,8 +20,8 @@ class MicroblazeMachine(QemuSystemTest):
          'day17.tar.xz'),
         '3ba7439dfbea7af4876662c97f8e1f0cdad9231fc166e4861d17042489270057')
 
-    def test_microblaze_s3adsp1800_be(self):
-        self.set_machine('petalogix-s3adsp1800')
+    def do_ballerina_be_test(self, machine):
+        self.set_machine(machine)
         self.archive_extract(self.ASSET_IMAGE_BE)
         self.vm.set_console()
         self.vm.add_args('-kernel',
@@ -34,5 +34,8 @@ def test_microblaze_s3adsp1800_be(self):
         # message, that's why we don't test for a later string here. This
         # needs some investigation by a microblaze wizard one day...
 
+    def test_microblaze_s3adsp1800_legacy_be(self):
+        self.do_ballerina_be_test('petalogix-s3adsp1800')
+
 if __name__ == '__main__':
     QemuSystemTest.main()
diff --git a/tests/functional/test_microblazeel_s3adsp1800.py b/tests/functional/test_microblazeel_s3adsp1800.py
index ccebf5c2ce1..1f0812eec63 100755
--- a/tests/functional/test_microblazeel_s3adsp1800.py
+++ b/tests/functional/test_microblazeel_s3adsp1800.py
@@ -22,9 +22,9 @@ class MicroblazeelMachine(QemuSystemTest):
          'day05.tar.xz'),
         'b9b3d43c5dd79db88ada495cc6e0d1f591153fe41355e925d791fbf44de50c22')
 
-    def test_microblazeel_s3adsp1800_le(self):
+    def do_xmaton_le_test(self, machine):
         self.require_netdev('user')
-        self.set_machine('petalogix-s3adsp1800')
+        self.set_machine(machine)
         self.archive_extract(self.ASSET_IMAGE_LE)
         self.vm.set_console()
         self.vm.add_args('-kernel', self.scratch_file('day13', 'xmaton.bin'))
@@ -39,5 +39,8 @@ def test_microblazeel_s3adsp1800_le(self):
                 'tftp -g -r xmaton.png 10.0.2.2 ; md5sum xmaton.png',
                 '821cd3cab8efd16ad6ee5acc3642a8ea')
 
+    def test_microblaze_s3adsp1800_legacy_le(self):
+        self.do_xmaton_le_test('petalogix-s3adsp1800')
+
 if __name__ == '__main__':
     QemuSystemTest.main()
-- 
2.47.1

[PATCH v6 10/11] tests/functional: Remove sleep() kludges from microblaze tests

[Philippe Mathieu-Daudé]

Commit f0ec14c78c4 ("tests/avocado: Fix console data loss") fixed
QEMUMachine's problem with console, we don't need to use the sleep()
kludges.

Suggested-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Message-Id: <20250206131052.30207-15-philmd@linaro.org>
---
 tests/functional/test_microblazeel_s3adsp1800.py | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/tests/functional/test_microblazeel_s3adsp1800.py b/tests/functional/test_microblazeel_s3adsp1800.py
index 1f0812eec63..d50b98342d7 100755
--- a/tests/functional/test_microblazeel_s3adsp1800.py
+++ b/tests/functional/test_microblazeel_s3adsp1800.py
@@ -7,8 +7,7 @@
 # This work is licensed under the terms of the GNU GPL, version 2 or
 # later. See the COPYING file in the top-level directory.
 
-import time
-from qemu_test import exec_command, exec_command_and_wait_for_pattern
+from qemu_test import exec_command_and_wait_for_pattern
 from qemu_test import QemuSystemTest, Asset
 from qemu_test import wait_for_console_pattern
 
@@ -32,9 +31,8 @@ def do_xmaton_le_test(self, machine):
         self.vm.add_args('-nic', f'user,tftp={tftproot}')
         self.vm.launch()
         wait_for_console_pattern(self, 'QEMU Advent Calendar 2023')
-        time.sleep(0.1)
-        exec_command(self, 'root')
-        time.sleep(0.1)
+        wait_for_console_pattern(self, 'buildroot login:')
+        exec_command_and_wait_for_pattern(self, 'root', '#')
         exec_command_and_wait_for_pattern(self,
                 'tftp -g -r xmaton.png 10.0.2.2 ; md5sum xmaton.png',
                 '821cd3cab8efd16ad6ee5acc3642a8ea')
-- 
2.47.1

[PATCH v6 11/11] tests/functional: Have microblaze tests inherit common parent class

[Philippe Mathieu-Daudé]

Have the MicroblazeMachine class being common to both
MicroblazeBigEndianMachine and MicroblazeLittleEndianMachine
classes. Move the xmaton and ballerina tests to the parent class.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Message-Id: <20250206131052.30207-16-philmd@linaro.org>
---
 .../functional/test_microblaze_s3adsp1800.py  | 24 +++++++++++++++
 .../test_microblazeel_s3adsp1800.py           | 30 ++-----------------
 2 files changed, 27 insertions(+), 27 deletions(-)

diff --git a/tests/functional/test_microblaze_s3adsp1800.py b/tests/functional/test_microblaze_s3adsp1800.py
index c4226f49cf3..650416e0c09 100755
--- a/tests/functional/test_microblaze_s3adsp1800.py
+++ b/tests/functional/test_microblaze_s3adsp1800.py
@@ -7,6 +7,7 @@
 # This work is licensed under the terms of the GNU GPL, version 2 or
 # later. See the COPYING file in the top-level directory.
 
+from qemu_test import exec_command_and_wait_for_pattern
 from qemu_test import QemuSystemTest, Asset
 from qemu_test import wait_for_console_pattern
 
@@ -20,6 +21,11 @@ class MicroblazeMachine(QemuSystemTest):
          'day17.tar.xz'),
         '3ba7439dfbea7af4876662c97f8e1f0cdad9231fc166e4861d17042489270057')
 
+    ASSET_IMAGE_LE = Asset(
+        ('https://qemu-advcal.gitlab.io/qac-best-of-multiarch/download/'
+         'day05.tar.xz'),
+        'b9b3d43c5dd79db88ada495cc6e0d1f591153fe41355e925d791fbf44de50c22')
+
     def do_ballerina_be_test(self, machine):
         self.set_machine(machine)
         self.archive_extract(self.ASSET_IMAGE_BE)
@@ -34,6 +40,24 @@ def do_ballerina_be_test(self, machine):
         # message, that's why we don't test for a later string here. This
         # needs some investigation by a microblaze wizard one day...
 
+    def do_xmaton_le_test(self, machine):
+        self.require_netdev('user')
+        self.set_machine(machine)
+        self.archive_extract(self.ASSET_IMAGE_LE)
+        self.vm.set_console()
+        self.vm.add_args('-kernel', self.scratch_file('day13', 'xmaton.bin'))
+        tftproot = self.scratch_file('day13')
+        self.vm.add_args('-nic', f'user,tftp={tftproot}')
+        self.vm.launch()
+        wait_for_console_pattern(self, 'QEMU Advent Calendar 2023')
+        wait_for_console_pattern(self, 'buildroot login:')
+        exec_command_and_wait_for_pattern(self, 'root', '#')
+        exec_command_and_wait_for_pattern(self,
+                'tftp -g -r xmaton.png 10.0.2.2 ; md5sum xmaton.png',
+                '821cd3cab8efd16ad6ee5acc3642a8ea')
+
+class MicroblazeBigEndianMachine(MicroblazeMachine):
+
     def test_microblaze_s3adsp1800_legacy_be(self):
         self.do_ballerina_be_test('petalogix-s3adsp1800')
 
diff --git a/tests/functional/test_microblazeel_s3adsp1800.py b/tests/functional/test_microblazeel_s3adsp1800.py
index d50b98342d7..56645bd0bb2 100755
--- a/tests/functional/test_microblazeel_s3adsp1800.py
+++ b/tests/functional/test_microblazeel_s3adsp1800.py
@@ -7,35 +7,11 @@
 # This work is licensed under the terms of the GNU GPL, version 2 or
 # later. See the COPYING file in the top-level directory.
 
-from qemu_test import exec_command_and_wait_for_pattern
-from qemu_test import QemuSystemTest, Asset
-from qemu_test import wait_for_console_pattern
+from qemu_test import QemuSystemTest
 
+from test_microblaze_s3adsp1800 import MicroblazeMachine
 
-class MicroblazeelMachine(QemuSystemTest):
-
-    timeout = 90
-
-    ASSET_IMAGE_LE = Asset(
-        ('https://qemu-advcal.gitlab.io/qac-best-of-multiarch/download/'
-         'day05.tar.xz'),
-        'b9b3d43c5dd79db88ada495cc6e0d1f591153fe41355e925d791fbf44de50c22')
-
-    def do_xmaton_le_test(self, machine):
-        self.require_netdev('user')
-        self.set_machine(machine)
-        self.archive_extract(self.ASSET_IMAGE_LE)
-        self.vm.set_console()
-        self.vm.add_args('-kernel', self.scratch_file('day13', 'xmaton.bin'))
-        tftproot = self.scratch_file('day13')
-        self.vm.add_args('-nic', f'user,tftp={tftproot}')
-        self.vm.launch()
-        wait_for_console_pattern(self, 'QEMU Advent Calendar 2023')
-        wait_for_console_pattern(self, 'buildroot login:')
-        exec_command_and_wait_for_pattern(self, 'root', '#')
-        exec_command_and_wait_for_pattern(self,
-                'tftp -g -r xmaton.png 10.0.2.2 ; md5sum xmaton.png',
-                '821cd3cab8efd16ad6ee5acc3642a8ea')
+class MicroblazeLittleEndianMachine(MicroblazeMachine):
 
     def test_microblaze_s3adsp1800_legacy_le(self):
         self.do_xmaton_le_test('petalogix-s3adsp1800')
-- 
2.47.1

Re: [PATCH v6 01/11] hw/qdev-properties-system: Introduce EndianMode QAPI enum

[Thomas Huth]

On 12/02/2025 12.24, Philippe Mathieu-Daudé wrote:
> Introduce the EndianMode type and the DEFINE_PROP_ENDIAN() macros.
> Endianness can be BIG, LITTLE or unspecified (default).
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   qapi/common.json                    | 16 ++++++++++++++++
>   include/hw/qdev-properties-system.h |  7 +++++++
>   hw/core/qdev-properties-system.c    | 11 +++++++++++
>   3 files changed, 34 insertions(+)
> 
> diff --git a/qapi/common.json b/qapi/common.json
> index 6ffc7a37890..217feaaf683 100644
> --- a/qapi/common.json
> +++ b/qapi/common.json
> @@ -212,3 +212,19 @@
>   ##
>   { 'struct': 'HumanReadableText',
>     'data': { 'human-readable-text': 'str' } }
> +
> +##
> +# @EndianMode:
> +#
> +# An enumeration of three options: little, big, and unspecified
> +#
> +# @little: Little endianness
> +#
> +# @big: Big endianness
> +#
> +# @unspecified: Endianness not specified
> +#
> +# Since: 10.0
> +##
> +{ 'enum': 'EndianMode',
> +  'data': [ 'little', 'big', 'unspecified' ] }

Should 'unspecified' come first? ... so that it gets the value 0, i.e. when 
someone forgets to properly initialize a related variable, the chances are 
higher that it ends up as "unspecified" than as "little" ?

Apart from that:
Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v6 02/11] hw/intc/xilinx_intc: Make device endianness configurable

[Thomas Huth]

On 12/02/2025 12.24, Philippe Mathieu-Daudé wrote:
> 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 for each machine using the device.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/intc/xilinx_intc.c                    | 60 ++++++++++++++++++------
>   hw/microblaze/petalogix_ml605_mmu.c      |  1 +
>   hw/microblaze/petalogix_s3adsp1800_mmu.c |  3 ++
>   hw/ppc/virtex_ml507.c                    |  1 +
>   hw/riscv/microblaze-v-generic.c          |  1 +
>   5 files changed, 52 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/intc/xilinx_intc.c b/hw/intc/xilinx_intc.c
> index 6930f83907a..523402b688c 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
> @@ -23,10 +26,12 @@
>    */
>   
>   #include "qemu/osdep.h"
> +#include "qapi/error.h"
>   #include "hw/sysbus.h"
>   #include "qemu/module.h"
>   #include "hw/irq.h"
>   #include "hw/qdev-properties.h"
> +#include "hw/qdev-properties-system.h"
>   #include "qom/object.h"
>   
>   #define D(x)
> @@ -49,6 +54,7 @@ struct XpsIntc
>   {
>       SysBusDevice parent_obj;
>   
> +    EndianMode model_endianness;
>       MemoryRegion mmio;
>       qemu_irq parent_irq;
>   
> @@ -140,18 +146,29 @@ static void pic_write(void *opaque, hwaddr addr,
>       update_irq(p);
>   }
>   
> -static const MemoryRegionOps pic_ops = {
> -    .read = pic_read,
> -    .write = pic_write,
> -    .endianness = DEVICE_NATIVE_ENDIAN,
> -    .impl = {
> -        .min_access_size = 4,
> -        .max_access_size = 4,
> +static const MemoryRegionOps pic_ops[2] = {
> +    [0 ... 1] = {

Hmm, ok, so here we have now an assumption that ENDIAN_MODE_BIG and 
ENDIAN_MODE_LITTLE match 0 and 1, which would not work anymore when using 0 
as unspecified... a little bit ugly ... so maybe instead of changing pic_ops 
into an array ....

> +        .read = pic_read,
> +        .write = pic_write,
> +        .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,
> +        },
>       },
> -    .valid = {
> -        .min_access_size = 4,
> -        .max_access_size = 4
> -    }
> +    [ENDIAN_MODE_BIG].endianness = DEVICE_BIG_ENDIAN,
> +    [ENDIAN_MODE_LITTLE].endianness = DEVICE_LITTLE_ENDIAN,
>   };
>   
>   static void irq_handler(void *opaque, int irq, int level)
> @@ -174,13 +191,27 @@ static void xilinx_intc_init(Object *obj)
>   
>       qdev_init_gpio_in(DEVICE(obj), irq_handler, 32);
>       sysbus_init_irq(SYS_BUS_DEVICE(obj), &p->parent_irq);
> -
> -    memory_region_init_io(&p->mmio, obj, &pic_ops, p, "xlnx.xps-intc",
> -                          R_MAX * 4);
>       sysbus_init_mmio(SYS_BUS_DEVICE(obj), &p->mmio);
>   }
>   
> +static void xilinx_intc_realize(DeviceState *dev, Error **errp)
> +{
> +    XpsIntc *p = XILINX_INTC(dev);
> +
> +    if (p->model_endianness == ENDIAN_MODE_UNSPECIFIED) {
> +        error_setg(errp, TYPE_XILINX_INTC " property 'endianness'"
> +                         " must be set to 'big' or 'little'");
> +        return;
> +    }

... would it be possible to patch in the right value for pic_ops.endianness
here instead?

  Thomas

> +    memory_region_init_io(&p->mmio, OBJECT(dev),
> +                          &pic_ops[p->model_endianness],
> +                          p, "xlnx.xps-intc",
> +                          R_MAX * 4);
> +}
> +
>   static const Property xilinx_intc_properties[] = {
> +    DEFINE_PROP_ENDIAN_NODEFAULT("endianness", XpsIntc, model_endianness),
>       DEFINE_PROP_UINT32("kind-of-intr", XpsIntc, c_kind_of_intr, 0),
>   };
>   
> @@ -188,6 +219,7 @@ static void xilinx_intc_class_init(ObjectClass *klass, void *data)
>   {
>       DeviceClass *dc = DEVICE_CLASS(klass);
>   
> +    dc->realize = xilinx_intc_realize;
>       device_class_set_props(dc, xilinx_intc_properties);
>   }
>   
> diff --git a/hw/microblaze/petalogix_ml605_mmu.c b/hw/microblaze/petalogix_ml605_mmu.c
> index 8b44be75a22..55398cc67d1 100644
> --- a/hw/microblaze/petalogix_ml605_mmu.c
> +++ b/hw/microblaze/petalogix_ml605_mmu.c
> @@ -111,6 +111,7 @@ petalogix_ml605_init(MachineState *machine)
>   
>   
>       dev = qdev_new("xlnx.xps-intc");
> +    qdev_prop_set_enum(dev, "endianness", ENDIAN_MODE_LITTLE);
>       qdev_prop_set_uint32(dev, "kind-of-intr", 1 << TIMER_IRQ);
>       sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>       sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, INTC_BASEADDR);
> diff --git a/hw/microblaze/petalogix_s3adsp1800_mmu.c b/hw/microblaze/petalogix_s3adsp1800_mmu.c
> index 2c0d8c34cd2..15cabe11777 100644
> --- a/hw/microblaze/petalogix_s3adsp1800_mmu.c
> +++ b/hw/microblaze/petalogix_s3adsp1800_mmu.c
> @@ -71,6 +71,8 @@ petalogix_s3adsp1800_init(MachineState *machine)
>       MemoryRegion *phys_ram = g_new(MemoryRegion, 1);
>       qemu_irq irq[32];
>       MemoryRegion *sysmem = get_system_memory();
> +    EndianMode endianness = TARGET_BIG_ENDIAN ? ENDIAN_MODE_BIG
> +                                              : ENDIAN_MODE_LITTLE;
>   
>       cpu = MICROBLAZE_CPU(object_new(TYPE_MICROBLAZE_CPU));
>       object_property_set_str(OBJECT(cpu), "version", "7.10.d", &error_abort);
> @@ -95,6 +97,7 @@ petalogix_s3adsp1800_init(MachineState *machine)
>                             64 * KiB, 1, 0x89, 0x18, 0x0000, 0x0, 1);
>   
>       dev = qdev_new("xlnx.xps-intc");
> +    qdev_prop_set_enum(dev, "endianness", endianness);
>       qdev_prop_set_uint32(dev, "kind-of-intr",
>                            1 << ETHLITE_IRQ | 1 << UARTLITE_IRQ);
>       sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> diff --git a/hw/ppc/virtex_ml507.c b/hw/ppc/virtex_ml507.c
> index 23238119273..df8f9644829 100644
> --- a/hw/ppc/virtex_ml507.c
> +++ b/hw/ppc/virtex_ml507.c
> @@ -217,6 +217,7 @@ static void virtex_init(MachineState *machine)
>   
>       cpu_irq = qdev_get_gpio_in(DEVICE(cpu), PPC40x_INPUT_INT);
>       dev = qdev_new("xlnx.xps-intc");
> +    qdev_prop_set_enum(dev, "endianness", ENDIAN_MODE_BIG);
>       qdev_prop_set_uint32(dev, "kind-of-intr", 0);
>       sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>       sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, INTC_BASEADDR);
> diff --git a/hw/riscv/microblaze-v-generic.c b/hw/riscv/microblaze-v-generic.c
> index 26788a1824a..ebdd461ae98 100644
> --- a/hw/riscv/microblaze-v-generic.c
> +++ b/hw/riscv/microblaze-v-generic.c
> @@ -79,6 +79,7 @@ static void mb_v_generic_init(MachineState *machine)
>       memory_region_add_subregion(sysmem, ddr_base, phys_ram);
>   
>       dev = qdev_new("xlnx.xps-intc");
> +    qdev_prop_set_enum(dev, "endianness", ENDIAN_MODE_LITTLE);
>       qdev_prop_set_uint32(dev, "kind-of-intr",
>                            1 << UARTLITE_IRQ);
>       sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);

Re: [PATCH v6 01/11] hw/qdev-properties-system: Introduce EndianMode QAPI enum

[Philippe Mathieu-Daudé]

On 12/2/25 12:37, Thomas Huth wrote:
> On 12/02/2025 12.24, Philippe Mathieu-Daudé wrote:
>> Introduce the EndianMode type and the DEFINE_PROP_ENDIAN() macros.
>> Endianness can be BIG, LITTLE or unspecified (default).
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   qapi/common.json                    | 16 ++++++++++++++++
>>   include/hw/qdev-properties-system.h |  7 +++++++
>>   hw/core/qdev-properties-system.c    | 11 +++++++++++
>>   3 files changed, 34 insertions(+)
>>
>> diff --git a/qapi/common.json b/qapi/common.json
>> index 6ffc7a37890..217feaaf683 100644
>> --- a/qapi/common.json
>> +++ b/qapi/common.json
>> @@ -212,3 +212,19 @@
>>   ##
>>   { 'struct': 'HumanReadableText',
>>     'data': { 'human-readable-text': 'str' } }
>> +
>> +##
>> +# @EndianMode:
>> +#
>> +# An enumeration of three options: little, big, and unspecified
>> +#
>> +# @little: Little endianness
>> +#
>> +# @big: Big endianness
>> +#
>> +# @unspecified: Endianness not specified
>> +#
>> +# Since: 10.0
>> +##
>> +{ 'enum': 'EndianMode',
>> +  'data': [ 'little', 'big', 'unspecified' ] }
> 
> Should 'unspecified' come first? ... so that it gets the value 0, i.e. 
> when someone forgets to properly initialize a related variable, the 
> chances are higher that it ends up as "unspecified" than as "little" ?

Hmm but then in this series the dual-endianness regions are defined as:

+static const MemoryRegionOps pic_ops[2] = {
+    [0 ... 1] = {
+        .read = pic_read,
+        .write = pic_write,
+        .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,
+        },
      },
-    .valid = {
-        .min_access_size = 4,
-        .max_access_size = 4
-    }
+    [ENDIAN_MODE_BIG].endianness = DEVICE_BIG_ENDIAN,
+    [ENDIAN_MODE_LITTLE].endianness = DEVICE_LITTLE_ENDIAN,
  };

We could declare the array using the confusing __MAX definition
(at the price of wasting the ENDIAN_MODE_UNSPECIFIED entry) as:

static const MemoryRegionOps pic_ops[ENDIAN_MODE__MAX - 1] { }

WDYT?

> Apart from that:
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 

Thanks!

Re: [PATCH v6 02/11] hw/intc/xilinx_intc: Make device endianness configurable

[Philippe Mathieu-Daudé]

On 12/2/25 12:42, Thomas Huth wrote:
> On 12/02/2025 12.24, Philippe Mathieu-Daudé wrote:
>> 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 for each machine using the device.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/intc/xilinx_intc.c                    | 60 ++++++++++++++++++------
>>   hw/microblaze/petalogix_ml605_mmu.c      |  1 +
>>   hw/microblaze/petalogix_s3adsp1800_mmu.c |  3 ++
>>   hw/ppc/virtex_ml507.c                    |  1 +
>>   hw/riscv/microblaze-v-generic.c          |  1 +
>>   5 files changed, 52 insertions(+), 14 deletions(-)
>>
>> diff --git a/hw/intc/xilinx_intc.c b/hw/intc/xilinx_intc.c
>> index 6930f83907a..523402b688c 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
>> @@ -23,10 +26,12 @@
>>    */
>>   #include "qemu/osdep.h"
>> +#include "qapi/error.h"
>>   #include "hw/sysbus.h"
>>   #include "qemu/module.h"
>>   #include "hw/irq.h"
>>   #include "hw/qdev-properties.h"
>> +#include "hw/qdev-properties-system.h"
>>   #include "qom/object.h"
>>   #define D(x)
>> @@ -49,6 +54,7 @@ struct XpsIntc
>>   {
>>       SysBusDevice parent_obj;
>> +    EndianMode model_endianness;
>>       MemoryRegion mmio;
>>       qemu_irq parent_irq;
>> @@ -140,18 +146,29 @@ static void pic_write(void *opaque, hwaddr addr,
>>       update_irq(p);
>>   }
>> -static const MemoryRegionOps pic_ops = {
>> -    .read = pic_read,
>> -    .write = pic_write,
>> -    .endianness = DEVICE_NATIVE_ENDIAN,
>> -    .impl = {
>> -        .min_access_size = 4,
>> -        .max_access_size = 4,
>> +static const MemoryRegionOps pic_ops[2] = {
>> +    [0 ... 1] = {
> 
> Hmm, ok, so here we have now an assumption that ENDIAN_MODE_BIG and 
> ENDIAN_MODE_LITTLE match 0 and 1, which would not work anymore when 
> using 0 as unspecified... a little bit ugly ... so maybe instead of 
> changing pic_ops into an array ....
> 
>> +        .read = pic_read,
>> +        .write = pic_write,
>> +        .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,
>> +        },
>>       },
>> -    .valid = {
>> -        .min_access_size = 4,
>> -        .max_access_size = 4
>> -    }
>> +    [ENDIAN_MODE_BIG].endianness = DEVICE_BIG_ENDIAN,
>> +    [ENDIAN_MODE_LITTLE].endianness = DEVICE_LITTLE_ENDIAN,
>>   };
>>   static void irq_handler(void *opaque, int irq, int level)
>> @@ -174,13 +191,27 @@ static void xilinx_intc_init(Object *obj)
>>       qdev_init_gpio_in(DEVICE(obj), irq_handler, 32);
>>       sysbus_init_irq(SYS_BUS_DEVICE(obj), &p->parent_irq);
>> -
>> -    memory_region_init_io(&p->mmio, obj, &pic_ops, p, "xlnx.xps-intc",
>> -                          R_MAX * 4);
>>       sysbus_init_mmio(SYS_BUS_DEVICE(obj), &p->mmio);
>>   }
>> +static void xilinx_intc_realize(DeviceState *dev, Error **errp)
>> +{
>> +    XpsIntc *p = XILINX_INTC(dev);
>> +
>> +    if (p->model_endianness == ENDIAN_MODE_UNSPECIFIED) {
>> +        error_setg(errp, TYPE_XILINX_INTC " property 'endianness'"
>> +                         " must be set to 'big' or 'little'");
>> +        return;
>> +    }
> 
> ... would it be possible to patch in the right value for pic_ops.endianness
> here instead?

Ah, clever than my reply on the previous patch. I'll give it a try, thanks!

> 
>   Thomas

Re: [PATCH v6 11/11] tests/functional: Have microblaze tests inherit common parent class

[Thomas Huth]

On 12/02/2025 12.24, Philippe Mathieu-Daudé wrote:
> Have the MicroblazeMachine class being common to both
> MicroblazeBigEndianMachine and MicroblazeLittleEndianMachine
> classes. Move the xmaton and ballerina tests to the parent class.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> Message-Id: <20250206131052.30207-16-philmd@linaro.org>
> ---
>   .../functional/test_microblaze_s3adsp1800.py  | 24 +++++++++++++++
>   .../test_microblazeel_s3adsp1800.py           | 30 ++-----------------
>   2 files changed, 27 insertions(+), 27 deletions(-)
> 
> diff --git a/tests/functional/test_microblaze_s3adsp1800.py b/tests/functional/test_microblaze_s3adsp1800.py
> index c4226f49cf3..650416e0c09 100755
> --- a/tests/functional/test_microblaze_s3adsp1800.py
> +++ b/tests/functional/test_microblaze_s3adsp1800.py
> @@ -7,6 +7,7 @@
>   # This work is licensed under the terms of the GNU GPL, version 2 or
>   # later. See the COPYING file in the top-level directory.
>   
> +from qemu_test import exec_command_and_wait_for_pattern
>   from qemu_test import QemuSystemTest, Asset
>   from qemu_test import wait_for_console_pattern
>   
> @@ -20,6 +21,11 @@ class MicroblazeMachine(QemuSystemTest):
>            'day17.tar.xz'),
>           '3ba7439dfbea7af4876662c97f8e1f0cdad9231fc166e4861d17042489270057')
>   
> +    ASSET_IMAGE_LE = Asset(
> +        ('https://qemu-advcal.gitlab.io/qac-best-of-multiarch/download/'
> +         'day05.tar.xz'),
> +        'b9b3d43c5dd79db88ada495cc6e0d1f591153fe41355e925d791fbf44de50c22')
> +
>       def do_ballerina_be_test(self, machine):
>           self.set_machine(machine)
>           self.archive_extract(self.ASSET_IMAGE_BE)
> @@ -34,6 +40,24 @@ def do_ballerina_be_test(self, machine):
>           # message, that's why we don't test for a later string here. This
>           # needs some investigation by a microblaze wizard one day...
>   
> +    def do_xmaton_le_test(self, machine):
> +        self.require_netdev('user')
> +        self.set_machine(machine)
> +        self.archive_extract(self.ASSET_IMAGE_LE)
> +        self.vm.set_console()
> +        self.vm.add_args('-kernel', self.scratch_file('day13', 'xmaton.bin'))
> +        tftproot = self.scratch_file('day13')
> +        self.vm.add_args('-nic', f'user,tftp={tftproot}')
> +        self.vm.launch()
> +        wait_for_console_pattern(self, 'QEMU Advent Calendar 2023')
> +        wait_for_console_pattern(self, 'buildroot login:')
> +        exec_command_and_wait_for_pattern(self, 'root', '#')
> +        exec_command_and_wait_for_pattern(self,
> +                'tftp -g -r xmaton.png 10.0.2.2 ; md5sum xmaton.png',
> +                '821cd3cab8efd16ad6ee5acc3642a8ea')
> +
> +class MicroblazeBigEndianMachine(MicroblazeMachine):

Add this here 'til the problem with the precaching is fixed:

     ASSET_IMAGE_BE = MicroblazeMachine.ASSET_IMAGE_BE

?

>       def test_microblaze_s3adsp1800_legacy_be(self):
>           self.do_ballerina_be_test('petalogix-s3adsp1800')
>   
> diff --git a/tests/functional/test_microblazeel_s3adsp1800.py b/tests/functional/test_microblazeel_s3adsp1800.py
> index d50b98342d7..56645bd0bb2 100755
> --- a/tests/functional/test_microblazeel_s3adsp1800.py
> +++ b/tests/functional/test_microblazeel_s3adsp1800.py
> @@ -7,35 +7,11 @@
>   # This work is licensed under the terms of the GNU GPL, version 2 or
>   # later. See the COPYING file in the top-level directory.
>   
> -from qemu_test import exec_command_and_wait_for_pattern
> -from qemu_test import QemuSystemTest, Asset
> -from qemu_test import wait_for_console_pattern
> +from qemu_test import QemuSystemTest
>   
> +from test_microblaze_s3adsp1800 import MicroblazeMachine
>   
> -class MicroblazeelMachine(QemuSystemTest):
> -
> -    timeout = 90
> -
> -    ASSET_IMAGE_LE = Asset(
> -        ('https://qemu-advcal.gitlab.io/qac-best-of-multiarch/download/'
> -         'day05.tar.xz'),
> -        'b9b3d43c5dd79db88ada495cc6e0d1f591153fe41355e925d791fbf44de50c22')
> -
> -    def do_xmaton_le_test(self, machine):
> -        self.require_netdev('user')
> -        self.set_machine(machine)
> -        self.archive_extract(self.ASSET_IMAGE_LE)
> -        self.vm.set_console()
> -        self.vm.add_args('-kernel', self.scratch_file('day13', 'xmaton.bin'))
> -        tftproot = self.scratch_file('day13')
> -        self.vm.add_args('-nic', f'user,tftp={tftproot}')
> -        self.vm.launch()
> -        wait_for_console_pattern(self, 'QEMU Advent Calendar 2023')
> -        wait_for_console_pattern(self, 'buildroot login:')
> -        exec_command_and_wait_for_pattern(self, 'root', '#')
> -        exec_command_and_wait_for_pattern(self,
> -                'tftp -g -r xmaton.png 10.0.2.2 ; md5sum xmaton.png',
> -                '821cd3cab8efd16ad6ee5acc3642a8ea')
> +class MicroblazeLittleEndianMachine(MicroblazeMachine):

And add this here:

     ASSET_IMAGE_LE = MicroblazeMachine.ASSET_IMAGE_LE

?

  Thomas

>       def test_microblaze_s3adsp1800_legacy_le(self):
>           self.do_xmaton_le_test('petalogix-s3adsp1800')

Re: [PATCH v6 07/11] tests/functional: Avoid using www.qemu-advent-calendar.org URL

[Thomas Huth]

On 12/02/2025 12.24, Philippe Mathieu-Daudé wrote:
> Avoid fetching assets from www.qemu-advent-calendar.org
> website, prefer fetching microblaze assets from GitLab servers.
> 
> Suggested-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   tests/functional/test_microblazeel_s3adsp1800.py | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/functional/test_microblazeel_s3adsp1800.py b/tests/functional/test_microblazeel_s3adsp1800.py
> index c382afe6bfa..5bf94d88dd8 100755
> --- a/tests/functional/test_microblazeel_s3adsp1800.py
> +++ b/tests/functional/test_microblazeel_s3adsp1800.py
> @@ -18,7 +18,8 @@ class MicroblazeelMachine(QemuSystemTest):
>       timeout = 90
>   
>       ASSET_IMAGE = Asset(
> -        ('http://www.qemu-advent-calendar.org/2023/download/day13.tar.gz'),
> +        ('https://qemu-advcal.gitlab.io/qac-best-of-multiarch/download/'
> +         'day05.tar.xz'),
>           'b9b3d43c5dd79db88ada495cc6e0d1f591153fe41355e925d791fbf44de50c22')

You'd likely need to adjust the sha256 sum as well since I repacked with xz 
instead of gz ... but according to Eldon, the original download should be 
working again, so I'd suggest to drop this patch for now.

  Thomas

Re: [PATCH v6 07/11] tests/functional: Avoid using www.qemu-advent-calendar.org URL

[Philippe Mathieu-Daudé]

On 12/2/25 12:49, Thomas Huth wrote:
> On 12/02/2025 12.24, Philippe Mathieu-Daudé wrote:
>> Avoid fetching assets from www.qemu-advent-calendar.org
>> website, prefer fetching microblaze assets from GitLab servers.
>>
>> Suggested-by: Thomas Huth <thuth@redhat.com>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   tests/functional/test_microblazeel_s3adsp1800.py | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/functional/test_microblazeel_s3adsp1800.py b/tests/ 
>> functional/test_microblazeel_s3adsp1800.py
>> index c382afe6bfa..5bf94d88dd8 100755
>> --- a/tests/functional/test_microblazeel_s3adsp1800.py
>> +++ b/tests/functional/test_microblazeel_s3adsp1800.py
>> @@ -18,7 +18,8 @@ class MicroblazeelMachine(QemuSystemTest):
>>       timeout = 90
>>       ASSET_IMAGE = Asset(
>> -        ('http://www.qemu-advent-calendar.org/2023/download/ 
>> day13.tar.gz'),
>> +        ('https://qemu-advcal.gitlab.io/qac-best-of-multiarch/download/'
>> +         'day05.tar.xz'),
>>           
>> 'b9b3d43c5dd79db88ada495cc6e0d1f591153fe41355e925d791fbf44de50c22')
> 
> You'd likely need to adjust the sha256 sum as well since I repacked with 
> xz instead of gz ... but according to Eldon, the original download 
> should be working again, so I'd suggest to drop this patch for now.

Indeed, I forgot to flush my cache before running my tests, and now
figured it out:

9d031aa55fe988fddffab26932552c53dc9adeb75f30d04ece8abce02b226179 
day05.tar.xz

Re: [PATCH v6 11/11] tests/functional: Have microblaze tests inherit common parent class

[Philippe Mathieu-Daudé]

On 12/2/25 12:46, Thomas Huth wrote:
> On 12/02/2025 12.24, Philippe Mathieu-Daudé wrote:
>> Have the MicroblazeMachine class being common to both
>> MicroblazeBigEndianMachine and MicroblazeLittleEndianMachine
>> classes. Move the xmaton and ballerina tests to the parent class.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>> Message-Id: <20250206131052.30207-16-philmd@linaro.org>
>> ---
>>   .../functional/test_microblaze_s3adsp1800.py  | 24 +++++++++++++++
>>   .../test_microblazeel_s3adsp1800.py           | 30 ++-----------------
>>   2 files changed, 27 insertions(+), 27 deletions(-)
>>
>> diff --git a/tests/functional/test_microblaze_s3adsp1800.py b/tests/ 
>> functional/test_microblaze_s3adsp1800.py
>> index c4226f49cf3..650416e0c09 100755
>> --- a/tests/functional/test_microblaze_s3adsp1800.py
>> +++ b/tests/functional/test_microblaze_s3adsp1800.py
>> @@ -7,6 +7,7 @@
>>   # This work is licensed under the terms of the GNU GPL, version 2 or
>>   # later. See the COPYING file in the top-level directory.
>> +from qemu_test import exec_command_and_wait_for_pattern
>>   from qemu_test import QemuSystemTest, Asset
>>   from qemu_test import wait_for_console_pattern
>> @@ -20,6 +21,11 @@ class MicroblazeMachine(QemuSystemTest):
>>            'day17.tar.xz'),
>>           
>> '3ba7439dfbea7af4876662c97f8e1f0cdad9231fc166e4861d17042489270057')
>> +    ASSET_IMAGE_LE = Asset(
>> +        ('https://qemu-advcal.gitlab.io/qac-best-of-multiarch/download/'
>> +         'day05.tar.xz'),
>> +        
>> 'b9b3d43c5dd79db88ada495cc6e0d1f591153fe41355e925d791fbf44de50c22')
>> +
>>       def do_ballerina_be_test(self, machine):
>>           self.set_machine(machine)
>>           self.archive_extract(self.ASSET_IMAGE_BE)
>> @@ -34,6 +40,24 @@ def do_ballerina_be_test(self, machine):
>>           # message, that's why we don't test for a later string here. 
>> This
>>           # needs some investigation by a microblaze wizard one day...
>> +    def do_xmaton_le_test(self, machine):
>> +        self.require_netdev('user')
>> +        self.set_machine(machine)
>> +        self.archive_extract(self.ASSET_IMAGE_LE)
>> +        self.vm.set_console()
>> +        self.vm.add_args('-kernel', self.scratch_file('day13', 
>> 'xmaton.bin'))
>> +        tftproot = self.scratch_file('day13')
>> +        self.vm.add_args('-nic', f'user,tftp={tftproot}')
>> +        self.vm.launch()
>> +        wait_for_console_pattern(self, 'QEMU Advent Calendar 2023')
>> +        wait_for_console_pattern(self, 'buildroot login:')
>> +        exec_command_and_wait_for_pattern(self, 'root', '#')
>> +        exec_command_and_wait_for_pattern(self,
>> +                'tftp -g -r xmaton.png 10.0.2.2 ; md5sum xmaton.png',
>> +                '821cd3cab8efd16ad6ee5acc3642a8ea')
>> +
>> +class MicroblazeBigEndianMachine(MicroblazeMachine):
> 
> Add this here 'til the problem with the precaching is fixed:
> 
>      ASSET_IMAGE_BE = MicroblazeMachine.ASSET_IMAGE_BE
> 
> ?

Actually the cache works, I mis-interpreted the network issue.

I'll update Daniel on the other thread.

Re: [PATCH v6 01/11] hw/qdev-properties-system: Introduce EndianMode QAPI enum

[Philippe Mathieu-Daudé]

On 12/2/25 12:43, Philippe Mathieu-Daudé wrote:
> On 12/2/25 12:37, Thomas Huth wrote:
>> On 12/02/2025 12.24, Philippe Mathieu-Daudé wrote:
>>> Introduce the EndianMode type and the DEFINE_PROP_ENDIAN() macros.
>>> Endianness can be BIG, LITTLE or unspecified (default).
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>   qapi/common.json                    | 16 ++++++++++++++++
>>>   include/hw/qdev-properties-system.h |  7 +++++++
>>>   hw/core/qdev-properties-system.c    | 11 +++++++++++
>>>   3 files changed, 34 insertions(+)
>>>
>>> diff --git a/qapi/common.json b/qapi/common.json
>>> index 6ffc7a37890..217feaaf683 100644
>>> --- a/qapi/common.json
>>> +++ b/qapi/common.json
>>> @@ -212,3 +212,19 @@
>>>   ##
>>>   { 'struct': 'HumanReadableText',
>>>     'data': { 'human-readable-text': 'str' } }
>>> +
>>> +##
>>> +# @EndianMode:
>>> +#
>>> +# An enumeration of three options: little, big, and unspecified
>>> +#
>>> +# @little: Little endianness
>>> +#
>>> +# @big: Big endianness
>>> +#
>>> +# @unspecified: Endianness not specified
>>> +#
>>> +# Since: 10.0
>>> +##
>>> +{ 'enum': 'EndianMode',
>>> +  'data': [ 'little', 'big', 'unspecified' ] }
>>
>> Should 'unspecified' come first? ... so that it gets the value 0, i.e. 
>> when someone forgets to properly initialize a related variable, the 
>> chances are higher that it ends up as "unspecified" than as "little" ?

BTW I'm not sure QAPI guaranty enums are following an order
(at least, as in C, I wouldn't rely on that assumption).

Re: [PATCH v6 01/11] hw/qdev-properties-system: Introduce EndianMode QAPI enum

[Daniel P. Berrangé]

On Wed, Feb 12, 2025 at 01:02:18PM +0100, Philippe Mathieu-Daudé wrote:
> On 12/2/25 12:43, Philippe Mathieu-Daudé wrote:
> > On 12/2/25 12:37, Thomas Huth wrote:
> > > On 12/02/2025 12.24, Philippe Mathieu-Daudé wrote:
> > > > Introduce the EndianMode type and the DEFINE_PROP_ENDIAN() macros.
> > > > Endianness can be BIG, LITTLE or unspecified (default).
> > > > 
> > > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > > > ---
> > > >   qapi/common.json                    | 16 ++++++++++++++++
> > > >   include/hw/qdev-properties-system.h |  7 +++++++
> > > >   hw/core/qdev-properties-system.c    | 11 +++++++++++
> > > >   3 files changed, 34 insertions(+)
> > > > 
> > > > diff --git a/qapi/common.json b/qapi/common.json
> > > > index 6ffc7a37890..217feaaf683 100644
> > > > --- a/qapi/common.json
> > > > +++ b/qapi/common.json
> > > > @@ -212,3 +212,19 @@
> > > >   ##
> > > >   { 'struct': 'HumanReadableText',
> > > >     'data': { 'human-readable-text': 'str' } }
> > > > +
> > > > +##
> > > > +# @EndianMode:
> > > > +#
> > > > +# An enumeration of three options: little, big, and unspecified
> > > > +#
> > > > +# @little: Little endianness
> > > > +#
> > > > +# @big: Big endianness
> > > > +#
> > > > +# @unspecified: Endianness not specified
> > > > +#
> > > > +# Since: 10.0
> > > > +##
> > > > +{ 'enum': 'EndianMode',
> > > > +  'data': [ 'little', 'big', 'unspecified' ] }
> > > 
> > > Should 'unspecified' come first? ... so that it gets the value 0,
> > > i.e. when someone forgets to properly initialize a related variable,
> > > the chances are higher that it ends up as "unspecified" than as
> > > "little" ?
> 
> BTW I'm not sure QAPI guaranty enums are following an order
> (at least, as in C, I wouldn't rely on that assumption).

If we don't document a guaranteed order IMHO we should, mostly just for the
sake for guaranteeing exactly what will be the 0 value . It is pretty common
to want a particular enum constant to be special default for the 0 value.
It allows enums to be retrofitted into existing code, with confidence that
any code forgetting to initialize a variable/field will get the special
default. Missed initialization is relatively common as a C bug, and we
use -ftrivial-auto-var-init=zero to give well defined (usually safe)
semantics.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v6 01/11] hw/qdev-properties-system: Introduce EndianMode QAPI enum

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 3468 bytes --]

On Wed, 12 Feb 2025, Philippe Mathieu-Daudé wrote:
> On 12/2/25 12:37, Thomas Huth wrote:
>> On 12/02/2025 12.24, Philippe Mathieu-Daudé wrote:
>>> Introduce the EndianMode type and the DEFINE_PROP_ENDIAN() macros.
>>> Endianness can be BIG, LITTLE or unspecified (default).
>>> 
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>   qapi/common.json                    | 16 ++++++++++++++++
>>>   include/hw/qdev-properties-system.h |  7 +++++++
>>>   hw/core/qdev-properties-system.c    | 11 +++++++++++
>>>   3 files changed, 34 insertions(+)
>>> 
>>> diff --git a/qapi/common.json b/qapi/common.json
>>> index 6ffc7a37890..217feaaf683 100644
>>> --- a/qapi/common.json
>>> +++ b/qapi/common.json
>>> @@ -212,3 +212,19 @@
>>>   ##
>>>   { 'struct': 'HumanReadableText',
>>>     'data': { 'human-readable-text': 'str' } }
>>> +
>>> +##
>>> +# @EndianMode:
>>> +#
>>> +# An enumeration of three options: little, big, and unspecified
>>> +#
>>> +# @little: Little endianness
>>> +#
>>> +# @big: Big endianness
>>> +#
>>> +# @unspecified: Endianness not specified
>>> +#
>>> +# Since: 10.0
>>> +##
>>> +{ 'enum': 'EndianMode',
>>> +  'data': [ 'little', 'big', 'unspecified' ] }
>> 
>> Should 'unspecified' come first? ... so that it gets the value 0, i.e. when 
>> someone forgets to properly initialize a related variable, the chances are 
>> higher that it ends up as "unspecified" than as "little" ?
>
> Hmm but then in this series the dual-endianness regions are defined as:
>
> +static const MemoryRegionOps pic_ops[2] = {
> +    [0 ... 1] = {

This is already confusing as you'd have to know that 0 and 1 here means 
ENDIAN_MODE_LITTLE and ENDIAN_MODE_BIG (using those constants here as well 
might be clearer). It's easy to miss what this does so maybe repeating the 
definitions for each case would be longer but less confusing and then it 
does not matter what the values are.

Or what uses the ops.endianness now should look at the property introduced 
by this patch to avoid having to propagate it like below and drop the 
ops.endianness? Or it should move to the memory region and set when the 
ops are assigned?

Regards,
BALATON Zoltan

> +        .read = pic_read,
> +        .write = pic_write,
> +        .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,
> +        },
>     },
> -    .valid = {
> -        .min_access_size = 4,
> -        .max_access_size = 4
> -    }
> +    [ENDIAN_MODE_BIG].endianness = DEVICE_BIG_ENDIAN,
> +    [ENDIAN_MODE_LITTLE].endianness = DEVICE_LITTLE_ENDIAN,
> };
>
> We could declare the array using the confusing __MAX definition
> (at the price of wasting the ENDIAN_MODE_UNSPECIFIED entry) as:
>
> static const MemoryRegionOps pic_ops[ENDIAN_MODE__MAX - 1] { }
>
> WDYT?
>
>> Apart from that:
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>> 
>
> Thanks!
>
>

Re: [PATCH v6 01/11] hw/qdev-properties-system: Introduce EndianMode QAPI enum

[Philippe Mathieu-Daudé]

On 12/2/25 13:56, BALATON Zoltan wrote:
> On Wed, 12 Feb 2025, Philippe Mathieu-Daudé wrote:
>> On 12/2/25 12:37, Thomas Huth wrote:
>>> On 12/02/2025 12.24, Philippe Mathieu-Daudé wrote:
>>>> Introduce the EndianMode type and the DEFINE_PROP_ENDIAN() macros.
>>>> Endianness can be BIG, LITTLE or unspecified (default).
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>> ---
>>>>   qapi/common.json                    | 16 ++++++++++++++++
>>>>   include/hw/qdev-properties-system.h |  7 +++++++
>>>>   hw/core/qdev-properties-system.c    | 11 +++++++++++
>>>>   3 files changed, 34 insertions(+)
>>>>
>>>> diff --git a/qapi/common.json b/qapi/common.json
>>>> index 6ffc7a37890..217feaaf683 100644
>>>> --- a/qapi/common.json
>>>> +++ b/qapi/common.json
>>>> @@ -212,3 +212,19 @@
>>>>   ##
>>>>   { 'struct': 'HumanReadableText',
>>>>     'data': { 'human-readable-text': 'str' } }
>>>> +
>>>> +##
>>>> +# @EndianMode:
>>>> +#
>>>> +# An enumeration of three options: little, big, and unspecified
>>>> +#
>>>> +# @little: Little endianness
>>>> +#
>>>> +# @big: Big endianness
>>>> +#
>>>> +# @unspecified: Endianness not specified
>>>> +#
>>>> +# Since: 10.0
>>>> +##
>>>> +{ 'enum': 'EndianMode',
>>>> +  'data': [ 'little', 'big', 'unspecified' ] }
>>>
>>> Should 'unspecified' come first? ... so that it gets the value 0, 
>>> i.e. when someone forgets to properly initialize a related variable, 
>>> the chances are higher that it ends up as "unspecified" than as 
>>> "little" ?
>>
>> Hmm but then in this series the dual-endianness regions are defined as:
>>
>> +static const MemoryRegionOps pic_ops[2] = {
>> +    [0 ... 1] = {
> 
> This is already confusing as you'd have to know that 0 and 1 here means 
> ENDIAN_MODE_LITTLE and ENDIAN_MODE_BIG (using those constants here as 
> well might be clearer). It's easy to miss what this does so maybe 
> repeating the definitions for each case would be longer but less 
> confusing and then it does not matter what the values are.
> 
> Or what uses the ops.endianness now should look at the property 
> introduced by this patch to avoid having to propagate it like below and 
> drop the ops.endianness? Or it should move to the memory region and set 
> when the ops are assigned?

I'm not understanding well what you ask, but maybe the answer is in v7 :)

Re: [PATCH v6 01/11] hw/qdev-properties-system: Introduce EndianMode QAPI enum

[Philippe Mathieu-Daudé]

On 12/2/25 14:53, Philippe Mathieu-Daudé wrote:
> On 12/2/25 13:56, BALATON Zoltan wrote:
>> On Wed, 12 Feb 2025, Philippe Mathieu-Daudé wrote:
>>> On 12/2/25 12:37, Thomas Huth wrote:
>>>> On 12/02/2025 12.24, Philippe Mathieu-Daudé wrote:
>>>>> Introduce the EndianMode type and the DEFINE_PROP_ENDIAN() macros.
>>>>> Endianness can be BIG, LITTLE or unspecified (default).
>>>>>
>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>> ---
>>>>>   qapi/common.json                    | 16 ++++++++++++++++
>>>>>   include/hw/qdev-properties-system.h |  7 +++++++
>>>>>   hw/core/qdev-properties-system.c    | 11 +++++++++++
>>>>>   3 files changed, 34 insertions(+)
>>>>>
>>>>> diff --git a/qapi/common.json b/qapi/common.json
>>>>> index 6ffc7a37890..217feaaf683 100644
>>>>> --- a/qapi/common.json
>>>>> +++ b/qapi/common.json
>>>>> @@ -212,3 +212,19 @@
>>>>>   ##
>>>>>   { 'struct': 'HumanReadableText',
>>>>>     'data': { 'human-readable-text': 'str' } }
>>>>> +
>>>>> +##
>>>>> +# @EndianMode:
>>>>> +#
>>>>> +# An enumeration of three options: little, big, and unspecified
>>>>> +#
>>>>> +# @little: Little endianness
>>>>> +#
>>>>> +# @big: Big endianness
>>>>> +#
>>>>> +# @unspecified: Endianness not specified
>>>>> +#
>>>>> +# Since: 10.0
>>>>> +##
>>>>> +{ 'enum': 'EndianMode',
>>>>> +  'data': [ 'little', 'big', 'unspecified' ] }
>>>>
>>>> Should 'unspecified' come first? ... so that it gets the value 0, 
>>>> i.e. when someone forgets to properly initialize a related variable, 
>>>> the chances are higher that it ends up as "unspecified" than as 
>>>> "little" ?
>>>
>>> Hmm but then in this series the dual-endianness regions are defined as:
>>>
>>> +static const MemoryRegionOps pic_ops[2] = {
>>> +    [0 ... 1] = {
>>
>> This is already confusing as you'd have to know that 0 and 1 here 
>> means ENDIAN_MODE_LITTLE and ENDIAN_MODE_BIG (using those constants 
>> here as well might be clearer). It's easy to miss what this does so 

At this point 0 / 1 only mean "from the index #0 included to the index
#1 included", 0 being the first one and 1 the last one.

>> maybe repeating the definitions for each case would be longer but less 
>> confusing and then it does not matter what the values are.

This is what I tried to do with:

+    [ENDIAN_MODE_BIG].endianness = DEVICE_BIG_ENDIAN,
+    [ENDIAN_MODE_LITTLE].endianness = DEVICE_LITTLE_ENDIAN,
};

but in v7 we are back of picking an arbitrary value.

>> Or what uses the ops.endianness now should look at the property 
>> introduced by this patch to avoid having to propagate it like below 
>> and drop the ops.endianness? Or it should move to the memory region 
>> and set when the ops are assigned?
> 
> I'm not understanding well what you ask, but maybe the answer is in v7 :)

Re: [PATCH v6 01/11] hw/qdev-properties-system: Introduce EndianMode QAPI enum

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 4095 bytes --]

On Wed, 12 Feb 2025, Philippe Mathieu-Daudé wrote:
> On 12/2/25 14:53, Philippe Mathieu-Daudé wrote:
>> On 12/2/25 13:56, BALATON Zoltan wrote:
>>> On Wed, 12 Feb 2025, Philippe Mathieu-Daudé wrote:
>>>> On 12/2/25 12:37, Thomas Huth wrote:
>>>>> On 12/02/2025 12.24, Philippe Mathieu-Daudé wrote:
>>>>>> Introduce the EndianMode type and the DEFINE_PROP_ENDIAN() macros.
>>>>>> Endianness can be BIG, LITTLE or unspecified (default).
>>>>>> 
>>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>>> ---
>>>>>>   qapi/common.json                    | 16 ++++++++++++++++
>>>>>>   include/hw/qdev-properties-system.h |  7 +++++++
>>>>>>   hw/core/qdev-properties-system.c    | 11 +++++++++++
>>>>>>   3 files changed, 34 insertions(+)
>>>>>> 
>>>>>> diff --git a/qapi/common.json b/qapi/common.json
>>>>>> index 6ffc7a37890..217feaaf683 100644
>>>>>> --- a/qapi/common.json
>>>>>> +++ b/qapi/common.json
>>>>>> @@ -212,3 +212,19 @@
>>>>>>   ##
>>>>>>   { 'struct': 'HumanReadableText',
>>>>>>     'data': { 'human-readable-text': 'str' } }
>>>>>> +
>>>>>> +##
>>>>>> +# @EndianMode:
>>>>>> +#
>>>>>> +# An enumeration of three options: little, big, and unspecified
>>>>>> +#
>>>>>> +# @little: Little endianness
>>>>>> +#
>>>>>> +# @big: Big endianness
>>>>>> +#
>>>>>> +# @unspecified: Endianness not specified
>>>>>> +#
>>>>>> +# Since: 10.0
>>>>>> +##
>>>>>> +{ 'enum': 'EndianMode',
>>>>>> +  'data': [ 'little', 'big', 'unspecified' ] }
>>>>> 
>>>>> Should 'unspecified' come first? ... so that it gets the value 0, i.e. 
>>>>> when someone forgets to properly initialize a related variable, the 
>>>>> chances are higher that it ends up as "unspecified" than as "little" ?
>>>> 
>>>> Hmm but then in this series the dual-endianness regions are defined as:
>>>> 
>>>> +static const MemoryRegionOps pic_ops[2] = {
>>>> +    [0 ... 1] = {
>>> 
>>> This is already confusing as you'd have to know that 0 and 1 here means 
>>> ENDIAN_MODE_LITTLE and ENDIAN_MODE_BIG (using those constants here as well 
>>> might be clearer). It's easy to miss what this does so 
>
> At this point 0 / 1 only mean "from the index #0 included to the index
> #1 included", 0 being the first one and 1 the last one.
>
>>> maybe repeating the definitions for each case would be longer but less 
>>> confusing and then it does not matter what the values are.
>
> This is what I tried to do with:
>
> +    [ENDIAN_MODE_BIG].endianness = DEVICE_BIG_ENDIAN,
> +    [ENDIAN_MODE_LITTLE].endianness = DEVICE_LITTLE_ENDIAN,
> };
>
> but in v7 we are back of picking an arbitrary value.
>
>>> Or what uses the ops.endianness now should look at the property introduced 
>>> by this patch to avoid having to propagate it like below and drop the 
>>> ops.endianness? Or it should move to the memory region and set when the 
>>> ops are assigned?
>> 
>> I'm not understanding well what you ask, but maybe the answer is in v7 :)

I'm not sure I understand it well either. I think what I was asking about 
is the same as what Thomas asked if this could be avoided to make it 
necessary to allocate two separate ops for this. Looks like from now on 
this ops struct should really loose the endianness value and this should 
be assigned when the ops is added to the io region because that's where it 
decides which endianness is it based on the property added in this series. 
But I don't know if that could be done or would need deeper changes as 
what later uses this ops struct might not have access to the property and 
if we have a single ops struct it may need to be copied to set different 
endianness intstead of just referencing it. So I'm not sure there's a 
better way but I think this change makes an already cryptic boiler plate 
even more confusing for people less knowledgeable about QEMU and C 
programming so it makes even harder to write devices. But as long as it's 
just a few devices that need to work with different endianness then it 
might be OK. But wasn't that what NATIVE_ENDIAN was meant for? What can't 
that be kept then?

Regards,
BALATON Zoltan

Re: [PATCH v6 01/11] hw/qdev-properties-system: Introduce EndianMode QAPI enum

[Philippe Mathieu-Daudé]

On 12/2/25 17:23, BALATON Zoltan wrote:
> On Wed, 12 Feb 2025, Philippe Mathieu-Daudé wrote:
>> On 12/2/25 14:53, Philippe Mathieu-Daudé wrote:
>>> On 12/2/25 13:56, BALATON Zoltan wrote:
>>>> On Wed, 12 Feb 2025, Philippe Mathieu-Daudé wrote:
>>>>> On 12/2/25 12:37, Thomas Huth wrote:
>>>>>> On 12/02/2025 12.24, Philippe Mathieu-Daudé wrote:
>>>>>>> Introduce the EndianMode type and the DEFINE_PROP_ENDIAN() macros.
>>>>>>> Endianness can be BIG, LITTLE or unspecified (default).
>>>>>>>
>>>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>>>> ---
>>>>>>>   qapi/common.json                    | 16 ++++++++++++++++
>>>>>>>   include/hw/qdev-properties-system.h |  7 +++++++
>>>>>>>   hw/core/qdev-properties-system.c    | 11 +++++++++++
>>>>>>>   3 files changed, 34 insertions(+)


>>>>>>> +{ 'enum': 'EndianMode',
>>>>>>> +  'data': [ 'little', 'big', 'unspecified' ] }
>>>>>>
>>>>>> Should 'unspecified' come first? ... so that it gets the value 0, 
>>>>>> i.e. when someone forgets to properly initialize a related 
>>>>>> variable, the chances are higher that it ends up as "unspecified" 
>>>>>> than as "little" ?
>>>>>
>>>>> Hmm but then in this series the dual-endianness regions are defined 
>>>>> as:
>>>>>
>>>>> +static const MemoryRegionOps pic_ops[2] = {
>>>>> +    [0 ... 1] = {
>>>>
>>>> This is already confusing as you'd have to know that 0 and 1 here 
>>>> means ENDIAN_MODE_LITTLE and ENDIAN_MODE_BIG (using those constants 
>>>> here as well might be clearer). It's easy to miss what this does so 
>>
>> At this point 0 / 1 only mean "from the index #0 included to the index
>> #1 included", 0 being the first one and 1 the last one.
>>
>>>> maybe repeating the definitions for each case would be longer but 
>>>> less confusing and then it does not matter what the values are.
>>
>> This is what I tried to do with:
>>
>> +    [ENDIAN_MODE_BIG].endianness = DEVICE_BIG_ENDIAN,
>> +    [ENDIAN_MODE_LITTLE].endianness = DEVICE_LITTLE_ENDIAN,
>> };
>>
>> but in v7 we are back of picking an arbitrary value.
>>
>>>> Or what uses the ops.endianness now should look at the property 
>>>> introduced by this patch to avoid having to propagate it like below 
>>>> and drop the ops.endianness? Or it should move to the memory region 
>>>> and set when the ops are assigned?
>>>
>>> I'm not understanding well what you ask, but maybe the answer is in 
>>> v7 :)
> 
> I'm not sure I understand it well either. I think what I was asking 
> about is the same as what Thomas asked if this could be avoided to make 
> it necessary to allocate two separate ops for this. Looks like from now 
> on this ops struct should really loose the endianness value and this 
> should be assigned when the ops is added to the io region because that's 
> where it decides which endianness is it based on the property added in 
> this series. But I don't know if that could be done or would need deeper 
> changes as what later uses this ops struct might not have access to the 
> property and if we have a single ops struct it may need to be copied to 
> set different endianness intstead of just referencing it. So I'm not 
> sure there's a better way but I think this change makes an already 
> cryptic boiler plate even more confusing for people less knowledgeable 
> about QEMU and C programming so it makes even harder to write devices. 
> But as long as it's just a few devices that need to work with different 
> endianness then it might be OK. But wasn't that what NATIVE_ENDIAN was 
> meant for? What can't that be kept then?

Moving toward a single binary able to run heterogeneous machines, we
can't rely on a particular target endianness, so we need to remove
DEVICE_NATIVE_ENDIAN. The endianness is a property a device / machine,
not of the binary.

Re: [PATCH v6 01/11] hw/qdev-properties-system: Introduce EndianMode QAPI enum

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 4431 bytes --]

On Wed, 12 Feb 2025, Philippe Mathieu-Daudé wrote:
> On 12/2/25 17:23, BALATON Zoltan wrote:
>> On Wed, 12 Feb 2025, Philippe Mathieu-Daudé wrote:
>>> On 12/2/25 14:53, Philippe Mathieu-Daudé wrote:
>>>> On 12/2/25 13:56, BALATON Zoltan wrote:
>>>>> On Wed, 12 Feb 2025, Philippe Mathieu-Daudé wrote:
>>>>>> On 12/2/25 12:37, Thomas Huth wrote:
>>>>>>> On 12/02/2025 12.24, Philippe Mathieu-Daudé wrote:
>>>>>>>> Introduce the EndianMode type and the DEFINE_PROP_ENDIAN() macros.
>>>>>>>> Endianness can be BIG, LITTLE or unspecified (default).
>>>>>>>> 
>>>>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>>>>> ---
>>>>>>>>   qapi/common.json                    | 16 ++++++++++++++++
>>>>>>>>   include/hw/qdev-properties-system.h |  7 +++++++
>>>>>>>>   hw/core/qdev-properties-system.c    | 11 +++++++++++
>>>>>>>>   3 files changed, 34 insertions(+)
>
>
>>>>>>>> +{ 'enum': 'EndianMode',
>>>>>>>> +  'data': [ 'little', 'big', 'unspecified' ] }
>>>>>>> 
>>>>>>> Should 'unspecified' come first? ... so that it gets the value 0, i.e. 
>>>>>>> when someone forgets to properly initialize a related variable, the 
>>>>>>> chances are higher that it ends up as "unspecified" than as "little" ?
>>>>>> 
>>>>>> Hmm but then in this series the dual-endianness regions are defined as:
>>>>>> 
>>>>>> +static const MemoryRegionOps pic_ops[2] = {
>>>>>> +    [0 ... 1] = {
>>>>> 
>>>>> This is already confusing as you'd have to know that 0 and 1 here means 
>>>>> ENDIAN_MODE_LITTLE and ENDIAN_MODE_BIG (using those constants here as 
>>>>> well might be clearer). It's easy to miss what this does so 
>>> 
>>> At this point 0 / 1 only mean "from the index #0 included to the index
>>> #1 included", 0 being the first one and 1 the last one.
>>> 
>>>>> maybe repeating the definitions for each case would be longer but less 
>>>>> confusing and then it does not matter what the values are.
>>> 
>>> This is what I tried to do with:
>>> 
>>> +    [ENDIAN_MODE_BIG].endianness = DEVICE_BIG_ENDIAN,
>>> +    [ENDIAN_MODE_LITTLE].endianness = DEVICE_LITTLE_ENDIAN,
>>> };
>>> 
>>> but in v7 we are back of picking an arbitrary value.
>>> 
>>>>> Or what uses the ops.endianness now should look at the property 
>>>>> introduced by this patch to avoid having to propagate it like below and 
>>>>> drop the ops.endianness? Or it should move to the memory region and set 
>>>>> when the ops are assigned?
>>>> 
>>>> I'm not understanding well what you ask, but maybe the answer is in v7 :)
>> 
>> I'm not sure I understand it well either. I think what I was asking about 
>> is the same as what Thomas asked if this could be avoided to make it 
>> necessary to allocate two separate ops for this. Looks like from now on 
>> this ops struct should really loose the endianness value and this should be 
>> assigned when the ops is added to the io region because that's where it 
>> decides which endianness is it based on the property added in this series. 
>> But I don't know if that could be done or would need deeper changes as what 
>> later uses this ops struct might not have access to the property and if we 
>> have a single ops struct it may need to be copied to set different 
>> endianness intstead of just referencing it. So I'm not sure there's a 
>> better way but I think this change makes an already cryptic boiler plate 
>> even more confusing for people less knowledgeable about QEMU and C 
>> programming so it makes even harder to write devices. But as long as it's 
>> just a few devices that need to work with different endianness then it 
>> might be OK. But wasn't that what NATIVE_ENDIAN was meant for? What can't 
>> that be kept then?
>
> Moving toward a single binary able to run heterogeneous machines, we
> can't rely on a particular target endianness, so we need to remove
> DEVICE_NATIVE_ENDIAN. The endianness is a property a device / machine,
> not of the binary.

So then can the behaviour of NATIVE_ENDIAN be changed to look at the 
machine endianness instead of replacing it with a constant? Or would that 
be too much overhead? If always looking up the endianness is not wanted 
could the ops declaration keep NATIVE_ENDIAN but when the ops is added to 
the memory region can the register function replace it with the 
appropriate constant (if needed copying the struct)? Or is there something 
that prevents doing that?

Regards,
BALATON Zoltan

Re: [PATCH v6 01/11] hw/qdev-properties-system: Introduce EndianMode QAPI enum

[Thomas Huth]

On 12/02/2025 23.34, BALATON Zoltan wrote:
> On Wed, 12 Feb 2025, Philippe Mathieu-Daudé wrote:
>> On 12/2/25 17:23, BALATON Zoltan wrote:
>>> On Wed, 12 Feb 2025, Philippe Mathieu-Daudé wrote:
>>>> On 12/2/25 14:53, Philippe Mathieu-Daudé wrote:
>>>>> On 12/2/25 13:56, BALATON Zoltan wrote:
>>>>>> On Wed, 12 Feb 2025, Philippe Mathieu-Daudé wrote:
>>>>>>> On 12/2/25 12:37, Thomas Huth wrote:
>>>>>>>> On 12/02/2025 12.24, Philippe Mathieu-Daudé wrote:
>>>>>>>>> Introduce the EndianMode type and the DEFINE_PROP_ENDIAN() macros.
>>>>>>>>> Endianness can be BIG, LITTLE or unspecified (default).
>>>>>>>>>
>>>>>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>>>>>> ---
>>>>>>>>>   qapi/common.json                    | 16 ++++++++++++++++
>>>>>>>>>   include/hw/qdev-properties-system.h |  7 +++++++
>>>>>>>>>   hw/core/qdev-properties-system.c    | 11 +++++++++++
>>>>>>>>>   3 files changed, 34 insertions(+)
>>
>>
>>>>>>>>> +{ 'enum': 'EndianMode',
>>>>>>>>> +  'data': [ 'little', 'big', 'unspecified' ] }
>>>>>>>>
>>>>>>>> Should 'unspecified' come first? ... so that it gets the value 0, 
>>>>>>>> i.e. when someone forgets to properly initialize a related variable, 
>>>>>>>> the chances are higher that it ends up as "unspecified" than as 
>>>>>>>> "little" ?
>>>>>>>
>>>>>>> Hmm but then in this series the dual-endianness regions are defined as:
>>>>>>>
>>>>>>> +static const MemoryRegionOps pic_ops[2] = {
>>>>>>> +    [0 ... 1] = {
>>>>>>
>>>>>> This is already confusing as you'd have to know that 0 and 1 here 
>>>>>> means ENDIAN_MODE_LITTLE and ENDIAN_MODE_BIG (using those constants 
>>>>>> here as well might be clearer). It's easy to miss what this does so 
>>>>
>>>> At this point 0 / 1 only mean "from the index #0 included to the index
>>>> #1 included", 0 being the first one and 1 the last one.
>>>>
>>>>>> maybe repeating the definitions for each case would be longer but less 
>>>>>> confusing and then it does not matter what the values are.
>>>>
>>>> This is what I tried to do with:
>>>>
>>>> +    [ENDIAN_MODE_BIG].endianness = DEVICE_BIG_ENDIAN,
>>>> +    [ENDIAN_MODE_LITTLE].endianness = DEVICE_LITTLE_ENDIAN,
>>>> };
>>>>
>>>> but in v7 we are back of picking an arbitrary value.
>>>>
>>>>>> Or what uses the ops.endianness now should look at the property 
>>>>>> introduced by this patch to avoid having to propagate it like below 
>>>>>> and drop the ops.endianness? Or it should move to the memory region 
>>>>>> and set when the ops are assigned?
>>>>>
>>>>> I'm not understanding well what you ask, but maybe the answer is in v7 :)
>>>
>>> I'm not sure I understand it well either. I think what I was asking about 
>>> is the same as what Thomas asked if this could be avoided to make it 
>>> necessary to allocate two separate ops for this. Looks like from now on 
>>> this ops struct should really loose the endianness value and this should 
>>> be assigned when the ops is added to the io region because that's where 
>>> it decides which endianness is it based on the property added in this 
>>> series. But I don't know if that could be done or would need deeper 
>>> changes as what later uses this ops struct might not have access to the 
>>> property and if we have a single ops struct it may need to be copied to 
>>> set different endianness intstead of just referencing it. So I'm not sure 
>>> there's a better way but I think this change makes an already cryptic 
>>> boiler plate even more confusing for people less knowledgeable about QEMU 
>>> and C programming so it makes even harder to write devices. But as long 
>>> as it's just a few devices that need to work with different endianness 
>>> then it might be OK. But wasn't that what NATIVE_ENDIAN was meant for? 
>>> What can't that be kept then?
>>
>> Moving toward a single binary able to run heterogeneous machines, we
>> can't rely on a particular target endianness, so we need to remove
>> DEVICE_NATIVE_ENDIAN. The endianness is a property a device / machine,
>> not of the binary.
> 
> So then can the behaviour of NATIVE_ENDIAN be changed to look at the machine 
> endianness instead of replacing it with a constant?

No, that does not work. First, the machine knows about its devices, but a 
device should not know about the wiring of the global machine (just like in 
real life). Second, imagine a board with e.g. a big endian main CPU and a 
little endian service processor - how should a device know the right 
endianness here?

  Thomas

> Or would that be too 
> much overhead? If always looking up the endianness is not wanted could the 
> ops declaration keep NATIVE_ENDIAN 

IMHO we should get rid of NATIVE_ENDIAN completely since there is no 
"native" endian in multi-CPU boards.

  Thomas

Re: [PATCH v6 01/11] hw/qdev-properties-system: Introduce EndianMode QAPI enum

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 7606 bytes --]

On Thu, 13 Feb 2025, Thomas Huth wrote:
> On 12/02/2025 23.34, BALATON Zoltan wrote:
>> On Wed, 12 Feb 2025, Philippe Mathieu-Daudé wrote:
>>> On 12/2/25 17:23, BALATON Zoltan wrote:
>>>> On Wed, 12 Feb 2025, Philippe Mathieu-Daudé wrote:
>>>>> On 12/2/25 14:53, Philippe Mathieu-Daudé wrote:
>>>>>> On 12/2/25 13:56, BALATON Zoltan wrote:
>>>>>>> On Wed, 12 Feb 2025, Philippe Mathieu-Daudé wrote:
>>>>>>>> On 12/2/25 12:37, Thomas Huth wrote:
>>>>>>>>> On 12/02/2025 12.24, Philippe Mathieu-Daudé wrote:
>>>>>>>>>> Introduce the EndianMode type and the DEFINE_PROP_ENDIAN() macros.
>>>>>>>>>> Endianness can be BIG, LITTLE or unspecified (default).
>>>>>>>>>> 
>>>>>>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>>>>>>> ---
>>>>>>>>>>   qapi/common.json                    | 16 ++++++++++++++++
>>>>>>>>>>   include/hw/qdev-properties-system.h |  7 +++++++
>>>>>>>>>>   hw/core/qdev-properties-system.c    | 11 +++++++++++
>>>>>>>>>>   3 files changed, 34 insertions(+)
>>> 
>>> 
>>>>>>>>>> +{ 'enum': 'EndianMode',
>>>>>>>>>> +  'data': [ 'little', 'big', 'unspecified' ] }
>>>>>>>>> 
>>>>>>>>> Should 'unspecified' come first? ... so that it gets the value 0, 
>>>>>>>>> i.e. when someone forgets to properly initialize a related variable, 
>>>>>>>>> the chances are higher that it ends up as "unspecified" than as 
>>>>>>>>> "little" ?
>>>>>>>> 
>>>>>>>> Hmm but then in this series the dual-endianness regions are defined 
>>>>>>>> as:
>>>>>>>> 
>>>>>>>> +static const MemoryRegionOps pic_ops[2] = {
>>>>>>>> +    [0 ... 1] = {
>>>>>>> 
>>>>>>> This is already confusing as you'd have to know that 0 and 1 here 
>>>>>>> means ENDIAN_MODE_LITTLE and ENDIAN_MODE_BIG (using those constants 
>>>>>>> here as well might be clearer). It's easy to miss what this does so 
>>>>> 
>>>>> At this point 0 / 1 only mean "from the index #0 included to the index
>>>>> #1 included", 0 being the first one and 1 the last one.
>>>>> 
>>>>>>> maybe repeating the definitions for each case would be longer but less 
>>>>>>> confusing and then it does not matter what the values are.
>>>>> 
>>>>> This is what I tried to do with:
>>>>> 
>>>>> +    [ENDIAN_MODE_BIG].endianness = DEVICE_BIG_ENDIAN,
>>>>> +    [ENDIAN_MODE_LITTLE].endianness = DEVICE_LITTLE_ENDIAN,
>>>>> };
>>>>> 
>>>>> but in v7 we are back of picking an arbitrary value.
>>>>> 
>>>>>>> Or what uses the ops.endianness now should look at the property 
>>>>>>> introduced by this patch to avoid having to propagate it like below 
>>>>>>> and drop the ops.endianness? Or it should move to the memory region 
>>>>>>> and set when the ops are assigned?
>>>>>> 
>>>>>> I'm not understanding well what you ask, but maybe the answer is in v7 
>>>>>> :)
>>>> 
>>>> I'm not sure I understand it well either. I think what I was asking about 
>>>> is the same as what Thomas asked if this could be avoided to make it 
>>>> necessary to allocate two separate ops for this. Looks like from now on 
>>>> this ops struct should really loose the endianness value and this should 
>>>> be assigned when the ops is added to the io region because that's where 
>>>> it decides which endianness is it based on the property added in this 
>>>> series. But I don't know if that could be done or would need deeper 
>>>> changes as what later uses this ops struct might not have access to the 
>>>> property and if we have a single ops struct it may need to be copied to 
>>>> set different endianness intstead of just referencing it. So I'm not sure 
>>>> there's a better way but I think this change makes an already cryptic 
>>>> boiler plate even more confusing for people less knowledgeable about QEMU 
>>>> and C programming so it makes even harder to write devices. But as long 
>>>> as it's just a few devices that need to work with different endianness 
>>>> then it might be OK. But wasn't that what NATIVE_ENDIAN was meant for? 
>>>> What can't that be kept then?
>>> 
>>> Moving toward a single binary able to run heterogeneous machines, we
>>> can't rely on a particular target endianness, so we need to remove
>>> DEVICE_NATIVE_ENDIAN. The endianness is a property a device / machine,
>>> not of the binary.
>> 
>> So then can the behaviour of NATIVE_ENDIAN be changed to look at the 
>> machine endianness instead of replacing it with a constant?
>
> No, that does not work. First, the machine knows about its devices, but a 
> device should not know about the wiring of the global machine (just like in 
> real life).

That means all devices should be either big or little endian and there 
should be no native endian ones. Why do we have those then? That's why 
this endianness property should either be removed from ops and only 
attached to it when added to a machine if needed or kept to show which 
machines it can be attached to: only big, little or both endian which is 
what it seems to be doing now.

> Second, imagine a board with e.g. a big endian main CPU and a 
> little endian service processor - how should a device know the right 
> endianness here?

How would that work with this series? So the proposed solution is to 
double the devices now marked as NATIVE_ENDIAN to have a big and a little 
endian variant for them so the board can choose? That does not exist in 
real as you wrote, there's only one device so then this is probably not 
the right way to model it.

>> Or would that be too much overhead? If always looking up the endianness is 
>> not wanted could the ops declaration keep NATIVE_ENDIAN 
>
> IMHO we should get rid of NATIVE_ENDIAN completely since there is no "native" 
> endian in multi-CPU boards.

If we say NATIVE_ENDIAN means that the device can be attached to either 
big or little endian machine then we can keep this constant but when 
adding the ops to a memory region the board has to then decide which 
endianness it is and replace it with either big or little. Then we don't 
need two versions of the same device and NATIVE_ENDIAN means that the 
device can be used in both machines.

In real life probably all devices can be used with either CPU and if they 
are accessed in little or big endian is only determinded by how they are 
wired on the board. So the device endianness only means what endianness 
the device expects for something (what exactly? e.g. a video chip may have 
a frame buffer and a registers area with different endianness). So this 
should be the board that decides this not the device. Therefore it may not 
need to be defined when MemoryRegionOps is defined at all (or only as a 
hint to show what the device expects normally) and then 
memory_region_init_io which takes the MemoryRegionOps should also take an 
endianness corresponding the board and set it at that point. It can warn 
if the device endianness does not match what the board sets but you can 
still connect a big endian device to a little endian CPU as long as the 
drivers write the right values or the data lines are connected the right 
way, the latter of which corresponds to NATIVE_ENDIAN now as the 
conversion is done by the wiring so drivers don't need to care.

But if it's simpler to just double the few devices that need to be used 
this way then it's a possible solution but if there's a cleaner one with 
not much more complexity then maybe that should be considered, because the 
way to define these doubled devices is a bit confusing for new people (on 
top of that defining devices is already confusing with the lot of boiler 
plate code needed). So if this could be kept simpler that would be a good 
thing IMO.

Regards,
BALATON Zoltan

Re: [PATCH v6 01/11] hw/qdev-properties-system: Introduce EndianMode QAPI enum

[Philippe Mathieu-Daudé]

On 13/2/25 14:59, BALATON Zoltan wrote:
> On Thu, 13 Feb 2025, Thomas Huth wrote:
>> On 12/02/2025 23.34, BALATON Zoltan wrote:
>>> On Wed, 12 Feb 2025, Philippe Mathieu-Daudé wrote:
>>>> On 12/2/25 17:23, BALATON Zoltan wrote:
>>>>> On Wed, 12 Feb 2025, Philippe Mathieu-Daudé wrote:
>>>>>> On 12/2/25 14:53, Philippe Mathieu-Daudé wrote:
>>>>>>> On 12/2/25 13:56, BALATON Zoltan wrote:
>>>>>>>> On Wed, 12 Feb 2025, Philippe Mathieu-Daudé wrote:
>>>>>>>>> On 12/2/25 12:37, Thomas Huth wrote:
>>>>>>>>>> On 12/02/2025 12.24, Philippe Mathieu-Daudé wrote:
>>>>>>>>>>> Introduce the EndianMode type and the DEFINE_PROP_ENDIAN() 
>>>>>>>>>>> macros.
>>>>>>>>>>> Endianness can be BIG, LITTLE or unspecified (default).
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>>>>>>>> ---
>>>>>>>>>>>   qapi/common.json                    | 16 ++++++++++++++++
>>>>>>>>>>>   include/hw/qdev-properties-system.h |  7 +++++++
>>>>>>>>>>>   hw/core/qdev-properties-system.c    | 11 +++++++++++
>>>>>>>>>>>   3 files changed, 34 insertions(+)
>>>>
>>>>
>>>>>>>>>>> +{ 'enum': 'EndianMode',
>>>>>>>>>>> +  'data': [ 'little', 'big', 'unspecified' ] }
>>>>>>>>>>
>>>>>>>>>> Should 'unspecified' come first? ... so that it gets the value 
>>>>>>>>>> 0, i.e. when someone forgets to properly initialize a related 
>>>>>>>>>> variable, the chances are higher that it ends up as 
>>>>>>>>>> "unspecified" than as "little" ?
>>>>>>>>>
>>>>>>>>> Hmm but then in this series the dual-endianness regions are 
>>>>>>>>> defined as:
>>>>>>>>>
>>>>>>>>> +static const MemoryRegionOps pic_ops[2] = {
>>>>>>>>> +    [0 ... 1] = {
>>>>>>>>
>>>>>>>> This is already confusing as you'd have to know that 0 and 1 
>>>>>>>> here means ENDIAN_MODE_LITTLE and ENDIAN_MODE_BIG (using those 
>>>>>>>> constants here as well might be clearer). It's easy to miss what 
>>>>>>>> this does so 
>>>>>>
>>>>>> At this point 0 / 1 only mean "from the index #0 included to the 
>>>>>> index
>>>>>> #1 included", 0 being the first one and 1 the last one.
>>>>>>
>>>>>>>> maybe repeating the definitions for each case would be longer 
>>>>>>>> but less confusing and then it does not matter what the values are.
>>>>>>
>>>>>> This is what I tried to do with:
>>>>>>
>>>>>> +    [ENDIAN_MODE_BIG].endianness = DEVICE_BIG_ENDIAN,
>>>>>> +    [ENDIAN_MODE_LITTLE].endianness = DEVICE_LITTLE_ENDIAN,
>>>>>> };
>>>>>>
>>>>>> but in v7 we are back of picking an arbitrary value.
>>>>>>
>>>>>>>> Or what uses the ops.endianness now should look at the property 
>>>>>>>> introduced by this patch to avoid having to propagate it like 
>>>>>>>> below and drop the ops.endianness? Or it should move to the 
>>>>>>>> memory region and set when the ops are assigned?
>>>>>>>
>>>>>>> I'm not understanding well what you ask, but maybe the answer is 
>>>>>>> in v7 :)
>>>>>
>>>>> I'm not sure I understand it well either. I think what I was asking 
>>>>> about is the same as what Thomas asked if this could be avoided to 
>>>>> make it necessary to allocate two separate ops for this. Looks like 
>>>>> from now on this ops struct should really loose the endianness 
>>>>> value and this should be assigned when the ops is added to the io 
>>>>> region because that's where it decides which endianness is it based 
>>>>> on the property added in this series. But I don't know if that 
>>>>> could be done or would need deeper changes as what later uses this 
>>>>> ops struct might not have access to the property and if we have a 
>>>>> single ops struct it may need to be copied to set different 
>>>>> endianness intstead of just referencing it. So I'm not sure there's 
>>>>> a better way but I think this change makes an already cryptic 
>>>>> boiler plate even more confusing for people less knowledgeable 
>>>>> about QEMU and C programming so it makes even harder to write 
>>>>> devices. But as long as it's just a few devices that need to work 
>>>>> with different endianness then it might be OK. But wasn't that what 
>>>>> NATIVE_ENDIAN was meant for? What can't that be kept then?
>>>>
>>>> Moving toward a single binary able to run heterogeneous machines, we
>>>> can't rely on a particular target endianness, so we need to remove
>>>> DEVICE_NATIVE_ENDIAN. The endianness is a property a device / machine,
>>>> not of the binary.
>>>
>>> So then can the behaviour of NATIVE_ENDIAN be changed to look at the 
>>> machine endianness instead of replacing it with a constant?
>>
>> No, that does not work. First, the machine knows about its devices, 
>> but a device should not know about the wiring of the global machine 
>> (just like in real life).
> 
> That means all devices should be either big or little endian and there 
> should be no native endian ones. Why do we have those then? That's why 
> this endianness property should either be removed from ops and only 
> attached to it when added to a machine if needed or kept to show which 
> machines it can be attached to: only big, little or both endian which is 
> what it seems to be doing now.

Well, devices don't have endianness, endianness is more a property of
the bus [*] between cpu <-> devices, how the bytes are serialized.

QEMU devices have an endianness property so the core memory can access
them using the proper ordering, see:

$ git grep -wW memory_region_big_endian
system/memory.c:356:static bool memory_region_big_endian(MemoryRegion *mr)
system/memory.c-357-{
system/memory.c-358-#if TARGET_BIG_ENDIAN
system/memory.c-359-    return mr->ops->endianness != DEVICE_LITTLE_ENDIAN;
system/memory.c-360-#else
system/memory.c-361-    return mr->ops->endianness == DEVICE_BIG_ENDIAN;
system/memory.c-362-#endif
system/memory.c-363-}
--
system/memory.c=521=static MemTxResult access_with_adjusted_size(hwaddr 
addr,
system/memory.c-522-                                      uint64_t *value,
system/memory.c-523-                                      unsigned size,
system/memory.c-524-                                      unsigned 
access_size_min,
system/memory.c-525-                                      unsigned 
access_size_max,
system/memory.c-526-                                      MemTxResult 
(*access_fn)
system/memory.c-527- 
(MemoryRegion *mr,
system/memory.c-528- 
hwaddr addr,
system/memory.c-529- 
uint64_t *value,
system/memory.c-530- 
unsigned size,
system/memory.c-531- 
signed shift,
system/memory.c-532- 
uint64_t mask,
system/memory.c-533- 
MemTxAttrs attrs),
system/memory.c-534-                                      MemoryRegion *mr,
system/memory.c-535-                                      MemTxAttrs attrs)
system/memory.c-536-{
system/memory.c-537-    uint64_t access_mask;
system/memory.c-538-    unsigned access_size;
system/memory.c-539-    unsigned i;
system/memory.c-540-    MemTxResult r = MEMTX_OK;
system/memory.c-541-    bool reentrancy_guard_applied = false;
system/memory.c-542-
system/memory.c-543-    if (!access_size_min) {
system/memory.c-544-        access_size_min = 1;
system/memory.c-545-    }
system/memory.c-546-    if (!access_size_max) {
system/memory.c-547-        access_size_max = 4;
system/memory.c-548-    }
system/memory.c-549-
system/memory.c-550-    /* Do not allow more than one simultaneous 
access to a device's IO Regions */
system/memory.c-551-    if (mr->dev && !mr->disable_reentrancy_guard &&
system/memory.c-552-        !mr->ram_device && !mr->ram && 
!mr->rom_device && !mr->readonly) {
system/memory.c-553-        if 
(mr->dev->mem_reentrancy_guard.engaged_in_io) {
system/memory.c-554-            warn_report_once("Blocked re-entrant IO 
on MemoryRegion: "
system/memory.c-555-                             "%s at addr: 0x%" 
HWADDR_PRIX,
system/memory.c-556-                             memory_region_name(mr), 
addr);
system/memory.c-557-            return MEMTX_ACCESS_ERROR;
system/memory.c-558-        }
system/memory.c-559-        mr->dev->mem_reentrancy_guard.engaged_in_io 
= true;
system/memory.c-560-        reentrancy_guard_applied = true;
system/memory.c-561-    }
system/memory.c-562-
system/memory.c-563-    /* FIXME: support unaligned access? */
system/memory.c-564-    access_size = MAX(MIN(size, access_size_max), 
access_size_min);
system/memory.c-565-    access_mask = MAKE_64BIT_MASK(0, access_size * 8);
system/memory.c:566:    if (memory_region_big_endian(mr)) {
system/memory.c-567-        for (i = 0; i < size; i += access_size) {
system/memory.c-568-            r |= access_fn(mr, addr + i, value, 
access_size,
system/memory.c-569-                        (size - access_size - i) * 
8, access_mask, attrs);
system/memory.c-570-        }
system/memory.c-571-    } else {
system/memory.c-572-        for (i = 0; i < size; i += access_size) {
system/memory.c-573-            r |= access_fn(mr, addr + i, value, 
access_size, i * 8,
system/memory.c-574-                        access_mask, attrs);
system/memory.c-575-        }
system/memory.c-576-    }
system/memory.c-577-    if (mr->dev && reentrancy_guard_applied) {
system/memory.c-578-        mr->dev->mem_reentrancy_guard.engaged_in_io 
= false;
system/memory.c-579-    }
system/memory.c-580-    return r;
system/memory.c-581-}

[*] See chapter 2 "The Basics of Endianness", in particular section
2.2 'Endianness Definition by Bus Specification':
https://www.intel.com/content/dam/www/public/us/en/documents/application-notes/ixp4xx-ixc1100-big-endian-little-endian-modes-note.pdf

>> Second, imagine a board with e.g. a big endian main CPU and a little 
>> endian service processor - how should a device know the right 
>> endianness here?
> 
> How would that work with this series? So the proposed solution is to 
> double the devices now marked as NATIVE_ENDIAN to have a big and a 
> little endian variant for them so the board can choose? That does not 
> exist in real as you wrote, there's only one device so then this is 
> probably not the right way to model it.
> 
>>> Or would that be too much overhead? If always looking up the 
>>> endianness is not wanted could the ops declaration keep NATIVE_ENDIAN 
>>
>> IMHO we should get rid of NATIVE_ENDIAN completely since there is no 
>> "native" endian in multi-CPU boards.
> 
> If we say NATIVE_ENDIAN means that the device can be attached to either 
> big or little endian machine then we can keep this constant but when 
> adding the ops to a memory region the board has to then decide which 
> endianness it is and replace it with either big or little. Then we don't 
> need two versions of the same device and NATIVE_ENDIAN means that the 
> device can be used in both machines.
> 
> In real life probably all devices can be used with either CPU and if 
> they are accessed in little or big endian is only determinded by how 
> they are wired on the board. So the device endianness only means what 
> endianness the device expects for something (what exactly? e.g. a video 
> chip may have a frame buffer and a registers area with different 
> endianness). So this should be the board that decides this not the 
> device. Therefore it may not need to be defined when MemoryRegionOps is 
> defined at all (or only as a hint to show what the device expects 
> normally) and then memory_region_init_io which takes the MemoryRegionOps 
> should also take an endianness corresponding the board and set it at 
> that point. It can warn if the device endianness does not match what the 
> board sets but you can still connect a big endian device to a little 
> endian CPU as long as the drivers write the right values or the data 
> lines are connected the right way, the latter of which corresponds to 
> NATIVE_ENDIAN now as the conversion is done by the wiring so drivers 
> don't need to care.
> 
> But if it's simpler to just double the few devices that need to be used 
> this way then it's a possible solution but if there's a cleaner one with 
> not much more complexity then maybe that should be considered, because 
> the way to define these doubled devices is a bit confusing for new 
> people (on top of that defining devices is already confusing with the 
> lot of boiler plate code needed). So if this could be kept simpler that 
> would be a good thing IMO.
> 
> Regards,
> BALATON Zoltan

Re: [PATCH v6 01/11] hw/qdev-properties-system: Introduce EndianMode QAPI enum

[Thomas Huth]

On 13/02/2025 14.59, BALATON Zoltan wrote:
> On Thu, 13 Feb 2025, Thomas Huth wrote:
>> On 12/02/2025 23.34, BALATON Zoltan wrote:
[...]
>>> So then can the behaviour of NATIVE_ENDIAN be changed to look at the 
>>> machine endianness instead of replacing it with a constant?
>>
>> No, that does not work. First, the machine knows about its devices, but a 
>> device should not know about the wiring of the global machine (just like 
>> in real life).
> 
> That means all devices should be either big or little endian and there 
> should be no native endian ones. Why do we have those then?

Some device can indeed be either big or little endian - think of devices 
that are synthesized in an FPGA for example. But in most cases, it rather 
depends on the bus wiring. Anyway, we need a config knob to allow either the 
one or the other endianness for certain devices.

> That's why this 
> endianness property should either be removed from ops and only attached to 
> it when added to a machine if needed or kept to show which machines it can 
> be attached to: only big, little or both endian which is what it seems to be 
> doing now.

Again, devices should not know about machines, not the other way round. So 
the device should offer a config switch (property) and the machine should 
set it to the value that it needs.

>> Second, imagine a board with e.g. a big endian main CPU and a little 
>> endian service processor - how should a device know the right endianness 
>> here?
> 
> How would that work with this series? So the proposed solution is to double 
> the devices now marked as NATIVE_ENDIAN to have a big and a little endian 
> variant for them so the board can choose?

This is not doubling the devices. It just introduces a config property to 
let the machine switch the endianness.

> That does not exist in real as you 
> wrote, there's only one device so then this is probably not the right way to 
> model it.

Some devices can exist in both, big and little endian variants. We could of 
course create two devices for this, but that's nonsense if it can simply be 
handled by a property instead.

>>> Or would that be too much overhead? If always looking up the endianness 
>>> is not wanted could the ops declaration keep NATIVE_ENDIAN 
>>
>> IMHO we should get rid of NATIVE_ENDIAN completely since there is no 
>> "native" endian in multi-CPU boards.
> 
> If we say NATIVE_ENDIAN means that the device can be attached to either big 
> or little endian machine then we can keep this constant but when adding the 
> ops to a memory region the board has to then decide which endianness it is 
> and replace it with either big or little. Then we don't need two versions of 
> the same device and NATIVE_ENDIAN means that the device can be used in both 
> machines.

Well, it's currently the devices that are calling memory_region_init_io().
And since memory_region_init_io() does not copy the MemoryRegionOps struct,
we need two implementations right now for this, one for big and one for 
little endian. So I think Philippe's series here is fine. But feel free to 
suggest clean up patches on top if you think that the 
memory_region_init_io() needs to be handled differently in QEMU everywhere.

  Thomas

Re: [PATCH v6 01/11] hw/qdev-properties-system: Introduce EndianMode QAPI enum

[BALATON Zoltan]

On Thu, 13 Feb 2025, Thomas Huth wrote:
> On 13/02/2025 14.59, BALATON Zoltan wrote:
>> On Thu, 13 Feb 2025, Thomas Huth wrote:
>>> On 12/02/2025 23.34, BALATON Zoltan wrote:
> [...]
>>>> So then can the behaviour of NATIVE_ENDIAN be changed to look at the 
>>>> machine endianness instead of replacing it with a constant?
>>> 
>>> No, that does not work. First, the machine knows about its devices, but a 
>>> device should not know about the wiring of the global machine (just like 
>>> in real life).
>> 
>> That means all devices should be either big or little endian and there 
>> should be no native endian ones. Why do we have those then?
>
> Some device can indeed be either big or little endian - think of devices that 
> are synthesized in an FPGA for example. But in most cases, it rather depends 
> on the bus wiring. Anyway, we need a config knob to allow either the one or 
> the other endianness for certain devices.
>
>> That's why this endianness property should either be removed from ops and 
>> only attached to it when added to a machine if needed or kept to show which 
>> machines it can be attached to: only big, little or both endian which is 
>> what it seems to be doing now.
>
> Again, devices should not know about machines, not the other way round. So 
> the device should offer a config switch (property) and the machine should set 
> it to the value that it needs.

That would mean this endianness in ops should be set when the memory 
region is mapped in the machine not when defining the device. So all 
device ops should really be NATIVE_ENDIAN and only assigned an endianness 
when they are mapped based on the machine/bus/cpu they are connected to.

>>> Second, imagine a board with e.g. a big endian main CPU and a little 
>>> endian service processor - how should a device know the right endianness 
>>> here?
>> 
>> How would that work with this series? So the proposed solution is to double 
>> the devices now marked as NATIVE_ENDIAN to have a big and a little endian 
>> variant for them so the board can choose?
>
> This is not doubling the devices. It just introduces a config property to let 
> the machine switch the endianness.

Yes, I've oversimpified, each ops has its own endianness config not the 
device. But since the endianness is not a property of the device or its 
regions but the bus it's connected to, this config switch may be at the 
wrong place now.

>> That does not exist in real as you wrote, there's only one device so then 
>> this is probably not the right way to model it.
>
> Some devices can exist in both, big and little endian variants. We could of 
> course create two devices for this, but that's nonsense if it can simply be 
> handled by a property instead.
>
>>>> Or would that be too much overhead? If always looking up the endianness 
>>>> is not wanted could the ops declaration keep NATIVE_ENDIAN 
>>> 
>>> IMHO we should get rid of NATIVE_ENDIAN completely since there is no 
>>> "native" endian in multi-CPU boards.
>> 
>> If we say NATIVE_ENDIAN means that the device can be attached to either big 
>> or little endian machine then we can keep this constant but when adding the 
>> ops to a memory region the board has to then decide which endianness it is 
>> and replace it with either big or little. Then we don't need two versions 
>> of the same device and NATIVE_ENDIAN means that the device can be used in 
>> both machines.
>
> Well, it's currently the devices that are calling memory_region_init_io().

That answers my question. Then it seems it's not so simple to set 
endianness when the device is mapped because it would need to be done 
somewhere else. It may still be possible but might be too much work to 
find it out.

> And since memory_region_init_io() does not copy the MemoryRegionOps struct,
> we need two implementations right now for this, one for big and one for 
> little endian. So I think Philippe's series here is fine.

It could copy the MemoryRegionOps when needed but seems it's not 
memory_region_init_io that would need to handle that. Given the current 
way it's implemented doubling the ops region may be the simplest even if 
not the correct way to do it so it's OK if there's no simple alternative 
that's more correct.

> But feel free to 
> suggest clean up patches on top if you think that the memory_region_init_io() 
> needs to be handled differently in QEMU everywhere.

I think so as I've described above but not enough to try to solve it so 
I'm OK with Philippe's series if there's no other way to make it less like 
a workaround for something that could be done clearer. Looks like other 
way might be too complex for now.

Regards,
BALATON Zoltan