[PATCH] target/i386: Support up to 32768 CPUs without IRQ remapping

[PATCH] target/i386: Support up to 32768 CPUs without IRQ remapping

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 9059 bytes --]

The IOAPIC has an 'Extended Destination ID' field in its RTE, which maps
to bits 11-4 of the MSI address. Since those address bits fall within a
given 4KiB page they were historically non-trivial to use on real hardware.

The Intel IOMMU uses the lowest bit to indicate a remappable format MSI,
and then the remaining 7 bits are part of the index.

Where the remappable format bit isn't set, we can actually use the other
seven to allow external (IOAPIC and MSI) interrupts to reach up to 32768
CPUs instead of just the 255 permitted on bare metal.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 hw/i386/kvm/apic.c                          |  7 ++
 hw/i386/pc.c                                | 16 ++---
 include/standard-headers/asm-x86/kvm_para.h |  1 +
 target/i386/cpu.c                           |  5 +-
 target/i386/kvm.c                           | 74 +++++++++++++++------
 target/i386/kvm_i386.h                      |  2 +
 6 files changed, 75 insertions(+), 30 deletions(-)

diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c
index 4eb2d77b87..aeb3366ae8 100644
--- a/hw/i386/kvm/apic.c
+++ b/hw/i386/kvm/apic.c
@@ -183,6 +183,13 @@ static void kvm_send_msi(MSIMessage *msg)
 {
     int ret;
 
+    /*
+     * The message has already passed through interrupt remapping if enabled,
+     * but the legacy extended destination ID in low bits still needs to be
+     * handled.
+     */
+    msg->address = kvm_swizzle_msi_ext_dest_id(msg->address);
+
     ret = kvm_irqchip_send_msi(kvm_state, *msg);
     if (ret < 0) {
         fprintf(stderr, "KVM: injection failed, MSI lost (%s)\n",
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index e87be5d29a..a06c091227 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -99,6 +99,7 @@
 
 GlobalProperty pc_compat_5_1[] = {
     { "ICH9-LPC", "x-smi-cpu-hotplug", "off" },
+    { TYPE_X86_CPU, "kvm-msi-ext-dest-id", "off" },
 };
 const size_t pc_compat_5_1_len = G_N_ELEMENTS(pc_compat_5_1);
 
@@ -807,17 +808,12 @@ void pc_machine_done(Notifier *notifier, void *data)
         fw_cfg_modify_i16(x86ms->fw_cfg, FW_CFG_NB_CPUS, x86ms->boot_cpus);
     }
 
-    if (x86ms->apic_id_limit > 255 && !xen_enabled()) {
-        IntelIOMMUState *iommu = INTEL_IOMMU_DEVICE(x86_iommu_get_default());
 
-        if (!iommu || !x86_iommu_ir_supported(X86_IOMMU_DEVICE(iommu)) ||
-            iommu->intr_eim != ON_OFF_AUTO_ON) {
-            error_report("current -smp configuration requires "
-                         "Extended Interrupt Mode enabled. "
-                         "You can add an IOMMU using: "
-                         "-device intel-iommu,intremap=on,eim=on");
-            exit(EXIT_FAILURE);
-        }
+    if (x86ms->apic_id_limit > 255 && !xen_enabled() &&
+        !kvm_irqchip_in_kernel()) {
+        error_report("current -smp configuration requires kernel "
+                     "irqchip support.");
+        exit(EXIT_FAILURE);
     }
 }
 
diff --git a/include/standard-headers/asm-x86/kvm_para.h b/include/standard-headers/asm-x86/kvm_para.h
index 07877d3295..215d01b4ec 100644
--- a/include/standard-headers/asm-x86/kvm_para.h
+++ b/include/standard-headers/asm-x86/kvm_para.h
@@ -32,6 +32,7 @@
 #define KVM_FEATURE_POLL_CONTROL	12
 #define KVM_FEATURE_PV_SCHED_YIELD	13
 #define KVM_FEATURE_ASYNC_PF_INT	14
+#define KVM_FEATURE_MSI_EXT_DEST_ID	15
 
 #define KVM_HINTS_REALTIME      0
 
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index f37eb7b675..a93f50a6a7 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -799,7 +799,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
             "kvmclock", "kvm-nopiodelay", "kvm-mmu", "kvmclock",
             "kvm-asyncpf", "kvm-steal-time", "kvm-pv-eoi", "kvm-pv-unhalt",
             NULL, "kvm-pv-tlb-flush", NULL, "kvm-pv-ipi",
-            "kvm-poll-control", "kvm-pv-sched-yield", "kvm-asyncpf-int", NULL,
+            "kvm-poll-control", "kvm-pv-sched-yield", "kvm-asyncpf-int", "kvm-msi-ext-dest-id",
             NULL, NULL, NULL, NULL,
             NULL, NULL, NULL, NULL,
             "kvmclock-stable-bit", NULL, NULL, NULL,
@@ -4109,6 +4109,7 @@ static PropValue kvm_default_props[] = {
     { "kvm-pv-eoi", "on" },
     { "kvmclock-stable-bit", "on" },
     { "x2apic", "on" },
+    { "kvm-msi-ext-dest-id", "off" },
     { "acpi", "off" },
     { "monitor", "off" },
     { "svm", "off" },
@@ -5132,6 +5133,8 @@ static void x86_cpu_load_model(X86CPU *cpu, X86CPUModel *model)
     if (kvm_enabled()) {
         if (!kvm_irqchip_in_kernel()) {
             x86_cpu_change_kvm_default("x2apic", "off");
+        } else if (kvm_irqchip_is_split() && kvm_enable_x2apic()) {
+            x86_cpu_change_kvm_default("kvm-msi-ext-dest-id", "on");
         }
 
         x86_cpu_apply_props(cpu, kvm_default_props);
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index f6dae4cfb6..90952cae7c 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -420,6 +420,9 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function,
         if (!kvm_irqchip_in_kernel()) {
             ret &= ~(1U << KVM_FEATURE_PV_UNHALT);
         }
+        if (kvm_irqchip_is_split()) {
+            ret |= 1U << KVM_FEATURE_MSI_EXT_DEST_ID;
+        }
     } else if (function == KVM_CPUID_FEATURES && reg == R_EDX) {
         ret |= 1U << KVM_HINTS_REALTIME;
     }
@@ -4583,38 +4586,71 @@ int kvm_arch_irqchip_create(KVMState *s)
     }
 }
 
+uint64_t kvm_swizzle_msi_ext_dest_id(uint64_t address)
+{
+        CPUX86State *env = &X86_CPU(first_cpu)->env;
+        uint64_t ext_id;
+
+        if (!first_cpu ||
+            !(env->features[FEAT_KVM] & (1 << KVM_FEATURE_MSI_EXT_DEST_ID))) {
+            return address;
+        }
+
+        /*
+         * If the remappable format bit is set, or the upper bits are
+         * already set in address_hi, or the low extended bits aren't
+         * there anyway, do nothing.
+         */
+        ext_id = address & (0xff << MSI_ADDR_DEST_IDX_SHIFT);
+        if (!ext_id || (ext_id & (1 << MSI_ADDR_DEST_IDX_SHIFT)) ||
+            (address >> 32))
+            return address;
+
+        address &= ~ext_id;
+        address |= ext_id << 35;
+        return address;
+}
+
 int kvm_arch_fixup_msi_route(struct kvm_irq_routing_entry *route,
                              uint64_t address, uint32_t data, PCIDevice *dev)
 {
     X86IOMMUState *iommu = x86_iommu_get_default();
 
     if (iommu) {
-        int ret;
-        MSIMessage src, dst;
         X86IOMMUClass *class = X86_IOMMU_DEVICE_GET_CLASS(iommu);
 
-        if (!class->int_remap) {
-            return 0;
-        }
+        if (class->int_remap) {
+            int ret;
+            MSIMessage src, dst;
 
-        src.address = route->u.msi.address_hi;
-        src.address <<= VTD_MSI_ADDR_HI_SHIFT;
-        src.address |= route->u.msi.address_lo;
-        src.data = route->u.msi.data;
+            src.address = route->u.msi.address_hi;
+            src.address <<= VTD_MSI_ADDR_HI_SHIFT;
+            src.address |= route->u.msi.address_lo;
+            src.data = route->u.msi.data;
 
-        ret = class->int_remap(iommu, &src, &dst, dev ? \
-                               pci_requester_id(dev) : \
-                               X86_IOMMU_SID_INVALID);
-        if (ret) {
-            trace_kvm_x86_fixup_msi_error(route->gsi);
-            return 1;
-        }
+            ret = class->int_remap(iommu, &src, &dst, dev ?     \
+                                   pci_requester_id(dev) :      \
+                                   X86_IOMMU_SID_INVALID);
+            if (ret) {
+                trace_kvm_x86_fixup_msi_error(route->gsi);
+                return 1;
+            }
+
+            /*
+             * Handled untranslated compatibilty format interrupt with
+             * extended destination ID in the low bits 11-5. */
+            dst.address = kvm_swizzle_msi_ext_dest_id(dst.address);
 
-        route->u.msi.address_hi = dst.address >> VTD_MSI_ADDR_HI_SHIFT;
-        route->u.msi.address_lo = dst.address & VTD_MSI_ADDR_LO_MASK;
-        route->u.msi.data = dst.data;
+            route->u.msi.address_hi = dst.address >> VTD_MSI_ADDR_HI_SHIFT;
+            route->u.msi.address_lo = dst.address & VTD_MSI_ADDR_LO_MASK;
+            route->u.msi.data = dst.data;
+            return 0;
+        }
     }
 
+    address = kvm_swizzle_msi_ext_dest_id(address);
+    route->u.msi.address_hi = address >> VTD_MSI_ADDR_HI_SHIFT;
+    route->u.msi.address_lo = address & VTD_MSI_ADDR_LO_MASK;
     return 0;
 }
 
diff --git a/target/i386/kvm_i386.h b/target/i386/kvm_i386.h
index 0fce4e51d2..ede94760ae 100644
--- a/target/i386/kvm_i386.h
+++ b/target/i386/kvm_i386.h
@@ -49,4 +49,6 @@ bool kvm_has_waitpkg(void);
 
 bool kvm_hv_vpindex_settable(void);
 
+uint64_t kvm_swizzle_msi_ext_dest_id(uint64_t address);
+
 #endif


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]

