[PATCH v2 1/4] target/i386: Fix sanity check on max APIC ID / X2APIC enablement

[PATCH v2 1/4] target/i386: Fix sanity check on max APIC ID / X2APIC enablement

[David Woodhouse]

The check on x86ms->apic_id_limit in pc_machine_done() had two problems.

Firstly, we need KVM to support the X2APIC API in order to allow IRQ
delivery to APICs >= 255. So we need to call/check kvm_enable_x2apic(),
which was done elsewhere in *some* cases but not all.

Secondly, microvm needs the same check. So move it from pc_machine_done()
to x86_cpus_init() where it will work for both.

The check in kvm_cpu_instance_init() is now redundant and can be dropped.

Signed-off-by: David Woodhouse <dwmw2@infradead.org>
Acked-by: Claudio Fontana <cfontana@suse.de>
---
 hw/i386/pc.c              |  8 --------
 hw/i386/x86.c             | 16 ++++++++++++++++
 target/i386/kvm/kvm-cpu.c |  2 +-
 3 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index a2ef40ecbc..9959f93216 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -736,14 +736,6 @@ void pc_machine_done(Notifier *notifier, void *data)
         /* update FW_CFG_NB_CPUS to account for -device added CPUs */
         fw_cfg_modify_i16(x86ms->fw_cfg, FW_CFG_NB_CPUS, x86ms->boot_cpus);
     }
-
-
-    if (x86ms->apic_id_limit > 255 && !xen_enabled() &&
-        !kvm_irqchip_in_kernel()) {
-        error_report("current -smp configuration requires kernel "
-                     "irqchip support.");
-        exit(EXIT_FAILURE);
-    }
 }
 
 void pc_guest_info_init(PCMachineState *pcms)
diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index b84840a1bb..f64639b873 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -39,6 +39,7 @@
 #include "sysemu/replay.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/cpu-timers.h"
+#include "sysemu/xen.h"
 #include "trace.h"
 
 #include "hw/i386/x86.h"
@@ -136,6 +137,21 @@ void x86_cpus_init(X86MachineState *x86ms, int default_cpu_version)
      */
     x86ms->apic_id_limit = x86_cpu_apic_id_from_index(x86ms,
                                                       ms->smp.max_cpus - 1) + 1;
+
+    /*
+     * Can we support APIC ID 255 or higher?
+     *
+     * Under Xen: yes.
+     * With userspace emulated lapic: no
+     * With KVM's in-kernel lapic: only if X2APIC API is enabled.
+     */
+    if (x86ms->apic_id_limit > 255 && !xen_enabled() &&
+        (!kvm_irqchip_in_kernel() || !kvm_enable_x2apic())) {
+        error_report("current -smp configuration requires kernel "
+                     "irqchip and X2APIC API support.");
+        exit(EXIT_FAILURE);
+    }
+
     possible_cpus = mc->possible_cpu_arch_ids(ms);
     for (i = 0; i < ms->smp.cpus; i++) {
         x86_cpu_new(x86ms, possible_cpus->cpus[i].arch_id, &error_fatal);
diff --git a/target/i386/kvm/kvm-cpu.c b/target/i386/kvm/kvm-cpu.c
index d95028018e..c60cb2dafb 100644
--- a/target/i386/kvm/kvm-cpu.c
+++ b/target/i386/kvm/kvm-cpu.c
@@ -165,7 +165,7 @@ static void kvm_cpu_instance_init(CPUState *cs)
         /* only applies to builtin_x86_defs cpus */
         if (!kvm_irqchip_in_kernel()) {
             x86_cpu_change_kvm_default("x2apic", "off");
-        } else if (kvm_irqchip_is_split() && kvm_enable_x2apic()) {
+        } else if (kvm_irqchip_is_split()) {
             x86_cpu_change_kvm_default("kvm-msi-ext-dest-id", "on");
         }
 
-- 
2.31.1

[PATCH v2 1/4] target/i386: Fix sanity check on max APIC ID / X2APIC enablement

[David Woodhouse]

The check on x86ms->apic_id_limit in pc_machine_done() had two problems.

Firstly, we need KVM to support the X2APIC API in order to allow IRQ
delivery to APICs >= 255. So we need to call/check kvm_enable_x2apic(),
which was done elsewhere in *some* cases but not all.

Secondly, microvm needs the same check. So move it from pc_machine_done()
to x86_cpus_init() where it will work for both.

The check in kvm_cpu_instance_init() is now redundant and can be dropped.

Signed-off-by: David Woodhouse <dwmw2@infradead.org>
Acked-by: Claudio Fontana <cfontana@suse.de>
---
 hw/i386/pc.c              |  8 --------
 hw/i386/x86.c             | 16 ++++++++++++++++
 target/i386/kvm/kvm-cpu.c |  2 +-
 3 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index a2ef40ecbc..9959f93216 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -736,14 +736,6 @@ void pc_machine_done(Notifier *notifier, void *data)
         /* update FW_CFG_NB_CPUS to account for -device added CPUs */
         fw_cfg_modify_i16(x86ms->fw_cfg, FW_CFG_NB_CPUS, x86ms->boot_cpus);
     }
-
-
-    if (x86ms->apic_id_limit > 255 && !xen_enabled() &&
-        !kvm_irqchip_in_kernel()) {
-        error_report("current -smp configuration requires kernel "
-                     "irqchip support.");
-        exit(EXIT_FAILURE);
-    }
 }
 
 void pc_guest_info_init(PCMachineState *pcms)
diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index b84840a1bb..f64639b873 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -39,6 +39,7 @@
 #include "sysemu/replay.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/cpu-timers.h"
+#include "sysemu/xen.h"
 #include "trace.h"
 
 #include "hw/i386/x86.h"
@@ -136,6 +137,21 @@ void x86_cpus_init(X86MachineState *x86ms, int default_cpu_version)
      */
     x86ms->apic_id_limit = x86_cpu_apic_id_from_index(x86ms,
                                                       ms->smp.max_cpus - 1) + 1;
