* [PATCH kvmtool] kvmtool: virtio: fix endian for big endian hosts @ 2025-01-15 10:11 Ben Dooks 2025-01-15 14:24 ` Andrew Jones 0 siblings, 1 reply; 6+ messages in thread From: Ben Dooks @ 2025-01-15 10:11 UTC (permalink / raw) To: kvm; +Cc: felix.chong, lawrence.hunter, roan.richmond, Ben Dooks 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH kvmtool] kvmtool: virtio: fix endian for big endian hosts 2025-01-15 10:11 [PATCH kvmtool] kvmtool: virtio: fix endian for big endian hosts Ben Dooks @ 2025-01-15 14:24 ` Andrew Jones 2025-01-15 15:09 ` Ben Dooks 0 siblings, 1 reply; 6+ messages in thread From: Andrew Jones @ 2025-01-15 14:24 UTC (permalink / raw) To: Ben Dooks; +Cc: kvm, felix.chong, lawrence.hunter, roan.richmond 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' */ .version = legacy ? 1 : 2, .device_id = subsys_id, - .vendor_id = 0x4d564b4c , /* 'LKVM' */ + .vendor_id = le32_to_cpu(0x4d564b4c), /* 'LKVM' */ .queue_num_max = 256, }; ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH kvmtool] kvmtool: virtio: fix endian for big endian hosts 2025-01-15 14:24 ` Andrew Jones @ 2025-01-15 15:09 ` Ben Dooks 2025-01-16 9:28 ` Andrew Jones 0 siblings, 1 reply; 6+ messages in thread From: Ben Dooks @ 2025-01-15 15:09 UTC (permalink / raw) To: Andrew Jones; +Cc: kvm, felix.chong, lawrence.hunter, roan.richmond 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. > .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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH kvmtool] kvmtool: virtio: fix endian for big endian hosts 2025-01-15 15:09 ` Ben Dooks @ 2025-01-16 9:28 ` Andrew Jones 2025-01-17 9:57 ` Ben Dooks 0 siblings, 1 reply; 6+ messages in thread From: Andrew Jones @ 2025-01-16 9:28 UTC (permalink / raw) To: Ben Dooks; +Cc: kvm, felix.chong, lawrence.hunter, roan.richmond 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH kvmtool] kvmtool: virtio: fix endian for big endian hosts 2025-01-16 9:28 ` Andrew Jones @ 2025-01-17 9:57 ` Ben Dooks 2025-01-17 10:07 ` Andrew Jones 0 siblings, 1 reply; 6+ messages in thread From: Ben Dooks @ 2025-01-17 9:57 UTC (permalink / raw) To: Andrew Jones; +Cc: kvm, felix.chong, lawrence.hunter, roan.richmond 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? Thanks for the review. -- Ben Dooks http://www.codethink.co.uk/ Senior Engineer Codethink - Providing Genius https://www.codethink.co.uk/privacy.html ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH kvmtool] kvmtool: virtio: fix endian for big endian hosts 2025-01-17 9:57 ` Ben Dooks @ 2025-01-17 10:07 ` Andrew Jones 0 siblings, 0 replies; 6+ messages in thread From: Andrew Jones @ 2025-01-17 10:07 UTC (permalink / raw) To: Ben Dooks; +Cc: kvm, felix.chong, lawrence.hunter, roan.richmond 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-01-17 10:39 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-01-15 10:11 [PATCH kvmtool] kvmtool: virtio: fix endian for big endian hosts Ben Dooks 2025-01-15 14:24 ` Andrew Jones 2025-01-15 15:09 ` Ben Dooks 2025-01-16 9:28 ` Andrew Jones 2025-01-17 9:57 ` Ben Dooks 2025-01-17 10:07 ` Andrew Jones
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox