[libvirt] [PATCHv5 00/13] 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 v4: * only probe the capability for PCI devices v5: * instead of a separate element, use one attribute under the driver element Ján Tomko (13): Use a separate buffer for <input> subelements Use a separate buffer for <disk><driver> Use a separate buffer for <controller><driver> Use a separate buffer for <filesystem><driver> Add compatibility attribute to memballoon Add compatibility attribute to disks Add compatibility attribute to controllers Add compatibility attribute to filesystems Add compatibility attribute to interfaces Add compatibility attribute to rng devices Add compatibility attribute to video Add compatibility attribute to input devices qemu: format options for enforcing virtio revisions docs/formatdomain.html.in | 60 +++++- docs/schemas/domaincommon.rng | 42 ++++ src/conf/domain_conf.c | 220 +++++++++++++++------ src/conf/domain_conf.h | 17 ++ src/qemu/qemu_command.c | 70 +++++++ .../qemuxml2argv-virtio-revision.args | 62 ++++++ .../qemuxml2argv-virtio-revision.xml | 109 ++++++++++ tests/qemuxml2argvtest.c | 11 ++ .../qemuxml2xmlout-virtio-revision.xml | 108 ++++++++++ tests/qemuxml2xmltest.c | 2 + 10 files changed, 641 insertions(+), 60 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 14d4f7d..3ed9040 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -21852,6 +21852,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 && @@ -21874,17 +21875,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
On Wed, Aug 24, 2016 at 00:20:43 +0200, Ján Tomko wrote:
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(-)
ACK
Eliminate the big condition at the start. Instead use a buffer and only format the element if the buffer is non-empty. --- src/conf/domain_conf.c | 55 +++++++++++++++++++++++++------------------------- 1 file changed, 27 insertions(+), 28 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3ed9040..99e8a8b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -19810,6 +19810,7 @@ virDomainDiskDefFormat(virBufferPtr buf, const char *sgio = virDomainDeviceSGIOTypeToString(def->sgio); const char *discard = virDomainDiskDiscardTypeToString(def->discard); const char *detect_zeroes = virDomainDiskDetectZeroesTypeToString(def->detect_zeroes); + virBuffer driverBuf = VIR_BUFFER_INITIALIZER; if (!type || !def->src->type) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -19861,35 +19862,33 @@ virDomainDiskDefFormat(virBufferPtr buf, virBufferAddLit(buf, ">\n"); virBufferAdjustIndent(buf, 2); - if (def->src->driverName || def->src->format > 0 || def->cachemode || - def->error_policy || def->rerror_policy || def->iomode || - def->ioeventfd || def->event_idx || def->copy_on_read || - def->discard || def->iothread || def->detect_zeroes) { + virBufferEscapeString(&driverBuf, " name='%s'", def->src->driverName); + if (def->src->format > 0) + virBufferAsprintf(&driverBuf, " type='%s'", + virStorageFileFormatTypeToString(def->src->format)); + if (def->cachemode) + virBufferAsprintf(&driverBuf, " cache='%s'", cachemode); + if (def->error_policy) + virBufferAsprintf(&driverBuf, " error_policy='%s'", error_policy); + if (def->rerror_policy) + virBufferAsprintf(&driverBuf, " rerror_policy='%s'", rerror_policy); + if (def->iomode) + virBufferAsprintf(&driverBuf, " io='%s'", iomode); + if (def->ioeventfd) + virBufferAsprintf(&driverBuf, " ioeventfd='%s'", ioeventfd); + if (def->event_idx) + virBufferAsprintf(&driverBuf, " event_idx='%s'", event_idx); + if (def->copy_on_read) + virBufferAsprintf(&driverBuf, " copy_on_read='%s'", copy_on_read); + if (def->discard) + virBufferAsprintf(&driverBuf, " discard='%s'", discard); + if (def->iothread) + virBufferAsprintf(&driverBuf, " iothread='%u'", def->iothread); + if (def->detect_zeroes) + virBufferAsprintf(&driverBuf, " detect_zeroes='%s'", detect_zeroes); + if (virBufferUse(&driverBuf)) { virBufferAddLit(buf, "<driver"); - virBufferEscapeString(buf, " name='%s'", def->src->driverName); - if (def->src->format > 0) - virBufferAsprintf(buf, " type='%s'", - virStorageFileFormatTypeToString(def->src->format)); - if (def->cachemode) - virBufferAsprintf(buf, " cache='%s'", cachemode); - if (def->error_policy) - virBufferAsprintf(buf, " error_policy='%s'", error_policy); - if (def->rerror_policy) - virBufferAsprintf(buf, " rerror_policy='%s'", rerror_policy); - if (def->iomode) - virBufferAsprintf(buf, " io='%s'", iomode); - if (def->ioeventfd) - virBufferAsprintf(buf, " ioeventfd='%s'", ioeventfd); - if (def->event_idx) - virBufferAsprintf(buf, " event_idx='%s'", event_idx); - if (def->copy_on_read) - virBufferAsprintf(buf, " copy_on_read='%s'", copy_on_read); - if (def->discard) - virBufferAsprintf(buf, " discard='%s'", discard); - if (def->iothread) - virBufferAsprintf(buf, " iothread='%u'", def->iothread); - if (def->detect_zeroes) - virBufferAsprintf(buf, " detect_zeroes='%s'", detect_zeroes); + virBufferAddBuffer(buf, &driverBuf); virBufferAddLit(buf, "/>\n"); } -- 2.7.3
On Wed, Aug 24, 2016 at 00:20:44 +0200, Ján Tomko wrote:
Eliminate the big condition at the start. Instead use a buffer and only format the element if the buffer is non-empty. --- src/conf/domain_conf.c | 55 +++++++++++++++++++++++++------------------------- 1 file changed, 27 insertions(+), 28 deletions(-)
ACK
Make adding new attributes easier. --- src/conf/domain_conf.c | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 99e8a8b..af1c12e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -20080,6 +20080,7 @@ virDomainControllerDefFormat(virBufferPtr buf, const char *model = NULL; const char *modelName = NULL; bool pcihole64 = false, pciModel = false, pciTarget = false; + virBuffer driverBuf = VIR_BUFFER_INITIALIZER; if (!type) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -20184,26 +20185,26 @@ virDomainControllerDefFormat(virBufferPtr buf, } } - if (def->queues || def->cmd_per_lun || - def->max_sectors || def->ioeventfd || def->iothread) { - virBufferAddLit(buf, "<driver"); - if (def->queues) - virBufferAsprintf(buf, " queues='%u'", def->queues); + if (def->queues) + virBufferAsprintf(&driverBuf, " queues='%u'", def->queues); - if (def->cmd_per_lun) - virBufferAsprintf(buf, " cmd_per_lun='%u'", def->cmd_per_lun); + if (def->cmd_per_lun) + virBufferAsprintf(&driverBuf, " cmd_per_lun='%u'", def->cmd_per_lun); - if (def->max_sectors) - virBufferAsprintf(buf, " max_sectors='%u'", def->max_sectors); + if (def->max_sectors) + virBufferAsprintf(&driverBuf, " max_sectors='%u'", def->max_sectors); - if (def->ioeventfd) { - virBufferAsprintf(buf, " ioeventfd='%s'", - virTristateSwitchTypeToString(def->ioeventfd)); - } + if (def->ioeventfd) { + virBufferAsprintf(&driverBuf, " ioeventfd='%s'", + virTristateSwitchTypeToString(def->ioeventfd)); + } - if (def->iothread) - virBufferAsprintf(buf, " iothread='%u'", def->iothread); + if (def->iothread) + virBufferAsprintf(&driverBuf, " iothread='%u'", def->iothread); + if (virBufferUse(&driverBuf)) { + virBufferAddLit(buf, "<driver"); + virBufferAddBuffer(buf, &driverBuf); virBufferAddLit(buf, "/>\n"); } -- 2.7.3
On Wed, Aug 24, 2016 at 00:20:45 +0200, Ján Tomko wrote:
Make adding new attributes easier. --- src/conf/domain_conf.c | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-)
ACK
Format the attributes in a separate buffer and only print the element if it's not empty. --- src/conf/domain_conf.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index af1c12e..1e694fd 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -20252,6 +20252,7 @@ virDomainFSDefFormat(virBufferPtr buf, const char *fsdriver = virDomainFSDriverTypeToString(def->fsdriver); const char *wrpolicy = virDomainFSWrpolicyTypeToString(def->wrpolicy); const char *src = def->src->path; + virBuffer driverBuf = VIR_BUFFER_INITIALIZER; if (!type) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -20271,16 +20272,21 @@ virDomainFSDefFormat(virBufferPtr buf, type, accessmode); virBufferAdjustIndent(buf, 2); if (def->fsdriver) { - virBufferAsprintf(buf, "<driver type='%s'", fsdriver); + virBufferAsprintf(&driverBuf, " type='%s'", fsdriver); if (def->format) - virBufferAsprintf(buf, " format='%s'", + virBufferAsprintf(&driverBuf, " format='%s'", virStorageFileFormatTypeToString(def->format)); /* Don't generate anything if wrpolicy is set to default */ if (def->wrpolicy) - virBufferAsprintf(buf, " wrpolicy='%s'", wrpolicy); + virBufferAsprintf(&driverBuf, " wrpolicy='%s'", wrpolicy); + + } + if (virBufferUse(&driverBuf)) { + virBufferAddLit(buf, "<driver"); + virBufferAddBuffer(buf, &driverBuf); virBufferAddLit(buf, "/>\n"); } -- 2.7.3
On Wed, Aug 24, 2016 at 00:20:46 +0200, Ján Tomko wrote:
Format the attributes in a separate buffer and only print the element if it's not empty. --- src/conf/domain_conf.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)
ACK
A new attribute to alter the virtio revision: <memballoon model='virtio'> <driver compatibility='transitional'/> </memballoon> https://bugzilla.redhat.com/show_bug.cgi?id=1227354 --- docs/formatdomain.html.in | 8 ++++ docs/schemas/domaincommon.rng | 15 ++++++ src/conf/domain_conf.c | 40 ++++++++++++++++ src/conf/domain_conf.h | 10 ++++ .../qemuxml2argv-virtio-revision.xml | 53 ++++++++++++++++++++++ .../qemuxml2xmlout-virtio-revision.xml | 53 ++++++++++++++++++++++ tests/qemuxml2xmltest.c | 2 + 7 files changed, 181 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 bfbb0f2..56dddbd 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -6373,6 +6373,7 @@ qemu-kvm -net nic,model=? /dev/null <memballoon model='virtio'> <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> <stats period='10'/> + <driver compatibility='transitional'/> </memballoon> </devices> </domain></pre> @@ -6417,6 +6418,13 @@ qemu-kvm -net nic,model=? /dev/null <span class='since'>Since 1.1.1, requires QEMU 1.5</span> </p> </dd> + <dt><code>driver</code></dt> + <dd> + The <code>compatibility</code> attribute can be used to specify the + compatibility of virtio devices. Allowed values are <code>legacy</code>, + <code>transitional</code> and <code>modern</code>. + <span class="since">Since 2.2.0</span>. + </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..8ed4b9d 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3612,6 +3612,11 @@ </attribute> </element> </optional> + <optional> + <element name="driver"> + <ref name="compatibility"/> + </element> + </optional> </interleave> </element> </define> @@ -4830,6 +4835,16 @@ </element> </define> + <define name="compatibility"> + <attribute name="compatibility"> + <choice> + <value>legacy</value> + <value>transitional</value> + <value>modern</value> + </choice> + </attribute> + </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 1e694fd..53c3453 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -834,6 +834,13 @@ VIR_ENUM_IMPL(virDomainLoader, "rom", "pflash") +VIR_ENUM_IMPL(virDomainDriverCompatibility, + VIR_DOMAIN_DRIVER_COMPATIBILITY_LAST, + "default", + "legacy", + "transitional", + "modern") + /* 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 +1066,31 @@ virDomainXMLOptionGetNamespace(virDomainXMLOptionPtr xmlopt) return &xmlopt->ns; } +static int +virDomainDriverCompatibilityParseXML(xmlXPathContextPtr ctxt, + virDomainDriverCompatibility *res) +{ + char *str = NULL; + int ret = -1; + int val; + + if (!(str = virXPathString("string(./driver/@compatibility)", ctxt))) + return 0; + + if ((val = virDomainDriverCompatibilityTypeFromString(str)) < 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("unable to parse the compatibility attribute")); + goto cleanup; + } + + *res = val; + ret = 0; + + cleanup: + VIR_FREE(str); + return ret; +} + void virBlkioDeviceArrayClear(virBlkioDevicePtr devices, @@ -12065,6 +12097,9 @@ virDomainMemballoonDefParseXML(xmlNodePtr node, else if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0) goto error; + if (virDomainDriverCompatibilityParseXML(ctxt, &def->compatibility) < 0) + goto error; + cleanup: VIR_FREE(model); VIR_FREE(deflate); @@ -21506,6 +21541,11 @@ virDomainMemballoonDefFormat(virBufferPtr buf, return -1; } + if (def->compatibility) { + virBufferAsprintf(&childrenBuf, "<driver compatibility='%s'/>\n", + virDomainDriverCompatibilityTypeToString(def->compatibility)); + } + if (!virBufferUse(&childrenBuf)) { virBufferAddLit(buf, "/>\n"); } else { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 8b26724..ac9f552 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -154,6 +154,14 @@ typedef virDomainTPMDef *virDomainTPMDefPtr; typedef struct _virDomainIOMMUDef virDomainIOMMUDef; typedef virDomainIOMMUDef *virDomainIOMMUDefPtr; +typedef enum { + VIR_DOMAIN_DRIVER_COMPATIBILITY_DEFAULT, + VIR_DOMAIN_DRIVER_COMPATIBILITY_LEGACY, + VIR_DOMAIN_DRIVER_COMPATIBILITY_TRANSITIONAL, + VIR_DOMAIN_DRIVER_COMPATIBILITY_MODERN, + VIR_DOMAIN_DRIVER_COMPATIBILITY_LAST, +} virDomainDriverCompatibility; + /* Flags for the 'type' field in virDomainDeviceDef */ typedef enum { VIR_DOMAIN_DEVICE_NONE = 0, @@ -1546,6 +1554,7 @@ struct _virDomainMemballoonDef { virDomainDeviceInfo info; int period; /* seconds between collections */ int autodeflate; /* enum virTristateSwitch */ + virDomainDriverCompatibility compatibility; }; struct _virDomainNVRAMDef { @@ -3022,6 +3031,7 @@ VIR_ENUM_DECL(virDomainTPMBackend) VIR_ENUM_DECL(virDomainMemoryModel) VIR_ENUM_DECL(virDomainMemoryBackingModel) VIR_ENUM_DECL(virDomainIOMMUModel) +VIR_ENUM_DECL(virDomainDriverCompatibility) /* 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..2e71db1 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml @@ -0,0 +1,53 @@ +<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'/> + <driver compatibility='transitional'/> + </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..2e71db1 --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml @@ -0,0 +1,53 @@ +<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'/> + <driver compatibility='transitional'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 7601a5f..811e8c9 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("virtio-revision", QEMU_CAPS_VIRTIO_SCSI); + virObjectUnref(cfg); DO_TEST("acpi-table", NONE); -- 2.7.3
On Wed, Aug 24, 2016 at 00:20:47 +0200, Ján Tomko wrote:
A new attribute to alter the virtio revision: <memballoon model='virtio'> <driver compatibility='transitional'/> </memballoon>
https://bugzilla.redhat.com/show_bug.cgi?id=1227354 --- docs/formatdomain.html.in | 8 ++++ docs/schemas/domaincommon.rng | 15 ++++++ src/conf/domain_conf.c | 40 ++++++++++++++++ src/conf/domain_conf.h | 10 ++++ .../qemuxml2argv-virtio-revision.xml | 53 ++++++++++++++++++++++ .../qemuxml2xmlout-virtio-revision.xml | 53 ++++++++++++++++++++++ tests/qemuxml2xmltest.c | 2 + 7 files changed, 181 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 bfbb0f2..56dddbd 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -6373,6 +6373,7 @@ qemu-kvm -net nic,model=? /dev/null <memballoon model='virtio'> <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> <stats period='10'/> + <driver compatibility='transitional'/>
Hmm, this is a though one. Since virtio is not entirely meant to be qemu specific but rather a standard the setting here makes sense.
</memballoon> </devices> </domain></pre> @@ -6417,6 +6418,13 @@ qemu-kvm -net nic,model=? /dev/null <span class='since'>Since 1.1.1, requires QEMU 1.5</span> </p> </dd> + <dt><code>driver</code></dt> + <dd> + The <code>compatibility</code> attribute can be used to specify the + compatibility of virtio devices. Allowed values are <code>legacy</code>, + <code>transitional</code> and <code>modern</code>. + <span class="since">Since 2.2.0</span>.
I'd like to see a more specific description of the individual values. Since virtio is meant to be a standard, we should describe the vales a bit more IMO.
+ </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..8ed4b9d 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3612,6 +3612,11 @@ </attribute> </element> </optional> + <optional> + <element name="driver"> + <ref name="compatibility"/> + </element> + </optional> </interleave> </element> </define> @@ -4830,6 +4835,16 @@ </element> </define>
+ <define name="compatibility">
I'd add a <optional> block here since the attribute should not be mandatory.
+ <attribute name="compatibility"> + <choice> + <value>legacy</value> + <value>transitional</value> + <value>modern</value> + </choice> + </attribute> + </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 1e694fd..53c3453 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -834,6 +834,13 @@ VIR_ENUM_IMPL(virDomainLoader, "rom", "pflash")
+VIR_ENUM_IMPL(virDomainDriverCompatibility, + VIR_DOMAIN_DRIVER_COMPATIBILITY_LAST, + "default", + "legacy", + "transitional", + "modern") + /* 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 +1066,31 @@ virDomainXMLOptionGetNamespace(virDomainXMLOptionPtr xmlopt) return &xmlopt->ns; }
+static int +virDomainDriverCompatibilityParseXML(xmlXPathContextPtr ctxt, + virDomainDriverCompatibility *res) +{ + char *str = NULL; + int ret = -1; + int val; + + if (!(str = virXPathString("string(./driver/@compatibility)", ctxt))) + return 0; + + if ((val = virDomainDriverCompatibilityTypeFromString(str)) < 0) {
This still allows to use the "default" setting which is not documented though.
+ virReportError(VIR_ERR_XML_ERROR, "%s", + _("unable to parse the compatibility attribute")); + goto cleanup; + } + + *res = val; + ret = 0; + + cleanup: + VIR_FREE(str); + return ret; +} +
void virBlkioDeviceArrayClear(virBlkioDevicePtr devices, @@ -12065,6 +12097,9 @@ virDomainMemballoonDefParseXML(xmlNodePtr node, else if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0) goto error;
+ if (virDomainDriverCompatibilityParseXML(ctxt, &def->compatibility) < 0) + goto error; + cleanup: VIR_FREE(model); VIR_FREE(deflate); @@ -21506,6 +21541,11 @@ virDomainMemballoonDefFormat(virBufferPtr buf, return -1; }
+ if (def->compatibility) {
"default" is not formatted though.
+ virBufferAsprintf(&childrenBuf, "<driver compatibility='%s'/>\n", + virDomainDriverCompatibilityTypeToString(def->compatibility)); + } + if (!virBufferUse(&childrenBuf)) { virBufferAddLit(buf, "/>\n"); } else { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 8b26724..ac9f552 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -154,6 +154,14 @@ typedef virDomainTPMDef *virDomainTPMDefPtr; typedef struct _virDomainIOMMUDef virDomainIOMMUDef; typedef virDomainIOMMUDef *virDomainIOMMUDefPtr;
+typedef enum { + VIR_DOMAIN_DRIVER_COMPATIBILITY_DEFAULT, + VIR_DOMAIN_DRIVER_COMPATIBILITY_LEGACY, + VIR_DOMAIN_DRIVER_COMPATIBILITY_TRANSITIONAL, + VIR_DOMAIN_DRIVER_COMPATIBILITY_MODERN, + VIR_DOMAIN_DRIVER_COMPATIBILITY_LAST, +} virDomainDriverCompatibility; + /* Flags for the 'type' field in virDomainDeviceDef */ typedef enum { VIR_DOMAIN_DEVICE_NONE = 0, @@ -1546,6 +1554,7 @@ struct _virDomainMemballoonDef { virDomainDeviceInfo info; int period; /* seconds between collections */ int autodeflate; /* enum virTristateSwitch */ + virDomainDriverCompatibility compatibility; };
struct _virDomainNVRAMDef { @@ -3022,6 +3031,7 @@ VIR_ENUM_DECL(virDomainTPMBackend) VIR_ENUM_DECL(virDomainMemoryModel) VIR_ENUM_DECL(virDomainMemoryBackingModel) VIR_ENUM_DECL(virDomainIOMMUModel) +VIR_ENUM_DECL(virDomainDriverCompatibility) /* from libvirt.h */ VIR_ENUM_DECL(virDomainState) VIR_ENUM_DECL(virDomainNostateReason)
ACK if you clarify the docs from me. I think this is the least worst approach we can use and still hide the very weird tuple of qemu arguments used to configure this. Peter
A new attribute to alter the virtio revision: <disk> <driver compatibility='transitional'/> </disk> https://bugzilla.redhat.com/show_bug.cgi?id=1227354 --- docs/formatdomain.html.in | 8 +++++++- docs/schemas/domaincommon.rng | 3 +++ src/conf/domain_conf.c | 7 +++++++ src/conf/domain_conf.h | 1 + tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml | 12 ++++++++++++ tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml | 12 ++++++++++++ 6 files changed, 42 insertions(+), 1 deletion(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 56dddbd..3f06613 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2084,7 +2084,7 @@ <target dev='vdc' bus='virtio'/> </disk> <disk type='file' device='disk'> - <driver name='qemu' type='qcow2'/> + <driver name='qemu' type='qcow2' compatibility='modern'/> <source file='/var/lib/libvirt/images/domain.qcow'/> <backingStore type='file'> <format type='qcow2'/> @@ -2667,6 +2667,12 @@ <code>bus</code> and "pci" or "ccw" <code>address</code> types. <span class='since'>Since 1.2.8 (QEMU 2.1)</span> </li> + <li> + The <code>compatibility</code> attribute can be used to specify + the compatibility of virtio devices. Allowed values are + <code>legacy</code>, <code>transitional</code> and <code>modern</code>. + <span class="since">Since 2.2.0</span>. + </li> </ul> </dd> <dt><code>backenddomain</code></dt> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 8ed4b9d..2cf3c1b 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1641,6 +1641,9 @@ <optional> <ref name="detect_zeroes"/> </optional> + <optional> + <ref name="compatibility"/> + </optional> <empty/> </element> </define> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 53c3453..b7a5e26 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7671,6 +7671,9 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, } } + if (virDomainDriverCompatibilityParseXML(ctxt, &def->compatibility) < 0) + goto error; + /* Disk volume types will have authentication information handled in * virStorageTranslateDiskSourcePool */ @@ -19921,6 +19924,10 @@ virDomainDiskDefFormat(virBufferPtr buf, virBufferAsprintf(&driverBuf, " iothread='%u'", def->iothread); if (def->detect_zeroes) virBufferAsprintf(&driverBuf, " detect_zeroes='%s'", detect_zeroes); + if (def->compatibility) { + virBufferAsprintf(&driverBuf, " compatibility='%s'", + virDomainDriverCompatibilityTypeToString(def->compatibility)); + } if (virBufferUse(&driverBuf)) { virBufferAddLit(buf, "<driver"); virBufferAddBuffer(buf, &driverBuf); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index ac9f552..5ceb22a 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -621,6 +621,7 @@ struct _virDomainDiskDef { unsigned int iothread; /* unused = 0, > 0 specific thread # */ int detect_zeroes; /* enum virDomainDiskDetectZeroes */ char *domain_name; /* backend domain name */ + virDomainDriverCompatibility compatibility; }; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml b/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml index 2e71db1..c37a6c8 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml @@ -14,6 +14,18 @@ <on_crash>destroy</on_crash> <devices> <emulator>/usr/bin/qemu</emulator> + <disk type='file' device='disk'> + <driver name='qemu' type='raw' compatibility='legacy'/> + <source file='/var/lib/libvirt/images/img1'/> + <target dev='vda' bus='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x0a' function='0x0'/> + </disk> + <disk type='file' device='disk'> + <driver name='qemu' type='raw' compatibility='modern'/> + <source file='/var/lib/libvirt/images/img2'/> + <target dev='vdb' bus='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x0b' function='0x0'/> + </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 2e71db1..c37a6c8 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml @@ -14,6 +14,18 @@ <on_crash>destroy</on_crash> <devices> <emulator>/usr/bin/qemu</emulator> + <disk type='file' device='disk'> + <driver name='qemu' type='raw' compatibility='legacy'/> + <source file='/var/lib/libvirt/images/img1'/> + <target dev='vda' bus='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x0a' function='0x0'/> + </disk> + <disk type='file' device='disk'> + <driver name='qemu' type='raw' compatibility='modern'/> + <source file='/var/lib/libvirt/images/img2'/> + <target dev='vdb' bus='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x0b' function='0x0'/> + </disk> <controller type='usb' index='0'> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> </controller> -- 2.7.3
On Wed, Aug 24, 2016 at 00:20:48 +0200, Ján Tomko wrote:
A new attribute to alter the virtio revision: <disk> <driver compatibility='transitional'/> </disk>
https://bugzilla.redhat.com/show_bug.cgi?id=1227354 --- docs/formatdomain.html.in | 8 +++++++- docs/schemas/domaincommon.rng | 3 +++ src/conf/domain_conf.c | 7 +++++++ src/conf/domain_conf.h | 1 + tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml | 12 ++++++++++++ tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml | 12 ++++++++++++ 6 files changed, 42 insertions(+), 1 deletion(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 56dddbd..3f06613 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2084,7 +2084,7 @@ <target dev='vdc' bus='virtio'/> </disk> <disk type='file' device='disk'> - <driver name='qemu' type='qcow2'/> + <driver name='qemu' type='qcow2' compatibility='modern'/> <source file='/var/lib/libvirt/images/domain.qcow'/> <backingStore type='file'> <format type='qcow2'/> @@ -2667,6 +2667,12 @@ <code>bus</code> and "pci" or "ccw" <code>address</code> types. <span class='since'>Since 1.2.8 (QEMU 2.1)</span> </li> + <li> + The <code>compatibility</code> attribute can be used to specify + the compatibility of virtio devices. Allowed values are + <code>legacy</code>, <code>transitional</code> and <code>modern</code>. + <span class="since">Since 2.2.0</span>.
Same rant about the values. Perhaps we could create a link to this information so that it's not scattered across the docs html file.
+ </li> </ul> </dd> <dt><code>backenddomain</code></dt> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 8ed4b9d..2cf3c1b 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1641,6 +1641,9 @@ <optional> <ref name="detect_zeroes"/> </optional> + <optional> + <ref name="compatibility"/> + </optional>
You should be able to drop the <optional> section if you hide it into the name itself.
<empty/> </element> </define>
ACK with the docs upgraded.
<controller type='scsi' index='0' model='virtio-scsi'> <driver compatibility='modern'/> </controller> https://bugzilla.redhat.com/show_bug.cgi?id=1227354 --- docs/formatdomain.html.in | 9 ++++++++- docs/schemas/domaincommon.rng | 3 +++ src/conf/domain_conf.c | 7 +++++++ src/conf/domain_conf.h | 1 + tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml | 10 ++++++++++ tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml | 10 ++++++++++ 6 files changed, 39 insertions(+), 1 deletion(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 3f06613..e208767 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3126,7 +3126,7 @@ <address type='pci' domain='0x0000' bus='0x00' slot='0x0a' function='0x0'/> </controller> <controller type='scsi' index='0' model='virtio-scsi'> - <driver iothread='4'> + <driver iothread='4' compatibility='modern'> <address type='pci' domain='0x0000' bus='0x00' slot='0x0b' function='0x0'/> </controller> ... @@ -3238,6 +3238,13 @@ <code>iothread</code> value. The <code>iothread</code> value must be within the range 1 to the domain iothreads value. </dd> + <dt><code>compatibility</code></dt> + <dd> + The <code>compatibility</code> attribute can be used to specify the + compatibility of virtio devices. Allowed values are <code>legacy</code>, + <code>transitional</code> and <code>modern</code>. + <span class="since">Since 2.2.0</span>. + </dd> </dl> <p> USB companion controllers have an optional diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 2cf3c1b..9f912d2 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1944,6 +1944,9 @@ <optional> <ref name="driverIOThread"/> </optional> + <optional> + <ref name="compatibility"/> + </optional> </element> </optional> </interleave> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b7a5e26..1916bb8 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8283,6 +8283,9 @@ virDomainControllerDefParseXML(xmlNodePtr node, cur = cur->next; } + if (virDomainDriverCompatibilityParseXML(ctxt, &def->compatibility) < 0) + goto error; + /* node is parsed differently from target attributes because * someone thought it should be a subelement instead... */ @@ -20244,6 +20247,10 @@ virDomainControllerDefFormat(virBufferPtr buf, if (def->iothread) virBufferAsprintf(&driverBuf, " iothread='%u'", def->iothread); + if (def->compatibility) + virBufferAsprintf(&driverBuf, " compatibility='%s'", + virDomainDriverCompatibilityTypeToString(def->compatibility)); + if (virBufferUse(&driverBuf)) { virBufferAddLit(buf, "<driver"); virBufferAddBuffer(buf, &driverBuf); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 5ceb22a..1641ad9 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -764,6 +764,7 @@ struct _virDomainControllerDef { virDomainUSBControllerOpts usbopts; } opts; virDomainDeviceInfo info; + virDomainDriverCompatibility compatibility; }; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml b/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml index c37a6c8..176a270 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml @@ -26,12 +26,22 @@ <target dev='vdb' bus='virtio'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x0b' function='0x0'/> </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'> + <driver compatibility='transitional'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x08' function='0x0'/> + </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 c37a6c8..176a270 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml @@ -26,12 +26,22 @@ <target dev='vdb' bus='virtio'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x0b' function='0x0'/> </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'> + <driver compatibility='transitional'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x08' function='0x0'/> + </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 24, 2016 at 00:20:49 +0200, Ján Tomko wrote:
<controller type='scsi' index='0' model='virtio-scsi'> <driver compatibility='modern'/> </controller>
https://bugzilla.redhat.com/show_bug.cgi?id=1227354 --- docs/formatdomain.html.in | 9 ++++++++- docs/schemas/domaincommon.rng | 3 +++ src/conf/domain_conf.c | 7 +++++++ src/conf/domain_conf.h | 1 + tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml | 10 ++++++++++ tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml | 10 ++++++++++ 6 files changed, 39 insertions(+), 1 deletion(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 3f06613..e208767 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3126,7 +3126,7 @@ <address type='pci' domain='0x0000' bus='0x00' slot='0x0a' function='0x0'/> </controller> <controller type='scsi' index='0' model='virtio-scsi'> - <driver iothread='4'> + <driver iothread='4' compatibility='modern'>
Missing slash for non-pair tag.
<address type='pci' domain='0x0000' bus='0x00' slot='0x0b' function='0x0'/> </controller> ... @@ -3238,6 +3238,13 @@ <code>iothread</code> value. The <code>iothread</code> value must be within the range 1 to the domain iothreads value. </dd> + <dt><code>compatibility</code></dt> + <dd> + The <code>compatibility</code> attribute can be used to specify the + compatibility of virtio devices. Allowed values are <code>legacy</code>, + <code>transitional</code> and <code>modern</code>. + <span class="since">Since 2.2.0</span>. + </dd> </dl> <p> USB companion controllers have an optional
As in previous [...]
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml b/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml index c37a6c8..176a270 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml @@ -26,12 +26,22 @@ <target dev='vdb' bus='virtio'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x0b' function='0x0'/> </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>
Is the disk necessary?
<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'> + <driver compatibility='transitional'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x08' function='0x0'/> + </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'/>
ACK
<filesystem ...> <driver compatibility='modern'/> ... </filesystem> https://bugzilla.redhat.com/show_bug.cgi?id=1227354 --- docs/formatdomain.html.in | 6 ++++++ docs/schemas/domaincommon.rng | 3 +++ src/conf/domain_conf.c | 9 +++++++++ src/conf/domain_conf.h | 1 + tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml | 12 ++++++++++++ tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml | 12 ++++++++++++ 6 files changed, 43 insertions(+) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index e208767..81ca852 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2953,6 +2953,12 @@ or "handle", but no formats. Virtuozzo driver supports a type of "ploop" with a format of "ploop". </li> + <li> + The <code>compatibility</code> attribute can be used to specify + the compatibility of virtio devices. Allowed values are + <code>legacy</code>, <code>transitional</code> and <code>modern</code>. + <span class="since">Since 2.2.0</span>. + </li> </ul> </dd> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 9f912d2..baa9105 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2129,6 +2129,9 @@ <value>immediate</value> </attribute> </optional> + <optional> + <ref name='compatibility'/> + </optional> <empty/> </element> </define> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 1916bb8..2f3927a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8686,6 +8686,9 @@ virDomainFSDefParseXML(xmlNodePtr node, goto error; } + if (virDomainDriverCompatibilityParseXML(ctxt, &def->compatibility) < 0) + goto error; + def->src->path = source; source = NULL; def->dst = target; @@ -20333,6 +20336,11 @@ virDomainFSDefFormat(virBufferPtr buf, } + if (def->compatibility) { + virBufferAsprintf(&driverBuf, " compatibility='%s'", + virDomainDriverCompatibilityTypeToString(def->compatibility)); + } + if (virBufferUse(&driverBuf)) { virBufferAddLit(buf, "<driver"); virBufferAddBuffer(buf, &driverBuf); @@ -20391,6 +20399,7 @@ virDomainFSDefFormat(virBufferPtr buf, virBufferAsprintf(buf, "<space_soft_limit unit='bytes'>" "%llu</space_soft_limit>\n", def->space_soft_limit); } + virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</filesystem>\n"); return 0; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 1641ad9..0a4017f 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -827,6 +827,7 @@ struct _virDomainFSDef { unsigned long long space_hard_limit; /* in bytes */ unsigned long long space_soft_limit; /* in bytes */ bool symlinksResolved; + virDomainDriverCompatibility compatibility; }; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml b/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml index 176a270..98697ef 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml @@ -46,6 +46,18 @@ <controller type='virtio-serial' index='0'> <address type='pci' domain='0x0000' bus='0x00' slot='0x09' function='0x0'/> </controller> + <filesystem type='mount' accessmode='passthrough'> + <driver compatibility='legacy'/> + <source dir='/export/fs1'/> + <target dir='fs1'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </filesystem> + <filesystem type='mount' accessmode='mapped'> + <driver type='path' wrpolicy='immediate' compatibility='modern'/> + <source dir='/export/fs2'/> + <target dir='fs2'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> + </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 176a270..98697ef 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml @@ -46,6 +46,18 @@ <controller type='virtio-serial' index='0'> <address type='pci' domain='0x0000' bus='0x00' slot='0x09' function='0x0'/> </controller> + <filesystem type='mount' accessmode='passthrough'> + <driver compatibility='legacy'/> + <source dir='/export/fs1'/> + <target dir='fs1'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </filesystem> + <filesystem type='mount' accessmode='mapped'> + <driver type='path' wrpolicy='immediate' compatibility='modern'/> + <source dir='/export/fs2'/> + <target dir='fs2'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> + </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'/> <driver compatibility='modern'/> </interface> https://bugzilla.redhat.com/show_bug.cgi?id=1227354 --- docs/formatdomain.html.in | 7 +++++++ docs/schemas/domaincommon.rng | 3 +++ src/conf/domain_conf.c | 8 +++++++- src/conf/domain_conf.h | 1 + tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml | 12 ++++++++++++ tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml | 12 ++++++++++++ 6 files changed, 42 insertions(+), 1 deletion(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 81ca852..fa7e2bb 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -4739,6 +4739,13 @@ qemu-kvm -net nic,model=? /dev/null <span class="since">virtio-net since 1.0.6 (QEMU and KVM only)</span> <span class="since">vhost-user since 1.2.17 (QEMU and KVM only)</span> </dd> + <dt><code>compatibility</code></dt> + <dd> + The <code>compatibility</code> attribute can be used to specify the + compatibility of virtio devices. Allowed values are <code>legacy</code>, + <code>transitional</code> and <code>modern</code>. + <span class="since">Since 2.2.0</span>. + </dd> </dl> <p> Offloading options for the host and guest can be configured using diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index baa9105..1b305a2 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2553,6 +2553,9 @@ </optional> </group> </choice> + <optional> + <ref name="compatibility"/> + </optional> <interleave> <optional> <element name='host'> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 2f3927a..885430d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9664,6 +9664,9 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, goto error; } + if (virDomainDriverCompatibilityParseXML(ctxt, &def->compatibility) < 0) + goto error; + cleanup: ctxt->node = oldnode; VIR_FREE(macaddr); @@ -20832,7 +20835,10 @@ virDomainVirtioNetDriverFormat(char **outstr, } if (def->driver.virtio.queues) virBufferAsprintf(&buf, "queues='%u' ", def->driver.virtio.queues); - + if (def->compatibility) { + virBufferAsprintf(&buf, "compatibility='%s'", + virDomainDriverCompatibilityTypeToString(def->compatibility)); + } virBufferTrim(&buf, " ", -1); if (virBufferCheckError(&buf) < 0) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 0a4017f..e7c0ba2 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -994,6 +994,7 @@ struct _virDomainNetDef { virNetDevVlan vlan; int trustGuestRxFilters; /* enum virTristateBool */ int linkstate; + virDomainDriverCompatibility compatibility; }; /* 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 98697ef..02b5598 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml @@ -58,6 +58,18 @@ <target dir='fs2'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> </filesystem> + <interface type='user'> + <mac address='52:54:56:58:5a:5c'/> + <model type='virtio'/> + <driver compatibility='legacy'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0'/> + </interface> + <interface type='user'> + <mac address='52:54:56:5a:5c:5e'/> + <model type='virtio'/> + <driver compatibility='modern'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0'/> + </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 98697ef..02b5598 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml @@ -58,6 +58,18 @@ <target dir='fs2'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> </filesystem> + <interface type='user'> + <mac address='52:54:56:58:5a:5c'/> + <model type='virtio'/> + <driver compatibility='legacy'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0'/> + </interface> + <interface type='user'> + <mac address='52:54:56:5a:5c:5e'/> + <model type='virtio'/> + <driver compatibility='modern'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0'/> + </interface> <input type='mouse' bus='virtio'> <address type='pci' domain='0x0000' bus='0x00' slot='0x0e' function='0x0'/> </input> -- 2.7.3
<rng model='virtio'> <driver compatibility='modern'/> <backend model='random'>/dev/random</backend> </rng> https://bugzilla.redhat.com/show_bug.cgi?id=1227354 --- docs/formatdomain.html.in | 7 +++++++ docs/schemas/domaincommon.rng | 5 +++++ src/conf/domain_conf.c | 8 ++++++++ src/conf/domain_conf.h | 1 + tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml | 5 +++++ tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml | 4 ++++ 6 files changed, 30 insertions(+) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index fa7e2bb..41a9325 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -6532,6 +6532,13 @@ qemu-kvm -net nic,model=? /dev/null </dd> </dl> </dd> + <dt><code>driver</code></dt> + <dd> + The <code>compatibility</code> attribute of the <code>driver</code> + element can be used to specify the compatibility of virtio devices. + Allowed values are <code>legacy</code>, <code>transitional</code> + and <code>modern</code>. <span class="since">Since 2.2.0</span>. + </dd> </dl> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 1b305a2..ed00ace 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -4798,6 +4798,11 @@ <interleave> <ref name="rng-backend"/> <optional> + <element name="driver"> + <ref name="compatibility"/> + </element> + </optional> + <optional> <ref name="rng-rate"/> </optional> <optional> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 885430d..166f5fb 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -12044,6 +12044,9 @@ virDomainRNGDefParseXML(xmlNodePtr node, if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0) goto error; + if (virDomainDriverCompatibilityParseXML(ctxt, &def->compatibility) < 0) + goto error; + cleanup: VIR_FREE(model); VIR_FREE(backend); @@ -21749,6 +21752,11 @@ virDomainRNGDefFormat(virBufferPtr buf, break; } + if (def->compatibility) { + virBufferAsprintf(buf, "<driver compatibility='%s'/>", + virDomainDriverCompatibilityTypeToString(def->compatibility)); + } + if (virDomainDeviceInfoNeedsFormat(&def->info, flags)) { if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) return -1; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index e7c0ba2..342218e 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1947,6 +1947,7 @@ struct _virDomainRNGDef { } source; virDomainDeviceInfo info; + virDomainDriverCompatibility compatibility; }; typedef enum { diff --git a/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml b/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml index 02b5598..5b9726a 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml @@ -95,5 +95,10 @@ <address type='pci' domain='0x0000' bus='0x00' slot='0x0c' function='0x0'/> <driver compatibility='transitional'/> </memballoon> + <rng model='virtio'> + <backend model='random'>/dev/random</backend> + <driver compatibility='modern'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x0d' function='0x0'/> + </rng> </devices> </domain> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml index 02b5598..ab69bb7 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml @@ -95,5 +95,9 @@ <address type='pci' domain='0x0000' bus='0x00' slot='0x0c' function='0x0'/> <driver compatibility='transitional'/> </memballoon> + <rng model='virtio'> + <backend model='random'>/dev/random</backend> + <driver compatibility='modern'/><address type='pci' domain='0x0000' bus='0x00' slot='0x0d' function='0x0'/> + </rng> </devices> </domain> -- 2.7.3
On Wed, Aug 24, 2016 at 00:20:52 +0200, Ján Tomko wrote:
<rng model='virtio'> <driver compatibility='modern'/> <backend model='random'>/dev/random</backend> </rng>
https://bugzilla.redhat.com/show_bug.cgi?id=1227354 --- docs/formatdomain.html.in | 7 +++++++ docs/schemas/domaincommon.rng | 5 +++++ src/conf/domain_conf.c | 8 ++++++++ src/conf/domain_conf.h | 1 + tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml | 5 +++++ tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml | 4 ++++ 6 files changed, 30 insertions(+)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index fa7e2bb..41a9325 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -6532,6 +6532,13 @@ qemu-kvm -net nic,model=? /dev/null </dd> </dl> </dd> + <dt><code>driver</code></dt> + <dd> + The <code>compatibility</code> attribute of the <code>driver</code> + element can be used to specify the compatibility of virtio devices. + Allowed values are <code>legacy</code>, <code>transitional</code> + and <code>modern</code>. <span class="since">Since 2.2.0</span>. + </dd>
</dl>
Same as above.
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 885430d..166f5fb 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -12044,6 +12044,9 @@ virDomainRNGDefParseXML(xmlNodePtr node, if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0) goto error;
+ if (virDomainDriverCompatibilityParseXML(ctxt, &def->compatibility) < 0) + goto error; + cleanup: VIR_FREE(model); VIR_FREE(backend); @@ -21749,6 +21752,11 @@ virDomainRNGDefFormat(virBufferPtr buf, break; }
+ if (def->compatibility) { + virBufferAsprintf(buf, "<driver compatibility='%s'/>",
Missing newline, see test output ...
+ virDomainDriverCompatibilityTypeToString(def->compatibility)); + } + if (virDomainDeviceInfoNeedsFormat(&def->info, flags)) { if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) return -1; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index e7c0ba2..342218e 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1947,6 +1947,7 @@ struct _virDomainRNGDef { } source;
virDomainDeviceInfo info; + virDomainDriverCompatibility compatibility; };
typedef enum {
[...]
diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml index 02b5598..ab69bb7 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml @@ -95,5 +95,9 @@ <address type='pci' domain='0x0000' bus='0x00' slot='0x0c' function='0x0'/> <driver compatibility='transitional'/> </memballoon> + <rng model='virtio'> + <backend model='random'>/dev/random</backend> + <driver compatibility='modern'/><address type='pci' domain='0x0000' bus='0x00' slot='0x0d' function='0x0'/>
^^ VIR_TEST_REGENERATE_OUTPUT is not your friend sometimes ;) (been there, done that)
+ </rng> </devices> </domain>
ACK .. similar to previous cases
<video> <driver compatibility='modern'/> <model type='virtio' heads='1' primary='yes'/> </video> https://bugzilla.redhat.com/show_bug.cgi?id=1227354 --- docs/formatdomain.html.in | 8 ++++++++ docs/schemas/domaincommon.rng | 5 +++++ src/conf/domain_conf.c | 17 +++++++++++++++-- src/conf/domain_conf.h | 1 + tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml | 1 + .../qemuxml2xmlout-virtio-revision.xml | 1 + 6 files changed, 31 insertions(+), 2 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 41a9325..d640e8d 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -5641,6 +5641,14 @@ 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>driver</code></dt> + <dd> + The <code>compatibility</code> attribute can be used to specify the + compatibility of virtio devices. Allowed values are <code>legacy</code>, + <code>transitional</code> and <code>modern</code>. + <span class="since">Since 2.2.0</span>. + </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 ed00ace..f250cbc 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3080,6 +3080,11 @@ <define name="video"> <element name="video"> <optional> + <element name="driver"> + <ref name="compatibility"/> + </element> + </optional> + <optional> <element name="model"> <choice> <attribute name="type"> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 166f5fb..03c856c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -12605,11 +12605,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; @@ -12618,6 +12620,8 @@ virDomainVideoDefParseXML(xmlNodePtr node, char *vgamem = NULL; char *primary = NULL; + ctxt->node = node; + if (VIR_ALLOC(def) < 0) return NULL; @@ -12719,7 +12723,12 @@ virDomainVideoDefParseXML(xmlNodePtr node, if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0) goto error; + if (virDomainDriverCompatibilityParseXML(ctxt, &def->compatibility) < 0) + goto error; + cleanup: + ctxt->node = saved; + VIR_FREE(type); VIR_FREE(ram); VIR_FREE(vram); @@ -13414,7 +13423,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: @@ -17084,7 +17093,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) { @@ -21896,6 +21905,10 @@ virDomainVideoDefFormat(virBufferPtr buf, virBufferAddLit(buf, "<video>\n"); virBufferAdjustIndent(buf, 2); + if (def->compatibility) { + virBufferAsprintf(buf, "<driver compatibility='%s'/>\n", + virDomainDriverCompatibilityTypeToString(def->compatibility)); + } virBufferAsprintf(buf, "<model type='%s'", model); if (def->ram) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 342218e..ecccc0a 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1322,6 +1322,7 @@ struct _virDomainVideoDef { bool primary; virDomainVideoAccelDefPtr accel; virDomainDeviceInfo info; + virDomainDriverCompatibility compatibility; }; /* graphics console modes */ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml b/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml index 5b9726a..fecf9e5 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml @@ -86,6 +86,7 @@ <input type='mouse' bus='ps2'/> <input type='keyboard' bus='ps2'/> <video> + <driver compatibility='modern'/> <model type='virtio' heads='1' primary='yes'> <acceleration accel3d='yes'/> </model> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml index ab69bb7..f1c728e 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml @@ -86,6 +86,7 @@ <input type='mouse' bus='ps2'/> <input type='keyboard' bus='ps2'/> <video> + <driver compatibility='modern'/> <model type='virtio' heads='1' primary='yes'> <acceleration accel3d='yes'/> </model> -- 2.7.3
Hi Ján, I apply your patches of virtio compatibility attribute. And I find virtio video and input devices using virtio modern when set compatibility as legacy. The xml: <video> <driver compatibility='legacy'/> <model type='virtio' heads='1' primary='yes'/> <alias name='video0'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> </video> <input type='mouse' bus='virtio'> <driver compatibility='legacy'/> <alias name='input1'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x09' function='0x0'/> </input> <input type='keyboard' bus='virtio'> <driver compatibility='legacy'/> <alias name='input2'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x0a' function='0x0'/> </input> Check virtio version in guest: # for i in `find /sys/devices -name 'virtio[0-9]*'`;do
echo -e $(lspci -s $(echo $i|egrep -o '[0-9a-fA-F]{4}:[0-9a-fA-F]{2}:[01][0-9a-fA-F]\.[0-7]'))'\t'$(cat $i/features|cut -c 33) done
00:02.0 VGA compatible controller: Red Hat, Inc Virtio GPU (rev 01) 1 00:03.0 Ethernet controller: Red Hat, Inc Virtio network device 1 00:05.0 Communication controller: Red Hat, Inc Virtio console 1 00:07.0 SCSI storage controller: Red Hat, Inc Virtio block device 1 00:08.0 Unclassified device [00ff]: Red Hat, Inc Virtio memory balloon 1 00:09.0 Mouse controller: Red Hat, Inc Virtio input (rev 01) 1 00:0a.0 Keyboard controller: Red Hat, Inc Virtio input (rev 01) 1 For the issue I got Gerd's reply on virt-devel: <snippet>
Right now the default is "disable-legacy=off,disable-modern=on" (with the exception of virtio-input and virtio-gpu, those two are newer than virtio 1.0 and thus don't support legacy virtio). </snippet>
So, maybe we should make compatibility='legacy' invalid in virtio video and input devices in case of confusion. ----- Original Message ----- From: "Ján Tomko" <jtomko@redhat.com> To: libvir-list@redhat.com Sent: Tuesday, August 23, 2016 6:20:53 PM Subject: [libvirt] [PATCHv5 11/13] Add compatibility attribute to video <video> <driver compatibility='modern'/> <model type='virtio' heads='1' primary='yes'/> </video> https://bugzilla.redhat.com/show_bug.cgi?id=1227354 --- docs/formatdomain.html.in | 8 ++++++++ docs/schemas/domaincommon.rng | 5 +++++ src/conf/domain_conf.c | 17 +++++++++++++++-- src/conf/domain_conf.h | 1 + tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml | 1 + .../qemuxml2xmlout-virtio-revision.xml | 1 + 6 files changed, 31 insertions(+), 2 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 41a9325..d640e8d 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -5641,6 +5641,14 @@ 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>driver</code></dt> + <dd> + The <code>compatibility</code> attribute can be used to specify the + compatibility of virtio devices. Allowed values are <code>legacy</code>, + <code>transitional</code> and <code>modern</code>. + <span class="since">Since 2.2.0</span>. + </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 ed00ace..f250cbc 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3080,6 +3080,11 @@ <define name="video"> <element name="video"> <optional> + <element name="driver"> + <ref name="compatibility"/> + </element> + </optional> + <optional> <element name="model"> <choice> <attribute name="type"> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 166f5fb..03c856c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -12605,11 +12605,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; @@ -12618,6 +12620,8 @@ virDomainVideoDefParseXML(xmlNodePtr node, char *vgamem = NULL; char *primary = NULL; + ctxt->node = node; + if (VIR_ALLOC(def) < 0) return NULL; @@ -12719,7 +12723,12 @@ virDomainVideoDefParseXML(xmlNodePtr node, if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0) goto error; + if (virDomainDriverCompatibilityParseXML(ctxt, &def->compatibility) < 0) + goto error; + cleanup: + ctxt->node = saved; + VIR_FREE(type); VIR_FREE(ram); VIR_FREE(vram); @@ -13414,7 +13423,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: @@ -17084,7 +17093,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) { @@ -21896,6 +21905,10 @@ virDomainVideoDefFormat(virBufferPtr buf, virBufferAddLit(buf, "<video>\n"); virBufferAdjustIndent(buf, 2); + if (def->compatibility) { + virBufferAsprintf(buf, "<driver compatibility='%s'/>\n", + virDomainDriverCompatibilityTypeToString(def->compatibility)); + } virBufferAsprintf(buf, "<model type='%s'", model); if (def->ram) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 342218e..ecccc0a 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1322,6 +1322,7 @@ struct _virDomainVideoDef { bool primary; virDomainVideoAccelDefPtr accel; virDomainDeviceInfo info; + virDomainDriverCompatibility compatibility; }; /* graphics console modes */ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml b/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml index 5b9726a..fecf9e5 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml @@ -86,6 +86,7 @@ <input type='mouse' bus='ps2'/> <input type='keyboard' bus='ps2'/> <video> + <driver compatibility='modern'/> <model type='virtio' heads='1' primary='yes'> <acceleration accel3d='yes'/> </model> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml index ab69bb7..f1c728e 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml @@ -86,6 +86,7 @@ <input type='mouse' bus='ps2'/> <input type='keyboard' bus='ps2'/> <video> + <driver compatibility='modern'/> <model type='virtio' heads='1' primary='yes'> <acceleration accel3d='yes'/> </model> -- 2.7.3 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
<input type='mouse' bus='virtio'> <driver compatibility='modern'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x0e' function='0x0'/> </input> <input type='keyboard' bus='virtio'> <driver compatibility='modern'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x10' function='0x0'/> </input> <input type='tablet' bus='virtio'> <driver compatibility='modern'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x11' function='0x0'/> </input> <input type='passthrough' bus='virtio'> <driver compatibility='modern'/> <source evdev='/dev/input/event1234'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x12' function='0x0'/> </input> https://bugzilla.redhat.com/show_bug.cgi?id=1227354 --- docs/formatdomain.html.in | 7 +++++++ docs/schemas/domaincommon.rng | 5 +++++ src/conf/domain_conf.c | 7 +++++++ src/conf/domain_conf.h | 1 + tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml | 4 ++++ tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml | 4 ++++ 6 files changed, 28 insertions(+) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index d640e8d..8913e91 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -5230,6 +5230,13 @@ qemu-kvm -net nic,model=? /dev/null event device passed through to guests. (KVM only) </p> + <p> + The <code>compatibility</code> attribute of the <code>driver</code> element + can be used to specify the compatibility of virtio devices. Allowed values + are <code>legacy</code>, <code>transitional</code> and <code>modern</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 f250cbc..b69c3ce 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3792,6 +3792,11 @@ <define name="input"> <element name="input"> + <optional> + <element name="driver"> + <ref name="compatibility"/> + </element> + </optional> <choice> <group> <attribute name="type"> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 03c856c..ad349fb 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10730,6 +10730,9 @@ virDomainInputDefParseXML(const virDomainDef *dom, goto error; } + if (virDomainDriverCompatibilityParseXML(ctxt, &def->compatibility) < 0) + goto error; + cleanup: VIR_FREE(evdev); VIR_FREE(type); @@ -21972,6 +21975,10 @@ virDomainInputDefFormat(virBufferPtr buf, type, bus); virBufferAdjustIndent(&childbuf, virBufferGetIndent(buf, false) + 2); + if (def->compatibility) { + virBufferAsprintf(&childbuf, "<driver compatibility='%s'/>\n", + virDomainDriverCompatibilityTypeToString(def->compatibility)); + } virBufferEscapeString(&childbuf, "<source evdev='%s'/>\n", def->source.evdev); if (virDomainDeviceInfoFormat(&childbuf, &def->info, flags) < 0) return -1; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index ecccc0a..0f5fc0d 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1229,6 +1229,7 @@ struct _virDomainInputDef { char *evdev; } source; virDomainDeviceInfo info; + virDomainDriverCompatibility compatibility; }; typedef enum { diff --git a/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml b/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml index fecf9e5..1414a0d 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml @@ -71,15 +71,19 @@ <address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0'/> </interface> <input type='mouse' bus='virtio'> + <driver compatibility='modern'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x0e' function='0x0'/> </input> <input type='keyboard' bus='virtio'> + <driver compatibility='modern'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x10' function='0x0'/> </input> <input type='tablet' bus='virtio'> + <driver compatibility='modern'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x11' function='0x0'/> </input> <input type='passthrough' bus='virtio'> + <driver compatibility='modern'/> <source evdev='/dev/input/event1234'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x12' function='0x0'/> </input> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml index f1c728e..d9dc27c 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml @@ -71,15 +71,19 @@ <address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0'/> </interface> <input type='mouse' bus='virtio'> + <driver compatibility='modern'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x0e' function='0x0'/> </input> <input type='keyboard' bus='virtio'> + <driver compatibility='modern'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x10' function='0x0'/> </input> <input type='tablet' bus='virtio'> + <driver compatibility='modern'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x11' function='0x0'/> </input> <input type='passthrough' bus='virtio'> + <driver compatibility='modern'/> <source evdev='/dev/input/event1234'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x12' function='0x0'/> </input> -- 2.7.3
https://bugzilla.redhat.com/show_bug.cgi?id=1227354 Translate the optional <driver compatibility> 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 | 70 ++++++++++++++++++++++ .../qemuxml2argv-virtio-revision.args | 62 +++++++++++++++++++ tests/qemuxml2argvtest.c | 11 ++++ 3 files changed, 143 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 a6dea6a..470d84c 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -378,6 +378,46 @@ qemuBuildDeviceAddressStr(virBufferPtr buf, } static int +qemuBuildVirtioCompatibilityStr(virBufferPtr buf, + virDomainDriverCompatibility compat, + virDomainDeviceInfoPtr info, + virQEMUCapsPtr qemuCaps) +{ + if (!compat) + 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 (!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; + } + + switch (compat) { + case VIR_DOMAIN_DRIVER_COMPATIBILITY_LEGACY: + virBufferAddLit(buf, ",disable-legacy=off,disable-modern=on"); + break; + case VIR_DOMAIN_DRIVER_COMPATIBILITY_TRANSITIONAL: + virBufferAddLit(buf, ",disable-legacy=off,disable-modern=off"); + break; + case VIR_DOMAIN_DRIVER_COMPATIBILITY_MODERN: + virBufferAddLit(buf, ",disable-legacy=on,disable-modern=off"); + break; + case VIR_DOMAIN_DRIVER_COMPATIBILITY_DEFAULT: + case VIR_DOMAIN_DRIVER_COMPATIBILITY_LAST: + break; + } + return 0; +} + +static int qemuBuildRomStr(virBufferPtr buf, virDomainDeviceInfoPtr info) { @@ -1993,6 +2033,11 @@ qemuBuildDriveDevStr(const virDomainDef *def, (disk->device == VIR_DOMAIN_DISK_DEVICE_LUN) ? "on" : "off"); } + + if (qemuBuildVirtioCompatibilityStr(&opt, disk->compatibility, &disk->info, + qemuCaps) < 0) + goto error; + if (qemuBuildDeviceAddressStr(&opt, def, &disk->info, qemuCaps) < 0) goto error; break; @@ -2314,6 +2359,8 @@ qemuBuildFSDevStr(const virDomainDef *def, QEMU_FSDEV_HOST_PREFIX, fs->info.alias); virBufferAsprintf(&opt, ",mount_tag=%s", fs->dst); + qemuBuildVirtioCompatibilityStr(&opt, fs->compatibility, &fs->info, qemuCaps); + if (qemuBuildDeviceAddressStr(&opt, def, &fs->info, qemuCaps) < 0) goto error; @@ -2569,6 +2616,9 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, def->iothread); } } + if (qemuBuildVirtioCompatibilityStr(&buf, def->compatibility, &def->info, + qemuCaps) < 0) + goto error; break; case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC: virBufferAddLit(&buf, "lsi"); @@ -2614,6 +2664,9 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, virBufferAsprintf(&buf, ",vectors=%d", def->opts.vioserial.vectors); } + if (qemuBuildVirtioCompatibilityStr(&buf, def->compatibility, &def->info, + qemuCaps) < 0) + goto error; break; case VIR_DOMAIN_CONTROLLER_TYPE_CCID: @@ -3560,12 +3613,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 && + qemuBuildVirtioCompatibilityStr(&buf, net->compatibility, &net->info, qemuCaps) < 0) + goto error; if (virBufferCheckError(&buf) < 0) goto error; @@ -3830,6 +3887,10 @@ qemuBuildMemballoonCommandLine(virCommandPtr cmd, virTristateSwitchTypeToString(def->memballoon->autodeflate)); } + if (qemuBuildVirtioCompatibilityStr(&buf, def->memballoon->compatibility, + &def->memballoon->info, qemuCaps) < 0) + goto error; + virCommandAddArg(cmd, "-device"); virCommandAddArgBuffer(cmd, &buf); return 0; @@ -3960,6 +4021,9 @@ qemuBuildVirtioInputDevStr(const virDomainDef *def, if (qemuBuildDeviceAddressStr(&buf, def, &dev->info, qemuCaps) < 0) goto error; + if (qemuBuildVirtioCompatibilityStr(&buf, dev->compatibility, &dev->info, qemuCaps) < 0) + goto error; + if (virBufferCheckError(&buf) < 0) goto error; @@ -4323,6 +4387,9 @@ qemuBuildDeviceVideoStr(const virDomainDef *def, if (qemuBuildDeviceAddressStr(&buf, def, &video->info, qemuCaps) < 0) goto error; + if (qemuBuildVirtioCompatibilityStr(&buf, video->compatibility, &video->info, qemuCaps) < 0) + goto error; + if (virBufferCheckError(&buf) < 0) goto error; @@ -5569,6 +5636,9 @@ qemuBuildRNGDevStr(const virDomainDef *def, virBufferAddLit(&buf, ",period=1000"); } + if (qemuBuildVirtioCompatibilityStr(&buf, dev->compatibility, &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..4ef7663 --- /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=off,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 e8b8cb4..8a29427 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -2106,6 +2106,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 Wed, Aug 24, 2016 at 00:20:42 +0200, Ján Tomko wrote:
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
v4: * only probe the capability for PCI devices
v5: * instead of a separate element, use one attribute under the driver element
So as usual the qemu documentation for this is rather sparse, but from skimming through the qemu commits and the bugzilla I've got the following feeling: I'm basically wondering whether it's worth to expose this to be controlled manually or we can have good enough chance to try to controll it according to other settings and then infer the required mode. (basically what Daniel said in the comment in bugzilla). The "--disable-modern" flag is basically considered just as a safeguard if qemu would be buggy, but I'm not really a fan of such workaround. Since the "modern" approach is supposed to be compatible I don't quite see a big need to use it. The "--disable-legacy" part is more important feature, as it allows to assign more devices on a PCIe bus (as the IO region is not necessary) but the legacy mode is required for legacy guests and/or if booting from such device is necessary (?). This design basically requires the users to do the correct decision, which was also the reason why I asked for more docs. In this current version I'm also missing any form of safeguards that an invalid configuration won't be accepted. Currently we'd allow to specify it for a emulated device as well as virio. Obviously making the user responsible for correct setting is much easier. Sigh. It'd be great if somebody else could also state their opinion on this. I can't say that I'm a big fan of this approach but it looks like as if it would be the only sensible one. Peter
On Wed, Sep 07, 2016 at 05:10:01PM +0200, Peter Krempa wrote:
On Wed, Aug 24, 2016 at 00:20:42 +0200, Ján Tomko wrote:
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
v4: * only probe the capability for PCI devices
v5: * instead of a separate element, use one attribute under the driver element
So as usual the qemu documentation for this is rather sparse, but from skimming through the qemu commits and the bugzilla I've got the following feeling:
I'm basically wondering whether it's worth to expose this to be controlled manually or we can have good enough chance to try to controll it according to other settings and then infer the required mode. (basically what Daniel said in the comment in bugzilla).
The "--disable-modern" flag is basically considered just as a safeguard if qemu would be buggy, but I'm not really a fan of such workaround. Since the "modern" approach is supposed to be compatible I don't quite see a big need to use it.
The "--disable-legacy" part is more important feature, as it allows to assign more devices on a PCIe bus (as the IO region is not necessary) but the legacy mode is required for legacy guests and/or if booting from such device is necessary (?).
This design basically requires the users to do the correct decision, which was also the reason why I asked for more docs.
In this current version I'm also missing any form of safeguards that an invalid configuration won't be accepted. Currently we'd allow to specify it for a emulated device as well as virio.
Obviously making the user responsible for correct setting is much easier.
Sigh. It'd be great if somebody else could also state their opinion on this. I can't say that I'm a big fan of this approach but it looks like as if it would be the only sensible one.
I tend to agree. I'd be incredibly happy if we didn't add any of this to the XML and would simply "do the best thing" automatically. I'm particularly not a fan of adding something "just in case there is a bug" 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 Wed, Sep 07, 2016 at 16:13:57 +0100, Daniel Berrange wrote:
On Wed, Sep 07, 2016 at 05:10:01PM +0200, Peter Krempa wrote:
On Wed, Aug 24, 2016 at 00:20:42 +0200, Ján Tomko wrote:
[...]
Obviously making the user responsible for correct setting is much easier.
Sigh. It'd be great if somebody else could also state their opinion on this. I can't say that I'm a big fan of this approach but it looks like as if it would be the only sensible one.
I tend to agree. I'd be incredibly happy if we didn't add any of this to the XML and would simply "do the best thing" automatically.
I'm particularly not a fan of adding something "just in case there is a bug"
Yep, that's why I don't see a reason for using --disable-modern in libvirt. It does not have any value besides working around "possible bugs". On the other hand, I'm kind of worried that with "--disable-legacy" we are entering the domain of policy decisions as it's uncertain to me whether: * Enabling it would break guests with very old drivers. * Guest firmware actually supports the modern mode (both ovmf and bios) * other possible caveats that I can't think of right now In such case we'd need to expose a knob to set it, but I'd prefer dropping the possibility to use legacy mode only since it's of no value. Peter
On Wed, Sep 07, 2016 at 05:52:02PM +0200, Peter Krempa wrote:
On Wed, Sep 07, 2016 at 16:13:57 +0100, Daniel Berrange wrote:
On Wed, Sep 07, 2016 at 05:10:01PM +0200, Peter Krempa wrote:
On Wed, Aug 24, 2016 at 00:20:42 +0200, Ján Tomko wrote:
[...]
Obviously making the user responsible for correct setting is much easier.
Sigh. It'd be great if somebody else could also state their opinion on this. I can't say that I'm a big fan of this approach but it looks like as if it would be the only sensible one.
I tend to agree. I'd be incredibly happy if we didn't add any of this to the XML and would simply "do the best thing" automatically.
I'm particularly not a fan of adding something "just in case there is a bug"
Yep, that's why I don't see a reason for using --disable-modern in libvirt. It does not have any value besides working around "possible bugs".
On the other hand, I'm kind of worried that with "--disable-legacy" we are entering the domain of policy decisions as it's uncertain to me whether: * Enabling it would break guests with very old drivers. * Guest firmware actually supports the modern mode (both ovmf and bios) * other possible caveats that I can't think of right now
In such case we'd need to expose a knob to set it, but I'd prefer dropping the possibility to use legacy mode only since it's of no value.
We could take a completely different approach to this based on the model. Effectively virtio-1.0 with disable-legacy is a completely separate hardware from the original virtio-0.9, with or without disable-modern. You even get a completely different PCI ID with disable-legacy set. So we could in fact represent this in the XML as a different model called "virtio1.0", and remap that to virtio+disable-legacy in the QEMU argv. This is in fact what we've done with naming in libosinfo already, due to the fact it has different PCI IDs and so is logically a different device. IOW, <sound model="virtio"/> == QEMU virtio <sound model="virtio1.0"/> == QEMU virtio + disable-legacy and explicitly don't provide any disable-modern flag mapping, since there is no compelling use case for that. 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 :|
Dear Daniel, "Daniel P. Berrange" <berrange@redhat.com> writes: [...]
<sound model="virtio"/> == QEMU virtio <sound model="virtio1.0"/> == QEMU virtio + disable-legacy
What would this do for devices using the virtio-ccw transport? Sascha -- Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg https://se-silbe.de/ USt-IdNr. DE281696641
On 09/07/2016 02:35 PM, Sascha Silbe wrote:
Dear Daniel,
"Daniel P. Berrange" <berrange@redhat.com> writes:
[...]
<sound model="virtio"/> == QEMU virtio <sound model="virtio1.0"/> == QEMU virtio + disable-legacy
What would this do for devices using the virtio-ccw transport?
From libvirt's point of view, the option "disable-legacy=on" would be added to the device's commandline argument. For the effect, you would need to ask a qemu virtio person, or even better - a qemu s390 person who knows something about about virtio. I Cc'ed Michael (the former) and Cornelia Huck (the latter, according to this patch I found: https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg01024.html )
Dear Laine, Laine Stump <laine@laine.org> writes:
On 09/07/2016 02:35 PM, Sascha Silbe wrote:
"Daniel P. Berrange" <berrange@redhat.com> writes: [...]
<sound model="virtio"/> == QEMU virtio <sound model="virtio1.0"/> == QEMU virtio + disable-legacy
What would this do for devices using the virtio-ccw transport?
From libvirt's point of view, the option "disable-legacy=on" would be added to the device's commandline argument.
Which would break s390x guests. virtio-ccw doesn't have any concept of "legacy" or "modern" devices (that's purely a virtio-pci concept), so virtio-*-ccw devices don't recognise that switch: silbe@oc4731375738:~$ ~/build/qemu-devel/x86_64-softmmu/qemu-system-x86_64 -device virtio-blk,help 2>&1 |grep legacy virtio-blk-pci.disable-legacy=OnOffAuto (on/off/auto) silbe@oc4731375738:~$ ~/build/qemu-devel/s390x-softmmu/qemu-system-s390x -device virtio-blk,help 2>&1 |grep legacy That nicely illustrates the issue I have with a) mixing virtio-pci legacy/modern into the model name and b) conflating it with virtio 0.9/1.0 (or transitional/non-transitional for that matter). FWIW, the thing closest to virtio-pci legacy/modern is virtio-ccw max_revision. But I doubt there's any reason to set this beyond debugging and testing.
For the effect, you would need to ask a qemu virtio person, or even better - a qemu s390 person who knows something about about virtio. I Cc'ed Michael (the former) and Cornelia Huck (the latter, according to this patch I found: https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg01024.html )
Thanks, but I'm working on qemu for s390x myself. :) Sascha -- Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg https://se-silbe.de/ USt-IdNr. DE281696641
On 09/07/2016 03:38 PM, Sascha Silbe wrote:
Dear Laine,
Laine Stump <laine@laine.org> writes:
On 09/07/2016 02:35 PM, Sascha Silbe wrote:
"Daniel P. Berrange" <berrange@redhat.com> writes: [...]
<sound model="virtio"/> == QEMU virtio <sound model="virtio1.0"/> == QEMU virtio + disable-legacy
What would this do for devices using the virtio-ccw transport? From libvirt's point of view, the option "disable-legacy=on" would be added to the device's commandline argument. Which would break s390x guests. virtio-ccw doesn't have any concept of "legacy" or "modern" devices (that's purely a virtio-pci concept), so virtio-*-ccw devices don't recognise that switch:
Okay, so you already know what would happen in qemu. Looking at Jan's code in this patch series, (which I didn't do before, but should have) when someone tries to set the option for disable-legacy=on when the device address is anything except PCI , it logs an error and fails. No code for Dan's suggestion has been written yet, but if there's no concept of a legacy mode for virtio-*-ccw, then we would do the same thing. And also I would guess that libosinfo would never suggest that anyone try to add a "virtio1.0" model device to an s390 virtual machine).
silbe@oc4731375738:~$ ~/build/qemu-devel/x86_64-softmmu/qemu-system-x86_64 -device virtio-blk,help 2>&1 |grep legacy virtio-blk-pci.disable-legacy=OnOffAuto (on/off/auto) silbe@oc4731375738:~$ ~/build/qemu-devel/s390x-softmmu/qemu-system-s390x -device virtio-blk,help 2>&1 |grep legacy
That nicely illustrates the issue I have with a) mixing virtio-pci legacy/modern into the model name and b) conflating it with virtio 0.9/1.0 (or transitional/non-transitional for that matter).
FWIW, the thing closest to virtio-pci legacy/modern is virtio-ccw max_revision. But I doubt there's any reason to set this beyond debugging and testing.
Definitely - once we've added an option to libvirt, we have to keep it there forever - our backward compatibility guarantee requires it. So we don't want to add anything unless there's a clear use for it.
[I've browsed through the thread a bit, but as I'm not a libvirt developer I may be missing some basic things] On Wed, 7 Sep 2016 16:34:17 -0400 Laine Stump <laine@laine.org> wrote:
On 09/07/2016 03:38 PM, Sascha Silbe wrote:
Dear Laine,
Laine Stump <laine@laine.org> writes:
On 09/07/2016 02:35 PM, Sascha Silbe wrote:
"Daniel P. Berrange" <berrange@redhat.com> writes: [...]
<sound model="virtio"/> == QEMU virtio <sound model="virtio1.0"/> == QEMU virtio + disable-legacy
I'm not a fan of the virtio1.0 name, but that has already been commented upon elsewhere.
What would this do for devices using the virtio-ccw transport? From libvirt's point of view, the option "disable-legacy=on" would be added to the device's commandline argument. Which would break s390x guests. virtio-ccw doesn't have any concept of "legacy" or "modern" devices (that's purely a virtio-pci concept), so virtio-*-ccw devices don't recognise that switch:
Okay, so you already know what would happen in qemu. Looking at Jan's code in this patch series, (which I didn't do before, but should have) when someone tries to set the option for disable-legacy=on when the device address is anything except PCI , it logs an error and fails.
Can't you make this a virtio-pci only switch? Or is that problem occurring when expanding a generic virtio device?
No code for Dan's suggestion has been written yet, but if there's no concept of a legacy mode for virtio-*-ccw, then we would do the same thing. And also I would guess that libosinfo would never suggest that anyone try to add a "virtio1.0" model device to an s390 virtual machine).
The thing is that, unlike virtio-pci, we don't have any reason to actually disable support for legacy virtio devices, and therefore don't have a switch for it.
silbe@oc4731375738:~$ ~/build/qemu-devel/x86_64-softmmu/qemu-system-x86_64 -device virtio-blk,help 2>&1 |grep legacy virtio-blk-pci.disable-legacy=OnOffAuto (on/off/auto) silbe@oc4731375738:~$ ~/build/qemu-devel/s390x-softmmu/qemu-system-s390x -device virtio-blk,help 2>&1 |grep legacy
That nicely illustrates the issue I have with a) mixing virtio-pci legacy/modern into the model name and b) conflating it with virtio 0.9/1.0 (or transitional/non-transitional for that matter).
FWIW, the thing closest to virtio-pci legacy/modern is virtio-ccw max_revision. But I doubt there's any reason to set this beyond debugging and testing.
Definitely - once we've added an option to libvirt, we have to keep it there forever - our backward compatibility guarantee requires it. So we don't want to add anything unless there's a clear use for it.
I don't see any reason why you'd want to make max_revision configurable from libvirt. QEMU using it for compat machines is the only reason for it.
On 09/13/2016 10:43 AM, Cornelia Huck wrote:
[I've browsed through the thread a bit, but as I'm not a libvirt developer I may be missing some basic things]
On Wed, 7 Sep 2016 16:34:17 -0400 Laine Stump <laine@laine.org> wrote:
On 09/07/2016 03:38 PM, Sascha Silbe wrote:
Dear Laine,
Laine Stump <laine@laine.org> writes:
On 09/07/2016 02:35 PM, Sascha Silbe wrote:
"Daniel P. Berrange" <berrange@redhat.com> writes: [...]
<sound model="virtio"/> == QEMU virtio <sound model="virtio1.0"/> == QEMU virtio + disable-legacy
I'm not a fan of the virtio1.0 name, but that has already been commented upon elsewhere.
What would this do for devices using the virtio-ccw transport? From libvirt's point of view, the option "disable-legacy=on" would be added to the device's commandline argument. Which would break s390x guests. virtio-ccw doesn't have any concept of "legacy" or "modern" devices (that's purely a virtio-pci concept), so virtio-*-ccw devices don't recognise that switch: Okay, so you already know what would happen in qemu. Looking at Jan's code in this patch series, (which I didn't do before, but should have) when someone tries to set the option for disable-legacy=on when the device address is anything except PCI , it logs an error and fails. Can't you make this a virtio-pci only switch? Or is that problem occurring when expanding a generic virtio device?
Taking disks as an example (but it's the same for other devices), in libvirt virtio-blk-pci, virtio-blk-ccw, virtio-blk-s390, and virtio-blk-device (mmio) devices are all derived from a single base device: <disk ....> <target .... bus='virtio'/> The choice between -pci, -ccw, -device, -s390 is made based on the type of <address> element in the disk definition. <address type='pci|ccw|virtio-s390|virtio-mmio' .../> (no, I don't know why the s390 and mmio address types include "virtio-"; seems a bit redundant to me). Even if that wasn't the case, we try to maintain consistency between the formats of the different kinds of devices, so it's always possible someone would try to set [whatever option we come up with] for the wrong type of device (since in the end the configuration is just a text file). If they do that, Jan's patches would log an error; so yesy, it is a virtio-pci-only switch.
No code for Dan's suggestion has been written yet, but if there's no concept of a legacy mode for virtio-*-ccw, then we would do the same thing. And also I would guess that libosinfo would never suggest that anyone try to add a "virtio1.0" model device to an s390 virtual machine). The thing is that, unlike virtio-pci, we don't have any reason to actually disable support for legacy virtio devices, and therefore don't have a switch for it.
Yes.
silbe@oc4731375738:~$ ~/build/qemu-devel/x86_64-softmmu/qemu-system-x86_64 -device virtio-blk,help 2>&1 |grep legacy virtio-blk-pci.disable-legacy=OnOffAuto (on/off/auto) silbe@oc4731375738:~$ ~/build/qemu-devel/s390x-softmmu/qemu-system-s390x -device virtio-blk,help 2>&1 |grep legacy
That nicely illustrates the issue I have with a) mixing virtio-pci legacy/modern into the model name and b) conflating it with virtio 0.9/1.0 (or transitional/non-transitional for that matter).
FWIW, the thing closest to virtio-pci legacy/modern is virtio-ccw max_revision. But I doubt there's any reason to set this beyond debugging and testing. Definitely - once we've added an option to libvirt, we have to keep it there forever - our backward compatibility guarantee requires it. So we don't want to add anything unless there's a clear use for it.
I don't see any reason why you'd want to make max_revision configurable from libvirt. QEMU using it for compat machines is the only reason for it.
Good.
On 09/13/2016 05:16 PM, Laine Stump wrote:
On 09/13/2016 10:43 AM, Cornelia Huck wrote:
What would this do for devices using the virtio-ccw transport? From libvirt's point of view, the option "disable-legacy=on" would be added to the device's commandline argument. Which would break s390x guests. virtio-ccw doesn't have any concept of "legacy" or "modern" devices (that's purely a virtio-pci concept), so virtio-*-ccw devices don't recognise that switch: Okay, so you already know what would happen in qemu. Looking at Jan's code in this patch series, (which I didn't do before, but should have) when someone tries to set the option for disable-legacy=on when the device address is anything except PCI , it logs an error and fails. Can't you make this a virtio-pci only switch? Or is that problem occurring when expanding a generic virtio device?
Taking disks as an example (but it's the same for other devices), in libvirt virtio-blk-pci, virtio-blk-ccw, virtio-blk-s390, and virtio-blk-device (mmio) devices are all derived from a single base device:
<disk ....> <target .... bus='virtio'/>
The choice between -pci, -ccw, -device, -s390 is made based on the type of <address> element in the disk definition.
<address type='pci|ccw|virtio-s390|virtio-mmio' .../>
(no, I don't know why the s390 and mmio address types include "virtio-"; seems a bit redundant to me).
Even if that wasn't the case, we try to maintain consistency between the formats of the different kinds of devices, so it's always possible someone would try to set [whatever option we come up with] for the wrong type of device (since in the end the configuration is just a text file). If they do that, Jan's patches would log an error; so yesy, it is a virtio-pci-only switch.
I totally agree that commonality is good and should tried to be reached. In this case it is already known that the compatibility setting legacy, transition and modern are not "common virtio" but virtio-pci only. Looking at the current proposal the user finds in the documentation <dt><code>driver</code></dt> The <code>compatibility</code> attribute can be used to specify the compatibility of virtio devices. Allowed values are <code>legacy</code>, <code>transitional</code> and <code>modern</code>. <span class="since">Since 2.2.0</span>. This reads like a common virtio feature and when the user specifies it on non virtio-pci devices he ends up when starting the domain with an error message: "... setting the virtio revision is only supported for PCI devices" Next thing he asks himself: When is e.g. virtio-ccw going to support this feature? I think that by renaming the driver attribute compatibility into pci-compatibility and also changing the documentation to clearly state that it is used "to specify the compatibility of virtio-pci devices only" would make it much easier for the users. -- 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 Wed, Sep 07, 2016 at 09:38:04PM +0200, Sascha Silbe wrote:
Dear Laine,
Laine Stump <laine@laine.org> writes:
On 09/07/2016 02:35 PM, Sascha Silbe wrote:
"Daniel P. Berrange" <berrange@redhat.com> writes: [...]
<sound model="virtio"/> == QEMU virtio <sound model="virtio1.0"/> == QEMU virtio + disable-legacy
What would this do for devices using the virtio-ccw transport?
From libvirt's point of view, the option "disable-legacy=on" would be added to the device's commandline argument.
Which would break s390x guests. virtio-ccw doesn't have any concept of "legacy" or "modern" devices (that's purely a virtio-pci concept),
Legacy is a generic concept found in virtio 1 spec. Modern isn't, virtio 1 only has transitional/non-transitional. If you want to stick to the spec you should therefore use legacy/transitional/non-transitional at the API level. It is true that not all transports have all options. ccw does not support non-transitional devices, mmio does not support transitional ones. So in a quest to give users the most flexibility QEMU interface became messy as usual. Sorry about that :(
so virtio-*-ccw devices don't recognise that switch:
silbe@oc4731375738:~$ ~/build/qemu-devel/x86_64-softmmu/qemu-system-x86_64 -device virtio-blk,help 2>&1 |grep legacy virtio-blk-pci.disable-legacy=OnOffAuto (on/off/auto) silbe@oc4731375738:~$ ~/build/qemu-devel/s390x-softmmu/qemu-system-s390x -device virtio-blk,help 2>&1 |grep legacy
That nicely illustrates the issue I have with a) mixing virtio-pci legacy/modern into the model name and b) conflating it with virtio 0.9/1.0 (or transitional/non-transitional for that matter).
So the point is that qemu would typically do the right thing. So maybe for the model it makes sense to have legacy/transitional/non-transitional/auto and then translate it to whatever qemu flags are.
FWIW, the thing closest to virtio-pci legacy/modern is virtio-ccw max_revision. But I doubt there's any reason to set this beyond debugging and testing.
Can be helpful as work-arounds for guest/host bugs as well.
For the effect, you would need to ask a qemu virtio person, or even better - a qemu s390 person who knows something about about virtio. I Cc'ed Michael (the former) and Cornelia Huck (the latter, according to this patch I found: https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg01024.html )
Thanks, but I'm working on qemu for s390x myself. :)
Sascha -- Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg https://se-silbe.de/ USt-IdNr. DE281696641
On Mon, 19 Sep 2016 22:07:13 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote:
On Wed, Sep 07, 2016 at 09:38:04PM +0200, Sascha Silbe wrote:
Dear Laine,
Laine Stump <laine@laine.org> writes:
On 09/07/2016 02:35 PM, Sascha Silbe wrote:
"Daniel P. Berrange" <berrange@redhat.com> writes: [...]
<sound model="virtio"/> == QEMU virtio <sound model="virtio1.0"/> == QEMU virtio + disable-legacy
What would this do for devices using the virtio-ccw transport?
From libvirt's point of view, the option "disable-legacy=on" would be added to the device's commandline argument.
Which would break s390x guests. virtio-ccw doesn't have any concept of "legacy" or "modern" devices (that's purely a virtio-pci concept),
Legacy is a generic concept found in virtio 1 spec. Modern isn't, virtio 1 only has transitional/non-transitional.
If you want to stick to the spec you should therefore use legacy/transitional/non-transitional at the API level.
It is true that not all transports have all options. ccw does not support non-transitional devices, mmio does not support transitional ones.
We basically have: - legacy devices: pci, ccw - standard-compliant devices: pci, ccw, mmio - of these, pci and ccw have transitional support, which can be disabled for pci Per default, I don't think libvirt should offer legacy devices - I see this as a machine-version backwards-compatibility option which should be handled via the machine version only. It makes sense to have a way to force pci devices to either transitional or non-transitional. For ccw and mmio, this is not applicable. Therefore, I think libvirt should offer a switch to handle transitional/non-transitional pci only and not provide any other virtio-version switches.
On 09/07/2016 12:40 PM, Daniel P. Berrange wrote:
On Wed, Sep 07, 2016 at 05:52:02PM +0200, Peter Krempa wrote:
On Wed, Sep 07, 2016 at 16:13:57 +0100, Daniel Berrange wrote:
On Wed, Sep 07, 2016 at 05:10:01PM +0200, Peter Krempa wrote:
On Wed, Aug 24, 2016 at 00:20:42 +0200, Ján Tomko wrote:
Obviously making the user responsible for correct setting is much easier.
Sigh. It'd be great if somebody else could also state their opinion on this. I can't say that I'm a big fan of this approach but it looks like as if it would be the only sensible one. I tend to agree. I'd be incredibly happy if we didn't add any of this to the XML and would simply "do the best thing" automatically.
I'm particularly not a fan of adding something "just in case there is a bug" Yep, that's why I don't see a reason for using --disable-modern in libvirt. It does not have any value besides working around "possible bugs".
On the other hand, I'm kind of worried that with "--disable-legacy" we are entering the domain of policy decisions as it's uncertain to me whether: * Enabling it would break guests with very old drivers. * Guest firmware actually supports the modern mode (both ovmf and bios) * other possible caveats that I can't think of right now
In such case we'd need to expose a knob to set it, but I'd prefer dropping the possibility to use legacy mode only since it's of no value. We could take a completely different approach to this based on the model.
Effectively virtio-1.0 with disable-legacy is a completely separate hardware from the original virtio-0.9, with or without disable-modern. You even get a completely different PCI ID with disable-legacy set. So we could in fact represent this in the XML as a different model called "virtio1.0", and remap that to virtio+disable-legacy in the QEMU argv.
This is in fact what we've done with naming in libosinfo already, due to the fact it has different PCI IDs and so is logically a different device.
IOW,
<sound model="virtio"/> == QEMU virtio <sound model="virtio1.0"/> == QEMU virtio + disable-legacy
(you worried me for a minute, with your virtual ("nonexistent") virtio sound device. I wondered if I had missed one!) Yes, I agree it's a good idea for a different PCI device ID be represented with a different device model. I just don't like the name "virtio1.0", which will be obsolete and technically incorrect as soon as there is a virtio-1.1. I can't think of anything better right now though. And what about virtio input and video devices, which have a model "virtio" that has from the beginning meant "legacy-free virtio" (since they came into existence after virtio-1.0)? Also, note that qemu's behavior in 2.7.0+ kind of muddies the waters a bit - if you plug a virtio device into a PCI Express slot, it will automatically get "disable-legacy=on", thus turning the "virtio" device into a "virtio1.0" device (ie changing the PCI device ID). This means libvirt will forever more need to explicitly add "disable- legacy=off" for virtio devices in PCI Express slots.
and explicitly don't provide any disable-modern flag mapping, since there is no compelling use case for that.
Agreed.
participants (9)
-
Boris Fiuczynski -
Cornelia Huck -
Daniel P. Berrange -
Han Han -
Ján Tomko -
Laine Stump -
Michael S. Tsirkin -
Peter Krempa -
Sascha Silbe