Re: [libvirt] [PATCH 1/3] Introduce <iommu> device

28 Jun 2016

On 06/23/2016 06:47 AM, Ján Tomko wrote:
...
A device with an attribude 'model'.
Attribute
...
intel-iommu is accepted so far:
<devices>
  ...
  <iommu model='intel-iommu'/>
</devices>
The text reads like it already exists, but we're adding the XML here.
...
https://bugzilla.redhat.com/show_bug.cgi?id=1235580
---
 docs/schemas/domaincommon.rng                      | 11 +++++++
 src/conf/domain_conf.c                             | 37 ++++++++++++++++++++++
 src/conf/domain_conf.h                             | 14 ++++++++
 src/libvirt_private.syms                           |  2 ++
 .../qemuxml2argvdata/qemuxml2argv-intel-iommu.xml  | 37 ++++++++++++++++++++++
 .../qemuxml2xmlout-intel-iommu.xml                 | 37 ++++++++++++++++++++++
 tests/qemuxml2xmltest.c                            |  4 +++
 7 files changed, 142 insertions(+)
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-intel-iommu.xml
 create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-intel-iommu.xml
FYI: There is a qemu v5 of the series now:

https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg07686.html

w/r/t this patch:

There's no description in formatdomain.html.in

I think you need to add ABI checks too.

Although the implementation is to add a "-device intel-iommu' to the
command line, it would seem to me based purely on the former/current
means to enable this via "-machine,iommu=on" and the comments from the
qemu commit:

"The device is part of the machine properties because we wanted
to ensure is created before any other PCI device."

is this then perhaps more of a hypervisor feature since IIUC in the long
run it's the Intel VT-d and AMD IOV "support" that allow it to work. It
seems though if it were to be used prior to qemu 2.7, then providing
"machine,iommu=on" would be required.

In any case, probably need to wait for QEMU fixes to be finally accepted
as well...
...

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 162c2e0..c8a8ecb 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -3709,6 +3709,14 @@
    </optional>
   </define>
+  <define name="iommu">
+    <element name="iommu">
+      <attribute name="model">
+        <value>intel-iommu</value>
Isn't this redundant?

<iommu model='intel-iommu'>

Is the future command option going to be "-device amd-iommu" or "-device
ppc-iommu" or "-device arm-iommu"?

Seems we'd want to indicate "<iommu model='intel'/> and then generate
the command using '%s'-iommu, using *TypeToString.
...
+      </attribute>
+    </element>
+  </define>
+
   <define name="input">
     <element name="input">
       <choice>
@@ -4198,6 +4206,9 @@
         <zeroOrMore>
           <ref name="panic"/>
         </zeroOrMore>
