* [PATCH v2] virtio: skip legacy support check on machine types less than 5.1
@ 2020-09-15 13:05 Stefano Garzarella
2020-09-16 7:32 ` Stefano Garzarella
2020-09-16 9:08 ` Cornelia Huck
0 siblings, 2 replies; 9+ messages in thread
From: Stefano Garzarella @ 2020-09-15 13:05 UTC (permalink / raw)
To: qemu-devel
Cc: Eduardo Habkost, Cornelia Huck, Dr. David Alan Gilbert,
Michael S. Tsirkin
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2] virtio: skip legacy support check on machine types less than 5.1
2020-09-15 13:05 [PATCH v2] virtio: skip legacy support check on machine types less than 5.1 Stefano Garzarella
@ 2020-09-16 7:32 ` Stefano Garzarella
2020-09-16 9:08 ` Cornelia Huck
1 sibling, 0 replies; 9+ messages in thread
From: Stefano Garzarella @ 2020-09-16 7:32 UTC (permalink / raw)
To: qemu-devel
Cc: Cornelia Huck, Michael S. Tsirkin, qemu-stable,
Dr. David Alan Gilbert, Eduardo Habkost
+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
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] virtio: skip legacy support check on machine types less than 5.1
2020-09-15 13:05 [PATCH v2] virtio: skip legacy support check on machine types less than 5.1 Stefano Garzarella
2020-09-16 7:32 ` Stefano Garzarella
@ 2020-09-16 9:08 ` Cornelia Huck
2020-09-17 8:48 ` Stefano Garzarella
1 sibling, 1 reply; 9+ messages in thread
From: Cornelia Huck @ 2020-09-16 9:08 UTC (permalink / raw)
To: Stefano Garzarella
Cc: Eduardo Habkost, qemu-devel, Dr. David Alan Gilbert, Michael S. Tsirkin
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(),
> };
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] virtio: skip legacy support check on machine types less than 5.1
2020-09-16 9:08 ` Cornelia Huck
@ 2020-09-17 8:48 ` Stefano Garzarella
2020-09-17 9:22 ` Cornelia Huck
0 siblings, 1 reply; 9+ messages in thread
From: Stefano Garzarella @ 2020-09-17 8:48 UTC (permalink / raw)
To: Cornelia Huck
Cc: Eduardo Habkost, qemu-devel, Dr. David Alan Gilbert, Michael S. Tsirkin
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] virtio: skip legacy support check on machine types less than 5.1
2020-09-17 8:48 ` Stefano Garzarella
@ 2020-09-17 9:22 ` Cornelia Huck
2020-09-17 10:00 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 9+ messages in thread
From: Cornelia Huck @ 2020-09-17 9:22 UTC (permalink / raw)
To: Stefano Garzarella
Cc: Eduardo Habkost, qemu-devel, Dr. David Alan Gilbert, Michael S. Tsirkin
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] virtio: skip legacy support check on machine types less than 5.1
2020-09-17 9:22 ` Cornelia Huck
@ 2020-09-17 10:00 ` Dr. David Alan Gilbert
2020-09-17 10:47 ` Stefano Garzarella
0 siblings, 1 reply; 9+ messages in thread
From: Dr. David Alan Gilbert @ 2020-09-17 10:00 UTC (permalink / raw)
To: Cornelia Huck
Cc: Eduardo Habkost, Michael S. Tsirkin, qemu-devel, Stefano Garzarella
* 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] virtio: skip legacy support check on machine types less than 5.1
2020-09-17 10:00 ` Dr. David Alan Gilbert
@ 2020-09-17 10:47 ` Stefano Garzarella
2020-09-17 11:00 ` Cornelia Huck
0 siblings, 1 reply; 9+ messages in thread
From: Stefano Garzarella @ 2020-09-17 10:47 UTC (permalink / raw)
To: Dr. David Alan Gilbert, Cornelia Huck
Cc: Eduardo Habkost, qemu-devel, Michael S. Tsirkin
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] virtio: skip legacy support check on machine types less than 5.1
2020-09-17 10:47 ` Stefano Garzarella
@ 2020-09-17 11:00 ` Cornelia Huck
2020-09-17 12:09 ` Stefano Garzarella
0 siblings, 1 reply; 9+ messages in thread
From: Cornelia Huck @ 2020-09-17 11:00 UTC (permalink / raw)
To: Stefano Garzarella
Cc: Eduardo Habkost, Michael S. Tsirkin, Dr. David Alan Gilbert, qemu-devel
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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] virtio: skip legacy support check on machine types less than 5.1
2020-09-17 11:00 ` Cornelia Huck
@ 2020-09-17 12:09 ` Stefano Garzarella
0 siblings, 0 replies; 9+ messages in thread
From: Stefano Garzarella @ 2020-09-17 12:09 UTC (permalink / raw)
To: Cornelia Huck
Cc: Eduardo Habkost, Michael S. Tsirkin, Dr. David Alan Gilbert, qemu-devel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-09-17 12:10 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-15 13:05 [PATCH v2] virtio: skip legacy support check on machine types less than 5.1 Stefano Garzarella
2020-09-16 7:32 ` Stefano Garzarella
2020-09-16 9:08 ` Cornelia Huck
2020-09-17 8:48 ` Stefano Garzarella
2020-09-17 9:22 ` Cornelia Huck
2020-09-17 10:00 ` Dr. David Alan Gilbert
2020-09-17 10:47 ` Stefano Garzarella
2020-09-17 11:00 ` Cornelia Huck
2020-09-17 12:09 ` Stefano Garzarella
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.