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

[Thomas Huth <thuth@redhat.com>]

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