+
+    /*
+     * Can we support APIC ID 255 or higher?
+     *
+     * Under Xen: yes.
+     * With userspace emulated lapic: no
+     * With KVM's in-kernel lapic: only if X2APIC API is enabled.
+     */
+    if (x86ms->apic_id_limit > 255 && !xen_enabled() &&
+        (!kvm_irqchip_in_kernel() || !kvm_enable_x2apic())) {
+        error_report("current -smp configuration requires kernel "
+                     "irqchip and X2APIC API support.");
+        exit(EXIT_FAILURE);
+    }
+
     possible_cpus = mc->possible_cpu_arch_ids(ms);
     for (i = 0; i < ms->smp.cpus; i++) {
         x86_cpu_new(x86ms, possible_cpus->cpus[i].arch_id, &error_fatal);
diff --git a/target/i386/kvm/kvm-cpu.c b/target/i386/kvm/kvm-cpu.c
index d95028018e..c60cb2dafb 100644
--- a/target/i386/kvm/kvm-cpu.c
+++ b/target/i386/kvm/kvm-cpu.c
@@ -165,7 +165,7 @@ static void kvm_cpu_instance_init(CPUState *cs)
         /* only applies to builtin_x86_defs cpus */
         if (!kvm_irqchip_in_kernel()) {
             x86_cpu_change_kvm_default("x2apic", "off");
-        } else if (kvm_irqchip_is_split() && kvm_enable_x2apic()) {
+        } else if (kvm_irqchip_is_split()) {
             x86_cpu_change_kvm_default("kvm-msi-ext-dest-id", "on");
         }
 
-- 
2.31.1

[PATCH v2 2/4] intel_iommu: Support IR-only mode without DMA translation

[David Woodhouse]

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

By setting none of the SAGAW bits we can indicate to a guest that DMA
translation isn't supported. Tested by booting Windows 10, as well as
Linux guests with the fix at https://git.kernel.org/torvalds/c/c40aaaac10

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Acked-by: Claudio Fontana <cfontana@suse.de>
---
 hw/i386/intel_iommu.c         | 14 ++++++++++----
 include/hw/i386/intel_iommu.h |  1 +
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index f584449d8d..9a3cb2b789 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -2202,7 +2202,7 @@ static void vtd_handle_gcmd_write(IntelIOMMUState *s)
     uint32_t changed = status ^ val;
 
     trace_vtd_reg_write_gcmd(status, val);
-    if (changed & VTD_GCMD_TE) {
+    if ((changed & VTD_GCMD_TE) && s->dma_translation) {
         /* Translation enable/disable */
         vtd_handle_gcmd_te(s, val & VTD_GCMD_TE);
     }
@@ -3100,6 +3100,7 @@ static Property vtd_properties[] = {
     DEFINE_PROP_BOOL("caching-mode", IntelIOMMUState, caching_mode, FALSE),
     DEFINE_PROP_BOOL("x-scalable-mode", IntelIOMMUState, scalable_mode, FALSE),
     DEFINE_PROP_BOOL("dma-drain", IntelIOMMUState, dma_drain, true),
+    DEFINE_PROP_BOOL("dma-translation", IntelIOMMUState, dma_translation, true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -3605,12 +3606,17 @@ static void vtd_init(IntelIOMMUState *s)
     s->next_frcd_reg = 0;
     s->cap = VTD_CAP_FRO | VTD_CAP_NFR | VTD_CAP_ND |
              VTD_CAP_MAMV | VTD_CAP_PSI | VTD_CAP_SLLPS |
-             VTD_CAP_SAGAW_39bit | VTD_CAP_MGAW(s->aw_bits);
+             VTD_CAP_MGAW(s->aw_bits);
     if (s->dma_drain) {
         s->cap |= VTD_CAP_DRAIN;
     }
-    if (s->aw_bits == VTD_HOST_AW_48BIT) {
-        s->cap |= VTD_CAP_SAGAW_48bit;
+    if (s->dma_translation) {
+            if (s->aw_bits >= VTD_HOST_AW_39BIT) {
+                    s->cap |= VTD_CAP_SAGAW_39bit;
+            }
+            if (s->aw_bits >= VTD_HOST_AW_48BIT) {
+                    s->cap |= VTD_CAP_SAGAW_48bit;
+            }
     }
     s->ecap = VTD_ECAP_QI | VTD_ECAP_IRO;
 
diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
index 41783ee46d..42d6a6a636 100644
--- a/include/hw/i386/intel_iommu.h
+++ b/include/hw/i386/intel_iommu.h
@@ -266,6 +266,7 @@ struct IntelIOMMUState {
     bool buggy_eim;                 /* Force buggy EIM unless eim=off */
     uint8_t aw_bits;                /* Host/IOVA address width (in bits) */
     bool dma_drain;                 /* Whether DMA r/w draining enabled */
+    bool dma_translation;           /* Whether DMA translation supported */
 
     /*
      * Protects IOMMU states in general.  Currently it protects the
-- 
2.31.1

[PATCH v2 2/4] intel_iommu: Support IR-only mode without DMA translation

[David Woodhouse]

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

By setting none of the SAGAW bits we can indicate to a guest that DMA
translation isn't supported. Tested by booting Windows 10, as well as
Linux guests with the fix at https://git.kernel.org/torvalds/c/c40aaaac10

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Acked-by: Claudio Fontana <cfontana@suse.de>
---
 hw/i386/intel_iommu.c         | 14 ++++++++++----
 include/hw/i386/intel_iommu.h |  1 +
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index f584449d8d..9a3cb2b789 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -2202,7 +2202,7 @@ static void vtd_handle_gcmd_write(IntelIOMMUState *s)
     uint32_t changed = status ^ val;
 
     trace_vtd_reg_write_gcmd(status, val);
-    if (changed & VTD_GCMD_TE) {
+    if ((changed & VTD_GCMD_TE) && s->dma_translation) {
         /* Translation enable/disable */
         vtd_handle_gcmd_te(s, val & VTD_GCMD_TE);
     }
@@ -3100,6 +3100,7 @@ static Property vtd_properties[] = {
     DEFINE_PROP_BOOL("caching-mode", IntelIOMMUState, caching_mode, FALSE),
     DEFINE_PROP_BOOL("x-scalable-mode", IntelIOMMUState, scalable_mode, FALSE),
     DEFINE_PROP_BOOL("dma-drain", IntelIOMMUState, dma_drain, true),
+    DEFINE_PROP_BOOL("dma-translation", IntelIOMMUState, dma_translation, true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -3605,12 +3606,17 @@ static void vtd_init(IntelIOMMUState *s)
     s->next_frcd_reg = 0;
     s->cap = VTD_CAP_FRO | VTD_CAP_NFR | VTD_CAP_ND |
              VTD_CAP_MAMV | VTD_CAP_PSI | VTD_CAP_SLLPS |
-             VTD_CAP_SAGAW_39bit | VTD_CAP_MGAW(s->aw_bits);
+             VTD_CAP_MGAW(s->aw_bits);
     if (s->dma_drain) {
         s->cap |= VTD_CAP_DRAIN;
     }
-    if (s->aw_bits == VTD_HOST_AW_48BIT) {
-        s->cap |= VTD_CAP_SAGAW_48bit;
+    if (s->dma_translation) {
+            if (s->aw_bits >= VTD_HOST_AW_39BIT) {
+                    s->cap |= VTD_CAP_SAGAW_39bit;
+            }
+            if (s->aw_bits >= VTD_HOST_AW_48BIT) {
+                    s->cap |= VTD_CAP_SAGAW_48bit;
+            }
     }
     s->ecap = VTD_ECAP_QI | VTD_ECAP_IRO;
 
diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
index 41783ee46d..42d6a6a636 100644
--- a/include/hw/i386/intel_iommu.h
+++ b/include/hw/i386/intel_iommu.h
@@ -266,6 +266,7 @@ struct IntelIOMMUState {
     bool buggy_eim;                 /* Force buggy EIM unless eim=off */
     uint8_t aw_bits;                /* Host/IOVA address width (in bits) */
     bool dma_drain;                 /* Whether DMA r/w draining enabled */
+    bool dma_translation;           /* Whether DMA translation supported */
 
     /*
      * Protects IOMMU states in general.  Currently it protects the
-- 
2.31.1

[PATCH v2 3/4] intel_iommu: Only allow interrupt remapping to be enabled if it's supported

[David Woodhouse]

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

We should probably check if we were meant to be exposing IR, before
letting the guest turn the IRE bit on.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Reviewed-by: Peter Xu <peterx@redhat.com>
Acked-by: Jason Wang <jasowang@redhat.com>
---
 hw/i386/intel_iommu.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 9a3cb2b789..bd288d45bb 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -2197,6 +2197,7 @@ static void vtd_handle_gcmd_ire(IntelIOMMUState *s, bool en)
 /* Handle write to Global Command Register */
 static void vtd_handle_gcmd_write(IntelIOMMUState *s)
 {
+    X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s);
     uint32_t status = vtd_get_long_raw(s, DMAR_GSTS_REG);
     uint32_t val = vtd_get_long_raw(s, DMAR_GCMD_REG);
     uint32_t changed = status ^ val;
@@ -2218,7 +2219,8 @@ static void vtd_handle_gcmd_write(IntelIOMMUState *s)
         /* Set/update the interrupt remapping root-table pointer */
         vtd_handle_gcmd_sirtp(s);
     }
-    if (changed & VTD_GCMD_IRE) {
+    if ((changed & VTD_GCMD_IRE) &&
+        x86_iommu_ir_supported(x86_iommu)) {
         /* Interrupt remap enable/disable */
         vtd_handle_gcmd_ire(s, val & VTD_GCMD_IRE);
     }
-- 
2.31.1

[PATCH v2 3/4] intel_iommu: Only allow interrupt remapping to be enabled if it's supported

[David Woodhouse]

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

We should probably check if we were meant to be exposing IR, before
letting the guest turn the IRE bit on.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Reviewed-by: Peter Xu <peterx@redhat.com>
Acked-by: Jason Wang <jasowang@redhat.com>
---
 hw/i386/intel_iommu.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 9a3cb2b789..bd288d45bb 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -2197,6 +2197,7 @@ static void vtd_handle_gcmd_ire(IntelIOMMUState *s, bool en)
 /* Handle write to Global Command Register */
 static void vtd_handle_gcmd_write(IntelIOMMUState *s)
 {
+    X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s);
     uint32_t status = vtd_get_long_raw(s, DMAR_GSTS_REG);
     uint32_t val = vtd_get_long_raw(s, DMAR_GCMD_REG);
     uint32_t changed = status ^ val;
@@ -2218,7 +2219,8 @@ static void vtd_handle_gcmd_write(IntelIOMMUState *s)
         /* Set/update the interrupt remapping root-table pointer */
         vtd_handle_gcmd_sirtp(s);
     }
-    if (changed & VTD_GCMD_IRE) {
+    if ((changed & VTD_GCMD_IRE) &&
+        x86_iommu_ir_supported(x86_iommu)) {
         /* Interrupt remap enable/disable */
         vtd_handle_gcmd_ire(s, val & VTD_GCMD_IRE);
     }
-- 
2.31.1

[PATCH v2 4/4] intel_iommu: Fix irqchip / X2APIC configuration checks

[David Woodhouse]

We don't need to check kvm_enable_x2apic(). It's perfectly OK to support
interrupt remapping even if we can't address CPUs above 254. Kind of
pointless, but still functional.

The check on kvm_enable_x2apic() needs to happen *anyway* in order to
allow CPUs above 254 even without an IOMMU, so allow that to happen
elsewhere.

However, we do require the *split* irqchip in order to rewrite I/OAPIC
destinations. So fix that check while we're here.

Signed-off-by: David Woodhouse <dwmw2@infradead.org>
Reviewed-by: Peter Xu <peterx@redhat.com>
Acked-by: Jason Wang <jasowang@redhat.com>
---
 hw/i386/intel_iommu.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index bd288d45bb..0d1c72f08e 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -3760,15 +3760,10 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error **errp)
                                               ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
     }
     if (s->intr_eim == ON_OFF_AUTO_ON && !s->buggy_eim) {
-        if (!kvm_irqchip_in_kernel()) {
+        if (!kvm_irqchip_is_split()) {
             error_setg(errp, "eim=on requires accel=kvm,kernel-irqchip=split");
             return false;
         }
-        if (!kvm_enable_x2apic()) {
-            error_setg(errp, "eim=on requires support on the KVM side"
-                             "(X2APIC_API, first shipped in v4.7)");
-            return false;
-        }
     }
 
     /* Currently only address widths supported are 39 and 48 bits */
-- 
2.31.1

[PATCH v2 4/4] intel_iommu: Fix irqchip / X2APIC configuration checks

[David Woodhouse]

We don't need to check kvm_enable_x2apic(). It's perfectly OK to support
interrupt remapping even if we can't address CPUs above 254. Kind of
pointless, but still functional.

The check on kvm_enable_x2apic() needs to happen *anyway* in order to
allow CPUs above 254 even without an IOMMU, so allow that to happen
elsewhere.

However, we do require the *split* irqchip in order to rewrite I/OAPIC
destinations. So fix that check while we're here.

Signed-off-by: David Woodhouse <dwmw2@infradead.org>
Reviewed-by: Peter Xu <peterx@redhat.com>
Acked-by: Jason Wang <jasowang@redhat.com>
---
 hw/i386/intel_iommu.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index bd288d45bb..0d1c72f08e 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -3760,15 +3760,10 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error **errp)
                                               ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
     }
     if (s->intr_eim == ON_OFF_AUTO_ON && !s->buggy_eim) {
-        if (!kvm_irqchip_in_kernel()) {
+        if (!kvm_irqchip_is_split()) {
             error_setg(errp, "eim=on requires accel=kvm,kernel-irqchip=split");
             return false;
         }
-        if (!kvm_enable_x2apic()) {
-            error_setg(errp, "eim=on requires support on the KVM side"
-                             "(X2APIC_API, first shipped in v4.7)");
-            return false;
-        }
     }
 
     /* Currently only address widths supported are 39 and 48 bits */
-- 
2.31.1

Re: [PATCH v2 1/4] target/i386: Fix sanity check on max APIC ID / X2APIC enablement

[Peter Xu]

On Thu, Dec 09, 2021 at 10:08:37PM +0000, David Woodhouse wrote:
> The check on x86ms->apic_id_limit in pc_machine_done() had two problems.
> 
> Firstly, we need KVM to support the X2APIC API in order to allow IRQ
> delivery to APICs >= 255. So we need to call/check kvm_enable_x2apic(),
> which was done elsewhere in *some* cases but not all.
> 
> Secondly, microvm needs the same check. So move it from pc_machine_done()
> to x86_cpus_init() where it will work for both.
> 
> The check in kvm_cpu_instance_init() is now redundant and can be dropped.
> 
> Signed-off-by: David Woodhouse <dwmw2@infradead.org>
> Acked-by: Claudio Fontana <cfontana@suse.de>
> ---
>  hw/i386/pc.c              |  8 --------
>  hw/i386/x86.c             | 16 ++++++++++++++++
>  target/i386/kvm/kvm-cpu.c |  2 +-
>  3 files changed, 17 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index a2ef40ecbc..9959f93216 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -736,14 +736,6 @@ void pc_machine_done(Notifier *notifier, void *data)
>          /* update FW_CFG_NB_CPUS to account for -device added CPUs */
>          fw_cfg_modify_i16(x86ms->fw_cfg, FW_CFG_NB_CPUS, x86ms->boot_cpus);
>      }
> -
> -
> -    if (x86ms->apic_id_limit > 255 && !xen_enabled() &&
> -        !kvm_irqchip_in_kernel()) {
> -        error_report("current -smp configuration requires kernel "
> -                     "irqchip support.");
> -        exit(EXIT_FAILURE);
> -    }
>  }
>  
>  void pc_guest_info_init(PCMachineState *pcms)
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index b84840a1bb..f64639b873 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -39,6 +39,7 @@
>  #include "sysemu/replay.h"
>  #include "sysemu/sysemu.h"
>  #include "sysemu/cpu-timers.h"
> +#include "sysemu/xen.h"
>  #include "trace.h"
>  
>  #include "hw/i386/x86.h"
> @@ -136,6 +137,21 @@ void x86_cpus_init(X86MachineState *x86ms, int default_cpu_version)
>       */
>      x86ms->apic_id_limit = x86_cpu_apic_id_from_index(x86ms,
>                                                        ms->smp.max_cpus - 1) + 1;
> +
> +    /*
> +     * Can we support APIC ID 255 or higher?
> +     *
> +     * Under Xen: yes.
> +     * With userspace emulated lapic: no
> +     * With KVM's in-kernel lapic: only if X2APIC API is enabled.
> +     */
> +    if (x86ms->apic_id_limit > 255 && !xen_enabled() &&
> +        (!kvm_irqchip_in_kernel() || !kvm_enable_x2apic())) {

I'm wondering whether we should still leave it be in the accel code, or is
therer something that guarantees when reaching here kvm accel is initialized?

> +        error_report("current -smp configuration requires kernel "
> +                     "irqchip and X2APIC API support.");
> +        exit(EXIT_FAILURE);
> +    }
> +
>      possible_cpus = mc->possible_cpu_arch_ids(ms);
>      for (i = 0; i < ms->smp.cpus; i++) {
>          x86_cpu_new(x86ms, possible_cpus->cpus[i].arch_id, &error_fatal);
> diff --git a/target/i386/kvm/kvm-cpu.c b/target/i386/kvm/kvm-cpu.c
> index d95028018e..c60cb2dafb 100644
> --- a/target/i386/kvm/kvm-cpu.c
> +++ b/target/i386/kvm/kvm-cpu.c
> @@ -165,7 +165,7 @@ static void kvm_cpu_instance_init(CPUState *cs)
>          /* only applies to builtin_x86_defs cpus */
>          if (!kvm_irqchip_in_kernel()) {
>              x86_cpu_change_kvm_default("x2apic", "off");
> -        } else if (kvm_irqchip_is_split() && kvm_enable_x2apic()) {
> +        } else if (kvm_irqchip_is_split()) {
>              x86_cpu_change_kvm_default("kvm-msi-ext-dest-id", "on");
>          }
>  
> -- 
> 2.31.1
> 

-- 
Peter Xu

Re: [PATCH v2 1/4] target/i386: Fix sanity check on max APIC ID / X2APIC enablement

[Peter Xu]

On Thu, Dec 09, 2021 at 10:08:37PM +0000, David Woodhouse wrote:
> The check on x86ms->apic_id_limit in pc_machine_done() had two problems.
> 
> Firstly, we need KVM to support the X2APIC API in order to allow IRQ
> delivery to APICs >= 255. So we need to call/check kvm_enable_x2apic(),
> which was done elsewhere in *some* cases but not all.
> 
> Secondly, microvm needs the same check. So move it from pc_machine_done()
> to x86_cpus_init() where it will work for both.
> 
> The check in kvm_cpu_instance_init() is now redundant and can be dropped.
> 
> Signed-off-by: David Woodhouse <dwmw2@infradead.org>
> Acked-by: Claudio Fontana <cfontana@suse.de>
> ---
>  hw/i386/pc.c              |  8 --------
>  hw/i386/x86.c             | 16 ++++++++++++++++
>  target/i386/kvm/kvm-cpu.c |  2 +-
>  3 files changed, 17 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index a2ef40ecbc..9959f93216 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -736,14 +736,6 @@ void pc_machine_done(Notifier *notifier, void *data)
>          /* update FW_CFG_NB_CPUS to account for -device added CPUs */
>          fw_cfg_modify_i16(x86ms->fw_cfg, FW_CFG_NB_CPUS, x86ms->boot_cpus);
>      }
> -
> -
> -    if (x86ms->apic_id_limit > 255 && !xen_enabled() &&
> -        !kvm_irqchip_in_kernel()) {
> -        error_report("current -smp configuration requires kernel "
> -                     "irqchip support.");
> -        exit(EXIT_FAILURE);
> -    }
>  }
>  
>  void pc_guest_info_init(PCMachineState *pcms)
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index b84840a1bb..f64639b873 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -39,6 +39,7 @@
>  #include "sysemu/replay.h"
>  #include "sysemu/sysemu.h"
>  #include "sysemu/cpu-timers.h"
> +#include "sysemu/xen.h"
>  #include "trace.h"
>  
>  #include "hw/i386/x86.h"
> @@ -136,6 +137,21 @@ void x86_cpus_init(X86MachineState *x86ms, int default_cpu_version)
>       */
>      x86ms->apic_id_limit = x86_cpu_apic_id_from_index(x86ms,
>                                                        ms->smp.max_cpus - 1) + 1;
> +
> +    /*
> +     * Can we support APIC ID 255 or higher?
> +     *
> +     * Under Xen: yes.
> +     * With userspace emulated lapic: no
> +     * With KVM's in-kernel lapic: only if X2APIC API is enabled.
> +     */
> +    if (x86ms->apic_id_limit > 255 && !xen_enabled() &&
> +        (!kvm_irqchip_in_kernel() || !kvm_enable_x2apic())) {

I'm wondering whether we should still leave it be in the accel code, or is
therer something that guarantees when reaching here kvm accel is initialized?

> +        error_report("current -smp configuration requires kernel "
> +                     "irqchip and X2APIC API support.");
> +        exit(EXIT_FAILURE);
> +    }
> +
>      possible_cpus = mc->possible_cpu_arch_ids(ms);
>      for (i = 0; i < ms->smp.cpus; i++) {
>          x86_cpu_new(x86ms, possible_cpus->cpus[i].arch_id, &error_fatal);
> diff --git a/target/i386/kvm/kvm-cpu.c b/target/i386/kvm/kvm-cpu.c
> index d95028018e..c60cb2dafb 100644
> --- a/target/i386/kvm/kvm-cpu.c
> +++ b/target/i386/kvm/kvm-cpu.c
> @@ -165,7 +165,7 @@ static void kvm_cpu_instance_init(CPUState *cs)
>          /* only applies to builtin_x86_defs cpus */
>          if (!kvm_irqchip_in_kernel()) {
>              x86_cpu_change_kvm_default("x2apic", "off");
> -        } else if (kvm_irqchip_is_split() && kvm_enable_x2apic()) {
> +        } else if (kvm_irqchip_is_split()) {
>              x86_cpu_change_kvm_default("kvm-msi-ext-dest-id", "on");
>          }
>  
> -- 
> 2.31.1
> 

-- 
Peter Xu

Re: [PATCH v2 4/4] intel_iommu: Fix irqchip / X2APIC configuration checks

[Peter Xu]

Hi, David,

On Thu, Dec 09, 2021 at 10:08:40PM +0000, David Woodhouse wrote:
> We don't need to check kvm_enable_x2apic(). It's perfectly OK to support
> interrupt remapping even if we can't address CPUs above 254. Kind of
> pointless, but still functional.

We only checks kvm_enable_x2apic() if eim=on is set, right?  I mean, we can
still enable IR without x2apic even with current code?

Could you elaborate what's the use scenario for this patch?  Thanks in advance.

> 
> The check on kvm_enable_x2apic() needs to happen *anyway* in order to
> allow CPUs above 254 even without an IOMMU, so allow that to happen
> elsewhere.
> 
> However, we do require the *split* irqchip in order to rewrite I/OAPIC
> destinations. So fix that check while we're here.
> 
> Signed-off-by: David Woodhouse <dwmw2@infradead.org>
> Reviewed-by: Peter Xu <peterx@redhat.com>
> Acked-by: Jason Wang <jasowang@redhat.com>

I think the r-b and a-b should be for patch 2 not this one? :)

> ---
>  hw/i386/intel_iommu.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index bd288d45bb..0d1c72f08e 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -3760,15 +3760,10 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error **errp)
>                                                ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
>      }
>      if (s->intr_eim == ON_OFF_AUTO_ON && !s->buggy_eim) {
> -        if (!kvm_irqchip_in_kernel()) {
> +        if (!kvm_irqchip_is_split()) {

I think this is okay, but note that we'll already fail if !split in
x86_iommu_realize():

    bool irq_all_kernel = kvm_irqchip_in_kernel() && !kvm_irqchip_is_split();

    /* Both Intel and AMD IOMMU IR only support "kernel-irqchip={off|split}" */
    if (x86_iommu_ir_supported(x86_iommu) && irq_all_kernel) {
        error_setg(errp, "Interrupt Remapping cannot work with "
                         "kernel-irqchip=on, please use 'split|off'.");
        return;
    }

>              error_setg(errp, "eim=on requires accel=kvm,kernel-irqchip=split");
>              return false;
>          }
> -        if (!kvm_enable_x2apic()) {
> -            error_setg(errp, "eim=on requires support on the KVM side"
> -                             "(X2APIC_API, first shipped in v4.7)");
> -            return false;
> -        }
>      }
>  
>      /* Currently only address widths supported are 39 and 48 bits */
> -- 
> 2.31.1
> 

-- 
Peter Xu

Re: [PATCH v2 4/4] intel_iommu: Fix irqchip / X2APIC configuration checks

[Peter Xu]

Hi, David,

On Thu, Dec 09, 2021 at 10:08:40PM +0000, David Woodhouse wrote:
> We don't need to check kvm_enable_x2apic(). It's perfectly OK to support
> interrupt remapping even if we can't address CPUs above 254. Kind of
> pointless, but still functional.

We only checks kvm_enable_x2apic() if eim=on is set, right?  I mean, we can
still enable IR without x2apic even with current code?

Could you elaborate what's the use scenario for this patch?  Thanks in advance.

> 
> The check on kvm_enable_x2apic() needs to happen *anyway* in order to
> allow CPUs above 254 even without an IOMMU, so allow that to happen
> elsewhere.
> 
> However, we do require the *split* irqchip in order to rewrite I/OAPIC
> destinations. So fix that check while we're here.
> 
> Signed-off-by: David Woodhouse <dwmw2@infradead.org>
> Reviewed-by: Peter Xu <peterx@redhat.com>
> Acked-by: Jason Wang <jasowang@redhat.com>

I think the r-b and a-b should be for patch 2 not this one? :)

> ---
>  hw/i386/intel_iommu.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index bd288d45bb..0d1c72f08e 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -3760,15 +3760,10 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error **errp)
>                                                ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
>      }
>      if (s->intr_eim == ON_OFF_AUTO_ON && !s->buggy_eim) {
> -        if (!kvm_irqchip_in_kernel()) {
> +        if (!kvm_irqchip_is_split()) {

I think this is okay, but note that we'll already fail if !split in
x86_iommu_realize():

    bool irq_all_kernel = kvm_irqchip_in_kernel() && !kvm_irqchip_is_split();

    /* Both Intel and AMD IOMMU IR only support "kernel-irqchip={off|split}" */
    if (x86_iommu_ir_supported(x86_iommu) && irq_all_kernel) {
        error_setg(errp, "Interrupt Remapping cannot work with "
                         "kernel-irqchip=on, please use 'split|off'.");
        return;
    }

>              error_setg(errp, "eim=on requires accel=kvm,kernel-irqchip=split");
>              return false;
>          }
> -        if (!kvm_enable_x2apic()) {
> -            error_setg(errp, "eim=on requires support on the KVM side"
> -                             "(X2APIC_API, first shipped in v4.7)");
> -            return false;
> -        }
>      }
>  
>      /* Currently only address widths supported are 39 and 48 bits */
> -- 
> 2.31.1
> 

-- 
Peter Xu

Re: [PATCH v2 2/4] intel_iommu: Support IR-only mode without DMA translation

[Peter Xu]

On Thu, Dec 09, 2021 at 10:08:38PM +0000, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> By setting none of the SAGAW bits we can indicate to a guest that DMA
> translation isn't supported. Tested by booting Windows 10, as well as
> Linux guests with the fix at https://git.kernel.org/torvalds/c/c40aaaac10
> 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> Acked-by: Claudio Fontana <cfontana@suse.de>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH v2 2/4] intel_iommu: Support IR-only mode without DMA translation

