[Qemu-devel] [PATCH v2 0/7] intel_iommu: fix EIM

[Qemu-devel] [PATCH v2 0/7] intel_iommu: fix EIM

[Radim Krčmář]

v1: http://lists.nongnu.org/archive/html/qemu-devel/2016-09/msg05960.html

The x2APIC mode works on >=2.8 machine types with this series and <2.7
remain compatible (aka broken).

[4/7] and [5/7] can be squished.


Radim Krčmář (7):
  apic: add global apic_get_class()
  apic: add send_msi() to APICCommonClass
  intel_iommu: pass whole remapped addresses to apic
  intel-iommu: exit on invalid configuraton earlier
  intel-iommu: add OnOffAuto intr_eim as "eim" property
  intel_iommu: reject broken EIM
  intel-iommu: keep buggy EIM enabled in 2.7 machine type

 hw/i386/intel_iommu.c           | 76 +++++++++++++++++++++++++++++------------
 hw/i386/kvm/apic.c              | 19 +++++++----
 hw/i386/pc_q35.c                |  2 ++
 hw/i386/xen/xen_apic.c          |  6 ++++
 hw/intc/apic.c                  |  8 +++--
 hw/intc/apic_common.c           | 11 ++++++
 include/hw/i386/apic_internal.h |  7 ++++
 include/hw/i386/intel_iommu.h   |  1 +
 include/hw/i386/pc.h            |  2 ++
 target-i386/kvm-stub.c          |  5 +++
 target-i386/kvm.c               | 13 +++++++
 target-i386/kvm_i386.h          |  1 +
 12 files changed, 122 insertions(+), 29 deletions(-)

-- 
2.10.0

[Qemu-devel] [PATCH v2 1/7] apic: add global apic_get_class()

[Radim Krčmář]

Every configuration has only up to one APIC class and we'll be extending
the class with a function that can be called without an instanced
object, so a direct access to the class is convenient.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
v2: assert() instead of error_report() and exit() [Peter]
---
 hw/intc/apic_common.c           | 11 +++++++++++
 include/hw/i386/apic_internal.h |  3 +++
 2 files changed, 14 insertions(+)

diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index 14ac43c18666..a766f0efc805 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -18,6 +18,7 @@
  * License along with this library; if not, see <http://www.gnu.org/licenses/>
  */
 #include "qemu/osdep.h"
+#include "qemu/error-report.h"
 #include "qapi/error.h"
 #include "qemu-common.h"
 #include "cpu.h"
@@ -296,6 +297,13 @@ static int apic_load_old(QEMUFile *f, void *opaque, int version_id)
 
 static const VMStateDescription vmstate_apic_common;
 
+APICCommonClass *apic_class;
+
+APICCommonClass *apic_get_class(void)
+{
+    return apic_class;
+}
+
 static void apic_common_realize(DeviceState *dev, Error **errp)
 {
     APICCommonState *s = APIC_COMMON(dev);
@@ -306,6 +314,9 @@ static void apic_common_realize(DeviceState *dev, Error **errp)
     info = APIC_COMMON_GET_CLASS(s);
     info->realize(dev, errp);
 
+    assert(apic_class == info || !apic_class);
+    apic_class = info;
+
     /* Note: We need at least 1M to map the VAPIC option ROM */
     if (!vapic && s->vapic_control & VAPIC_ENABLE_MASK &&
         ram_size >= 1024 * 1024) {
diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
index 06c4e9f6f95b..9ba8a5c87f90 100644
--- a/include/hw/i386/apic_internal.h
+++ b/include/hw/i386/apic_internal.h
@@ -222,4 +222,7 @@ static inline int apic_get_bit(uint32_t *tab, int index)
     return !!(tab[i] & mask);
 }
 
+
+APICCommonClass *apic_get_class(void);
+
 #endif /* QEMU_APIC_INTERNAL_H */
-- 
2.10.0

[Qemu-devel] [PATCH v2 2/7] apic: add send_msi() to APICCommonClass

[Radim Krčmář]

The MMIO based interface to APIC doesn't work well with MSIs that have
upper address bits set (remapped x2APIC MSIs).  A specialized interface
is a quick and dirty way to avoid the shortcoming.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
v2: change apic_send_msi() to accept MSIMessage [Igor]
---
 hw/i386/kvm/apic.c              | 19 +++++++++++++------
 hw/i386/xen/xen_apic.c          |  6 ++++++
 hw/intc/apic.c                  |  8 ++++++--
 include/hw/i386/apic_internal.h |  4 ++++
 4 files changed, 29 insertions(+), 8 deletions(-)

diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c
index c016e63fc2ba..be55102c00ca 100644
--- a/hw/i386/kvm/apic.c
+++ b/hw/i386/kvm/apic.c
@@ -169,6 +169,17 @@ static void kvm_apic_external_nmi(APICCommonState *s)
     run_on_cpu(CPU(s->cpu), do_inject_external_nmi, s);
 }
 