+        <optional>
+          <ref name="iommu"/>
+        </optional>
       </interleave>
     </element>
   </define>
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 9443281..07889ff 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -802,6 +802,9 @@ VIR_ENUM_IMPL(virDomainTPMModel, VIR_DOMAIN_TPM_MODEL_LAST,
 VIR_ENUM_IMPL(virDomainTPMBackend, VIR_DOMAIN_TPM_TYPE_LAST,
               "passthrough")
+VIR_ENUM_IMPL(virDomainIOMMUModel, VIR_DOMAIN_IOMMU_MODEL_LAST,
+              "intel-iommu")
+
 VIR_ENUM_IMPL(virDomainDiskDiscard, VIR_DOMAIN_DISK_DISCARD_LAST,
               "default",
               "unmap",
@@ -2615,6 +2618,8 @@ void virDomainDefFree(virDomainDefPtr def)
         virDomainPanicDefFree(def->panics[i]);
     VIR_FREE(def->panics);
+    VIR_FREE(def->iommu);
+
     VIR_FREE(def->idmap.uidmap);
     VIR_FREE(def->idmap.gidmap);
@@ -17239,6 +17244,33 @@ virDomainDefParseXML(xmlDocPtr xml,
     }
     VIR_FREE(nodes);
+    if ((n = virXPathNodeSet("./devices/iommu", ctxt, &nodes)) < 0)
+        goto error;
+
+    if (n > 1) {
+        virReportError(VIR_ERR_XML_ERROR, "%s",
+                       _("only a single IOMMU device is supported"));
+        goto error;
+    }
+
+    if (n > 0) {
+        int val;
+        if (VIR_ALLOC(def->iommu) < 0)
+            goto error;
+
+        if (!(tmp = virXMLPropString(nodes[0], "model"))) {
+            virReportError(VIR_ERR_XML_ERROR, "%s",
+                           _("missing model for IOMMU device"));
+            goto error;
+        }
+        if ((val = virDomainIOMMUModelTypeFromString(tmp)) < 0) {
You won't need val, if you go with

 if ((def->iommu->model = ...

IDC either way.
...
+            virReportError(VIR_ERR_XML_ERROR, _("unknown IOMMU model: %s"), tmp);
+            goto error;
+        }
+        def->iommu->model = val;
+    }
+    VIR_FREE(nodes);
+
     /* analysis of the user namespace mapping */
     if ((n = virXPathNodeSet("./idmap/uid", ctxt, &nodes)) < 0)
         goto error;
@@ -23561,6 +23593,11 @@ virDomainDefFormatInternal(virDomainDefPtr def,
             goto error;
     }
+    if (def->iommu) {
+        virBufferAsprintf(buf, "<iommu model='%s'/>\n",
+                          virDomainIOMMUModelTypeToString(def->iommu->model));
+    }
+
     virBufferAdjustIndent(buf, -2);
     virBufferAddLit(buf, "</devices>\n");
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 6e81e52..918c185 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -150,6 +150,9 @@ typedef virDomainShmemDef *virDomainShmemDefPtr;
 typedef struct _virDomainTPMDef virDomainTPMDef;
 typedef virDomainTPMDef *virDomainTPMDefPtr;
+typedef struct _virDomainIOMMUDef virDomainIOMMUDef;
+typedef virDomainIOMMUDef *virDomainIOMMUDefPtr;
+
 /* Flags for the 'type' field in virDomainDeviceDef */
 typedef enum {
     VIR_DOMAIN_DEVICE_NONE = 0,
@@ -2112,6 +2115,15 @@ struct _virDomainKeyWrapDef {
     int dea; /* enum virTristateSwitch */
 };
+typedef enum {
+    VIR_DOMAIN_IOMMU_MODEL_INTEL,
+
+    VIR_DOMAIN_IOMMU_MODEL_LAST
+} virDomainIOMMUModel;
+
+struct _virDomainIOMMUDef {
+    virDomainIOMMUModel model;
+};
 /*
  * Guest VM main configuration
  *
@@ -2249,6 +2261,7 @@ struct _virDomainDef {
     virCPUDefPtr cpu;
     virSysinfoDefPtr sysinfo;
     virDomainRedirFilterDefPtr redirfilter;
+    virDomainIOMMUDefPtr iommu;
void *namespaceData;
     virDomainXMLNamespace ns;
@@ -3006,6 +3019,7 @@ VIR_ENUM_DECL(virDomainTPMModel)
 VIR_ENUM_DECL(virDomainTPMBackend)
 VIR_ENUM_DECL(virDomainMemoryModel)
 VIR_ENUM_DECL(virDomainMemoryBackingModel)
+VIR_ENUM_DECL(virDomainIOMMUModel)
 /* from libvirt.h */
 VIR_ENUM_DECL(virDomainState)
 VIR_ENUM_DECL(virDomainNostateReason)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 501c23e..73cecfd 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -343,6 +343,8 @@ virDomainHubTypeToString;
 virDomainHypervTypeFromString;
 virDomainHypervTypeToString;
 virDomainInputDefFree;
+virDomainIOMMUModelTypeFromString;
+virDomainIOMMUModelTypeToString;
 virDomainIOThreadIDAdd;
 virDomainIOThreadIDDefFree;
 virDomainIOThreadIDDel;
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-intel-iommu.xml b/tests/qemuxml2argvdata/qemuxml2argv-intel-iommu.xml
new file mode 100644
index 0000000..8d95383
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-intel-iommu.xml
@@ -0,0 +1,37 @@
+<domain type='qemu'>
+  <name>QEMUGuest1</name>
+  <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+  <memory unit='KiB'>219100</memory>
+  <currentMemory unit='KiB'>219100</currentMemory>
+  <vcpu placement='static'>1</vcpu>
+  <os>
+    <type arch='x86_64' machine='q35'>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='pci' index='0' model='pcie-root'/>
+    <controller type='pci' index='1' model='dmi-to-pci-bridge'>
+      <model name='i82801b11-bridge'/>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x1e' function='0x0'/>
+    </controller>
+    <controller type='pci' index='2' model='pci-bridge'>
+      <model name='pci-bridge'/>
+      <target chassisNr='2'/>
+      <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/>
+    </controller>
+    <controller type='sata' index='0'>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/>
+    </controller>
+    <input type='mouse' bus='ps2'/>
+    <input type='keyboard' bus='ps2'/>
+    <memballoon model='virtio'>
+      <address type='pci' domain='0x0000' bus='0x02' slot='0x01' function='0x0'/>
+    </memballoon>
+    <iommu model='intel-iommu'/>
+  </devices>
+</domain>
diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-intel-iommu.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-intel-iommu.xml
new file mode 100644
Since I recently went through this with the secret/LUKS changes and the
fact that the input/output files don't differ, if you create a soft link
in the output directory to the argvdata directory for it's file instead
of a whole new file, then the xml2xml test will use that.


John
...
index 0000000..8d95383
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-intel-iommu.xml
@@ -0,0 +1,37 @@
+<domain type='qemu'>
+  <name>QEMUGuest1</name>
+  <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+  <memory unit='KiB'>219100</memory>
+  <currentMemory unit='KiB'>219100</currentMemory>
+  <vcpu placement='static'>1</vcpu>
+  <os>
+    <type arch='x86_64' machine='q35'>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='pci' index='0' model='pcie-root'/>
+    <controller type='pci' index='1' model='dmi-to-pci-bridge'>
+      <model name='i82801b11-bridge'/>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x1e' function='0x0'/>
+    </controller>
+    <controller type='pci' index='2' model='pci-bridge'>
+      <model name='pci-bridge'/>
+      <target chassisNr='2'/>
+      <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/>
+    </controller>
+    <controller type='sata' index='0'>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/>
+    </controller>
+    <input type='mouse' bus='ps2'/>
+    <input type='keyboard' bus='ps2'/>
+    <memballoon model='virtio'>
+      <address type='pci' domain='0x0000' bus='0x02' slot='0x01' function='0x0'/>
+    </memballoon>
+    <iommu model='intel-iommu'/>
+  </devices>
+</domain>
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index 7db9cb7..855eb40 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -829,6 +829,10 @@ mymain(void)
     DO_TEST("video-qxl-heads");
     DO_TEST("video-qxl-noheads");
+    DO_TEST_FULL("intel-iommu", WHEN_ACTIVE, GIC_NONE,
+                 QEMU_CAPS_DEVICE_PCI_BRIDGE,
+                 QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE);
+
     qemuTestDriverFree(&driver);
return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;


    

Re: [libvirt] [PATCH 1/3] Introduce <iommu> device

John Ferlan