On Tue, 15 Sep 2020 15:05:14 +0200
Stefano Garzarella <sgarz...@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-sta...@nongnu.org
> Fixes: 9b3a35ec82 ("virtio: verify that legacy support is not accidentally
> on")
> Suggested-by: Dr. David Alan Gilbert <dgilb...@redhat.com>
> Suggested-by: Cornelia Huck <coh...@redhat.com>
> Signed-off-by: Stefano Garzarella <sgarz...@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(),
> };
>