Re: [PATCH] target/i386: Support up to 32768 CPUs without IRQ remapping

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 776 bytes --]

On Mon, 2020-10-05 at 15:18 +0100, David Woodhouse wrote:
> The IOAPIC has an 'Extended Destination ID' field in its RTE, which maps
> to bits 11-4 of the MSI address. Since those address bits fall within a
> given 4KiB page they were historically non-trivial to use on real hardware.
> 
> The Intel IOMMU uses the lowest bit to indicate a remappable format MSI,
> and then the remaining 7 bits are part of the index.
> 
> Where the remappable format bit isn't set, we can actually use the other
> seven to allow external (IOAPIC and MSI) interrupts to reach up to 32768
> CPUs instead of just the 255 permitted on bare metal.
> 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>

Corresponding kernel patch at 
https://patchwork.kernel.org/patch/11820535/

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]

Re: [PATCH] target/i386: Support up to 32768 CPUs without IRQ remapping

[Paolo Bonzini]

On 05/10/20 16:18, David Woodhouse wrote:
> +        if (kvm_irqchip_is_split()) {
> +            ret |= 1U << KVM_FEATURE_MSI_EXT_DEST_ID;
> +        }

IIUC this is because in-kernel IOAPIC still doesn't work; and when it
does, KVM will advertise the feature itself so no other QEMU changes
will be needed.

