[PATCH] qemu: Enable I/O APIC even more frequently

Michal Privoznik posted 1 patch 3 days, 3 hours ago
src/qemu/qemu_postparse.c                     | 21 +++++++++---
...m-autoadd-v2.x86_64-latest.abi-update.args |  1 +
...im-autoadd-v2.x86_64-latest.abi-update.xml |  1 +
.../intel-iommu-eim-autoadd-v2.xml            | 32 +++++++++++++++++++
tests/qemuxmlconftest.c                       |  1 +
5 files changed, 51 insertions(+), 5 deletions(-)
create mode 120000 tests/qemuxmlconfdata/intel-iommu-eim-autoadd-v2.x86_64-latest.abi-update.args
create mode 120000 tests/qemuxmlconfdata/intel-iommu-eim-autoadd-v2.x86_64-latest.abi-update.xml
create mode 100644 tests/qemuxmlconfdata/intel-iommu-eim-autoadd-v2.xml
[PATCH] qemu: Enable I/O APIC even more frequently
Posted by Michal Privoznik 3 days, 3 hours ago
In my previous commit v10.10.0-48-g2d222ecf6e I've made us enable
I/O APIC when there is an IOMMU with EIM. This works well. What
does not work is case when there's just an IOMMU without EIM but
with 256+ vCPUS. Problem is that post parsing happens in two
stages: general domain post parse (where
qemuDomainDefEnableDefaultFeatures() is called) and then per
device post parse (where qemuDomainIOMMUDefPostParse() is
called). Now, in aforementioned case it is the device post parse
phase where EIM is enabled but the code that would enable
VIR_DOMAIN_FEATURE_IOAPIC has already run.

To resolve this, make the domain post parse callback "foresee"
the future enabling of EIM so that it can turn on I/O APIC
beforehand.

Resolves: https://issues.redhat.com/browse/RHEL-65844
Fixes: 2d222ecf6e73614a400b830ac56e9aaa1bc55ecc
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/qemu/qemu_postparse.c                     | 21 +++++++++---
 ...m-autoadd-v2.x86_64-latest.abi-update.args |  1 +
 ...im-autoadd-v2.x86_64-latest.abi-update.xml |  1 +
 .../intel-iommu-eim-autoadd-v2.xml            | 32 +++++++++++++++++++
 tests/qemuxmlconftest.c                       |  1 +
 5 files changed, 51 insertions(+), 5 deletions(-)
 create mode 120000 tests/qemuxmlconfdata/intel-iommu-eim-autoadd-v2.x86_64-latest.abi-update.args
 create mode 120000 tests/qemuxmlconfdata/intel-iommu-eim-autoadd-v2.x86_64-latest.abi-update.xml
 create mode 100644 tests/qemuxmlconfdata/intel-iommu-eim-autoadd-v2.xml

diff --git a/src/qemu/qemu_postparse.c b/src/qemu/qemu_postparse.c
index 4fbd849ebf..34b95ece1b 100644
--- a/src/qemu/qemu_postparse.c
+++ b/src/qemu/qemu_postparse.c
@@ -783,6 +783,15 @@ qemuDomainPstoreDefPostParse(virDomainPstoreDef *pstore,
 }
 
 
