[Qemu-devel] [PATCH v5 00/18] IOMMU: Enable interrupt remapping for Intel IOMMU

[Qemu-devel] [PATCH v5 00/18] IOMMU: Enable interrupt remapping for Intel IOMMU

[Peter Xu]

v5 changes:
- patch 10: add vector checking for IOAPIC interrupts (this may help
  debug in the future, will only generate warning if specify
  IOMMU_DEBUG)
- patch 13: replace error_report() with a trace. [Jan]
- patch 14: rename parameter "intr" to "intremap", to be aligned
  with kernel parameter [Jan]
- patch 15: fix comments for vtd_iec_notify_fn
- patch 17 & 18 (added): fix issue when IR enabled with devices
  using level-triggered interrupts, like e1000. Adding it to the end
  of series, since this issue never happen without IR.

  Patch 17 adds read-only check for IOAPIC entries.
  Patch 18 clears remote IRR bit when entry configured as
  edge-triggered.

v4 changes (all patch number corresponds to v3):
- add one patch at the start of v3 series: I missed to send the
  first patch in v3. adding it in. [Jan]
- patch 9: add support for compatible mode (no reason not to support
  it, if not, we will get some warnings when using split irqchip)
- patch 11: further simplify ioapic_update_kvm_routes() using the
  helper function.
- patch 12: tweak on kvm_arch_fixup_msi_route() rather than
  ioapic_update_kvm_routes() only. [Radim]
- add patch 15: introduce IEC (Interrupt Entry Cache) invalidation
  notifier list. We can register to this list if we want to be
  notified when we got IR invalidation requests [Radim]
- add patch 16: let IOAPIC the first consumer for the above IEC
  notifier list. [Radim]
- several other trivial fixes (like moving some defines from .c to
  .h, moving several lines of changes from one patch to another to
  make it make more sense, etc.)

v3 changes (all patch numbers corresponds to v2):
- patch 1 (-> v3 patch 13)
  - move to the end of series [Alex]
- patch 10 (dropped)
  - drop this one, since re-worked on IOAPIC support, so we do not
    need this any more.
- patch 12 (-> v3 patch 10)
  - leverage MSI path for IOAPIC IR [Jan]
- patch 13 (v3 -> patch 9)
  - remove vtd_interrupt_remap_msi() declaration by reordering the
    functions [mst]
  - vtd_generate_msi_message(): init msg using {}, remove FIXME
    [mst]
- new patches
  - v3 patch 11: introduce ioapic_entry_parse() helper function
  - v3 patch 12: add support for kernel-irqchip=split. This needs
    more reviews, logically this should enable lots of things:
	splitted irqchip, irqfd, vhost, and irqfd support for
	passthrough devices (not tested). Please refer to the patch for
	more information.

v2 changes:
- patch 1
  - rename "int_remap" to "intr" in several places [Marcel]
  - remove "Intel" specific words in desc or commit message, prepare
    itself with further AMD support [Marcel]
  - avoid using object_property_get_bool() [Marcel]
- patch 5
  - use PCI bus number 0xff rather than 0xf0 for the IOAPIC scope
    definition. (please let me know if anyone knows how I can avoid
	user using PCI bus number 0xff... TIA)
- patch 11
  - fix comments [Marcel]
- all
  - remove intr_supported variable [Marcel]

This patchset provide very basic functionalities for interrupt
remapping (IR) support of the emulated Intel IOMMU device.

By default, IR is disabled to be better compatible with current
QEMU. To enable IR, we can using the following command to boot a
IR-supported VM with virtio-net device with vhost (still do not
support kvm-ioapic, so we need to specify kernel-irqchip={split|off}
here):

$ qemu-system-x86_64 -M q35,iommu=on,intr=on,kernel-irqchip=split \
     -enable-kvm -m 1024 \
	 -netdev tap,id=net0,vhost=on \
     -device virtio-net-pci,netdev=user.0 \
     -monitor telnet::3333,server,nowait \
	 /var/lib/libvirt/images/vm1.qcow2

When guest boots, we can verify whether IR enabled by grepping the
dmesg like:

[root@localhost ~]# journalctl -k | grep "DMAR-IR"
Feb 19 11:21:23 localhost.localdomain kernel: DMAR-IR: IOAPIC id 0 under DRHD base  0xfed90000 IOMMU 0
Feb 19 11:21:23 localhost.localdomain kernel: DMAR-IR: Enabled IRQ remapping in xapic mode

Currently supported:

- Emulated/Splitted irqchip
- Generic PCI Devices
- vhost devices
- pass through device support? Not tested, but suppose it should work.
- IEC (Interrupt Entry Cache) cache invalidation notification

TODO List:

- EIM support
- IR fault reporting
- source-id validation for IRTE
- migration support (for IOMMU as general?)
- more?

Peter Xu (18):
  acpi: enable INTR for DMAR report structure
  intel_iommu: allow queued invalidation for IR
  intel_iommu: set IR bit for ECAP register
  acpi: add DMAR scope definition for root IOAPIC
  intel_iommu: define interrupt remap table addr register
  intel_iommu: handle interrupt remap enable
  intel_iommu: define several structs for IOMMU IR
  intel_iommu: provide helper function vtd_get_iommu
  intel_iommu: add IR translation faults defines
  intel_iommu: Add support for PCI MSI remap
  q35: ioapic: add support for emulated IOAPIC IR
  ioapic: introduce ioapic_entry_parse() helper
  intel_iommu: add support for split irqchip
  q35: add "intremap" parameter to enable IR
  intel_iommu: introduce IEC notifiers
  ioapic: register VT-d IEC invalidate notifier
  ioapic: keep RO bits for IOAPIC entry
  ioapic: clear remote irr bit for edge-triggered interrupts

 hw/core/machine.c                 |  22 +++
 hw/i386/acpi-build.c              |  36 ++--
 hw/i386/intel_iommu.c             | 387 +++++++++++++++++++++++++++++++++++++-
 hw/i386/intel_iommu_internal.h    |  47 ++++-
 hw/i386/pc.c                      |   3 +
 hw/intc/ioapic.c                  | 158 +++++++++++-----
 hw/pci-host/q35.c                 |   4 +
 include/hw/acpi/acpi-defs.h       |  15 ++
 include/hw/boards.h               |   1 +
 include/hw/i386/apic-msidef.h     |   1 +
 include/hw/i386/intel_iommu.h     | 145 ++++++++++++++
 include/hw/i386/ioapic_internal.h |   6 +
 include/hw/i386/pc.h              |   4 +
 include/hw/pci-host/q35.h         |   9 +
 target-i386/kvm.c                 |  24 +++
 trace-events                      |   3 +
 16 files changed, 806 insertions(+), 59 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH v5 01/18] acpi: enable INTR for DMAR report structure

[Peter Xu]

Introduce iommu_intr in MachineState to show whether IOMMU IR is
enabled. By default, IR is off.

In ACPI DMA remapping report structure, enable INTR flag when specified.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/core/machine.c             |  2 ++
 hw/i386/acpi-build.c          | 12 +++++++++---
 include/hw/boards.h           |  1 +
 include/hw/i386/intel_iommu.h |  2 ++
 4 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 6dbbc85..276ad61 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -382,6 +382,8 @@ static void machine_initfn(Object *obj)
     ms->kvm_shadow_mem = -1;
     ms->dump_guest_core = true;
     ms->mem_merge = true;
+    /* Disable interrupt remapping by default. */
+    ms->iommu_intr = false;
 
     object_property_add_str(obj, "accel",
                             machine_get_accel, machine_set_accel, NULL);
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 6477003..80dd1bb 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2575,16 +2575,22 @@ build_mcfg_q35(GArray *table_data, GArray *linker, AcpiMcfgInfo *info)
 }
 
 static void
