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
>
> > .version = legacy ? 1 : 2,
> > .device_id = subsys_id,
> > - .vendor_id = 0x4d564b4c , /* 'LKVM' */
> > + .vendor_id = le32_to_cpu(0x4d564b4c), /* 'LKVM' */
> > .queue_num_max = 256,
> > };
> >
>
>
> --
> Ben Dooks http://www.codethink.co.uk/
> Senior Engineer Codethink - Providing Genius
>
> https://www.codethink.co.uk/privacy.html
