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

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

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.