[libvirt] [PATCHv3 00/11] qemu: allow disabling certain virtio revisions
https://bugzilla.redhat.com/show_bug.cgi?id=1227354 v1: https://www.redhat.com/archives/libvir-list/2016-July/msg01235.html v2: https://www.redhat.com/archives/libvir-list/2016-August/msg00412.html * probe for the qemu capability * add the attribute to virtio1-only devices such as virtio-gpu and virtio-input devices * allow multiple revisions to be specified v3: * touch up documentation * rename the capability from "virtio-revision" to "virtio-disable-legacy" * move the formatting in qemuBuildNicDevStr after the address and only do it for virtio * get rid of novelty enum names I left the bitmap usage intact, since enforcing both revision makes sense according to the discussion under: [PATCH 0/6] Use more PCIe, less legacy PCI slots and I considered specifying both features in the XML nicer than adding a new value. Ján Tomko (11): Use separate buffer for <input> subelements Add virtio revision attribute to memballoon Add virtio revision attribute to disks Add virtio revision attribute to controllers Add virtio revision attribute to filesystems Add virtio revision attribute to interfaces Add virtio revision to rng devices Add virtio revision attribute to video Add virtio revision attribute to input devices Introduce QEMU_CAPS_VIRTIO_DISABLE_LEGACY qemu: format options for enforcing virtio revisions docs/formatdomain.html.in | 69 ++++++++++ docs/schemas/domaincommon.rng | 37 ++++++ src/conf/domain_conf.c | 148 +++++++++++++++++++-- src/conf/domain_conf.h | 16 +++ src/qemu/qemu_capabilities.c | 6 + src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 56 ++++++++ tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml | 1 + .../caps_2.6.0-gicv2.aarch64.xml | 1 + .../caps_2.6.0-gicv3.aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml | 1 + tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml | 1 + .../qemuxml2argv-virtio-revision.args | 62 +++++++++ .../qemuxml2argv-virtio-revision.xml | 113 ++++++++++++++++ tests/qemuxml2argvtest.c | 11 ++ .../qemuxml2xmlout-virtio-revision.xml | 113 ++++++++++++++++ tests/qemuxml2xmltest.c | 2 + 19 files changed, 630 insertions(+), 11 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml -- 2.7.3
Instead of figuring out upfront whether <input> will be a single or a pair element, format the subelements into a separate buffer and close <input/> early if this buffer is empty. --- src/conf/domain_conf.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 82876f3..5dbeb1a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -21844,6 +21844,7 @@ virDomainInputDefFormat(virBufferPtr buf, { const char *type = virDomainInputTypeToString(def->type); const char *bus = virDomainInputBusTypeToString(def->bus); + virBuffer childbuf = VIR_BUFFER_INITIALIZER; /* don't format keyboard into migratable XML for backward compatibility */ if (flags & VIR_DOMAIN_DEF_FORMAT_MIGRATABLE && @@ -21866,17 +21867,17 @@ virDomainInputDefFormat(virBufferPtr buf, virBufferAsprintf(buf, "<input type='%s' bus='%s'", type, bus); - if (virDomainDeviceInfoNeedsFormat(&def->info, flags) || - def->type == VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH) { + virBufferAdjustIndent(&childbuf, virBufferGetIndent(buf, false) + 2); + virBufferEscapeString(&childbuf, "<source evdev='%s'/>\n", def->source.evdev); + if (virDomainDeviceInfoFormat(&childbuf, &def->info, flags) < 0) + return -1; + + if (!virBufferUse(&childbuf)) { + virBufferAddLit(buf, "/>\n"); + } else { virBufferAddLit(buf, ">\n"); - virBufferAdjustIndent(buf, 2); - virBufferEscapeString(buf, "<source evdev='%s'/>\n", def->source.evdev); - if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) - return -1; - virBufferAdjustIndent(buf, -2); + virBufferAddBuffer(buf, &childbuf); virBufferAddLit(buf, "</input>\n"); - } else { - virBufferAddLit(buf, "/>\n"); } return 0; -- 2.7.3
<memballoon model='virtio'> <virtio revision='0.9'/> </memballoon> https://bugzilla.redhat.com/show_bug.cgi?id=1227354 --- docs/formatdomain.html.in | 9 +++ docs/schemas/domaincommon.rng | 16 ++++ src/conf/domain_conf.c | 86 ++++++++++++++++++++++ src/conf/domain_conf.h | 9 +++ .../qemuxml2argv-virtio-revision.xml | 54 ++++++++++++++ .../qemuxml2xmlout-virtio-revision.xml | 54 ++++++++++++++ tests/qemuxml2xmltest.c | 2 + 7 files changed, 230 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 5acb3b9..8287b39 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -6415,6 +6415,15 @@ qemu-kvm -net nic,model=? /dev/null <span class='since'>Since 1.1.1, requires QEMU 1.5</span> </p> </dd> + <dt><code>virtio</code></dt> + <dd> + <p> + Optional <code>virtio</code> elements can be used to enforce a particular + virtio revision in QEMU. The valid values for the <code>revision</code> + are <code>0.9</code> and <code>1.0</code>. + <span class='since'>Since 2.2.0</span> + </p> + </dd> </dl> <h4><a name="elementsRng">Random number generator device</a></h4> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 052f28c..64088ba 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3612,6 +3612,9 @@ </attribute> </element> </optional> + <optional> + <ref name="virtioRevision"/> + </optional> </interleave> </element> </define> @@ -4830,6 +4833,19 @@ </element> </define> + <define name="virtioRevision"> + <zeroOrMore> + <element name="virtio"> + <attribute name="revision"> + <choice> + <value>0.9</value> + <value>1.0</value> + </choice> + </attribute> + </element> + </zeroOrMore> + </define> + <define name="usbmaster"> <element name="master"> <attribute name="startport"> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5dbeb1a..b006bc9 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -834,6 +834,12 @@ VIR_ENUM_IMPL(virDomainLoader, "rom", "pflash") +VIR_ENUM_IMPL(virDomainVirtioRevision, + VIR_DOMAIN_VIRTIO_REVISION_LAST, + "default", + "0.9", + "1.0") + /* Internal mapping: subset of block job types that can be present in * <mirror> XML (remaining types are not two-phase). */ VIR_ENUM_DECL(virDomainBlockJob) @@ -1059,6 +1065,81 @@ virDomainXMLOptionGetNamespace(virDomainXMLOptionPtr xmlopt) return &xmlopt->ns; } +static int +virDomainVirtioRevisionParseXML(xmlXPathContextPtr ctxt, + virBitmapPtr *res) +{ + xmlNodePtr save = ctxt->node; + xmlNodePtr *nodes = NULL; + char *str = NULL; + int ret = -1; + size_t i; + int n; + virBitmapPtr revmap = NULL; + + if ((n = virXPathNodeSet("./virtio", ctxt, &nodes)) < 0) + goto cleanup; + + if (n == 0) { + ret = 0; + goto cleanup; + } + + if (!(revmap = virBitmapNew(VIR_DOMAIN_VIRTIO_REVISION_LAST))) + goto cleanup; + + for (i = 0; i < n; i++) { + int val; + + ctxt->node = nodes[i]; + + if (!(str = virXPathString("string(./@revision)", ctxt))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Missing 'revision' attribute in <virtio> element")); + goto cleanup; + } + + if ((val = virDomainVirtioRevisionTypeFromString(str)) <= 0) { + virReportError(VIR_ERR_XML_ERROR, + _("Unable to parse virtio revision: '%s'"), + str); + goto cleanup; + } + + ignore_value(virBitmapSetBit(revmap, val)); + } + + ret = 0; + VIR_STEAL_PTR(*res, revmap); + + cleanup: + ctxt->node = save; + virBitmapFree(revmap); + VIR_FREE(nodes); + VIR_FREE(str); + return ret; +} + + +static void +virDomainVirtioRevisionFormatXML(virBufferPtr buf, + virBitmapPtr revmap) +{ + size_t i; + + if (!revmap) + return; + + for (i = VIR_DOMAIN_VIRTIO_REVISION_DEFAULT + 1; + i < VIR_DOMAIN_VIRTIO_REVISION_LAST; + i++) { + if (virBitmapIsBitSet(revmap, i)) { + virBufferAsprintf(buf, "<virtio revision='%s'/>\n", + virDomainVirtioRevisionTypeToString(i)); + } + } +} + void virBlkioDeviceArrayClear(virBlkioDevicePtr devices, @@ -12061,6 +12142,9 @@ virDomainMemballoonDefParseXML(xmlNodePtr node, else if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0) goto error; + if (virDomainVirtioRevisionParseXML(ctxt, &def->virtio_rev) < 0) + goto error; + cleanup: VIR_FREE(model); VIR_FREE(deflate); @@ -21492,6 +21576,8 @@ virDomainMemballoonDefFormat(virBufferPtr buf, return -1; } + virDomainVirtioRevisionFormatXML(&childrenBuf, def->virtio_rev); + if (!virBufferUse(&childrenBuf)) { virBufferAddLit(buf, "/>\n"); } else { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 8b26724..46d1213 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -154,6 +154,13 @@ typedef virDomainTPMDef *virDomainTPMDefPtr; typedef struct _virDomainIOMMUDef virDomainIOMMUDef; typedef virDomainIOMMUDef *virDomainIOMMUDefPtr; +typedef enum { + VIR_DOMAIN_VIRTIO_REVISION_DEFAULT, + VIR_DOMAIN_VIRTIO_REVISION_0_9, + VIR_DOMAIN_VIRTIO_REVISION_1_0, + VIR_DOMAIN_VIRTIO_REVISION_LAST, +} virDomainVirtioRevision; + /* Flags for the 'type' field in virDomainDeviceDef */ typedef enum { VIR_DOMAIN_DEVICE_NONE = 0, @@ -1546,6 +1553,7 @@ struct _virDomainMemballoonDef { virDomainDeviceInfo info; int period; /* seconds between collections */ int autodeflate; /* enum virTristateSwitch */ + virBitmapPtr virtio_rev; }; struct _virDomainNVRAMDef { @@ -3022,6 +3030,7 @@ VIR_ENUM_DECL(virDomainTPMBackend) VIR_ENUM_DECL(virDomainMemoryModel) VIR_ENUM_DECL(virDomainMemoryBackingModel) VIR_ENUM_DECL(virDomainIOMMUModel) +VIR_ENUM_DECL(virDomainVirtioRevision) /* from libvirt.h */ VIR_ENUM_DECL(virDomainState) VIR_ENUM_DECL(virDomainNostateReason) diff --git a/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml b/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml new file mode 100644 index 0000000..d971e89 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml @@ -0,0 +1,54 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='ide' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <controller type='virtio-serial' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x09' function='0x0'/> + </controller> + <input type='mouse' bus='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x0e' function='0x0'/> + </input> + <input type='keyboard' bus='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x10' function='0x0'/> + </input> + <input type='tablet' bus='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x11' function='0x0'/> + </input> + <input type='passthrough' bus='virtio'> + <source evdev='/dev/input/event1234'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x12' function='0x0'/> + </input> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <video> + <model type='virtio' heads='1' primary='yes'> + <acceleration accel3d='yes'/> + </model> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </video> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x0c' function='0x0'/> + <virtio revision='0.9'/> + <virtio revision='1.0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml new file mode 100644 index 0000000..d971e89 --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml @@ -0,0 +1,54 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='ide' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <controller type='virtio-serial' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x09' function='0x0'/> + </controller> + <input type='mouse' bus='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x0e' function='0x0'/> + </input> + <input type='keyboard' bus='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x10' function='0x0'/> + </input> + <input type='tablet' bus='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x11' function='0x0'/> + </input> + <input type='passthrough' bus='virtio'> + <source evdev='/dev/input/event1234'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x12' function='0x0'/> + </input> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <video> + <model type='virtio' heads='1' primary='yes'> + <acceleration accel3d='yes'/> + </model> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </video> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x0c' function='0x0'/> + <virtio revision='0.9'/> + <virtio revision='1.0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 7601a5f..6952abe 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -862,6 +862,8 @@ mymain(void) DO_TEST("virtio-input", NONE); DO_TEST("virtio-input-passthrough", NONE); + DO_TEST_FULL("virtio-revision", WHEN_BOTH, GIC_NONE, QEMU_CAPS_VIRTIO_SCSI, NONE); + virObjectUnref(cfg); DO_TEST("acpi-table", NONE); -- 2.7.3
<disk ...> <virtio revision='0.9'/> </disk> https://bugzilla.redhat.com/show_bug.cgi?id=1227354 --- docs/formatdomain.html.in | 9 +++++++++ docs/schemas/domaincommon.rng | 3 +++ src/conf/domain_conf.c | 4 ++++ src/conf/domain_conf.h | 1 + tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml | 14 ++++++++++++++ .../qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml | 14 ++++++++++++++ 6 files changed, 45 insertions(+) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 8287b39..922098a 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2810,6 +2810,15 @@ </dd> </dl> </dd> + <dt><code>virtio</code></dt> + <dd> + <p> + Optional <code>virtio</code> elements can be used to enforce a particular + virtio revision in QEMU. The valid values for the <code>revision</code> + are <code>0.9</code> and <code>1.0</code>. + <span class='since'>Since 2.2.0</span> + </p> + </dd> </dl> <h4><a name="elementsFilesystems">Filesystems</a></h4> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 64088ba..6b40553 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1201,6 +1201,9 @@ </data> </element> </optional> + <optional> + <ref name="virtioRevision"/> + </optional> </interleave> </define> <define name="snapshot"> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b006bc9..9ab1f5d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7716,6 +7716,9 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, } } + if (virDomainVirtioRevisionParseXML(ctxt, &def->virtio_rev) < 0) + goto error; + /* Disk volume types will have authentication information handled in * virStorageTranslateDiskSourcePool */ @@ -20141,6 +20144,7 @@ virDomainDiskDefFormat(virBufferPtr buf, if (virDomainDeviceInfoFormat(buf, &def->info, flags | VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT) < 0) return -1; + virDomainVirtioRevisionFormatXML(buf, def->virtio_rev); virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</disk>\n"); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 46d1213..4510d73 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -620,6 +620,7 @@ struct _virDomainDiskDef { unsigned int iothread; /* unused = 0, > 0 specific thread # */ int detect_zeroes; /* enum virDomainDiskDetectZeroes */ char *domain_name; /* backend domain name */ + virBitmapPtr virtio_rev; }; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml b/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml index d971e89..1a8a3de 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml @@ -14,6 +14,20 @@ <on_crash>destroy</on_crash> <devices> <emulator>/usr/bin/qemu</emulator> + <disk type='file' device='disk'> + <driver name='qemu' type='raw'/> + <source file='/var/lib/libvirt/images/img1'/> + <target dev='vda' bus='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x0a' function='0x0'/> + <virtio revision='0.9'/> + </disk> + <disk type='file' device='disk'> + <driver name='qemu' type='raw'/> + <source file='/var/lib/libvirt/images/img2'/> + <target dev='vdb' bus='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x0b' function='0x0'/> + <virtio revision='1.0'/> + </disk> <controller type='usb' index='0'> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> </controller> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml index d971e89..1a8a3de 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml @@ -14,6 +14,20 @@ <on_crash>destroy</on_crash> <devices> <emulator>/usr/bin/qemu</emulator> + <disk type='file' device='disk'> + <driver name='qemu' type='raw'/> + <source file='/var/lib/libvirt/images/img1'/> + <target dev='vda' bus='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x0a' function='0x0'/> + <virtio revision='0.9'/> + </disk> + <disk type='file' device='disk'> + <driver name='qemu' type='raw'/> + <source file='/var/lib/libvirt/images/img2'/> + <target dev='vdb' bus='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x0b' function='0x0'/> + <virtio revision='1.0'/> + </disk> <controller type='usb' index='0'> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> </controller> -- 2.7.3
<controller type='scsi' index='0' model='virtio-scsi'> <virtio revision='0.9'/> </controller> https://bugzilla.redhat.com/show_bug.cgi?id=1227354 --- docs/formatdomain.html.in | 8 ++++++++ docs/schemas/domaincommon.rng | 3 +++ src/conf/domain_conf.c | 4 ++++ src/conf/domain_conf.h | 1 + tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml | 10 ++++++++++ tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml | 10 ++++++++++ 6 files changed, 36 insertions(+) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 922098a..6fa0c1a 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3150,6 +3150,14 @@ additional attributes that control specific features, such as: </p> + <p> + For virtio controllers, optional <code>virtio</code> elements + can be used to enforce a particular virtio revision in QEMU. + The valid values for the <code>revision</code> + are <code>0.9</code> and <code>1.0</code>. + <span class='since'>Since 2.2.0</span> + </p> + <dl> <dt><code>virtio-serial</code></dt> <dd>The <code>virtio-serial</code> controller has two additional diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 6b40553..4f4db8c 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1946,6 +1946,9 @@ </optional> </element> </optional> + <optional> + <ref name="virtioRevision"/> + </optional> </interleave> </element> </define> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9ab1f5d..098c853 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8328,6 +8328,9 @@ virDomainControllerDefParseXML(xmlNodePtr node, cur = cur->next; } + if (virDomainVirtioRevisionParseXML(ctxt, &def->virtio_rev) < 0) + goto error; + /* node is parsed differently from target attributes because * someone thought it should be a subelement instead... */ @@ -20296,6 +20299,7 @@ virDomainControllerDefFormat(virBufferPtr buf, virBufferAsprintf(buf, "<pcihole64 unit='KiB'>%lu</" "pcihole64>\n", def->opts.pciopts.pcihole64size); } + virDomainVirtioRevisionFormatXML(buf, def->virtio_rev); virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</controller>\n"); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 4510d73..23f320b 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -763,6 +763,7 @@ struct _virDomainControllerDef { virDomainUSBControllerOpts usbopts; } opts; virDomainDeviceInfo info; + virBitmapPtr virtio_rev; }; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml b/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml index 1a8a3de..7972617 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml @@ -28,12 +28,22 @@ <address type='pci' domain='0x0000' bus='0x00' slot='0x0b' function='0x0'/> <virtio revision='1.0'/> </disk> + <disk type='file' device='disk'> + <driver name='qemu' type='raw'/> + <source file='/var/lib/libvirt/images/img3'/> + <target dev='sda' bus='scsi'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> <controller type='usb' index='0'> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> </controller> <controller type='ide' index='0'> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> </controller> + <controller type='scsi' index='0' model='virtio-scsi'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x08' function='0x0'/> + <virtio revision='0.9'/> + </controller> <controller type='pci' index='0' model='pci-root'/> <controller type='virtio-serial' index='0'> <address type='pci' domain='0x0000' bus='0x00' slot='0x09' function='0x0'/> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml index 1a8a3de..7972617 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml @@ -28,12 +28,22 @@ <address type='pci' domain='0x0000' bus='0x00' slot='0x0b' function='0x0'/> <virtio revision='1.0'/> </disk> + <disk type='file' device='disk'> + <driver name='qemu' type='raw'/> + <source file='/var/lib/libvirt/images/img3'/> + <target dev='sda' bus='scsi'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> <controller type='usb' index='0'> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> </controller> <controller type='ide' index='0'> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> </controller> + <controller type='scsi' index='0' model='virtio-scsi'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x08' function='0x0'/> + <virtio revision='0.9'/> + </controller> <controller type='pci' index='0' model='pci-root'/> <controller type='virtio-serial' index='0'> <address type='pci' domain='0x0000' bus='0x00' slot='0x09' function='0x0'/> -- 2.7.3
On Wed, Aug 10, 2016 at 03:27:15PM +0200, Ján Tomko wrote:
<controller type='scsi' index='0' model='virtio-scsi'> <virtio revision='0.9'/>
I'm wondering about generalizing this. eg what if there are other device models where we want the ability to set a revision. We don't really want to invent a new sub-elment named after each device model
</controller>
Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Thu, Aug 11, 2016 at 01:00:08PM +0100, Daniel P. Berrange wrote:
On Wed, Aug 10, 2016 at 03:27:15PM +0200, Ján Tomko wrote:
<controller type='scsi' index='0' model='virtio-scsi'> <virtio revision='0.9'/>
I'm wondering about generalizing this. eg what if there are other device models where we want the ability to set a revision. We don't really want to invent a new sub-elment named after each device model
Not even a new attribute? :) <revision virtio='0.9'/> How about: <revision type='virtio' version='0.9'/> Jan
On Thu, Aug 11, 2016 at 03:25:53PM +0200, Ján Tomko wrote:
On Thu, Aug 11, 2016 at 01:00:08PM +0100, Daniel P. Berrange wrote:
On Wed, Aug 10, 2016 at 03:27:15PM +0200, Ján Tomko wrote:
<controller type='scsi' index='0' model='virtio-scsi'> <virtio revision='0.9'/>
I'm wondering about generalizing this. eg what if there are other device models where we want the ability to set a revision. We don't really want to invent a new sub-elment named after each device model
Not even a new attribute? :) <revision virtio='0.9'/>
How about: <revision type='virtio' version='0.9'/>
Both of those are quite repetative - we already know its virtio. Most devices we have alrady include a <driver> or <model> sub-element, so we should really just add a revision= attrbute to those existing elements. For places which don't already have this we can add a new <driver> element Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Thu, Aug 11, 2016 at 02:31:55PM +0100, Daniel P. Berrange wrote:
On Thu, Aug 11, 2016 at 03:25:53PM +0200, Ján Tomko wrote:
On Thu, Aug 11, 2016 at 01:00:08PM +0100, Daniel P. Berrange wrote:
On Wed, Aug 10, 2016 at 03:27:15PM +0200, Ján Tomko wrote:
<controller type='scsi' index='0' model='virtio-scsi'> <virtio revision='0.9'/>
I'm wondering about generalizing this. eg what if there are other device models where we want the ability to set a revision. We don't really want to invent a new sub-elment named after each device model
Not even a new attribute? :) <revision virtio='0.9'/>
How about: <revision type='virtio' version='0.9'/>
Both of those are quite repetative - we already know its virtio.
I guess one device having <revision>s of different types is unlikely.
Most devices we have alrady include a <driver> or <model> sub-element, so we should really just add a revision= attrbute to those existing
What I liked about having it as a separate element is that it can be repeated, e.g.: <revision type='virtio' version='0.9'/> <revision type='virtio' version='1.0'/> for a device with both 0.9 and 1.0. I could not come up with a nice way to represent that in a single attribute: * '0.9+1.0' feels like the two values should rather be separated at the XML level * 'all' will not be true if there happens to be another virtio revision in the future Jan
elements. For places which don't already have this we can add a new <driver> element
Regards, Daniel
On Thu, 11 Aug 2016 16:17:10 +0200 Ján Tomko <jtomko@redhat.com> wrote:
On Thu, Aug 11, 2016 at 02:31:55PM +0100, Daniel P. Berrange wrote:
On Thu, Aug 11, 2016 at 03:25:53PM +0200, Ján Tomko wrote:
On Thu, Aug 11, 2016 at 01:00:08PM +0100, Daniel P. Berrange wrote:
On Wed, Aug 10, 2016 at 03:27:15PM +0200, Ján Tomko wrote:
<controller type='scsi' index='0' model='virtio-scsi'> <virtio revision='0.9'/>
I'm wondering about generalizing this. eg what if there are other device models where we want the ability to set a revision. We don't really want to invent a new sub-elment named after each device model
Not even a new attribute? :) <revision virtio='0.9'/>
How about: <revision type='virtio' version='0.9'/>
Both of those are quite repetative - we already know its virtio.
I guess one device having <revision>s of different types is unlikely.
Most devices we have alrady include a <driver> or <model> sub-element, so we should really just add a revision= attrbute to those existing
What I liked about having it as a separate element is that it can be repeated, e.g.: <revision type='virtio' version='0.9'/> <revision type='virtio' version='1.0'/>
for a device with both 0.9 and 1.0.
I could not come up with a nice way to represent that in a single attribute: * '0.9+1.0' feels like the two values should rather be separated at the XML level * 'all' will not be true if there happens to be another virtio revision in the future
[not a libvirt developer, but let me comment from the qemu virtio perspective] I don't think you are expressing the concept of virtio (standard) revisions (more like releases!) here correctly. Let me elaborate: - The disable-legacy/disable-modern attributes are virtio-pci only. Moreover, they don't express 'compliant to virtio-1.0' or so: They do exactly What It Says On The Tin. A device with both disable attributes off is in fact virtio-1.0 compliant (transitional devices are compliant), as is a device with disable-legacy off. But it might also be virtio-1.1 compliant! (That's the most likely release of the standard in the near future.) - virtio-ccw does not have the concept of these disable switches. Instead, there are virtio-ccw specific 'revisions' which count upwards and may be limited by the 'max_revision' attribute. However, this is not an attribute that is supposed to be set by the user, but for backwards compatibility only. Unlike pci, ccw has nothing to gain by disabling legacy support. - We may actually want to add some kind of versioning scheme to virtio devices in future versions of the standard. But that's just a very vague idea right now. Am I right in assuming that you simply want to be able to control whether your virtio-pci devices are legacy, transitional or modern? Then I think you'd be best off adding these as virtio-pci attributes only and leave the concept of a 'virtio revision' for the future when we might introduce it in the standard.
On Fri, Aug 12, 2016 at 10:47:13AM +0200, Cornelia Huck wrote:
On Thu, 11 Aug 2016 16:17:10 +0200 Ján Tomko <jtomko@redhat.com> wrote:
On Thu, Aug 11, 2016 at 02:31:55PM +0100, Daniel P. Berrange wrote:
On Thu, Aug 11, 2016 at 03:25:53PM +0200, Ján Tomko wrote:
On Thu, Aug 11, 2016 at 01:00:08PM +0100, Daniel P. Berrange wrote:
On Wed, Aug 10, 2016 at 03:27:15PM +0200, Ján Tomko wrote:
<controller type='scsi' index='0' model='virtio-scsi'> <virtio revision='0.9'/>
I'm wondering about generalizing this. eg what if there are other device models where we want the ability to set a revision. We don't really want to invent a new sub-elment named after each device model
Not even a new attribute? :) <revision virtio='0.9'/>
How about: <revision type='virtio' version='0.9'/>
Both of those are quite repetative - we already know its virtio.
I guess one device having <revision>s of different types is unlikely.
Most devices we have alrady include a <driver> or <model> sub-element, so we should really just add a revision= attrbute to those existing
What I liked about having it as a separate element is that it can be repeated, e.g.: <revision type='virtio' version='0.9'/> <revision type='virtio' version='1.0'/>
for a device with both 0.9 and 1.0.
I could not come up with a nice way to represent that in a single attribute: * '0.9+1.0' feels like the two values should rather be separated at the XML level * 'all' will not be true if there happens to be another virtio revision in the future
[not a libvirt developer, but let me comment from the qemu virtio perspective]
I don't think you are expressing the concept of virtio (standard) revisions (more like releases!) here correctly. Let me elaborate:
- The disable-legacy/disable-modern attributes are virtio-pci only. Moreover, they don't express 'compliant to virtio-1.0' or so: They do exactly What It Says On The Tin. A device with both disable attributes off is in fact virtio-1.0 compliant (transitional devices are compliant), as is a device with disable-legacy off. But it might also be virtio-1.1 compliant! (That's the most likely release of the standard in the near future.)
- virtio-ccw does not have the concept of these disable switches. Instead, there are virtio-ccw specific 'revisions' which count upwards and may be limited by the 'max_revision' attribute. However, this is not an attribute that is supposed to be set by the user, but for backwards compatibility only. Unlike pci, ccw has nothing to gain by disabling legacy support.
- We may actually want to add some kind of versioning scheme to virtio devices in future versions of the standard. But that's just a very vague idea right now.
Am I right in assuming that you simply want to be able to control whether your virtio-pci devices are legacy, transitional or modern?
Yes.
Then I think you'd be best off adding these as virtio-pci attributes only and leave the concept of a 'virtio revision' for the future when we might introduce it in the standard.
So XML like this: <model legacy='on/off' modern='on/off'/> or <model compatibility='legacy/transitional/modern'/> (which could possibly be reused for other hypervisors with a similar concept, not just QEMU and virtio) was what we needed all along, but I misunderstood their purpose. That's good to know :) Jan
On 08/12/2016 04:59 PM, Ján Tomko wrote:
On Fri, Aug 12, 2016 at 10:47:13AM +0200, Cornelia Huck wrote:
On Thu, 11 Aug 2016 16:17:10 +0200 Ján Tomko <jtomko@redhat.com> wrote:
On Thu, Aug 11, 2016 at 02:31:55PM +0100, Daniel P. Berrange wrote:
On Thu, Aug 11, 2016 at 03:25:53PM +0200, Ján Tomko wrote:
On Thu, Aug 11, 2016 at 01:00:08PM +0100, Daniel P. Berrange wrote:
On Wed, Aug 10, 2016 at 03:27:15PM +0200, Ján Tomko wrote: > <controller type='scsi' index='0' model='virtio-scsi'> > <virtio revision='0.9'/>
I'm wondering about generalizing this. eg what if there are other device models where we want the ability to set a revision. We don't really want to invent a new sub-elment named after each device model
Not even a new attribute? :) <revision virtio='0.9'/>
How about: <revision type='virtio' version='0.9'/>
Both of those are quite repetative - we already know its virtio.
I guess one device having <revision>s of different types is unlikely.
Most devices we have alrady include a <driver> or <model> sub-element, so we should really just add a revision= attrbute to those existing
What I liked about having it as a separate element is that it can be repeated, e.g.: <revision type='virtio' version='0.9'/> <revision type='virtio' version='1.0'/>
for a device with both 0.9 and 1.0.
I could not come up with a nice way to represent that in a single attribute: * '0.9+1.0' feels like the two values should rather be separated at the XML level * 'all' will not be true if there happens to be another virtio revision in the future
[not a libvirt developer, but let me comment from the qemu virtio perspective]
I don't think you are expressing the concept of virtio (standard) revisions (more like releases!) here correctly. Let me elaborate:
- The disable-legacy/disable-modern attributes are virtio-pci only. Moreover, they don't express 'compliant to virtio-1.0' or so: They do exactly What It Says On The Tin. A device with both disable attributes off is in fact virtio-1.0 compliant (transitional devices are compliant), as is a device with disable-legacy off. But it might also be virtio-1.1 compliant! (That's the most likely release of the standard in the near future.)
- virtio-ccw does not have the concept of these disable switches. Instead, there are virtio-ccw specific 'revisions' which count upwards and may be limited by the 'max_revision' attribute. However, this is not an attribute that is supposed to be set by the user, but for backwards compatibility only. Unlike pci, ccw has nothing to gain by disabling legacy support.
- We may actually want to add some kind of versioning scheme to virtio devices in future versions of the standard. But that's just a very vague idea right now.
Am I right in assuming that you simply want to be able to control whether your virtio-pci devices are legacy, transitional or modern?
Yes.
Then I think you'd be best off adding these as virtio-pci attributes only and leave the concept of a 'virtio revision' for the future when we might introduce it in the standard.
So XML like this: <model legacy='on/off' modern='on/off'/> or <model compatibility='legacy/transitional/modern'/> (which could possibly be reused for other hypervisors with a similar concept, not just QEMU and virtio) Sorry to be a pain in the bud... but... both above proposals are virtio-PCI only.
was what we needed all along, but I misunderstood their purpose.
That's good to know :)
Jan
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On Mon, Aug 15, 2016 at 11:01:22AM +0200, Boris Fiuczynski wrote:
On 08/12/2016 04:59 PM, Ján Tomko wrote:
On Fri, Aug 12, 2016 at 10:47:13AM +0200, Cornelia Huck wrote:
On Thu, 11 Aug 2016 16:17:10 +0200 Ján Tomko <jtomko@redhat.com> wrote:
On Thu, Aug 11, 2016 at 02:31:55PM +0100, Daniel P. Berrange wrote:
On Thu, Aug 11, 2016 at 03:25:53PM +0200, Ján Tomko wrote:
On Thu, Aug 11, 2016 at 01:00:08PM +0100, Daniel P. Berrange wrote: > On Wed, Aug 10, 2016 at 03:27:15PM +0200, Ján Tomko wrote: > > <controller type='scsi' index='0' model='virtio-scsi'> > > <virtio revision='0.9'/> > > I'm wondering about generalizing this. eg what if there are > other device models where we want the ability to set a > revision. We don't really want to invent a new sub-elment > named after each device model
Not even a new attribute? :) <revision virtio='0.9'/>
How about: <revision type='virtio' version='0.9'/>
Both of those are quite repetative - we already know its virtio.
I guess one device having <revision>s of different types is unlikely.
Most devices we have alrady include a <driver> or <model> sub-element, so we should really just add a revision= attrbute to those existing
What I liked about having it as a separate element is that it can be repeated, e.g.: <revision type='virtio' version='0.9'/> <revision type='virtio' version='1.0'/>
for a device with both 0.9 and 1.0.
I could not come up with a nice way to represent that in a single attribute: * '0.9+1.0' feels like the two values should rather be separated at the XML level * 'all' will not be true if there happens to be another virtio revision in the future
[not a libvirt developer, but let me comment from the qemu virtio perspective]
I don't think you are expressing the concept of virtio (standard) revisions (more like releases!) here correctly. Let me elaborate:
- The disable-legacy/disable-modern attributes are virtio-pci only. Moreover, they don't express 'compliant to virtio-1.0' or so: They do exactly What It Says On The Tin. A device with both disable attributes off is in fact virtio-1.0 compliant (transitional devices are compliant), as is a device with disable-legacy off. But it might also be virtio-1.1 compliant! (That's the most likely release of the standard in the near future.)
- virtio-ccw does not have the concept of these disable switches. Instead, there are virtio-ccw specific 'revisions' which count upwards and may be limited by the 'max_revision' attribute. However, this is not an attribute that is supposed to be set by the user, but for backwards compatibility only. Unlike pci, ccw has nothing to gain by disabling legacy support.
- We may actually want to add some kind of versioning scheme to virtio devices in future versions of the standard. But that's just a very vague idea right now.
Am I right in assuming that you simply want to be able to control whether your virtio-pci devices are legacy, transitional or modern?
Yes.
Then I think you'd be best off adding these as virtio-pci attributes only and leave the concept of a 'virtio revision' for the future when we might introduce it in the standard.
So XML like this: <model legacy='on/off' modern='on/off'/> or <model compatibility='legacy/transitional/modern'/> (which could possibly be reused for other hypervisors with a similar concept, not just QEMU and virtio) Sorry to be a pain in the bud... but... both above proposals are virtio-PCI only.
Yes. Jan
On 08/11/2016 09:31 AM, Daniel P. Berrange wrote:
On Thu, Aug 11, 2016 at 03:25:53PM +0200, Ján Tomko wrote:
On Thu, Aug 11, 2016 at 01:00:08PM +0100, Daniel P. Berrange wrote:
On Wed, Aug 10, 2016 at 03:27:15PM +0200, Ján Tomko wrote:
<controller type='scsi' index='0' model='virtio-scsi'> <virtio revision='0.9'/> I'm wondering about generalizing this. eg what if there are other device models where we want the ability to set a revision. We don't really want to invent a new sub-elment named after each device model Not even a new attribute? :) <revision virtio='0.9'/>
How about: <revision type='virtio' version='0.9'/> Both of those are quite repetative - we already know its virtio.
Most devices we have alrady include a <driver> or <model> sub-element, so we should really just add a revision= attrbute to those existing elements. For places which don't already have this we can add a new <driver> element
Yeah, I was going to suggest <driver> as well. The only thing missing would be the ability to specify multiple versions. Also, it does make the parsing and formatting a bit awkward, since <driver> has different parse/format functions for each device that uses it (necessary due to the difference in possible attributes). In spite of that, it would definitely look better from the outside.
On Thu, Aug 11, 2016 at 10:17:34AM -0400, Laine Stump wrote:
On 08/11/2016 09:31 AM, Daniel P. Berrange wrote:
On Thu, Aug 11, 2016 at 03:25:53PM +0200, Ján Tomko wrote:
On Thu, Aug 11, 2016 at 01:00:08PM +0100, Daniel P. Berrange wrote:
On Wed, Aug 10, 2016 at 03:27:15PM +0200, Ján Tomko wrote:
<controller type='scsi' index='0' model='virtio-scsi'> <virtio revision='0.9'/> I'm wondering about generalizing this. eg what if there are other device models where we want the ability to set a revision. We don't really want to invent a new sub-elment named after each device model Not even a new attribute? :) <revision virtio='0.9'/>
How about: <revision type='virtio' version='0.9'/> Both of those are quite repetative - we already know its virtio.
Most devices we have alrady include a <driver> or <model> sub-element, so we should really just add a revision= attrbute to those existing elements. For places which don't already have this we can add a new <driver> element
Yeah, I was going to suggest <driver> as well.
The only thing missing would be the ability to specify multiple versions.
I think that's something we want to allow - didn't you mention somewhere that 'disable-legacy=off,disable-modern=off' gets different results than omitting it completely? Other than awkward enum values, it could be represented as a (rather verbose) subelement: <driver> <revision>0.9</revision> <revision>1.0</revision> </driver>
Also, it does make the parsing and formatting a bit awkward, since <driver> has different parse/format functions for each device that uses it (necessary due to the difference in possible attributes).
That means more work for me, but should not be a factor when deciding on better-looking XML.
In spite of that, it would definitely look better from the outside.
Personally I liked the consistence of having the same element under each device more (and the ability to have more of them), but putting it under <driver>/<model> also has its beauty, if we can figure out how to express multiple versions. Jan
On 08/11/2016 11:11 AM, Ján Tomko wrote:
On Thu, Aug 11, 2016 at 10:17:34AM -0400, Laine Stump wrote:
On 08/11/2016 09:31 AM, Daniel P. Berrange wrote:
On Thu, Aug 11, 2016 at 03:25:53PM +0200, Ján Tomko wrote:
On Thu, Aug 11, 2016 at 01:00:08PM +0100, Daniel P. Berrange wrote:
On Wed, Aug 10, 2016 at 03:27:15PM +0200, Ján Tomko wrote:
<controller type='scsi' index='0' model='virtio-scsi'> <virtio revision='0.9'/> I'm wondering about generalizing this. eg what if there are other device models where we want the ability to set a revision. We don't really want to invent a new sub-elment named after each device model Not even a new attribute? :) <revision virtio='0.9'/>
How about: <revision type='virtio' version='0.9'/> Both of those are quite repetative - we already know its virtio.
Most devices we have alrady include a <driver> or <model> sub-element, so we should really just add a revision= attrbute to those existing elements. For places which don't already have this we can add a new <driver> element
Yeah, I was going to suggest <driver> as well.
The only thing missing would be the ability to specify multiple versions.
I think that's something we want to allow - didn't you mention somewhere that 'disable-legacy=off,disable-modern=off' gets different results than omitting it completely?
Yes, depending on the qemu version and which bus you plug it into.
Other than awkward enum values, it could be represented as a (rather verbose) subelement: <driver> <revision>0.9</revision> <revision>1.0</revision>
It's not rational, but I *hate* single value elements! They're just too redundant!
</driver>
Also, it does make the parsing and formatting a bit awkward, since <driver> has different parse/format functions for each device that uses it (necessary due to the difference in possible attributes).
That means more work for me, but should not be a factor when deciding on better-looking XML.
Oh, I don't know - maintainability is important :-)
In spite of that, it would definitely look better from the outside.
Personally I liked the consistence of having the same element under each device more (and the ability to have more of them), but putting it under <driver>/<model> also has its beauty, if we can figure out how to express multiple versions.
<filesystem ...> ... <virtio revision='1.0'/> </filesystem> https://bugzilla.redhat.com/show_bug.cgi?id=1227354 --- docs/formatdomain.html.in | 10 ++++++++++ docs/schemas/domaincommon.rng | 3 +++ src/conf/domain_conf.c | 6 ++++++ src/conf/domain_conf.h | 1 + tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml | 13 +++++++++++++ tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml | 13 +++++++++++++ 6 files changed, 46 insertions(+) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 6fa0c1a..ebe7a14 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2997,6 +2997,16 @@ hard limit is enforced. <span class="since">Since 0.9.13</span> </dd> + + <dt><code>virtio</code></dt> + <dd> + <p> + Optional <code>virtio</code> elements can be used to enforce a particular + virtio revision in QEMU. The valid values for the <code>revision</code> + are <code>0.9</code> and <code>1.0</code>. + <span class='since'>Since 2.2.0</span> + </p> + </dd> </dl> <h4><a name="elementsAddress">Device Addresses</a></h4> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 4f4db8c..58fc6e3 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2099,6 +2099,9 @@ <ref name='scaledInteger'/> </element> </optional> + <optional> + <ref name="virtioRevision"/> + </optional> </interleave> </element> </define> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 098c853..3ab2644 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8731,6 +8731,9 @@ virDomainFSDefParseXML(xmlNodePtr node, goto error; } + if (virDomainVirtioRevisionParseXML(ctxt, &def->virtio_rev) < 0) + goto error; + def->src->path = source; source = NULL; def->dst = target; @@ -20420,6 +20423,9 @@ virDomainFSDefFormat(virBufferPtr buf, virBufferAsprintf(buf, "<space_soft_limit unit='bytes'>" "%llu</space_soft_limit>\n", def->space_soft_limit); } + + virDomainVirtioRevisionFormatXML(buf, def->virtio_rev); + virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</filesystem>\n"); return 0; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 23f320b..6c1ce1b 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -826,6 +826,7 @@ struct _virDomainFSDef { unsigned long long space_hard_limit; /* in bytes */ unsigned long long space_soft_limit; /* in bytes */ bool symlinksResolved; + virBitmapPtr virtio_rev; }; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml b/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml index 7972617..ad846e1 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml @@ -48,6 +48,19 @@ <controller type='virtio-serial' index='0'> <address type='pci' domain='0x0000' bus='0x00' slot='0x09' function='0x0'/> </controller> + <filesystem type='mount' accessmode='passthrough'> + <source dir='/export/fs1'/> + <target dir='fs1'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + <virtio revision='0.9'/> + </filesystem> + <filesystem type='mount' accessmode='mapped'> + <driver type='path' wrpolicy='immediate'/> + <source dir='/export/fs2'/> + <target dir='fs2'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> + <virtio revision='1.0'/> + </filesystem> <input type='mouse' bus='virtio'> <address type='pci' domain='0x0000' bus='0x00' slot='0x0e' function='0x0'/> </input> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml index 7972617..ad846e1 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml @@ -48,6 +48,19 @@ <controller type='virtio-serial' index='0'> <address type='pci' domain='0x0000' bus='0x00' slot='0x09' function='0x0'/> </controller> + <filesystem type='mount' accessmode='passthrough'> + <source dir='/export/fs1'/> + <target dir='fs1'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + <virtio revision='0.9'/> + </filesystem> + <filesystem type='mount' accessmode='mapped'> + <driver type='path' wrpolicy='immediate'/> + <source dir='/export/fs2'/> + <target dir='fs2'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> + <virtio revision='1.0'/> + </filesystem> <input type='mouse' bus='virtio'> <address type='pci' domain='0x0000' bus='0x00' slot='0x0e' function='0x0'/> </input> -- 2.7.3
<interface type='user'> <mac address='52:54:56:5a:5c:5e'/> <model type='virtio'/> <virtio revision='1.0'/> </interface> https://bugzilla.redhat.com/show_bug.cgi?id=1227354 --- docs/formatdomain.html.in | 6 ++++++ docs/schemas/domaincommon.rng | 3 +++ src/conf/domain_conf.c | 5 +++++ src/conf/domain_conf.h | 1 + tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml | 12 ++++++++++++ tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml | 12 ++++++++++++ 6 files changed, 39 insertions(+) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index ebe7a14..155cc69 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -4048,6 +4048,12 @@ attribute <code>type='pci'</code> as <a href="#elementsAddress">documented above</a>. </p> + <p> + Optional <code>virtio</code> elements can be used to enforce a particular + virtio revision in QEMU. The valid values for the <code>revision</code> + are <code>0.9</code> and <code>1.0</code>. + <span class='since'>Since 2.2.0</span> + </p> <h5><a name="elementsNICSVirtual">Virtual network</a></h5> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 58fc6e3..c106cc1 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2414,6 +2414,9 @@ <ref name="virYesNo"/> </attribute> </optional> + <optional> + <ref name="virtioRevision"/> + </optional> </element> </define> <!-- diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3ab2644..3400833 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9709,6 +9709,9 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, goto error; } + if (virDomainVirtioRevisionParseXML(ctxt, &def->virtio_rev) < 0) + goto error; + cleanup: ctxt->node = oldnode; VIR_FREE(macaddr); @@ -21159,6 +21162,8 @@ virDomainNetDefFormat(virBufferPtr buf, | VIR_DOMAIN_DEF_FORMAT_ALLOW_ROM) < 0) return -1; + virDomainVirtioRevisionFormatXML(buf, def->virtio_rev); + virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</interface>\n"); return 0; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 6c1ce1b..eab2b8e 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -993,6 +993,7 @@ struct _virDomainNetDef { virNetDevVlan vlan; int trustGuestRxFilters; /* enum virTristateBool */ int linkstate; + virBitmapPtr virtio_rev; }; /* Used for prefix of ifname of any network name generated dynamically diff --git a/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml b/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml index ad846e1..1f9f26f 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml @@ -61,6 +61,18 @@ <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> <virtio revision='1.0'/> </filesystem> + <interface type='user'> + <mac address='52:54:56:58:5a:5c'/> + <model type='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0'/> + <virtio revision='0.9'/> + </interface> + <interface type='user'> + <mac address='52:54:56:5a:5c:5e'/> + <model type='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0'/> + <virtio revision='1.0'/> + </interface> <input type='mouse' bus='virtio'> <address type='pci' domain='0x0000' bus='0x00' slot='0x0e' function='0x0'/> </input> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml index ad846e1..1f9f26f 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml @@ -61,6 +61,18 @@ <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> <virtio revision='1.0'/> </filesystem> + <interface type='user'> + <mac address='52:54:56:58:5a:5c'/> + <model type='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0'/> + <virtio revision='0.9'/> + </interface> + <interface type='user'> + <mac address='52:54:56:5a:5c:5e'/> + <model type='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0'/> + <virtio revision='1.0'/> + </interface> <input type='mouse' bus='virtio'> <address type='pci' domain='0x0000' bus='0x00' slot='0x0e' function='0x0'/> </input> -- 2.7.3
<rng model='virtio'> <backend model='random'>/dev/random</backend> <virtio revision='1.0'/> </rng> https://bugzilla.redhat.com/show_bug.cgi?id=1227354 --- docs/formatdomain.html.in | 9 +++++++++ docs/schemas/domaincommon.rng | 3 +++ src/conf/domain_conf.c | 5 +++++ src/conf/domain_conf.h | 1 + tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml | 5 +++++ tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml | 5 +++++ 6 files changed, 28 insertions(+) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 155cc69..0f9fd78 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -6538,6 +6538,15 @@ qemu-kvm -net nic,model=? /dev/null </dd> </dl> </dd> + <dt><code>virtio</code></dt> + <dd> + <p> + Optional <code>virtio</code> elements can be used to enforce a particular + virtio revision in QEMU. The valid values for the <code>revision</code> + are <code>0.9</code> and <code>1.0</code>. + <span class='since'>Since 2.2.0</span> + </p> + </dd> </dl> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index c106cc1..2909edf 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -4804,6 +4804,9 @@ <optional> <ref name="address"/> </optional> + <optional> + <ref name="virtioRevision"/> + </optional> </interleave> </element> </define> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3400833..700b2fc 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -12089,6 +12089,9 @@ virDomainRNGDefParseXML(xmlNodePtr node, if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0) goto error; + if (virDomainVirtioRevisionParseXML(ctxt, &def->virtio_rev) < 0) + goto error; + cleanup: VIR_FREE(model); VIR_FREE(backend); @@ -21776,6 +21779,8 @@ virDomainRNGDefFormat(virBufferPtr buf, return -1; } + virDomainVirtioRevisionFormatXML(buf, def->virtio_rev); + virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</rng>\n"); return 0; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index eab2b8e..6ff9d43 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1946,6 +1946,7 @@ struct _virDomainRNGDef { } source; virDomainDeviceInfo info; + virBitmapPtr virtio_rev; }; typedef enum { diff --git a/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml b/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml index 1f9f26f..c3a4654 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml @@ -99,5 +99,10 @@ <virtio revision='0.9'/> <virtio revision='1.0'/> </memballoon> + <rng model='virtio'> + <backend model='random'>/dev/random</backend> + <address type='pci' domain='0x0000' bus='0x00' slot='0x0d' function='0x0'/> + <virtio revision='1.0'/> + </rng> </devices> </domain> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml index 1f9f26f..c3a4654 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml @@ -99,5 +99,10 @@ <virtio revision='0.9'/> <virtio revision='1.0'/> </memballoon> + <rng model='virtio'> + <backend model='random'>/dev/random</backend> + <address type='pci' domain='0x0000' bus='0x00' slot='0x0d' function='0x0'/> + <virtio revision='1.0'/> + </rng> </devices> </domain> -- 2.7.3
<video> <model type='virtio' heads='1' primary='yes'/> <virtio revision='1.0'/> </video> https://bugzilla.redhat.com/show_bug.cgi?id=1227354 --- docs/formatdomain.html.in | 10 ++++++++++ docs/schemas/domaincommon.rng | 3 +++ src/conf/domain_conf.c | 15 +++++++++++++-- src/conf/domain_conf.h | 1 + tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml | 1 + .../qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml | 1 + 6 files changed, 29 insertions(+), 2 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 0f9fd78..8485e9b 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -5646,6 +5646,16 @@ qemu-kvm -net nic,model=? /dev/null The optional <code>address</code> sub-element can be used to tie the video device to a particular PCI slot. </dd> + + <dt><code>virtio</code></dt> + <dd> + <p> + Optional <code>virtio</code> elements can be used to enforce a particular + virtio revision in QEMU. The valid values for the <code>revision</code> + are <code>0.9</code> and <code>1.0</code>. + <span class='since'>Since 2.2.0</span> + </p> + </dd> </dl> <h4><a name="elementsConsole">Consoles, serial, parallel & channel devices</a></h4> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 2909edf..fb92f2e 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3150,6 +3150,9 @@ <optional> <ref name="address"/> </optional> + <optional> + <ref name="virtioRevision"/> + </optional> </element> </define> <!-- diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 700b2fc..fc344b2 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -12650,11 +12650,13 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node) static virDomainVideoDefPtr virDomainVideoDefParseXML(xmlNodePtr node, + xmlXPathContextPtr ctxt, const virDomainDef *dom, unsigned int flags) { virDomainVideoDefPtr def; xmlNodePtr cur; + xmlNodePtr saved = ctxt->node; char *type = NULL; char *heads = NULL; char *vram = NULL; @@ -12663,6 +12665,8 @@ virDomainVideoDefParseXML(xmlNodePtr node, char *vgamem = NULL; char *primary = NULL; + ctxt->node = node; + if (VIR_ALLOC(def) < 0) return NULL; @@ -12764,7 +12768,12 @@ virDomainVideoDefParseXML(xmlNodePtr node, if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0) goto error; + if (virDomainVirtioRevisionParseXML(ctxt, &def->virtio_rev) < 0) + goto error; + cleanup: + ctxt->node = saved; + VIR_FREE(type); VIR_FREE(ram); VIR_FREE(vram); @@ -13459,7 +13468,7 @@ virDomainDeviceDefParse(const char *xmlStr, goto error; break; case VIR_DOMAIN_DEVICE_VIDEO: - if (!(dev->data.video = virDomainVideoDefParseXML(node, def, flags))) + if (!(dev->data.video = virDomainVideoDefParseXML(node, ctxt, def, flags))) goto error; break; case VIR_DOMAIN_DEVICE_HOSTDEV: @@ -17125,7 +17134,7 @@ virDomainDefParseXML(xmlDocPtr xml, virDomainVideoDefPtr video; ssize_t insertAt = -1; - if (!(video = virDomainVideoDefParseXML(nodes[i], def, flags))) + if (!(video = virDomainVideoDefParseXML(nodes[i], ctxt, def, flags))) goto error; if (video->primary) { @@ -21942,6 +21951,8 @@ virDomainVideoDefFormat(virBufferPtr buf, if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) return -1; + virDomainVirtioRevisionFormatXML(buf, def->virtio_rev); + virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</video>\n"); return 0; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 6ff9d43..a80ac48 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1321,6 +1321,7 @@ struct _virDomainVideoDef { bool primary; virDomainVideoAccelDefPtr accel; virDomainDeviceInfo info; + virBitmapPtr virtio_rev; }; /* graphics console modes */ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml b/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml index c3a4654..cfb8581 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml @@ -93,6 +93,7 @@ <acceleration accel3d='yes'/> </model> <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + <virtio revision='1.0'/> </video> <memballoon model='virtio'> <address type='pci' domain='0x0000' bus='0x00' slot='0x0c' function='0x0'/> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml index c3a4654..cfb8581 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml @@ -93,6 +93,7 @@ <acceleration accel3d='yes'/> </model> <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + <virtio revision='1.0'/> </video> <memballoon model='virtio'> <address type='pci' domain='0x0000' bus='0x00' slot='0x0c' function='0x0'/> -- 2.7.3
<input type='mouse' bus='virtio'> <address type='pci' domain='0x0000' bus='0x00' slot='0x0e' function='0x0'/> <virtio revision='1.0'/> </input> <input type='keyboard' bus='virtio'> <address type='pci' domain='0x0000' bus='0x00' slot='0x10' function='0x0'/> <virtio revision='1.0'/> </input> <input type='tablet' bus='virtio'> <address type='pci' domain='0x0000' bus='0x00' slot='0x11' function='0x0'/> <virtio revision='1.0'/> </input> <input type='passthrough' bus='virtio'> <source evdev='/dev/input/event1234'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x12' function='0x0'/> <virtio revision='1.0'/> </input> https://bugzilla.redhat.com/show_bug.cgi?id=1227354 --- docs/formatdomain.html.in | 8 ++++++++ docs/schemas/domaincommon.rng | 3 +++ src/conf/domain_conf.c | 4 ++++ src/conf/domain_conf.h | 1 + tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml | 4 ++++ tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml | 4 ++++ 6 files changed, 24 insertions(+) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 8485e9b..d09d9c2 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -5235,6 +5235,14 @@ qemu-kvm -net nic,model=? /dev/null event device passed through to guests. (KVM only) </p> + <p> + For virtio input devices, optional <code>virtio</code> elements + can be used to enforce a particular virtio revision in QEMU. + The valid values for the <code>revision</code> + are <code>0.9</code> and <code>1.0</code>. + <span class='since'>Since 2.2.0</span> + </p> + <h4><a name="elementsHub">Hub devices</a></h4> <p> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index fb92f2e..2b4fa7c 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3828,6 +3828,9 @@ <optional> <ref name="address"/> </optional> + <optional> + <ref name="virtioRevision"/> + </optional> </element> </define> <define name="hub"> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index fc344b2..cef024c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10775,6 +10775,9 @@ virDomainInputDefParseXML(const virDomainDef *dom, goto error; } + if (virDomainVirtioRevisionParseXML(ctxt, &def->virtio_rev) < 0) + goto error; + cleanup: VIR_FREE(evdev); VIR_FREE(type); @@ -21992,6 +21995,7 @@ virDomainInputDefFormat(virBufferPtr buf, virBufferEscapeString(&childbuf, "<source evdev='%s'/>\n", def->source.evdev); if (virDomainDeviceInfoFormat(&childbuf, &def->info, flags) < 0) return -1; + virDomainVirtioRevisionFormatXML(&childbuf, def->virtio_rev); if (!virBufferUse(&childbuf)) { virBufferAddLit(buf, "/>\n"); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index a80ac48..6a60dee 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1228,6 +1228,7 @@ struct _virDomainInputDef { char *evdev; } source; virDomainDeviceInfo info; + virBitmapPtr virtio_rev; }; typedef enum { diff --git a/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml b/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml index cfb8581..2bdfe82 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml @@ -75,16 +75,20 @@ </interface> <input type='mouse' bus='virtio'> <address type='pci' domain='0x0000' bus='0x00' slot='0x0e' function='0x0'/> + <virtio revision='1.0'/> </input> <input type='keyboard' bus='virtio'> <address type='pci' domain='0x0000' bus='0x00' slot='0x10' function='0x0'/> + <virtio revision='1.0'/> </input> <input type='tablet' bus='virtio'> <address type='pci' domain='0x0000' bus='0x00' slot='0x11' function='0x0'/> + <virtio revision='1.0'/> </input> <input type='passthrough' bus='virtio'> <source evdev='/dev/input/event1234'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x12' function='0x0'/> + <virtio revision='1.0'/> </input> <input type='mouse' bus='ps2'/> <input type='keyboard' bus='ps2'/> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml index cfb8581..2bdfe82 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml @@ -75,16 +75,20 @@ </interface> <input type='mouse' bus='virtio'> <address type='pci' domain='0x0000' bus='0x00' slot='0x0e' function='0x0'/> + <virtio revision='1.0'/> </input> <input type='keyboard' bus='virtio'> <address type='pci' domain='0x0000' bus='0x00' slot='0x10' function='0x0'/> + <virtio revision='1.0'/> </input> <input type='tablet' bus='virtio'> <address type='pci' domain='0x0000' bus='0x00' slot='0x11' function='0x0'/> + <virtio revision='1.0'/> </input> <input type='passthrough' bus='virtio'> <source evdev='/dev/input/event1234'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x12' function='0x0'/> + <virtio revision='1.0'/> </input> <input type='mouse' bus='ps2'/> <input type='keyboard' bus='ps2'/> -- 2.7.3
Check whether the disable-legacy property is present on the following devices: virtio-balloon-pci virtio-blk-pci virtio-scsi-pci virtio-net-pci virtio-gpu-pci Assuming that if QEMU knows other virtio devices where this property is applicable, it will have at least one of these devices. Added in QEMU by: commit e266d421490e0ae83044bbebb209b2d3650c0ba6 virtio-pci: add flags to enable/disable legacy/modern --- src/qemu/qemu_capabilities.c | 6 ++++++ src/qemu/qemu_capabilities.h | 1 + tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_2.6.0-gicv2.aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_2.6.0-gicv3.aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml | 1 + tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml | 1 + 9 files changed, 14 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 43e3ea7..6c9af98 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -340,6 +340,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "display", /* 230 */ "intel-iommu", "smm", + "virtio-disable-legacy", ); @@ -1565,6 +1566,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = { static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioBalloon[] = { { "deflate-on-oom", QEMU_CAPS_VIRTIO_BALLOON_AUTODEFLATE }, + { "disable-legacy", QEMU_CAPS_VIRTIO_DISABLE_LEGACY }, }; static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioBlk[] = { @@ -1574,15 +1576,18 @@ static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioBlk[] = { { "event_idx", QEMU_CAPS_VIRTIO_BLK_EVENT_IDX }, { "scsi", QEMU_CAPS_VIRTIO_BLK_SCSI }, { "logical_block_size", QEMU_CAPS_BLOCKIO }, + { "disable-legacy", QEMU_CAPS_VIRTIO_DISABLE_LEGACY }, }; static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioNet[] = { { "tx", QEMU_CAPS_VIRTIO_TX_ALG }, { "event_idx", QEMU_CAPS_VIRTIO_NET_EVENT_IDX }, + { "disable-legacy", QEMU_CAPS_VIRTIO_DISABLE_LEGACY }, }; static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioSCSI[] = { { "iothread", QEMU_CAPS_VIRTIO_SCSI_IOTHREAD }, + { "disable-legacy", QEMU_CAPS_VIRTIO_DISABLE_LEGACY }, }; static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsPCIAssign[] = { @@ -1659,6 +1664,7 @@ static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsQxlVga[] = { static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioGpu[] = { { "virgl", QEMU_CAPS_DEVICE_VIRTIO_GPU_VIRGL }, + { "disable-legacy", QEMU_CAPS_VIRTIO_DISABLE_LEGACY }, }; static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsICH9[] = { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index d249e2e..11d2202 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -373,6 +373,7 @@ typedef enum { QEMU_CAPS_DISPLAY, /* -display */ QEMU_CAPS_DEVICE_INTEL_IOMMU, /* -device intel-iommu */ QEMU_CAPS_MACHINE_SMM_OPT, /* -machine xxx,smm=on/off/auto */ + QEMU_CAPS_VIRTIO_DISABLE_LEGACY, /* virtio-*pci.disable-legacy */ QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml index 339ee1f..d990879 100644 --- a/tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml @@ -184,6 +184,7 @@ <flag name='display'/> <flag name='intel-iommu'/> <flag name='smm'/> + <flag name='virtio-disable-legacy'/> <version>2004000</version> <kvmVersion>0</kvmVersion> <package></package> diff --git a/tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml index c1a68d0..61e04a7 100644 --- a/tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml @@ -189,6 +189,7 @@ <flag name='display'/> <flag name='intel-iommu'/> <flag name='smm'/> + <flag name='virtio-disable-legacy'/> <version>2005000</version> <kvmVersion>0</kvmVersion> <package></package> diff --git a/tests/qemucapabilitiesdata/caps_2.6.0-gicv2.aarch64.xml b/tests/qemucapabilitiesdata/caps_2.6.0-gicv2.aarch64.xml index 85d7d3f..697082a 100644 --- a/tests/qemucapabilitiesdata/caps_2.6.0-gicv2.aarch64.xml +++ b/tests/qemucapabilitiesdata/caps_2.6.0-gicv2.aarch64.xml @@ -158,6 +158,7 @@ <flag name='tls-creds-x509'/> <flag name='display'/> <flag name='smm'/> + <flag name='virtio-disable-legacy'/> <version>2005094</version> <kvmVersion>0</kvmVersion> <package></package> diff --git a/tests/qemucapabilitiesdata/caps_2.6.0-gicv3.aarch64.xml b/tests/qemucapabilitiesdata/caps_2.6.0-gicv3.aarch64.xml index deb1257..96b0af9 100644 --- a/tests/qemucapabilitiesdata/caps_2.6.0-gicv3.aarch64.xml +++ b/tests/qemucapabilitiesdata/caps_2.6.0-gicv3.aarch64.xml @@ -158,6 +158,7 @@ <flag name='tls-creds-x509'/> <flag name='display'/> <flag name='smm'/> + <flag name='virtio-disable-legacy'/> <version>2005094</version> <kvmVersion>0</kvmVersion> <package></package> diff --git a/tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml b/tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml index 2b7ea0e..f590967 100644 --- a/tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml +++ b/tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml @@ -152,6 +152,7 @@ <flag name='tls-creds-x509'/> <flag name='display'/> <flag name='smm'/> + <flag name='virtio-disable-legacy'/> <version>2005094</version> <kvmVersion>0</kvmVersion> <package></package> diff --git a/tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml index 495c114..ed75438 100644 --- a/tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml @@ -195,6 +195,7 @@ <flag name='display'/> <flag name='intel-iommu'/> <flag name='smm'/> + <flag name='virtio-disable-legacy'/> <version>2006000</version> <kvmVersion>0</kvmVersion> <package></package> diff --git a/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml index fafffa6..70e1637 100644 --- a/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml @@ -192,6 +192,7 @@ <flag name='display'/> <flag name='intel-iommu'/> <flag name='smm'/> + <flag name='virtio-disable-legacy'/> <version>2006091</version> <kvmVersion>0</kvmVersion> <package> (v2.7.0-rc1-52-g42e0d60)</package> -- 2.7.3
https://bugzilla.redhat.com/show_bug.cgi?id=1227354 Translate the optional <virtio revision> attributes to disable-legacy=on/off and disable-modern=on/off options for the following devices: <memballoon> virtio-balloon-pci <disk> virtio-blk-pci <controller> virtio-scsi-pci virtio-serial-pci <filesystem> virtio-9p-pci <interface> virtio-net-pci <rng> virtio-rng-pci <video> virtio-gpu-pci <input> virtio-input-host-pci virtio-keyboard-pci virtio-mouse-pci virtio-tablet-pci --- src/qemu/qemu_command.c | 56 +++++++++++++++++++ .../qemuxml2argv-virtio-revision.args | 62 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 11 ++++ 3 files changed, 129 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.args diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 55df23d..969def2 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -378,6 +378,36 @@ qemuBuildDeviceAddressStr(virBufferPtr buf, } static int +qemuBuildVirtioRevisionStr(virBufferPtr buf, + virBitmapPtr revmap, + virQEMUCapsPtr qemuCaps) +{ + if (!revmap) + return 0; + + if (virBitmapLastSetBit(revmap) && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_DISABLE_LEGACY)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("setting the virtio revision is not supported with " + "this QEMU binary")); + return -1; + } + + virBufferAddLit(buf, ",disable-legacy="); + if (virBitmapIsBitSet(revmap, VIR_DOMAIN_VIRTIO_REVISION_0_9)) + virBufferAddLit(buf, "off"); + else + virBufferAddLit(buf, "on"); + + virBufferAddLit(buf, ",disable-modern="); + if (virBitmapIsBitSet(revmap, VIR_DOMAIN_VIRTIO_REVISION_1_0)) + virBufferAddLit(buf, "off"); + else + virBufferAddLit(buf, "on"); + return 0; +} + +static int qemuBuildRomStr(virBufferPtr buf, virDomainDeviceInfoPtr info) { @@ -1988,6 +2018,10 @@ qemuBuildDriveDevStr(const virDomainDef *def, (disk->device == VIR_DOMAIN_DISK_DEVICE_LUN) ? "on" : "off"); } + + if (qemuBuildVirtioRevisionStr(&opt, disk->virtio_rev, qemuCaps) < 0) + goto error; + if (qemuBuildDeviceAddressStr(&opt, def, &disk->info, qemuCaps) < 0) goto error; break; @@ -2309,6 +2343,8 @@ qemuBuildFSDevStr(const virDomainDef *def, QEMU_FSDEV_HOST_PREFIX, fs->info.alias); virBufferAsprintf(&opt, ",mount_tag=%s", fs->dst); + qemuBuildVirtioRevisionStr(&opt, fs->virtio_rev, qemuCaps); + if (qemuBuildDeviceAddressStr(&opt, def, &fs->info, qemuCaps) < 0) goto error; @@ -2566,6 +2602,8 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, def->iothread); } } + if (qemuBuildVirtioRevisionStr(&buf, def->virtio_rev, qemuCaps) < 0) + goto error; break; case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC: virBufferAddLit(&buf, "lsi"); @@ -2611,6 +2649,8 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, virBufferAsprintf(&buf, ",vectors=%d", def->opts.vioserial.vectors); } + if (qemuBuildVirtioRevisionStr(&buf, def->virtio_rev, qemuCaps) < 0) + goto error; break; case VIR_DOMAIN_CONTROLLER_TYPE_CCID: @@ -3555,12 +3595,16 @@ qemuBuildNicDevStr(virDomainDefPtr def, virBufferAsprintf(&buf, ",id=%s", net->info.alias); virBufferAsprintf(&buf, ",mac=%s", virMacAddrFormat(&net->mac, macaddr)); + if (qemuBuildDeviceAddressStr(&buf, def, &net->info, qemuCaps) < 0) goto error; if (qemuBuildRomStr(&buf, &net->info) < 0) goto error; if (bootindex && virQEMUCapsGet(qemuCaps, QEMU_CAPS_BOOTINDEX)) virBufferAsprintf(&buf, ",bootindex=%u", bootindex); + if (usingVirtio && + qemuBuildVirtioRevisionStr(&buf, net->virtio_rev, qemuCaps) < 0) + goto error; if (virBufferCheckError(&buf) < 0) goto error; @@ -3825,6 +3869,9 @@ qemuBuildMemballoonCommandLine(virCommandPtr cmd, virTristateSwitchTypeToString(def->memballoon->autodeflate)); } + if (qemuBuildVirtioRevisionStr(&buf, def->memballoon->virtio_rev, qemuCaps) < 0) + goto error; + virCommandAddArg(cmd, "-device"); virCommandAddArgBuffer(cmd, &buf); return 0; @@ -3955,6 +4002,9 @@ qemuBuildVirtioInputDevStr(const virDomainDef *def, if (qemuBuildDeviceAddressStr(&buf, def, &dev->info, qemuCaps) < 0) goto error; + if (qemuBuildVirtioRevisionStr(&buf, dev->virtio_rev, qemuCaps) < 0) + goto error; + if (virBufferCheckError(&buf) < 0) goto error; @@ -4320,6 +4370,9 @@ qemuBuildDeviceVideoStr(const virDomainDef *def, if (qemuBuildDeviceAddressStr(&buf, def, &video->info, qemuCaps) < 0) goto error; + if (qemuBuildVirtioRevisionStr(&buf, video->virtio_rev, qemuCaps) < 0) + goto error; + if (virBufferCheckError(&buf) < 0) goto error; @@ -5566,6 +5619,9 @@ qemuBuildRNGDevStr(const virDomainDef *def, virBufferAddLit(&buf, ",period=1000"); } + if (qemuBuildVirtioRevisionStr(&buf, dev->virtio_rev, qemuCaps) < 0) + goto error; + if (qemuBuildDeviceAddressStr(&buf, def, &dev->info, qemuCaps) < 0) goto error; if (virBufferCheckError(&buf) < 0) diff --git a/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.args b/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.args new file mode 100644 index 0000000..79b771f --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.args @@ -0,0 +1,62 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu \ +-name QEMUGuest1 \ +-S \ +-M pc \ +-m 214 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-nographic \ +-nodefaults \ +-monitor unix:/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait \ +-no-acpi \ +-boot c \ +-device virtio-scsi-pci,disable-legacy=off,disable-modern=on,id=scsi0,\ +bus=pci.0,addr=0x8 \ +-device virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x9 \ +-usb \ +-drive file=/var/lib/libvirt/images/img1,format=raw,if=none,\ +id=drive-virtio-disk0 \ +-device virtio-blk-pci,disable-legacy=off,disable-modern=on,bus=pci.0,addr=0xa,\ +drive=drive-virtio-disk0,id=virtio-disk0 \ +-drive file=/var/lib/libvirt/images/img2,format=raw,if=none,\ +id=drive-virtio-disk1 \ +-device virtio-blk-pci,disable-legacy=on,disable-modern=off,bus=pci.0,addr=0xb,\ +drive=drive-virtio-disk1,id=virtio-disk1 \ +-drive file=/var/lib/libvirt/images/img3,format=raw,if=none,\ +id=drive-scsi0-0-0-0 \ +-device scsi-disk,bus=scsi0.0,channel=0,scsi-id=0,lun=0,\ +drive=drive-scsi0-0-0-0,id=scsi0-0-0-0 \ +-fsdev local,security_model=passthrough,id=fsdev-fs0,path=/export/fs1 \ +-device virtio-9p-pci,id=fs0,fsdev=fsdev-fs0,mount_tag=fs1,disable-legacy=off,\ +disable-modern=on,bus=pci.0,addr=0x3 \ +-fsdev local,security_model=mapped,writeout=immediate,id=fsdev-fs1,\ +path=/export/fs2 \ +-device virtio-9p-pci,id=fs1,fsdev=fsdev-fs1,mount_tag=fs2,disable-legacy=on,\ +disable-modern=off,bus=pci.0,addr=0x4 \ +-device virtio-net-pci,vlan=0,id=net0,mac=52:54:56:58:5a:5c,bus=pci.0,addr=0x6,\ +disable-legacy=off,disable-modern=on \ +-net user,vlan=0,name=hostnet0 \ +-device virtio-net-pci,vlan=1,id=net1,mac=52:54:56:5a:5c:5e,bus=pci.0,addr=0x7,\ +disable-legacy=on,disable-modern=off \ +-net user,vlan=1,name=hostnet1 \ +-device virtio-mouse-pci,id=input0,bus=pci.0,addr=0xe,disable-legacy=on,\ +disable-modern=off \ +-device virtio-keyboard-pci,id=input1,bus=pci.0,addr=0x10,disable-legacy=on,\ +disable-modern=off \ +-device virtio-tablet-pci,id=input2,bus=pci.0,addr=0x11,disable-legacy=on,\ +disable-modern=off \ +-device virtio-input-host-pci,id=input3,evdev=/dev/input/event1234,bus=pci.0,\ +addr=0x12,disable-legacy=on,disable-modern=off \ +-device virtio-vga,id=video0,virgl=on,bus=pci.0,addr=0x2,disable-legacy=on,\ +disable-modern=off \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0xc,disable-legacy=off,\ +disable-modern=off \ +-object rng-random,id=objrng0,filename=/dev/random \ +-device virtio-rng-pci,rng=objrng0,id=rng0,disable-legacy=on,\ +disable-modern=off,bus=pci.0,addr=0xd diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 75ac62e..6e7c17f 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -2087,6 +2087,17 @@ mymain(void) DO_TEST("intel-iommu", QEMU_CAPS_DEVICE_PCI_BRIDGE, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, QEMU_CAPS_DEVICE_INTEL_IOMMU); + DO_TEST("virtio-revision", QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_VIRTIO_KEYBOARD, + QEMU_CAPS_VIRTIO_MOUSE, QEMU_CAPS_VIRTIO_TABLET, + QEMU_CAPS_VIRTIO_INPUT_HOST, + QEMU_CAPS_FSDEV, QEMU_CAPS_FSDEV_WRITEOUT, + QEMU_CAPS_DEVICE_VIRTIO_GPU, + QEMU_CAPS_DEVICE_VIRTIO_GPU_VIRGL, + QEMU_CAPS_DEVICE_VIRTIO_RNG, + QEMU_CAPS_OBJECT_RNG_RANDOM, + QEMU_CAPS_DEVICE_VIDEO_PRIMARY, + QEMU_CAPS_VIRTIO_DISABLE_LEGACY); + qemuTestDriverFree(&driver); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; -- 2.7.3
Check whether the disable-legacy property is present on the following devices: virtio-balloon-pci virtio-blk-pci virtio-scsi-pci virtio-serial-pci virtio-9p-pci virtio-net-pci virtio-rng-pci virtio-gpu-pci virtio-input-host-pci virtio-keyboard-pci virtio-mouse-pci virtio-tablet-pci Assuming that if QEMU knows other virtio devices where this property is applicable, it will have at least one of these devices. Added in QEMU by: commit e266d421490e0ae83044bbebb209b2d3650c0ba6 virtio-pci: add flags to enable/disable legacy/modern --- v4: only check -pci devices rename the capability to virtio-pci-disable-legacy src/qemu/qemu_capabilities.c | 58 ++++++++++++++++++++++ src/qemu/qemu_capabilities.h | 1 + tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml | 1 + .../caps_2.6.0-gicv2.aarch64.xml | 1 + .../caps_2.6.0-gicv3.aarch64.xml | 1 + tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml | 1 + tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml | 1 + 9 files changed, 66 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 43e3ea7..46c6529 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -340,6 +340,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "display", /* 230 */ "intel-iommu", "smm", + "virtio-pci-disable-legacy", ); @@ -1741,6 +1742,34 @@ static struct virQEMUCapsObjectTypeProps virQEMUCapsObjectProps[] = { ARRAY_CARDINALITY(virQEMUCapsObjectPropsUSBNECXHCI) }, }; +struct virQEMUCapsPropObjects { + const char *prop; + int flag; + const char **objects; +}; + +static const char *virQEMUCapsVirtioPCIDisableLegacyObjects[] = { + "virtio-balloon-pci", + "virtio-blk-pci", + "virtio-scsi-pci", + "virtio-serial-pci", + "virtio-9p-pci", + "virtio-net-pci", + "virtio-rng-pci", + "virtio-gpu-pci", + "virtio-input-host-pci", + "virtio-keyboard-pci", + "virtio-mouse-pci", + "virtio-tablet-pci", + NULL +}; + +static struct virQEMUCapsPropObjects virQEMUCapsPropObjects[] = { + { "disable-legacy", + QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY, + virQEMUCapsVirtioPCIDisableLegacyObjects } +}; + static void virQEMUCapsProcessStringFlags(virQEMUCapsPtr qemuCaps, @@ -1762,6 +1791,31 @@ virQEMUCapsProcessStringFlags(virQEMUCapsPtr qemuCaps, static void +virQEMUCapsProcessProps(virQEMUCapsPtr qemuCaps, + size_t nprops, + struct virQEMUCapsPropObjects *props, + const char *object, + size_t nvalues, + char *const*values) +{ + size_t i, j; + + for (i = 0; i < nprops; i++) { + if (virQEMUCapsGet(qemuCaps, props[i].flag)) + continue; + + for (j = 0; j < nvalues; j++) { + if (STREQ(values[j], props[i].prop)) { + if (virStringArrayHasString((char **)props[i].objects, object)) + virQEMUCapsSet(qemuCaps, props[i].flag); + break; + } + } + } +} + + +static void virQEMUCapsFreeStringList(size_t len, char **values) { @@ -2470,6 +2524,10 @@ virQEMUCapsProbeQMPObjects(virQEMUCapsPtr qemuCaps, virQEMUCapsObjectProps[i].nprops, virQEMUCapsObjectProps[i].props, nvalues, values); + virQEMUCapsProcessProps(qemuCaps, + ARRAY_CARDINALITY(virQEMUCapsPropObjects), + virQEMUCapsPropObjects, type, + nvalues, values); virQEMUCapsFreeStringList(nvalues, values); } diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index d249e2e..776a0f3 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -373,6 +373,7 @@ typedef enum { QEMU_CAPS_DISPLAY, /* -display */ QEMU_CAPS_DEVICE_INTEL_IOMMU, /* -device intel-iommu */ QEMU_CAPS_MACHINE_SMM_OPT, /* -machine xxx,smm=on/off/auto */ + QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY, /* virtio-*pci.disable-legacy */ QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml index 339ee1f..db778ef 100644 --- a/tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml @@ -184,6 +184,7 @@ <flag name='display'/> <flag name='intel-iommu'/> <flag name='smm'/> + <flag name='virtio-pci-disable-legacy'/> <version>2004000</version> <kvmVersion>0</kvmVersion> <package></package> diff --git a/tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml index c1a68d0..fc915ad 100644 --- a/tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml @@ -189,6 +189,7 @@ <flag name='display'/> <flag name='intel-iommu'/> <flag name='smm'/> + <flag name='virtio-pci-disable-legacy'/> <version>2005000</version> <kvmVersion>0</kvmVersion> <package></package> diff --git a/tests/qemucapabilitiesdata/caps_2.6.0-gicv2.aarch64.xml b/tests/qemucapabilitiesdata/caps_2.6.0-gicv2.aarch64.xml index 85d7d3f..fd14665 100644 --- a/tests/qemucapabilitiesdata/caps_2.6.0-gicv2.aarch64.xml +++ b/tests/qemucapabilitiesdata/caps_2.6.0-gicv2.aarch64.xml @@ -158,6 +158,7 @@ <flag name='tls-creds-x509'/> <flag name='display'/> <flag name='smm'/> + <flag name='virtio-pci-disable-legacy'/> <version>2005094</version> <kvmVersion>0</kvmVersion> <package></package> diff --git a/tests/qemucapabilitiesdata/caps_2.6.0-gicv3.aarch64.xml b/tests/qemucapabilitiesdata/caps_2.6.0-gicv3.aarch64.xml index deb1257..eb708f8 100644 --- a/tests/qemucapabilitiesdata/caps_2.6.0-gicv3.aarch64.xml +++ b/tests/qemucapabilitiesdata/caps_2.6.0-gicv3.aarch64.xml @@ -158,6 +158,7 @@ <flag name='tls-creds-x509'/> <flag name='display'/> <flag name='smm'/> + <flag name='virtio-pci-disable-legacy'/> <version>2005094</version> <kvmVersion>0</kvmVersion> <package></package> diff --git a/tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml b/tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml index 2b7ea0e..482b384 100644 --- a/tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml +++ b/tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml @@ -152,6 +152,7 @@ <flag name='tls-creds-x509'/> <flag name='display'/> <flag name='smm'/> + <flag name='virtio-pci-disable-legacy'/> <version>2005094</version> <kvmVersion>0</kvmVersion> <package></package> diff --git a/tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml index 495c114..60f1fcf 100644 --- a/tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml @@ -195,6 +195,7 @@ <flag name='display'/> <flag name='intel-iommu'/> <flag name='smm'/> + <flag name='virtio-pci-disable-legacy'/> <version>2006000</version> <kvmVersion>0</kvmVersion> <package></package> diff --git a/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml index fafffa6..ccb190b 100644 --- a/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml @@ -192,6 +192,7 @@ <flag name='display'/> <flag name='intel-iommu'/> <flag name='smm'/> + <flag name='virtio-pci-disable-legacy'/> <version>2006091</version> <kvmVersion>0</kvmVersion> <package> (v2.7.0-rc1-52-g42e0d60)</package> -- 2.7.3
https://bugzilla.redhat.com/show_bug.cgi?id=1227354 Translate the optional <virtio revision> attributes to disable-legacy=on/off and disable-modern=on/off options for the following devices: <memballoon> virtio-balloon-pci <disk> virtio-blk-pci <controller> virtio-scsi-pci virtio-serial-pci <filesystem> virtio-9p-pci <interface> virtio-net-pci <rng> virtio-rng-pci <video> virtio-gpu-pci <input> virtio-input-host-pci virtio-keyboard-pci virtio-mouse-pci virtio-tablet-pci --- v4: only format the options for devices with a PCI address use the renamed capability src/qemu/qemu_command.c | 68 ++++++++++++++++++++++ .../qemuxml2argv-virtio-revision.args | 62 ++++++++++++++++++++ tests/qemuxml2argvtest.c | 11 ++++ 3 files changed, 141 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.args diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 55df23d..efc6c3a 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -378,6 +378,44 @@ qemuBuildDeviceAddressStr(virBufferPtr buf, } static int +qemuBuildVirtioRevisionStr(virBufferPtr buf, + virBitmapPtr revmap, + virDomainDeviceInfoPtr info, + virQEMUCapsPtr qemuCaps) +{ + if (!revmap) + return 0; + + if (info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("setting the virtio revision is only supported " + "for PCI devices")); + return -1; + } + + if (virBitmapLastSetBit(revmap) && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("setting the virtio revision is not supported with " + "this QEMU binary")); + return -1; + } + + virBufferAddLit(buf, ",disable-legacy="); + if (virBitmapIsBitSet(revmap, VIR_DOMAIN_VIRTIO_REVISION_0_9)) + virBufferAddLit(buf, "off"); + else + virBufferAddLit(buf, "on"); + + virBufferAddLit(buf, ",disable-modern="); + if (virBitmapIsBitSet(revmap, VIR_DOMAIN_VIRTIO_REVISION_1_0)) + virBufferAddLit(buf, "off"); + else + virBufferAddLit(buf, "on"); + return 0; +} + +static int qemuBuildRomStr(virBufferPtr buf, virDomainDeviceInfoPtr info) { @@ -1988,6 +2026,11 @@ qemuBuildDriveDevStr(const virDomainDef *def, (disk->device == VIR_DOMAIN_DISK_DEVICE_LUN) ? "on" : "off"); } + + if (qemuBuildVirtioRevisionStr(&opt, disk->virtio_rev, &disk->info, + qemuCaps) < 0) + goto error; + if (qemuBuildDeviceAddressStr(&opt, def, &disk->info, qemuCaps) < 0) goto error; break; @@ -2309,6 +2352,8 @@ qemuBuildFSDevStr(const virDomainDef *def, QEMU_FSDEV_HOST_PREFIX, fs->info.alias); virBufferAsprintf(&opt, ",mount_tag=%s", fs->dst); + qemuBuildVirtioRevisionStr(&opt, fs->virtio_rev, &fs->info, qemuCaps); + if (qemuBuildDeviceAddressStr(&opt, def, &fs->info, qemuCaps) < 0) goto error; @@ -2566,6 +2611,9 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, def->iothread); } } + if (qemuBuildVirtioRevisionStr(&buf, def->virtio_rev, &def->info, + qemuCaps) < 0) + goto error; break; case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC: virBufferAddLit(&buf, "lsi"); @@ -2611,6 +2659,9 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, virBufferAsprintf(&buf, ",vectors=%d", def->opts.vioserial.vectors); } + if (qemuBuildVirtioRevisionStr(&buf, def->virtio_rev, &def->info, + qemuCaps) < 0) + goto error; break; case VIR_DOMAIN_CONTROLLER_TYPE_CCID: @@ -3555,12 +3606,16 @@ qemuBuildNicDevStr(virDomainDefPtr def, virBufferAsprintf(&buf, ",id=%s", net->info.alias); virBufferAsprintf(&buf, ",mac=%s", virMacAddrFormat(&net->mac, macaddr)); + if (qemuBuildDeviceAddressStr(&buf, def, &net->info, qemuCaps) < 0) goto error; if (qemuBuildRomStr(&buf, &net->info) < 0) goto error; if (bootindex && virQEMUCapsGet(qemuCaps, QEMU_CAPS_BOOTINDEX)) virBufferAsprintf(&buf, ",bootindex=%u", bootindex); + if (usingVirtio && + qemuBuildVirtioRevisionStr(&buf, net->virtio_rev, &net->info, qemuCaps) < 0) + goto error; if (virBufferCheckError(&buf) < 0) goto error; @@ -3825,6 +3880,10 @@ qemuBuildMemballoonCommandLine(virCommandPtr cmd, virTristateSwitchTypeToString(def->memballoon->autodeflate)); } + if (qemuBuildVirtioRevisionStr(&buf, def->memballoon->virtio_rev, + &def->memballoon->info, qemuCaps) < 0) + goto error; + virCommandAddArg(cmd, "-device"); virCommandAddArgBuffer(cmd, &buf); return 0; @@ -3955,6 +4014,9 @@ qemuBuildVirtioInputDevStr(const virDomainDef *def, if (qemuBuildDeviceAddressStr(&buf, def, &dev->info, qemuCaps) < 0) goto error; + if (qemuBuildVirtioRevisionStr(&buf, dev->virtio_rev, &dev->info, qemuCaps) < 0) + goto error; + if (virBufferCheckError(&buf) < 0) goto error; @@ -4320,6 +4382,9 @@ qemuBuildDeviceVideoStr(const virDomainDef *def, if (qemuBuildDeviceAddressStr(&buf, def, &video->info, qemuCaps) < 0) goto error; + if (qemuBuildVirtioRevisionStr(&buf, video->virtio_rev, &video->info, qemuCaps) < 0) + goto error; + if (virBufferCheckError(&buf) < 0) goto error; @@ -5566,6 +5631,9 @@ qemuBuildRNGDevStr(const virDomainDef *def, virBufferAddLit(&buf, ",period=1000"); } + if (qemuBuildVirtioRevisionStr(&buf, dev->virtio_rev, &dev->info, qemuCaps) < 0) + goto error; + if (qemuBuildDeviceAddressStr(&buf, def, &dev->info, qemuCaps) < 0) goto error; if (virBufferCheckError(&buf) < 0) diff --git a/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.args b/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.args new file mode 100644 index 0000000..79b771f --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.args @@ -0,0 +1,62 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu \ +-name QEMUGuest1 \ +-S \ +-M pc \ +-m 214 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-nographic \ +-nodefaults \ +-monitor unix:/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait \ +-no-acpi \ +-boot c \ +-device virtio-scsi-pci,disable-legacy=off,disable-modern=on,id=scsi0,\ +bus=pci.0,addr=0x8 \ +-device virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x9 \ +-usb \ +-drive file=/var/lib/libvirt/images/img1,format=raw,if=none,\ +id=drive-virtio-disk0 \ +-device virtio-blk-pci,disable-legacy=off,disable-modern=on,bus=pci.0,addr=0xa,\ +drive=drive-virtio-disk0,id=virtio-disk0 \ +-drive file=/var/lib/libvirt/images/img2,format=raw,if=none,\ +id=drive-virtio-disk1 \ +-device virtio-blk-pci,disable-legacy=on,disable-modern=off,bus=pci.0,addr=0xb,\ +drive=drive-virtio-disk1,id=virtio-disk1 \ +-drive file=/var/lib/libvirt/images/img3,format=raw,if=none,\ +id=drive-scsi0-0-0-0 \ +-device scsi-disk,bus=scsi0.0,channel=0,scsi-id=0,lun=0,\ +drive=drive-scsi0-0-0-0,id=scsi0-0-0-0 \ +-fsdev local,security_model=passthrough,id=fsdev-fs0,path=/export/fs1 \ +-device virtio-9p-pci,id=fs0,fsdev=fsdev-fs0,mount_tag=fs1,disable-legacy=off,\ +disable-modern=on,bus=pci.0,addr=0x3 \ +-fsdev local,security_model=mapped,writeout=immediate,id=fsdev-fs1,\ +path=/export/fs2 \ +-device virtio-9p-pci,id=fs1,fsdev=fsdev-fs1,mount_tag=fs2,disable-legacy=on,\ +disable-modern=off,bus=pci.0,addr=0x4 \ +-device virtio-net-pci,vlan=0,id=net0,mac=52:54:56:58:5a:5c,bus=pci.0,addr=0x6,\ +disable-legacy=off,disable-modern=on \ +-net user,vlan=0,name=hostnet0 \ +-device virtio-net-pci,vlan=1,id=net1,mac=52:54:56:5a:5c:5e,bus=pci.0,addr=0x7,\ +disable-legacy=on,disable-modern=off \ +-net user,vlan=1,name=hostnet1 \ +-device virtio-mouse-pci,id=input0,bus=pci.0,addr=0xe,disable-legacy=on,\ +disable-modern=off \ +-device virtio-keyboard-pci,id=input1,bus=pci.0,addr=0x10,disable-legacy=on,\ +disable-modern=off \ +-device virtio-tablet-pci,id=input2,bus=pci.0,addr=0x11,disable-legacy=on,\ +disable-modern=off \ +-device virtio-input-host-pci,id=input3,evdev=/dev/input/event1234,bus=pci.0,\ +addr=0x12,disable-legacy=on,disable-modern=off \ +-device virtio-vga,id=video0,virgl=on,bus=pci.0,addr=0x2,disable-legacy=on,\ +disable-modern=off \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0xc,disable-legacy=off,\ +disable-modern=off \ +-object rng-random,id=objrng0,filename=/dev/random \ +-device virtio-rng-pci,rng=objrng0,id=rng0,disable-legacy=on,\ +disable-modern=off,bus=pci.0,addr=0xd diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index ad0693f..f600638 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -2103,6 +2103,17 @@ mymain(void) DO_TEST("intel-iommu", QEMU_CAPS_DEVICE_PCI_BRIDGE, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, QEMU_CAPS_DEVICE_INTEL_IOMMU); + DO_TEST("virtio-revision", QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_VIRTIO_KEYBOARD, + QEMU_CAPS_VIRTIO_MOUSE, QEMU_CAPS_VIRTIO_TABLET, + QEMU_CAPS_VIRTIO_INPUT_HOST, + QEMU_CAPS_FSDEV, QEMU_CAPS_FSDEV_WRITEOUT, + QEMU_CAPS_DEVICE_VIRTIO_GPU, + QEMU_CAPS_DEVICE_VIRTIO_GPU_VIRGL, + QEMU_CAPS_DEVICE_VIRTIO_RNG, + QEMU_CAPS_OBJECT_RNG_RANDOM, + QEMU_CAPS_DEVICE_VIDEO_PRIMARY, + QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY); + qemuTestDriverFree(&driver); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; -- 2.7.3
On Thu, 2016-08-11 at 13:57 +0200, Ján Tomko wrote:
Check whether the disable-legacy property is present on the following devices: virtio-balloon-pci virtio-blk-pci virtio-scsi-pci virtio-serial-pci virtio-9p-pci virtio-net-pci virtio-rng-pci virtio-gpu-pci virtio-input-host-pci virtio-keyboard-pci virtio-mouse-pci virtio-tablet-pci Assuming that if QEMU knows other virtio devices where this property is applicable, it will have at least one of these devices. Added in QEMU by: commit e266d421490e0ae83044bbebb209b2d3650c0ba6 virtio-pci: add flags to enable/disable legacy/modern
I looked at this patch because it's a requirement for Laine's PCIe series. I'll just point out a couple things; I see there have been a few comments about the design of the interface that you'll need to address, so I don't think it's very useful to look at the whole series before you've had a chance to do so.
+struct virQEMUCapsPropObjects { + const char *prop; + int flag; + const char **objects; +}; + +static const char *virQEMUCapsVirtioPCIDisableLegacyObjects[] = { + "virtio-balloon-pci", + "virtio-blk-pci", + "virtio-scsi-pci", + "virtio-serial-pci", + "virtio-9p-pci", + "virtio-net-pci", + "virtio-rng-pci", + "virtio-gpu-pci", + "virtio-input-host-pci", + "virtio-keyboard-pci", + "virtio-mouse-pci", + "virtio-tablet-pci", + NULL +}; + +static struct virQEMUCapsPropObjects virQEMUCapsPropObjects[] = {
Please, don't :) Use something like virQEMUCapsPropTypeObjects (to mirror the existing virQEMUCapsObjectTypeProps), or virQEMUCapsPropObjectsType, or anything really - just make sure the name of the type and the name of the variable containing a bunch of instances of said type are not the same.
static void +virQEMUCapsProcessProps(virQEMUCapsPtr qemuCaps, + size_t nprops, + struct virQEMUCapsPropObjects *props, + const char *object, + size_t nvalues, + char *const*values) +{ + size_t i, j; + + for (i = 0; i < nprops; i++) { + if (virQEMUCapsGet(qemuCaps, props[i].flag)) + continue; + + for (j = 0; j < nvalues; j++) { + if (STREQ(values[j], props[i].prop)) { + if (virStringArrayHasString((char **)props[i].objects, object))
Rather than casting a const char ** to char **, which happens in other places as well, it would be IMHO much better to make virStringArrayHasString() accept a const char ** as the first argument. And guess what? I just posted a patch[1] that does exactly that :) Everything else looks good. [1] https://www.redhat.com/archives/libvir-list/2016-August/msg00784.html -- Andrea Bolognani / Red Hat / Virtualization
On 08/16/2016 07:50 AM, Andrea Bolognani wrote:
Check whether the disable-legacy property is present on the following devices: virtio-balloon-pci virtio-blk-pci virtio-scsi-pci virtio-serial-pci virtio-9p-pci virtio-net-pci virtio-rng-pci virtio-gpu-pci virtio-input-host-pci virtio-keyboard-pci virtio-mouse-pci virtio-tablet-pci
Assuming that if QEMU knows other virtio devices where this property is applicable, it will have at least one of these devices.
Added in QEMU by: commit e266d421490e0ae83044bbebb209b2d3650c0ba6 virtio-pci: add flags to enable/disable legacy/modern I looked at this patch because it's a requirement for Laine's PCIe series. I'll just point out a couple things; I see there have been a few comments about the design of the interface
On Thu, 2016-08-11 at 13:57 +0200, Ján Tomko wrote: that you'll need to address, so I don't think it's very useful to look at the whole series before you've had a chance to do so.
+struct virQEMUCapsPropObjects { + const char *prop; + int flag; + const char **objects; +}; + +static const char *virQEMUCapsVirtioPCIDisableLegacyObjects[] = { + "virtio-balloon-pci", + "virtio-blk-pci", + "virtio-scsi-pci", + "virtio-serial-pci", + "virtio-9p-pci", + "virtio-net-pci", + "virtio-rng-pci", + "virtio-gpu-pci", + "virtio-input-host-pci", + "virtio-keyboard-pci", + "virtio-mouse-pci", + "virtio-tablet-pci", + NULL +}; + +static struct virQEMUCapsPropObjects virQEMUCapsPropObjects[] = { Please, don't :)
Use something like virQEMUCapsPropTypeObjects (to mirror the existing virQEMUCapsObjectTypeProps), or virQEMUCapsPropObjectsType, or anything really - just make sure the name of the type and the name of the variable containing a bunch of instances of said type are not the same.
static void +virQEMUCapsProcessProps(virQEMUCapsPtr qemuCaps, + size_t nprops, + struct virQEMUCapsPropObjects *props, + const char *object, + size_t nvalues, + char *const*values) +{ + size_t i, j; + + for (i = 0; i < nprops; i++) { + if (virQEMUCapsGet(qemuCaps, props[i].flag)) + continue; + + for (j = 0; j < nvalues; j++) { + if (STREQ(values[j], props[i].prop)) { + if (virStringArrayHasString((char **)props[i].objects, object)) Rather than casting a const char ** to char **, which happens in other places as well, it would be IMHO much better to make virStringArrayHasString() accept a const char ** as the first argument.
And guess what? I just posted a patch[1] that does exactly that :)
Everything else looks good.
I'll ACK this pending the two changes abologna suggested. If you're confident you won't want to change this, but might be delayed in redoing the rest of the series, feel free to push this one ahead of the rest, as I am using it in my "Use more PCIe less PCI" series, which is mostly ACKed.
On Wed, Aug 17, 2016 at 11:43:50AM -0400, Laine Stump wrote:
On 08/16/2016 07:50 AM, Andrea Bolognani wrote:
On Thu, 2016-08-11 at 13:57 +0200, Ján Tomko wrote:
+static struct virQEMUCapsPropObjects virQEMUCapsPropObjects[] = { Please, don't :)
Use something like virQEMUCapsPropTypeObjects (to mirror the existing virQEMUCapsObjectTypeProps), or virQEMUCapsPropObjectsType, or anything really - just make sure the name of the type and the name of the variable containing a bunch of instances of said type are not the same.
static void +virQEMUCapsProcessProps(virQEMUCapsPtr qemuCaps, + size_t nprops, + struct virQEMUCapsPropObjects *props, + const char *object, + size_t nvalues, + char *const*values) +{ + size_t i, j; + + for (i = 0; i < nprops; i++) { + if (virQEMUCapsGet(qemuCaps, props[i].flag)) + continue; + + for (j = 0; j < nvalues; j++) { + if (STREQ(values[j], props[i].prop)) { + if (virStringArrayHasString((char **)props[i].objects, object)) Rather than casting a const char ** to char **, which happens in other places as well, it would be IMHO much better to make virStringArrayHasString() accept a const char ** as the first argument.
And guess what? I just posted a patch[1] that does exactly that :)
Everything else looks good.
I'll ACK this pending the two changes abologna suggested. If you're confident you won't want to change this, but might be delayed in redoing the rest of the series, feel free to push this one ahead of the rest, as I am using it in my "Use more PCIe less PCI" series, which is mostly ACKed.
Thanks, pushed now. Jan
participants (6)
-
Andrea Bolognani -
Boris Fiuczynski -
Cornelia Huck -
Daniel P. Berrange -
Ján Tomko -
Laine Stump