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

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

On 13/2/25 14:59, BALATON Zoltan wrote:
> On Thu, 13 Feb 2025, Thomas Huth wrote:
>> 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).
> 
> That means all devices should be either big or little endian and there 
> should be no native endian ones. Why do we have those then? That's why 
> this endianness property should either be removed from ops and only 
> attached to it when added to a machine if needed or kept to show which 
> machines it can be attached to: only big, little or both endian which is 
> what it seems to be doing now.

Well, devices don't have endianness, endianness is more a property of
the bus [*] between cpu <-> devices, how the bytes are serialized.

QEMU devices have an endianness property so the core memory can access
them using the proper ordering, see:

$ git grep -wW memory_region_big_endian
system/memory.c:356:static bool memory_region_big_endian(MemoryRegion *mr)
system/memory.c-357-{
system/memory.c-358-#if TARGET_BIG_ENDIAN
system/memory.c-359-    return mr->ops->endianness != DEVICE_LITTLE_ENDIAN;
system/memory.c-360-#else
system/memory.c-361-    return mr->ops->endianness == DEVICE_BIG_ENDIAN;
system/memory.c-362-#endif
system/memory.c-363-}
--
system/memory.c=521=static MemTxResult access_with_adjusted_size(hwaddr 
addr,
system/memory.c-522-                                      uint64_t *value,
system/memory.c-523-                                      unsigned size,
system/memory.c-524-                                      unsigned 
access_size_min,
system/memory.c-525-                                      unsigned 
access_size_max,
system/memory.c-526-                                      MemTxResult 
(*access_fn)
system/memory.c-527- 
(MemoryRegion *mr,
system/memory.c-528- 
hwaddr addr,
system/memory.c-529- 
uint64_t *value,
system/memory.c-530- 
unsigned size,
system/memory.c-531- 
signed shift,
system/memory.c-532- 
uint64_t mask,
system/memory.c-533- 
MemTxAttrs attrs),
system/memory.c-534-                                      MemoryRegion *mr,
system/memory.c-535-                                      MemTxAttrs attrs)
system/memory.c-536-{
system/memory.c-537-    uint64_t access_mask;
system/memory.c-538-    unsigned access_size;
system/memory.c-539-    unsigned i;
system/memory.c-540-    MemTxResult r = MEMTX_OK;
system/memory.c-541-    bool reentrancy_guard_applied = false;
system/memory.c-542-
system/memory.c-543-    if (!access_size_min) {
system/memory.c-544-        access_size_min = 1;
system/memory.c-545-    }
system/memory.c-546-    if (!access_size_max) {
system/memory.c-547-        access_size_max = 4;
system/memory.c-548-    }
system/memory.c-549-
system/memory.c-550-    /* Do not allow more than one simultaneous 
access to a device's IO Regions */
system/memory.c-551-    if (mr->dev && !mr->disable_reentrancy_guard &&
system/memory.c-552-        !mr->ram_device && !mr->ram && 
!mr->rom_device && !mr->readonly) {
system/memory.c-553-        if 
(mr->dev->mem_reentrancy_guard.engaged_in_io) {
system/memory.c-554-            warn_report_once("Blocked re-entrant IO 
on MemoryRegion: "
system/memory.c-555-                             "%s at addr: 0x%" 
HWADDR_PRIX,
system/memory.c-556-                             memory_region_name(mr), 
addr);
system/memory.c-557-            return MEMTX_ACCESS_ERROR;
system/memory.c-558-        }
system/memory.c-559-        mr->dev->mem_reentrancy_guard.engaged_in_io 
= true;
system/memory.c-560-        reentrancy_guard_applied = true;
system/memory.c-561-    }
system/memory.c-562-
system/memory.c-563-    /* FIXME: support unaligned access? */
system/memory.c-564-    access_size = MAX(MIN(size, access_size_max), 
access_size_min);
system/memory.c-565-    access_mask = MAKE_64BIT_MASK(0, access_size * 8);
system/memory.c:566:    if (memory_region_big_endian(mr)) {
system/memory.c-567-        for (i = 0; i < size; i += access_size) {
system/memory.c-568-            r |= access_fn(mr, addr + i, value, 
access_size,
system/memory.c-569-                        (size - access_size - i) * 
8, access_mask, attrs);
system/memory.c-570-        }
system/memory.c-571-    } else {
system/memory.c-572-        for (i = 0; i < size; i += access_size) {
system/memory.c-573-            r |= access_fn(mr, addr + i, value, 
access_size, i * 8,
system/memory.c-574-                        access_mask, attrs);
system/memory.c-575-        }
system/memory.c-576-    }
system/memory.c-577-    if (mr->dev && reentrancy_guard_applied) {
system/memory.c-578-        mr->dev->mem_reentrancy_guard.engaged_in_io 
= false;
system/memory.c-579-    }
system/memory.c-580-    return r;
system/memory.c-581-}

[*] See chapter 2 "The Basics of Endianness", in particular section
2.2 'Endianness Definition by Bus Specification':
https://www.intel.com/content/dam/www/public/us/en/documents/application-notes/ixp4xx-ixc1100-big-endian-little-endian-modes-note.pdf

>> 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?
> 
> How would that work with this series? So the proposed solution is to 
> double the devices now marked as NATIVE_ENDIAN to have a big and a 
> little endian variant for them so the board can choose? That does not 
> exist in real as you wrote, there's only one device so then this is 
> probably not the right way to model it.
> 
>>> 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.
> 
> If we say NATIVE_ENDIAN means that the device can be attached to either 
> big or little endian machine then we can keep this constant but when 
> adding the ops to a memory region the board has to then decide which 
> endianness it is and replace it with either big or little. Then we don't 
> need two versions of the same device and NATIVE_ENDIAN means that the 
> device can be used in both machines.
> 
> In real life probably all devices can be used with either CPU and if 
> they are accessed in little or big endian is only determinded by how 
> they are wired on the board. So the device endianness only means what 
> endianness the device expects for something (what exactly? e.g. a video 
> chip may have a frame buffer and a registers area with different 
> endianness). So this should be the board that decides this not the 
> device. Therefore it may not need to be defined when MemoryRegionOps is 
> defined at all (or only as a hint to show what the device expects 
> normally) and then memory_region_init_io which takes the MemoryRegionOps 
> should also take an endianness corresponding the board and set it at 
> that point. It can warn if the device endianness does not match what the 
> board sets but you can still connect a big endian device to a little 
> endian CPU as long as the drivers write the right values or the data 
> lines are connected the right way, the latter of which corresponds to 
> NATIVE_ENDIAN now as the conversion is done by the wiring so drivers 
> don't need to care.
> 
> But if it's simpler to just double the few devices that need to be used 
> this way then it's a possible solution but if there's a cleaner one with 
> not much more complexity then maybe that should be considered, because 
> the way to define these doubled devices is a bit confusing for new 
> people (on top of that defining devices is already confusing with the 
> lot of boiler plate code needed). So if this could be kept simpler that 
> would be a good thing IMO.
> 
> Regards,
> BALATON Zoltan