+static bool
+qemuDomainNeedsIOMMUWithEIM(const virDomainDef *def)
+{
+    return ARCH_IS_X86(def->os.arch) &&
+        virDomainDefGetVcpusMax(def) > QEMU_MAX_VCPUS_WITHOUT_EIM &&
+        qemuDomainIsQ35(def);
+}
+
+
 static int
 qemuDomainIOMMUDefPostParse(virDomainIOMMUDef *iommu,
                             const virDomainDef *def,
@@ -793,9 +802,7 @@ qemuDomainIOMMUDefPostParse(virDomainIOMMUDef *iommu,
      * (EIM) is not explicitly turned off, let's enable it. If we didn't then
      * guest will have troubles with interrupts. */
     if (parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE &&
-        ARCH_IS_X86(def->os.arch) &&
-        virDomainDefGetVcpusMax(def) > QEMU_MAX_VCPUS_WITHOUT_EIM &&
-        qemuDomainIsQ35(def) &&
+        qemuDomainNeedsIOMMUWithEIM(def) &&
         iommu && iommu->model == VIR_DOMAIN_IOMMU_MODEL_INTEL) {
 
         /* eim requires intremap. */
@@ -1548,9 +1555,13 @@ qemuDomainDefEnableDefaultFeatures(virDomainDef *def,
         def->features[VIR_DOMAIN_FEATURE_GIC] = VIR_TRISTATE_SWITCH_ON;
     }
 
-    /* IOMMU with intremap requires split I/O APIC */
+    /* IOMMU with intremap requires split I/O APIC. But it may happen that
+     * domain already has IOMMU without inremap. This will be fixed in
+     * qemuDomainIOMMUDefPostParse() but there domain definition can't be
+     * modified so change it now. */
     if (def->iommu &&
-        def->iommu->intremap == VIR_TRISTATE_SWITCH_ON &&
+        (def->iommu->intremap == VIR_TRISTATE_SWITCH_ON ||
+         qemuDomainNeedsIOMMUWithEIM(def)) &&
         def->features[VIR_DOMAIN_FEATURE_IOAPIC] == VIR_DOMAIN_IOAPIC_NONE) {
         def->features[VIR_DOMAIN_FEATURE_IOAPIC] = VIR_DOMAIN_IOAPIC_QEMU;
     }
diff --git a/tests/qemuxmlconfdata/intel-iommu-eim-autoadd-v2.x86_64-latest.abi-update.args b/tests/qemuxmlconfdata/intel-iommu-eim-autoadd-v2.x86_64-latest.abi-update.args
new file mode 120000
index 0000000000..a7fdee3d71
--- /dev/null
+++ b/tests/qemuxmlconfdata/intel-iommu-eim-autoadd-v2.x86_64-latest.abi-update.args
@@ -0,0 +1 @@
+intel-iommu-eim-autoadd.x86_64-latest.abi-update.args
\ No newline at end of file
diff --git a/tests/qemuxmlconfdata/intel-iommu-eim-autoadd-v2.x86_64-latest.abi-update.xml b/tests/qemuxmlconfdata/intel-iommu-eim-autoadd-v2.x86_64-latest.abi-update.xml
new file mode 120000
index 0000000000..928ea1b4c8
--- /dev/null
+++ b/tests/qemuxmlconfdata/intel-iommu-eim-autoadd-v2.x86_64-latest.abi-update.xml
@@ -0,0 +1 @@
+intel-iommu-eim-autoadd.x86_64-latest.abi-update.xml
\ No newline at end of file
diff --git a/tests/qemuxmlconfdata/intel-iommu-eim-autoadd-v2.xml b/tests/qemuxmlconfdata/intel-iommu-eim-autoadd-v2.xml
new file mode 100644
index 0000000000..b39ee55786
--- /dev/null
+++ b/tests/qemuxmlconfdata/intel-iommu-eim-autoadd-v2.xml
@@ -0,0 +1,32 @@
+<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'>288</vcpu>
+  <os>
+    <type arch='x86_64' machine='q35'>hvm</type>
+    <boot dev='hd'/>
+  </os>
+  <cpu mode='custom' match='exact' check='none'>
+    <model fallback='forbid'>qemu64</model>
+  </cpu>
+  <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='usb' index='0' model='none'/>
+    <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'/>
+    <iommu model="intel"/>
+    <audio id='1' type='none'/>
+    <watchdog model='itco' action='reset'/>
+    <memballoon model='none'/>
+  </devices>
+</domain>
diff --git a/tests/qemuxmlconftest.c b/tests/qemuxmlconftest.c
index 23f1d9eab7..21b56dc94e 100644
--- a/tests/qemuxmlconftest.c
+++ b/tests/qemuxmlconftest.c
@@ -2774,6 +2774,7 @@ mymain(void)
     DO_TEST_CAPS_LATEST("intel-iommu-dma-translation");
     DO_TEST_CAPS_LATEST_PARSE_ERROR("intel-iommu-wrong-machine");
     DO_TEST_CAPS_LATEST_ABI_UPDATE("intel-iommu-eim-autoadd");
+    DO_TEST_CAPS_LATEST_ABI_UPDATE("intel-iommu-eim-autoadd-v2");
     DO_TEST_CAPS_ARCH_LATEST("iommu-smmuv3", "aarch64");
     DO_TEST_CAPS_LATEST("virtio-iommu-x86_64");
     DO_TEST_CAPS_VER_PARSE_ERROR("virtio-iommu-x86_64", "6.1.0");
-- 
2.45.2
Re: [PATCH] qemu: Enable I/O APIC even more frequently
Posted by Jiri Denemark 3 days, 2 hours ago
On Wed, Dec 18, 2024 at 12:57:02 +0100, Michal Privoznik wrote:
> In my previous commit v10.10.0-48-g2d222ecf6e I've made us enable
> I/O APIC when there is an IOMMU with EIM. This works well. What
> does not work is case when there's just an IOMMU without EIM but
> with 256+ vCPUS. Problem is that post parsing happens in two
> stages: general domain post parse (where
> qemuDomainDefEnableDefaultFeatures() is called) and then per
> device post parse (where qemuDomainIOMMUDefPostParse() is
> called). Now, in aforementioned case it is the device post parse
> phase where EIM is enabled but the code that would enable
> VIR_DOMAIN_FEATURE_IOAPIC has already run.
> 
> To resolve this, make the domain post parse callback "foresee"
> the future enabling of EIM so that it can turn on I/O APIC
> beforehand.
> 
> Resolves: https://issues.redhat.com/browse/RHEL-65844
> Fixes: 2d222ecf6e73614a400b830ac56e9aaa1bc55ecc
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/qemu/qemu_postparse.c                     | 21 +++++++++---
>  ...m-autoadd-v2.x86_64-latest.abi-update.args |  1 +
>  ...im-autoadd-v2.x86_64-latest.abi-update.xml |  1 +
>  .../intel-iommu-eim-autoadd-v2.xml            | 32 +++++++++++++++++++
>  tests/qemuxmlconftest.c                       |  1 +
>  5 files changed, 51 insertions(+), 5 deletions(-)
>  create mode 120000 tests/qemuxmlconfdata/intel-iommu-eim-autoadd-v2.x86_64-latest.abi-update.args
>  create mode 120000 tests/qemuxmlconfdata/intel-iommu-eim-autoadd-v2.x86_64-latest.abi-update.xml
>  create mode 100644 tests/qemuxmlconfdata/intel-iommu-eim-autoadd-v2.xml
> 
> diff --git a/src/qemu/qemu_postparse.c b/src/qemu/qemu_postparse.c
> index 4fbd849ebf..34b95ece1b 100644
> --- a/src/qemu/qemu_postparse.c
> +++ b/src/qemu/qemu_postparse.c
> @@ -783,6 +783,15 @@ qemuDomainPstoreDefPostParse(virDomainPstoreDef *pstore,
>  }
>  
>  
> +static bool
> +qemuDomainNeedsIOMMUWithEIM(const virDomainDef *def)
> +{
> +    return ARCH_IS_X86(def->os.arch) &&
> +        virDomainDefGetVcpusMax(def) > QEMU_MAX_VCPUS_WITHOUT_EIM &&
> +        qemuDomainIsQ35(def);

Cosmetic: I would align these two lines with the ARCH_IS_X86 above.

> +}
> +
> +
>  static int
>  qemuDomainIOMMUDefPostParse(virDomainIOMMUDef *iommu,
>                              const virDomainDef *def,

Reviewed-by: Jiri Denemark <jdenemar@redhat.com>