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

[Stefano Garzarella]

On Thu, Sep 17, 2020 at 11:00:00AM +0100, Dr. David Alan Gilbert wrote:
> * Cornelia Huck (cohuck@redhat.com) wrote:
> > On Thu, 17 Sep 2020 10:48:28 +0200
> > Stefano Garzarella <sgarzare@redhat.com> wrote:
> > 
> > > 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?
> > 
> > I was thinking about adding a new virtio_legacy_check flag into
> > MachineClass to get a machine-wide config and avoid introducing a new
> > config knob. The drawback is that every machine type supporting compat
> > machines would need to take care about disabling the check in their
> > 5.1-or-older machines themselves.

Thank you for clarifying!

> > 
> > Not sure what the preferable solution is; I'm not really opposed to
> > your approach, though.
> 
> I think a device property is fine - they make it easy to add it to the
> global compat list.
>

Okay, so I'll leave the device property.


I also need to update a series [1] that I sent to force virtio version 1
on vhost-vsock devices.
Also in this case I need to care about migration and force it only on new
machine types.

Do you think I can reuse the same property also in vhost-vsock-pci and
vhost-vsock-ccw to force virtio version 1, or it is better to add a new
property for each device.

The two things (disable legacy check and force version 1) are related,
so maybe I can use a single property in the virtio-device class,

Thanks,
Stefano

[1] https://lists.nongnu.org/archive/html/qemu-devel/2020-08/msg04437.html