+static void kvm_send_msi(MSIMessage *msg)
+{
+    int ret;
+
+    ret = kvm_irqchip_send_msi(kvm_state, *msg);
+    if (ret < 0) {
+        fprintf(stderr, "KVM: injection failed, MSI lost (%s)\n",
+                strerror(-ret));
+    }
+}
+
 static uint64_t kvm_apic_mem_read(void *opaque, hwaddr addr,
                                   unsigned size)
 {
@@ -179,13 +190,8 @@ static void kvm_apic_mem_write(void *opaque, hwaddr addr,
                                uint64_t data, unsigned size)
 {
     MSIMessage msg = { .address = addr, .data = data };
-    int ret;
 
-    ret = kvm_irqchip_send_msi(kvm_state, msg);
-    if (ret < 0) {
-        fprintf(stderr, "KVM: injection failed, MSI lost (%s)\n",
-                strerror(-ret));
-    }
+    kvm_send_msi(&msg);
 }
 
 static const MemoryRegionOps kvm_apic_io_ops = {
@@ -232,6 +238,7 @@ static void kvm_apic_class_init(ObjectClass *klass, void *data)
     k->enable_tpr_reporting = kvm_apic_enable_tpr_reporting;
     k->vapic_base_update = kvm_apic_vapic_base_update;
     k->external_nmi = kvm_apic_external_nmi;
+    k->send_msi = kvm_send_msi;
 }
 
 static const TypeInfo kvm_apic_info = {
diff --git a/hw/i386/xen/xen_apic.c b/hw/i386/xen/xen_apic.c
index 21d68ee04b0a..55769eba7ede 100644
--- a/hw/i386/xen/xen_apic.c
+++ b/hw/i386/xen/xen_apic.c
@@ -68,6 +68,11 @@ static void xen_apic_external_nmi(APICCommonState *s)
 {
 }
 
+static void xen_send_msi(MSIMessage *msi)
+{
+    xen_hvm_inject_msi(msi->address, msi->data);
+}
+
 static void xen_apic_class_init(ObjectClass *klass, void *data)
 {
     APICCommonClass *k = APIC_COMMON_CLASS(klass);
@@ -78,6 +83,7 @@ static void xen_apic_class_init(ObjectClass *klass, void *data)
     k->get_tpr = xen_apic_get_tpr;
     k->vapic_base_update = xen_apic_vapic_base_update;
     k->external_nmi = xen_apic_external_nmi;
+    k->send_msi = xen_send_msi;
 }
 
 static const TypeInfo xen_apic_info = {
diff --git a/hw/intc/apic.c b/hw/intc/apic.c
index 7bd1d279c463..fe15fb602473 100644
--- a/hw/intc/apic.c
+++ b/hw/intc/apic.c
@@ -740,8 +740,10 @@ static uint32_t apic_mem_readl(void *opaque, hwaddr addr)
     return val;
 }
 
-static void apic_send_msi(hwaddr addr, uint32_t data)
+static void apic_send_msi(MSIMessage *msi)
 {
+    uint64_t addr = msi->address;
+    uint32_t data = msi->data;
     uint8_t dest = (addr & MSI_ADDR_DEST_ID_MASK) >> MSI_ADDR_DEST_ID_SHIFT;
     uint8_t vector = (data & MSI_DATA_VECTOR_MASK) >> MSI_DATA_VECTOR_SHIFT;
     uint8_t dest_mode = (addr >> MSI_ADDR_DEST_MODE_SHIFT) & 0x1;
@@ -762,7 +764,8 @@ static void apic_mem_writel(void *opaque, hwaddr addr, uint32_t val)
          * APIC is connected directly to the CPU.
          * Mapping them on the global bus happens to work because
          * MSI registers are reserved in APIC MMIO and vice versa. */
-        apic_send_msi(addr, val);
+        MSIMessage msi = { .address = addr, .data = val };
+        apic_send_msi(&msi);
         return;
     }
 
@@ -913,6 +916,7 @@ static void apic_class_init(ObjectClass *klass, void *data)
     k->external_nmi = apic_external_nmi;
     k->pre_save = apic_pre_save;
     k->post_load = apic_post_load;
+    k->send_msi = apic_send_msi;
 }
 
 static const TypeInfo apic_info = {
diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
index 9ba8a5c87f90..32b083ad2926 100644
--- a/include/hw/i386/apic_internal.h
+++ b/include/hw/i386/apic_internal.h
@@ -146,6 +146,10 @@ typedef struct APICCommonClass
     void (*pre_save)(APICCommonState *s);
     void (*post_load)(APICCommonState *s);
     void (*reset)(APICCommonState *s);
+    /* send_msi emulates an APIC bus and its proper place would be in a new
+     * device, but it's convenient to have it here for now.
+     */
+    void (*send_msi)(MSIMessage *msi);
 } APICCommonClass;
 
 struct APICCommonState {
-- 
2.10.0

[Qemu-devel] [PATCH v2 3/7] intel_iommu: pass whole remapped addresses to apic

[Radim Krčmář]

The MMIO interface to APIC only allowed 8 bit addresses, which is not
enough for 32 bit addresses from EIM remapping.
Intel stored upper 24 bits in the high MSI address, so use the same
technique. The technique is also used in KVM MSI interface.
Other APICs are unlikely to handle those upper bits.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
v2: fix build with enabled DEBUG_INTEL_IOMMU [Peter]
---
 hw/i386/intel_iommu.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 9f4e64af1ad5..c39b62b898d8 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -31,6 +31,7 @@
 #include "hw/i386/x86-iommu.h"
 #include "hw/pci-host/q35.h"
 #include "sysemu/kvm.h"
+#include "hw/i386/apic_internal.h"
 
 /*#define DEBUG_INTEL_IOMMU*/
 #ifdef DEBUG_INTEL_IOMMU
@@ -279,18 +280,17 @@ static void vtd_update_iotlb(IntelIOMMUState *s, uint16_t source_id,
 static void vtd_generate_interrupt(IntelIOMMUState *s, hwaddr mesg_addr_reg,
                                    hwaddr mesg_data_reg)
 {
-    hwaddr addr;
-    uint32_t data;
+    MSIMessage msi;
 
     assert(mesg_data_reg < DMAR_REG_SIZE);
     assert(mesg_addr_reg < DMAR_REG_SIZE);
 
-    addr = vtd_get_long_raw(s, mesg_addr_reg);
-    data = vtd_get_long_raw(s, mesg_data_reg);
+    msi.address = vtd_get_long_raw(s, mesg_addr_reg);
+    msi.data = vtd_get_long_raw(s, mesg_data_reg);
 
-    VTD_DPRINTF(FLOG, "msi: addr 0x%"PRIx64 " data 0x%"PRIx32, addr, data);
-    address_space_stl_le(&address_space_memory, addr, data,
-                         MEMTXATTRS_UNSPECIFIED, NULL);
+    VTD_DPRINTF(FLOG, "msi: addr 0x%"PRIx64 " data 0x%"PRIx32,
+                msi.address, msi.data);
+    apic_get_class()->send_msi(&msi);
 }
 
 /* Generate a fault event to software via MSI if conditions are met.
@@ -2133,6 +2133,7 @@ static void vtd_generate_msi_message(VTDIrq *irq, MSIMessage *msg_out)
     msg.dest_mode = irq->dest_mode;
     msg.redir_hint = irq->redir_hint;
     msg.dest = irq->dest;
+    msg.__addr_hi = irq->dest & 0xffffff00;
     msg.__addr_head = cpu_to_le32(0xfee);
     /* Keep this from original MSI address bits */
     msg.__not_used = irq->msi_addr_last_bits;
@@ -2281,11 +2282,7 @@ static MemTxResult vtd_mem_ir_write(void *opaque, hwaddr addr,
                 " for device sid 0x%04x",
                 to.address, to.data, sid);
 
-    if (dma_memory_write(&address_space_memory, to.address,
-                         &to.data, size)) {
-        VTD_DPRINTF(GENERAL, "error: fail to write 0x%"PRIx64
-                    " value 0x%"PRIx32, to.address, to.data);
-    }
+    apic_get_class()->send_msi(&to);
 
     return MEMTX_OK;
 }
-- 
2.10.0

[Qemu-devel] [PATCH v2 4/7] intel-iommu: exit on invalid configuraton earlier

[Radim Krčmář]

No point in configuring the device if realization is going to fail.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 hw/i386/intel_iommu.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index c39b62b898d8..eb488c14625d 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -2457,6 +2457,15 @@ static void vtd_realize(DeviceState *dev, Error **errp)
 
     VTD_DPRINTF(GENERAL, "");
     x86_iommu->type = TYPE_INTEL;
+
+    /* Currently Intel IOMMU IR only support "kernel-irqchip={off|split}" */
+    if (x86_iommu->intr_supported && kvm_irqchip_in_kernel() &&
+        !kvm_irqchip_is_split()) {
+        error_report("Intel Interrupt Remapping cannot work with "
+                     "kernel-irqchip=on, please use 'split|off'.");
+        exit(1);
+    }
+
     memset(s->vtd_as_by_bus_num, 0, sizeof(s->vtd_as_by_bus_num));
     memory_region_init_io(&s->csrmem, OBJECT(s), &vtd_mem_ops, s,
                           "intel_iommu", DMAR_REG_SIZE);
@@ -2471,14 +2480,6 @@ static void vtd_realize(DeviceState *dev, Error **errp)
     pci_setup_iommu(bus, vtd_host_dma_iommu, dev);
     /* Pseudo address space under root PCI bus. */
     pcms->ioapic_as = vtd_host_dma_iommu(bus, s, Q35_PSEUDO_DEVFN_IOAPIC);
-
-    /* Currently Intel IOMMU IR only support "kernel-irqchip={off|split}" */
-    if (x86_iommu->intr_supported && kvm_irqchip_in_kernel() &&
-        !kvm_irqchip_is_split()) {
-        error_report("Intel Interrupt Remapping cannot work with "
-                     "kernel-irqchip=on, please use 'split|off'.");
-        exit(1);
-    }
 }
 
 static void vtd_class_init(ObjectClass *klass, void *data)
-- 
2.10.0

[Qemu-devel] [PATCH v2 5/7] intel-iommu: add OnOffAuto intr_eim as "eim" property

[Radim Krčmář]

The default (auto) emulates the current behavior.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 hw/i386/intel_iommu.c         | 20 +++++++++++++++++++-
 include/hw/i386/intel_iommu.h |  1 +
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index eb488c14625d..47141cea64f4 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -2011,6 +2011,8 @@ static const MemoryRegionOps vtd_mem_ops = {
 
 static Property vtd_properties[] = {
     DEFINE_PROP_UINT32("version", IntelIOMMUState, version, 0),
+    DEFINE_PROP_ON_OFF_AUTO("eim", IntelIOMMUState, intr_eim,
+                            ON_OFF_AUTO_AUTO),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -2367,7 +2369,11 @@ static void vtd_init(IntelIOMMUState *s)
     s->ecap = VTD_ECAP_QI | VTD_ECAP_IRO;
 
     if (x86_iommu->intr_supported) {
-        s->ecap |= VTD_ECAP_IR | VTD_ECAP_EIM | VTD_ECAP_MHMV;
+        s->ecap |= VTD_ECAP_IR | VTD_ECAP_MHMV;
+        if (s->intr_eim == ON_OFF_AUTO_ON) {
+            s->ecap |= VTD_ECAP_EIM;
+        }
+        assert(s->intr_eim != ON_OFF_AUTO_AUTO);
     }
 
     vtd_reset_context_cache(s);
@@ -2466,6 +2472,18 @@ static void vtd_realize(DeviceState *dev, Error **errp)
         exit(1);
     }
 
+    if (s->intr_eim == ON_OFF_AUTO_ON && !x86_iommu->intr_supported) {
+        error_report("intel-iommu,eim=on cannot be selected without "
+                     "intremap=on.");
+        exit(1);
+    }
+    if (s->intr_eim == ON_OFF_AUTO_AUTO && !x86_iommu->intr_supported) {
+        s->intr_eim = ON_OFF_AUTO_OFF;
+    }
+    if (s->intr_eim == ON_OFF_AUTO_AUTO) {
+        s->intr_eim = ON_OFF_AUTO_ON;
+    }
+
     memset(s->vtd_as_by_bus_num, 0, sizeof(s->vtd_as_by_bus_num));
     memory_region_init_io(&s->csrmem, OBJECT(s), &vtd_mem_ops, s,
                           "intel_iommu", DMAR_REG_SIZE);
diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
index a42dbd745a70..b5ac60927b1f 100644
--- a/include/hw/i386/intel_iommu.h
+++ b/include/hw/i386/intel_iommu.h
@@ -289,6 +289,7 @@ struct IntelIOMMUState {
     dma_addr_t intr_root;           /* Interrupt remapping table pointer */
     uint32_t intr_size;             /* Number of IR table entries */
     bool intr_eime;                 /* Extended interrupt mode enabled */
+    OnOffAuto intr_eim;             /* Toggle for EIM cabability */
 };
 
 /* Find the VTD Address space associated with the given bus pointer,
-- 
2.10.0

[Qemu-devel] [PATCH v2 6/7] intel_iommu: reject broken EIM

[Radim Krčmář]

Cluster x2APIC cannot work without KVM's x2apic API when the maximal
APIC ID is greater than 8 and only KVM's LAPIC can support x2APIC, so we
forbid other APICs and also the old KVM case with less than 9, to
simplify the code.

There is no point in enabling EIM in forbidden APICs, so we keep it
enabled only for the KVM APIC;  unconditionally, because making the
option depend on KVM version would be a maintanance burden.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
v2:
 * adapt to new intr_eim parameter
 * provide first linux version that has x2apic api
 * disable QEMU's LAPIC
---
 hw/i386/intel_iommu.c  | 16 +++++++++++++++-
 target-i386/kvm-stub.c |  5 +++++
 target-i386/kvm.c      | 13 +++++++++++++
 target-i386/kvm_i386.h |  1 +
 4 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 47141cea64f4..b1afe5b133e0 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -32,6 +32,7 @@
 #include "hw/pci-host/q35.h"
 #include "sysemu/kvm.h"
 #include "hw/i386/apic_internal.h"
+#include "kvm_i386.h"
 
 /*#define DEBUG_INTEL_IOMMU*/
 #ifdef DEBUG_INTEL_IOMMU
@@ -2481,7 +2482,20 @@ static void vtd_realize(DeviceState *dev, Error **errp)
         s->intr_eim = ON_OFF_AUTO_OFF;
     }
     if (s->intr_eim == ON_OFF_AUTO_AUTO) {
-        s->intr_eim = ON_OFF_AUTO_ON;
+        s->intr_eim = kvm_irqchip_in_kernel() ? ON_OFF_AUTO_ON
+                                              : ON_OFF_AUTO_OFF;
+    }
+    if (s->intr_eim == ON_OFF_AUTO_ON) {
+        if (kvm_irqchip_in_kernel() && !kvm_enable_x2apic()) {
+            error_report("intel-iommu,eim=on requires support on the KVM side "
+                         "(X2APIC_API, first shipped in v4.7).");
+            exit(1);
+        }
+        if (!kvm_irqchip_in_kernel()) {
+            error_report("intel-iommu,eim=on requires "
+                         "accel=kvm,kernel-irqchip=split.");
+            exit(1);
+        }
     }
 
     memset(s->vtd_as_by_bus_num, 0, sizeof(s->vtd_as_by_bus_num));
diff --git a/target-i386/kvm-stub.c b/target-i386/kvm-stub.c
index cdf15061091d..bda4dc2f0c57 100644
--- a/target-i386/kvm-stub.c
+++ b/target-i386/kvm-stub.c
@@ -25,6 +25,11 @@ bool kvm_has_smm(void)
     return 1;
 }
 