-build_dmar_q35(GArray *table_data, GArray *linker)
+build_dmar_q35(MachineState *ms, GArray *table_data, GArray *linker)
 {
     int dmar_start = table_data->len;
 
     AcpiTableDmar *dmar;
     AcpiDmarHardwareUnit *drhd;
+    uint8_t dmar_flags = 0;
+
+    if (ms->iommu_intr) {
+        /* enable INTR for the IOMMU device */
+        dmar_flags |= DMAR_REPORT_F_INTR;
+    }
 
     dmar = acpi_data_push(table_data, sizeof(*dmar));
     dmar->host_address_width = VTD_HOST_ADDRESS_WIDTH - 1;
-    dmar->flags = 0;    /* No intr_remap for now */
+    dmar->flags = dmar_flags;
 
     /* DMAR Remapping Hardware Unit Definition structure */
     drhd = acpi_data_push(table_data, sizeof(*drhd));
@@ -2745,7 +2751,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
     }
     if (acpi_has_iommu()) {
         acpi_add_table(table_offsets, tables_blob);
-        build_dmar_q35(tables_blob, tables->linker);
+        build_dmar_q35(MACHINE(pcms), tables_blob, tables->linker);
     }
     if (pcms->acpi_nvdimm_state.is_enabled) {
         nvdimm_build_acpi(table_offsets, tables_blob, tables->linker);
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 8d4fe56..43f4976 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -152,6 +152,7 @@ struct MachineState {
     bool igd_gfx_passthru;
     char *firmware;
     bool iommu;
+    bool iommu_intr;
     bool suppress_vmdesc;
     bool enforce_config_section;
 
diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
index b024ffa..0d89796 100644
--- a/include/hw/i386/intel_iommu.h
+++ b/include/hw/i386/intel_iommu.h
@@ -44,6 +44,8 @@
 #define VTD_HOST_ADDRESS_WIDTH      39
 #define VTD_HAW_MASK                ((1ULL << VTD_HOST_ADDRESS_WIDTH) - 1)
 
+#define DMAR_REPORT_F_INTR          (1)
+
 typedef struct VTDContextEntry VTDContextEntry;
 typedef struct VTDContextCacheEntry VTDContextCacheEntry;
 typedef struct IntelIOMMUState IntelIOMMUState;
-- 
2.4.3

[Qemu-devel] [PATCH v5 02/18] intel_iommu: allow queued invalidation for IR

[Peter Xu]

Queued invalidation is required for IR. This patch add basic support for
interrupt cache invalidate requests. Since we currently have no IR cache
implemented yet, we can just skip all interrupt cache invalidation
requests for now.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/i386/intel_iommu.c          | 9 +++++++++
 hw/i386/intel_iommu_internal.h | 2 ++
 2 files changed, 11 insertions(+)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 347718f..4b0558e 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -1400,6 +1400,15 @@ static bool vtd_process_inv_desc(IntelIOMMUState *s)
         }
         break;
 
+    case VTD_INV_DESC_IEC:
+        VTD_DPRINTF(INV, "Interrupt Entry Cache Invalidation "
+                    "not implemented yet");
+        /*
+         * Since currently we do not cache interrupt entries, we can
+         * just mark this descriptor as "good" and move on.
+         */
+        break;
+
     default:
         VTD_DPRINTF(GENERAL, "error: unkonw Invalidation Descriptor type "
                     "hi 0x%"PRIx64 " lo 0x%"PRIx64 " type %"PRIu8,
diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
index e5f514c..b648e69 100644
--- a/hw/i386/intel_iommu_internal.h
+++ b/hw/i386/intel_iommu_internal.h
@@ -286,6 +286,8 @@ typedef struct VTDInvDesc VTDInvDesc;
 #define VTD_INV_DESC_TYPE               0xf
 #define VTD_INV_DESC_CC                 0x1 /* Context-cache Invalidate Desc */
 #define VTD_INV_DESC_IOTLB              0x2
+#define VTD_INV_DESC_IEC                0x4 /* Interrupt Entry Cache
+                                               Invalidate Descriptor */
 #define VTD_INV_DESC_WAIT               0x5 /* Invalidation Wait Descriptor */
 #define VTD_INV_DESC_NONE               0   /* Not an Invalidate Descriptor */
 
-- 
2.4.3

[Qemu-devel] [PATCH v5 03/18] intel_iommu: set IR bit for ECAP register

[Peter Xu]

Enable IR in IOMMU Extended Capability register.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/i386/intel_iommu.c          | 7 +++++++
 hw/i386/intel_iommu_internal.h | 2 ++
 2 files changed, 9 insertions(+)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 4b0558e..17668d6 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -24,6 +24,7 @@
 #include "exec/address-spaces.h"
 #include "intel_iommu_internal.h"
 #include "hw/pci/pci.h"
+#include "hw/boards.h"
 
 /*#define DEBUG_INTEL_IOMMU*/
 #ifdef DEBUG_INTEL_IOMMU
@@ -1941,6 +1942,8 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn)
  */
 static void vtd_init(IntelIOMMUState *s)
 {
+    MachineState *ms = MACHINE(qdev_get_machine());
+
     memset(s->csr, 0, DMAR_REG_SIZE);
     memset(s->wmask, 0, DMAR_REG_SIZE);
     memset(s->w1cmask, 0, DMAR_REG_SIZE);
@@ -1961,6 +1964,10 @@ static void vtd_init(IntelIOMMUState *s)
              VTD_CAP_SAGAW | VTD_CAP_MAMV | VTD_CAP_PSI | VTD_CAP_SLLPS;
     s->ecap = VTD_ECAP_QI | VTD_ECAP_IRO;
 
+    if (ms->iommu_intr) {
+        s->ecap |= VTD_ECAP_IR;
+    }
+
     vtd_reset_context_cache(s);
     vtd_reset_iotlb(s);
 
diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
index b648e69..5b98a11 100644
--- a/hw/i386/intel_iommu_internal.h
+++ b/hw/i386/intel_iommu_internal.h
@@ -176,6 +176,8 @@
 /* (offset >> 4) << 8 */
 #define VTD_ECAP_IRO                (DMAR_IOTLB_REG_OFFSET << 4)
 #define VTD_ECAP_QI                 (1ULL << 1)
+/* Interrupt Remapping support */
+#define VTD_ECAP_IR                 (1ULL << 3)
 
 /* CAP_REG */
 /* (offset >> 4) << 24 */
-- 
2.4.3

[Qemu-devel] [PATCH v5 04/18] acpi: add DMAR scope definition for root IOAPIC

[Peter Xu]

To enable interrupt remapping for intel IOMMU device, each IOAPIC device
in the system reported via ACPI MADT must be explicitly enumerated under
one specific remapping hardware unit. This patch adds the root-complex
IOAPIC into the default DMAR device.

Please refer to VT-d spec 8.3.1.1 for more information.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/i386/acpi-build.c        | 17 +++++++++++++++--
 include/hw/acpi/acpi-defs.h | 15 +++++++++++++++
 include/hw/pci-host/q35.h   |  9 +++++++++
 3 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 80dd1bb..5d2d87b 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -77,6 +77,9 @@
 #define ACPI_BUILD_DPRINTF(fmt, ...)
 #endif
 
+/* Default IOAPIC ID */
+#define ACPI_BUILD_IOAPIC_ID 0x0
+
 typedef struct AcpiMcfgInfo {
     uint64_t mcfg_base;
     uint32_t mcfg_size;
@@ -375,7 +378,6 @@ build_madt(GArray *table_data, GArray *linker, PCMachineState *pcms)
     io_apic = acpi_data_push(table_data, sizeof *io_apic);
     io_apic->type = ACPI_APIC_IO;
     io_apic->length = sizeof(*io_apic);
-#define ACPI_BUILD_IOAPIC_ID 0x0
     io_apic->io_apic_id = ACPI_BUILD_IOAPIC_ID;
     io_apic->address = cpu_to_le32(IO_APIC_DEFAULT_ADDRESS);
     io_apic->interrupt = cpu_to_le32(0);
@@ -2582,6 +2584,9 @@ build_dmar_q35(MachineState *ms, GArray *table_data, GArray *linker)
     AcpiTableDmar *dmar;
     AcpiDmarHardwareUnit *drhd;
     uint8_t dmar_flags = 0;
+    AcpiDmarDeviceScope *scope = NULL;
+    /* Root complex IOAPIC use one path[0] only */
+    uint16_t scope_size = sizeof(*scope) + sizeof(uint16_t);
 
     if (ms->iommu_intr) {
         /* enable INTR for the IOMMU device */
@@ -2595,11 +2600,19 @@ build_dmar_q35(MachineState *ms, GArray *table_data, GArray *linker)
     /* DMAR Remapping Hardware Unit Definition structure */
     drhd = acpi_data_push(table_data, sizeof(*drhd));
     drhd->type = cpu_to_le16(ACPI_DMAR_TYPE_HARDWARE_UNIT);
-    drhd->length = cpu_to_le16(sizeof(*drhd));   /* No device scope now */
+    drhd->length = cpu_to_le16(sizeof(*drhd) + scope_size);
     drhd->flags = ACPI_DMAR_INCLUDE_PCI_ALL;
     drhd->pci_segment = cpu_to_le16(0);
     drhd->address = cpu_to_le64(Q35_HOST_BRIDGE_IOMMU_ADDR);
 
+    /* Scope definition for the root-complex IOAPIC */
+    scope = acpi_data_push(table_data, scope_size);
+    scope->entry_type = cpu_to_le16(ACPI_DMAR_DEV_SCOPE_TYPE_IOAPIC);
+    scope->length = scope_size;
+    scope->enumeration_id = cpu_to_le16(ACPI_BUILD_IOAPIC_ID);
+    scope->bus = cpu_to_le16(Q35_PSEUDO_BUS_PLATFORM);
+    scope->path[0] = cpu_to_le16(Q35_PSEUDO_DEVFN_IOAPIC);
+
     build_header(linker, table_data, (void *)(table_data->data + dmar_start),
                  "DMAR", table_data->len - dmar_start, 1, NULL, NULL);
 }
diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
index c7a03d4..2430af6 100644
--- a/include/hw/acpi/acpi-defs.h
+++ b/include/hw/acpi/acpi-defs.h
@@ -556,6 +556,20 @@ enum {
 /*
  * Sub-structures for DMAR
  */
+
+#define ACPI_DMAR_DEV_SCOPE_TYPE_IOAPIC     (0x03)
+
+/* Device scope structure for DRHD. */
+struct AcpiDmarDeviceScope {
+    uint8_t entry_type;
+    uint8_t length;
+    uint16_t reserved;
+    uint8_t enumeration_id;
+    uint8_t bus;
+    uint16_t path[0];           /* list of dev:func pairs */
+} QEMU_PACKED;
+typedef struct AcpiDmarDeviceScope AcpiDmarDeviceScope;
+
 /* Type 0: Hardware Unit Definition */
 struct AcpiDmarHardwareUnit {
     uint16_t type;
@@ -564,6 +578,7 @@ struct AcpiDmarHardwareUnit {
     uint8_t reserved;
     uint16_t pci_segment;   /* The PCI Segment associated with this unit */
     uint64_t address;   /* Base address of remapping hardware register-set */
+    AcpiDmarDeviceScope scope[0];
 } QEMU_PACKED;
 typedef struct AcpiDmarHardwareUnit AcpiDmarHardwareUnit;
 
diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
index c5c073d..9afc221 100644
--- a/include/hw/pci-host/q35.h
+++ b/include/hw/pci-host/q35.h
@@ -175,4 +175,13 @@ typedef struct Q35PCIHost {
 
 uint64_t mch_mcfg_base(void);
 
+/*
+ * Arbitary but unique BNF number for IOAPIC device. This is only
+ * used when interrupt remapping is enabled.
+ *
+ * TODO: make sure there would have no conflict with real PCI bus
+ */
+#define Q35_PSEUDO_BUS_PLATFORM         (0xff)
+#define Q35_PSEUDO_DEVFN_IOAPIC         (0x00)
+
 #endif /* HW_Q35_H */
-- 
2.4.3

[Qemu-devel] [PATCH v5 05/18] intel_iommu: define interrupt remap table addr register

[Peter Xu]

Defined Interrupt Remap Table Address register to store IR table
pointer. Also, do proper handling on global command register writes to
store table pointer and its size.

One more debug flag "DEBUG_IR" is added for interrupt remapping.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/i386/intel_iommu.c          | 52 +++++++++++++++++++++++++++++++++++++++++-
 hw/i386/intel_iommu_internal.h |  4 ++++
 include/hw/i386/intel_iommu.h  |  5 ++++
 3 files changed, 60 insertions(+), 1 deletion(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 17668d6..00b873c 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -30,7 +30,7 @@
 #ifdef DEBUG_INTEL_IOMMU
 enum {
     DEBUG_GENERAL, DEBUG_CSR, DEBUG_INV, DEBUG_MMU, DEBUG_FLOG,
-    DEBUG_CACHE,
+    DEBUG_CACHE, DEBUG_IR,
 };
 #define VTD_DBGBIT(x)   (1 << DEBUG_##x)
 static int vtd_dbgflags = VTD_DBGBIT(GENERAL) | VTD_DBGBIT(CSR);
@@ -900,6 +900,19 @@ static void vtd_root_table_setup(IntelIOMMUState *s)
                 (s->root_extended ? "(extended)" : ""));
 }
 
+static void vtd_interrupt_remap_table_setup(IntelIOMMUState *s)
+{
+    uint64_t value = 0;
+    value = vtd_get_quad_raw(s, DMAR_IRTA_REG);
+    s->intr_size = 1UL << ((value & VTD_IRTA_SIZE_MASK) + 1);
+    s->intr_root = value & VTD_IRTA_ADDR_MASK;
+
+    /* TODO: invalidate interrupt entry cache */
+
+    VTD_DPRINTF(CSR, "int remap table addr 0x%"PRIx64 " size %"PRIu32,
+                s->intr_root, s->intr_size);
+}
+
 static void vtd_context_global_invalidate(IntelIOMMUState *s)
 {
     s->context_cache_gen++;
@@ -1138,6 +1151,16 @@ static void vtd_handle_gcmd_srtp(IntelIOMMUState *s)
     vtd_set_clear_mask_long(s, DMAR_GSTS_REG, 0, VTD_GSTS_RTPS);
 }
 
+/* Set Interrupt Remap Table Pointer */
+static void vtd_handle_gcmd_sirtp(IntelIOMMUState *s)
+{
+    VTD_DPRINTF(CSR, "set Interrupt Remap Table Pointer");
+
+    vtd_interrupt_remap_table_setup(s);
+    /* Ok - report back to driver */
+    vtd_set_clear_mask_long(s, DMAR_GSTS_REG, 0, VTD_GSTS_IRTPS);
+}
+
 /* Handle Translation Enable/Disable */
 static void vtd_handle_gcmd_te(IntelIOMMUState *s, bool en)
 {
@@ -1177,6 +1200,10 @@ static void vtd_handle_gcmd_write(IntelIOMMUState *s)
         /* Queued Invalidation Enable */
         vtd_handle_gcmd_qie(s, val & VTD_GCMD_QIE);
     }
+    if (val & VTD_GCMD_SIRTP) {
+        /* Set/update the interrupt remapping root-table pointer */
+        vtd_handle_gcmd_sirtp(s);
+    }
 }
 
 /* Handle write to Context Command Register */
@@ -1838,6 +1865,23 @@ static void vtd_mem_write(void *opaque, hwaddr addr,
         vtd_update_fsts_ppf(s);
         break;
 
+    case DMAR_IRTA_REG:
+        VTD_DPRINTF(IR, "DMAR_IRTA_REG write addr 0x%"PRIx64
+                    ", size %d, val 0x%"PRIx64, addr, size, val);
+        if (size == 4) {
+            vtd_set_long(s, addr, val);
+        } else {
+            vtd_set_quad(s, addr, val);
+        }
+        break;
+
+    case DMAR_IRTA_REG_HI:
+        VTD_DPRINTF(IR, "DMAR_IRTA_REG_HI write addr 0x%"PRIx64
+                    ", size %d, val 0x%"PRIx64, addr, size, val);
+        assert(size == 4);
+        vtd_set_long(s, addr, val);
+        break;
+
     default:
         VTD_DPRINTF(GENERAL, "error: unhandled reg write addr 0x%"PRIx64
                     ", size %d, val 0x%"PRIx64, addr, size, val);
@@ -2017,6 +2061,12 @@ static void vtd_init(IntelIOMMUState *s)
     /* Fault Recording Registers, 128-bit */
     vtd_define_quad(s, DMAR_FRCD_REG_0_0, 0, 0, 0);
     vtd_define_quad(s, DMAR_FRCD_REG_0_2, 0, 0, 0x8000000000000000ULL);
+
+    /*
+     * Interrupt remapping registers, not support extended interrupt
+     * mode for now.
+     */
+    vtd_define_quad(s, DMAR_IRTA_REG, 0, 0xfffffffffffff00fULL, 0);
 }
 
 /* Should not reset address_spaces when reset because devices will still use
diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
index 5b98a11..309833f 100644
--- a/hw/i386/intel_iommu_internal.h
+++ b/hw/i386/intel_iommu_internal.h
@@ -172,6 +172,10 @@
 #define VTD_RTADDR_RTT              (1ULL << 11)
 #define VTD_RTADDR_ADDR_MASK        (VTD_HAW_MASK ^ 0xfffULL)
 
+/* IRTA_REG */
+#define VTD_IRTA_ADDR_MASK          (VTD_HAW_MASK ^ 0xfffULL)
+#define VTD_IRTA_SIZE_MASK          (0xfULL)
+
 /* ECAP_REG */
 /* (offset >> 4) << 8 */
 #define VTD_ECAP_IRO                (DMAR_IOTLB_REG_OFFSET << 4)
diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
index 0d89796..cc49839 100644
--- a/include/hw/i386/intel_iommu.h
+++ b/include/hw/i386/intel_iommu.h
@@ -125,6 +125,11 @@ struct IntelIOMMUState {
     MemoryRegionIOMMUOps iommu_ops;
     GHashTable *vtd_as_by_busptr;   /* VTDBus objects indexed by PCIBus* reference */
     VTDBus *vtd_as_by_bus_num[VTD_PCI_BUS_MAX]; /* VTDBus objects indexed by bus number */
+
+    /* interrupt remapping */
+    bool intr_enabled;              /* Whether guest enabled IR */
+    dma_addr_t intr_root;           /* Interrupt remapping table pointer */
+    uint32_t intr_size;             /* Number of IR table entries */
 };
 
 /* Find the VTD Address space associated with the given bus pointer,
-- 
2.4.3

[Qemu-devel] [PATCH v5 06/18] intel_iommu: handle interrupt remap enable

[Peter Xu]

Handle writting to IRE bit in global command register.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/i386/intel_iommu.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 00b873c..4d14124 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -1180,6 +1180,22 @@ static void vtd_handle_gcmd_te(IntelIOMMUState *s, bool en)
     }
 }
 
+/* Handle Interrupt Remap Enable/Disable */
+static void vtd_handle_gcmd_ire(IntelIOMMUState *s, bool en)
+{
+    VTD_DPRINTF(CSR, "Interrupt Remap Enable %s", (en ? "on" : "off"));
+
+    if (en) {
+        s->intr_enabled = true;
+        /* Ok - report back to driver */
+        vtd_set_clear_mask_long(s, DMAR_GSTS_REG, 0, VTD_GSTS_IRES);
+    } else {
+        s->intr_enabled = false;
+        /* Ok - report back to driver */
+        vtd_set_clear_mask_long(s, DMAR_GSTS_REG, VTD_GSTS_IRES, 0);
+    }
+}
+
 /* Handle write to Global Command Register */
 static void vtd_handle_gcmd_write(IntelIOMMUState *s)
 {
@@ -1204,6 +1220,10 @@ 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) {
+        /* Interrupt remap enable/disable */
+        vtd_handle_gcmd_ire(s, val & VTD_GCMD_IRE);
+    }
 }
 
 /* Handle write to Context Command Register */
-- 
2.4.3

[Qemu-devel] [PATCH v5 07/18] intel_iommu: define several structs for IOMMU IR

[Peter Xu]

Several data structs are defined to better support the rest of the
patches: IRTE to parse remapping table entries, and IOAPIC/MSI related
structure bits to parse interrupt entries to be filled in by guest
kernel.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/hw/i386/intel_iommu.h | 60 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
index cc49839..4914fe6 100644
--- a/include/hw/i386/intel_iommu.h
+++ b/include/hw/i386/intel_iommu.h
@@ -52,6 +52,9 @@ typedef struct IntelIOMMUState IntelIOMMUState;
 typedef struct VTDAddressSpace VTDAddressSpace;
 typedef struct VTDIOTLBEntry VTDIOTLBEntry;
 typedef struct VTDBus VTDBus;
+typedef union VTD_IRTE VTD_IRTE;
+typedef union VTD_IR_IOAPICEntry VTD_IR_IOAPICEntry;
+typedef union VTD_IR_MSIAddress VTD_IR_MSIAddress;
 
 /* Context-Entry */
 struct VTDContextEntry {
@@ -90,6 +93,63 @@ struct VTDIOTLBEntry {
     bool write_flags;
 };
 
+/* Interrupt Remapping Table Entry Definition */
+union VTD_IRTE {
+    struct {
+        uint8_t present:1;          /* Whether entry present/available */
+        uint8_t fault_disable:1;    /* Fault Processing Disable */
+        uint8_t dest_mode:1;        /* Destination Mode */
+        uint8_t redir_hint:1;       /* Redirection Hint */
+        uint8_t trigger_mode:1;     /* Trigger Mode */
+        uint8_t delivery_mode:3;    /* Delivery Mode */
+        uint8_t __avail:4;          /* Available spaces for software */
+        uint8_t __reserved_0:3;     /* Reserved 0 */
+        uint8_t irte_mode:1;        /* IRTE Mode */
+        uint8_t vector:8;           /* Interrupt Vector */
+        uint8_t __reserved_1:8;     /* Reserved 1 */
+        uint32_t dest_id:32;        /* Destination ID */
+        uint16_t source_id:16;      /* Source-ID */
+        uint8_t sid_q:2;            /* Source-ID Qualifier */
+        uint8_t sid_vtype:2;        /* Source-ID Validation Type */
+        uint64_t __reserved_2:44;   /* Reserved 2 */
+    } QEMU_PACKED;
+    uint64_t data[2];
+};
+
+/* Programming format for IOAPIC table entries */
+union VTD_IR_IOAPICEntry {
+    struct {
+        uint8_t vector:8;           /* Vector */
+        uint8_t __zeros:3;          /* Reserved (all zero) */
+        uint8_t index_h:1;          /* Interrupt Index bit 15 */
+        uint8_t status:1;           /* Deliver Status */
+        uint8_t polarity:1;         /* Interrupt Polarity */
+        uint8_t remote_irr:1;       /* Remote IRR */
+        uint8_t trigger_mode:1;     /* Trigger Mode */
+        uint8_t mask:1;             /* Mask */
+        uint32_t __reserved:31;     /* Reserved (should all zero) */
+        uint8_t int_mode:1;         /* Interrupt Format */
+        uint16_t index_l:15;        /* Interrupt Index bits 14-0 */
+    } QEMU_PACKED;
+    uint64_t data;
+};
+
+/* Programming format for MSI/MSI-X addresses */
+union VTD_IR_MSIAddress {
+    struct {
+        uint8_t __not_care:2;
+        uint8_t index_h:1;          /* Interrupt index bit 15 */
+        uint8_t sub_valid:1;        /* SHV: Sub-Handle Valid bit */
+        uint8_t int_mode:1;         /* Interrupt format */
+        uint16_t index_l:15;        /* Interrupt index bit 14-0 */
+        uint16_t __head:12;         /* Should always be: 0x0fee */
+    } QEMU_PACKED;
+    uint32_t data;
+};
+
+/* When IR is enabled, all MSI/MSI-X data bits should be zero */
+#define VTD_IR_MSI_DATA          (0)
+
 /* The iommu (DMAR) device state struct */
 struct IntelIOMMUState {
     SysBusDevice busdev;
-- 
2.4.3

[Qemu-devel] [PATCH v5 08/18] intel_iommu: provide helper function vtd_get_iommu

[Peter Xu]

Moves acpi_get_iommu() under VT-d to make it a public function.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/i386/acpi-build.c          |  7 +------
 hw/i386/intel_iommu.c         | 13 +++++++++++++
 include/hw/i386/intel_iommu.h |  2 ++
 3 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 5d2d87b..b064bc2 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2677,12 +2677,7 @@ static bool acpi_get_mcfg(AcpiMcfgInfo *mcfg)
 
 static bool acpi_has_iommu(void)
 {
-    bool ambiguous;
-    Object *intel_iommu;
-
-    intel_iommu = object_resolve_path_type("", TYPE_INTEL_IOMMU_DEVICE,
-                                           &ambiguous);
-    return intel_iommu && !ambiguous;
+    return !!vtd_iommu_get();
 }
 
 static
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 4d14124..a44289f 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -2001,6 +2001,19 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn)
     return vtd_dev_as;
 }
 
+IntelIOMMUState *vtd_iommu_get(void)
+{
+    bool ambiguous = false;
+    Object *intel_iommu = NULL;
+
+    intel_iommu = object_resolve_path_type("", TYPE_INTEL_IOMMU_DEVICE,
+                                 &ambiguous);
+    if (ambiguous)
+        intel_iommu = NULL;
+
+    return (IntelIOMMUState *)intel_iommu;
+}
+
 /* Do the initialization. It will also be called when reset, so pay
  * attention when adding new initialization stuff.
  */
diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
index 4914fe6..9ee84f7 100644
--- a/include/hw/i386/intel_iommu.h
+++ b/include/hw/i386/intel_iommu.h
@@ -196,5 +196,7 @@ struct IntelIOMMUState {
  * create a new one if none exists
  */
 VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn);
+/* Get default IOMMU object */
+IntelIOMMUState *vtd_iommu_get(void);
 
 #endif
-- 
2.4.3

[Qemu-devel] [PATCH v5 09/18] intel_iommu: add IR translation faults defines

[Peter Xu]

Adding translation fault definitions for interrupt remapping. Please
refer to VT-d spec section 7.1.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/i386/intel_iommu_internal.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
index 309833f..2a9987f 100644
--- a/hw/i386/intel_iommu_internal.h
+++ b/hw/i386/intel_iommu_internal.h
@@ -271,6 +271,19 @@ typedef enum VTDFaultReason {
      * context-entry.
      */
     VTD_FR_CONTEXT_ENTRY_TT,
+
+    /* Interrupt remapping transition faults */
+    VTD_FR_IR_REQ_RSVD = 0x20, /* One or more IR request reserved
+                                * fields set */
+    VTD_FR_IR_INDEX_OVER = 0x21, /* Index value greater than max */
+    VTD_FR_IR_ENTRY_P = 0x22,    /* Present (P) not set in IRTE */
+    VTD_FR_IR_ROOT_INVAL = 0x23, /* IR Root table invalid */
+    VTD_FR_IR_IRTE_RSVD = 0x24,  /* IRTE Rsvd field non-zero with
+                                  * Present flag set */
+    VTD_FR_IR_REQ_COMPAT = 0x25, /* Encountered compatible IR
+                                  * request while disabled */
+    VTD_FR_IR_SID_ERR = 0x26,   /* Invalid Source-ID */
+
     /* This is not a normal fault reason. We use this to indicate some faults
      * that are not referenced by the VT-d specification.
      * Fault event with such reason should not be recorded.
-- 
2.4.3

[Qemu-devel] [PATCH v5 10/18] intel_iommu: Add support for PCI MSI remap

[Peter Xu]

This patch enables interrupt remapping for PCI devices.

To play the trick, one memory region "iommu_ir" is added as child region
of the original iommu memory region, covering range 0xfeeXXXXX (which is
the address range for APIC). All the writes to this range will be taken
as MSI, and translation is carried out only when IR is enabled.

Idea suggested by Paolo Bonzini.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/i386/intel_iommu.c          | 239 +++++++++++++++++++++++++++++++++++++++++
 hw/i386/intel_iommu_internal.h |   2 +
 include/hw/i386/intel_iommu.h  |  52 +++++++++
 3 files changed, 293 insertions(+)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index a44289f..ff8388f 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -1969,6 +1969,240 @@ static Property vtd_properties[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
+/* Read IRTE entry with specific index */
+static int vtd_irte_get(IntelIOMMUState *iommu, uint16_t index,
+                        VTD_IRTE *entry)
+{
+    dma_addr_t addr = 0x00;
+
+    addr = iommu->intr_root + index * sizeof(*entry);
+    if (dma_memory_read(&address_space_memory, addr, entry,
+                        sizeof(*entry))) {
+        VTD_DPRINTF(GENERAL, "error: fail to access IR root at 0x%"PRIx64
+                    " + %"PRIu16, iommu->intr_root, index);
+        return -VTD_FR_IR_ROOT_INVAL;
+    }
+
+    if (!entry->present) {
+        VTD_DPRINTF(GENERAL, "error: present flag not set in IRTE"
+                    " entry index %u value 0x%"PRIx64 " 0x%"PRIx64,
+                    index, le64_to_cpu(entry->data[1]),
+                    le64_to_cpu(entry->data[0]));
+        return -VTD_FR_IR_ENTRY_P;
+    }
+
+    if (entry->__reserved_0 || entry->__reserved_1 || \
+        entry->__reserved_2) {
+        VTD_DPRINTF(GENERAL, "error: IRTE entry index %"PRIu16
+                    " reserved fields non-zero: 0x%"PRIx64 " 0x%"PRIx64,
+                    index, le64_to_cpu(entry->data[1]),
+                    le64_to_cpu(entry->data[0]));
+        return -VTD_FR_IR_IRTE_RSVD;
+    }
+
+    /*
+     * TODO: Check Source-ID corresponds to SVT (Source Validation
+     * Type) bits
+     */
+
+    return 0;
+}
+
+/* Fetch IRQ information of specific IR index */
+static int vtd_remap_irq_get(IntelIOMMUState *iommu, uint16_t index, VTDIrq *irq)
+{
+    VTD_IRTE irte;
+    int ret = 0;
+
+    bzero(&irte, sizeof(irte));
+
+    ret = vtd_irte_get(iommu, index, &irte);
+    if (ret) {
+        return ret;
+    }
+
+    irq->trigger_mode = irte.trigger_mode;
+    irq->vector = irte.vector;
+    irq->delivery_mode = irte.delivery_mode;
+    /* Not support EIM yet: please refer to vt-d 9.10 DST bits */
+#define  VTD_IR_APIC_DEST_MASK         (0xff00ULL)
+#define  VTD_IR_APIC_DEST_SHIFT        (8)
+    irq->dest = (irte.dest_id & VTD_IR_APIC_DEST_MASK) >> \
+        VTD_IR_APIC_DEST_SHIFT;
+    irq->dest_mode = irte.dest_mode;
+    irq->redir_hint = irte.redir_hint;
+
+    VTD_DPRINTF(IR, "remapping interrupt index %d: trig:%u,vec:%u,"
+                "deliver:%u,dest:%u,dest_mode:%u", index,
+                irq->trigger_mode, irq->vector, irq->delivery_mode,
+                irq->dest, irq->dest_mode);
+
+    return 0;
+}
+
+/* Generate one MSI message from VTDIrq info */
+static void vtd_generate_msi_message(VTDIrq *irq, MSIMessage *msg_out)
+{
+    VTD_MSIMessage msg = {};
+
+    /* Generate address bits */
+    msg.dest_mode = irq->dest_mode;
+    msg.redir_hint = irq->redir_hint;
+    msg.dest = irq->dest;
+    msg.__addr_head = 0xfee;
+    /* Keep this from original MSI address bits */
+    msg.__not_used = irq->msi_addr_last_bits;
+
+    /* Generate data bits */
+    msg.vector = irq->vector;
+    msg.delivery_mode = irq->delivery_mode;
+    msg.level = 1;
+    msg.trigger_mode = irq->trigger_mode;
+
+    msg_out->address = msg.msi_addr;
+    msg_out->data = msg.msi_data;
+}
+
+/* Interrupt remapping for MSI/MSI-X entry */
+static int vtd_interrupt_remap_msi(IntelIOMMUState *iommu,
+                                   MSIMessage *origin,
+                                   MSIMessage *translated)
+{
+    int ret = 0;
+    VTD_IR_MSIAddress addr;
+    uint16_t index = 0;
+    VTDIrq irq = {0};
+
+    assert(origin && translated);
+
+    if (!iommu || !iommu->intr_enabled) {
+        goto do_not_translate;
+    }
+
+    if (origin->address & VTD_MSI_ADDR_HI_MASK) {
+        VTD_DPRINTF(GENERAL, "error: MSI addr high 32 bits nonzero"
+                    " during interrupt remapping: 0x%"PRIx32,
+                    (uint32_t)((origin->address & VTD_MSI_ADDR_HI_MASK) >> \
+                    VTD_MSI_ADDR_HI_SHIFT));
+        return -VTD_FR_IR_REQ_RSVD;
+    }
+
+    addr.data = origin->address & VTD_MSI_ADDR_LO_MASK;
+    if (addr.__head != 0xfee) {
+        VTD_DPRINTF(GENERAL, "error: MSI addr low 32 bits invalid: "
+                    "0x%"PRIx32, addr.data);
+        return -VTD_FR_IR_REQ_RSVD;
+    }
+
+    /* This is compatible mode. */
+    if (addr.int_mode != VTD_IR_INT_FORMAT_REMAP) {
+        goto do_not_translate;
+    }
+
+    index = addr.index_h << 15 | addr.index_l;
+
+    ret = vtd_remap_irq_get(iommu, index, &irq);
+    if (ret) {
+        return ret;
+    }
+
+    if (addr.sub_valid == 1) {
+        VTD_DPRINTF(IR, "received MSI interrupt");
+        if (origin->data) {
+            VTD_DPRINTF(GENERAL, "error: MSI data bits non-zero for "
+                        "interrupt remappable entry: 0x%"PRIx32,
+                        origin->data);
+            return -VTD_FR_IR_REQ_RSVD;
+        }
+    } else {
+        uint8_t vector = origin->data & 0xff;
+        VTD_DPRINTF(IR, "received IOAPIC interrupt");
+        /* IOAPIC entry vector should be aligned with IRTE vector
+         * (see vt-d spec 5.1.5.1). */
+        if (vector != irq.vector) {
+            VTD_DPRINTF(GENERAL, "IOAPIC vector inconsistent: "
+                        "entry: %d, IRTE: %d, index: %d\n",
+                        vector, irq.vector, index);
+        }
+    }
+
+    /*
+     * We'd better keep the last two bits, assuming that guest OS
+     * might modify it. Keep it does not hurt after all.
+     */
+    irq.msi_addr_last_bits = addr.__not_care;
+
+    /* Translate VTDIrq to MSI message */
+    vtd_generate_msi_message(&irq, translated);
+
+    VTD_DPRINTF(IR, "mapping MSI 0x%"PRIx64":0x%"PRIx32 " -> "
+                "0x%"PRIx64":0x%"PRIx32, origin->address, origin->data,
+                translated->address, translated->data);
+    return 0;
+
+do_not_translate:
+    memcpy(translated, origin, sizeof(*origin));
+    return 0;
+}
+
+static uint64_t vtd_mem_ir_read(void *opaque, hwaddr addr, unsigned size)
+{
+    uint64_t data = 0;
+
+    addr += VTD_INTERRUPT_ADDR_FIRST;
+
+    VTD_DPRINTF(IR, "read mem_ir addr 0x%"PRIx64 " size %u",
+                addr, size);
+
+    if (dma_memory_read(&address_space_memory, addr, &data, size)) {
+        VTD_DPRINTF(GENERAL, "error: fail to access 0x%"PRIx64, addr);
+        return (uint32_t) -1;
+    }
+
+    return data;
+}
+
+static void vtd_mem_ir_write(void *opaque, hwaddr addr,
+                             uint64_t value, unsigned size)
+{
+    int ret = 0;
+    MSIMessage from = {0}, to = {0};
+
+    from.address = (uint64_t) addr + VTD_INTERRUPT_ADDR_FIRST;
+    from.data = (uint32_t) value;
+
+    ret = vtd_interrupt_remap_msi(opaque, &from, &to);
+    if (ret) {
+        /* TODO: report error */
+        VTD_DPRINTF(GENERAL, "int remap fail for addr 0x%"PRIx64
+                    " data 0x%"PRIx32, from.address, from.data);
+        /* Drop this interrupt */
+        return;
+    }
+
+    VTD_DPRINTF(IR, "delivering MSI 0x%"PRIx64":0x%"PRIx32,
+                to.address, to.data);
+
+    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);
+    }
+}
+
+static const MemoryRegionOps vtd_mem_ir_ops = {
+    .read = vtd_mem_ir_read,
+    .write = vtd_mem_ir_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .impl = {
+        .min_access_size = 4,
+        .max_access_size = 4,
+    },
+    .valid = {
+        .min_access_size = 4,
+        .max_access_size = 4,
+    },
+};
 
 VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn)
 {
@@ -1995,6 +2229,11 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn)
         vtd_dev_as->context_cache_entry.context_cache_gen = 0;
         memory_region_init_iommu(&vtd_dev_as->iommu, OBJECT(s),
                                  &s->iommu_ops, "intel_iommu", UINT64_MAX);
+        memory_region_init_io(&vtd_dev_as->iommu_ir, OBJECT(s),
+                              &vtd_mem_ir_ops, s, "intel_iommu_ir",
+                              VTD_INTERRUPT_ADDR_SIZE);
+        memory_region_add_subregion(&vtd_dev_as->iommu, VTD_INTERRUPT_ADDR_FIRST,
+                                    &vtd_dev_as->iommu_ir);
         address_space_init(&vtd_dev_as->as,
                            &vtd_dev_as->iommu, "intel_iommu");
     }
diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
index 2a9987f..e1a08cb 100644
--- a/hw/i386/intel_iommu_internal.h
+++ b/hw/i386/intel_iommu_internal.h
@@ -110,6 +110,8 @@
 /* Interrupt Address Range */
 #define VTD_INTERRUPT_ADDR_FIRST    0xfee00000ULL
 #define VTD_INTERRUPT_ADDR_LAST     0xfeefffffULL
+#define VTD_INTERRUPT_ADDR_SIZE     (VTD_INTERRUPT_ADDR_LAST - \
+                                     VTD_INTERRUPT_ADDR_FIRST + 1)
 
 /* The shift of source_id in the key of IOTLB hash table */
 #define VTD_IOTLB_SID_SHIFT         36
diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
index 9ee84f7..5945670 100644
--- a/include/hw/i386/intel_iommu.h
+++ b/include/hw/i386/intel_iommu.h
@@ -23,6 +23,8 @@
 #define INTEL_IOMMU_H
 #include "hw/qdev.h"
 #include "sysemu/dma.h"
+#include "hw/i386/ioapic.h"
+#include "hw/pci/msi.h"
 
 #define TYPE_INTEL_IOMMU_DEVICE "intel-iommu"
 #define INTEL_IOMMU_DEVICE(obj) \
@@ -46,6 +48,10 @@
 
 #define DMAR_REPORT_F_INTR          (1)
 
+#define  VTD_MSI_ADDR_HI_MASK        (0xffffffff00000000ULL)
+#define  VTD_MSI_ADDR_HI_SHIFT       (32)
+#define  VTD_MSI_ADDR_LO_MASK        (0x00000000ffffffffULL)
+
 typedef struct VTDContextEntry VTDContextEntry;
 typedef struct VTDContextCacheEntry VTDContextCacheEntry;
 typedef struct IntelIOMMUState IntelIOMMUState;
@@ -55,6 +61,8 @@ typedef struct VTDBus VTDBus;
 typedef union VTD_IRTE VTD_IRTE;
 typedef union VTD_IR_IOAPICEntry VTD_IR_IOAPICEntry;
 typedef union VTD_IR_MSIAddress VTD_IR_MSIAddress;
