|
| From: | BALATON Zoltan |
| Subject: | Re: [PATCH v6 01/11] hw/qdev-properties-system: Introduce EndianMode QAPI enum |
| Date: | Thu, 13 Feb 2025 15:56:24 +0100 (CET) |
On Thu, 13 Feb 2025, Thomas Huth wrote:
On 13/02/2025 14.59, BALATON Zoltan wrote:On Thu, 13 Feb 2025, Thomas Huth wrote:On 12/02/2025 23.34, BALATON Zoltan wrote:[...]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?Some device can indeed be either big or little endian - think of devices that are synthesized in an FPGA for example. But in most cases, it rather depends on the bus wiring. Anyway, we need a config knob to allow either the one or the other endianness for certain devices.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.Again, devices should not know about machines, not the other way round. So the device should offer a config switch (property) and the machine should set it to the value that it needs.
That would mean this endianness in ops should be set when the memory region is mapped in the machine not when defining the device. So all device ops should really be NATIVE_ENDIAN and only assigned an endianness when they are mapped based on the machine/bus/cpu they are connected to.
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?This is not doubling the devices. It just introduces a config property to let the machine switch the endianness.
Yes, I've oversimpified, each ops has its own endianness config not the device. But since the endianness is not a property of the device or its regions but the bus it's connected to, this config switch may be at the wrong place now.
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.Some devices can exist in both, big and little endian variants. We could of course create two devices for this, but that's nonsense if it can simply be handled by a property instead.Or would that be too much overhead? If always looking up the endianness is not wanted could the ops declaration keep NATIVE_ENDIANIMHO 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.Well, it's currently the devices that are calling memory_region_init_io().
That answers my question. Then it seems it's not so simple to set endianness when the device is mapped because it would need to be done somewhere else. It may still be possible but might be too much work to find it out.
And since memory_region_init_io() does not copy the MemoryRegionOps struct,we need two implementations right now for this, one for big and one for little endian. So I think Philippe's series here is fine.
It could copy the MemoryRegionOps when needed but seems it's not memory_region_init_io that would need to handle that. Given the current way it's implemented doubling the ops region may be the simplest even if not the correct way to do it so it's OK if there's no simple alternative that's more correct.
But feel free to suggest clean up patches on top if you think that the memory_region_init_io() needs to be handled differently in QEMU everywhere.
I think so as I've described above but not enough to try to solve it so I'm OK with Philippe's series if there's no other way to make it less like a workaround for something that could be done clearer. Looks like other way might be too complex for now.
Regards, BALATON Zoltan
| [Prev in Thread] | Current Thread | [Next in Thread] |