[libvirt] [PATCH 0/2] qemu: Fixes for (non-)transitional virtio devices
Andrea Bolognani (2): qemu: Improve validation for virtio input devices docs: Document configuration quirks for virtio devices docs/formatdomain.html.in | 18 +++++++++++ src/libvirt_private.syms | 1 + src/qemu/qemu_domain.c | 32 +++++++++++++++++++ .../virtio-transitional-not-supported.xml | 11 +++++++ tests/qemuxml2argvtest.c | 4 +++ 5 files changed, 66 insertions(+) create mode 100644 tests/qemuxml2argvdata/virtio-transitional-not-supported.xml -- 2.20.1
While the parser and schema have to accept all possible models, virtio-(non-)transitional models are only applicable to type=passthrough and should be otherwise rejected. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/libvirt_private.syms | 1 + src/qemu/qemu_domain.c | 32 +++++++++++++++++++ .../virtio-transitional-not-supported.xml | 11 +++++++ tests/qemuxml2argvtest.c | 4 +++ 4 files changed, 48 insertions(+) create mode 100644 tests/qemuxml2argvdata/virtio-transitional-not-supported.xml diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 69643732e0..2645e55988 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -419,6 +419,7 @@ virDomainInputBusTypeToString; virDomainInputDefFind; virDomainInputDefFree; virDomainInputDefGetPath; +virDomainInputTypeToString; virDomainIOMMUModelTypeFromString; virDomainIOMMUModelTypeToString; virDomainIOThreadIDAdd; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 1487268a89..f37e6cf81a 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -5971,6 +5971,38 @@ qemuDomainDeviceDefValidateInput(const virDomainInputDef *input, if (input->bus != VIR_DOMAIN_INPUT_BUS_VIRTIO) return 0; + /* Only type=passthrough supports model=virtio-(non-)transitional */ + switch ((virDomainInputModel)input->model) { + case VIR_DOMAIN_INPUT_MODEL_VIRTIO_TRANSITIONAL: + case VIR_DOMAIN_INPUT_MODEL_VIRTIO_NON_TRANSITIONAL: + switch ((virDomainInputType)input->type) { + case VIR_DOMAIN_INPUT_TYPE_MOUSE: + case VIR_DOMAIN_INPUT_TYPE_TABLET: + case VIR_DOMAIN_INPUT_TYPE_KBD: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("virtio (non-)transitional models are not " + "supported for input type=%s"), + virDomainInputTypeToString(input->type)); + return -1; + case VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH: + break; + case VIR_DOMAIN_INPUT_TYPE_LAST: + default: + virReportEnumRangeError(virDomainInputType, + input->type); + return -1; + } + break; + case VIR_DOMAIN_INPUT_MODEL_VIRTIO: + case VIR_DOMAIN_INPUT_MODEL_DEFAULT: + break; + case VIR_DOMAIN_INPUT_MODEL_LAST: + default: + virReportEnumRangeError(virDomainInputModel, + input->model); + return -1; + } + switch ((virDomainInputType)input->type) { case VIR_DOMAIN_INPUT_TYPE_MOUSE: baseName = "virtio-mouse"; diff --git a/tests/qemuxml2argvdata/virtio-transitional-not-supported.xml b/tests/qemuxml2argvdata/virtio-transitional-not-supported.xml new file mode 100644 index 0000000000..881d91d5a6 --- /dev/null +++ b/tests/qemuxml2argvdata/virtio-transitional-not-supported.xml @@ -0,0 +1,11 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <os> + <type arch='x86_64' machine='q35'>hvm</type> + </os> + <devices> + <input type='keyboard' bus='virtio' model='virtio-non-transitional'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index de9eb6abdb..113ad54b9c 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -3001,6 +3001,10 @@ mymain(void) DO_TEST_CAPS_VER("virtio-non-transitional", "3.1.0"); DO_TEST_CAPS_LATEST("virtio-transitional"); DO_TEST_CAPS_LATEST("virtio-non-transitional"); + DO_TEST_PARSE_ERROR("virtio-transitional-not-supported", + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, + QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_IOH3420); /* Simple headless guests for various architectures */ DO_TEST_CAPS_ARCH_LATEST("aarch64-virt-headless", "aarch64"); -- 2.20.1
On 3/6/19 7:17 AM, Andrea Bolognani wrote:
While the parser and schema have to accept all possible models, virtio-(non-)transitional models are only applicable to type=passthrough and should be otherwise rejected.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/libvirt_private.syms | 1 + src/qemu/qemu_domain.c | 32 +++++++++++++++++++ .../virtio-transitional-not-supported.xml | 11 +++++++ tests/qemuxml2argvtest.c | 4 +++ 4 files changed, 48 insertions(+) create mode 100644 tests/qemuxml2argvdata/virtio-transitional-not-supported.xml
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 69643732e0..2645e55988 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -419,6 +419,7 @@ virDomainInputBusTypeToString; virDomainInputDefFind; virDomainInputDefFree; virDomainInputDefGetPath; +virDomainInputTypeToString; virDomainIOMMUModelTypeFromString; virDomainIOMMUModelTypeToString; virDomainIOThreadIDAdd; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 1487268a89..f37e6cf81a 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -5971,6 +5971,38 @@ qemuDomainDeviceDefValidateInput(const virDomainInputDef *input, if (input->bus != VIR_DOMAIN_INPUT_BUS_VIRTIO) return 0;
+ /* Only type=passthrough supports model=virtio-(non-)transitional */ + switch ((virDomainInputModel)input->model) { + case VIR_DOMAIN_INPUT_MODEL_VIRTIO_TRANSITIONAL: + case VIR_DOMAIN_INPUT_MODEL_VIRTIO_NON_TRANSITIONAL: + switch ((virDomainInputType)input->type) { + case VIR_DOMAIN_INPUT_TYPE_MOUSE: + case VIR_DOMAIN_INPUT_TYPE_TABLET: + case VIR_DOMAIN_INPUT_TYPE_KBD: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("virtio (non-)transitional models are not " + "supported for input type=%s"), + virDomainInputTypeToString(input->type)); + return -1; + case VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH: + break; + case VIR_DOMAIN_INPUT_TYPE_LAST: + default: + virReportEnumRangeError(virDomainInputType, + input->type); + return -1; + } + break; + case VIR_DOMAIN_INPUT_MODEL_VIRTIO: + case VIR_DOMAIN_INPUT_MODEL_DEFAULT: + break; + case VIR_DOMAIN_INPUT_MODEL_LAST: + default: + virReportEnumRangeError(virDomainInputModel, + input->model); + return -1; + } + switch ((virDomainInputType)input->type) { case VIR_DOMAIN_INPUT_TYPE_MOUSE: baseName = "virtio-mouse"; diff --git a/tests/qemuxml2argvdata/virtio-transitional-not-supported.xml b/tests/qemuxml2argvdata/virtio-transitional-not-supported.xml new file mode 100644 index 0000000000..881d91d5a6 --- /dev/null +++ b/tests/qemuxml2argvdata/virtio-transitional-not-supported.xml @@ -0,0 +1,11 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <os> + <type arch='x86_64' machine='q35'>hvm</type> + </os> + <devices> + <input type='keyboard' bus='virtio' model='virtio-non-transitional'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index de9eb6abdb..113ad54b9c 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -3001,6 +3001,10 @@ mymain(void) DO_TEST_CAPS_VER("virtio-non-transitional", "3.1.0"); DO_TEST_CAPS_LATEST("virtio-transitional"); DO_TEST_CAPS_LATEST("virtio-non-transitional"); + DO_TEST_PARSE_ERROR("virtio-transitional-not-supported", + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, + QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_IOH3420);
/* Simple headless guests for various architectures */ DO_TEST_CAPS_ARCH_LATEST("aarch64-virt-headless", "aarch64");
I'm thoroughly 'meh' to the idea of adding XML tests to exercise validation failures like this, but only enough to make this comment Reviewed-by: Cole Robinson <crobinso@redhat.com> - Cole
On Fri, 2019-03-08 at 15:05 -0500, Cole Robinson wrote:
On 3/6/19 7:17 AM, Andrea Bolognani wrote:
+ DO_TEST_PARSE_ERROR("virtio-transitional-not-supported", + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, + QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_IOH3420);
I'm thoroughly 'meh' to the idea of adding XML tests to exercise validation failures like this, but only enough to make this comment
I respectfully disagree: having test coverage for the negative cases is very useful to stop regressions from sneaking back in. Anyway, I've fixed the typo you spotted and pushed the series. Thanks for the review! :) -- Andrea Bolognani / Red Hat / Virtualization
Some devices (namely virtio-scsi, virtio-gpu, virtio-keyboard, virtio-tablet and virtio-mouse, plus virtio-crypto which is not supported by libvirt) don't follow the same rules as all other virtio devices, which is something that ought to be documented. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- docs/formatdomain.html.in | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index eb00c01d96..a9a8cfcd46 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -4132,6 +4132,24 @@ </dd> </dl> + <p> + While the information outlined above apply to most virtio devices, + there are a few exceptions: + </p> + + <ul> + <li> + for SCSI controllers, <code>virtio-scsi</code> must be used instead + of <code>virtio</code> for backwards compatibility reasons; + </li> + <li> + some devices, such as GPUs and input devices (keyboard, tablet and + mouse), are only defined in the virtio 1.0 spec and as such don't + have a transitional variant: the only accepted model is + <code>virtio</code>, which will result in a non-transitional device. + </li> + </ul> + <p> For more details see the <a href="https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg00923.html">qemu patch posting</a> and the -- 2.20.1
On 3/6/19 7:17 AM, Andrea Bolognani wrote:
Some devices (namely virtio-scsi, virtio-gpu, virtio-keyboard, virtio-tablet and virtio-mouse, plus virtio-crypto which is not supported by libvirt) don't follow the same rules as all other virtio devices, which is something that ought to be documented.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- docs/formatdomain.html.in | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index eb00c01d96..a9a8cfcd46 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -4132,6 +4132,24 @@ </dd> </dl>
+ <p> + While the information outlined above apply to most virtio devices,
*applies
+ there are a few exceptions: + </p> + + <ul> + <li> + for SCSI controllers, <code>virtio-scsi</code> must be used instead + of <code>virtio</code> for backwards compatibility reasons; + </li> + <li> + some devices, such as GPUs and input devices (keyboard, tablet and + mouse), are only defined in the virtio 1.0 spec and as such don't + have a transitional variant: the only accepted model is + <code>virtio</code>, which will result in a non-transitional device. + </li> + </ul> + <p> For more details see the <a href="https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg00923.html">qemu patch posting</a> and the
Reviewed-by: Cole Robinson <crobinso@redhat.com> - Cole
participants (2)
-
Andrea Bolognani -
Cole Robinson