[libvirt] [PATCHv2 02/11] Add virtio revision attribute to memballoon
Laine Stump
laine at laine.org
Wed Aug 10 02:37:30 UTC 2016
On 08/08/2016 12:35 PM, Ján Tomko wrote:
> <memballoon model='virtio'>
> <virtio revision='0.9'/>
> </memballoon>
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1227354
> ---
> docs/formatdomain.html.in | 9 +++
> docs/schemas/domaincommon.rng | 16 ++++
> src/conf/domain_conf.c | 86 ++++++++++++++++++++++
> src/conf/domain_conf.h | 9 +++
> .../qemuxml2argv-virtio-revision.xml | 54 ++++++++++++++
> .../qemuxml2xmlout-virtio-revision.xml | 54 ++++++++++++++
> tests/qemuxml2xmltest.c | 2 +
> 7 files changed, 230 insertions(+)
> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml
> create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml
>
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 5acb3b9..d01f92d 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -6415,6 +6415,15 @@ qemu-kvm -net nic,model=? /dev/null
> <span class='since'>Since 1.1.1, requires QEMU 1.5</span>
> </p>
> </dd>
> + <dt><code>virtio</code></dt>
> + <dd>
> + <p>
> + An optional <code>virtio</code> can be used to enforce a particular
> + virtio revision in QEMU. The valid values for the <code>revision</code>
> + are <code>0.9</code> and <code>1.0</code>.
> + <span class='since'>Since 2.2.0</span>
> + </p>
> + </dd>
> </dl>
> <h4><a name="elementsRng">Random number generator device</a></h4>
>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 052f28c..64088ba 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -3612,6 +3612,9 @@
> </attribute>
> </element>
> </optional>
> + <optional>
> + <ref name="virtioRevision"/>
> + </optional>
> </interleave>
> </element>
> </define>
> @@ -4830,6 +4833,19 @@
> </element>
> </define>
>
> + <define name="virtioRevision">
> + <zeroOrMore>
> + <element name="virtio">
> + <attribute name="revision">
> + <choice>
> + <value>0.9</value>
> + <value>1.0</value>
> + </choice>
> + </attribute>
> + </element>
> + </zeroOrMore>
> + </define>
> +
> <define name="usbmaster">
> <element name="master">
> <attribute name="startport">
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 5dbeb1a..b006bc9 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -834,6 +834,12 @@ VIR_ENUM_IMPL(virDomainLoader,
> "rom",
> "pflash")
>
> +VIR_ENUM_IMPL(virDomainVirtioRevision,
> + VIR_DOMAIN_VIRTIO_REVISION_LAST,
> + "default",
> + "0.9",
> + "1.0")
> +
> /* Internal mapping: subset of block job types that can be present in
> * <mirror> XML (remaining types are not two-phase). */
> VIR_ENUM_DECL(virDomainBlockJob)
> @@ -1059,6 +1065,81 @@ virDomainXMLOptionGetNamespace(virDomainXMLOptionPtr xmlopt)
> return &xmlopt->ns;
> }
>
> +static int
> +virDomainVirtioRevisionParseXML(xmlXPathContextPtr ctxt,
> + virBitmapPtr *res)
> +{
> + xmlNodePtr save = ctxt->node;
> + xmlNodePtr *nodes = NULL;
> + char *str = NULL;
> + int ret = -1;
> + size_t i;
> + int n;
> + virBitmapPtr revmap = NULL;
> +
> + if ((n = virXPathNodeSet("./virtio", ctxt, &nodes)) < 0)
> + goto cleanup;
> +
> + if (n == 0) {
> + ret = 0;
> + goto cleanup;
> + }
> +
> + if (!(revmap = virBitmapNew(VIR_DOMAIN_VIRTIO_REVISION_LAST)))
> + goto cleanup;
> +
> + for (i = 0; i < n; i++) {
> + int val;
> +
> + ctxt->node = nodes[i];
> +
> + if (!(str = virXPathString("string(./@revision)", ctxt))) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("Missing 'revision' attribute in <virtio> element"));
> + goto cleanup;
> + }
> +
> + if ((val = virDomainVirtioRevisionTypeFromString(str)) <= 0) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("Unable to parse virtio revision: '%s'"),
> + str);
> + goto cleanup;
> + }
> +
> + ignore_value(virBitmapSetBit(revmap, val));
It is just really.... odd that you are storing an attribute that can
have one of two possible values as a bitmap. Unless you have some
specific future plan for that, it really should just be stored as, well,
the value itself.
(Maybe your idea is to maybe someday allow forcing support for both? If
so, then how about an enum value called "ALL" that gets turned into
"disable-legacy=off,disable-modern=off"?)
> + }
> +
> + ret = 0;
> + VIR_STEAL_PTR(*res, revmap);
> +
> + cleanup:
> + ctxt->node = save;
> + virBitmapFree(revmap);
> + VIR_FREE(nodes);
> + VIR_FREE(str);
> + return ret;
> +}
> +
> +
> +static void
> +virDomainVirtioRevisionFormatXML(virBufferPtr buf,
> + virBitmapPtr revmap)
> +{
> + size_t i;
> +
> + if (!revmap)
> + return;
> +
> + for (i = VIR_DOMAIN_VIRTIO_REVISION_DEFAULT + 1;
> + i < VIR_DOMAIN_VIRTIO_REVISION_LAST;
> + i++) {
> + if (virBitmapIsBitSet(revmap, i)) {
> + virBufferAsprintf(buf, "<virtio revision='%s'/>\n",
> + virDomainVirtioRevisionTypeToString(i));
> + }
> + }
> +}
> +
>
> void
> virBlkioDeviceArrayClear(virBlkioDevicePtr devices,
> @@ -12061,6 +12142,9 @@ virDomainMemballoonDefParseXML(xmlNodePtr node,
> else if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0)
> goto error;
>
> + if (virDomainVirtioRevisionParseXML(ctxt, &def->virtio_rev) < 0)
> + goto error;
> +
> cleanup:
> VIR_FREE(model);
> VIR_FREE(deflate);
> @@ -21492,6 +21576,8 @@ virDomainMemballoonDefFormat(virBufferPtr buf,
> return -1;
> }
>
> + virDomainVirtioRevisionFormatXML(&childrenBuf, def->virtio_rev);
> +
> if (!virBufferUse(&childrenBuf)) {
> virBufferAddLit(buf, "/>\n");
> } else {
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 8b26724..2b39f41 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -154,6 +154,13 @@ typedef virDomainTPMDef *virDomainTPMDefPtr;
> typedef struct _virDomainIOMMUDef virDomainIOMMUDef;
> typedef virDomainIOMMUDef *virDomainIOMMUDefPtr;
>
> +typedef enum {
> + VIR_DOMAIN_VIRTIO_REVISION_DEFAULT,
> + VIR_DOMAIN_VIRTIO_REVISION_HERITAGE,
> + VIR_DOMAIN_VIRTIO_REVISION_CONTEMPORARY,
> + VIR_DOMAIN_VIRTIO_REVISION_LAST,
> +} virDomainVirtioRevision;
And beyond storing it in a bitmap, why do you invent *yet another* name
for these things. It was confusing enough that virtio 0.9 is also known
as "legacy", and 1.0 as "modern", but now you've come up with a 3rd way
to say the same thing. I would suggest giving them the same names as the
strings that they represent:
VIR_DOMAIN_VIRTIO_REVISION_0_9
VIR_DOMAIN_VIRTIO_REVISION_1_0
> +
> /* Flags for the 'type' field in virDomainDeviceDef */
> typedef enum {
> VIR_DOMAIN_DEVICE_NONE = 0,
> @@ -1546,6 +1553,7 @@ struct _virDomainMemballoonDef {
> virDomainDeviceInfo info;
> int period; /* seconds between collections */
> int autodeflate; /* enum virTristateSwitch */
> + virBitmapPtr virtio_rev;
> };
>
> struct _virDomainNVRAMDef {
> @@ -3022,6 +3030,7 @@ VIR_ENUM_DECL(virDomainTPMBackend)
> VIR_ENUM_DECL(virDomainMemoryModel)
> VIR_ENUM_DECL(virDomainMemoryBackingModel)
> VIR_ENUM_DECL(virDomainIOMMUModel)
> +VIR_ENUM_DECL(virDomainVirtioRevision)
> /* from libvirt.h */
> VIR_ENUM_DECL(virDomainState)
> VIR_ENUM_DECL(virDomainNostateReason)
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml b/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml
> new file mode 100644
> index 0000000..d971e89
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml
> @@ -0,0 +1,54 @@
> +<domain type='qemu'>
> + <name>QEMUGuest1</name>
> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
> + <memory unit='KiB'>219136</memory>
> + <currentMemory unit='KiB'>219136</currentMemory>
> + <vcpu placement='static'>1</vcpu>
> + <os>
> + <type arch='x86_64' machine='pc'>hvm</type>
> + <boot dev='hd'/>
> + </os>
> + <clock offset='utc'/>
> + <on_poweroff>destroy</on_poweroff>
> + <on_reboot>restart</on_reboot>
> + <on_crash>destroy</on_crash>
> + <devices>
> + <emulator>/usr/bin/qemu</emulator>
> + <controller type='usb' index='0'>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
> + </controller>
> + <controller type='ide' index='0'>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/>
> + </controller>
> + <controller type='pci' index='0' model='pci-root'/>
> + <controller type='virtio-serial' index='0'>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x09' function='0x0'/>
> + </controller>
> + <input type='mouse' bus='virtio'>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x0e' function='0x0'/>
> + </input>
> + <input type='keyboard' bus='virtio'>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x10' function='0x0'/>
> + </input>
> + <input type='tablet' bus='virtio'>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x11' function='0x0'/>
> + </input>
> + <input type='passthrough' bus='virtio'>
> + <source evdev='/dev/input/event1234'/>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x12' function='0x0'/>
> + </input>
> + <input type='mouse' bus='ps2'/>
> + <input type='keyboard' bus='ps2'/>
> + <video>
> + <model type='virtio' heads='1' primary='yes'>
> + <acceleration accel3d='yes'/>
> + </model>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/>
> + </video>
> + <memballoon model='virtio'>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x0c' function='0x0'/>
> + <virtio revision='0.9'/>
> + <virtio revision='1.0'/>
> + </memballoon>
> + </devices>
> +</domain>
> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml
> new file mode 100644
> index 0000000..d971e89
> --- /dev/null
> +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml
> @@ -0,0 +1,54 @@
> +<domain type='qemu'>
> + <name>QEMUGuest1</name>
> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
> + <memory unit='KiB'>219136</memory>
> + <currentMemory unit='KiB'>219136</currentMemory>
> + <vcpu placement='static'>1</vcpu>
> + <os>
> + <type arch='x86_64' machine='pc'>hvm</type>
> + <boot dev='hd'/>
> + </os>
> + <clock offset='utc'/>
> + <on_poweroff>destroy</on_poweroff>
> + <on_reboot>restart</on_reboot>
> + <on_crash>destroy</on_crash>
> + <devices>
> + <emulator>/usr/bin/qemu</emulator>
> + <controller type='usb' index='0'>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
> + </controller>
> + <controller type='ide' index='0'>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/>
> + </controller>
> + <controller type='pci' index='0' model='pci-root'/>
> + <controller type='virtio-serial' index='0'>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x09' function='0x0'/>
> + </controller>
> + <input type='mouse' bus='virtio'>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x0e' function='0x0'/>
> + </input>
> + <input type='keyboard' bus='virtio'>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x10' function='0x0'/>
> + </input>
> + <input type='tablet' bus='virtio'>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x11' function='0x0'/>
> + </input>
> + <input type='passthrough' bus='virtio'>
> + <source evdev='/dev/input/event1234'/>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x12' function='0x0'/>
> + </input>
> + <input type='mouse' bus='ps2'/>
> + <input type='keyboard' bus='ps2'/>
> + <video>
> + <model type='virtio' heads='1' primary='yes'>
> + <acceleration accel3d='yes'/>
> + </model>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/>
> + </video>
> + <memballoon model='virtio'>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x0c' function='0x0'/>
> + <virtio revision='0.9'/>
> + <virtio revision='1.0'/>
> + </memballoon>
> + </devices>
> +</domain>
> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
> index 7601a5f..6952abe 100644
> --- a/tests/qemuxml2xmltest.c
> +++ b/tests/qemuxml2xmltest.c
> @@ -862,6 +862,8 @@ mymain(void)
> DO_TEST("virtio-input", NONE);
> DO_TEST("virtio-input-passthrough", NONE);
>
> + DO_TEST_FULL("virtio-revision", WHEN_BOTH, GIC_NONE, QEMU_CAPS_VIRTIO_SCSI, NONE);
> +
> virObjectUnref(cfg);
>
> DO_TEST("acpi-table", NONE);
More information about the libvir-list
mailing list