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_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.
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
