* [PATCH v3 1/4] virtio: skip legacy support check on machine types less than 5.1
2020-09-18 7:47 [PATCH v3 0/4] vhost-vsock: force virtio version 1 Stefano Garzarella
@ 2020-09-18 7:47 ` Stefano Garzarella
2020-09-18 9:13 ` Cornelia Huck
2020-09-18 7:47 ` [PATCH v3 2/4] vhost-vsock-pci: force virtio version 1 Stefano Garzarella
` (3 subsequent siblings)
4 siblings, 1 reply; 13+ messages in thread
From: Stefano Garzarella @ 2020-09-18 7:47 UTC (permalink / raw)
To: qemu-devel, Michael S. Tsirkin, Dr. David Alan Gilbert,
Cornelia Huck
Cc: Thomas Huth, Qinghua Cheng, Eduardo Habkost, David Hildenbrand,
qemu-stable, Halil Pasic, Christian Borntraeger, qemu-s390x,
Richard Henderson, Qian Cai
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 add a virtio-device property
(x-disable-legacy-check) to skip the legacy error, printing a warning
instaed, for machine types < 5.1.
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>
---
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");
+ } 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) {
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 5bc769f685..bb91e34594 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1597,8 +1597,18 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
if (legacy) {
if (!virtio_legacy_allowed(vdev)) {
- error_setg(errp, "device is modern-only, use disable-legacy=on");
- 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");
+ } else {
+ error_setg(errp,
+ "device is modern-only, use disable-legacy=on");
+ return;
+ }
}
if (virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM)) {
error_setg(errp, "VIRTIO_F_IOMMU_PLATFORM was supported by"
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index e983025217..b85277da67 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -3304,6 +3304,11 @@ bool virtio_legacy_allowed(VirtIODevice *vdev)
}
}
+bool virtio_legacy_check_disabled(VirtIODevice *vdev)
+{
+ return vdev->disable_legacy_check;
+}
+
hwaddr virtio_queue_get_desc_addr(VirtIODevice *vdev, int n)
{
return vdev->vq[n].vring.desc;
@@ -3713,6 +3718,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] 13+ messages in thread* Re: [PATCH v3 1/4] virtio: skip legacy support check on machine types less than 5.1
2020-09-18 7:47 ` [PATCH v3 1/4] virtio: skip legacy support check on machine types less than 5.1 Stefano Garzarella
@ 2020-09-18 9:13 ` Cornelia Huck
2020-09-18 9:35 ` Stefano Garzarella
0 siblings, 1 reply; 13+ messages in thread
From: Cornelia Huck @ 2020-09-18 9:13 UTC (permalink / raw)
To: Stefano Garzarella
Cc: Thomas Huth, Qinghua Cheng, Eduardo Habkost, Michael S. Tsirkin,
qemu-stable, David Hildenbrand, qemu-devel,
Dr. David Alan Gilbert, Halil Pasic, Christian Borntraeger,
qemu-s390x, Richard Henderson, Qian Cai
On Fri, 18 Sep 2020 09:47:07 +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.
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/
>
> 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>
> ---
> 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.
(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 <cohuck@redhat.com>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v3 1/4] virtio: skip legacy support check on machine types less than 5.1
2020-09-18 9:13 ` Cornelia Huck
@ 2020-09-18 9:35 ` Stefano Garzarella
0 siblings, 0 replies; 13+ messages in thread
From: Stefano Garzarella @ 2020-09-18 9:35 UTC (permalink / raw)
To: Cornelia Huck
Cc: Thomas Huth, Qinghua Cheng, Eduardo Habkost, Michael S. Tsirkin,
qemu-stable, David Hildenbrand, qemu-devel,
Dr. David Alan Gilbert, Halil Pasic, Christian Borntraeger,
qemu-s390x, Richard Henderson, Qian Cai
On Fri, Sep 18, 2020 at 11:13:42AM +0200, Cornelia Huck wrote:
> On Fri, 18 Sep 2020 09:47:07 +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.
>
> 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-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>
> > ---
> > 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 <cohuck@redhat.com>
>
Thanks,
Stefano
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3 2/4] vhost-vsock-pci: force virtio version 1
2020-09-18 7:47 [PATCH v3 0/4] vhost-vsock: force virtio version 1 Stefano Garzarella
2020-09-18 7:47 ` [PATCH v3 1/4] virtio: skip legacy support check on machine types less than 5.1 Stefano Garzarella
@ 2020-09-18 7:47 ` Stefano Garzarella
2020-09-18 9:19 ` Cornelia Huck
2020-09-18 7:47 ` [PATCH v3 3/4] vhost-user-vsock-pci: " Stefano Garzarella
` (2 subsequent siblings)
4 siblings, 1 reply; 13+ messages in thread
From: Stefano Garzarella @ 2020-09-18 7:47 UTC (permalink / raw)
To: qemu-devel, Michael S. Tsirkin, Dr. David Alan Gilbert,
Cornelia Huck
Cc: Thomas Huth, Qinghua Cheng, Eduardo Habkost, David Hildenbrand,
qemu-stable, Halil Pasic, Christian Borntraeger, qemu-s390x,
Richard Henderson, Qian Cai
Commit 9b3a35ec82 ("virtio: verify that legacy support is not
accidentally on") added a safety check that requires to set
'disable-legacy=on' on vhost-vsock-pci device:
$ ./qemu-system-x86_64 ... -device vhost-vsock-pci,guest-cid=5
qemu-system-x86_64: -device vhost-vsock-pci,guest-cid=5:
device is modern-only, use disable-legacy=on
virtio-vsock was introduced after the release of VIRTIO 1.0
specifications, so it should be 'modern-only'.
In addition Cornelia verified that forcing a legacy mode on
vhost-vsock-pci device using x86-64 host and s390x guest, so with
different endianness, produces strange behaviours.
This patch forces virtio version 1 and removes the 'transitional_name'
property removing the need to specify 'disable-legacy=on' on
vhost-vsock-pci device.
To avoid migration issues, we force virtio version 1 only when
legacy check is enabled in the new machine types (>= 5.1).
Cc: qemu-stable@nongnu.org
Reported-by: Qian Cai <caiqian@redhat.com>
Reported-by: Qinghua Cheng <qcheng@redhat.com>
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1868449
Suggested-by: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
v3:
- forced virtio version 1 only with new machine types
v2:
- fixed commit message [Cornelia]
---
hw/virtio/vhost-vsock-pci.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/hw/virtio/vhost-vsock-pci.c b/hw/virtio/vhost-vsock-pci.c
index e56067b427..205da8d1f5 100644
--- a/hw/virtio/vhost-vsock-pci.c
+++ b/hw/virtio/vhost-vsock-pci.c
@@ -44,6 +44,15 @@ static void vhost_vsock_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
{
VHostVSockPCI *dev = VHOST_VSOCK_PCI(vpci_dev);
DeviceState *vdev = DEVICE(&dev->vdev);
+ VirtIODevice *virtio_dev = VIRTIO_DEVICE(vdev);
+
+ /*
+ * To avoid migration issues, we force virtio version 1 only when
+ * legacy check is enabled in the new machine types (>= 5.1).
+ */
+ if (!virtio_legacy_check_disabled(virtio_dev)) {
+ virtio_pci_force_virtio_1(vpci_dev);
+ }
qdev_realize(vdev, BUS(&vpci_dev->bus), errp);
}
@@ -73,7 +82,6 @@ static void vhost_vsock_pci_instance_init(Object *obj)
static const VirtioPCIDeviceTypeInfo vhost_vsock_pci_info = {
.base_name = TYPE_VHOST_VSOCK_PCI,
.generic_name = "vhost-vsock-pci",
- .transitional_name = "vhost-vsock-pci-transitional",
.non_transitional_name = "vhost-vsock-pci-non-transitional",
.instance_size = sizeof(VHostVSockPCI),
.instance_init = vhost_vsock_pci_instance_init,
--
2.26.2
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v3 2/4] vhost-vsock-pci: force virtio version 1
2020-09-18 7:47 ` [PATCH v3 2/4] vhost-vsock-pci: force virtio version 1 Stefano Garzarella
@ 2020-09-18 9:19 ` Cornelia Huck
2020-09-18 10:00 ` Stefano Garzarella
0 siblings, 1 reply; 13+ messages in thread
From: Cornelia Huck @ 2020-09-18 9:19 UTC (permalink / raw)
To: Stefano Garzarella
Cc: Thomas Huth, Qinghua Cheng, Eduardo Habkost, Michael S. Tsirkin,
qemu-stable, David Hildenbrand, qemu-devel,
Dr. David Alan Gilbert, Halil Pasic, Christian Borntraeger,
qemu-s390x, Richard Henderson, Qian Cai
On Fri, 18 Sep 2020 09:47:08 +0200
Stefano Garzarella <sgarzare@redhat.com> wrote:
> Commit 9b3a35ec82 ("virtio: verify that legacy support is not
> accidentally on") added a safety check that requires to set
> 'disable-legacy=on' on vhost-vsock-pci device:
>
> $ ./qemu-system-x86_64 ... -device vhost-vsock-pci,guest-cid=5
> qemu-system-x86_64: -device vhost-vsock-pci,guest-cid=5:
> device is modern-only, use disable-legacy=on
>
> virtio-vsock was introduced after the release of VIRTIO 1.0
> specifications, so it should be 'modern-only'.
> In addition Cornelia verified that forcing a legacy mode on
> vhost-vsock-pci device using x86-64 host and s390x guest, so with
> different endianness, produces strange behaviours.
>
> This patch forces virtio version 1 and removes the 'transitional_name'
> property removing the need to specify 'disable-legacy=on' on
> vhost-vsock-pci device.
>
> To avoid migration issues, we force virtio version 1 only when
> legacy check is enabled in the new machine types (>= 5.1).
>
> Cc: qemu-stable@nongnu.org
> Reported-by: Qian Cai <caiqian@redhat.com>
> Reported-by: Qinghua Cheng <qcheng@redhat.com>
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1868449
> Suggested-by: Cornelia Huck <cohuck@redhat.com>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
> v3:
> - forced virtio version 1 only with new machine types
> v2:
> - fixed commit message [Cornelia]
> ---
> hw/virtio/vhost-vsock-pci.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/hw/virtio/vhost-vsock-pci.c b/hw/virtio/vhost-vsock-pci.c
> index e56067b427..205da8d1f5 100644
> --- a/hw/virtio/vhost-vsock-pci.c
> +++ b/hw/virtio/vhost-vsock-pci.c
> @@ -44,6 +44,15 @@ static void vhost_vsock_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
> {
> VHostVSockPCI *dev = VHOST_VSOCK_PCI(vpci_dev);
> DeviceState *vdev = DEVICE(&dev->vdev);
> + VirtIODevice *virtio_dev = VIRTIO_DEVICE(vdev);
> +
> + /*
> + * To avoid migration issues, we force virtio version 1 only when
> + * legacy check is enabled in the new machine types (>= 5.1).
> + */
> + if (!virtio_legacy_check_disabled(virtio_dev)) {
> + virtio_pci_force_virtio_1(vpci_dev);
> + }
>
> qdev_realize(vdev, BUS(&vpci_dev->bus), errp);
> }
> @@ -73,7 +82,6 @@ static void vhost_vsock_pci_instance_init(Object *obj)
> static const VirtioPCIDeviceTypeInfo vhost_vsock_pci_info = {
> .base_name = TYPE_VHOST_VSOCK_PCI,
> .generic_name = "vhost-vsock-pci",
> - .transitional_name = "vhost-vsock-pci-transitional",
Hm... this means that vhost-vsock-pci-transitional won't work on compat
machines, which could also lead to migration compatibility issues (I
think?)
Is this way of specifying the device sufficiently uncommon so that we
can ignore that?
> .non_transitional_name = "vhost-vsock-pci-non-transitional",
> .instance_size = sizeof(VHostVSockPCI),
> .instance_init = vhost_vsock_pci_instance_init,
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v3 2/4] vhost-vsock-pci: force virtio version 1
2020-09-18 9:19 ` Cornelia Huck
@ 2020-09-18 10:00 ` Stefano Garzarella
0 siblings, 0 replies; 13+ messages in thread
From: Stefano Garzarella @ 2020-09-18 10:00 UTC (permalink / raw)
To: Cornelia Huck
Cc: Thomas Huth, Qinghua Cheng, Eduardo Habkost, Michael S. Tsirkin,
qemu-stable, David Hildenbrand, qemu-devel,
Dr. David Alan Gilbert, Halil Pasic, Christian Borntraeger,
qemu-s390x, Richard Henderson, Qian Cai
On Fri, Sep 18, 2020 at 11:19:01AM +0200, Cornelia Huck wrote:
> On Fri, 18 Sep 2020 09:47:08 +0200
> Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> > Commit 9b3a35ec82 ("virtio: verify that legacy support is not
> > accidentally on") added a safety check that requires to set
> > 'disable-legacy=on' on vhost-vsock-pci device:
> >
> > $ ./qemu-system-x86_64 ... -device vhost-vsock-pci,guest-cid=5
> > qemu-system-x86_64: -device vhost-vsock-pci,guest-cid=5:
> > device is modern-only, use disable-legacy=on
> >
> > virtio-vsock was introduced after the release of VIRTIO 1.0
> > specifications, so it should be 'modern-only'.
> > In addition Cornelia verified that forcing a legacy mode on
> > vhost-vsock-pci device using x86-64 host and s390x guest, so with
> > different endianness, produces strange behaviours.
> >
> > This patch forces virtio version 1 and removes the 'transitional_name'
> > property removing the need to specify 'disable-legacy=on' on
> > vhost-vsock-pci device.
> >
> > To avoid migration issues, we force virtio version 1 only when
> > legacy check is enabled in the new machine types (>= 5.1).
> >
> > Cc: qemu-stable@nongnu.org
> > Reported-by: Qian Cai <caiqian@redhat.com>
> > Reported-by: Qinghua Cheng <qcheng@redhat.com>
> > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1868449
> > Suggested-by: Cornelia Huck <cohuck@redhat.com>
> > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > ---
> > v3:
> > - forced virtio version 1 only with new machine types
> > v2:
> > - fixed commit message [Cornelia]
> > ---
> > hw/virtio/vhost-vsock-pci.c | 10 +++++++++-
> > 1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/virtio/vhost-vsock-pci.c b/hw/virtio/vhost-vsock-pci.c
> > index e56067b427..205da8d1f5 100644
> > --- a/hw/virtio/vhost-vsock-pci.c
> > +++ b/hw/virtio/vhost-vsock-pci.c
> > @@ -44,6 +44,15 @@ static void vhost_vsock_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
> > {
> > VHostVSockPCI *dev = VHOST_VSOCK_PCI(vpci_dev);
> > DeviceState *vdev = DEVICE(&dev->vdev);
> > + VirtIODevice *virtio_dev = VIRTIO_DEVICE(vdev);
> > +
> > + /*
> > + * To avoid migration issues, we force virtio version 1 only when
> > + * legacy check is enabled in the new machine types (>= 5.1).
> > + */
> > + if (!virtio_legacy_check_disabled(virtio_dev)) {
> > + virtio_pci_force_virtio_1(vpci_dev);
> > + }
> >
> > qdev_realize(vdev, BUS(&vpci_dev->bus), errp);
> > }
> > @@ -73,7 +82,6 @@ static void vhost_vsock_pci_instance_init(Object *obj)
> > static const VirtioPCIDeviceTypeInfo vhost_vsock_pci_info = {
> > .base_name = TYPE_VHOST_VSOCK_PCI,
> > .generic_name = "vhost-vsock-pci",
> > - .transitional_name = "vhost-vsock-pci-transitional",
>
> Hm... this means that vhost-vsock-pci-transitional won't work on compat
> machines, which could also lead to migration compatibility issues (I
> think?)
>
Yes, I think you are right.
> Is this way of specifying the device sufficiently uncommon so that we
> can ignore that?
I think it's really uncommon and we can safetly eliminate it, since it
has never been really transitional. But if we want to leave it for
migration compatibility, that's fine with me.
Thanks,
Stefano
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3 3/4] vhost-user-vsock-pci: force virtio version 1
2020-09-18 7:47 [PATCH v3 0/4] vhost-vsock: force virtio version 1 Stefano Garzarella
2020-09-18 7:47 ` [PATCH v3 1/4] virtio: skip legacy support check on machine types less than 5.1 Stefano Garzarella
2020-09-18 7:47 ` [PATCH v3 2/4] vhost-vsock-pci: force virtio version 1 Stefano Garzarella
@ 2020-09-18 7:47 ` Stefano Garzarella
2020-09-18 9:23 ` Cornelia Huck
2020-09-18 7:47 ` [PATCH v3 4/4] vhost-vsock-ccw: " Stefano Garzarella
2020-09-18 8:00 ` [PATCH v3 0/4] vhost-vsock: " no-reply
4 siblings, 1 reply; 13+ messages in thread
From: Stefano Garzarella @ 2020-09-18 7:47 UTC (permalink / raw)
To: qemu-devel, Michael S. Tsirkin, Dr. David Alan Gilbert,
Cornelia Huck
Cc: Thomas Huth, Qinghua Cheng, Eduardo Habkost, David Hildenbrand,
qemu-stable, Halil Pasic, Christian Borntraeger, qemu-s390x,
Richard Henderson, Qian Cai
Commit 9b3a35ec82 ("virtio: verify that legacy support is not
accidentally on") added a safety check that requires to set
'disable-legacy=on' on vhost-user-vsock-pci device:
$ ./qemu-system-x86_64 ... \
-chardev socket,id=char0,reconnect=0,path=/tmp/vhost4.socket \
-device vhost-user-vsock-pci,chardev=char0
qemu-system-x86_64: -device vhost-user-vsock-pci,chardev=char0:
device is modern-only, use disable-legacy=on
virtio-vsock was introduced after the release of VIRTIO 1.0
specifications, so it should be 'modern-only'.
This patch forces virtio version 1 and removes the 'transitional_name'
property, as done for vhost-vsock-pci, removing the need to specify
'disable-legacy=on' on vhost-user-vsock-pci device.
Cc: qemu-stable@nongnu.org
Suggested-by: Cornelia Huck <cohuck@redhat.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
v2:
- fixed commit message [Cornelia]
---
hw/virtio/vhost-user-vsock-pci.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/virtio/vhost-user-vsock-pci.c b/hw/virtio/vhost-user-vsock-pci.c
index 763f89984e..14109121e2 100644
--- a/hw/virtio/vhost-user-vsock-pci.c
+++ b/hw/virtio/vhost-user-vsock-pci.c
@@ -41,6 +41,7 @@ static void vhost_user_vsock_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
VHostUserVSockPCI *dev = VHOST_USER_VSOCK_PCI(vpci_dev);
DeviceState *vdev = DEVICE(&dev->vdev);
+ virtio_pci_force_virtio_1(vpci_dev);
qdev_realize(vdev, BUS(&vpci_dev->bus), errp);
}
@@ -69,7 +70,6 @@ static void vhost_user_vsock_pci_instance_init(Object *obj)
static const VirtioPCIDeviceTypeInfo vhost_user_vsock_pci_info = {
.base_name = TYPE_VHOST_USER_VSOCK_PCI,
.generic_name = "vhost-user-vsock-pci",
- .transitional_name = "vhost-user-vsock-pci-transitional",
.non_transitional_name = "vhost-user-vsock-pci-non-transitional",
.instance_size = sizeof(VHostUserVSockPCI),
.instance_init = vhost_user_vsock_pci_instance_init,
--
2.26.2
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v3 3/4] vhost-user-vsock-pci: force virtio version 1
2020-09-18 7:47 ` [PATCH v3 3/4] vhost-user-vsock-pci: " Stefano Garzarella
@ 2020-09-18 9:23 ` Cornelia Huck
2020-09-18 10:01 ` Stefano Garzarella
0 siblings, 1 reply; 13+ messages in thread
From: Cornelia Huck @ 2020-09-18 9:23 UTC (permalink / raw)
To: Stefano Garzarella
Cc: Thomas Huth, Qinghua Cheng, Eduardo Habkost, Michael S. Tsirkin,
qemu-stable, David Hildenbrand, qemu-devel,
Dr. David Alan Gilbert, Halil Pasic, Christian Borntraeger,
qemu-s390x, Richard Henderson, Qian Cai
On Fri, 18 Sep 2020 09:47:09 +0200
Stefano Garzarella <sgarzare@redhat.com> wrote:
> Commit 9b3a35ec82 ("virtio: verify that legacy support is not
> accidentally on") added a safety check that requires to set
> 'disable-legacy=on' on vhost-user-vsock-pci device:
>
> $ ./qemu-system-x86_64 ... \
> -chardev socket,id=char0,reconnect=0,path=/tmp/vhost4.socket \
> -device vhost-user-vsock-pci,chardev=char0
> qemu-system-x86_64: -device vhost-user-vsock-pci,chardev=char0:
> device is modern-only, use disable-legacy=on
>
> virtio-vsock was introduced after the release of VIRTIO 1.0
> specifications, so it should be 'modern-only'.
>
> This patch forces virtio version 1 and removes the 'transitional_name'
> property, as done for vhost-vsock-pci, removing the need to specify
> 'disable-legacy=on' on vhost-user-vsock-pci device.
>
> Cc: qemu-stable@nongnu.org
> Suggested-by: Cornelia Huck <cohuck@redhat.com>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
> v2:
> - fixed commit message [Cornelia]
> ---
> hw/virtio/vhost-user-vsock-pci.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/virtio/vhost-user-vsock-pci.c b/hw/virtio/vhost-user-vsock-pci.c
> index 763f89984e..14109121e2 100644
> --- a/hw/virtio/vhost-user-vsock-pci.c
> +++ b/hw/virtio/vhost-user-vsock-pci.c
> @@ -41,6 +41,7 @@ static void vhost_user_vsock_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
> VHostUserVSockPCI *dev = VHOST_USER_VSOCK_PCI(vpci_dev);
> DeviceState *vdev = DEVICE(&dev->vdev);
>
> + virtio_pci_force_virtio_1(vpci_dev);
Maybe add a comment
/* unlike vhost-vsock, we do not need to care about pre-5.1 compat */
to help when we look at this in the future?
> qdev_realize(vdev, BUS(&vpci_dev->bus), errp);
> }
>
> @@ -69,7 +70,6 @@ static void vhost_user_vsock_pci_instance_init(Object *obj)
> static const VirtioPCIDeviceTypeInfo vhost_user_vsock_pci_info = {
> .base_name = TYPE_VHOST_USER_VSOCK_PCI,
> .generic_name = "vhost-user-vsock-pci",
> - .transitional_name = "vhost-user-vsock-pci-transitional",
> .non_transitional_name = "vhost-user-vsock-pci-non-transitional",
> .instance_size = sizeof(VHostUserVSockPCI),
> .instance_init = vhost_user_vsock_pci_instance_init,
Otherwise,
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v3 3/4] vhost-user-vsock-pci: force virtio version 1
2020-09-18 9:23 ` Cornelia Huck
@ 2020-09-18 10:01 ` Stefano Garzarella
0 siblings, 0 replies; 13+ messages in thread
From: Stefano Garzarella @ 2020-09-18 10:01 UTC (permalink / raw)
To: Cornelia Huck
Cc: Thomas Huth, Qinghua Cheng, Eduardo Habkost, Michael S. Tsirkin,
qemu-stable, David Hildenbrand, qemu-devel,
Dr. David Alan Gilbert, Halil Pasic, Christian Borntraeger,
qemu-s390x, Richard Henderson, Qian Cai
On Fri, Sep 18, 2020 at 11:23:16AM +0200, Cornelia Huck wrote:
> On Fri, 18 Sep 2020 09:47:09 +0200
> Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> > Commit 9b3a35ec82 ("virtio: verify that legacy support is not
> > accidentally on") added a safety check that requires to set
> > 'disable-legacy=on' on vhost-user-vsock-pci device:
> >
> > $ ./qemu-system-x86_64 ... \
> > -chardev socket,id=char0,reconnect=0,path=/tmp/vhost4.socket \
> > -device vhost-user-vsock-pci,chardev=char0
> > qemu-system-x86_64: -device vhost-user-vsock-pci,chardev=char0:
> > device is modern-only, use disable-legacy=on
> >
> > virtio-vsock was introduced after the release of VIRTIO 1.0
> > specifications, so it should be 'modern-only'.
> >
> > This patch forces virtio version 1 and removes the 'transitional_name'
> > property, as done for vhost-vsock-pci, removing the need to specify
> > 'disable-legacy=on' on vhost-user-vsock-pci device.
> >
> > Cc: qemu-stable@nongnu.org
> > Suggested-by: Cornelia Huck <cohuck@redhat.com>
> > Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > ---
> > v2:
> > - fixed commit message [Cornelia]
> > ---
> > hw/virtio/vhost-user-vsock-pci.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/hw/virtio/vhost-user-vsock-pci.c b/hw/virtio/vhost-user-vsock-pci.c
> > index 763f89984e..14109121e2 100644
> > --- a/hw/virtio/vhost-user-vsock-pci.c
> > +++ b/hw/virtio/vhost-user-vsock-pci.c
> > @@ -41,6 +41,7 @@ static void vhost_user_vsock_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
> > VHostUserVSockPCI *dev = VHOST_USER_VSOCK_PCI(vpci_dev);
> > DeviceState *vdev = DEVICE(&dev->vdev);
> >
> > + virtio_pci_force_virtio_1(vpci_dev);
>
> Maybe add a comment
>
> /* unlike vhost-vsock, we do not need to care about pre-5.1 compat */
>
>
> to help when we look at this in the future?
Sure, I'll add in v4.
>
> > qdev_realize(vdev, BUS(&vpci_dev->bus), errp);
> > }
> >
> > @@ -69,7 +70,6 @@ static void vhost_user_vsock_pci_instance_init(Object *obj)
> > static const VirtioPCIDeviceTypeInfo vhost_user_vsock_pci_info = {
> > .base_name = TYPE_VHOST_USER_VSOCK_PCI,
> > .generic_name = "vhost-user-vsock-pci",
> > - .transitional_name = "vhost-user-vsock-pci-transitional",
> > .non_transitional_name = "vhost-user-vsock-pci-non-transitional",
> > .instance_size = sizeof(VHostUserVSockPCI),
> > .instance_init = vhost_user_vsock_pci_instance_init,
>
> Otherwise,
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>
Thanks,
Stefano
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3 4/4] vhost-vsock-ccw: force virtio version 1
2020-09-18 7:47 [PATCH v3 0/4] vhost-vsock: force virtio version 1 Stefano Garzarella
` (2 preceding siblings ...)
2020-09-18 7:47 ` [PATCH v3 3/4] vhost-user-vsock-pci: " Stefano Garzarella
@ 2020-09-18 7:47 ` Stefano Garzarella
2020-09-18 9:24 ` Cornelia Huck
2020-09-18 8:00 ` [PATCH v3 0/4] vhost-vsock: " no-reply
4 siblings, 1 reply; 13+ messages in thread
From: Stefano Garzarella @ 2020-09-18 7:47 UTC (permalink / raw)
To: qemu-devel, Michael S. Tsirkin, Dr. David Alan Gilbert,
Cornelia Huck
Cc: Thomas Huth, Qinghua Cheng, Eduardo Habkost, David Hildenbrand,
qemu-stable, Halil Pasic, Christian Borntraeger, qemu-s390x,
Richard Henderson, Qian Cai
virtio-vsock was introduced after the release of VIRTIO 1.0
specifications, so it should be 'modern-only'.
This patch forces virtio version 1 as done for vhost-vsock-pci.
To avoid migration issues, we force virtio version 1 only when
legacy check is enabled in the new machine types (>= 5.1).
Cc: qemu-stable@nongnu.org
Suggested-by: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
v3:
- forced virtio version 1 only with new machine types
---
hw/s390x/vhost-vsock-ccw.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/hw/s390x/vhost-vsock-ccw.c b/hw/s390x/vhost-vsock-ccw.c
index 0822ecca89..246416a8f9 100644
--- a/hw/s390x/vhost-vsock-ccw.c
+++ b/hw/s390x/vhost-vsock-ccw.c
@@ -40,9 +40,21 @@ static void vhost_vsock_ccw_class_init(ObjectClass *klass, void *data)
static void vhost_vsock_ccw_instance_init(Object *obj)
{
VHostVSockCCWState *dev = VHOST_VSOCK_CCW(obj);
+ VirtioCcwDevice *ccw_dev = VIRTIO_CCW_DEVICE(obj);
+ VirtIODevice *virtio_dev;
virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
TYPE_VHOST_VSOCK);
+
+ virtio_dev = VIRTIO_DEVICE(&dev->vdev);
+
+ /*
+ * To avoid migration issues, we force virtio version 1 only when
+ * legacy check is enabled in the new machine types (>= 5.1).
+ */
+ if (!virtio_legacy_check_disabled(virtio_dev)) {
+ ccw_dev->force_revision_1 = true;
+ }
}
static const TypeInfo vhost_vsock_ccw_info = {
--
2.26.2
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v3 4/4] vhost-vsock-ccw: force virtio version 1
2020-09-18 7:47 ` [PATCH v3 4/4] vhost-vsock-ccw: " Stefano Garzarella
@ 2020-09-18 9:24 ` Cornelia Huck
0 siblings, 0 replies; 13+ messages in thread
From: Cornelia Huck @ 2020-09-18 9:24 UTC (permalink / raw)
To: Stefano Garzarella
Cc: Thomas Huth, Qinghua Cheng, Eduardo Habkost, Michael S. Tsirkin,
qemu-stable, David Hildenbrand, qemu-devel,
Dr. David Alan Gilbert, Halil Pasic, Christian Borntraeger,
qemu-s390x, Richard Henderson, Qian Cai
On Fri, 18 Sep 2020 09:47:10 +0200
Stefano Garzarella <sgarzare@redhat.com> wrote:
> virtio-vsock was introduced after the release of VIRTIO 1.0
> specifications, so it should be 'modern-only'.
>
> This patch forces virtio version 1 as done for vhost-vsock-pci.
>
> To avoid migration issues, we force virtio version 1 only when
> legacy check is enabled in the new machine types (>= 5.1).
>
> Cc: qemu-stable@nongnu.org
> Suggested-by: Cornelia Huck <cohuck@redhat.com>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
> v3:
> - forced virtio version 1 only with new machine types
> ---
> hw/s390x/vhost-vsock-ccw.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 0/4] vhost-vsock: force virtio version 1
2020-09-18 7:47 [PATCH v3 0/4] vhost-vsock: force virtio version 1 Stefano Garzarella
` (3 preceding siblings ...)
2020-09-18 7:47 ` [PATCH v3 4/4] vhost-vsock-ccw: " Stefano Garzarella
@ 2020-09-18 8:00 ` no-reply
4 siblings, 0 replies; 13+ messages in thread
From: no-reply @ 2020-09-18 8:00 UTC (permalink / raw)
To: sgarzare
Cc: thuth, qcheng, ehabkost, mst, qemu-stable, cohuck, david,
qemu-devel, dgilbert, pasic, borntraeger, qemu-s390x, caiqian,
rth
Patchew URL: https://patchew.org/QEMU/20200918074710.27810-1-sgarzare@redhat.com/
Hi,
This series failed build test on FreeBSD host. Please find the details below.
The full log is available at
http://patchew.org/logs/20200918074710.27810-1-sgarzare@redhat.com/testing.FreeBSD/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 13+ messages in thread