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

[Stefano Garzarella]

On Mon, Sep 21, 2020 at 11:40:54AM +0200, Cornelia Huck wrote:
> 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");

I forgot to mention that I changed 'max_rev' to 'max_revision' since it
is the parameter that the user can touch.

> 
> Message looks good to me.

Thanks!

Stefano

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