I queued this, though of course it has to wait for the corresponding
kernel patches to be accepted (or separated into doc and non-KVM parts;
we'll see).

Paolo

Re: [PATCH] target/i386: Support up to 32768 CPUs without IRQ remapping

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1107 bytes --]

On Thu, 2020-10-08 at 08:56 +0200, Paolo Bonzini wrote:
> On 05/10/20 16:18, David Woodhouse wrote:
> > +        if (kvm_irqchip_is_split()) {
> > +            ret |= 1U << KVM_FEATURE_MSI_EXT_DEST_ID;
> > +        }
> 
> IIUC this is because in-kernel IOAPIC still doesn't work; and when it
> does, KVM will advertise the feature itself so no other QEMU changes
> will be needed.

More the MSI handling than the IOAPIC. I haven't actually worked out
*what* handles cycles to addresses in the 0xFEExxxxx range for the in-
kernel irqchip and turns them into interrupts (after putting them
through interrupt remapping, if/when the kernel learns to do that).

Ideally the IOAPIC would just swizzle the bits in its RTE to create an
MSI message and pass it on to the same code to be (translated and)
delivered.

You'll note my qemu patch didn't touch IOAPIC code at all, because
qemu's IOAPIC really does just that.

> I queued this, though of course it has to wait for the corresponding
> kernel patches to be accepted (or separated into doc and non-KVM
> parts; we'll see).

Thanks.

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]

Re: [PATCH] target/i386: Support up to 32768 CPUs without IRQ remapping