+typedef struct VTDIrq VTDIrq;
+typedef struct VTD_MSIMessage VTD_MSIMessage;
 
 /* Context-Entry */
 struct VTDContextEntry {
@@ -75,6 +83,7 @@ struct VTDAddressSpace {
     uint8_t devfn;
     AddressSpace as;
     MemoryRegion iommu;
+    MemoryRegion iommu_ir;      /* Interrupt region: 0xfeeXXXXX */
     IntelIOMMUState *iommu_state;
     VTDContextCacheEntry context_cache_entry;
 };
@@ -116,6 +125,9 @@ union VTD_IRTE {
     uint64_t data[2];
 };
 
+#define VTD_IR_INT_FORMAT_COMPAT     (0) /* Compatible Interrupt */
+#define VTD_IR_INT_FORMAT_REMAP      (1) /* Remappable Interrupt */
+
 /* Programming format for IOAPIC table entries */
 union VTD_IR_IOAPICEntry {
     struct {
@@ -147,6 +159,46 @@ union VTD_IR_MSIAddress {
     uint32_t data;
 };
 
+/* Generic IRQ entry information */
+struct VTDIrq {
+    /* Used by both IOAPIC/MSI interrupt remapping */
+    uint8_t trigger_mode;
+    uint8_t vector;
+    uint8_t delivery_mode;
+    uint32_t dest;
+    uint8_t dest_mode;
+
+    /* only used by MSI interrupt remapping */
+    uint8_t redir_hint;
+    uint8_t msi_addr_last_bits;
+};
+
+struct VTD_MSIMessage {
+    union {
+        struct {
+            uint16_t __not_used:2;
+            uint16_t dest_mode:1;
+            uint16_t redir_hint:1;
+            uint16_t __reserved:8;
+            uint16_t dest:8;
+            uint16_t __addr_head:12; /* 0xfee */
+            uint32_t __addr_hi:32;
+        } QEMU_PACKED;
+        uint64_t msi_addr;
+    };
+    union {
+        struct {
+            uint16_t vector:8;
+            uint16_t delivery_mode:3;
+            uint16_t __resved:3;
+            uint16_t level:1;
+            uint16_t trigger_mode:1;
+            uint16_t __resved1:16;
+        } QEMU_PACKED;
+        uint32_t msi_data;
+    };
+};
+
 /* When IR is enabled, all MSI/MSI-X data bits should be zero */
 #define VTD_IR_MSI_DATA          (0)
 
-- 
2.4.3

[Qemu-devel] [PATCH v5 11/18] q35: ioapic: add support for emulated IOAPIC IR

[Peter Xu]

This patch translates all IOAPIC interrupts into MSI ones. One pseudo
ioapic address space is added to transfer the MSI message. By default,
it will be system memory address space. When IR is enabled, it will be
IOMMU address space.

Currently, only emulated IOAPIC is supported.

Idea suggested by Jan Kiszka and Rita Sinha in the following patch:

https://lists.gnu.org/archive/html/qemu-devel/2016-03/msg01933.html

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/i386/pc.c                      |  3 +++
 hw/intc/ioapic.c                  | 28 ++++++++++++++++++++++++----
 hw/pci-host/q35.c                 |  4 ++++
 include/hw/i386/apic-msidef.h     |  1 +
 include/hw/i386/ioapic_internal.h |  1 +
 include/hw/i386/pc.h              |  4 ++++
 6 files changed, 37 insertions(+), 4 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 99437e0..365e82f 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1395,6 +1395,9 @@ void pc_memory_init(PCMachineState *pcms,
         rom_add_option(option_rom[i].name, option_rom[i].bootindex);
     }
     pcms->fw_cfg = fw_cfg;
+
+    /* Init default IOAPIC address space */
+    pcms->ioapic_as = &address_space_memory;
 }
 
 qemu_irq pc_allocate_cpu_irq(void)
diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c
index 378e663..92334a6 100644
--- a/hw/intc/ioapic.c
+++ b/hw/intc/ioapic.c
@@ -28,6 +28,8 @@
 #include "hw/i386/ioapic_internal.h"
 #include "include/hw/pci/msi.h"
 #include "sysemu/kvm.h"
+#include "target-i386/cpu.h"
+#include "hw/i386/apic-msidef.h"
 
 //#define DEBUG_IOAPIC
 
@@ -49,13 +51,15 @@ extern int ioapic_no;
 
 static void ioapic_service(IOAPICCommonState *s)
 {
+    AddressSpace *ioapic_as = PC_MACHINE(qdev_get_machine())->ioapic_as;
+    uint32_t addr, data;
     uint8_t i;
     uint8_t trig_mode;
     uint8_t vector;
     uint8_t delivery_mode;
     uint32_t mask;
     uint64_t entry;
-    uint8_t dest;
+    uint16_t dest_idx;
     uint8_t dest_mode;
 
     for (i = 0; i < IOAPIC_NUM_PINS; i++) {
@@ -66,7 +70,14 @@ static void ioapic_service(IOAPICCommonState *s)
             entry = s->ioredtbl[i];
             if (!(entry & IOAPIC_LVT_MASKED)) {
                 trig_mode = ((entry >> IOAPIC_LVT_TRIGGER_MODE_SHIFT) & 1);
-                dest = entry >> IOAPIC_LVT_DEST_SHIFT;
+                /*
+                 * By default, this would be dest_id[8] +
+                 * reserved[8]. When IR is enabled, this would be
+                 * interrupt_index[15] + interrupt_format[1]. This
+                 * field never means anything, but only used to
+                 * generate corresponding MSI.
+                 */
+                dest_idx = entry >> IOAPIC_LVT_DEST_IDX_SHIFT;
                 dest_mode = (entry >> IOAPIC_LVT_DEST_MODE_SHIFT) & 1;
                 delivery_mode =
                     (entry >> IOAPIC_LVT_DELIV_MODE_SHIFT) & IOAPIC_DM_MASK;
@@ -96,8 +107,17 @@ static void ioapic_service(IOAPICCommonState *s)
 #else
                 (void)coalesce;
 #endif
-                apic_deliver_irq(dest, dest_mode, delivery_mode, vector,
-                                 trig_mode);
+                /* No matter whether IR is enabled, we translate
+                 * the IOAPIC message into a MSI one, and its
+                 * address space will decide whether we need a
+                 * translation. */
+                addr = APIC_DEFAULT_ADDRESS | \
+                    (dest_idx << MSI_ADDR_DEST_IDX_SHIFT) |
+                    (dest_mode << MSI_ADDR_DEST_MODE_SHIFT);
+                data = (vector << MSI_DATA_VECTOR_SHIFT) |
+                    (trig_mode << MSI_DATA_TRIGGER_SHIFT) |
+                    (delivery_mode << MSI_DATA_DELIVERY_MODE_SHIFT);
+                stl_le_phys(ioapic_as, addr, data);
             }
         }
     }
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 70f897e..d32c123 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -437,6 +437,7 @@ static AddressSpace *q35_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
 
 static void mch_init_dmar(MCHPCIState *mch)
 {
+    PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
     PCIBus *pci_bus = PCI_BUS(qdev_get_parent_bus(DEVICE(mch)));
 
     mch->iommu = INTEL_IOMMU_DEVICE(qdev_create(NULL, TYPE_INTEL_IOMMU_DEVICE));
@@ -446,6 +447,9 @@ static void mch_init_dmar(MCHPCIState *mch)
     sysbus_mmio_map(SYS_BUS_DEVICE(mch->iommu), 0, Q35_HOST_BRIDGE_IOMMU_ADDR);
 
     pci_setup_iommu(pci_bus, q35_host_dma_iommu, mch->iommu);
+    /* Pseudo address space under root PCI bus. */
+    pcms->ioapic_as = q35_host_dma_iommu(pci_bus, mch->iommu,
+                                         Q35_PSEUDO_DEVFN_IOAPIC);
 }
 
 static void mch_realize(PCIDevice *d, Error **errp)
diff --git a/include/hw/i386/apic-msidef.h b/include/hw/i386/apic-msidef.h
index 6e2eb71..8b4d4cc 100644
--- a/include/hw/i386/apic-msidef.h
+++ b/include/hw/i386/apic-msidef.h
@@ -25,6 +25,7 @@
 #define MSI_ADDR_REDIRECTION_SHIFT      3
 
 #define MSI_ADDR_DEST_ID_SHIFT          12
+#define MSI_ADDR_DEST_IDX_SHIFT         4
 #define  MSI_ADDR_DEST_ID_MASK          0x00ffff0
 
 #endif /* HW_APIC_MSIDEF_H */
diff --git a/include/hw/i386/ioapic_internal.h b/include/hw/i386/ioapic_internal.h
index 797ed47..d279f2d 100644
--- a/include/hw/i386/ioapic_internal.h
+++ b/include/hw/i386/ioapic_internal.h
@@ -31,6 +31,7 @@
 #define IOAPIC_VERSION                  0x11
 
 #define IOAPIC_LVT_DEST_SHIFT           56
+#define IOAPIC_LVT_DEST_IDX_SHIFT       48
 #define IOAPIC_LVT_MASKED_SHIFT         16
 #define IOAPIC_LVT_TRIGGER_MODE_SHIFT   15
 #define IOAPIC_LVT_REMOTE_IRR_SHIFT     14
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 96f0b66..cde6934 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -72,6 +72,10 @@ struct PCMachineState {
     uint64_t numa_nodes;
     uint64_t *node_mem;
     uint64_t *node_cpu;
+
+    /* Address space used by IOAPIC device. All IOAPIC interrupts
+     * will be translated to MSI messages in the address space. */
+    AddressSpace *ioapic_as;
 };
 
 #define PC_MACHINE_ACPI_DEVICE_PROP "acpi-device"
-- 
2.4.3

[Qemu-devel] [PATCH v5 12/18] ioapic: introduce ioapic_entry_parse() helper

[Peter Xu]

Abstract IOAPIC entry parsing logic into a helper function.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/intc/ioapic.c | 110 +++++++++++++++++++++++++++----------------------------
 1 file changed, 54 insertions(+), 56 deletions(-)

diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c
index 92334a6..d6e88d5 100644
--- a/hw/intc/ioapic.c
+++ b/hw/intc/ioapic.c
@@ -49,18 +49,56 @@ static IOAPICCommonState *ioapics[MAX_IOAPICS];
 /* global variable from ioapic_common.c */
 extern int ioapic_no;
 
+struct ioapic_entry_info {
+    /* fields parsed from IOAPIC entries */
+    uint8_t masked;
+    uint8_t trig_mode;
+    uint16_t dest_idx;
+    uint8_t dest_mode;
+    uint8_t delivery_mode;
+    uint8_t vector;
+
+    /* MSI message generated from above parsed fields */
+    uint32_t addr;
+    uint32_t data;
+};
+
+static void ioapic_entry_parse(uint64_t entry, struct ioapic_entry_info *info)
+{
+    bzero(info, sizeof(*info));
+    info->masked = (entry >> IOAPIC_LVT_MASKED_SHIFT) & 1;
+    info->trig_mode = (entry >> IOAPIC_LVT_TRIGGER_MODE_SHIFT) & 1;
+    /*
+     * By default, this would be dest_id[8] + reserved[8]. When IR
+     * is enabled, this would be interrupt_index[15] +
+     * interrupt_format[1]. This field never means anything, but
+     * only used to generate corresponding MSI.
+     */
+    info->dest_idx = (entry >> IOAPIC_LVT_DEST_IDX_SHIFT) & 0xffff;
+    info->dest_mode = (entry >> IOAPIC_LVT_DEST_MODE_SHIFT) & 1;
+    info->delivery_mode = (entry >> IOAPIC_LVT_DELIV_MODE_SHIFT) \
+        & IOAPIC_DM_MASK;
+    if (info->delivery_mode == IOAPIC_DM_EXTINT) {
+        info->vector = pic_read_irq(isa_pic);
+    } else {
+        info->vector = entry & IOAPIC_VECTOR_MASK;
+    }
+
+    info->addr = APIC_DEFAULT_ADDRESS | \
+        (info->dest_idx << MSI_ADDR_DEST_IDX_SHIFT) | \
+        (info->dest_mode << MSI_ADDR_DEST_MODE_SHIFT);
+    info->data = (info->vector << MSI_DATA_VECTOR_SHIFT) | \
+        (info->trig_mode << MSI_DATA_TRIGGER_SHIFT) | \
+        (info->delivery_mode << MSI_DATA_DELIVERY_MODE_SHIFT);
+}
+
 static void ioapic_service(IOAPICCommonState *s)
 {
     AddressSpace *ioapic_as = PC_MACHINE(qdev_get_machine())->ioapic_as;
-    uint32_t addr, data;
+    struct ioapic_entry_info info;
     uint8_t i;
-    uint8_t trig_mode;
-    uint8_t vector;
-    uint8_t delivery_mode;
     uint32_t mask;
     uint64_t entry;
-    uint16_t dest_idx;
-    uint8_t dest_mode;
 
     for (i = 0; i < IOAPIC_NUM_PINS; i++) {
         mask = 1 << i;
@@ -68,33 +106,18 @@ static void ioapic_service(IOAPICCommonState *s)
             int coalesce = 0;
 
             entry = s->ioredtbl[i];
-            if (!(entry & IOAPIC_LVT_MASKED)) {
-                trig_mode = ((entry >> IOAPIC_LVT_TRIGGER_MODE_SHIFT) & 1);
-                /*
-                 * By default, this would be dest_id[8] +
-                 * reserved[8]. When IR is enabled, this would be
-                 * interrupt_index[15] + interrupt_format[1]. This
-                 * field never means anything, but only used to
-                 * generate corresponding MSI.
-                 */
-                dest_idx = entry >> IOAPIC_LVT_DEST_IDX_SHIFT;
-                dest_mode = (entry >> IOAPIC_LVT_DEST_MODE_SHIFT) & 1;
-                delivery_mode =
-                    (entry >> IOAPIC_LVT_DELIV_MODE_SHIFT) & IOAPIC_DM_MASK;
-                if (trig_mode == IOAPIC_TRIGGER_EDGE) {
+            ioapic_entry_parse(entry, &info);
+            if (!info.masked) {
+                if (info.trig_mode == IOAPIC_TRIGGER_EDGE) {
                     s->irr &= ~mask;
                 } else {
                     coalesce = s->ioredtbl[i] & IOAPIC_LVT_REMOTE_IRR;
                     s->ioredtbl[i] |= IOAPIC_LVT_REMOTE_IRR;
                 }
-                if (delivery_mode == IOAPIC_DM_EXTINT) {
-                    vector = pic_read_irq(isa_pic);
-                } else {
-                    vector = entry & IOAPIC_VECTOR_MASK;
-                }
+
 #ifdef CONFIG_KVM
                 if (kvm_irqchip_is_split()) {
-                    if (trig_mode == IOAPIC_TRIGGER_EDGE) {
+                    if (info.trig_mode == IOAPIC_TRIGGER_EDGE) {
                         kvm_set_irq(kvm_state, i, 1);
                         kvm_set_irq(kvm_state, i, 0);
                     } else {
@@ -111,13 +134,7 @@ static void ioapic_service(IOAPICCommonState *s)
                  * the IOAPIC message into a MSI one, and its
                  * address space will decide whether we need a
                  * translation. */
-                addr = APIC_DEFAULT_ADDRESS | \
-                    (dest_idx << MSI_ADDR_DEST_IDX_SHIFT) |
-                    (dest_mode << MSI_ADDR_DEST_MODE_SHIFT);
-                data = (vector << MSI_DATA_VECTOR_SHIFT) |
-                    (trig_mode << MSI_DATA_TRIGGER_SHIFT) |
-                    (delivery_mode << MSI_DATA_DELIVERY_MODE_SHIFT);
-                stl_le_phys(ioapic_as, addr, data);
+                stl_le_phys(ioapic_as, info.addr, info.data);
             }
         }
     }
@@ -168,30 +185,11 @@ static void ioapic_update_kvm_routes(IOAPICCommonState *s)
 
     if (kvm_irqchip_is_split()) {
         for (i = 0; i < IOAPIC_NUM_PINS; i++) {
-            uint64_t entry = s->ioredtbl[i];
-            uint8_t trig_mode;
-            uint8_t delivery_mode;
-            uint8_t dest;
-            uint8_t dest_mode;
-            uint64_t pin_polarity;
             MSIMessage msg;
-
-            trig_mode = ((entry >> IOAPIC_LVT_TRIGGER_MODE_SHIFT) & 1);
-            dest = entry >> IOAPIC_LVT_DEST_SHIFT;
-            dest_mode = (entry >> IOAPIC_LVT_DEST_MODE_SHIFT) & 1;
-            pin_polarity = (entry >> IOAPIC_LVT_TRIGGER_MODE_SHIFT) & 1;
-            delivery_mode =
-                (entry >> IOAPIC_LVT_DELIV_MODE_SHIFT) & IOAPIC_DM_MASK;
-
-            msg.address = APIC_DEFAULT_ADDRESS;
-            msg.address |= dest_mode << 2;
-            msg.address |= dest << 12;
-
-            msg.data = entry & IOAPIC_VECTOR_MASK;
-            msg.data |= delivery_mode << APIC_DELIVERY_MODE_SHIFT;
-            msg.data |= pin_polarity << APIC_POLARITY_SHIFT;
-            msg.data |= trig_mode << APIC_TRIG_MODE_SHIFT;
-
+            struct ioapic_entry_info info;
+            ioapic_entry_parse(s->ioredtbl[i], &info);
+            msg.address = info.addr;
+            msg.data = info.data;
             kvm_irqchip_update_msi_route(kvm_state, i, msg, NULL);
         }
         kvm_irqchip_commit_routes(kvm_state);
-- 
2.4.3

[Qemu-devel] [PATCH v5 13/18] intel_iommu: add support for split irqchip

[Peter Xu]

In split irqchip mode, IOAPIC is working in user space, only update
kernel irq routes when entry changed. When IR is enabled, we directly
update the kernel with translated messages. It works just like a kernel
cache for the remapping entries.

Since KVM irqfd is using kernel gsi routes to deliver interrupts, as
long as we can support split irqchip, we will support irqfd as
well. Also, since kernel gsi routes will cache translated interrupts,
irqfd delivery will not suffer from any performance impact due to IR.

And, since we supported irqfd, vhost devices will be able to work
seamlessly with IR now. Logically this should contain both vhost-net and
vhost-user case.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/i386/intel_iommu.c         |  5 +++++
 include/hw/i386/intel_iommu.h |  2 ++
 target-i386/kvm.c             | 24 ++++++++++++++++++++++++
 trace-events                  |  3 +++
 4 files changed, 34 insertions(+)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index ff8388f..a8a57db 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -2145,6 +2145,11 @@ do_not_translate:
     return 0;
 }
 
+int vtd_int_remap(void *iommu, MSIMessage *src, MSIMessage *dst)
+{
+    return vtd_interrupt_remap_msi(iommu, src, dst);
+}
+
 static uint64_t vtd_mem_ir_read(void *opaque, hwaddr addr, unsigned size)
 {
     uint64_t data = 0;
diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
index 5945670..5910e6f 100644
--- a/include/hw/i386/intel_iommu.h
+++ b/include/hw/i386/intel_iommu.h
@@ -25,6 +25,7 @@
 #include "sysemu/dma.h"
 #include "hw/i386/ioapic.h"
 #include "hw/pci/msi.h"
+#include "hw/sysbus.h"
 
 #define TYPE_INTEL_IOMMU_DEVICE "intel-iommu"
 #define INTEL_IOMMU_DEVICE(obj) \
@@ -250,5 +251,6 @@ struct IntelIOMMUState {
 VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn);
 /* Get default IOMMU object */
 IntelIOMMUState *vtd_iommu_get(void);
+int vtd_int_remap(void *iommu, MSIMessage *src, MSIMessage *dst);
 
 #endif
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 799fdfa..ea5387c 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -36,6 +36,7 @@
 #include "hw/i386/apic.h"
 #include "hw/i386/apic_internal.h"
 #include "hw/i386/apic-msidef.h"
+#include "hw/i386/intel_iommu.h"
 
 #include "exec/ioport.h"
 #include "standard-headers/asm-x86/hyperv.h"
@@ -43,6 +44,7 @@
 #include "hw/pci/msi.h"
 #include "migration/migration.h"
 #include "exec/memattrs.h"
+#include "trace.h"
 
 //#define DEBUG_KVM
 
@@ -3327,6 +3329,28 @@ int kvm_device_msix_deassign(KVMState *s, uint32_t dev_id)
 int kvm_arch_fixup_msi_route(struct kvm_irq_routing_entry *route,
                              uint64_t address, uint32_t data, PCIDevice *dev)
 {
+    IntelIOMMUState *iommu = vtd_iommu_get();
+
+    if (iommu) {
+        int ret;
+        MSIMessage src, dst;
+
+        src.address = route->u.msi.address_hi;
+        src.address <<= VTD_MSI_ADDR_HI_SHIFT;
+        src.address |= route->u.msi.address_lo;
+        src.data = route->u.msi.data;
+
+        ret = vtd_int_remap(iommu, &src, &dst);
+        if (ret) {
+            trace_kvm_x86_fixup_msi_error(route->gsi);
+            return 1;
+        }
+
+        route->u.msi.address_hi = dst.address >> VTD_MSI_ADDR_HI_SHIFT;
+        route->u.msi.address_lo = dst.address & VTD_MSI_ADDR_LO_MASK;
+        route->u.msi.data = dst.data;
+    }
+
     return 0;
 }
 
diff --git a/trace-events b/trace-events
index 8350743..b03d310 100644
--- a/trace-events
+++ b/trace-events
@@ -1909,3 +1909,6 @@ aspeed_vic_update_fiq(int flags) "Raising FIQ: %d"
 aspeed_vic_update_irq(int flags) "Raising IRQ: %d"
 aspeed_vic_read(uint64_t offset, unsigned size, uint32_t value) "From 0x%" PRIx64 " of size %u: 0x%" PRIx32
 aspeed_vic_write(uint64_t offset, unsigned size, uint32_t data) "To 0x%" PRIx64 " of size %u: 0x%" PRIx32
+
+# target-i386/kvm.c
+kvm_x86_fixup_msi_error(uint32_t gsi) "VT-d failed to remap interrupt for GSI %" PRIu32
-- 
2.4.3

[Qemu-devel] [PATCH v5 14/18] q35: add "intremap" parameter to enable IR

[Peter Xu]

One flag is added to specify whether to enable IR for emulated IOMMU. By
default, interrupt remapping is not supportted. To enable it, we should
specify something like:

$ qemu-system-x86_64 -M q35,iommu=on,intremap=on

To be more clear, the following command:

$ qemu-system-x86_64 -M q35,iommu=on

Will enable IOMMU only, without interrupt remapping support.

Currently, Intel IOMMU IR only support kernel-irqchip={off|split}. We
need to specify either of it in -M as well.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/core/machine.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 276ad61..5994b9f 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -300,6 +300,20 @@ static void machine_set_iommu(Object *obj, bool value, Error **errp)
     ms->iommu = value;
 }
 
+static bool machine_get_intremap(Object *obj, Error **errp)
+{
+    MachineState *ms = MACHINE(obj);
+
+    return ms->iommu_intr;
+}
+
+static void machine_set_intremap(Object *obj, bool value, Error **errp)
+{
+    MachineState *ms = MACHINE(obj);
+
+    ms->iommu_intr = value;
+}
+
 static void machine_set_suppress_vmdesc(Object *obj, bool value, Error **errp)
 {
     MachineState *ms = MACHINE(obj);
@@ -480,6 +494,12 @@ static void machine_initfn(Object *obj)
     object_property_set_description(obj, "iommu",
                                     "Set on/off to enable/disable Intel IOMMU (VT-d)",
                                     NULL);
+    object_property_add_bool(obj, "intremap", machine_get_intremap,
+                             machine_set_intremap, NULL);
+    object_property_set_description(obj, "intremap",
+                                    "Set on/off to enable/disable IOMMU"
+                                    " interrupt remapping",
+                                    NULL);
     object_property_add_bool(obj, "suppress-vmdesc",
                              machine_get_suppress_vmdesc,
                              machine_set_suppress_vmdesc, NULL);
-- 
2.4.3

[Qemu-devel] [PATCH v5 15/18] intel_iommu: introduce IEC notifiers

[Peter Xu]

This patch introduces Intel VT-d IEC (Interrupt Entry Cache)
invalidation notifier list. When vIOMMU receives IEC invalidate request,
all the registered units will be notified with specific invalidation
requests.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/i386/intel_iommu.c          | 56 ++++++++++++++++++++++++++++++++++++------
 hw/i386/intel_iommu_internal.h | 24 +++++++++++++++---
 include/hw/i386/intel_iommu.h  | 22 +++++++++++++++++
 3 files changed, 91 insertions(+), 11 deletions(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index a8a57db..7122e5b 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -900,6 +900,22 @@ static void vtd_root_table_setup(IntelIOMMUState *s)
                 (s->root_extended ? "(extended)" : ""));
 }
 
+static void vtd_iec_notify_all(IntelIOMMUState *s, bool global,
+                               uint32_t index, uint32_t mask)
+{
+    VTD_IEC_Notifier *notifier;
+
+    VTD_DPRINTF(INV, "notify IEC invalidate: global=%d, index=%u, mask=%u",
+                global, index, mask);
+
+    QLIST_FOREACH(notifier, &s->iec_notifiers, list) {
+        if (notifier->iec_notify) {
+            notifier->iec_notify(notifier->private, global,
+                                 index, mask);
+        }
+    }
+}
+
 static void vtd_interrupt_remap_table_setup(IntelIOMMUState *s)
 {
     uint64_t value = 0;
@@ -907,7 +923,8 @@ static void vtd_interrupt_remap_table_setup(IntelIOMMUState *s)
     s->intr_size = 1UL << ((value & VTD_IRTA_SIZE_MASK) + 1);
     s->intr_root = value & VTD_IRTA_ADDR_MASK;
 
-    /* TODO: invalidate interrupt entry cache */
+    /* Notify global invalidation */
+    vtd_iec_notify_all(s, true, 0, 0);
 
     VTD_DPRINTF(CSR, "int remap table addr 0x%"PRIx64 " size %"PRIu32,
                 s->intr_root, s->intr_size);
@@ -1409,6 +1426,21 @@ static bool vtd_process_iotlb_desc(IntelIOMMUState *s, VTDInvDesc *inv_desc)
     return true;
 }
 
+static bool vtd_process_inv_iec_desc(IntelIOMMUState *s,
+                                     VTDInvDesc *inv_desc)
+{
+    VTD_DPRINTF(INV, "inv ir glob %d index %d mask %d",
+                inv_desc->iec.granularity,
+                inv_desc->iec.index,
+                inv_desc->iec.index_mask);
+
+    vtd_iec_notify_all(s, inv_desc->iec.granularity,
+                       inv_desc->iec.index,
+                       inv_desc->iec.index_mask);
+
+    return true;
+}
+
 static bool vtd_process_inv_desc(IntelIOMMUState *s)
 {
     VTDInvDesc inv_desc;
@@ -1449,12 +1481,12 @@ static bool vtd_process_inv_desc(IntelIOMMUState *s)
         break;
 
     case VTD_INV_DESC_IEC:
-        VTD_DPRINTF(INV, "Interrupt Entry Cache Invalidation "
-                    "not implemented yet");
-        /*
-         * Since currently we do not cache interrupt entries, we can
-         * just mark this descriptor as "good" and move on.
-         */
+        VTD_DPRINTF(INV, "Invalidation Interrupt Entry Cache "
+                    "Descriptor hi 0x%"PRIx64 " lo 0x%"PRIx64,
+                    inv_desc.hi, inv_desc.lo);
+        if (!vtd_process_inv_iec_desc(s, &inv_desc)) {
+            return false;
+        }
         break;
 
     default:
@@ -2209,6 +2241,15 @@ static const MemoryRegionOps vtd_mem_ir_ops = {
     },
 };
 
+void vtd_iec_register_notifier(IntelIOMMUState *s, vtd_iec_notify_fn fn,
+                               void *data)
+{
+    VTD_IEC_Notifier *notifier = g_new0(VTD_IEC_Notifier, 1);
+    notifier->iec_notify = fn;
+    notifier->private = data;
+    QLIST_INSERT_HEAD(&s->iec_notifiers, notifier, list);
+}
+
 VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn)
 {
     uintptr_t key = (uintptr_t)bus;
@@ -2371,6 +2412,7 @@ static void vtd_realize(DeviceState *dev, Error **errp)
                                      g_free, g_free);
     s->vtd_as_by_busptr = g_hash_table_new_full(vtd_uint64_hash, vtd_uint64_equal,
                                               g_free, g_free);
+    QLIST_INIT(&s->iec_notifiers);
     vtd_init(s);
 }
 
diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
index e1a08cb..10c20fe 100644
--- a/hw/i386/intel_iommu_internal.h
+++ b/hw/i386/intel_iommu_internal.h
@@ -296,12 +296,28 @@ typedef enum VTDFaultReason {
 
 #define VTD_CONTEXT_CACHE_GEN_MAX       0xffffffffUL
 
+/* Interrupt Entry Cache Invalidation Descriptor: VT-d 6.5.2.7. */
+struct VTDInvDescIEC {
+    uint32_t type:4;            /* Should always be 0x4 */
+    uint32_t granularity:1;     /* If set, it's global IR invalidation */
+    uint32_t resved_1:22;
+    uint32_t index_mask:5;      /* 2^N for continuous int invalidation */
+    uint32_t index:16;          /* Start index to invalidate */
+    uint32_t reserved_2:16;
+};
+typedef struct VTDInvDescIEC VTDInvDescIEC;
+
 /* Queued Invalidation Descriptor */
-struct VTDInvDesc {
-    uint64_t lo;
-    uint64_t hi;
+union VTDInvDesc {
+    struct {
+        uint64_t lo;
+        uint64_t hi;
+    };
+    union {
+        VTDInvDescIEC iec;
+    };
 };
-typedef struct VTDInvDesc VTDInvDesc;
+typedef union VTDInvDesc VTDInvDesc;
 
 /* Masks for struct VTDInvDesc */
 #define VTD_INV_DESC_TYPE               0xf
diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
index 5910e6f..4fe92cf 100644
--- a/include/hw/i386/intel_iommu.h
+++ b/include/hw/i386/intel_iommu.h
@@ -203,6 +203,24 @@ struct VTD_MSIMessage {
 /* When IR is enabled, all MSI/MSI-X data bits should be zero */
 #define VTD_IR_MSI_DATA          (0)
 
+/**
+ * vtd_iec_notify_fn - IEC (Interrupt Entry Cache) notifier hook,
+ *                     triggered when IR invalidation happens.
+ * @private: private data
+ * @global: whether this is a global IEC invalidation
+ * @index: IRTE index to invalidate (start from)
+ * @mask: invalidation mask
+ */
+typedef void (*vtd_iec_notify_fn)(void *private, bool global,
+                                  uint32_t index, uint32_t mask);
+
+struct VTD_IEC_Notifier {
+    vtd_iec_notify_fn iec_notify;
+    void *private;
+    QLIST_ENTRY(VTD_IEC_Notifier) list;
+};
+typedef struct VTD_IEC_Notifier VTD_IEC_Notifier;
+
 /* The iommu (DMAR) device state struct */
 struct IntelIOMMUState {
     SysBusDevice busdev;
@@ -243,6 +261,7 @@ struct IntelIOMMUState {
     bool intr_enabled;              /* Whether guest enabled IR */
     dma_addr_t intr_root;           /* Interrupt remapping table pointer */
     uint32_t intr_size;             /* Number of IR table entries */
+    QLIST_HEAD(, VTD_IEC_Notifier) iec_notifiers; /* IEC notify list */
 };
 
 /* Find the VTD Address space associated with the given bus pointer,
@@ -252,5 +271,8 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn);
 /* Get default IOMMU object */
 IntelIOMMUState *vtd_iommu_get(void);
 int vtd_int_remap(void *iommu, MSIMessage *src, MSIMessage *dst);
+/* Register IEC invalidate notifier */
+void vtd_iec_register_notifier(IntelIOMMUState *s, vtd_iec_notify_fn fn,
+                               void *data);
 
 #endif
-- 
2.4.3

[Qemu-devel] [PATCH v5 16/18] ioapic: register VT-d IEC invalidate notifier

[Peter Xu]

Let IOAPIC the first consumer of VT-d IEC invalidation notifiers. This
is only used for split irqchip case, when VT-d receives IR invalidation
requests, IOAPIC will be notified to update kernel irq routes. For
simplicity, we just update all IOAPIC routes, even if the invalidated
entries are not IOAPIC ones.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/intc/ioapic.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c
index d6e88d5..b41ab89 100644
--- a/hw/intc/ioapic.c
+++ b/hw/intc/ioapic.c
@@ -30,6 +30,7 @@
 #include "sysemu/kvm.h"
 #include "target-i386/cpu.h"
 #include "hw/i386/apic-msidef.h"
+#include "hw/i386/intel_iommu.h"
 
 //#define DEBUG_IOAPIC
 
@@ -197,6 +198,14 @@ static void ioapic_update_kvm_routes(IOAPICCommonState *s)
 #endif
 }
 
+static void ioapic_iec_notifier(void *private, bool global,
+                                uint32_t index, uint32_t mask)
+{
+    IOAPICCommonState *s = (IOAPICCommonState *)private;
+    /* For simplicity, we just update all the routes */
+    ioapic_update_kvm_routes(s);
+}
+
 void ioapic_eoi_broadcast(int vector)
 {
     IOAPICCommonState *s;
@@ -330,6 +339,18 @@ static void ioapic_realize(DeviceState *dev, Error **errp)
     qdev_init_gpio_in(dev, ioapic_set_irq, IOAPIC_NUM_PINS);
 
     ioapics[ioapic_no] = s;
+
+#ifdef CONFIG_KVM
+    if (kvm_irqchip_is_split()) {
+        IntelIOMMUState *iommu = vtd_iommu_get();
+        if (iommu) {
+            /* Register this IOAPIC with IOMMU IEC notifier, so that
+             * when there are IR invalidates, we can be notified to
+             * update kernel IR cache. */
+            vtd_iec_register_notifier(iommu, ioapic_iec_notifier, s);
+        }
+    }
+#endif
 }
 
 static void ioapic_class_init(ObjectClass *klass, void *data)
-- 
2.4.3

[Qemu-devel] [PATCH v5 17/18] ioapic: keep RO bits for IOAPIC entry

[Peter Xu]

Currently IOAPIC RO bits can be written. To be better aligned with
hardware, we should let them read-only.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/intc/ioapic.c                  | 4 ++++
 include/hw/i386/ioapic_internal.h | 5 +++++
 2 files changed, 9 insertions(+)

diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c
index b41ab89..d7ebb5c 100644
--- a/hw/intc/ioapic.c
+++ b/hw/intc/ioapic.c
@@ -307,6 +307,7 @@ ioapic_mem_write(void *opaque, hwaddr addr, uint64_t val,
         default:
             index = (s->ioregsel - IOAPIC_REG_REDTBL_BASE) >> 1;
             if (index >= 0 && index < IOAPIC_NUM_PINS) {
+                uint64_t ro_bits = s->ioredtbl[index] & IOAPIC_RO_BITS;
                 if (s->ioregsel & 1) {
                     s->ioredtbl[index] &= 0xffffffff;
                     s->ioredtbl[index] |= (uint64_t)val << 32;
@@ -314,6 +315,9 @@ ioapic_mem_write(void *opaque, hwaddr addr, uint64_t val,
                     s->ioredtbl[index] &= ~0xffffffffULL;
                     s->ioredtbl[index] |= val;
                 }
+                /* restore RO bits */
+                s->ioredtbl[index] &= IOAPIC_RW_BITS;
+                s->ioredtbl[index] |= ro_bits;
                 ioapic_service(s);
             }
         }
diff --git a/include/hw/i386/ioapic_internal.h b/include/hw/i386/ioapic_internal.h
index d279f2d..31dafb3 100644
--- a/include/hw/i386/ioapic_internal.h
+++ b/include/hw/i386/ioapic_internal.h
@@ -48,6 +48,11 @@
 #define IOAPIC_LVT_DEST_MODE            (1 << IOAPIC_LVT_DEST_MODE_SHIFT)
 #define IOAPIC_LVT_DELIV_MODE           (7 << IOAPIC_LVT_DELIV_MODE_SHIFT)
 
+/* Bits that are read-only for IOAPIC entry */
+#define IOAPIC_RO_BITS                  (IOAPIC_LVT_REMOTE_IRR | \
+                                         IOAPIC_LVT_DELIV_STATUS)
+#define IOAPIC_RW_BITS                  (~(uint64_t)IOAPIC_RO_BITS)
+
 #define IOAPIC_TRIGGER_EDGE             0
 #define IOAPIC_TRIGGER_LEVEL            1
 
-- 
2.4.3

[Qemu-devel] [PATCH v5 18/18] ioapic: clear remote irr bit for edge-triggered interrupts

[Peter Xu]

This is to better emulate IOAPIC version 0x1X hardware. Linux kernel
leveraged this "feature" to do explicit EOI since EOI register is still
not introduced at that time. This will also fix the issue that level
triggered interrupts failed to work when IR enabled (tested with Linux
kernel version 4.5).

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/intc/ioapic.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c
index d7ebb5c..ce06954 100644
--- a/hw/intc/ioapic.c
+++ b/hw/intc/ioapic.c
@@ -281,6 +281,34 @@ ioapic_mem_read(void *opaque, hwaddr addr, unsigned int size)
     return val;
 }
 
+/*
+ * This is to satisfy the hack in Linux kernel. One hack of it is to
+ * simulate clearing the Remote IRR bit of IOAPIC entry using the
+ * following:
+ *
+ * "For IO-APIC's with EOI register, we use that to do an explicit EOI.
+ * Otherwise, we simulate the EOI message manually by changing the trigger
+ * mode to edge and then back to level, with RTE being masked during
+ * this."
+ *
+ * (See linux kernel __eoi_ioapic_pin() comment in commit c0205701)
+ *
+ * This is based on the assumption that, Remote IRR bit will be
+ * cleared by IOAPIC hardware when configured as edge-triggered
+ * interrupts.
+ *
+ * Without this, level-triggered interrupts in IR mode might fail to
+ * work correctly.
+ */
+static inline void
+ioapic_fix_edge_remote_irr(uint64_t *entry)
+{
+    if (!(*entry & IOAPIC_LVT_TRIGGER_MODE)) {
+        /* Edge-triggered interrupts, make sure remote IRR is zero */
+        *entry &= ~((uint64_t)IOAPIC_LVT_REMOTE_IRR);
+    }
+}
+
 static void
 ioapic_mem_write(void *opaque, hwaddr addr, uint64_t val,
                  unsigned int size)
@@ -318,6 +346,7 @@ ioapic_mem_write(void *opaque, hwaddr addr, uint64_t val,
                 /* restore RO bits */
                 s->ioredtbl[index] &= IOAPIC_RW_BITS;
                 s->ioredtbl[index] |= ro_bits;
+                ioapic_fix_edge_remote_irr(&s->ioredtbl[index]);
                 ioapic_service(s);
             }
         }
-- 
2.4.3

Re: [Qemu-devel] [PATCH v5 00/18] IOMMU: Enable interrupt remapping for Intel IOMMU

[Peter Xu]

On Thu, Apr 28, 2016 at 03:05:26PM +0800, Peter Xu wrote:
> v5 changes:
> - patch 10: add vector checking for IOAPIC interrupts (this may help
>   debug in the future, will only generate warning if specify
>   IOMMU_DEBUG)
> - patch 13: replace error_report() with a trace. [Jan]
> - patch 14: rename parameter "intr" to "intremap", to be aligned
>   with kernel parameter [Jan]
> - patch 15: fix comments for vtd_iec_notify_fn
> - patch 17 & 18 (added): fix issue when IR enabled with devices
>   using level-triggered interrupts, like e1000. Adding it to the end
>   of series, since this issue never happen without IR.
> 
>   Patch 17 adds read-only check for IOAPIC entries.
>   Patch 18 clears remote IRR bit when entry configured as
>   edge-triggered.

Sorry forgot to update github branch for better reference:

  https://github.com/xzpeter/qemu vtd-intr-v5

Thanks,

-- peterx

Re: [Qemu-devel] [PATCH v5 00/18] IOMMU: Enable interrupt remapping for Intel IOMMU

[Jan Kiszka]

On 2016-04-28 09:05, Peter Xu wrote:
> v5 changes:
> - patch 10: add vector checking for IOAPIC interrupts (this may help
>   debug in the future, will only generate warning if specify
>   IOMMU_DEBUG)
> - patch 13: replace error_report() with a trace. [Jan]
> - patch 14: rename parameter "intr" to "intremap", to be aligned
>   with kernel parameter [Jan]
> - patch 15: fix comments for vtd_iec_notify_fn
> - patch 17 & 18 (added): fix issue when IR enabled with devices
>   using level-triggered interrupts, like e1000. Adding it to the end
>   of series, since this issue never happen without IR.
> 
>   Patch 17 adds read-only check for IOAPIC entries.
>   Patch 18 clears remote IRR bit when entry configured as
>   edge-triggered.

IIUC, your series does not address irqfd yet, only by chance if the
target is an IOAPIC pin for which you set up routes now. Correct?

Instead of fiddling with irq routes for the IOAPIC - where we don't need
it -, I would suggest to do the following: Send IOAPIC events via
kvm_irqchip_send_msi to the kernel. Only irqfd users (vhost, vfio)
should use the pattern you are now applying to the IOAPIC: establish
static routes as an irqfd is set up, and that route should be translated
by the iommu first, register an IEC notifier to update any affected
route when the iommu translation changes.

Jan

Re: [Qemu-devel] [PATCH v5 15/18] intel_iommu: introduce IEC notifiers

[Jan Kiszka]

On 2016-04-28 09:05, Peter Xu wrote:
> This patch introduces Intel VT-d IEC (Interrupt Entry Cache)
> invalidation notifier list. When vIOMMU receives IEC invalidate request,
> all the registered units will be notified with specific invalidation
> requests.

This should be designed to be IOMMU-agnostic, i.e. become reusable for
the AMD implementation. I suspect we will have the same need for route
invalidations there as well...

Jan

> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  hw/i386/intel_iommu.c          | 56 ++++++++++++++++++++++++++++++++++++------
>  hw/i386/intel_iommu_internal.h | 24 +++++++++++++++---
>  include/hw/i386/intel_iommu.h  | 22 +++++++++++++++++
>  3 files changed, 91 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index a8a57db..7122e5b 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -900,6 +900,22 @@ static void vtd_root_table_setup(IntelIOMMUState *s)
>                  (s->root_extended ? "(extended)" : ""));
>  }
>  
> +static void vtd_iec_notify_all(IntelIOMMUState *s, bool global,
> +                               uint32_t index, uint32_t mask)
> +{
> +    VTD_IEC_Notifier *notifier;
> +
> +    VTD_DPRINTF(INV, "notify IEC invalidate: global=%d, index=%u, mask=%u",
> +                global, index, mask);
> +
> +    QLIST_FOREACH(notifier, &s->iec_notifiers, list) {
> +        if (notifier->iec_notify) {
> +            notifier->iec_notify(notifier->private, global,
> +                                 index, mask);
> +        }
> +    }
> +}
> +
>  static void vtd_interrupt_remap_table_setup(IntelIOMMUState *s)
>  {
>      uint64_t value = 0;
> @@ -907,7 +923,8 @@ static void vtd_interrupt_remap_table_setup(IntelIOMMUState *s)
>      s->intr_size = 1UL << ((value & VTD_IRTA_SIZE_MASK) + 1);
>      s->intr_root = value & VTD_IRTA_ADDR_MASK;
>  
> -    /* TODO: invalidate interrupt entry cache */
> +    /* Notify global invalidation */
> +    vtd_iec_notify_all(s, true, 0, 0);
>  
>      VTD_DPRINTF(CSR, "int remap table addr 0x%"PRIx64 " size %"PRIu32,
>                  s->intr_root, s->intr_size);
> @@ -1409,6 +1426,21 @@ static bool vtd_process_iotlb_desc(IntelIOMMUState *s, VTDInvDesc *inv_desc)
>      return true;
>  }
>  
> +static bool vtd_process_inv_iec_desc(IntelIOMMUState *s,
> +                                     VTDInvDesc *inv_desc)
> +{
> +    VTD_DPRINTF(INV, "inv ir glob %d index %d mask %d",
> +                inv_desc->iec.granularity,
> +                inv_desc->iec.index,
> +                inv_desc->iec.index_mask);
> +
> +    vtd_iec_notify_all(s, inv_desc->iec.granularity,
> +                       inv_desc->iec.index,
> +                       inv_desc->iec.index_mask);
> +
> +    return true;
> +}
> +
>  static bool vtd_process_inv_desc(IntelIOMMUState *s)
>  {
>      VTDInvDesc inv_desc;
> @@ -1449,12 +1481,12 @@ static bool vtd_process_inv_desc(IntelIOMMUState *s)
>          break;
>  
>      case VTD_INV_DESC_IEC:
> -        VTD_DPRINTF(INV, "Interrupt Entry Cache Invalidation "
> -                    "not implemented yet");
> -        /*
> -         * Since currently we do not cache interrupt entries, we can
> -         * just mark this descriptor as "good" and move on.
> -         */
> +        VTD_DPRINTF(INV, "Invalidation Interrupt Entry Cache "
> +                    "Descriptor hi 0x%"PRIx64 " lo 0x%"PRIx64,
> +                    inv_desc.hi, inv_desc.lo);
> +        if (!vtd_process_inv_iec_desc(s, &inv_desc)) {
> +            return false;
> +        }
>          break;
>  
>      default:
> @@ -2209,6 +2241,15 @@ static const MemoryRegionOps vtd_mem_ir_ops = {
>      },
>  };
>  
> +void vtd_iec_register_notifier(IntelIOMMUState *s, vtd_iec_notify_fn fn,
> +                               void *data)
> +{
> +    VTD_IEC_Notifier *notifier = g_new0(VTD_IEC_Notifier, 1);
> +    notifier->iec_notify = fn;
> +    notifier->private = data;
> +    QLIST_INSERT_HEAD(&s->iec_notifiers, notifier, list);
> +}
> +
>  VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn)
>  {
>      uintptr_t key = (uintptr_t)bus;
> @@ -2371,6 +2412,7 @@ static void vtd_realize(DeviceState *dev, Error **errp)
>                                       g_free, g_free);
>      s->vtd_as_by_busptr = g_hash_table_new_full(vtd_uint64_hash, vtd_uint64_equal,
>                                                g_free, g_free);
> +    QLIST_INIT(&s->iec_notifiers);
>      vtd_init(s);
>  }
>  
> diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
> index e1a08cb..10c20fe 100644
> --- a/hw/i386/intel_iommu_internal.h
> +++ b/hw/i386/intel_iommu_internal.h
> @@ -296,12 +296,28 @@ typedef enum VTDFaultReason {
>  
>  #define VTD_CONTEXT_CACHE_GEN_MAX       0xffffffffUL
>  
> +/* Interrupt Entry Cache Invalidation Descriptor: VT-d 6.5.2.7. */
> +struct VTDInvDescIEC {
> +    uint32_t type:4;            /* Should always be 0x4 */
> +    uint32_t granularity:1;     /* If set, it's global IR invalidation */
> +    uint32_t resved_1:22;
> +    uint32_t index_mask:5;      /* 2^N for continuous int invalidation */
> +    uint32_t index:16;          /* Start index to invalidate */
> +    uint32_t reserved_2:16;
> +};
> +typedef struct VTDInvDescIEC VTDInvDescIEC;
> +
>  /* Queued Invalidation Descriptor */
> -struct VTDInvDesc {
> -    uint64_t lo;
> -    uint64_t hi;
> +union VTDInvDesc {
> +    struct {
> +        uint64_t lo;
> +        uint64_t hi;
> +    };
> +    union {
> +        VTDInvDescIEC iec;
> +    };
>  };
> -typedef struct VTDInvDesc VTDInvDesc;
> +typedef union VTDInvDesc VTDInvDesc;
>  
>  /* Masks for struct VTDInvDesc */
>  #define VTD_INV_DESC_TYPE               0xf
> diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
> index 5910e6f..4fe92cf 100644
> --- a/include/hw/i386/intel_iommu.h
> +++ b/include/hw/i386/intel_iommu.h
> @@ -203,6 +203,24 @@ struct VTD_MSIMessage {
>  /* When IR is enabled, all MSI/MSI-X data bits should be zero */
>  #define VTD_IR_MSI_DATA          (0)
>  
> +/**
> + * vtd_iec_notify_fn - IEC (Interrupt Entry Cache) notifier hook,
> + *                     triggered when IR invalidation happens.
> + * @private: private data
> + * @global: whether this is a global IEC invalidation
> + * @index: IRTE index to invalidate (start from)
> + * @mask: invalidation mask
> + */
> +typedef void (*vtd_iec_notify_fn)(void *private, bool global,
> +                                  uint32_t index, uint32_t mask);
> +
> +struct VTD_IEC_Notifier {
> +    vtd_iec_notify_fn iec_notify;
> +    void *private;
> +    QLIST_ENTRY(VTD_IEC_Notifier) list;
> +};
> +typedef struct VTD_IEC_Notifier VTD_IEC_Notifier;
> +
>  /* The iommu (DMAR) device state struct */
>  struct IntelIOMMUState {
>      SysBusDevice busdev;
> @@ -243,6 +261,7 @@ struct IntelIOMMUState {
>      bool intr_enabled;              /* Whether guest enabled IR */
>      dma_addr_t intr_root;           /* Interrupt remapping table pointer */
>      uint32_t intr_size;             /* Number of IR table entries */
> +    QLIST_HEAD(, VTD_IEC_Notifier) iec_notifiers; /* IEC notify list */
>  };
>  
>  /* Find the VTD Address space associated with the given bus pointer,
> @@ -252,5 +271,8 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn);
>  /* Get default IOMMU object */
>  IntelIOMMUState *vtd_iommu_get(void);
>  int vtd_int_remap(void *iommu, MSIMessage *src, MSIMessage *dst);
> +/* Register IEC invalidate notifier */
> +void vtd_iec_register_notifier(IntelIOMMUState *s, vtd_iec_notify_fn fn,
> +                               void *data);
>  
>  #endif
> 

Re: [Qemu-devel] [PATCH v5 10/18] intel_iommu: Add support for PCI MSI remap

[Jan Kiszka]

On 2016-04-28 09:05, Peter Xu wrote:
> This patch enables interrupt remapping for PCI devices.
> 
> To play the trick, one memory region "iommu_ir" is added as child region
> of the original iommu memory region, covering range 0xfeeXXXXX (which is
> the address range for APIC). All the writes to this range will be taken
> as MSI, and translation is carried out only when IR is enabled.
> 
> Idea suggested by Paolo Bonzini.

This still lacks source (device ID) identification, right? Were did the
memory write attribute thing go? Given that you actually introduce a
separate MSI target address space for the IOAPIC (btw, once there will
be more than one instance, like on real hw today) and that you will need
yet another one for each HPET, why not address this with a common scheme
now, ie. by transmitting the source ID along the write via that attribute?

Jan

Re: [Qemu-devel] [PATCH v5 10/18] intel_iommu: Add support for PCI MSI remap

[Peter Xu]

On Thu, Apr 28, 2016 at 09:32:17AM +0200, Jan Kiszka wrote:
> On 2016-04-28 09:05, Peter Xu wrote:
> > This patch enables interrupt remapping for PCI devices.
> > 
> > To play the trick, one memory region "iommu_ir" is added as child region
> > of the original iommu memory region, covering range 0xfeeXXXXX (which is
> > the address range for APIC). All the writes to this range will be taken
> > as MSI, and translation is carried out only when IR is enabled.
> > 
> > Idea suggested by Paolo Bonzini.
> 
> This still lacks source (device ID) identification, right? Were did the
> memory write attribute thing go? Given that you actually introduce a
> separate MSI target address space for the IOAPIC (btw, once there will
> be more than one instance, like on real hw today) and that you will need
> yet another one for each HPET, why not address this with a common scheme
> now, ie. by transmitting the source ID along the write via that attribute?

Yes, it still lacks verification for SID. Currently there are two
ways to implement it. One is to use MemAttrs and provide
write_with_attrs() rather than write(). The other one is to keep
using write(), but instead passing in VTDAddressSpace rather than
IntelIOMMUState when init each IR memory region:

-----------------

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 7122e5b..6493093 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -2205,6 +2205,9 @@ static void vtd_mem_ir_write(void *opaque, hwaddr addr,
     int ret = 0;
     MSIMessage from = {0}, to = {0};
 
+    VTDAddressSpace *as = opaque;
+    /* Do whatever we want using as->bus and as->devfn... */
+
     from.address = (uint64_t) addr + VTD_INTERRUPT_ADDR_FIRST;
     from.data = (uint32_t) value;
 
@@ -2276,7 +2279,7 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn)
         memory_region_init_iommu(&vtd_dev_as->iommu, OBJECT(s),
                                  &s->iommu_ops, "intel_iommu", UINT64_MAX);
         memory_region_init_io(&vtd_dev_as->iommu_ir, OBJECT(s),
-                              &vtd_mem_ir_ops, s, "intel_iommu_ir",
+                              &vtd_mem_ir_ops, vtd_dev_as, "intel_iommu_ir",
                               VTD_INTERRUPT_ADDR_SIZE);
         memory_region_add_subregion(&vtd_dev_as->iommu, VTD_INTERRUPT_ADDR_FIRST,
                                     &vtd_dev_as->iommu_ir);

-----------------

If to use the above method, we'll need no scheme change. But now
since MemAttrs is ready (it's ready, right?), maybe I should start
to use it, which is the standard way to do.

Thanks to point out. Will make the change in v6.

-- peterx

Re: [Qemu-devel] [PATCH v5 15/18] intel_iommu: introduce IEC notifiers

[Peter Xu]

On Thu, Apr 28, 2016 at 09:26:01AM +0200, Jan Kiszka wrote:
> On 2016-04-28 09:05, Peter Xu wrote:
> > This patch introduces Intel VT-d IEC (Interrupt Entry Cache)
> > invalidation notifier list. When vIOMMU receives IEC invalidate request,
> > all the registered units will be notified with specific invalidation
> > requests.
> 
> This should be designed to be IOMMU-agnostic, i.e. become reusable for
> the AMD implementation. I suspect we will have the same need for route
> invalidations there as well...

Yes possibly...

I feel like there are lots of things that can be shared between
Intel and AMD IOMMUs. I just do not know what is the most suitable
"extent" that we should abstract these shared functionalities
between the two, and how.

For example, AFAIU, a better solution for current IOMMU
codes (including Intel and AMD) is to provide a common framework
(like...  X86IOMMU?), abstract these shared things out into a
framework, like per device name spaces, iotlb, IEC notifications,
etc... However, that will need a lot of further work. Also, I still
do not know whether this is a good idea even in the future.

So, will this be a good point that we start to think about common
code blocks for both Intel and AMD IOMMU?

Thanks,

-- peterx

Re: [Qemu-devel] [PATCH v5 15/18] intel_iommu: introduce IEC notifiers

[Jan Kiszka]

On 2016-04-28 10:29, Peter Xu wrote:
> On Thu, Apr 28, 2016 at 09:26:01AM +0200, Jan Kiszka wrote:
>> On 2016-04-28 09:05, Peter Xu wrote:
>>> This patch introduces Intel VT-d IEC (Interrupt Entry Cache)
>>> invalidation notifier list. When vIOMMU receives IEC invalidate request,
>>> all the registered units will be notified with specific invalidation
>>> requests.
>>
>> This should be designed to be IOMMU-agnostic, i.e. become reusable for
>> the AMD implementation. I suspect we will have the same need for route
>> invalidations there as well...
> 
> Yes possibly...
> 
> I feel like there are lots of things that can be shared between
> Intel and AMD IOMMUs. I just do not know what is the most suitable
> "extent" that we should abstract these shared functionalities
> between the two, and how.

A rough indicator: if you add something that has "vtd" in its name to
non-vtd code, think twice about some reusable abstraction ;). So,
something like "vtd_get_iommu" could already be named and designed to
have two provides (e.g. allow an IOMMU to register itself as provider,
return that registered instance(s) when requested).

> 
> For example, AFAIU, a better solution for current IOMMU
> codes (including Intel and AMD) is to provide a common framework
> (like...  X86IOMMU?), abstract these shared things out into a
> framework, like per device name spaces, iotlb, IEC notifications,
> etc... However, that will need a lot of further work. Also, I still
> do not know whether this is a good idea even in the future.
> 
> So, will this be a good point that we start to think about common
> code blocks for both Intel and AMD IOMMU?

The core iommu code can still be refactored later on. I'm now more
concerned about the hooks you add to generic code, see above.

Jan

Re: [Qemu-devel] [PATCH v5 15/18] intel_iommu: introduce IEC notifiers

[Peter Xu]

On Thu, Apr 28, 2016 at 10:36:19AM +0200, Jan Kiszka wrote:
> On 2016-04-28 10:29, Peter Xu wrote:
> > On Thu, Apr 28, 2016 at 09:26:01AM +0200, Jan Kiszka wrote:
> >> On 2016-04-28 09:05, Peter Xu wrote:
> >>> This patch introduces Intel VT-d IEC (Interrupt Entry Cache)
> >>> invalidation notifier list. When vIOMMU receives IEC invalidate request,
> >>> all the registered units will be notified with specific invalidation
> >>> requests.
> >>
> >> This should be designed to be IOMMU-agnostic, i.e. become reusable for
> >> the AMD implementation. I suspect we will have the same need for route
> >> invalidations there as well...
> > 
> > Yes possibly...
> > 
> > I feel like there are lots of things that can be shared between
> > Intel and AMD IOMMUs. I just do not know what is the most suitable
> > "extent" that we should abstract these shared functionalities
> > between the two, and how.
> 
> A rough indicator: if you add something that has "vtd" in its name to
> non-vtd code, think twice about some reusable abstraction ;). So,
> something like "vtd_get_iommu" could already be named and designed to
> have two provides (e.g. allow an IOMMU to register itself as provider,
> return that registered instance(s) when requested).

Yes, thanks for the hints. :)

Before that, I was considering that the AMD guy who is going to add
its support will better consider this and finally make sure the two
coops well (anyway, I know nothing about AMD IOMMU before reading
recent patches, and there is still no amd_iommu.c yet for me to read
at..). But you are right, best to start consider it from the very
beginning.

> 
> > 
> > For example, AFAIU, a better solution for current IOMMU
> > codes (including Intel and AMD) is to provide a common framework
> > (like...  X86IOMMU?), abstract these shared things out into a
> > framework, like per device name spaces, iotlb, IEC notifications,
> > etc... However, that will need a lot of further work. Also, I still
> > do not know whether this is a good idea even in the future.
> > 
> > So, will this be a good point that we start to think about common
> > code blocks for both Intel and AMD IOMMU?
> 
> The core iommu code can still be refactored later on. I'm now more
> concerned about the hooks you add to generic code, see above.

Will try to make them look better in v6. Hopefully there will have
no vtd_*() in common codes. ;)

Thanks!

-- peterx

Re: [Qemu-devel] [PATCH v5 00/18] IOMMU: Enable interrupt remapping for Intel IOMMU

[Peter Xu]

On Thu, Apr 28, 2016 at 09:19:28AM +0200, Jan Kiszka wrote:
> On 2016-04-28 09:05, Peter Xu wrote:
> > v5 changes:
> > - patch 10: add vector checking for IOAPIC interrupts (this may help
> >   debug in the future, will only generate warning if specify
> >   IOMMU_DEBUG)
> > - patch 13: replace error_report() with a trace. [Jan]
> > - patch 14: rename parameter "intr" to "intremap", to be aligned
> >   with kernel parameter [Jan]
> > - patch 15: fix comments for vtd_iec_notify_fn
> > - patch 17 & 18 (added): fix issue when IR enabled with devices
> >   using level-triggered interrupts, like e1000. Adding it to the end
> >   of series, since this issue never happen without IR.
> > 
> >   Patch 17 adds read-only check for IOAPIC entries.
> >   Patch 18 clears remote IRR bit when entry configured as
> >   edge-triggered.
> 
> IIUC, your series does not address irqfd yet, only by chance if the
> target is an IOAPIC pin for which you set up routes now. Correct?

Ah, yes, vhost and vfio should be in the notifier list as well. I
just missed that. :(

If not consider the IEC invalidation, the series should work for
irqfd, right?

> 
> Instead of fiddling with irq routes for the IOAPIC - where we don't need
> it -, I would suggest to do the following: Send IOAPIC events via
> kvm_irqchip_send_msi to the kernel. Only irqfd users (vhost, vfio)
> should use the pattern you are now applying to the IOAPIC: establish
> static routes as an irqfd is set up, and that route should be translated
> by the iommu first, register an IEC notifier to update any affected
> route when the iommu translation changes.

Yes, maybe that's the right thing to do. Or say, when split irqchip,
IOAPIC can avoid using GSI routes any more. If with that, I should
also remove lots of things, like: IEC notifiers for IOAPIC, and all
things related to msi route sync-up in IOAPIC codes with KVM (so I
suppose we will save 24 gsi route entries for KVM, which sounds
good).

(Ah... I think the series is keep growing...)

Thanks,

-- peterx

[Qemu-devel] [PATCH 19/18] intel_iommu: Add support for Extended Interrupt Mode

[Jan Kiszka]

As neither QEMU nor KVM support more than 255 CPUs so far, this is
simple: we only need to switch the destination ID translation in
vtd_remap_irq_get if EIME is set.

Once CFI support is there, it will have to take EIM into account as
well. So far, nothing to do for this.

This patch allows to use x2APIC in split irqchip mode of KVM.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

Include in v6 - if you like to. Keep for later otherwise.

 hw/i386/intel_iommu.c          | 16 +++++++++-------
 hw/i386/intel_iommu_internal.h |  2 ++
 include/hw/i386/intel_iommu.h  |  1 +
 3 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 7122e5b..23bae30 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -922,6 +922,7 @@ static void vtd_interrupt_remap_table_setup(IntelIOMMUState *s)
     value = vtd_get_quad_raw(s, DMAR_IRTA_REG);
     s->intr_size = 1UL << ((value & VTD_IRTA_SIZE_MASK) + 1);
     s->intr_root = value & VTD_IRTA_ADDR_MASK;
+    s->intr_eime = value & VTD_IRTA_EIME;
 
     /* Notify global invalidation */
     vtd_iec_notify_all(s, true, 0, 0);
@@ -2056,11 +2057,13 @@ static int vtd_remap_irq_get(IntelIOMMUState *iommu, uint16_t index, VTDIrq *irq
     irq->trigger_mode = irte.trigger_mode;
     irq->vector = irte.vector;
     irq->delivery_mode = irte.delivery_mode;
-    /* Not support EIM yet: please refer to vt-d 9.10 DST bits */
+    irq->dest = irte.dest_id;
+    if (!iommu->intr_eime) {
 #define  VTD_IR_APIC_DEST_MASK         (0xff00ULL)
 #define  VTD_IR_APIC_DEST_SHIFT        (8)
-    irq->dest = (irte.dest_id & VTD_IR_APIC_DEST_MASK) >> \
-        VTD_IR_APIC_DEST_SHIFT;
+        irq->dest = (irq->dest & VTD_IR_APIC_DEST_MASK) >>
+            VTD_IR_APIC_DEST_SHIFT;
+    }
     irq->dest_mode = irte.dest_mode;
     irq->redir_hint = irte.redir_hint;
 
@@ -2327,7 +2330,7 @@ static void vtd_init(IntelIOMMUState *s)
     s->ecap = VTD_ECAP_QI | VTD_ECAP_IRO;
 
     if (ms->iommu_intr) {
-        s->ecap |= VTD_ECAP_IR;
+        s->ecap |= VTD_ECAP_IR | VTD_ECAP_EIM;
     }
 
     vtd_reset_context_cache(s);
@@ -2381,10 +2384,9 @@ static void vtd_init(IntelIOMMUState *s)
     vtd_define_quad(s, DMAR_FRCD_REG_0_2, 0, 0, 0x8000000000000000ULL);
 
     /*
-     * Interrupt remapping registers, not support extended interrupt
-     * mode for now.
+     * Interrupt remapping registers.
      */
-    vtd_define_quad(s, DMAR_IRTA_REG, 0, 0xfffffffffffff00fULL, 0);
+    vtd_define_quad(s, DMAR_IRTA_REG, 0, 0xfffffffffffff80fULL, 0);
 }
 
 /* Should not reset address_spaces when reset because devices will still use
diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
index 10c20fe..72b0114 100644
--- a/hw/i386/intel_iommu_internal.h
+++ b/hw/i386/intel_iommu_internal.h
@@ -176,6 +176,7 @@
 
 /* IRTA_REG */
 #define VTD_IRTA_ADDR_MASK          (VTD_HAW_MASK ^ 0xfffULL)
+#define VTD_IRTA_EIME               (1ULL << 11)
 #define VTD_IRTA_SIZE_MASK          (0xfULL)
 
 /* ECAP_REG */
@@ -184,6 +185,7 @@
 #define VTD_ECAP_QI                 (1ULL << 1)
 /* Interrupt Remapping support */
 #define VTD_ECAP_IR                 (1ULL << 3)
+#define VTD_ECAP_EIM                (1ULL << 4)
 
 /* CAP_REG */
 /* (offset >> 4) << 24 */
diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
index 4fe92cf..c0c5819 100644
--- a/include/hw/i386/intel_iommu.h
+++ b/include/hw/i386/intel_iommu.h
@@ -261,6 +261,7 @@ struct IntelIOMMUState {
     bool intr_enabled;              /* Whether guest enabled IR */
     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 */
     QLIST_HEAD(, VTD_IEC_Notifier) iec_notifiers; /* IEC notify list */
 };
 
-- 
2.1.4

Re: [Qemu-devel] [PATCH v5 00/18] IOMMU: Enable interrupt remapping for Intel IOMMU

[Jan Kiszka]

On 2016-04-28 11:18, Peter Xu wrote:
> On Thu, Apr 28, 2016 at 09:19:28AM +0200, Jan Kiszka wrote:
>> On 2016-04-28 09:05, Peter Xu wrote:
>>> v5 changes:
>>> - patch 10: add vector checking for IOAPIC interrupts (this may help
>>>   debug in the future, will only generate warning if specify
>>>   IOMMU_DEBUG)
>>> - patch 13: replace error_report() with a trace. [Jan]
>>> - patch 14: rename parameter "intr" to "intremap", to be aligned
>>>   with kernel parameter [Jan]
>>> - patch 15: fix comments for vtd_iec_notify_fn
>>> - patch 17 & 18 (added): fix issue when IR enabled with devices
>>>   using level-triggered interrupts, like e1000. Adding it to the end
>>>   of series, since this issue never happen without IR.
>>>
>>>   Patch 17 adds read-only check for IOAPIC entries.
>>>   Patch 18 clears remote IRR bit when entry configured as
>>>   edge-triggered.
>>
>> IIUC, your series does not address irqfd yet, only by chance if the
>> target is an IOAPIC pin for which you set up routes now. Correct?
> 
> Ah, yes, vhost and vfio should be in the notifier list as well. I
> just missed that. :(
> 
> If not consider the IEC invalidation, the series should work for
> irqfd, right?

Hmm, yeah - you are hooking to the route update path and translate what
the kernel will get.

> 
>>
>> Instead of fiddling with irq routes for the IOAPIC - where we don't need
>> it -, I would suggest to do the following: Send IOAPIC events via
>> kvm_irqchip_send_msi to the kernel. Only irqfd users (vhost, vfio)
>> should use the pattern you are now applying to the IOAPIC: establish
>> static routes as an irqfd is set up, and that route should be translated
>> by the iommu first, register an IEC notifier to update any affected
>> route when the iommu translation changes.
> 
> Yes, maybe that's the right thing to do. Or say, when split irqchip,
> IOAPIC can avoid using GSI routes any more. If with that, I should
> also remove lots of things, like: IEC notifiers for IOAPIC, and all
> things related to msi route sync-up in IOAPIC codes with KVM (so I
> suppose we will save 24 gsi route entries for KVM, which sounds
> good).

Right.

> 
> (Ah... I think the series is keep growing...)

(Looks like I'm contributing to it, sorry ;) )

Jan

Re: [Qemu-devel] [PATCH v5 15/18] intel_iommu: introduce IEC notifiers

[David Kiarie]

On Thu, Apr 28, 2016 at 11:49 AM, Peter Xu <peterx@redhat.com> wrote:
> On Thu, Apr 28, 2016 at 10:36:19AM +0200, Jan Kiszka wrote:
>> On 2016-04-28 10:29, Peter Xu wrote:
>> > On Thu, Apr 28, 2016 at 09:26:01AM +0200, Jan Kiszka wrote:
>> >> On 2016-04-28 09:05, Peter Xu wrote:
>> >>> This patch introduces Intel VT-d IEC (Interrupt Entry Cache)
>> >>> invalidation notifier list. When vIOMMU receives IEC invalidate request,
>> >>> all the registered units will be notified with specific invalidation
>> >>> requests.
>> >>
>> >> This should be designed to be IOMMU-agnostic, i.e. become reusable for
>> >> the AMD implementation. I suspect we will have the same need for route
>> >> invalidations there as well...
>> >
>> > Yes possibly...
>> >
>> > I feel like there are lots of things that can be shared between
>> > Intel and AMD IOMMUs. I just do not know what is the most suitable
>> > "extent" that we should abstract these shared functionalities
>> > between the two, and how.
>>
>> A rough indicator: if you add something that has "vtd" in its name to
>> non-vtd code, think twice about some reusable abstraction ;). So,
>> something like "vtd_get_iommu" could already be named and designed to
>> have two provides (e.g. allow an IOMMU to register itself as provider,
>> return that registered instance(s) when requested).
>
> Yes, thanks for the hints. :)
>
> Before that, I was considering that the AMD guy who is going to add
> its support will better consider this and finally make sure the two
> coops well (anyway, I know nothing about AMD IOMMU before reading
> recent patches, and there is still no amd_iommu.c yet for me to read
> at..). But you are right, best to start consider it from the very
> beginning.

I think AMD IOMMU could be a benefit greatly from the Intel IOMMU
cache implementation. There could be a few differences but I think
much of the code could be reused. The thing is, AMD IOMMU spec doesn't
mention anything to do with it's cache design except the available
interface (for system programmers) and I have a bit of a hard time
trying to design a cache from scratch. Trying to do this will however
require me to dig too much into Intel IOMMU and I would prefer to
reserve that for later, probably.

>
>>
>> >
>> > For example, AFAIU, a better solution for current IOMMU
>> > codes (including Intel and AMD) is to provide a common framework
>> > (like...  X86IOMMU?), abstract these shared things out into a
>> > framework, like per device name spaces, iotlb, IEC notifications,
>> > etc... However, that will need a lot of further work. Also, I still
>> > do not know whether this is a good idea even in the future.
>> >
>> > So, will this be a good point that we start to think about common
>> > code blocks for both Intel and AMD IOMMU?
>>
>> The core iommu code can still be refactored later on. I'm now more
>> concerned about the hooks you add to generic code, see above.
>
> Will try to make them look better in v6. Hopefully there will have
> no vtd_*() in common codes. ;)
>
> Thanks!
>
> -- peterx

Re: [Qemu-devel] [PATCH v5 15/18] intel_iommu: introduce IEC notifiers

[Peter Xu]

On Thu, Apr 28, 2016 at 06:56:05PM +0300, David Kiarie wrote:

[...]

> I think AMD IOMMU could be a benefit greatly from the Intel IOMMU
> cache implementation. There could be a few differences but I think
> much of the code could be reused. The thing is, AMD IOMMU spec doesn't
> mention anything to do with it's cache design except the available
> interface (for system programmers) and I have a bit of a hard time
> trying to design a cache from scratch. Trying to do this will however
> require me to dig too much into Intel IOMMU and I would prefer to
> reserve that for later, probably.

Sure. I will not touch that part in this series. My first step would
be trying to remove vtd_* stuffs in common codes only, as Jan has
suggested.

Thanks,

-- peterx

Re: [Qemu-devel] [PATCH 19/18] intel_iommu: Add support for Extended Interrupt Mode

[Peter Xu]

On Thu, Apr 28, 2016 at 11:30:23AM +0200, Jan Kiszka wrote:
> As neither QEMU nor KVM support more than 255 CPUs so far, this is
> simple: we only need to switch the destination ID translation in
> vtd_remap_irq_get if EIME is set.
> 
> Once CFI support is there, it will have to take EIM into account as
> well. So far, nothing to do for this.
> 
> This patch allows to use x2APIC in split irqchip mode of KVM.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> 
> Include in v6 - if you like to. Keep for later otherwise.

Sure. Will include this in v6. :)

Thanks,

-- peterx

Re: [Qemu-devel] [PATCH v5 17/18] ioapic: keep RO bits for IOAPIC entry

[Radim Krčmář]

2016-04-28 15:05+0800, Peter Xu:
> Currently IOAPIC RO bits can be written. To be better aligned with
> hardware, we should let them read-only.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---

Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>

Re: [Qemu-devel] [PATCH v5 18/18] ioapic: clear remote irr bit for edge-triggered interrupts

[Radim Krčmář]

2016-04-28 15:05+0800, Peter Xu:
> This is to better emulate IOAPIC version 0x1X hardware. Linux kernel
> leveraged this "feature" to do explicit EOI since EOI register is still
> not introduced at that time. This will also fix the issue that level
> triggered interrupts failed to work when IR enabled (tested with Linux
> kernel version 4.5).
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---

Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>

Re: [Qemu-devel] [PATCH v5 00/18] IOMMU: Enable interrupt remapping for Intel IOMMU

[Radim Krčmář]

2016-04-28 17:18+0800, Peter Xu:
> On Thu, Apr 28, 2016 at 09:19:28AM +0200, Jan Kiszka wrote:
>> Instead of fiddling with irq routes for the IOAPIC - where we don't need
>> it -, I would suggest to do the following: Send IOAPIC events via
>> kvm_irqchip_send_msi to the kernel. Only irqfd users (vhost, vfio)
>> should use the pattern you are now applying to the IOAPIC: establish
>> static routes as an irqfd is set up, and that route should be translated
>> by the iommu first, register an IEC notifier to update any affected
>> route when the iommu translation changes.
> 
> Yes, maybe that's the right thing to do. Or say, when split irqchip,
> IOAPIC can avoid using GSI routes any more. If with that, I should
> also remove lots of things, like: IEC notifiers for IOAPIC, and all
> things related to msi route sync-up in IOAPIC codes with KVM (so I
> suppose we will save 24 gsi route entries for KVM, which sounds
> good).

Sadly, we can't get rid of those GSI routes.  KVM uses them to decide
whether it should forward EOI to userspace.  And QEMU also has to remap
them, because KVM uses dest_id for that decision.

Re: [Qemu-devel] [PATCH v5 00/18] IOMMU: Enable interrupt remapping for Intel IOMMU

[Peter Xu]

On Fri, Apr 29, 2016 at 09:52:14PM +0200, Radim Krčmář wrote:
> 2016-04-28 17:18+0800, Peter Xu:
> > On Thu, Apr 28, 2016 at 09:19:28AM +0200, Jan Kiszka wrote:
> >> Instead of fiddling with irq routes for the IOAPIC - where we don't need
> >> it -, I would suggest to do the following: Send IOAPIC events via
> >> kvm_irqchip_send_msi to the kernel. Only irqfd users (vhost, vfio)
> >> should use the pattern you are now applying to the IOAPIC: establish
> >> static routes as an irqfd is set up, and that route should be translated
> >> by the iommu first, register an IEC notifier to update any affected
> >> route when the iommu translation changes.
> > 
> > Yes, maybe that's the right thing to do. Or say, when split irqchip,
> > IOAPIC can avoid using GSI routes any more. If with that, I should
> > also remove lots of things, like: IEC notifiers for IOAPIC, and all
> > things related to msi route sync-up in IOAPIC codes with KVM (so I
> > suppose we will save 24 gsi route entries for KVM, which sounds
> > good).
> 
> Sadly, we can't get rid of those GSI routes.  KVM uses them to decide
> whether it should forward EOI to userspace.  And QEMU also has to remap
> them, because KVM uses dest_id for that decision.

Thanks to point out.  Actually I have started to test the new patch
and did encountered issue when using split irqchip mode. The above
explains. :)

So I think I will keep this part of codes un-touched in this
series, and I can move forward with IR changes.

However, I am still wondering whether we can avoid this in KVM when
when using split irqchip mode? E.g., what if we do not do this check
in KVM and let all EOI be passed to userspace? Like:

---

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 36591fa..c0a6e51 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -936,10 +936,6 @@ static void kvm_ioapic_send_eoi(struct kvm_lapic *apic, int vector)
 {
        int trigger_mode;

-       /* Eoi the ioapic only if the ioapic doesn't own the vector. */
-       if (!kvm_ioapic_handles_vector(apic, vector))
-               return;
-
        /* Request a KVM exit to inform the userspace IOAPIC. */
        if (irqchip_split(apic->vcpu->kvm)) {
                apic->vcpu->arch.pending_ioapic_eoi = vector;
@@ -947,6 +943,10 @@ static void kvm_ioapic_send_eoi(struct kvm_lapic *apic, int vector)
                return;
        }

+       /* Eoi the ioapic only if the ioapic doesn't own the vector. */
+       if (!kvm_ioapic_handles_vector(apic, vector))
+               return;
+
        if (apic_test_vector(vector, apic->regs + APIC_TMR))
                trigger_mode = IOAPIC_LEVEL_TRIG;
        else

---

I believe this will brings more user-space context switches, I just
do not know how this will impact the system (only for curiosity :).

Thanks,

-- peterx

Re: [Qemu-devel] [PATCH v5 00/18] IOMMU: Enable interrupt remapping for Intel IOMMU

[Jan Kiszka]

On 2016-05-03 05:22, Peter Xu wrote:
> On Fri, Apr 29, 2016 at 09:52:14PM +0200, Radim Krčmář wrote:
>> 2016-04-28 17:18+0800, Peter Xu:
>>> On Thu, Apr 28, 2016 at 09:19:28AM +0200, Jan Kiszka wrote:
>>>> Instead of fiddling with irq routes for the IOAPIC - where we don't need
>>>> it -, I would suggest to do the following: Send IOAPIC events via
>>>> kvm_irqchip_send_msi to the kernel. Only irqfd users (vhost, vfio)
>>>> should use the pattern you are now applying to the IOAPIC: establish
>>>> static routes as an irqfd is set up, and that route should be translated
>>>> by the iommu first, register an IEC notifier to update any affected
>>>> route when the iommu translation changes.
>>>
>>> Yes, maybe that's the right thing to do. Or say, when split irqchip,
>>> IOAPIC can avoid using GSI routes any more. If with that, I should
>>> also remove lots of things, like: IEC notifiers for IOAPIC, and all
>>> things related to msi route sync-up in IOAPIC codes with KVM (so I
>>> suppose we will save 24 gsi route entries for KVM, which sounds
>>> good).
>>
>> Sadly, we can't get rid of those GSI routes.  KVM uses them to decide
>> whether it should forward EOI to userspace.  And QEMU also has to remap
>> them, because KVM uses dest_id for that decision.
> 
> Thanks to point out.  Actually I have started to test the new patch
> and did encountered issue when using split irqchip mode. The above
> explains. :)
> 
> So I think I will keep this part of codes un-touched in this
> series, and I can move forward with IR changes.
> 
> However, I am still wondering whether we can avoid this in KVM when
> when using split irqchip mode? E.g., what if we do not do this check
> in KVM and let all EOI be passed to userspace? Like:
> 
> ---
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 36591fa..c0a6e51 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -936,10 +936,6 @@ static void kvm_ioapic_send_eoi(struct kvm_lapic *apic, int vector)
>  {
>         int trigger_mode;
> 
> -       /* Eoi the ioapic only if the ioapic doesn't own the vector. */
> -       if (!kvm_ioapic_handles_vector(apic, vector))
> -               return;
> -
>         /* Request a KVM exit to inform the userspace IOAPIC. */
>         if (irqchip_split(apic->vcpu->kvm)) {
>                 apic->vcpu->arch.pending_ioapic_eoi = vector;
> @@ -947,6 +943,10 @@ static void kvm_ioapic_send_eoi(struct kvm_lapic *apic, int vector)
>                 return;
>         }
> 
> +       /* Eoi the ioapic only if the ioapic doesn't own the vector. */
> +       if (!kvm_ioapic_handles_vector(apic, vector))
> +               return;
> +
>         if (apic_test_vector(vector, apic->regs + APIC_TMR))
>                 trigger_mode = IOAPIC_LEVEL_TRIG;
>         else
> 
> ---
> 
> I believe this will brings more user-space context switches, I just
> do not know how this will impact the system (only for curiosity :).

Yes, this doesn't look good from the performance POV. Given that most
EOIs of the APICs will not trigger a message to an IOAPIC and userspace
exits are expensive, that should be measurable.

But you should optimize route updating a bit: I noticed real delays
(almost a second) when reprogramming all IOAPIC pins during Jailhouse
handover. That's because of the heavyweight synchronize_srcu we have on
each route change. Even if that is an extreme case, you should try to
reduce route updates to only those that were actually affected by an
IRTE change.

Jan

Re: [Qemu-devel] [PATCH v5 00/18] IOMMU: Enable interrupt remapping for Intel IOMMU

[Peter Xu]

On Tue, May 03, 2016 at 06:38:28AM +0200, Jan Kiszka wrote:
> On 2016-05-03 05:22, Peter Xu wrote:
> > On Fri, Apr 29, 2016 at 09:52:14PM +0200, Radim Krčmář wrote:
> >> 2016-04-28 17:18+0800, Peter Xu:
> >>> On Thu, Apr 28, 2016 at 09:19:28AM +0200, Jan Kiszka wrote:
> >>>> Instead of fiddling with irq routes for the IOAPIC - where we don't need
> >>>> it -, I would suggest to do the following: Send IOAPIC events via
> >>>> kvm_irqchip_send_msi to the kernel. Only irqfd users (vhost, vfio)
> >>>> should use the pattern you are now applying to the IOAPIC: establish
> >>>> static routes as an irqfd is set up, and that route should be translated
> >>>> by the iommu first, register an IEC notifier to update any affected
> >>>> route when the iommu translation changes.
> >>>
> >>> Yes, maybe that's the right thing to do. Or say, when split irqchip,
> >>> IOAPIC can avoid using GSI routes any more. If with that, I should
> >>> also remove lots of things, like: IEC notifiers for IOAPIC, and all
> >>> things related to msi route sync-up in IOAPIC codes with KVM (so I
> >>> suppose we will save 24 gsi route entries for KVM, which sounds
> >>> good).
> >>
> >> Sadly, we can't get rid of those GSI routes.  KVM uses them to decide
> >> whether it should forward EOI to userspace.  And QEMU also has to remap
> >> them, because KVM uses dest_id for that decision.
> > 
> > Thanks to point out.  Actually I have started to test the new patch
> > and did encountered issue when using split irqchip mode. The above
> > explains. :)
> > 
> > So I think I will keep this part of codes un-touched in this
> > series, and I can move forward with IR changes.
> > 
> > However, I am still wondering whether we can avoid this in KVM when
> > when using split irqchip mode? E.g., what if we do not do this check
> > in KVM and let all EOI be passed to userspace? Like:
> > 
> > ---
> > 
> > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > index 36591fa..c0a6e51 100644
> > --- a/arch/x86/kvm/lapic.c
> > +++ b/arch/x86/kvm/lapic.c
> > @@ -936,10 +936,6 @@ static void kvm_ioapic_send_eoi(struct kvm_lapic *apic, int vector)
> >  {
> >         int trigger_mode;
> > 
> > -       /* Eoi the ioapic only if the ioapic doesn't own the vector. */
> > -       if (!kvm_ioapic_handles_vector(apic, vector))
> > -               return;
> > -
> >         /* Request a KVM exit to inform the userspace IOAPIC. */
> >         if (irqchip_split(apic->vcpu->kvm)) {
> >                 apic->vcpu->arch.pending_ioapic_eoi = vector;
> > @@ -947,6 +943,10 @@ static void kvm_ioapic_send_eoi(struct kvm_lapic *apic, int vector)
> >                 return;
> >         }
> > 
> > +       /* Eoi the ioapic only if the ioapic doesn't own the vector. */
> > +       if (!kvm_ioapic_handles_vector(apic, vector))
> > +               return;
> > +
> >         if (apic_test_vector(vector, apic->regs + APIC_TMR))
> >                 trigger_mode = IOAPIC_LEVEL_TRIG;
> >         else
> > 
> > ---
> > 
> > I believe this will brings more user-space context switches, I just
> > do not know how this will impact the system (only for curiosity :).
> 
> Yes, this doesn't look good from the performance POV. Given that most
> EOIs of the APICs will not trigger a message to an IOAPIC and userspace
> exits are expensive, that should be measurable.
> 
> But you should optimize route updating a bit: I noticed real delays
> (almost a second) when reprogramming all IOAPIC pins during Jailhouse
> handover. That's because of the heavyweight synchronize_srcu we have on
> each route change. Even if that is an extreme case, you should try to
> reduce route updates to only those that were actually affected by an
> IRTE change.

Yes, this solution will possibly need one more IOMMU API besides
int_remap(), for IOAPIC to know whether specific IOAPIC entry needs
to be updated (IOAPIC needs to know the index information of each
IOAPIC entry, and see whether it is matched with IEC notified index
and mask, in ioapic_iec_notifier()).

Another problem that may cause this issue is that: now we are
committing route information every time when updating route entries
(please check kvm_update_routing_entry()). IMHO this is not
necessary - we will need 25 times of committing for each IEC
invalication, while actually we only need one at the end.

So... We'll have another solution for this: how about not committing
for each update, and just do one commit after all changes settled?
This seems to be much cleaner, and easier comparing to the above
proposal.

(These two solutions actually solves different problems, and either
 of them should help in reducing the 1 sec delay mentioned
 above. For now, I'd prefer first choose the 2nd solution to do
 explicit route commit, and add the 1st one into my todo list)

Thanks,

-- peterx

Re: [Qemu-devel] [PATCH v5 00/18] IOMMU: Enable interrupt remapping for Intel IOMMU

[Jan Kiszka]

On 2016-05-03 07:30, Peter Xu wrote:
> On Tue, May 03, 2016 at 06:38:28AM +0200, Jan Kiszka wrote:
>> On 2016-05-03 05:22, Peter Xu wrote:
>>> On Fri, Apr 29, 2016 at 09:52:14PM +0200, Radim Krčmář wrote:
>>>> 2016-04-28 17:18+0800, Peter Xu:
>>>>> On Thu, Apr 28, 2016 at 09:19:28AM +0200, Jan Kiszka wrote:
>>>>>> Instead of fiddling with irq routes for the IOAPIC - where we don't need
>>>>>> it -, I would suggest to do the following: Send IOAPIC events via
>>>>>> kvm_irqchip_send_msi to the kernel. Only irqfd users (vhost, vfio)
>>>>>> should use the pattern you are now applying to the IOAPIC: establish
>>>>>> static routes as an irqfd is set up, and that route should be translated
>>>>>> by the iommu first, register an IEC notifier to update any affected
>>>>>> route when the iommu translation changes.
>>>>>
>>>>> Yes, maybe that's the right thing to do. Or say, when split irqchip,
>>>>> IOAPIC can avoid using GSI routes any more. If with that, I should
>>>>> also remove lots of things, like: IEC notifiers for IOAPIC, and all
>>>>> things related to msi route sync-up in IOAPIC codes with KVM (so I
>>>>> suppose we will save 24 gsi route entries for KVM, which sounds
>>>>> good).
>>>>
>>>> Sadly, we can't get rid of those GSI routes.  KVM uses them to decide
>>>> whether it should forward EOI to userspace.  And QEMU also has to remap
>>>> them, because KVM uses dest_id for that decision.
>>>
>>> Thanks to point out.  Actually I have started to test the new patch
>>> and did encountered issue when using split irqchip mode. The above
>>> explains. :)
>>>
>>> So I think I will keep this part of codes un-touched in this
>>> series, and I can move forward with IR changes.
>>>
>>> However, I am still wondering whether we can avoid this in KVM when
>>> when using split irqchip mode? E.g., what if we do not do this check
>>> in KVM and let all EOI be passed to userspace? Like:
>>>
>>> ---
>>>
>>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>>> index 36591fa..c0a6e51 100644
>>> --- a/arch/x86/kvm/lapic.c
>>> +++ b/arch/x86/kvm/lapic.c
>>> @@ -936,10 +936,6 @@ static void kvm_ioapic_send_eoi(struct kvm_lapic *apic, int vector)
>>>  {
>>>         int trigger_mode;
>>>
>>> -       /* Eoi the ioapic only if the ioapic doesn't own the vector. */
>>> -       if (!kvm_ioapic_handles_vector(apic, vector))
>>> -               return;
>>> -
>>>         /* Request a KVM exit to inform the userspace IOAPIC. */
>>>         if (irqchip_split(apic->vcpu->kvm)) {
>>>                 apic->vcpu->arch.pending_ioapic_eoi = vector;
>>> @@ -947,6 +943,10 @@ static void kvm_ioapic_send_eoi(struct kvm_lapic *apic, int vector)
>>>                 return;
>>>         }
>>>
>>> +       /* Eoi the ioapic only if the ioapic doesn't own the vector. */
>>> +       if (!kvm_ioapic_handles_vector(apic, vector))
>>> +               return;
>>> +
>>>         if (apic_test_vector(vector, apic->regs + APIC_TMR))
>>>                 trigger_mode = IOAPIC_LEVEL_TRIG;
>>>         else
>>>
>>> ---
>>>
>>> I believe this will brings more user-space context switches, I just
>>> do not know how this will impact the system (only for curiosity :).
>>
>> Yes, this doesn't look good from the performance POV. Given that most
>> EOIs of the APICs will not trigger a message to an IOAPIC and userspace
>> exits are expensive, that should be measurable.
>>
>> But you should optimize route updating a bit: I noticed real delays
>> (almost a second) when reprogramming all IOAPIC pins during Jailhouse
>> handover. That's because of the heavyweight synchronize_srcu we have on
>> each route change. Even if that is an extreme case, you should try to
>> reduce route updates to only those that were actually affected by an
>> IRTE change.
> 
> Yes, this solution will possibly need one more IOMMU API besides
> int_remap(), for IOAPIC to know whether specific IOAPIC entry needs
> to be updated (IOAPIC needs to know the index information of each
> IOAPIC entry, and see whether it is matched with IEC notified index
> and mask, in ioapic_iec_notifier()).
> 
> Another problem that may cause this issue is that: now we are
> committing route information every time when updating route entries
> (please check kvm_update_routing_entry()). IMHO this is not
> necessary - we will need 25 times of committing for each IEC
> invalication, while actually we only need one at the end.
> 
> So... We'll have another solution for this: how about not committing
> for each update, and just do one commit after all changes settled?
> This seems to be much cleaner, and easier comparing to the above
> proposal.

If you see a number of invalidation requests queued in a row, you may
also process them atomically from the guest POV, which includes a single
route update at the end, before the guest gets told that everything was
done.

Jan

> 
> (These two solutions actually solves different problems, and either
>  of them should help in reducing the 1 sec delay mentioned
>  above. For now, I'd prefer first choose the 2nd solution to do
>  explicit route commit, and add the 1st one into my todo list)
> 
> Thanks,
> 
> -- peterx
> 

Re: [Qemu-devel] [PATCH v5 00/18] IOMMU: Enable interrupt remapping for Intel IOMMU

[Peter Xu]

On Tue, May 03, 2016 at 07:40:50AM +0200, Jan Kiszka wrote:
> On 2016-05-03 07:30, Peter Xu wrote:
> > On Tue, May 03, 2016 at 06:38:28AM +0200, Jan Kiszka wrote:
> >> On 2016-05-03 05:22, Peter Xu wrote:
> >>> On Fri, Apr 29, 2016 at 09:52:14PM +0200, Radim Krčmář wrote:
> >>>> 2016-04-28 17:18+0800, Peter Xu:
> >>>>> On Thu, Apr 28, 2016 at 09:19:28AM +0200, Jan Kiszka wrote:
> >>>>>> Instead of fiddling with irq routes for the IOAPIC - where we don't need
> >>>>>> it -, I would suggest to do the following: Send IOAPIC events via
> >>>>>> kvm_irqchip_send_msi to the kernel. Only irqfd users (vhost, vfio)
> >>>>>> should use the pattern you are now applying to the IOAPIC: establish
> >>>>>> static routes as an irqfd is set up, and that route should be translated
> >>>>>> by the iommu first, register an IEC notifier to update any affected
> >>>>>> route when the iommu translation changes.
> >>>>>
> >>>>> Yes, maybe that's the right thing to do. Or say, when split irqchip,
> >>>>> IOAPIC can avoid using GSI routes any more. If with that, I should
> >>>>> also remove lots of things, like: IEC notifiers for IOAPIC, and all
> >>>>> things related to msi route sync-up in IOAPIC codes with KVM (so I
> >>>>> suppose we will save 24 gsi route entries for KVM, which sounds
> >>>>> good).
> >>>>
> >>>> Sadly, we can't get rid of those GSI routes.  KVM uses them to decide
> >>>> whether it should forward EOI to userspace.  And QEMU also has to remap
> >>>> them, because KVM uses dest_id for that decision.
> >>>
> >>> Thanks to point out.  Actually I have started to test the new patch
> >>> and did encountered issue when using split irqchip mode. The above
> >>> explains. :)
> >>>
> >>> So I think I will keep this part of codes un-touched in this
> >>> series, and I can move forward with IR changes.
> >>>
> >>> However, I am still wondering whether we can avoid this in KVM when
> >>> when using split irqchip mode? E.g., what if we do not do this check
> >>> in KVM and let all EOI be passed to userspace? Like:
> >>>
> >>> ---
> >>>
> >>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> >>> index 36591fa..c0a6e51 100644
> >>> --- a/arch/x86/kvm/lapic.c
> >>> +++ b/arch/x86/kvm/lapic.c
> >>> @@ -936,10 +936,6 @@ static void kvm_ioapic_send_eoi(struct kvm_lapic *apic, int vector)
> >>>  {
> >>>         int trigger_mode;
> >>>
> >>> -       /* Eoi the ioapic only if the ioapic doesn't own the vector. */
> >>> -       if (!kvm_ioapic_handles_vector(apic, vector))
> >>> -               return;
> >>> -
> >>>         /* Request a KVM exit to inform the userspace IOAPIC. */
> >>>         if (irqchip_split(apic->vcpu->kvm)) {
> >>>                 apic->vcpu->arch.pending_ioapic_eoi = vector;
> >>> @@ -947,6 +943,10 @@ static void kvm_ioapic_send_eoi(struct kvm_lapic *apic, int vector)
> >>>                 return;
> >>>         }
> >>>
> >>> +       /* Eoi the ioapic only if the ioapic doesn't own the vector. */
> >>> +       if (!kvm_ioapic_handles_vector(apic, vector))
> >>> +               return;
> >>> +
> >>>         if (apic_test_vector(vector, apic->regs + APIC_TMR))
> >>>                 trigger_mode = IOAPIC_LEVEL_TRIG;
> >>>         else
> >>>
> >>> ---
> >>>
> >>> I believe this will brings more user-space context switches, I just
> >>> do not know how this will impact the system (only for curiosity :).
> >>
> >> Yes, this doesn't look good from the performance POV. Given that most
> >> EOIs of the APICs will not trigger a message to an IOAPIC and userspace
> >> exits are expensive, that should be measurable.
> >>
> >> But you should optimize route updating a bit: I noticed real delays
> >> (almost a second) when reprogramming all IOAPIC pins during Jailhouse
> >> handover. That's because of the heavyweight synchronize_srcu we have on
> >> each route change. Even if that is an extreme case, you should try to
> >> reduce route updates to only those that were actually affected by an
> >> IRTE change.
> > 
> > Yes, this solution will possibly need one more IOMMU API besides
> > int_remap(), for IOAPIC to know whether specific IOAPIC entry needs
> > to be updated (IOAPIC needs to know the index information of each
> > IOAPIC entry, and see whether it is matched with IEC notified index
> > and mask, in ioapic_iec_notifier()).
> > 
> > Another problem that may cause this issue is that: now we are
> > committing route information every time when updating route entries
> > (please check kvm_update_routing_entry()). IMHO this is not
> > necessary - we will need 25 times of committing for each IEC
> > invalication, while actually we only need one at the end.
> > 
> > So... We'll have another solution for this: how about not committing
> > for each update, and just do one commit after all changes settled?
> > This seems to be much cleaner, and easier comparing to the above
> > proposal.
> 
> If you see a number of invalidation requests queued in a row, you may
> also process them atomically from the guest POV, which includes a single
> route update at the end, before the guest gets told that everything was
> done.

Yes, this would be better as a 3rd step after the above two. If you
would not mind, I'll noting this down as well with the 1st proposal
(considering that this will need more code changes, not only in QI
logic, also in IEC notification path), and will only contain the 2nd
change (explicit route commit) in v6, to partially solve the problem
for now.

Thanks,

-- peterx

Re: [Qemu-devel] [PATCH v5 00/18] IOMMU: Enable interrupt remapping for Intel IOMMU

[Jan Kiszka]

On 2016-05-03 08:00, Peter Xu wrote:
> On Tue, May 03, 2016 at 07:40:50AM +0200, Jan Kiszka wrote:
>> On 2016-05-03 07:30, Peter Xu wrote:
>>> On Tue, May 03, 2016 at 06:38:28AM +0200, Jan Kiszka wrote:
>>>> On 2016-05-03 05:22, Peter Xu wrote:
>>>>> On Fri, Apr 29, 2016 at 09:52:14PM +0200, Radim Krčmář wrote:
>>>>>> 2016-04-28 17:18+0800, Peter Xu:
>>>>>>> On Thu, Apr 28, 2016 at 09:19:28AM +0200, Jan Kiszka wrote:
>>>>>>>> Instead of fiddling with irq routes for the IOAPIC - where we don't need
>>>>>>>> it -, I would suggest to do the following: Send IOAPIC events via
>>>>>>>> kvm_irqchip_send_msi to the kernel. Only irqfd users (vhost, vfio)
>>>>>>>> should use the pattern you are now applying to the IOAPIC: establish
>>>>>>>> static routes as an irqfd is set up, and that route should be translated
>>>>>>>> by the iommu first, register an IEC notifier to update any affected
>>>>>>>> route when the iommu translation changes.
>>>>>>>
>>>>>>> Yes, maybe that's the right thing to do. Or say, when split irqchip,
>>>>>>> IOAPIC can avoid using GSI routes any more. If with that, I should
>>>>>>> also remove lots of things, like: IEC notifiers for IOAPIC, and all
>>>>>>> things related to msi route sync-up in IOAPIC codes with KVM (so I
>>>>>>> suppose we will save 24 gsi route entries for KVM, which sounds
>>>>>>> good).
>>>>>>
>>>>>> Sadly, we can't get rid of those GSI routes.  KVM uses them to decide
>>>>>> whether it should forward EOI to userspace.  And QEMU also has to remap
>>>>>> them, because KVM uses dest_id for that decision.
>>>>>
>>>>> Thanks to point out.  Actually I have started to test the new patch
>>>>> and did encountered issue when using split irqchip mode. The above
>>>>> explains. :)
>>>>>
>>>>> So I think I will keep this part of codes un-touched in this
>>>>> series, and I can move forward with IR changes.
>>>>>
>>>>> However, I am still wondering whether we can avoid this in KVM when
>>>>> when using split irqchip mode? E.g., what if we do not do this check
>>>>> in KVM and let all EOI be passed to userspace? Like:
>>>>>
>>>>> ---
>>>>>
>>>>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>>>>> index 36591fa..c0a6e51 100644
>>>>> --- a/arch/x86/kvm/lapic.c
>>>>> +++ b/arch/x86/kvm/lapic.c
>>>>> @@ -936,10 +936,6 @@ static void kvm_ioapic_send_eoi(struct kvm_lapic *apic, int vector)
>>>>>  {
>>>>>         int trigger_mode;
>>>>>
>>>>> -       /* Eoi the ioapic only if the ioapic doesn't own the vector. */
>>>>> -       if (!kvm_ioapic_handles_vector(apic, vector))
>>>>> -               return;
>>>>> -
>>>>>         /* Request a KVM exit to inform the userspace IOAPIC. */
>>>>>         if (irqchip_split(apic->vcpu->kvm)) {
>>>>>                 apic->vcpu->arch.pending_ioapic_eoi = vector;
>>>>> @@ -947,6 +943,10 @@ static void kvm_ioapic_send_eoi(struct kvm_lapic *apic, int vector)
>>>>>                 return;
>>>>>         }
>>>>>
>>>>> +       /* Eoi the ioapic only if the ioapic doesn't own the vector. */
>>>>> +       if (!kvm_ioapic_handles_vector(apic, vector))
>>>>> +               return;
>>>>> +
>>>>>         if (apic_test_vector(vector, apic->regs + APIC_TMR))
>>>>>                 trigger_mode = IOAPIC_LEVEL_TRIG;
>>>>>         else
>>>>>
>>>>> ---
>>>>>
>>>>> I believe this will brings more user-space context switches, I just
>>>>> do not know how this will impact the system (only for curiosity :).
>>>>
>>>> Yes, this doesn't look good from the performance POV. Given that most
>>>> EOIs of the APICs will not trigger a message to an IOAPIC and userspace
>>>> exits are expensive, that should be measurable.
>>>>
>>>> But you should optimize route updating a bit: I noticed real delays
>>>> (almost a second) when reprogramming all IOAPIC pins during Jailhouse
>>>> handover. That's because of the heavyweight synchronize_srcu we have on
>>>> each route change. Even if that is an extreme case, you should try to
>>>> reduce route updates to only those that were actually affected by an
>>>> IRTE change.
>>>
>>> Yes, this solution will possibly need one more IOMMU API besides
>>> int_remap(), for IOAPIC to know whether specific IOAPIC entry needs
>>> to be updated (IOAPIC needs to know the index information of each
>>> IOAPIC entry, and see whether it is matched with IEC notified index
>>> and mask, in ioapic_iec_notifier()).
>>>
>>> Another problem that may cause this issue is that: now we are
>>> committing route information every time when updating route entries
>>> (please check kvm_update_routing_entry()). IMHO this is not
>>> necessary - we will need 25 times of committing for each IEC
>>> invalication, while actually we only need one at the end.
>>>
>>> So... We'll have another solution for this: how about not committing
>>> for each update, and just do one commit after all changes settled?
>>> This seems to be much cleaner, and easier comparing to the above
>>> proposal.
>>
>> If you see a number of invalidation requests queued in a row, you may
>> also process them atomically from the guest POV, which includes a single
>> route update at the end, before the guest gets told that everything was
>> done.
> 
> Yes, this would be better as a 3rd step after the above two. If you
> would not mind, I'll noting this down as well with the 1st proposal
> (considering that this will need more code changes, not only in QI
> logic, also in IEC notification path), and will only contain the 2nd
> change (explicit route commit) in v6, to partially solve the problem
> for now.

Ack.

Jan

Re: [Qemu-devel] [PATCH v5 00/18] IOMMU: Enable interrupt remapping for Intel IOMMU

[Paolo Bonzini]

On 03/05/2016 06:38, Jan Kiszka wrote:
> Yes, this doesn't look good from the performance POV. Given that most
> EOIs of the APICs will not trigger a message to an IOAPIC and userspace
> exits are expensive, that should be measurable.
> 
> But you should optimize route updating a bit: I noticed real delays
> (almost a second) when reprogramming all IOAPIC pins during Jailhouse
> handover. That's because of the heavyweight synchronize_srcu we have on
> each route change. Even if that is an extreme case, you should try to
> reduce route updates to only those that were actually affected by an
> IRTE change.

It's okay to make it synchronize_srcu_expedited.  Unlike RCU, expediting
SRCU grace periods doesn't have an impact on the whole system (the
reason why KVM uses SRCU here is exactly to avoid
synchronize_rcu_expedited).

Of course, optimizing userspace cannot hurt either.

Paolo