diff mbox series

[v3,2/4] vhost-vsock-pci: force virtio version 1

Message ID 20200918074710.27810-3-sgarzare@redhat.com
State New
Headers show
Series vhost-vsock: force virtio version 1 | expand

Commit Message

Stefano Garzarella Sept. 18, 2020, 7:47 a.m. UTC
Commit 9b3a35ec82 ("virtio: verify that legacy support is not
accidentally on") added a safety check that requires to set
'disable-legacy=on' on vhost-vsock-pci device:

    $ ./qemu-system-x86_64 ... -device vhost-vsock-pci,guest-cid=5
        qemu-system-x86_64: -device vhost-vsock-pci,guest-cid=5:
        device is modern-only, use disable-legacy=on

virtio-vsock was introduced after the release of VIRTIO 1.0
specifications, so it should be 'modern-only'.
In addition Cornelia verified that forcing a legacy mode on
vhost-vsock-pci device using x86-64 host and s390x guest, so with
different endianness, produces strange behaviours.

This patch forces virtio version 1 and removes the 'transitional_name'
property removing the need to specify 'disable-legacy=on' on
vhost-vsock-pci device.

To avoid migration issues, we force virtio version 1 only when
legacy check is enabled in the new machine types (>= 5.1).

Cc: qemu-stable@nongnu.org
Reported-by: Qian Cai <caiqian@redhat.com>
Reported-by: Qinghua Cheng <qcheng@redhat.com>
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1868449
Suggested-by: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
v3:
 - forced virtio version 1 only with new machine types
v2:
 - fixed commit message [Cornelia]
---
 hw/virtio/vhost-vsock-pci.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

Comments

Cornelia Huck Sept. 18, 2020, 9:19 a.m. UTC | #1
On Fri, 18 Sep 2020 09:47:08 +0200
Stefano Garzarella <sgarzare@redhat.com> wrote:

> Commit 9b3a35ec82 ("virtio: verify that legacy support is not

> accidentally on") added a safety check that requires to set

> 'disable-legacy=on' on vhost-vsock-pci device:

> 

>     $ ./qemu-system-x86_64 ... -device vhost-vsock-pci,guest-cid=5

>         qemu-system-x86_64: -device vhost-vsock-pci,guest-cid=5:

>         device is modern-only, use disable-legacy=on

> 

> virtio-vsock was introduced after the release of VIRTIO 1.0

> specifications, so it should be 'modern-only'.

> In addition Cornelia verified that forcing a legacy mode on

> vhost-vsock-pci device using x86-64 host and s390x guest, so with

> different endianness, produces strange behaviours.

> 

> This patch forces virtio version 1 and removes the 'transitional_name'

> property removing the need to specify 'disable-legacy=on' on

> vhost-vsock-pci device.

> 

> To avoid migration issues, we force virtio version 1 only when

> legacy check is enabled in the new machine types (>= 5.1).

> 

> Cc: qemu-stable@nongnu.org

> Reported-by: Qian Cai <caiqian@redhat.com>

> Reported-by: Qinghua Cheng <qcheng@redhat.com>

> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1868449

> Suggested-by: Cornelia Huck <cohuck@redhat.com>

> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>

> ---

> v3:

>  - forced virtio version 1 only with new machine types

> v2:

>  - fixed commit message [Cornelia]

> ---

>  hw/virtio/vhost-vsock-pci.c | 10 +++++++++-

>  1 file changed, 9 insertions(+), 1 deletion(-)

> 

> diff --git a/hw/virtio/vhost-vsock-pci.c b/hw/virtio/vhost-vsock-pci.c

> index e56067b427..205da8d1f5 100644

> --- a/hw/virtio/vhost-vsock-pci.c

> +++ b/hw/virtio/vhost-vsock-pci.c

> @@ -44,6 +44,15 @@ static void vhost_vsock_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)

>  {

>      VHostVSockPCI *dev = VHOST_VSOCK_PCI(vpci_dev);

>      DeviceState *vdev = DEVICE(&dev->vdev);

> +    VirtIODevice *virtio_dev = VIRTIO_DEVICE(vdev);

> +

> +    /*

> +     * To avoid migration issues, we force virtio version 1 only when

> +     * legacy check is enabled in the new machine types (>= 5.1).

> +     */

> +    if (!virtio_legacy_check_disabled(virtio_dev)) {

> +        virtio_pci_force_virtio_1(vpci_dev);

> +    }

>  

>      qdev_realize(vdev, BUS(&vpci_dev->bus), errp);

>  }

> @@ -73,7 +82,6 @@ static void vhost_vsock_pci_instance_init(Object *obj)

>  static const VirtioPCIDeviceTypeInfo vhost_vsock_pci_info = {

>      .base_name             = TYPE_VHOST_VSOCK_PCI,

>      .generic_name          = "vhost-vsock-pci",

> -    .transitional_name     = "vhost-vsock-pci-transitional",


Hm... this means that vhost-vsock-pci-transitional won't work on compat
machines, which could also lead to migration compatibility issues (I
think?)

Is this way of specifying the device sufficiently uncommon so that we
can ignore that?


>      .non_transitional_name = "vhost-vsock-pci-non-transitional",

>      .instance_size = sizeof(VHostVSockPCI),

>      .instance_init = vhost_vsock_pci_instance_init,
Stefano Garzarella Sept. 18, 2020, 10 a.m. UTC | #2
On Fri, Sep 18, 2020 at 11:19:01AM +0200, Cornelia Huck wrote:
> On Fri, 18 Sep 2020 09:47:08 +0200

> Stefano Garzarella <sgarzare@redhat.com> wrote:

> 

> > Commit 9b3a35ec82 ("virtio: verify that legacy support is not

> > accidentally on") added a safety check that requires to set

> > 'disable-legacy=on' on vhost-vsock-pci device:

> > 

> >     $ ./qemu-system-x86_64 ... -device vhost-vsock-pci,guest-cid=5

> >         qemu-system-x86_64: -device vhost-vsock-pci,guest-cid=5:

> >         device is modern-only, use disable-legacy=on

> > 

> > virtio-vsock was introduced after the release of VIRTIO 1.0

> > specifications, so it should be 'modern-only'.

> > In addition Cornelia verified that forcing a legacy mode on

> > vhost-vsock-pci device using x86-64 host and s390x guest, so with

> > different endianness, produces strange behaviours.

> > 

> > This patch forces virtio version 1 and removes the 'transitional_name'

> > property removing the need to specify 'disable-legacy=on' on

> > vhost-vsock-pci device.

> > 

> > To avoid migration issues, we force virtio version 1 only when

> > legacy check is enabled in the new machine types (>= 5.1).

> > 

> > Cc: qemu-stable@nongnu.org

> > Reported-by: Qian Cai <caiqian@redhat.com>

> > Reported-by: Qinghua Cheng <qcheng@redhat.com>

> > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1868449

> > Suggested-by: Cornelia Huck <cohuck@redhat.com>

> > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>

> > ---

> > v3:

> >  - forced virtio version 1 only with new machine types

> > v2:

> >  - fixed commit message [Cornelia]

> > ---

> >  hw/virtio/vhost-vsock-pci.c | 10 +++++++++-

> >  1 file changed, 9 insertions(+), 1 deletion(-)

> > 

> > diff --git a/hw/virtio/vhost-vsock-pci.c b/hw/virtio/vhost-vsock-pci.c

> > index e56067b427..205da8d1f5 100644

> > --- a/hw/virtio/vhost-vsock-pci.c

> > +++ b/hw/virtio/vhost-vsock-pci.c

> > @@ -44,6 +44,15 @@ static void vhost_vsock_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)

> >  {

> >      VHostVSockPCI *dev = VHOST_VSOCK_PCI(vpci_dev);

> >      DeviceState *vdev = DEVICE(&dev->vdev);

> > +    VirtIODevice *virtio_dev = VIRTIO_DEVICE(vdev);

> > +

> > +    /*

> > +     * To avoid migration issues, we force virtio version 1 only when

> > +     * legacy check is enabled in the new machine types (>= 5.1).

> > +     */

> > +    if (!virtio_legacy_check_disabled(virtio_dev)) {

> > +        virtio_pci_force_virtio_1(vpci_dev);

> > +    }

> >  

> >      qdev_realize(vdev, BUS(&vpci_dev->bus), errp);

> >  }

> > @@ -73,7 +82,6 @@ static void vhost_vsock_pci_instance_init(Object *obj)

> >  static const VirtioPCIDeviceTypeInfo vhost_vsock_pci_info = {

> >      .base_name             = TYPE_VHOST_VSOCK_PCI,

> >      .generic_name          = "vhost-vsock-pci",

> > -    .transitional_name     = "vhost-vsock-pci-transitional",

> 

> Hm... this means that vhost-vsock-pci-transitional won't work on compat

> machines, which could also lead to migration compatibility issues (I

> think?)

> 


Yes, I think you are right.

> Is this way of specifying the device sufficiently uncommon so that we

> can ignore that?


I think it's really uncommon and we can safetly eliminate it, since it
has never been really transitional.  But if we want to leave it for
migration compatibility, that's fine with me.

Thanks,
Stefano
diff mbox series

Patch

diff --git a/hw/virtio/vhost-vsock-pci.c b/hw/virtio/vhost-vsock-pci.c
index e56067b427..205da8d1f5 100644
--- a/hw/virtio/vhost-vsock-pci.c
+++ b/hw/virtio/vhost-vsock-pci.c
@@ -44,6 +44,15 @@  static void vhost_vsock_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
 {
     VHostVSockPCI *dev = VHOST_VSOCK_PCI(vpci_dev);
     DeviceState *vdev = DEVICE(&dev->vdev);
+    VirtIODevice *virtio_dev = VIRTIO_DEVICE(vdev);
+
+    /*
+     * To avoid migration issues, we force virtio version 1 only when
+     * legacy check is enabled in the new machine types (>= 5.1).
+     */
+    if (!virtio_legacy_check_disabled(virtio_dev)) {
+        virtio_pci_force_virtio_1(vpci_dev);
+    }
 
     qdev_realize(vdev, BUS(&vpci_dev->bus), errp);
 }
@@ -73,7 +82,6 @@  static void vhost_vsock_pci_instance_init(Object *obj)
 static const VirtioPCIDeviceTypeInfo vhost_vsock_pci_info = {
     .base_name             = TYPE_VHOST_VSOCK_PCI,
     .generic_name          = "vhost-vsock-pci",
-    .transitional_name     = "vhost-vsock-pci-transitional",
     .non_transitional_name = "vhost-vsock-pci-non-transitional",
     .instance_size = sizeof(VHostVSockPCI),
     .instance_init = vhost_vsock_pci_instance_init,