+bool kvm_enable_x2apic(void)
+{
+    return false;
+}
+
 /* This function is only called inside conditionals which we
  * rely on the compiler to optimize out when CONFIG_KVM is not
  * defined.
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index ee1f53e56953..0fd664648665 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -122,6 +122,19 @@ bool kvm_allows_irq0_override(void)
     return !kvm_irqchip_in_kernel() || kvm_has_gsi_routing();
 }
 
+static bool kvm_x2apic_api_set_flags(uint64_t flags)
+{
+    KVMState *s = KVM_STATE(current_machine->accelerator);
+
+    return !kvm_vm_enable_cap(s, KVM_CAP_X2APIC_API, 0, flags);
+}
+
+bool kvm_enable_x2apic(void)
+{
+    return kvm_x2apic_api_set_flags(KVM_X2APIC_API_USE_32BIT_IDS |
+                                    KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK);
+}
+
 static int kvm_get_tsc(CPUState *cs)
 {
     X86CPU *cpu = X86_CPU(cs);
diff --git a/target-i386/kvm_i386.h b/target-i386/kvm_i386.h
index 36407e0a5dc7..5c369b1c5b40 100644
--- a/target-i386/kvm_i386.h
+++ b/target-i386/kvm_i386.h
@@ -43,4 +43,5 @@ int kvm_device_msix_deassign(KVMState *s, uint32_t dev_id);
 
 void kvm_put_apicbase(X86CPU *cpu, uint64_t value);
 
+bool kvm_enable_x2apic(void);
 #endif
-- 
2.10.0

[Qemu-devel] [PATCH v2 7/7] intel-iommu: keep buggy EIM enabled in 2.7 machine type

[Radim Krčmář]

QEMU 2.7 allowed EIM even in configurations that were forbidden in the
last patch because they were not working, like old KVM or userspace
APIC.  In order to keep backward compatibility, we again allow guests to
misbehave in non-obvious ways, and make it the default.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 hw/i386/intel_iommu.c | 6 +++++-
 hw/i386/pc_q35.c      | 2 ++
 include/hw/i386/pc.h  | 2 ++
 3 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index b1afe5b133e0..d6657a361ff9 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -2458,6 +2458,7 @@ static AddressSpace *vtd_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
 static void vtd_realize(DeviceState *dev, Error **errp)
 {
     PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
+    PCMachineClass *pcmc = PC_MACHINE_CLASS(MACHINE_GET_CLASS(pcms));
     PCIBus *bus = pcms->bus;
     IntelIOMMUState *s = INTEL_IOMMU_DEVICE(dev);
     X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(dev);
@@ -2481,11 +2482,14 @@ static void vtd_realize(DeviceState *dev, Error **errp)
     if (s->intr_eim == ON_OFF_AUTO_AUTO && !x86_iommu->intr_supported) {
         s->intr_eim = ON_OFF_AUTO_OFF;
     }
+    if (s->intr_eim == ON_OFF_AUTO_AUTO && pcmc->buggy_intel_iommu_eim) {
+        s->intr_eim = ON_OFF_AUTO_ON;
+    }
     if (s->intr_eim == ON_OFF_AUTO_AUTO) {
         s->intr_eim = kvm_irqchip_in_kernel() ? ON_OFF_AUTO_ON
                                               : ON_OFF_AUTO_OFF;
     }
-    if (s->intr_eim == ON_OFF_AUTO_ON) {
+    if (s->intr_eim == ON_OFF_AUTO_ON && !pcmc->buggy_intel_iommu_eim) {
         if (kvm_irqchip_in_kernel() && !kvm_enable_x2apic()) {
             error_report("intel-iommu,eim=on requires support on the KVM side "
                          "(X2APIC_API, first shipped in v4.7).");
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 0b214f24c977..97f419fbf4dd 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -304,8 +304,10 @@ DEFINE_Q35_MACHINE(v2_8, "pc-q35-2.8", NULL,
 
 static void pc_q35_2_7_machine_options(MachineClass *m)
 {
+    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
     pc_q35_2_8_machine_options(m);
     m->alias = NULL;
+    pcmc->buggy_intel_iommu_eim = true;
     SET_MACHINE_COMPAT(m, PC_COMPAT_2_7);
 }
 
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 47bdf10cfd9b..4bd435f8fa5c 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -143,6 +143,8 @@ struct PCMachineClass {
     bool save_tsc_khz;
     /* generate legacy CPU hotplug AML */
     bool legacy_cpu_hotplug;
