Re: [PATCHv2 02/11] Add virtio revision attribute to memballoon

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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);


--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]