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!