[libvirt] [PATCH 0/4] conf: validate IOMMU interrupt remapping setting
*** BLURB HERE, THERE AND EVERYWHERE *** Ján Tomko (4): conf: use virXMLFormatElement for <iommu><driver> conf: use virXMLFormatElement for <iommu> tests: merge iommu tests conf: validate IOMMU interrupt remapping setting src/conf/domain_conf.c | 85 ++++++++++++++-------- .../qemuxml2argv-intel-iommu-caching-mode.args | 2 +- .../qemuxml2argv-intel-iommu-caching-mode.xml | 5 +- .../qemuxml2argv-intel-iommu-ioapic.args | 21 ------ .../qemuxml2argv-intel-iommu-ioapic.xml | 31 -------- tests/qemuxml2argvtest.c | 6 +- .../qemuxml2xmlout-intel-iommu-ioapic.xml | 1 - tests/qemuxml2xmltest.c | 1 - 8 files changed, 59 insertions(+), 93 deletions(-) delete mode 100644 tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-ioapic.args delete mode 100644 tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-ioapic.xml delete mode 120000 tests/qemuxml2xmloutdata/qemuxml2xmlout-intel-iommu-ioapic.xml -- 2.13.0
Use the new helper to simplify the code. This also fixes the bug of not formatting 'eim' in the useless case if it's the only enabled attribute. --- src/conf/domain_conf.c | 55 ++++++++++++++++++++++++++++---------------------- 1 file changed, 31 insertions(+), 24 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d97aab483..8e555ad9d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -25056,37 +25056,36 @@ virDomainDefIothreadShouldFormat(virDomainDefPtr def) } -static void +static int virDomainIOMMUDefFormat(virBufferPtr buf, const virDomainIOMMUDef *iommu) { virBuffer childBuf = VIR_BUFFER_INITIALIZER; + virBuffer driverAttrBuf = VIR_BUFFER_INITIALIZER; + int ret = -1; virBufferSetChildIndent(&childBuf, buf); - if (iommu->intremap != VIR_TRISTATE_SWITCH_ABSENT || - iommu->caching_mode != VIR_TRISTATE_SWITCH_ABSENT || - iommu->iotlb != VIR_TRISTATE_SWITCH_ABSENT) { - virBufferAddLit(&childBuf, "<driver"); - if (iommu->intremap != VIR_TRISTATE_SWITCH_ABSENT) { - virBufferAsprintf(&childBuf, " intremap='%s'", - virTristateSwitchTypeToString(iommu->intremap)); - } - if (iommu->caching_mode != VIR_TRISTATE_SWITCH_ABSENT) { - virBufferAsprintf(&childBuf, " caching_mode='%s'", - virTristateSwitchTypeToString(iommu->caching_mode)); - } - if (iommu->eim != VIR_TRISTATE_SWITCH_ABSENT) { - virBufferAsprintf(&childBuf, " eim='%s'", - virTristateSwitchTypeToString(iommu->eim)); - } - if (iommu->iotlb != VIR_TRISTATE_SWITCH_ABSENT) { - virBufferAsprintf(&childBuf, " iotlb='%s'", - virTristateSwitchTypeToString(iommu->iotlb)); - } - virBufferAddLit(&childBuf, "/>\n"); + if (iommu->intremap != VIR_TRISTATE_SWITCH_ABSENT) { + virBufferAsprintf(&driverAttrBuf, " intremap='%s'", + virTristateSwitchTypeToString(iommu->intremap)); + } + if (iommu->caching_mode != VIR_TRISTATE_SWITCH_ABSENT) { + virBufferAsprintf(&driverAttrBuf, " caching_mode='%s'", + virTristateSwitchTypeToString(iommu->caching_mode)); + } + if (iommu->eim != VIR_TRISTATE_SWITCH_ABSENT) { + virBufferAsprintf(&driverAttrBuf, " eim='%s'", + virTristateSwitchTypeToString(iommu->eim)); + } + if (iommu->iotlb != VIR_TRISTATE_SWITCH_ABSENT) { + virBufferAsprintf(&driverAttrBuf, " iotlb='%s'", + virTristateSwitchTypeToString(iommu->iotlb)); } + if (virXMLFormatElement(&childBuf, "driver", &driverAttrBuf, NULL) < 0) + goto cleanup; + virBufferAsprintf(buf, "<iommu model='%s'", virDomainIOMMUModelTypeToString(iommu->model)); @@ -25097,6 +25096,13 @@ virDomainIOMMUDefFormat(virBufferPtr buf, } else { virBufferAddLit(buf, "/>\n"); } + + ret = 0; + + cleanup: + virBufferFreeAndReset(&childBuf); + virBufferFreeAndReset(&driverAttrBuf); + return ret; } @@ -25866,8 +25872,9 @@ virDomainDefFormatInternal(virDomainDefPtr def, goto error; } - if (def->iommu) - virDomainIOMMUDefFormat(buf, def->iommu); + if (def->iommu && + virDomainIOMMUDefFormat(buf, def->iommu) < 0) + goto error; virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</devices>\n"); -- 2.13.0
On 08/29/2017 12:48 PM, Ján Tomko wrote:
Use the new helper to simplify the code. This also fixes the bug of not formatting 'eim' in the useless case if it's the only enabled attribute. --- src/conf/domain_conf.c | 55 ++++++++++++++++++++++++++++---------------------- 1 file changed, 31 insertions(+), 24 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John If I didn't read patch4, I might have said in a here that perhaps tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-eim.xml should be modified to remove "intremap='on' " - good thing I read ahead! ;-)
Simplify the formatting function even further. --- src/conf/domain_conf.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 8e555ad9d..a26731e75 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -25061,6 +25061,7 @@ virDomainIOMMUDefFormat(virBufferPtr buf, const virDomainIOMMUDef *iommu) { virBuffer childBuf = VIR_BUFFER_INITIALIZER; + virBuffer attrBuf = VIR_BUFFER_INITIALIZER; virBuffer driverAttrBuf = VIR_BUFFER_INITIALIZER; int ret = -1; @@ -25086,16 +25087,11 @@ virDomainIOMMUDefFormat(virBufferPtr buf, if (virXMLFormatElement(&childBuf, "driver", &driverAttrBuf, NULL) < 0) goto cleanup; - virBufferAsprintf(buf, "<iommu model='%s'", + virBufferAsprintf(&attrBuf, " model='%s'", virDomainIOMMUModelTypeToString(iommu->model)); - if (virBufferError(&childBuf) != 0 || virBufferUse(&childBuf)) { - virBufferAddLit(buf, ">\n"); - virBufferAddBuffer(buf, &childBuf); - virBufferAddLit(buf, "</iommu>\n"); - } else { - virBufferAddLit(buf, "/>\n"); - } + if (virXMLFormatElement(buf, "iommu", &attrBuf, &childBuf) < 0) + goto cleanup; ret = 0; -- 2.13.0
On 08/29/2017 12:48 PM, Ján Tomko wrote:
Simplify the formatting function even further. --- src/conf/domain_conf.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
Using intremap without <ioapic driver='qemu'/> does not work. Merge the tests to avoid a duplicit test once we start validating it. --- .../qemuxml2argv-intel-iommu-caching-mode.args | 2 +- .../qemuxml2argv-intel-iommu-caching-mode.xml | 5 +++- .../qemuxml2argv-intel-iommu-ioapic.args | 21 --------------- .../qemuxml2argv-intel-iommu-ioapic.xml | 31 ---------------------- tests/qemuxml2argvtest.c | 6 +---- .../qemuxml2xmlout-intel-iommu-ioapic.xml | 1 - tests/qemuxml2xmltest.c | 1 - 7 files changed, 6 insertions(+), 61 deletions(-) delete mode 100644 tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-ioapic.args delete mode 100644 tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-ioapic.xml delete mode 120000 tests/qemuxml2xmloutdata/qemuxml2xmlout-intel-iommu-ioapic.xml diff --git a/tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-caching-mode.args b/tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-caching-mode.args index 5d12aabf4..81feecfcf 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-caching-mode.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-caching-mode.args @@ -7,7 +7,7 @@ QEMU_AUDIO_DRV=none \ /usr/bin/qemu-system-x86_64 \ -name QEMUGuest1 \ -S \ --machine q35,accel=tcg \ +-machine q35,accel=kvm,kernel_irqchip=split \ -m 214 \ -smp 1,sockets=1,cores=1,threads=1 \ -uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-caching-mode.xml b/tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-caching-mode.xml index 5f3384da7..36a392403 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-caching-mode.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-caching-mode.xml @@ -1,4 +1,4 @@ -<domain type='qemu'> +<domain type='kvm'> <name>QEMUGuest1</name> <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> <memory unit='KiB'>219100</memory> @@ -8,6 +8,9 @@ <type arch='x86_64' machine='q35'>hvm</type> <boot dev='hd'/> </os> + <features> + <ioapic driver='qemu'/> + </features> <clock offset='utc'/> <on_poweroff>destroy</on_poweroff> <on_reboot>restart</on_reboot> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-ioapic.args b/tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-ioapic.args deleted file mode 100644 index 8e70bf910..000000000 --- a/tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-ioapic.args +++ /dev/null @@ -1,21 +0,0 @@ -LC_ALL=C \ -PATH=/bin \ -HOME=/home/test \ -USER=test \ -LOGNAME=test \ -QEMU_AUDIO_DRV=none \ -/usr/bin/qemu-system-x86_64 \ --name QEMUGuest1 \ --S \ --machine q35,accel=kvm,kernel_irqchip=split \ --m 214 \ --smp 1,sockets=1,cores=1,threads=1 \ --uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ --nographic \ --nodefaults \ --chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\ -server,nowait \ --mon chardev=charmonitor,id=monitor,mode=readline \ --no-acpi \ --boot c \ --device intel-iommu,intremap=on diff --git a/tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-ioapic.xml b/tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-ioapic.xml deleted file mode 100644 index bfe714ad8..000000000 --- a/tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-ioapic.xml +++ /dev/null @@ -1,31 +0,0 @@ -<domain type='kvm'> - <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> - <features> - <ioapic driver='qemu'/> - </features> - <clock offset='utc'/> - <on_poweroff>destroy</on_poweroff> - <on_reboot>restart</on_reboot> - <on_crash>destroy</on_crash> - <devices> - <emulator>/usr/bin/qemu-system-x86_64</emulator> - <controller type='pci' index='0' model='pcie-root'/> - <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='none'/> - <iommu model='intel'> - <driver intremap='on'/> - </iommu> - </devices> -</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 0c0bd16f9..3a0080297 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -2699,14 +2699,10 @@ mymain(void) DO_TEST("intel-iommu-machine", QEMU_CAPS_MACHINE_OPT, QEMU_CAPS_MACHINE_IOMMU); - DO_TEST("intel-iommu-ioapic", + DO_TEST("intel-iommu-caching-mode", QEMU_CAPS_MACHINE_OPT, QEMU_CAPS_MACHINE_KERNEL_IRQCHIP, QEMU_CAPS_MACHINE_KERNEL_IRQCHIP_SPLIT, - QEMU_CAPS_INTEL_IOMMU_INTREMAP, - QEMU_CAPS_DEVICE_INTEL_IOMMU); - DO_TEST("intel-iommu-caching-mode", - QEMU_CAPS_MACHINE_OPT, QEMU_CAPS_DEVICE_PCI_BRIDGE, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, QEMU_CAPS_DEVICE_IOH3420, diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-intel-iommu-ioapic.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-intel-iommu-ioapic.xml deleted file mode 120000 index 42d17b2c0..000000000 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-intel-iommu-ioapic.xml +++ /dev/null @@ -1 +0,0 @@ -../qemuxml2argvdata/qemuxml2argv-intel-iommu-ioapic.xml \ No newline at end of file diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 311b71356..de8a781c1 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -1187,7 +1187,6 @@ mymain(void) DO_TEST("intel-iommu-machine", QEMU_CAPS_MACHINE_OPT, QEMU_CAPS_MACHINE_IOMMU); - DO_TEST("intel-iommu-ioapic", NONE); DO_TEST("intel-iommu-caching-mode", NONE); DO_TEST("intel-iommu-eim", NONE); DO_TEST("intel-iommu-device-iotlb", NONE); -- 2.13.0
On 08/29/2017 12:48 PM, Ján Tomko wrote:
Using intremap without <ioapic driver='qemu'/> does not work. Merge the tests to avoid a duplicit test once we start validating it. --- .../qemuxml2argv-intel-iommu-caching-mode.args | 2 +- .../qemuxml2argv-intel-iommu-caching-mode.xml | 5 +++- .../qemuxml2argv-intel-iommu-ioapic.args | 21 --------------- .../qemuxml2argv-intel-iommu-ioapic.xml | 31 ---------------------- tests/qemuxml2argvtest.c | 6 +---- .../qemuxml2xmlout-intel-iommu-ioapic.xml | 1 - tests/qemuxml2xmltest.c | 1 - 7 files changed, 6 insertions(+), 61 deletions(-) delete mode 100644 tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-ioapic.args delete mode 100644 tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-ioapic.xml delete mode 120000 tests/qemuxml2xmloutdata/qemuxml2xmlout-intel-iommu-ioapic.xml
Reviewed-by: John Ferlan <jferlan@redhat.com> John
This option requires: <ioapic driver='qemu'/> Report an error in case someone tries to combine it with different ioapic setting. Setting 'eim' on without enabling 'intremap' does not make sense. https://bugzilla.redhat.com/show_bug.cgi?id=1457610 --- src/conf/domain_conf.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a26731e75..fd236f5d2 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5551,6 +5551,24 @@ virDomainDefValidateInternal(const virDomainDef *def) if (virDomainDefGetVcpusTopology(def, NULL) < 0) return -1; + if (def->iommu && + def->iommu->intremap == VIR_TRISTATE_SWITCH_ON && + (def->features[VIR_DOMAIN_FEATURE_IOAPIC] != VIR_TRISTATE_SWITCH_ON || + def->ioapic != VIR_DOMAIN_IOAPIC_QEMU)) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("IOMMU interrupt remapping requires split I/O APIC " + "(ioapic driver='qemu')")); + return -1; + } + + if (def->iommu && + def->iommu->eim == VIR_TRISTATE_SWITCH_ON && + def->iommu->intremap != VIR_TRISTATE_SWITCH_ON) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("IOMMU eim requires interrupt remapping to be enabled")); + return -1; + } + return 0; } -- 2.13.0
On 08/29/2017 12:48 PM, Ján Tomko wrote:
This option requires: <ioapic driver='qemu'/>
Just an extra empty line here would make it easier to read...
Report an error in case someone tries to combine it with different ioapic setting.
Setting 'eim' on without enabling 'intremap' does not make sense.
https://bugzilla.redhat.com/show_bug.cgi?id=1457610 --- src/conf/domain_conf.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
participants (2)
-
John Ferlan -
Ján Tomko