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

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

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 :)