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

[Andrew Jones <ajones@ventanamicro.com>]

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@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
> 
> 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