diff mbox series

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

Message ID 20200915130514.80989-1-sgarzare@redhat.com
State Superseded
Headers show
Series [v2] virtio: skip legacy support check on machine types less than 5.1 | expand

Commit Message

Stefano Garzarella Sept. 15, 2020, 1:05 p.m. UTC
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(-)

Comments

Stefano Garzarella Sept. 16, 2020, 7:32 a.m. UTC | #1
+Cc qemu-stable@nongnu.org

It was not my day... I forgot to cc stable.

Thanks,
Stefano

On Tue, Sep 15, 2020 at 03:05:14PM +0200, Stefano Garzarella 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" },

>  };

>  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");

>          return true;

> -    default:

> -        return false;

>      }

> +

> +    return ret;

>  }

>  

>  hwaddr virtio_queue_get_desc_addr(VirtIODevice *vdev, int n)

> @@ -3713,6 +3725,8 @@ static Property virtio_properties[] = {

>      DEFINE_VIRTIO_COMMON_FEATURES(VirtIODevice, host_features),

>      DEFINE_PROP_BOOL("use-started", VirtIODevice, use_started, true),

>      DEFINE_PROP_BOOL("use-disabled-flag", VirtIODevice, use_disabled_flag, true),

> +    DEFINE_PROP_BOOL("x-disable-legacy-check", VirtIODevice,

> +                     disable_legacy_check, false),

>      DEFINE_PROP_END_OF_LIST(),

>  };

>  

> -- 

> 2.26.2

> 

>
Cornelia Huck Sept. 16, 2020, 9:08 a.m. UTC | #2
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?

>  };
>  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.

>          return true;
> -    default:
> -        return false;
>      }
> +
> +    return ret;
>  }
>  
>  hwaddr virtio_queue_get_desc_addr(VirtIODevice *vdev, int n)
> @@ -3713,6 +3725,8 @@ static Property virtio_properties[] = {
>      DEFINE_VIRTIO_COMMON_FEATURES(VirtIODevice, host_features),
>      DEFINE_PROP_BOOL("use-started", VirtIODevice, use_started, true),
>      DEFINE_PROP_BOOL("use-disabled-flag", VirtIODevice, use_disabled_flag, true),
> +    DEFINE_PROP_BOOL("x-disable-legacy-check", VirtIODevice,
> +                     disable_legacy_check, false),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>
Stefano Garzarella Sept. 17, 2020, 8:48 a.m. UTC | #3
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
Cornelia Huck Sept. 17, 2020, 9:22 a.m. UTC | #4
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.

Not sure what the preferable solution is; I'm not really opposed to
your approach, though.
Dr. David Alan Gilbert Sept. 17, 2020, 10 a.m. UTC | #5
* 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.

> 

> 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.

Dave

-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Stefano Garzarella Sept. 17, 2020, 10:47 a.m. UTC | #6
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
Cornelia Huck Sept. 17, 2020, 11 a.m. UTC | #7
On Thu, 17 Sep 2020 12:47:21 +0200
Stefano Garzarella <sgarzare@redhat.com> wrote:

> 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,

So, 'x-disable-legacy-check==false' -> 'we need to force version 1'?
Seems reasonable to me.

> 
> Thanks,
> Stefano
> 
> [1] https://lists.nongnu.org/archive/html/qemu-devel/2020-08/msg04437.html
>
Stefano Garzarella Sept. 17, 2020, 12:09 p.m. UTC | #8
On Thu, Sep 17, 2020 at 01:00:08PM +0200, Cornelia Huck wrote:
> On Thu, 17 Sep 2020 12:47:21 +0200
> Stefano Garzarella <sgarzare@redhat.com> wrote:
> 
> > 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,
> 
> So, 'x-disable-legacy-check==false' -> 'we need to force version 1'?

Exaclty :-)

> Seems reasonable to me.

Thanks for the feedback, I'll go this way!

Stefano
diff mbox series

Patch

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" },
 };
 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");
         return true;
-    default:
-        return false;
     }
+
+    return ret;
 }
 
 hwaddr virtio_queue_get_desc_addr(VirtIODevice *vdev, int n)
@@ -3713,6 +3725,8 @@  static Property virtio_properties[] = {
     DEFINE_VIRTIO_COMMON_FEATURES(VirtIODevice, host_features),
     DEFINE_PROP_BOOL("use-started", VirtIODevice, use_started, true),
     DEFINE_PROP_BOOL("use-disabled-flag", VirtIODevice, use_disabled_flag, true),
+    DEFINE_PROP_BOOL("x-disable-legacy-check", VirtIODevice,
+                     disable_legacy_check, false),
     DEFINE_PROP_END_OF_LIST(),
 };