diff mbox series

[v3,1/4] virtio: skip legacy support check on machine types less than 5.1

Message ID 20200918074710.27810-2-sgarzare@redhat.com
State Superseded
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 check that returns an error if legacy support is on, but the
device is not legacy.

Unfortunately some devices were wrongly declared legacy even if they
were not (e.g vhost-vsock).

To avoid migration issues, we add a virtio-device property
(x-disable-legacy-check) to skip the legacy error, printing a warning
instaed, for machine types < 5.1.

Cc: qemu-stable@nongnu.org
Fixes: 9b3a35ec82 ("virtio: verify that legacy support is not accidentally on")
Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Suggested-by: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
v3:
 - added virtio_legacy_check_disabled() helper
 - moved warning where error was returned [Cornelia]
v2:
 - fixed Cornelia's e-mail address
---
 include/hw/virtio/virtio.h |  2 ++
 hw/core/machine.c          |  1 +
 hw/s390x/virtio-ccw.c      | 15 ++++++++++++---
 hw/virtio/virtio-pci.c     | 14 ++++++++++++--
 hw/virtio/virtio.c         |  7 +++++++
 5 files changed, 34 insertions(+), 5 deletions(-)

Comments

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

> Commit 9b3a35ec82 ("virtio: verify that legacy support is not accidentally
> on") added a check that returns an error if legacy support is on, but the
> device is not legacy.

s/is not legacy/does not support legacy/ ?

> 
> Unfortunately some devices were wrongly declared legacy even if they

s/legacy/legacy capable/ ?

> were not (e.g vhost-vsock).
> 
> To avoid migration issues, we add a virtio-device property
> (x-disable-legacy-check) to skip the legacy error, printing a warning
> instaed, for machine types < 5.1.

s/instaed/instead/

> 
> Cc: qemu-stable@nongnu.org
> Fixes: 9b3a35ec82 ("virtio: verify that legacy support is not accidentally on")
> Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Suggested-by: Cornelia Huck <cohuck@redhat.com>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
> v3:
>  - added virtio_legacy_check_disabled() helper
>  - moved warning where error was returned [Cornelia]
> v2:
>  - fixed Cornelia's e-mail address
> ---
>  include/hw/virtio/virtio.h |  2 ++
>  hw/core/machine.c          |  1 +
>  hw/s390x/virtio-ccw.c      | 15 ++++++++++++---
>  hw/virtio/virtio-pci.c     | 14 ++++++++++++--
>  hw/virtio/virtio.c         |  7 +++++++
>  5 files changed, 34 insertions(+), 5 deletions(-)
> 
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index 807280451b..f90cfb03e3 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -103,6 +103,7 @@ struct VirtIODevice
>      bool use_started;
>      bool started;
>      bool start_on_kick; /* when virtio 1.0 feature has not been negotiated */
> +    bool disable_legacy_check;
>      VMChangeStateEntry *vmstate;
>      char *bus_name;
>      uint8_t device_endian;
> @@ -396,5 +397,6 @@ static inline bool virtio_device_disabled(VirtIODevice *vdev)
>  }
>  
>  bool virtio_legacy_allowed(VirtIODevice *vdev);
> +bool virtio_legacy_check_disabled(VirtIODevice *vdev);
>  
>  #endif
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index ea26d61237..b686eab798 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -44,6 +44,7 @@ GlobalProperty hw_compat_5_0[] = {
>      { "vmport", "x-signal-unsupported-cmd", "off" },
>      { "vmport", "x-report-vmx-type", "off" },
>      { "vmport", "x-cmds-v2", "off" },
> +    { "virtio-device", "x-disable-legacy-check", "true" },
>  };
>  const size_t hw_compat_5_0_len = G_N_ELEMENTS(hw_compat_5_0);
>  
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index 8feb3451a0..97a01bc48a 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -1122,9 +1122,18 @@ static void virtio_ccw_device_plugged(DeviceState *d, Error **errp)
>      }
>  
>      if (!virtio_ccw_rev_max(dev) && !virtio_legacy_allowed(vdev)) {
> -        error_setg(errp, "Invalid value of property max_rev "
> -                   "(is %d expected >= 1)", virtio_ccw_rev_max(dev));
> -        return;
> +        /*
> +         * To avoid migration issues, we allow legacy mode when legacy
> +         * check is disabled in the old machine types (< 5.1).
> +         */
> +        if (virtio_legacy_check_disabled(vdev)) {
> +            warn_report("device is modern-only, but for backward "
> +                        "compatibility legacy is allowed");

Hm...

"device requires revision >= 1, but for backward compatibility
max_rev=0 is allowed"

seems more appropriate.

(I think I need to rethink the checking for ccw anyway.)

> +        } else {
> +            error_setg(errp, "Invalid value of property max_rev "
> +                       "(is %d expected >= 1)", virtio_ccw_rev_max(dev));
> +            return;
> +        }
>      }
>  
>      if (virtio_get_num_queues(vdev) > VIRTIO_QUEUE_MAX) {

(...)

With the message for ccw changed,

Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Stefano Garzarella Sept. 18, 2020, 9:35 a.m. UTC | #2
On Fri, Sep 18, 2020 at 11:13:42AM +0200, Cornelia Huck wrote:
> On Fri, 18 Sep 2020 09:47:07 +0200
> Stefano Garzarella <sgarzare@redhat.com> wrote:
> 
> > Commit 9b3a35ec82 ("virtio: verify that legacy support is not accidentally
> > on") added a check that returns an error if legacy support is on, but the
> > device is not legacy.
> 
> s/is not legacy/does not support legacy/ ?
> 
> > 
> > Unfortunately some devices were wrongly declared legacy even if they
> 
> s/legacy/legacy capable/ ?
> 
> > were not (e.g vhost-vsock).
> > 
> > To avoid migration issues, we add a virtio-device property
> > (x-disable-legacy-check) to skip the legacy error, printing a warning
> > instaed, for machine types < 5.1.
> 
> s/instaed/instead/
> 

Much better, I'll fix them in v4.

> > 
> > Cc: qemu-stable@nongnu.org
> > Fixes: 9b3a35ec82 ("virtio: verify that legacy support is not accidentally on")
> > Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > Suggested-by: Cornelia Huck <cohuck@redhat.com>
> > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > ---
> > v3:
> >  - added virtio_legacy_check_disabled() helper
> >  - moved warning where error was returned [Cornelia]
> > v2:
> >  - fixed Cornelia's e-mail address
> > ---
> >  include/hw/virtio/virtio.h |  2 ++
> >  hw/core/machine.c          |  1 +
> >  hw/s390x/virtio-ccw.c      | 15 ++++++++++++---
> >  hw/virtio/virtio-pci.c     | 14 ++++++++++++--
> >  hw/virtio/virtio.c         |  7 +++++++
> >  5 files changed, 34 insertions(+), 5 deletions(-)
> > 
> > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> > index 807280451b..f90cfb03e3 100644
> > --- a/include/hw/virtio/virtio.h
> > +++ b/include/hw/virtio/virtio.h
> > @@ -103,6 +103,7 @@ struct VirtIODevice
> >      bool use_started;
> >      bool started;
> >      bool start_on_kick; /* when virtio 1.0 feature has not been negotiated */
> > +    bool disable_legacy_check;
> >      VMChangeStateEntry *vmstate;
> >      char *bus_name;
> >      uint8_t device_endian;
> > @@ -396,5 +397,6 @@ static inline bool virtio_device_disabled(VirtIODevice *vdev)
> >  }
> >  
> >  bool virtio_legacy_allowed(VirtIODevice *vdev);
> > +bool virtio_legacy_check_disabled(VirtIODevice *vdev);
> >  
> >  #endif
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index ea26d61237..b686eab798 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -44,6 +44,7 @@ GlobalProperty hw_compat_5_0[] = {
> >      { "vmport", "x-signal-unsupported-cmd", "off" },
> >      { "vmport", "x-report-vmx-type", "off" },
> >      { "vmport", "x-cmds-v2", "off" },
> > +    { "virtio-device", "x-disable-legacy-check", "true" },
> >  };
> >  const size_t hw_compat_5_0_len = G_N_ELEMENTS(hw_compat_5_0);
> >  
> > diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> > index 8feb3451a0..97a01bc48a 100644
> > --- a/hw/s390x/virtio-ccw.c
> > +++ b/hw/s390x/virtio-ccw.c
> > @@ -1122,9 +1122,18 @@ static void virtio_ccw_device_plugged(DeviceState *d, Error **errp)
> >      }
> >  
> >      if (!virtio_ccw_rev_max(dev) && !virtio_legacy_allowed(vdev)) {
> > -        error_setg(errp, "Invalid value of property max_rev "
> > -                   "(is %d expected >= 1)", virtio_ccw_rev_max(dev));
> > -        return;
> > +        /*
> > +         * To avoid migration issues, we allow legacy mode when legacy
> > +         * check is disabled in the old machine types (< 5.1).
> > +         */
> > +        if (virtio_legacy_check_disabled(vdev)) {
> > +            warn_report("device is modern-only, but for backward "
> > +                        "compatibility legacy is allowed");
> 
> Hm...
> 
> "device requires revision >= 1, but for backward compatibility
> max_rev=0 is allowed"
> 
> seems more appropriate.

Me too, I forgot to update...

> 
> (I think I need to rethink the checking for ccw anyway.)
> 
> > +        } else {
> > +            error_setg(errp, "Invalid value of property max_rev "
> > +                       "(is %d expected >= 1)", virtio_ccw_rev_max(dev));
> > +            return;
> > +        }
> >      }
> >  
> >      if (virtio_get_num_queues(vdev) > VIRTIO_QUEUE_MAX) {
> 
> (...)
> 
> With the message for ccw changed,
> 
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> 

Thanks,
Stefano
diff mbox series

Patch

diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 807280451b..f90cfb03e3 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -103,6 +103,7 @@  struct VirtIODevice
     bool use_started;
     bool started;
     bool start_on_kick; /* when virtio 1.0 feature has not been negotiated */
+    bool disable_legacy_check;
     VMChangeStateEntry *vmstate;
     char *bus_name;
     uint8_t device_endian;
@@ -396,5 +397,6 @@  static inline bool virtio_device_disabled(VirtIODevice *vdev)
 }
 
 bool virtio_legacy_allowed(VirtIODevice *vdev);
