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

[Philippe Mathieu-Daudé]

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.