On Fri, Jan 17, 2025 at 09:57:29AM +0000, Ben Dooks wrote:
> 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?
yup, thanks
>
> Thanks for the review.
>
>
> --
> Ben Dooks http://www.codethink.co.uk/
> Senior Engineer Codethink - Providing Genius
>
> https://www.codethink.co.uk/privacy.html
