Re: [PATCH kvmtool] kvmtool: virtio: fix endian for big endian hosts

[Andrew Jones <ajones@ventanamicro.com>]

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@codethink.co.uk>
> > > ---
> > >   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