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

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

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

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