On 2/9/26 4:17 PM, Richard Henderson wrote:
On 2/9/26 18:04, Pierrick Bouvier wrote:
On 2/8/26 9:19 PM, Richard Henderson wrote:
On 2/7/26 08:19, Pierrick Bouvier wrote:
virtio_is_big_endian already checks for VIRTIO_F_VERSION_1 feature, so
this is redundant with big_endian_code.
Also, clarify why we explicitly check for ppc64 and arm arch.
Signed-off-by: Pierrick Bouvier <[email protected]>
---
include/hw/virtio/virtio-access.h | 17 ++++++-----------
1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/include/hw/virtio/virtio-access.h
b/include/hw/virtio/virtio-access.h
index 1bea3445979..9aed3d1da4f 100644
--- a/include/hw/virtio/virtio-access.h
+++ b/include/hw/virtio/virtio-access.h
@@ -22,21 +22,16 @@
#include "hw/virtio/virtio.h"
#include "hw/virtio/virtio-bus.h"
+static inline bool legacy_virtio_is_biendian(void)
+{
+ return target_ppc64() || target_base_arm();
+}
+
static inline bool virtio_access_is_big_endian(VirtIODevice *vdev)
{
- if (target_ppc64() || target_base_arm()) {
+ if (target_big_endian() || legacy_virtio_is_biendian()) {
return virtio_is_big_endian(vdev);
}
This now calls the hook for all big-endian, not just bi-endian.
I don't think you need to change anything here...
If I'm correct, that's what original code was doing:
#if defined(LEGACY_VIRTIO_IS_BIENDIAN)
return virtio_is_big_endian(vdev);
#elif TARGET_BIG_ENDIAN
/* duplicated code for virtio_is_big_endian(vdev) */
if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
/* Devices conforming to VIRTIO 1.0 or later are always LE. */
return false;
}
return true;
#else
return false;
From what I understand, legacy virtio devices follow host endianness (thus
considering
all big-endian arch), while virtio >= 1.0 devices are always little endian.
What did I miss to consider it should only be called for bi-endian arch?
if (target_big_endian() || ...) {
return virtio_is_big_endian(vdev);
}
invokes virtio_is_big_endian for e.g. sparc, which it never did before.
The first #if defined(LEGACY_VIRTIO_IS_BIENDIAN) is exactly your
legacy_virtio_is_biendian(), and is the only time that should ever be invoked.
Separately, you noticed that virtio_device_endian_needed already checks
VIRTIO_F_VERSION_1, so checking it a second time here (possibly with the rest
of the
changes in this patch set), is unnecessary. Which allows you do remove the
big-endian
check entirely.
I do think that (again, possibly with the rest of the changes in the patch set)
the
function virtio_access_is_big_endian should be merged into its single caller,
so that (1)
it's obvious no further check vs VERSION is required and (2) no further calls of
virtio_access_is_big_endian can be introduced.
r~
We had a call with Richard, and talked about this problem, so I get a
better insight about his position.
In short, he's not convinced that checking runtime endianness is solving
any problem, whether for virtio or other components.
However, we don't really have a choice to deal with it: legacy virtio
has this flaw by design and we have to live with it.
Ideally, it would be nice to get rid of legacy virtio, which is a dead
end since it has been proposed by Philippe and rejected by community. So
what we can try to do is to make it more simple.
The good news is that we identified that for biendian arch (ppc64 and
arm/aarch64), the cached value is correct, since it's set dynamically on
(guest) reset through set_features (see patch 2 for reference).
So having a precached field for checking this on every access is
correct, and thus allows us to preserve the performance we have with
this series.
I'll try to simplify code further to split the concepts of legacy/modern
and cpu endianness, which seems to be the source of confusion for everyone.
Regards,
Pierrick