+bool virtio_legacy_check_disabled(VirtIODevice *vdev);
 
 #endif
diff --git a/hw/core/machine.c b/hw/core/machine.c
index ea26d61237..b686eab798 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -44,6 +44,7 @@  GlobalProperty hw_compat_5_0[] = {
     { "vmport", "x-signal-unsupported-cmd", "off" },
     { "vmport", "x-report-vmx-type", "off" },
     { "vmport", "x-cmds-v2", "off" },
+    { "virtio-device", "x-disable-legacy-check", "true" },
 };
 const size_t hw_compat_5_0_len = G_N_ELEMENTS(hw_compat_5_0);
 
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 8feb3451a0..97a01bc48a 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -1122,9 +1122,18 @@  static void virtio_ccw_device_plugged(DeviceState *d, Error **errp)
     }
 
     if (!virtio_ccw_rev_max(dev) && !virtio_legacy_allowed(vdev)) {
-        error_setg(errp, "Invalid value of property max_rev "
-                   "(is %d expected >= 1)", virtio_ccw_rev_max(dev));
-        return;
+        /*
+         * To avoid migration issues, we allow legacy mode when legacy
+         * check is disabled in the old machine types (< 5.1).
+         */
+        if (virtio_legacy_check_disabled(vdev)) {
+            warn_report("device is modern-only, but for backward "
+                        "compatibility legacy is allowed");
+        } else {
+            error_setg(errp, "Invalid value of property max_rev "
+                       "(is %d expected >= 1)", virtio_ccw_rev_max(dev));
+            return;
+        }
     }
 
     if (virtio_get_num_queues(vdev) > VIRTIO_QUEUE_MAX) {
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 5bc769f685..bb91e34594 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1597,8 +1597,18 @@  static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
 
     if (legacy) {
         if (!virtio_legacy_allowed(vdev)) {
-            error_setg(errp, "device is modern-only, use disable-legacy=on");
-            return;
+            /*
+             * To avoid migration issues, we allow legacy mode when legacy
+             * check is disabled in the old machine types (< 5.1).
+             */
+            if (virtio_legacy_check_disabled(vdev)) {
+                warn_report("device is modern-only, but for backward "
+                            "compatibility legacy is allowed");
+            } else {
+                error_setg(errp,
+                           "device is modern-only, use disable-legacy=on");
+                return;
+            }
         }
         if (virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM)) {
             error_setg(errp, "VIRTIO_F_IOMMU_PLATFORM was supported by"
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index e983025217..b85277da67 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -3304,6 +3304,11 @@  bool virtio_legacy_allowed(VirtIODevice *vdev)
     }
 }
 
+bool virtio_legacy_check_disabled(VirtIODevice *vdev)
+{
+    return vdev->disable_legacy_check;
+}
+
 hwaddr virtio_queue_get_desc_addr(VirtIODevice *vdev, int n)
 {
     return vdev->vq[n].vring.desc;
@@ -3713,6 +3718,8 @@  static Property virtio_properties[] = {
     DEFINE_VIRTIO_COMMON_FEATURES(VirtIODevice, host_features),
     DEFINE_PROP_BOOL("use-started", VirtIODevice, use_started, true),
     DEFINE_PROP_BOOL("use-disabled-flag", VirtIODevice, use_disabled_flag, true),
+    DEFINE_PROP_BOOL("x-disable-legacy-check", VirtIODevice,
+                     disable_legacy_check, false),
     DEFINE_PROP_END_OF_LIST(),
 };