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

[Cornelia Huck]

On Mon, 21 Sep 2020 10:38:04 +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 does not support legacy.
> 
> Unfortunately some devices were wrongly declared legacy capable 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
> instead, 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>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
> v4:
>  - fixed commit message and warning message [Cornelia]
> 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/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index 8feb3451a0..c534cdf2e5 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 requires revision >= 1, but for backward "
> +                        "compatibility max_revision=0 is allowed");

Message looks good to me.

> +        } 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) {

(...)