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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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!