On 16/01/2025 09:28, Andrew Jones wrote:
On Wed, Jan 15, 2025 at 03:09:58PM +0000, Ben Dooks wrote:
On 15/01/2025 14:24, Andrew Jones wrote:
On Wed, Jan 15, 2025 at 10:11:25AM +0000, Ben Dooks wrote:
When running on a big endian host, the virtio mmio-modern.c correctly
sets all reads to return little endian values. However the header uses
a 4 byte char for the magic value, which is always going to be in the
correct endian regardless of host endian.
To make the simplest change, simply avoid endian convresion of the
read of the magic value. This fixes the following bug from the guest:
[ 0.592838] virtio-mmio 10020000.virtio: Wrong magic value 0x76697274!
Signed-off-by: Ben Dooks <ben.dooks@xxxxxxxxxxxxxxx>
---
virtio/mmio-modern.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/virtio/mmio-modern.c b/virtio/mmio-modern.c
index 6c0bb38..fd9c0cb 100644
--- a/virtio/mmio-modern.c
+++ b/virtio/mmio-modern.c
@@ -66,7 +66,10 @@ static void virtio_mmio_config_in(struct kvm_cpu *vcpu,
return;
}
- *data = cpu_to_le32(val);
+ if (addr != VIRTIO_MMIO_MAGIC_VALUE)
+ *data = cpu_to_le32(val);
+ else
+ *data = val;
}
static void virtio_mmio_config_out(struct kvm_cpu *vcpu,
--
2.37.2.352.g3c44437643
I think vendor_id should also have the same issue, but drivers don't
notice because they all use VIRTIO_DEV_ANY_ID. So how about the
change below instead?
Thanks,
drew
diff --git a/include/kvm/virtio-mmio.h b/include/kvm/virtio-mmio.h
index b428b8d32f48..133817c1dc44 100644
--- a/include/kvm/virtio-mmio.h
+++ b/include/kvm/virtio-mmio.h
@@ -18,7 +18,7 @@ struct virtio_mmio_ioevent_param {
};
struct virtio_mmio_hdr {
- char magic[4];
+ u32 magic;
u32 version;
u32 device_id;
u32 vendor_id;
diff --git a/virtio/mmio.c b/virtio/mmio.c
index fae73b52dae0..782268e8f842 100644
--- a/virtio/mmio.c
+++ b/virtio/mmio.c
@@ -6,6 +6,7 @@
#include "kvm/irq.h"
#include "kvm/fdt.h"
+#include <linux/byteorder.h>
#include <linux/virtio_mmio.h>
#include <string.h>
@@ -168,10 +169,10 @@ int virtio_mmio_init(struct kvm *kvm, void *dev, struct virtio_device *vdev,
return r;
vmmio->hdr = (struct virtio_mmio_hdr) {
- .magic = {'v', 'i', 'r', 't'},
+ .magic = le32_to_cpu(0x74726976), /* 'virt' */
just doing the change of magic type and then doing
.magic = 0x74726976;
should work, as then magic is in host order amd will get converted
to le32 in the IO code. Don't think vendor_id suffers as it was
converted from string to hex.
Oh, right. I overthought that one. I prefer the magic in hex better than
the special casing in virtio_mmio_config_in()
Thanks,
drew
Ok, will wait a few days to see if anyone else has a comment.
I assume you're ok with me re-doing my patch?
Thanks for the review.
--
Ben Dooks http://www.codethink.co.uk/
Senior Engineer Codethink - Providing Genius
https://www.codethink.co.uk/privacy.html
