On Fri, Sep 18, 2020 at 11:13:42AM +0200, Cornelia Huck wrote:
> On Fri, 18 Sep 2020 09:47:07 +0200
> Stefano Garzarella <sgarz...@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-sta...@nongnu.org
> > Fixes: 9b3a35ec82 ("virtio: verify that legacy support is not accidentally
> > on")
> > Suggested-by: Dr. David Alan Gilbert <dgilb...@redhat.com>
> > Suggested-by: Cornelia Huck <coh...@redhat.com>
> > Signed-off-by: Stefano Garzarella <sgarz...@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 <coh...@redhat.com>
>
Thanks,
Stefano
