qemu-arm
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Philippe Mathieu-Daudé
Subject: Re: [PATCH v6 01/11] hw/qdev-properties-system: Introduce EndianMode QAPI enum
Date: Thu, 13 Feb 2025 15:24:28 +0100
User-agent: Mozilla Thunderbird 
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
reply via email to
[Prev in Thread] Current Thread [Next in Thread]