Re: [PATCH v2] virtio: skip legacy support check on machine types less than 5.1

[Stefano Garzarella]

On Wed, Sep 16, 2020 at 11:08:48AM +0200, Cornelia Huck wrote:
> On Tue, 15 Sep 2020 15:05:14 +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.
> > 
> > Unfortunately some devices were wrongly declared legacy even if they
> > were not (e.g vhost-vsock).
> > 
> > To avoid migration issues, we disable this error for machine types < 5.1,
> > but we print a warning.
> > 
> > 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>
> > ---
> > v2:
> >  - fixed Cornelia's e-mail address
> > ---
> >  include/hw/virtio/virtio.h |  1 +
> >  hw/core/machine.c          |  1 +
> >  hw/virtio/virtio.c         | 18 ++++++++++++++++--
> >  3 files changed, 18 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> > index 807280451b..ed7cee348b 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;
> > 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" },
> 
> Hm... not sure if we actually should add a new device property for
> that. Maybe we can use a flag in the base machine type instead?

I am not very experienced with machine types.
I used the device property to easily access it from virtio devices.

Please, can you give me some suggestions where to look for the flags?

> 
> >  };
> >  const size_t hw_compat_5_0_len = G_N_ELEMENTS(hw_compat_5_0);
> >  
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index e983025217..30ccc43b8c 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -3287,6 +3287,8 @@ void virtio_init(VirtIODevice *vdev, const char *name,
> >   */
> >  bool virtio_legacy_allowed(VirtIODevice *vdev)
> >  {
> > +    bool ret = false;
> > +
> >      switch (vdev->device_id) {
> >      case VIRTIO_ID_NET:
> >      case VIRTIO_ID_BLOCK:
> > @@ -3298,10 +3300,20 @@ bool virtio_legacy_allowed(VirtIODevice *vdev)
> >      case VIRTIO_ID_9P:
> >      case VIRTIO_ID_RPROC_SERIAL:
> >      case VIRTIO_ID_CAIF:
> > +        ret = true;
> > +    }
> > +
> > +    /*
> > +     * For backward compatibility, we allow legacy mode with old machine 
> > types
> > +     * to get the migration working.
> > +     */
> > +    if (!ret && vdev->disable_legacy_check) {
> > +        warn_report("device is modern-only, but for backward compatibility 
> > "
> > +                    "legacy is allowed");
> 
> I don't think we should warn in the function that returns whether
> legacy is allowed or not. Future code might want to call this function
> for other purposes, and returning true with a warning is not that
> useful for code that wants to find out whether legacy is supported for
> a type in principle.
> 
> So even though it is more work, I think we should move this warning to
> the code that is actively trying to fence off misconfigured devices.

Yes, I got it. At first I did so, but then I wanted to reduce the
duplicate code.

But you're right, it's ugly.
I'll send a v3.

Thanks,
Stefano