+    /* enable buggy Intel-IOMMU EIM by default */
+    bool buggy_intel_iommu_eim;
 };
 
 #define TYPE_PC_MACHINE "generic-pc-machine"
-- 
2.10.0

Re: [Qemu-devel] [PATCH v2 6/7] intel_iommu: reject broken EIM

[Paolo Bonzini]

On 29/09/2016 13:23, Radim Krčmář wrote:
> Cluster x2APIC cannot work without KVM's x2apic API when the maximal
> APIC ID is greater than 8 and only KVM's LAPIC can support x2APIC, so we
> forbid other APICs and also the old KVM case with less than 9, to
> simplify the code.
> 
> There is no point in enabling EIM in forbidden APICs, so we keep it
> enabled only for the KVM APIC;  unconditionally, because making the
> option depend on KVM version would be a maintanance burden.
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
> v2:
>  * adapt to new intr_eim parameter
>  * provide first linux version that has x2apic api
>  * disable QEMU's LAPIC
> ---
>  hw/i386/intel_iommu.c  | 16 +++++++++++++++-
>  target-i386/kvm-stub.c |  5 +++++
>  target-i386/kvm.c      | 13 +++++++++++++
>  target-i386/kvm_i386.h |  1 +
>  4 files changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 47141cea64f4..b1afe5b133e0 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -32,6 +32,7 @@
>  #include "hw/pci-host/q35.h"
>  #include "sysemu/kvm.h"
>  #include "hw/i386/apic_internal.h"
> +#include "kvm_i386.h"
>  
>  /*#define DEBUG_INTEL_IOMMU*/
>  #ifdef DEBUG_INTEL_IOMMU
> @@ -2481,7 +2482,20 @@ static void vtd_realize(DeviceState *dev, Error **errp)
>          s->intr_eim = ON_OFF_AUTO_OFF;
>      }
>      if (s->intr_eim == ON_OFF_AUTO_AUTO) {
> -        s->intr_eim = ON_OFF_AUTO_ON;
> +        s->intr_eim = kvm_irqchip_in_kernel() ? ON_OFF_AUTO_ON
> +                                              : ON_OFF_AUTO_OFF;
> +    }
> +    if (s->intr_eim == ON_OFF_AUTO_ON) {
> +        if (kvm_irqchip_in_kernel() && !kvm_enable_x2apic()) {
> +            error_report("intel-iommu,eim=on requires support on the KVM side "
> +                         "(X2APIC_API, first shipped in v4.7).");
> +            exit(1);

Please use error_setg and return instead (same in patches 4 and 5).

Paolo

> +        }
> +        if (!kvm_irqchip_in_kernel()) {
> +            error_report("intel-iommu,eim=on requires "
> +                         "accel=kvm,kernel-irqchip=split.");
> +            exit(1);
> +        }
>      }
>  
>      memset(s->vtd_as_by_bus_num, 0, sizeof(s->vtd_as_by_bus_num));
> diff --git a/target-i386/kvm-stub.c b/target-i386/kvm-stub.c
> index cdf15061091d..bda4dc2f0c57 100644
> --- a/target-i386/kvm-stub.c
> +++ b/target-i386/kvm-stub.c
> @@ -25,6 +25,11 @@ bool kvm_has_smm(void)
>      return 1;
>  }
>  
> +bool kvm_enable_x2apic(void)
> +{
> +    return false;
> +}
> +
>  /* This function is only called inside conditionals which we
>   * rely on the compiler to optimize out when CONFIG_KVM is not
>   * defined.
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index ee1f53e56953..0fd664648665 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -122,6 +122,19 @@ bool kvm_allows_irq0_override(void)
>      return !kvm_irqchip_in_kernel() || kvm_has_gsi_routing();
>  }
>  
> +static bool kvm_x2apic_api_set_flags(uint64_t flags)
> +{
> +    KVMState *s = KVM_STATE(current_machine->accelerator);
> +
> +    return !kvm_vm_enable_cap(s, KVM_CAP_X2APIC_API, 0, flags);
> +}
> +
> +bool kvm_enable_x2apic(void)
> +{
> +    return kvm_x2apic_api_set_flags(KVM_X2APIC_API_USE_32BIT_IDS |
> +                                    KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK);
> +}
> +
>  static int kvm_get_tsc(CPUState *cs)
>  {
>      X86CPU *cpu = X86_CPU(cs);
> diff --git a/target-i386/kvm_i386.h b/target-i386/kvm_i386.h
> index 36407e0a5dc7..5c369b1c5b40 100644
> --- a/target-i386/kvm_i386.h
> +++ b/target-i386/kvm_i386.h
> @@ -43,4 +43,5 @@ int kvm_device_msix_deassign(KVMState *s, uint32_t dev_id);
>  
>  void kvm_put_apicbase(X86CPU *cpu, uint64_t value);
>  
> +bool kvm_enable_x2apic(void);
>  #endif
> 

Re: [Qemu-devel] [PATCH v2 7/7] intel-iommu: keep buggy EIM enabled in 2.7 machine type

[Paolo Bonzini]

On 29/09/2016 13:23, Radim Krčmář wrote:
> QEMU 2.7 allowed EIM even in configurations that were forbidden in the
> last patch because they were not working, like old KVM or userspace
> APIC.  In order to keep backward compatibility, we again allow guests to
> misbehave in non-obvious ways, and make it the default.
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>

Ugh, I misremembered that VTD_ECAP_EIM was not set in 2.7. :(  Perhaps
it's better to drop this patch...

Paolo

> ---
>  hw/i386/intel_iommu.c | 6 +++++-
>  hw/i386/pc_q35.c      | 2 ++
>  include/hw/i386/pc.h  | 2 ++
>  3 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index b1afe5b133e0..d6657a361ff9 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -2458,6 +2458,7 @@ static AddressSpace *vtd_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
>  static void vtd_realize(DeviceState *dev, Error **errp)
>  {
>      PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
> +    PCMachineClass *pcmc = PC_MACHINE_CLASS(MACHINE_GET_CLASS(pcms));
>      PCIBus *bus = pcms->bus;
>      IntelIOMMUState *s = INTEL_IOMMU_DEVICE(dev);
>      X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(dev);
> @@ -2481,11 +2482,14 @@ static void vtd_realize(DeviceState *dev, Error **errp)
>      if (s->intr_eim == ON_OFF_AUTO_AUTO && !x86_iommu->intr_supported) {
>          s->intr_eim = ON_OFF_AUTO_OFF;
>      }
> +    if (s->intr_eim == ON_OFF_AUTO_AUTO && pcmc->buggy_intel_iommu_eim) {
> +        s->intr_eim = ON_OFF_AUTO_ON;
> +    }
>      if (s->intr_eim == ON_OFF_AUTO_AUTO) {
>          s->intr_eim = kvm_irqchip_in_kernel() ? ON_OFF_AUTO_ON
>                                                : ON_OFF_AUTO_OFF;
>      }
> -    if (s->intr_eim == ON_OFF_AUTO_ON) {
> +    if (s->intr_eim == ON_OFF_AUTO_ON && !pcmc->buggy_intel_iommu_eim) {
>          if (kvm_irqchip_in_kernel() && !kvm_enable_x2apic()) {
>              error_report("intel-iommu,eim=on requires support on the KVM side "
>                           "(X2APIC_API, first shipped in v4.7).");
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 0b214f24c977..97f419fbf4dd 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -304,8 +304,10 @@ DEFINE_Q35_MACHINE(v2_8, "pc-q35-2.8", NULL,
>  
>  static void pc_q35_2_7_machine_options(MachineClass *m)
>  {
> +    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
>      pc_q35_2_8_machine_options(m);
>      m->alias = NULL;
> +    pcmc->buggy_intel_iommu_eim = true;
>      SET_MACHINE_COMPAT(m, PC_COMPAT_2_7);
>  }
>  
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 47bdf10cfd9b..4bd435f8fa5c 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -143,6 +143,8 @@ struct PCMachineClass {
>      bool save_tsc_khz;
>      /* generate legacy CPU hotplug AML */
>      bool legacy_cpu_hotplug;
> +    /* enable buggy Intel-IOMMU EIM by default */
> +    bool buggy_intel_iommu_eim;
>  };
>  
>  #define TYPE_PC_MACHINE "generic-pc-machine"
> 