[Peter Xu]

On Thu, Dec 09, 2021 at 10:08:38PM +0000, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> By setting none of the SAGAW bits we can indicate to a guest that DMA
> translation isn't supported. Tested by booting Windows 10, as well as
> Linux guests with the fix at https://git.kernel.org/torvalds/c/c40aaaac10
> 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> Acked-by: Claudio Fontana <cfontana@suse.de>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH v2 4/4] intel_iommu: Fix irqchip / X2APIC configuration checks

[David Woodhouse]

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

On Thu, 2021-12-16 at 16:47 +0800, Peter Xu wrote:
> Hi, David,
> 
> On Thu, Dec 09, 2021 at 10:08:40PM +0000, David Woodhouse wrote:
> > We don't need to check kvm_enable_x2apic(). It's perfectly OK to support
> > interrupt remapping even if we can't address CPUs above 254. Kind of
> > pointless, but still functional.
> 
> We only checks kvm_enable_x2apic() if eim=on is set, right?  I mean, we can
> still enable IR without x2apic even with current code?
> 
> Could you elaborate what's the use scenario for this patch?  Thanks in advance.

You can have IR, EIM *and* X2APIC if kvm_enable_x2apic() fails. You
just can't have any CPUs with an APIC ID > 254.

But qemu is going to bail out *anyway* if you attempt to have CPUs with
APIC IDs above 254 without the corresponding kernel-side support, so
there's no need to check it here.