[Paolo Bonzini]

On 08/10/20 09:29, David Woodhouse wrote:
> On Thu, 2020-10-08 at 08:56 +0200, Paolo Bonzini wrote:
>> On 05/10/20 16:18, David Woodhouse wrote:
>>> +        if (kvm_irqchip_is_split()) {
>>> +            ret |= 1U << KVM_FEATURE_MSI_EXT_DEST_ID;
>>> +        }
>>
>> IIUC this is because in-kernel IOAPIC still doesn't work; and when it
>> does, KVM will advertise the feature itself so no other QEMU changes
>> will be needed.
> 
> More the MSI handling than the IOAPIC. I haven't actually worked out
> *what* handles cycles to addresses in the 0xFEExxxxx range for the in-
> kernel irqchip and turns them into interrupts (after putting them
> through interrupt remapping, if/when the kernel learns to do that).

That's easy: it's QEMU. :)  See kvm_apic_mem_write in hw/i386/kvm/apic.c
(note that this memory region is never used when the CPU accesses
0xFEExxxxx, only when QEMU does.

Conversion from the IOAPIC and MSI formats to struct kvm_lapic_irq is
completely separate in KVM, it is respectively in ioapic_service and
kvm_set_msi_irq.  Both of them prepare a struct kvm_lapic_irq, but
they're two different paths.

> Ideally the IOAPIC would just swizzle the bits in its RTE to create an
> MSI message and pass it on to the same code to be (translated and)
> delivered.
> 
> You'll note my qemu patch didn't touch IOAPIC code at all, because
> qemu's IOAPIC really does just that.

Indeed the nice thing about irqchip=split is that the handling of device
interrupts is entirely confined within QEMU, no matter if they're IOAPIC
or MSI.  And because we had to implement interrupt remapping, the IOAPIC
is effectively using MSIs to deliver its interrupts.

There's still the hack to communicate IOAPIC routes to KVM and have it
set the EOI exit bitmap correctly, though.  The code is in
kvm_scan_ioapic_routes and it uses kvm_set_msi_irq (with irqchip=split
everything is also an MSI within the kernel).  I think you're not
handling that correctly for CPUs >255, so after all we _do_ need some
kernel support.

Paolo

>> I queued this, though of course it has to wait for the corresponding
>> kernel patches to be accepted (or separated into doc and non-KVM
>> parts; we'll see).
> 
> Thanks.
> 

Re: [PATCH] target/i386: Support up to 32768 CPUs without IRQ remapping

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 2123 bytes --]

On Thu, 2020-10-08 at 09:53 +0200, Paolo Bonzini wrote:
> Indeed the nice thing about irqchip=split is that the handling of device
> interrupts is entirely confined within QEMU, no matter if they're IOAPIC
> or MSI.  And because we had to implement interrupt remapping, the IOAPIC
> is effectively using MSIs to deliver its interrupts.
> 
> There's still the hack to communicate IOAPIC routes to KVM and have it
> set the EOI exit bitmap correctly, though.  The code is in
> kvm_scan_ioapic_routes and it uses kvm_set_msi_irq (with irqchip=split
> everything is also an MSI within the kernel).  I think you're not
> handling that correctly for CPUs >255, so after all we _do_ need some
> kernel support.

I think that works out OK.

In QEMU's ioapic_update_kvm_routes() it calls ioapic_entry_parse()
which generates the actual "bus" MSI with the extended dest ID in bits
11-5 of the address.

That MSI message is passed to kvm_irqchip_update_msi_route() which
passes it through translation —  which does interrupt remapping and
shifting the ext bits up into ->address_hi as the KVM X2APIC API
expects.

So when the kernel's kvm_scan_ioapic_routes() goes looking,
kvm_set_msi_irq() fills 'irq' in with the correct dest_id, and
kvm_apic_match_dest() does the right thing.

No?


As far as I can tell, we *do* have a QEMU bug — not related to the ext
dest ID — because for MSIs of assigned devices we don't update the KVM
IRQ routing table when the Interrupt Remapping IEC cache is flushed.

> Paolo
> 
> > > I queued this, though of course it has to wait for the corresponding
> > > kernel patches to be accepted (or separated into doc and non-KVM
> > > parts; we'll see).
> > 
> > Thanks.

So... it'll hit the tip.git tree and thus linux-next as soon as Linus
releases 5.10-rc1, and it'll then get merged into 5.11-rc1 and be in
the 5.11 release.

At which of those three points in time would you be happy to merge it
to QEMU? If it's either of the latter two, maybe it *is* worth doing a
patch which *only* reserves the feature bit, and trying to slip it into
5.10?

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]

Re: [PATCH] target/i386: Support up to 32768 CPUs without IRQ remapping

[Paolo Bonzini]

On 19/10/20 14:21, David Woodhouse wrote:
> On Thu, 2020-10-08 at 09:53 +0200, Paolo Bonzini wrote:
>> I think you're not
>> handling that correctly for CPUs >255, so after all we _do_ need some
>> kernel support.
> 
> I think that works out OK.
> 
> In QEMU's ioapic_update_kvm_routes() it calls ioapic_entry_parse()
> which generates the actual "bus" MSI with the extended dest ID in bits
> 11-5 of the address.
> 
> That MSI message is passed to kvm_irqchip_update_msi_route() which
> passes it through translation —  which does interrupt remapping and
> shifting the ext bits up into ->address_hi as the KVM X2APIC API
> expects.
> 
> So when the kernel's kvm_scan_ioapic_routes() goes looking,
> kvm_set_msi_irq() fills 'irq' in with the correct dest_id, and
> kvm_apic_match_dest() does the right thing.
> 
> No?

Yeah, that seems fine.

> As far as I can tell, we *do* have a QEMU bug — not related to the ext
> dest ID — because for MSIs of assigned devices we don't update the KVM
> IRQ routing table when the Interrupt Remapping IEC cache is flushed.

> So... it'll hit the tip.git tree and thus linux-next as soon as Linus
> releases 5.10-rc1, and it'll then get merged into 5.11-rc1 and be in
> the 5.11 release.
> 
> At which of those three points in time would you be happy to merge it
> to QEMU? If it's either of the latter two, maybe it *is* worth doing a
> patch which *only* reserves the feature bit, and trying to slip it into
> 5.10?

It would be 5.11-rc1 because of the KVM_FEATURE_MSI_EXT_DEST_ID
definition which would not be in your patch but rather synchronized from
the Linux tree by scripts/update-linux-headers.sh.

If you send me the doc patch any time before 5.10-rc7, it will be in 5.10.

Paolo

[PATCH] x86/kvm: Reserve KVM_FEATURE_MSI_EXT_DEST_ID

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1940 bytes --]