Re: [Qemu-devel] [PATCH v2 1/7] apic: add global apic_get_class()

[Eduardo Habkost]

On Thu, Sep 29, 2016 at 01:23:23PM +0200, Radim Krčmář wrote:
> Every configuration has only up to one APIC class and we'll be extending
> the class with a function that can be called without an instanced
> object, so a direct access to the class is convenient.
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
> v2: assert() instead of error_report() and exit() [Peter]
> ---
>  hw/intc/apic_common.c           | 11 +++++++++++
>  include/hw/i386/apic_internal.h |  3 +++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
> index 14ac43c18666..a766f0efc805 100644
> --- a/hw/intc/apic_common.c
> +++ b/hw/intc/apic_common.c
> @@ -18,6 +18,7 @@
>   * License along with this library; if not, see <http://www.gnu.org/licenses/>
>   */
>  #include "qemu/osdep.h"
> +#include "qemu/error-report.h"
>  #include "qapi/error.h"
>  #include "qemu-common.h"
>  #include "cpu.h"
> @@ -296,6 +297,13 @@ static int apic_load_old(QEMUFile *f, void *opaque, int version_id)
>  
>  static const VMStateDescription vmstate_apic_common;
>  
> +APICCommonClass *apic_class;
> +
> +APICCommonClass *apic_get_class(void)
> +{
> +    return apic_class;
> +}
> +

This will break in case we need to look at the APIC class before
realizing the CPUs for any reason. Instead of adding a global
variable, what about moving the class name logic that's already
inside x86_cpu_apic_create() to the apic_get_class() function?
Then x86_cpu_apic_create() can simply call apic_get_class() too.

>  static void apic_common_realize(DeviceState *dev, Error **errp)
>  {
>      APICCommonState *s = APIC_COMMON(dev);
> @@ -306,6 +314,9 @@ static void apic_common_realize(DeviceState *dev, Error **errp)
>      info = APIC_COMMON_GET_CLASS(s);
>      info->realize(dev, errp);
>  
> +    assert(apic_class == info || !apic_class);
> +    apic_class = info;
> +
>      /* Note: We need at least 1M to map the VAPIC option ROM */
>      if (!vapic && s->vapic_control & VAPIC_ENABLE_MASK &&
>          ram_size >= 1024 * 1024) {
> diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
> index 06c4e9f6f95b..9ba8a5c87f90 100644
> --- a/include/hw/i386/apic_internal.h
> +++ b/include/hw/i386/apic_internal.h
> @@ -222,4 +222,7 @@ static inline int apic_get_bit(uint32_t *tab, int index)
>      return !!(tab[i] & mask);
>  }
>  
> +
> +APICCommonClass *apic_get_class(void);
> +
>  #endif /* QEMU_APIC_INTERNAL_H */
> -- 
> 2.10.0
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 7/7] intel-iommu: keep buggy EIM enabled in 2.7 machine type

[Eduardo Habkost]

On Thu, Sep 29, 2016 at 01:23:29PM +0200, Radim Krčmář wrote:
> QEMU 2.7 allowed EIM even in configurations that were forbidden in the
> last patch because they were not working, like old KVM or userspace
> APIC.  In order to keep backward compatibility, we again allow guests to
> misbehave in non-obvious ways, and make it the default.
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>

If you break compatibility and fix it in separate patches, you
break bisectability (even for people that are bisecting bugs
unrelated to EIM).

(But I still don't understand if patch 6/7 really breaks
anything, or not.)

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 6/7] intel_iommu: reject broken EIM

[Igor Mammedov]

On Thu, 29 Sep 2016 13:23:28 +0200
Radim Krčmář <rkrcmar@redhat.com> wrote:

> Cluster x2APIC cannot work without KVM's x2apic API when the maximal
> APIC ID is greater than 8 and only KVM's LAPIC can support x2APIC, so we
> forbid other APICs and also the old KVM case with less than 9, to
> simplify the code.
> 
> There is no point in enabling EIM in forbidden APICs, so we keep it
> enabled only for the KVM APIC;  unconditionally, because making the
> option depend on KVM version would be a maintanance burden.
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
> v2:
>  * adapt to new intr_eim parameter
>  * provide first linux version that has x2apic api
>  * disable QEMU's LAPIC
> ---
>  hw/i386/intel_iommu.c  | 16 +++++++++++++++-
>  target-i386/kvm-stub.c |  5 +++++
>  target-i386/kvm.c      | 13 +++++++++++++
>  target-i386/kvm_i386.h |  1 +
>  4 files changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 47141cea64f4..b1afe5b133e0 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -32,6 +32,7 @@
>  #include "hw/pci-host/q35.h"
>  #include "sysemu/kvm.h"
>  #include "hw/i386/apic_internal.h"
> +#include "kvm_i386.h"
>  
>  /*#define DEBUG_INTEL_IOMMU*/
>  #ifdef DEBUG_INTEL_IOMMU
> @@ -2481,7 +2482,20 @@ static void vtd_realize(DeviceState *dev, Error **errp)
>          s->intr_eim = ON_OFF_AUTO_OFF;
>      }
>      if (s->intr_eim == ON_OFF_AUTO_AUTO) {
> -        s->intr_eim = ON_OFF_AUTO_ON;
> +        s->intr_eim = kvm_irqchip_in_kernel() ? ON_OFF_AUTO_ON
> +                                              : ON_OFF_AUTO_OFF;
> +    }
> +    if (s->intr_eim == ON_OFF_AUTO_ON) {
> +        if (kvm_irqchip_in_kernel() && !kvm_enable_x2apic()) {
> +            error_report("intel-iommu,eim=on requires support on the KVM side "
> +                         "(X2APIC_API, first shipped in v4.7).");
> +            exit(1);
> +        }
> +        if (!kvm_irqchip_in_kernel()) {
> +            error_report("intel-iommu,eim=on requires "
> +                         "accel=kvm,kernel-irqchip=split.");
> +            exit(1);
> +        }
>      }
>  
>      memset(s->vtd_as_by_bus_num, 0, sizeof(s->vtd_as_by_bus_num));
> diff --git a/target-i386/kvm-stub.c b/target-i386/kvm-stub.c
> index cdf15061091d..bda4dc2f0c57 100644
> --- a/target-i386/kvm-stub.c
> +++ b/target-i386/kvm-stub.c
> @@ -25,6 +25,11 @@ bool kvm_has_smm(void)
>      return 1;
>  }
>  
> +bool kvm_enable_x2apic(void)
> +{
> +    return false;
> +}
> +
>  /* This function is only called inside conditionals which we
>   * rely on the compiler to optimize out when CONFIG_KVM is not
>   * defined.
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index ee1f53e56953..0fd664648665 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -122,6 +122,19 @@ bool kvm_allows_irq0_override(void)
>      return !kvm_irqchip_in_kernel() || kvm_has_gsi_routing();
>  }
>  
> +static bool kvm_x2apic_api_set_flags(uint64_t flags)
> +{
> +    KVMState *s = KVM_STATE(current_machine->accelerator);
> +
> +    return !kvm_vm_enable_cap(s, KVM_CAP_X2APIC_API, 0, flags);
> +}
it's called only from one place so why not to inline it in the caller?

> +
> +bool kvm_enable_x2apic(void)
how about renaming it to kvm_has_x2apic() and making it so that
the second and following invocations wouldn't uselessly call
kvm_vm_enable_cap()

> +{
> +    return kvm_x2apic_api_set_flags(KVM_X2APIC_API_USE_32BIT_IDS |
> +                                    KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK);
> +}
> +
>  static int kvm_get_tsc(CPUState *cs)
>  {
>      X86CPU *cpu = X86_CPU(cs);
> diff --git a/target-i386/kvm_i386.h b/target-i386/kvm_i386.h
> index 36407e0a5dc7..5c369b1c5b40 100644
> --- a/target-i386/kvm_i386.h
> +++ b/target-i386/kvm_i386.h
> @@ -43,4 +43,5 @@ int kvm_device_msix_deassign(KVMState *s, uint32_t dev_id);
>  
>  void kvm_put_apicbase(X86CPU *cpu, uint64_t value);
>  
> +bool kvm_enable_x2apic(void);
>  #endif

Re: [Qemu-devel] [PATCH v2 1/7] apic: add global apic_get_class()

[Radim Krčmář]

2016-09-29 11:53-0300, Eduardo Habkost:
> On Thu, Sep 29, 2016 at 01:23:23PM +0200, Radim Krčmář wrote:
>> Every configuration has only up to one APIC class and we'll be extending
>> the class with a function that can be called without an instanced
>> object, so a direct access to the class is convenient.
>> 
>> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
>> ---
>> v2: assert() instead of error_report() and exit() [Peter]
>> ---
>> diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
>> @@ -296,6 +297,13 @@ static int apic_load_old(QEMUFile *f, void *opaque, int version_id)
>>  
>>  static const VMStateDescription vmstate_apic_common;
>>  
>> +APICCommonClass *apic_class;
>> +
>> +APICCommonClass *apic_get_class(void)
>> +{
>> +    return apic_class;
>> +}
>> +
> 
> This will break in case we need to look at the APIC class before
> realizing the CPUs for any reason. Instead of adding a global
> variable, what about moving the class name logic that's already
> inside x86_cpu_apic_create() to the apic_get_class() function?
> Then x86_cpu_apic_create() can simply call apic_get_class() too.

Sure, it'll be nicer, thanks.
ia64 seems dead, so no issues with making the code x86 specific.

Re: [Qemu-devel] [PATCH v2 6/7] intel_iommu: reject broken EIM

[Igor Mammedov]

On Thu, 29 Sep 2016 15:18:36 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 29/09/2016 13:23, Radim Krčmář wrote:
> > Cluster x2APIC cannot work without KVM's x2apic API when the maximal
> > APIC ID is greater than 8 and only KVM's LAPIC can support x2APIC, so we
> > forbid other APICs and also the old KVM case with less than 9, to
> > simplify the code.
> > 
> > There is no point in enabling EIM in forbidden APICs, so we keep it
> > enabled only for the KVM APIC;  unconditionally, because making the
> > option depend on KVM version would be a maintanance burden.
> > 
> > Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> > ---
> > v2:
> >  * adapt to new intr_eim parameter
> >  * provide first linux version that has x2apic api
> >  * disable QEMU's LAPIC
> > ---
> >  hw/i386/intel_iommu.c  | 16 +++++++++++++++-
> >  target-i386/kvm-stub.c |  5 +++++
> >  target-i386/kvm.c      | 13 +++++++++++++
> >  target-i386/kvm_i386.h |  1 +
> >  4 files changed, 34 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > index 47141cea64f4..b1afe5b133e0 100644
> > --- a/hw/i386/intel_iommu.c
> > +++ b/hw/i386/intel_iommu.c
> > @@ -32,6 +32,7 @@
> >  #include "hw/pci-host/q35.h"
> >  #include "sysemu/kvm.h"
> >  #include "hw/i386/apic_internal.h"
> > +#include "kvm_i386.h"
> >  
> >  /*#define DEBUG_INTEL_IOMMU*/
> >  #ifdef DEBUG_INTEL_IOMMU
> > @@ -2481,7 +2482,20 @@ static void vtd_realize(DeviceState *dev, Error **errp)
> >          s->intr_eim = ON_OFF_AUTO_OFF;
> >      }
> >      if (s->intr_eim == ON_OFF_AUTO_AUTO) {
> > -        s->intr_eim = ON_OFF_AUTO_ON;
> > +        s->intr_eim = kvm_irqchip_in_kernel() ? ON_OFF_AUTO_ON
> > +                                              : ON_OFF_AUTO_OFF;
> > +    }
> > +    if (s->intr_eim == ON_OFF_AUTO_ON) {
> > +        if (kvm_irqchip_in_kernel() && !kvm_enable_x2apic()) {
> > +            error_report("intel-iommu,eim=on requires support on the KVM side "
> > +                         "(X2APIC_API, first shipped in v4.7).");
> > +            exit(1);  
> 
> Please use error_setg and return instead (same in patches 4 and 5).
Radim's version is consistent with error handling used throughout the file.
If we are to use preferred error_setg() here that preceding cleanup
patch is in order to convert error_reports to error_setg.

> 
> Paolo
> 
> > +        }
> > +        if (!kvm_irqchip_in_kernel()) {
> > +            error_report("intel-iommu,eim=on requires "
> > +                         "accel=kvm,kernel-irqchip=split.");
this prints:
 -device intel-iommu,intremap=on,eim=on: intel-iommu,eim=on requires accel=kvm,kernel-irqchip=split
so 'intel-iommu,' not really needed, the same would happen with error_setg()

> > +            exit(1);
> > +        }
> >      }
[...]

Re: [Qemu-devel] [PATCH v2 7/7] intel-iommu: keep buggy EIM enabled in 2.7 machine type

[Radim Krčmář]