> > The check on kvm_enable_x2apic() needs to happen *anyway* in order to
> > allow CPUs above 254 even without an IOMMU, so allow that to happen
> > elsewhere.
> > 
> > However, we do require the *split* irqchip in order to rewrite I/OAPIC
> > destinations. So fix that check while we're here.
> > 
> > Signed-off-by: David Woodhouse <dwmw2@infradead.org>
> > Reviewed-by: Peter Xu <peterx@redhat.com>
> > Acked-by: Jason Wang <jasowang@redhat.com>
> 
> I think the r-b and a-b should be for patch 2 not this one? :)
> 

Yes, I think I must have swapped those round. Thanks.

> > ---
> >  hw/i386/intel_iommu.c | 7 +------
> >  1 file changed, 1 insertion(+), 6 deletions(-)
> > 
> > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > index bd288d45bb..0d1c72f08e 100644
> > --- a/hw/i386/intel_iommu.c
> > +++ b/hw/i386/intel_iommu.c
> > @@ -3760,15 +3760,10 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error **errp)
> >                                                ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
> >      }
> >      if (s->intr_eim == ON_OFF_AUTO_ON && !s->buggy_eim) {
> > -        if (!kvm_irqchip_in_kernel()) {
> > +        if (!kvm_irqchip_is_split()) {
> 
> I think this is okay, but note that we'll already fail if !split in
> x86_iommu_realize():
> 
>     bool irq_all_kernel = kvm_irqchip_in_kernel() && !kvm_irqchip_is_split();
> 
>     /* Both Intel and AMD IOMMU IR only support "kernel-irqchip={off|split}" */
>     if (x86_iommu_ir_supported(x86_iommu) && irq_all_kernel) {
>         error_setg(errp, "Interrupt Remapping cannot work with "
>                          "kernel-irqchip=on, please use 'split|off'.");
>         return;
>     }

OK, then perhaps the entire check is redundant?


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

Re: [PATCH v2 4/4] intel_iommu: Fix irqchip / X2APIC configuration checks

[David Woodhouse]

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

On Thu, 2021-12-16 at 16:47 +0800, Peter Xu wrote:
> Hi, David,
> 
> On Thu, Dec 09, 2021 at 10:08:40PM +0000, David Woodhouse wrote:
> > We don't need to check kvm_enable_x2apic(). It's perfectly OK to support
> > interrupt remapping even if we can't address CPUs above 254. Kind of
> > pointless, but still functional.
> 
> We only checks kvm_enable_x2apic() if eim=on is set, right?  I mean, we can
> still enable IR without x2apic even with current code?
> 
> Could you elaborate what's the use scenario for this patch?  Thanks in advance.

You can have IR, EIM *and* X2APIC if kvm_enable_x2apic() fails. You
just can't have any CPUs with an APIC ID > 254.

But qemu is going to bail out *anyway* if you attempt to have CPUs with
APIC IDs above 254 without the corresponding kernel-side support, so
there's no need to check it here.

> > The check on kvm_enable_x2apic() needs to happen *anyway* in order to
> > allow CPUs above 254 even without an IOMMU, so allow that to happen
> > elsewhere.
> > 
> > However, we do require the *split* irqchip in order to rewrite I/OAPIC
> > destinations. So fix that check while we're here.
> > 
> > Signed-off-by: David Woodhouse <dwmw2@infradead.org>
> > Reviewed-by: Peter Xu <peterx@redhat.com>
> > Acked-by: Jason Wang <jasowang@redhat.com>
> 
> I think the r-b and a-b should be for patch 2 not this one? :)
> 

Yes, I think I must have swapped those round. Thanks.

> > ---
> >  hw/i386/intel_iommu.c | 7 +------
> >  1 file changed, 1 insertion(+), 6 deletions(-)
> > 
> > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > index bd288d45bb..0d1c72f08e 100644
> > --- a/hw/i386/intel_iommu.c
> > +++ b/hw/i386/intel_iommu.c
> > @@ -3760,15 +3760,10 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error **errp)
> >                                                ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
> >      }
> >      if (s->intr_eim == ON_OFF_AUTO_ON && !s->buggy_eim) {
> > -        if (!kvm_irqchip_in_kernel()) {
> > +        if (!kvm_irqchip_is_split()) {
> 
> I think this is okay, but note that we'll already fail if !split in
> x86_iommu_realize():
> 
>     bool irq_all_kernel = kvm_irqchip_in_kernel() && !kvm_irqchip_is_split();
> 
>     /* Both Intel and AMD IOMMU IR only support "kernel-irqchip={off|split}" */
>     if (x86_iommu_ir_supported(x86_iommu) && irq_all_kernel) {
>         error_setg(errp, "Interrupt Remapping cannot work with "
>                          "kernel-irqchip=on, please use 'split|off'.");
>         return;
>     }

OK, then perhaps the entire check is redundant?


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

Re: [PATCH v2 4/4] intel_iommu: Fix irqchip / X2APIC configuration checks

[Peter Xu]

On Fri, Dec 17, 2021 at 04:51:20PM +0000, David Woodhouse wrote:
> On Thu, 2021-12-16 at 16:47 +0800, Peter Xu wrote:
> > Hi, David,
> > 
> > On Thu, Dec 09, 2021 at 10:08:40PM +0000, David Woodhouse wrote:
> > > We don't need to check kvm_enable_x2apic(). It's perfectly OK to support
> > > interrupt remapping even if we can't address CPUs above 254. Kind of
> > > pointless, but still functional.
> > 
> > We only checks kvm_enable_x2apic() if eim=on is set, right?  I mean, we can
> > still enable IR without x2apic even with current code?
> > 
> > Could you elaborate what's the use scenario for this patch?  Thanks in advance.
> 
> You can have IR, EIM *and* X2APIC if kvm_enable_x2apic() fails. You
> just can't have any CPUs with an APIC ID > 254.
> 
> But qemu is going to bail out *anyway* if you attempt to have CPUs with
> APIC IDs above 254 without the corresponding kernel-side support, so
> there's no need to check it here.

Ah OK.

> 
> > > The check on kvm_enable_x2apic() needs to happen *anyway* in order to
> > > allow CPUs above 254 even without an IOMMU, so allow that to happen
> > > elsewhere.
> > > 
> > > However, we do require the *split* irqchip in order to rewrite I/OAPIC
> > > destinations. So fix that check while we're here.
> > > 
> > > Signed-off-by: David Woodhouse <dwmw2@infradead.org>
> > > Reviewed-by: Peter Xu <peterx@redhat.com>
> > > Acked-by: Jason Wang <jasowang@redhat.com>
> > 
> > I think the r-b and a-b should be for patch 2 not this one? :)
> > 
> 
> Yes, I think I must have swapped those round. Thanks.
> 
> > > ---
> > >  hw/i386/intel_iommu.c | 7 +------
> > >  1 file changed, 1 insertion(+), 6 deletions(-)
> > > 
> > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > > index bd288d45bb..0d1c72f08e 100644
> > > --- a/hw/i386/intel_iommu.c
> > > +++ b/hw/i386/intel_iommu.c
> > > @@ -3760,15 +3760,10 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error **errp)
> > >                                                ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
> > >      }
> > >      if (s->intr_eim == ON_OFF_AUTO_ON && !s->buggy_eim) {
> > > -        if (!kvm_irqchip_in_kernel()) {
> > > +        if (!kvm_irqchip_is_split()) {
> > 
> > I think this is okay, but note that we'll already fail if !split in
> > x86_iommu_realize():
> > 
> >     bool irq_all_kernel = kvm_irqchip_in_kernel() && !kvm_irqchip_is_split();
> > 
> >     /* Both Intel and AMD IOMMU IR only support "kernel-irqchip={off|split}" */
> >     if (x86_iommu_ir_supported(x86_iommu) && irq_all_kernel) {
> >         error_setg(errp, "Interrupt Remapping cannot work with "
> >                          "kernel-irqchip=on, please use 'split|off'.");
> >         return;
> >     }
> 
> OK, then perhaps the entire check is redundant?

Yes, maybe.

It also reminded me that this is the only place that we used the "buggy_eim"
variable.  If we drop this chunk, that flag will become meaningless.

If we look back, it seems to decides whether we should call kvm_enable_x2apic()
at all, so as to be compatible with old qemus.  Please see commit fb506e701e
("intel_iommu: reject broken EIM", 2016-10-17).

hw_compat_2_7 has:

    { "intel-iommu", "x-buggy-eim", "true" },

It means kvm_enable_x2apic() (at least before commit c1bb5418e3 of yours)
should be skipped for 2.7 or older version of QEMU binaries.

Now after commit c1bb5418e3 we'll unconditionally call kvm_enable_x2apic() in
x86_cpu_load_model() anyway, even if x-buggy-eim=on.  IIUC it violates with the
original purpose of commit fb506e701e.

However maybe it's not necessary to maintain that awkward/buggy compatibility
at all for those old qemu binaries.  I can hardly imagine someone uses vIOMMU
2.7- versions for production purposes, and for relying on that buggy behavior
to work.

To summarize: I'm wondering whether we should also drop buggy-eim as a whole..

Thanks,

-- 
Peter Xu

Re: [PATCH v2 4/4] intel_iommu: Fix irqchip / X2APIC configuration checks

[Peter Xu]

On Fri, Dec 17, 2021 at 04:51:20PM +0000, David Woodhouse wrote:
> On Thu, 2021-12-16 at 16:47 +0800, Peter Xu wrote:
> > Hi, David,
> > 
> > On Thu, Dec 09, 2021 at 10:08:40PM +0000, David Woodhouse wrote:
> > > We don't need to check kvm_enable_x2apic(). It's perfectly OK to support
> > > interrupt remapping even if we can't address CPUs above 254. Kind of
> > > pointless, but still functional.
> > 
> > We only checks kvm_enable_x2apic() if eim=on is set, right?  I mean, we can
> > still enable IR without x2apic even with current code?
> > 
> > Could you elaborate what's the use scenario for this patch?  Thanks in advance.
> 
> You can have IR, EIM *and* X2APIC if kvm_enable_x2apic() fails. You
> just can't have any CPUs with an APIC ID > 254.
> 
> But qemu is going to bail out *anyway* if you attempt to have CPUs with
> APIC IDs above 254 without the corresponding kernel-side support, so
> there's no need to check it here.

Ah OK.

> 
> > > The check on kvm_enable_x2apic() needs to happen *anyway* in order to
> > > allow CPUs above 254 even without an IOMMU, so allow that to happen
> > > elsewhere.
> > > 
> > > However, we do require the *split* irqchip in order to rewrite I/OAPIC
> > > destinations. So fix that check while we're here.
> > > 
> > > Signed-off-by: David Woodhouse <dwmw2@infradead.org>
> > > Reviewed-by: Peter Xu <peterx@redhat.com>
> > > Acked-by: Jason Wang <jasowang@redhat.com>
> > 
> > I think the r-b and a-b should be for patch 2 not this one? :)
> > 
> 
> Yes, I think I must have swapped those round. Thanks.
> 
> > > ---
> > >  hw/i386/intel_iommu.c | 7 +------
> > >  1 file changed, 1 insertion(+), 6 deletions(-)
> > > 
> > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > > index bd288d45bb..0d1c72f08e 100644
> > > --- a/hw/i386/intel_iommu.c
> > > +++ b/hw/i386/intel_iommu.c
> > > @@ -3760,15 +3760,10 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error **errp)
> > >                                                ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
> > >      }
> > >      if (s->intr_eim == ON_OFF_AUTO_ON && !s->buggy_eim) {
> > > -        if (!kvm_irqchip_in_kernel()) {
> > > +        if (!kvm_irqchip_is_split()) {
> > 
> > I think this is okay, but note that we'll already fail if !split in
> > x86_iommu_realize():
> > 
> >     bool irq_all_kernel = kvm_irqchip_in_kernel() && !kvm_irqchip_is_split();
> > 
> >     /* Both Intel and AMD IOMMU IR only support "kernel-irqchip={off|split}" */
> >     if (x86_iommu_ir_supported(x86_iommu) && irq_all_kernel) {
> >         error_setg(errp, "Interrupt Remapping cannot work with "
> >                          "kernel-irqchip=on, please use 'split|off'.");
> >         return;
> >     }
> 
> OK, then perhaps the entire check is redundant?

Yes, maybe.

It also reminded me that this is the only place that we used the "buggy_eim"
variable.  If we drop this chunk, that flag will become meaningless.

If we look back, it seems to decides whether we should call kvm_enable_x2apic()
at all, so as to be compatible with old qemus.  Please see commit fb506e701e
("intel_iommu: reject broken EIM", 2016-10-17).

hw_compat_2_7 has:

    { "intel-iommu", "x-buggy-eim", "true" },

It means kvm_enable_x2apic() (at least before commit c1bb5418e3 of yours)
should be skipped for 2.7 or older version of QEMU binaries.

Now after commit c1bb5418e3 we'll unconditionally call kvm_enable_x2apic() in
x86_cpu_load_model() anyway, even if x-buggy-eim=on.  IIUC it violates with the
original purpose of commit fb506e701e.

However maybe it's not necessary to maintain that awkward/buggy compatibility
at all for those old qemu binaries.  I can hardly imagine someone uses vIOMMU
2.7- versions for production purposes, and for relying on that buggy behavior
to work.

To summarize: I'm wondering whether we should also drop buggy-eim as a whole..

Thanks,

-- 
Peter Xu

Re: [PATCH v2 1/4] target/i386: Fix sanity check on max APIC ID / X2APIC enablement

[Michael S. Tsirkin]

On Thu, Dec 09, 2021 at 10:08:37PM +0000, David Woodhouse wrote:
> The check on x86ms->apic_id_limit in pc_machine_done() had two problems.
> 
> Firstly, we need KVM to support the X2APIC API in order to allow IRQ
> delivery to APICs >= 255. So we need to call/check kvm_enable_x2apic(),
> which was done elsewhere in *some* cases but not all.
> 
> Secondly, microvm needs the same check. So move it from pc_machine_done()
> to x86_cpus_init() where it will work for both.
> 
> The check in kvm_cpu_instance_init() is now redundant and can be dropped.
> 
> Signed-off-by: David Woodhouse <dwmw2@infradead.org>
> Acked-by: Claudio Fontana <cfontana@suse.de>

Could I get an ack from KVM maintainers on this one please?
Thanks!

> ---
>  hw/i386/pc.c              |  8 --------
>  hw/i386/x86.c             | 16 ++++++++++++++++
>  target/i386/kvm/kvm-cpu.c |  2 +-
>  3 files changed, 17 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index a2ef40ecbc..9959f93216 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -736,14 +736,6 @@ void pc_machine_done(Notifier *notifier, void *data)
>          /* update FW_CFG_NB_CPUS to account for -device added CPUs */
>          fw_cfg_modify_i16(x86ms->fw_cfg, FW_CFG_NB_CPUS, x86ms->boot_cpus);
>      }
> -
> -
> -    if (x86ms->apic_id_limit > 255 && !xen_enabled() &&
> -        !kvm_irqchip_in_kernel()) {
> -        error_report("current -smp configuration requires kernel "
> -                     "irqchip support.");
> -        exit(EXIT_FAILURE);
> -    }
>  }
>  
>  void pc_guest_info_init(PCMachineState *pcms)
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index b84840a1bb..f64639b873 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -39,6 +39,7 @@
>  #include "sysemu/replay.h"
>  #include "sysemu/sysemu.h"
>  #include "sysemu/cpu-timers.h"
> +#include "sysemu/xen.h"
>  #include "trace.h"
>  
>  #include "hw/i386/x86.h"
> @@ -136,6 +137,21 @@ void x86_cpus_init(X86MachineState *x86ms, int default_cpu_version)
>       */
>      x86ms->apic_id_limit = x86_cpu_apic_id_from_index(x86ms,
>                                                        ms->smp.max_cpus - 1) + 1;
> +
> +    /*
> +     * Can we support APIC ID 255 or higher?
> +     *
> +     * Under Xen: yes.
> +     * With userspace emulated lapic: no
> +     * With KVM's in-kernel lapic: only if X2APIC API is enabled.
> +     */
> +    if (x86ms->apic_id_limit > 255 && !xen_enabled() &&
> +        (!kvm_irqchip_in_kernel() || !kvm_enable_x2apic())) {
> +        error_report("current -smp configuration requires kernel "
> +                     "irqchip and X2APIC API support.");
> +        exit(EXIT_FAILURE);
> +    }
> +
>      possible_cpus = mc->possible_cpu_arch_ids(ms);
>      for (i = 0; i < ms->smp.cpus; i++) {
>          x86_cpu_new(x86ms, possible_cpus->cpus[i].arch_id, &error_fatal);
> diff --git a/target/i386/kvm/kvm-cpu.c b/target/i386/kvm/kvm-cpu.c
> index d95028018e..c60cb2dafb 100644
> --- a/target/i386/kvm/kvm-cpu.c
> +++ b/target/i386/kvm/kvm-cpu.c
> @@ -165,7 +165,7 @@ static void kvm_cpu_instance_init(CPUState *cs)
>          /* only applies to builtin_x86_defs cpus */
>          if (!kvm_irqchip_in_kernel()) {
>              x86_cpu_change_kvm_default("x2apic", "off");
> -        } else if (kvm_irqchip_is_split() && kvm_enable_x2apic()) {
> +        } else if (kvm_irqchip_is_split()) {
>              x86_cpu_change_kvm_default("kvm-msi-ext-dest-id", "on");
>          }
>  
> -- 
> 2.31.1

Re: [PATCH v2 1/4] target/i386: Fix sanity check on max APIC ID / X2APIC enablement

[Michael S. Tsirkin]

On Thu, Dec 09, 2021 at 10:08:37PM +0000, David Woodhouse wrote:
> The check on x86ms->apic_id_limit in pc_machine_done() had two problems.
> 
> Firstly, we need KVM to support the X2APIC API in order to allow IRQ
> delivery to APICs >= 255. So we need to call/check kvm_enable_x2apic(),
> which was done elsewhere in *some* cases but not all.
> 
> Secondly, microvm needs the same check. So move it from pc_machine_done()
> to x86_cpus_init() where it will work for both.
> 
> The check in kvm_cpu_instance_init() is now redundant and can be dropped.
> 
> Signed-off-by: David Woodhouse <dwmw2@infradead.org>
> Acked-by: Claudio Fontana <cfontana@suse.de>

Could I get an ack from KVM maintainers on this one please?
Thanks!

> ---
>  hw/i386/pc.c              |  8 --------
>  hw/i386/x86.c             | 16 ++++++++++++++++
>  target/i386/kvm/kvm-cpu.c |  2 +-
>  3 files changed, 17 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index a2ef40ecbc..9959f93216 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -736,14 +736,6 @@ void pc_machine_done(Notifier *notifier, void *data)
>          /* update FW_CFG_NB_CPUS to account for -device added CPUs */
>          fw_cfg_modify_i16(x86ms->fw_cfg, FW_CFG_NB_CPUS, x86ms->boot_cpus);
>      }
> -
> -
> -    if (x86ms->apic_id_limit > 255 && !xen_enabled() &&
> -        !kvm_irqchip_in_kernel()) {
> -        error_report("current -smp configuration requires kernel "
> -                     "irqchip support.");
> -        exit(EXIT_FAILURE);
> -    }
>  }
>  
>  void pc_guest_info_init(PCMachineState *pcms)
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index b84840a1bb..f64639b873 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -39,6 +39,7 @@
>  #include "sysemu/replay.h"
>  #include "sysemu/sysemu.h"
>  #include "sysemu/cpu-timers.h"
> +#include "sysemu/xen.h"
>  #include "trace.h"
>  
>  #include "hw/i386/x86.h"
> @@ -136,6 +137,21 @@ void x86_cpus_init(X86MachineState *x86ms, int default_cpu_version)
>       */
>      x86ms->apic_id_limit = x86_cpu_apic_id_from_index(x86ms,
>                                                        ms->smp.max_cpus - 1) + 1;
> +
> +    /*
> +     * Can we support APIC ID 255 or higher?
> +     *
> +     * Under Xen: yes.
> +     * With userspace emulated lapic: no
> +     * With KVM's in-kernel lapic: only if X2APIC API is enabled.
> +     */
> +    if (x86ms->apic_id_limit > 255 && !xen_enabled() &&
> +        (!kvm_irqchip_in_kernel() || !kvm_enable_x2apic())) {
> +        error_report("current -smp configuration requires kernel "
> +                     "irqchip and X2APIC API support.");
> +        exit(EXIT_FAILURE);
> +    }
> +
>      possible_cpus = mc->possible_cpu_arch_ids(ms);
>      for (i = 0; i < ms->smp.cpus; i++) {
>          x86_cpu_new(x86ms, possible_cpus->cpus[i].arch_id, &error_fatal);
> diff --git a/target/i386/kvm/kvm-cpu.c b/target/i386/kvm/kvm-cpu.c
> index d95028018e..c60cb2dafb 100644
> --- a/target/i386/kvm/kvm-cpu.c
> +++ b/target/i386/kvm/kvm-cpu.c
> @@ -165,7 +165,7 @@ static void kvm_cpu_instance_init(CPUState *cs)
>          /* only applies to builtin_x86_defs cpus */
>          if (!kvm_irqchip_in_kernel()) {
>              x86_cpu_change_kvm_default("x2apic", "off");
> -        } else if (kvm_irqchip_is_split() && kvm_enable_x2apic()) {
> +        } else if (kvm_irqchip_is_split()) {
>              x86_cpu_change_kvm_default("kvm-msi-ext-dest-id", "on");
>          }
>  
> -- 
> 2.31.1