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;

On 06/23/2016 06:47 AM, Ján Tomko wrote:

Check whether QEMU supports -device intel-iommu

https://bugzilla.redhat.com/show_bug.cgi?id=1235580 --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml | 1 + 5 files changed, 6 insertions(+)

Your cover letter notes the qemu support -device option is broken and a "fix" is on qemu-devel (assuming it would go into QEMU 2.7). However, you've touched caps 2.4, 2.5, and 2.6. So, if -device is broken before qemu 2.7, what happens if we "pass" our capabilities checks and attempt to add using a broken mechanism? It seems one way to "detect" that the -device iommu is not broken is to not find the "iommu" property on the -machine command line (patch 5 of the qemu series) - a rather ugly means of detection. Or of course we could not care that it's broken? or we could document that although available prior to qemu 2.7, it really only works properly in 2.7 since as qemu-devel reviews pretty much note it wasn't something expected to be in production use yet. I don't think what's below is wrong, but I wonder if we need to add a means to detect things that have been discussed on the qemu-devel regarding the need to remove the machine attribute 'iommu'? If this is all possible, then perhaps we would have a helper API added that would detect the device flag and detect a missing iommu from machine properties and return true/false based on that (so that the "next" patch just calls it rather than checking the bit itself). John

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 4ed5b71..2076297 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -336,6 +336,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "drive-detect-zeroes",

"tls-creds-x509", /* 230 */ + "intel-iommu", );

@@ -1564,6 +1565,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = { { "pxb", QEMU_CAPS_DEVICE_PXB }, { "pxb-pcie", QEMU_CAPS_DEVICE_PXB_PCIE }, { "tls-creds-x509", QEMU_CAPS_OBJECT_TLS_CREDS_X509 }, + { "intel-iommu", QEMU_CAPS_DEVICE_INTEL_IOMMU }, };

static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioBalloon[] = { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index affb639..da448e9 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -370,6 +370,7 @@ typedef enum {

/* 230 */ QEMU_CAPS_OBJECT_TLS_CREDS_X509, /* -object tls-creds-x509 */ + QEMU_CAPS_DEVICE_INTEL_IOMMU, /* -device intel-iommu */

QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml index 112ac95..98c260c 100644 --- a/tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.4.0.x86_64.xml @@ -182,6 +182,7 @@ <flag name='qxl-vga.max_outputs'/> <flag name='spice-unix'/> <flag name='drive-detect-zeroes'/> + <flag name='intel-iommu'/> <version>2004000</version> <kvmVersion>0</kvmVersion> <package></package> diff --git a/tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml index 8157985..590c8c1 100644 --- a/tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml @@ -187,6 +187,7 @@ <flag name='spice-unix'/> <flag name='drive-detect-zeroes'/> <flag name='tls-creds-x509'/> + <flag name='intel-iommu'/> <version>2005000</version> <kvmVersion>0</kvmVersion> <package></package> diff --git a/tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml index 1d503dd..128ac11 100644 --- a/tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml @@ -193,6 +193,7 @@ <flag name='spice-unix'/> <flag name='drive-detect-zeroes'/> <flag name='tls-creds-x509'/> + <flag name='intel-iommu'/> <version>2006000</version> <kvmVersion>0</kvmVersion> <package></package>

On 06/23/2016 06:47 AM, Ján Tomko wrote:

<devices> <iommu model='intel-iommu'/> </devices>

results in:

-device intel-iommu

Might be nice to add a bit more "meat" to the commit message.

https://bugzilla.redhat.com/show_bug.cgi?id=1235580 --- src/qemu/qemu_command.c | 30 ++++++++++++++++++++++ .../qemuxml2argvdata/qemuxml2argv-intel-iommu.args | 22 ++++++++++++++++ tests/qemuxml2argvtest.c | 2 ++ 3 files changed, 54 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-intel-iommu.args

One other thing that wasn't 100% from the qemu patches - is there a specific need to ensure the "-device intel-iommu" is placed on the command line prior to any PCI device being placed there as well? It's that same snippet I pulled from review of patch 1: "The device is part of the machine properties because we wanted to ensure is created before any other PCI device." But qemu patch 5/5 removes iommu from the machine command line, so I'm left wondering if there isn't some assumption that the -device option will need to be generated before any other PCI device. I only mention this because if that's the case, then I think we need to add some comments prior to calling qemuBuildIOMMUCommandLine to indicate that placement for the "-device intel-iommu" must be done before other PCI devices. I realize there's a general feeling against too many comments; however, it might avoid having some bug creep in because someone didn't know or follow a bz link or a qemu doc link from a commit message... Alternatively if the XML implementation becomes a hypervisor feature, then qemuBuildBootCommandLine could add the option. That also provides a way to have "either" 'iommu=on' or 'device intel-iommu' be generated with "-machine" or after machine is created to help hide or ensure future generations don't erroneously move things. John

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 6944129..9f1d447 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6171,6 +6171,33 @@ qemuBuildBootCommandLine(virCommandPtr cmd,

static int +qemuBuildIOMMUCommandLine(virCommandPtr cmd, + const virDomainDef *def, + virQEMUCapsPtr qemuCaps) +{ + if (!def->iommu) + return 0; + + switch (def->iommu->model) { + case VIR_DOMAIN_IOMMU_MODEL_INTEL: + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_INTEL_IOMMU)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("IOMMU device: '%s' is not supported with " + "this QEMU binary"), + virDomainIOMMUModelTypeToString(def->iommu->model)); + return -1; + } + virCommandAddArgList(cmd, "-device", + virDomainIOMMUModelTypeToString(def->iommu->model), + NULL);

Obviously this would need adjustment if you changed the model...

+ case VIR_DOMAIN_IOMMU_MODEL_LAST: + break; + } + return 0; +} + + +static int qemuBuildGlobalControllerCommandLine(virCommandPtr cmd, const virDomainDef *def, virQEMUCapsPtr qemuCaps) @@ -9254,6 +9281,9 @@ qemuBuildCommandLine(virQEMUDriverPtr driver, if (qemuBuildBootCommandLine(cmd, def, qemuCaps, &emitBootindex) < 0) goto error;

+ if (qemuBuildIOMMUCommandLine(cmd, def, qemuCaps) < 0) + goto error; + if (qemuBuildGlobalControllerCommandLine(cmd, def, qemuCaps) < 0) goto error;

diff --git a/tests/qemuxml2argvdata/qemuxml2argv-intel-iommu.args b/tests/qemuxml2argvdata/qemuxml2argv-intel-iommu.args new file mode 100644 index 0000000..69e4490 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-intel-iommu.args @@ -0,0 +1,22 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu \ +-name QEMUGuest1 \ +-S \ +-M q35 \ +-m 214 \ +-smp 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 intel-iommu \ +-device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x1e \ +-device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x0 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.2,addr=0x1 diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index a4b8bf4..59fa420 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -2023,6 +2023,8 @@ mymain(void) QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_USB_HUB);

DO_TEST("acpi-table", NONE); + DO_TEST("intel-iommu", QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, QEMU_CAPS_DEVICE_INTEL_IOMMU);

qemuTestDriverFree(&driver);