2016-09-29 15:19+0200, Paolo Bonzini:
> On 29/09/2016 13:23, Radim Krčmář wrote:
>> QEMU 2.7 allowed EIM even in configurations that were forbidden in the
>> last patch because they were not working, like old KVM or userspace
>> APIC.  In order to keep backward compatibility, we again allow guests to
>> misbehave in non-obvious ways, and make it the default.
>> 
>> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> 
> Ugh, I misremembered that VTD_ECAP_EIM was not set in 2.7. :(  Perhaps
> it's better to drop this patch...

I think that adding this backward compatibility hack into code that is
supposed to be developed is not a good idea.


2016-09-29 12:01-0300, Eduardo Habkost:
> If you break compatibility and fix it in separate patches, you
> break bisectability (even for people that are bisecting bugs
> unrelated to EIM).

I'd keep it as a separate patch and let maintainers decide whether they
want to squish or drop it.

> (But I still don't understand if patch 6/7 really breaks
> anything, or not.)

Nothing useful.
It "breaks" three cases:

1) If user configured

     -machine kernel_irqchip=off -device intel_iommu,intremap=on

   QEMU 2.7 pc-q35-2.7 enabled (broken) EIM, but 2.8 wouldn't, leading
   to a different machine.

   (The same with new KVM and split irqchip.)

2) If user had old KVM and configured

     -machine kernel_irqchip=split -device intel_iommu,intremap=on

   QEMU 2.7 pc-q35-2.7 enabled (broken) EIM, but after offline migration
   to 2.8, QEMU would refuse to start.

3) If user started a pc-q35-2.7 with QEMU 2.8 on a new KVM, then they
   could use cluster x2APIC without a problem, but the guest wouldn't
   work after offline migration to QEMU 2.7 (I'm not sure if this case
   is supported).

Luckily, the intel-iommu device doesn't support live migration. :)

Re: [Qemu-devel] [PATCH v2 6/7] intel_iommu: reject broken EIM

[Radim Krčmář]