From: David Woodhouse <dwmw@amazon.co.uk>

No functional change; just reserve the feature bit for now so that VMMs
can start to implement it.

This will allow the host to indicate that MSI emulation supports 15-bit
destination IDs, allowing up to 32768 CPUs without interrupt remapping.

cf. https://patchwork.kernel.org/patch/11816693/ for qemu

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
---
 Documentation/virt/kvm/cpuid.rst     | 4 ++++
 arch/x86/include/uapi/asm/kvm_para.h | 1 +
 2 files changed, 5 insertions(+)

diff --git a/Documentation/virt/kvm/cpuid.rst b/Documentation/virt/kvm/cpuid.rst
index 9150e9d1c39b..f70b655821d5 100644
--- a/Documentation/virt/kvm/cpuid.rst
+++ b/Documentation/virt/kvm/cpuid.rst
@@ -92,6 +92,10 @@ KVM_FEATURE_ASYNC_PF_INT          14          guest checks this feature bit
                                               async pf acknowledgment msr
                                               0x4b564d07.
 
+KVM_FEATURE_MSI_EXT_DEST_ID       15          guest checks this feature bit
+                                              before using extended destination
+                                              ID bits in MSI address bits 11-5.
+
 KVM_FEATURE_CLOCSOURCE_STABLE_BIT 24          host will warn if no guest-side
                                               per-cpu warps are expeced in
                                               kvmclock
diff --git a/arch/x86/include/uapi/asm/kvm_para.h b/arch/x86/include/uapi/asm/kvm_para.h
index 812e9b4c1114..950afebfba88 100644
--- a/arch/x86/include/uapi/asm/kvm_para.h
+++ b/arch/x86/include/uapi/asm/kvm_para.h
@@ -32,6 +32,7 @@
 #define KVM_FEATURE_POLL_CONTROL	12
 #define KVM_FEATURE_PV_SCHED_YIELD	13
 #define KVM_FEATURE_ASYNC_PF_INT	14
+#define KVM_FEATURE_MSI_EXT_DEST_ID	15
 
 #define KVM_HINTS_REALTIME      0
 
-- 
2.17.1


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]