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

[Cornelia Huck]

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>