2016-09-29 18:06+0200, Igor Mammedov:
> On Thu, 29 Sep 2016 15:18:36 +0200
> Paolo Bonzini <pbonzini@redhat.com> wrote:
>> On 29/09/2016 13:23, Radim Krčmář wrote:
>> > Cluster x2APIC cannot work without KVM's x2apic API when the maximal
>> > APIC ID is greater than 8 and only KVM's LAPIC can support x2APIC, so we
>> > forbid other APICs and also the old KVM case with less than 9, to
>> > simplify the code.
>> > 
>> > There is no point in enabling EIM in forbidden APICs, so we keep it
>> > enabled only for the KVM APIC;  unconditionally, because making the
>> > option depend on KVM version would be a maintanance burden.
>> > 
>> > Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
>> > ---
>> > v2:
>> >  * adapt to new intr_eim parameter
>> >  * provide first linux version that has x2apic api
>> >  * disable QEMU's LAPIC
>> > ---
>> > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>> > @@ -2481,7 +2482,20 @@ static void vtd_realize(DeviceState *dev, Error **errp)
>> >          s->intr_eim = ON_OFF_AUTO_OFF;
>> >      }
>> >      if (s->intr_eim == ON_OFF_AUTO_AUTO) {
>> > -        s->intr_eim = ON_OFF_AUTO_ON;
>> > +        s->intr_eim = kvm_irqchip_in_kernel() ? ON_OFF_AUTO_ON
>> > +                                              : ON_OFF_AUTO_OFF;
>> > +    }
>> > +    if (s->intr_eim == ON_OFF_AUTO_ON) {
>> > +        if (kvm_irqchip_in_kernel() && !kvm_enable_x2apic()) {
>> > +            error_report("intel-iommu,eim=on requires support on the KVM side "
>> > +                         "(X2APIC_API, first shipped in v4.7).");
>> > +            exit(1);  
>> 
>> Please use error_setg and return instead (same in patches 4 and 5).
> Radim's version is consistent with error handling used throughout the file.
> If we are to use preferred error_setg() here that preceding cleanup
> patch is in order to convert error_reports to error_setg.

There is one more error_report() in the file and it doesn't have
"Error **" -- I'll leave it be and change the rest.
It amounts to one extra patch that before [4/7] (could be squashed too).

>> > +        }
>> > +        if (!kvm_irqchip_in_kernel()) {
>> > +            error_report("intel-iommu,eim=on requires "
>> > +                         "accel=kvm,kernel-irqchip=split.");
> this prints:
>  -device intel-iommu,intremap=on,eim=on: intel-iommu,eim=on requires accel=kvm,kernel-irqchip=split
> so 'intel-iommu,' not really needed, the same would happen with error_setg()

Yeah, really unreadable, thanks.

Re: [Qemu-devel] [PATCH v2 6/7] intel_iommu: reject broken EIM

[Paolo Bonzini]

On 29/09/2016 18:56, Radim Krčmář wrote:
> 2016-09-29 18:06+0200, Igor Mammedov:
>> On Thu, 29 Sep 2016 15:18:36 +0200
>> Paolo Bonzini <pbonzini@redhat.com> wrote:
>>> On 29/09/2016 13:23, Radim Krčmář wrote:
>>>> Cluster x2APIC cannot work without KVM's x2apic API when the maximal
>>>> APIC ID is greater than 8 and only KVM's LAPIC can support x2APIC, so we
>>>> forbid other APICs and also the old KVM case with less than 9, to
>>>> simplify the code.
>>>>
>>>> There is no point in enabling EIM in forbidden APICs, so we keep it
>>>> enabled only for the KVM APIC;  unconditionally, because making the
>>>> option depend on KVM version would be a maintanance burden.
>>>>
>>>> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
>>>> ---
>>>> v2:
>>>>  * adapt to new intr_eim parameter
>>>>  * provide first linux version that has x2apic api
>>>>  * disable QEMU's LAPIC
>>>> ---
>>>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>>>> @@ -2481,7 +2482,20 @@ static void vtd_realize(DeviceState *dev, Error **errp)
>>>>          s->intr_eim = ON_OFF_AUTO_OFF;
>>>>      }
>>>>      if (s->intr_eim == ON_OFF_AUTO_AUTO) {
>>>> -        s->intr_eim = ON_OFF_AUTO_ON;
>>>> +        s->intr_eim = kvm_irqchip_in_kernel() ? ON_OFF_AUTO_ON
>>>> +                                              : ON_OFF_AUTO_OFF;
>>>> +    }
>>>> +    if (s->intr_eim == ON_OFF_AUTO_ON) {
>>>> +        if (kvm_irqchip_in_kernel() && !kvm_enable_x2apic()) {
>>>> +            error_report("intel-iommu,eim=on requires support on the KVM side "
>>>> +                         "(X2APIC_API, first shipped in v4.7).");
>>>> +            exit(1);  
>>>
>>> Please use error_setg and return instead (same in patches 4 and 5).
>> Radim's version is consistent with error handling used throughout the file.
>> If we are to use preferred error_setg() here that preceding cleanup
>> patch is in order to convert error_reports to error_setg.
> 
> There is one more error_report() in the file and it doesn't have
> "Error **" -- I'll leave it be and change the rest.
> It amounts to one extra patch that before [4/7] (could be squashed too).

No problem then, keep using error_report I guess.

Paolo


>>>> +        }
>>>> +        if (!kvm_irqchip_in_kernel()) {
>>>> +            error_report("intel-iommu,eim=on requires "
>>>> +                         "accel=kvm,kernel-irqchip=split.");
>> this prints:
>>  -device intel-iommu,intremap=on,eim=on: intel-iommu,eim=on requires accel=kvm,kernel-irqchip=split
>> so 'intel-iommu,' not really needed, the same would happen with error_setg()
> 
> Yeah, really unreadable, thanks.
> 

Re: [Qemu-devel] [PATCH v2 5/7] intel-iommu: add OnOffAuto intr_eim as "eim" property

[Peter Xu]

On Thu, Sep 29, 2016 at 01:23:27PM +0200, Radim Krčmář wrote:
> The default (auto) emulates the current behavior.
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  hw/i386/intel_iommu.c         | 20 +++++++++++++++++++-
>  include/hw/i386/intel_iommu.h |  1 +
>  2 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index eb488c14625d..47141cea64f4 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -2011,6 +2011,8 @@ static const MemoryRegionOps vtd_mem_ops = {
>  
>  static Property vtd_properties[] = {
>      DEFINE_PROP_UINT32("version", IntelIOMMUState, version, 0),
> +    DEFINE_PROP_ON_OFF_AUTO("eim", IntelIOMMUState, intr_eim,
> +                            ON_OFF_AUTO_AUTO),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> @@ -2367,7 +2369,11 @@ static void vtd_init(IntelIOMMUState *s)
>      s->ecap = VTD_ECAP_QI | VTD_ECAP_IRO;
>  
>      if (x86_iommu->intr_supported) {
> -        s->ecap |= VTD_ECAP_IR | VTD_ECAP_EIM | VTD_ECAP_MHMV;
> +        s->ecap |= VTD_ECAP_IR | VTD_ECAP_MHMV;
> +        if (s->intr_eim == ON_OFF_AUTO_ON) {
> +            s->ecap |= VTD_ECAP_EIM;
> +        }
> +        assert(s->intr_eim != ON_OFF_AUTO_AUTO);
>      }
>  
>      vtd_reset_context_cache(s);
> @@ -2466,6 +2472,18 @@ static void vtd_realize(DeviceState *dev, Error **errp)
>          exit(1);
>      }
>  
> +    if (s->intr_eim == ON_OFF_AUTO_ON && !x86_iommu->intr_supported) {
> +        error_report("intel-iommu,eim=on cannot be selected without "
> +                     "intremap=on.");
> +        exit(1);
> +    }
> +    if (s->intr_eim == ON_OFF_AUTO_AUTO && !x86_iommu->intr_supported) {
> +        s->intr_eim = ON_OFF_AUTO_OFF;
> +    }
> +    if (s->intr_eim == ON_OFF_AUTO_AUTO) {
> +        s->intr_eim = ON_OFF_AUTO_ON;
> +    }

A single if() instead of above two might be nicer:

    if (s->intr_eim == ON_OFF_AUTO_AUTO) {
        e->intr_eim = x86_iommu->intr_supported ?
            ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
    }

Otherwise good to me.

Thanks,

-- peterx

Re: [Qemu-devel] [PATCH v2 7/7] intel-iommu: keep buggy EIM enabled in 2.7 machine type

[Peter Xu]

On Thu, Sep 29, 2016 at 01:23:29PM +0200, Radim Krčmář wrote:

[...]

> @@ -2481,11 +2482,14 @@ static void vtd_realize(DeviceState *dev, Error **errp)
>      if (s->intr_eim == ON_OFF_AUTO_AUTO && !x86_iommu->intr_supported) {
>          s->intr_eim = ON_OFF_AUTO_OFF;
>      }
> +    if (s->intr_eim == ON_OFF_AUTO_AUTO && pcmc->buggy_intel_iommu_eim) {
> +        s->intr_eim = ON_OFF_AUTO_ON;
> +    }
>      if (s->intr_eim == ON_OFF_AUTO_AUTO) {
>          s->intr_eim = kvm_irqchip_in_kernel() ? ON_OFF_AUTO_ON
>                                                : ON_OFF_AUTO_OFF;
>      }
> -    if (s->intr_eim == ON_OFF_AUTO_ON) {
> +    if (s->intr_eim == ON_OFF_AUTO_ON && !pcmc->buggy_intel_iommu_eim) {
>          if (kvm_irqchip_in_kernel() && !kvm_enable_x2apic()) {
>              error_report("intel-iommu,eim=on requires support on the KVM side "
>                           "(X2APIC_API, first shipped in v4.7).");

No matter how we would treat this patch, I see that we are stacking up
if clauses here. So IMHO maybe it's time to award EIM a new routine:

  int vtd_eim_detect(IntelIOMMUState *, Error **errp);

And squash all these conditions in. Then in vtd_realize():

  if (vtd_eim_detect(s, errp)) {
      return;
  }

Thanks,

-- peterx

Re: [Qemu-devel] [PATCH v2 5/7] intel-iommu: add OnOffAuto intr_eim as "eim" property

[Radim Krčmář]

2016-09-30 13:13+0800, Peter Xu:
> On Thu, Sep 29, 2016 at 01:23:27PM +0200, Radim Krčmář wrote:
>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>> @@ -2466,6 +2472,18 @@ static void vtd_realize(DeviceState *dev, Error **errp)
>>          exit(1);
>>      }
>>  
>> +    if (s->intr_eim == ON_OFF_AUTO_ON && !x86_iommu->intr_supported) {
>> +        error_report("intel-iommu,eim=on cannot be selected without "
>> +                     "intremap=on.");
>> +        exit(1);
>> +    }
>> +    if (s->intr_eim == ON_OFF_AUTO_AUTO && !x86_iommu->intr_supported) {
>> +        s->intr_eim = ON_OFF_AUTO_OFF;
>> +    }
>> +    if (s->intr_eim == ON_OFF_AUTO_AUTO) {
>> +        s->intr_eim = ON_OFF_AUTO_ON;
>> +    }
> 
> A single if() instead of above two might be nicer:
> 
>     if (s->intr_eim == ON_OFF_AUTO_AUTO) {
>         e->intr_eim = x86_iommu->intr_supported ?
>             ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
>     }

Hm, it'll end up looking like

    if (s->intr_eim == ON_OFF_AUTO_AUTO) {
        e->intr_eim = (x86_iommu->intr_supported && kvm_irqchip_in_kernel()) ||
                      pcmc->buggy_intel_iommu_eim ?
                                              ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
    }

It's harder to read, but less LOC ... I'll use it, thanks.

Re: [Qemu-devel] [PATCH v2 7/7] intel-iommu: keep buggy EIM enabled in 2.7 machine type

[Radim Krčmář]

2016-09-30 13:40+0800, Peter Xu:
> On Thu, Sep 29, 2016 at 01:23:29PM +0200, Radim Krčmář wrote:
> 
> [...]
> 
>> @@ -2481,11 +2482,14 @@ static void vtd_realize(DeviceState *dev, Error **errp)
>>      if (s->intr_eim == ON_OFF_AUTO_AUTO && !x86_iommu->intr_supported) {
>>          s->intr_eim = ON_OFF_AUTO_OFF;
>>      }
>> +    if (s->intr_eim == ON_OFF_AUTO_AUTO && pcmc->buggy_intel_iommu_eim) {
>> +        s->intr_eim = ON_OFF_AUTO_ON;
>> +    }
>>      if (s->intr_eim == ON_OFF_AUTO_AUTO) {
>>          s->intr_eim = kvm_irqchip_in_kernel() ? ON_OFF_AUTO_ON
>>                                                : ON_OFF_AUTO_OFF;
>>      }
>> -    if (s->intr_eim == ON_OFF_AUTO_ON) {
>> +    if (s->intr_eim == ON_OFF_AUTO_ON && !pcmc->buggy_intel_iommu_eim) {
>>          if (kvm_irqchip_in_kernel() && !kvm_enable_x2apic()) {
>>              error_report("intel-iommu,eim=on requires support on the KVM side "
>>                           "(X2APIC_API, first shipped in v4.7).");
> 
> No matter how we would treat this patch, I see that we are stacking up
> if clauses here. So IMHO maybe it's time to award EIM a new routine:
> 
>   int vtd_eim_detect(IntelIOMMUState *, Error **errp);
> 
> And squash all these conditions in. Then in vtd_realize():
> 
>   if (vtd_eim_detect(s, errp)) {
>       return;
>   }

Yeah, thanks.