[Qemu-devel] [PATCH v2 00/13] IOMMU: Enable interrupt remapping for Intel IOMMU

[Qemu-devel] [PATCH v2 00/13] IOMMU: Enable interrupt remapping for Intel IOMMU

[Peter Xu]

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 basic network (still do not support kvm-ioapic,
so we need to specify kernel-irqchip=off here):

$ qemu-system-x86_64 -M q35,iommu=on,intr=on,kernel-irqchip=off \
     -enable-kvm -m 1024 -s \
     -monitor telnet::3333,server,nowait \
     -netdev user,id=user.0,hostfwd=tcp::5555-:22 \
     -device virtio-net-pci,netdev=user.0 \
	 /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 only two devices are supported:

- Emulated IOAPIC device
- PCI Devices

TODO List:

- kvm-ioapic support
- vhost support
- pass through device support
- EIM support
- IR fault reporting
- source-id validation for IRTE
- IRTE cache and better queued invalidation
- migration support (for IOMMU as general?)
- more?

Peter Xu (13):
  q35: add "int-remap" flag to enable intr
  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
  ioapic-common: add iommu for IOAPICCommonState
  intel_iommu: add IR translation faults defines
  intel_iommu: ioapic: IR support for emulated IOAPIC
  intel_iommu: Add support for PCI MSI remap

 hw/core/machine.c                 |  21 ++
 hw/i386/acpi-build.c              |  42 ++--
 hw/i386/intel_iommu.c             | 400 +++++++++++++++++++++++++++++++++++++-
 hw/i386/intel_iommu_internal.h    |  23 +++
 hw/intc/ioapic.c                  |  36 +++-
 hw/intc/ioapic_common.c           |   2 +
 include/hw/acpi/acpi-defs.h       |  15 ++
 include/hw/boards.h               |   1 +
 include/hw/i386/intel_iommu.h     | 120 ++++++++++++
 include/hw/i386/ioapic_internal.h |   3 +
 include/hw/pci/msi.h              |   4 +
 11 files changed, 647 insertions(+), 20 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH v2 01/13] q35: add "int-remap" flag to enable intr

[Peter Xu]

One flag is added to specify whether to enable INTR 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,intr=on

To be more clear, the following command:

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

Will enable IOMMU only, without interrupt remapping support.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/core/machine.c   | 21 +++++++++++++++++++++
 include/hw/boards.h |  1 +
 2 files changed, 22 insertions(+)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 6dbbc85..b48fcaf 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_intr(Object *obj, Error **errp)
+{
+    MachineState *ms = MACHINE(obj);
+
+    return ms->iommu_intr;
+}
+
+static void machine_set_intr(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);
@@ -382,6 +396,7 @@ static void machine_initfn(Object *obj)
     ms->kvm_shadow_mem = -1;
     ms->dump_guest_core = true;
     ms->mem_merge = true;
+    ms->iommu_intr = false;
 
     object_property_add_str(obj, "accel",
                             machine_get_accel, machine_set_accel, NULL);
@@ -478,6 +493,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, "intr", machine_get_intr,
+                             machine_set_intr, NULL);
+    object_property_set_description(obj, "intr",
+                                    "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);
diff --git a/include/hw/boards.h b/include/hw/boards.h
index aad5f2a..5e15410 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -151,6 +151,7 @@ struct MachineState {
     bool igd_gfx_passthru;
     char *firmware;
     bool iommu;
+    bool iommu_intr;
     bool suppress_vmdesc;
     bool enforce_config_section;
 
-- 
2.4.3

[Qemu-devel] [PATCH v2 02/13] acpi: enable INTR for DMAR report structure

[Peter Xu]

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

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

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 35180ef..cf0121e 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2565,16 +2565,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));
@@ -2735,7 +2741,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/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 v2 03/13] 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 v2 04/13] 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 v2 05/13] 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        | 23 +++++++++++++++++++++--
 include/hw/acpi/acpi-defs.h | 15 +++++++++++++++
 2 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index cf0121e..5ad6f9a 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);
@@ -2572,6 +2574,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 */
@@ -2585,11 +2590,25 @@ 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;
+    /*
+     * An arbitary but unique bus number, to be used to generate
+     * source ID for IOAPIC device in BDF format.
+     */
+#define ACPI_IOAPIC_BUS_IR         (0xff)
+#define ACPI_IOAPIC_DEVFN_IR       (0x00)
+    scope->enumeration_id = cpu_to_le16(ACPI_BUILD_IOAPIC_ID);
+    scope->bus = cpu_to_le16(ACPI_IOAPIC_BUS_IR);
+    scope->path[0] = cpu_to_le16(ACPI_IOAPIC_DEVFN_IR);
+
     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;
 
-- 
2.4.3

[Qemu-devel] [PATCH v2 06/13] 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 v2 07/13] 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 v2 08/13] 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 v2 09/13] 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 5ad6f9a..213f174 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2673,12 +2673,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 v2 10/13] ioapic-common: add iommu for IOAPICCommonState

[Peter Xu]

When IR is enabled for IOMMU, each IOAPIC will belong to a specific
intel IOMMU. This pointer will store the owner of current IOAPIC, which
is always the default IOMMU device.

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

diff --git a/hw/intc/ioapic_common.c b/hw/intc/ioapic_common.c
index 1b7ec5e..f5b6417 100644
--- a/hw/intc/ioapic_common.c
+++ b/hw/intc/ioapic_common.c
@@ -137,6 +137,8 @@ static void ioapic_common_realize(DeviceState *dev, Error **errp)
 
     sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->io_memory);
     ioapic_no++;
+
+    s->iommu = vtd_iommu_get();
 }
 
 static const VMStateDescription vmstate_ioapic_common = {
diff --git a/include/hw/i386/ioapic_internal.h b/include/hw/i386/ioapic_internal.h
index 797ed47..41fc282 100644
--- a/include/hw/i386/ioapic_internal.h
+++ b/include/hw/i386/ioapic_internal.h
@@ -25,6 +25,7 @@
 #include "hw/hw.h"
 #include "exec/memory.h"
 #include "hw/sysbus.h"
+#include "hw/i386/intel_iommu.h"
 
 #define MAX_IOAPICS                     1
 
@@ -101,6 +102,8 @@ struct IOAPICCommonState {
     uint8_t ioregsel;
     uint32_t irr;
     uint64_t ioredtbl[IOAPIC_NUM_PINS];
+    /* IOMMU pointer that this IOAPIC belongs. */
+    IntelIOMMUState *iommu;
 };
 
 void ioapic_reset_common(DeviceState *dev);
-- 
2.4.3

[Qemu-devel] [PATCH v2 11/13] 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 v2 12/13] intel_iommu: ioapic: IR support for emulated IOAPIC

[Peter Xu]

This patch add the first device support for Intel IOMMU interrupt
remapping, which is the default IOAPIC device created alongside with Q35
platform. This will be the first step along the way to fully enable
IOMMU IR on x86 systems.

Currently, only emluated IOAPIC is supported. This requires
"kernel_irqchip=off" parameter specified.

Originally, IOAPIC has its own table to maintain IRQ information. When
IOMMU IR is enabled, guest OS will fill in the real IRQ data into IRTE
entries of IOMMU IR root table, while in IOAPIC only the index
information is maintained (with several legacy bits which might not be
covered by VT-d IR). If so, we need to talk to IOMMU to get the real IRQ
information to deliver.

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

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/i386/intel_iommu.c         | 127 ++++++++++++++++++++++++++++++++++++++++++
 hw/intc/ioapic.c              |  36 +++++++++---
 include/hw/i386/intel_iommu.h |  17 ++++++
 3 files changed, 173 insertions(+), 7 deletions(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index a44289f..1dcdc7b 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -2014,6 +2014,133 @@ IntelIOMMUState *vtd_iommu_get(void)
     return (IntelIOMMUState *)intel_iommu;
 }
 
+/* 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;
+
+    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;
+}
+
+/* Interrupt remapping for IOAPIC IRQ entry */
+int vtd_interrupt_remap_ioapic(IntelIOMMUState *iommu,
+                               uint64_t *ioapic_entry, VTDIrq *irq)
+{
+    int ret = 0;
+    uint16_t index = 0;
+    VTD_IR_IOAPICEntry *entry = (VTD_IR_IOAPICEntry *)ioapic_entry;
+
+    assert(iommu && entry && irq);
+    assert(iommu->intr_enabled);
+
+    /* TODO: Currently we still do not support compatible mode */
+    if (entry->int_mode != VTD_IR_INT_FORMAT_REMAP) {
+        VTD_DPRINTF(GENERAL, "error: trying to remap IOAPIC entry"
+                    " with compatible format: 0x%"PRIx64,
+                    le64_to_cpu(entry->data));
+        return -VTD_FR_IR_REQ_COMPAT;
+    }
+
+    if (entry->__zeros || entry->__reserved) {
+        VTD_DPRINTF(GENERAL, "error: reserved not empty for IOAPIC"
+                    "entry 0x%"PRIx64, le64_to_cpu(entry->data));
+        return -VTD_FR_IR_REQ_RSVD;
+    }
+
+    index = entry->index_h << 15 | entry->index_l;
+    ret = vtd_remap_irq_get(iommu, index, irq);
+    if (ret) {
+        return ret;
+    }
+
+    /* Trigger mode should be aligned between IOAPIC entry and IRTE
+     * entry */
+    if (irq->trigger_mode != entry->trigger_mode) {
+        /* This is possibly guest OS bug?! */
+        VTD_DPRINTF(GENERAL, "error: IOAPIC trigger mode inconsistent: "
+                    "0x%"PRIx64 " with IR table index %d",
+                    le64_to_cpu(entry->data), index);
+        /* Currently no such error defined */
+        return -VTD_FR_RESERVED_ERR;
+    }
+
+    /* Vector should be aligned too */
+    if (irq->vector != entry->vector) {
+        /*
+         * Latest linux kernel will not provide consistent
+         * vectors. Need some more digging to know why. Whatever,
+         * the one in IRTE is always correct. So directly use it.
+         */
+        VTD_DPRINTF(IR, "warn: IOAPIC vector inconsistent: "
+                    "index %d: entry=%d, IRTE=%d", index,
+                    entry->vector, irq->vector);
+    }
+
+    return 0;
+}
+
 /* Do the initialization. It will also be called when reset, so pay
  * attention when adding new initialization stuff.
  */
diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c
index 378e663..d963d45 100644
--- a/hw/intc/ioapic.c
+++ b/hw/intc/ioapic.c
@@ -57,6 +57,8 @@ static void ioapic_service(IOAPICCommonState *s)
     uint64_t entry;
     uint8_t dest;
     uint8_t dest_mode;
+    IntelIOMMUState *iommu = s->iommu;
+    VTDIrq irq = {0};
 
     for (i = 0; i < IOAPIC_NUM_PINS; i++) {
         mask = 1 << i;
@@ -65,11 +67,33 @@ 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;
-                dest_mode = (entry >> IOAPIC_LVT_DEST_MODE_SHIFT) & 1;
-                delivery_mode =
-                    (entry >> IOAPIC_LVT_DELIV_MODE_SHIFT) & IOAPIC_DM_MASK;
+
+                if (iommu && iommu->intr_enabled) {
+                    /*
+                    * Interrupt remapping is enabled in owner IOMMU,
+                    * we need to fetch the real IRQ information via
+                    * IRTE of the root mapping table
+                    */
+                    if (vtd_interrupt_remap_ioapic(iommu, &entry, &irq)) {
+                        DPRINTF("%s: IOAPIC remap fail on index %d "
+                                "entry 0x%lx, drop it for now\n",
+                                __func__, index, entry);
+                        return;
+                    }
+                    trig_mode = irq.trigger_mode;
+                    dest = irq.dest;
+                    dest_mode = irq.dest_mode;
+                    delivery_mode = irq.delivery_mode;
+                    vector = irq.vector;
+                } else {
+                    /* This is generic IOAPIC entry */
+                    trig_mode = ((entry >> IOAPIC_LVT_TRIGGER_MODE_SHIFT) & 1);
+                    dest = entry >> IOAPIC_LVT_DEST_SHIFT;
+                    dest_mode = (entry >> IOAPIC_LVT_DEST_MODE_SHIFT) & 1;
+                    delivery_mode =
+                        (entry >> IOAPIC_LVT_DELIV_MODE_SHIFT) & IOAPIC_DM_MASK;
+                    vector = entry & IOAPIC_VECTOR_MASK;
+                }
                 if (trig_mode == IOAPIC_TRIGGER_EDGE) {
                     s->irr &= ~mask;
                 } else {
@@ -78,8 +102,6 @@ static void ioapic_service(IOAPICCommonState *s)
                 }
                 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()) {
diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
index 9ee84f7..6a79207 100644
--- a/include/hw/i386/intel_iommu.h
+++ b/include/hw/i386/intel_iommu.h
@@ -23,6 +23,7 @@
 #define INTEL_IOMMU_H
 #include "hw/qdev.h"
 #include "sysemu/dma.h"
+#include "hw/i386/ioapic.h"
 
 #define TYPE_INTEL_IOMMU_DEVICE "intel-iommu"
 #define INTEL_IOMMU_DEVICE(obj) \
@@ -55,6 +56,7 @@ 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;
 
 /* Context-Entry */
 struct VTDContextEntry {
@@ -116,6 +118,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 +152,15 @@ union VTD_IR_MSIAddress {
     uint32_t data;
 };
 
+/* Generic IRQ entry information */
+struct VTDIrq {
+    uint8_t trigger_mode;
+    uint8_t vector;
+    uint8_t delivery_mode;
+    uint32_t dest;
+    uint8_t dest_mode;
+};
+
 /* When IR is enabled, all MSI/MSI-X data bits should be zero */
 #define VTD_IR_MSI_DATA          (0)
 
@@ -198,5 +212,8 @@ struct IntelIOMMUState {
 VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn);
 /* Get default IOMMU object */
 IntelIOMMUState *vtd_iommu_get(void);
+/* Interrupt remapping for IOAPIC IRQ entry */
+int vtd_interrupt_remap_ioapic(IntelIOMMUState *iommu,
+                               uint64_t *ioapic_entry, VTDIrq *irq);
 
 #endif
-- 
2.4.3

Re: [Qemu-devel] [PATCH v2 12/13] intel_iommu: ioapic: IR support for emulated IOAPIC

[Jan Kiszka]

On 2016-04-11 02:19, Peter Xu wrote:
> diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c
> index 378e663..d963d45 100644
> --- a/hw/intc/ioapic.c
> +++ b/hw/intc/ioapic.c
> @@ -57,6 +57,8 @@ static void ioapic_service(IOAPICCommonState *s)
>      uint64_t entry;
>      uint8_t dest;
>      uint8_t dest_mode;
> +    IntelIOMMUState *iommu = s->iommu;
> +    VTDIrq irq = {0};
>  
>      for (i = 0; i < IOAPIC_NUM_PINS; i++) {
>          mask = 1 << i;
> @@ -65,11 +67,33 @@ 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;
> -                dest_mode = (entry >> IOAPIC_LVT_DEST_MODE_SHIFT) & 1;
> -                delivery_mode =
> -                    (entry >> IOAPIC_LVT_DELIV_MODE_SHIFT) & IOAPIC_DM_MASK;
> +
> +                if (iommu && iommu->intr_enabled) {
> +                    /*
> +                    * Interrupt remapping is enabled in owner IOMMU,
> +                    * we need to fetch the real IRQ information via
> +                    * IRTE of the root mapping table
> +                    */
> +                    if (vtd_interrupt_remap_ioapic(iommu, &entry, &irq)) {

VT-d is only one of the possible IOMMUs on x86. Please introduce a
generic interface.

Look at Rita's and my patches: they translate the IOAPIC (and HPET...)
interrupts into MSI messages that are then - in a generic way -
intercepted by the respective IOMMU or directly dispatched to the APICs.
We may no longer need new memory regions for this, thanks to the region
attributes, but we also need no hard-coded hooks here.

Thanks,
Jan

Re: [Qemu-devel] [PATCH v2 12/13] intel_iommu: ioapic: IR support for emulated IOAPIC

[Peter Xu]

On Mon, Apr 11, 2016 at 10:22:18PM -0700, Jan Kiszka wrote:
> On 2016-04-11 02:19, Peter Xu wrote:
> > diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c
> > index 378e663..d963d45 100644
> > --- a/hw/intc/ioapic.c
> > +++ b/hw/intc/ioapic.c
> > @@ -57,6 +57,8 @@ static void ioapic_service(IOAPICCommonState *s)
> >      uint64_t entry;
> >      uint8_t dest;
> >      uint8_t dest_mode;
> > +    IntelIOMMUState *iommu = s->iommu;
> > +    VTDIrq irq = {0};
> >  
> >      for (i = 0; i < IOAPIC_NUM_PINS; i++) {
> >          mask = 1 << i;
> > @@ -65,11 +67,33 @@ 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;
> > -                dest_mode = (entry >> IOAPIC_LVT_DEST_MODE_SHIFT) & 1;
> > -                delivery_mode =
> > -                    (entry >> IOAPIC_LVT_DELIV_MODE_SHIFT) & IOAPIC_DM_MASK;
> > +
> > +                if (iommu && iommu->intr_enabled) {
> > +                    /*
> > +                    * Interrupt remapping is enabled in owner IOMMU,
> > +                    * we need to fetch the real IRQ information via
> > +                    * IRTE of the root mapping table
> > +                    */
> > +                    if (vtd_interrupt_remap_ioapic(iommu, &entry, &irq)) {
> 
> VT-d is only one of the possible IOMMUs on x86. Please introduce a
> generic interface.
> 
> Look at Rita's and my patches: they translate the IOAPIC (and HPET...)
> interrupts into MSI messages that are then - in a generic way -
> intercepted by the respective IOMMU or directly dispatched to the APICs.
> We may no longer need new memory regions for this, thanks to the region
> attributes, but we also need no hard-coded hooks here.

Yes, I should consider other x86 platforms like AMD. Thanks to point
out. It seems that there are many places in the patchset that lacks
thorough consideration about this. Will try to fix them in next
version.

Regarding to the above MSI solution: I'd say it is a good way to
hide everything else behind.  However, since we introduced one extra
layer (MSI) which actually does not exist, not sure there would be
problem too.  Also, I feel it a little bit hacky if we "create" one
MSI out of the air...  For example, if someone tries to capture MSIs
from QEMU inside in the APIC memory writes, he will see something he
cannot explain if he never knows this hack's there.  Considering the
above, I would prefer hooks, or better to provide a callback (a
function pointer that others like AMD can override) to do the
translation.  How do you think?

Thanks.

-- peterx

Re: [Qemu-devel] [PATCH v2 12/13] intel_iommu: ioapic: IR support for emulated IOAPIC

[Jan Kiszka]

On 2016-04-12 02:02, Peter Xu wrote:
> On Mon, Apr 11, 2016 at 10:22:18PM -0700, Jan Kiszka wrote:
>> On 2016-04-11 02:19, Peter Xu wrote:
>>> diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c
>>> index 378e663..d963d45 100644
>>> --- a/hw/intc/ioapic.c
>>> +++ b/hw/intc/ioapic.c
>>> @@ -57,6 +57,8 @@ static void ioapic_service(IOAPICCommonState *s)
>>>      uint64_t entry;
>>>      uint8_t dest;
>>>      uint8_t dest_mode;
>>> +    IntelIOMMUState *iommu = s->iommu;
>>> +    VTDIrq irq = {0};
>>>  
>>>      for (i = 0; i < IOAPIC_NUM_PINS; i++) {
>>>          mask = 1 << i;
>>> @@ -65,11 +67,33 @@ 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;
>>> -                dest_mode = (entry >> IOAPIC_LVT_DEST_MODE_SHIFT) & 1;
>>> -                delivery_mode =
>>> -                    (entry >> IOAPIC_LVT_DELIV_MODE_SHIFT) & IOAPIC_DM_MASK;
>>> +
>>> +                if (iommu && iommu->intr_enabled) {
>>> +                    /*
>>> +                    * Interrupt remapping is enabled in owner IOMMU,
>>> +                    * we need to fetch the real IRQ information via
>>> +                    * IRTE of the root mapping table
>>> +                    */
>>> +                    if (vtd_interrupt_remap_ioapic(iommu, &entry, &irq)) {
>>
>> VT-d is only one of the possible IOMMUs on x86. Please introduce a
>> generic interface.
>>
>> Look at Rita's and my patches: they translate the IOAPIC (and HPET...)
>> interrupts into MSI messages that are then - in a generic way -
>> intercepted by the respective IOMMU or directly dispatched to the APICs.
>> We may no longer need new memory regions for this, thanks to the region
>> attributes, but we also need no hard-coded hooks here.
> 
> Yes, I should consider other x86 platforms like AMD. Thanks to point
> out. It seems that there are many places in the patchset that lacks
> thorough consideration about this. Will try to fix them in next
> version.
> 
> Regarding to the above MSI solution: I'd say it is a good way to
> hide everything else behind.  However, since we introduced one extra
> layer (MSI) which actually does not exist, not sure there would be
> problem too.  Also, I feel it a little bit hacky if we "create" one
> MSI out of the air...  For example, if someone tries to capture MSIs
> from QEMU inside in the APIC memory writes, he will see something he
> cannot explain if he never knows this hack's there.  Considering the
> above, I would prefer hooks, or better to provide a callback (a
> function pointer that others like AMD can override) to do the
> translation.  How do you think?

The HPET does send MSIs, and I'm not sure how much different the
IOAPIC's message actually is. In any case, modelling it as MSI is
neither adding incorrectness nor making the code more complex (in fact,
the contrary is true!). Last but not least, it would be trivial to
filter out non-PCI MSI sources if we wanted to trace only PCI - because
we need to identify the origin anyway for remapping purposes. So,
explicit hooking looks like the wrong way to me.

Jan

Re: [Qemu-devel] [PATCH v2 12/13] intel_iommu: ioapic: IR support for emulated IOAPIC

[Peter Xu]

On Tue, Apr 12, 2016 at 08:39:02AM -0700, Jan Kiszka wrote:
> On 2016-04-12 02:02, Peter Xu wrote:

[...]

> > Yes, I should consider other x86 platforms like AMD. Thanks to point
> > out. It seems that there are many places in the patchset that lacks
> > thorough consideration about this. Will try to fix them in next
> > version.
> > 
> > Regarding to the above MSI solution: I'd say it is a good way to
> > hide everything else behind.  However, since we introduced one extra
> > layer (MSI) which actually does not exist, not sure there would be
> > problem too.  Also, I feel it a little bit hacky if we "create" one
> > MSI out of the air...  For example, if someone tries to capture MSIs
> > from QEMU inside in the APIC memory writes, he will see something he
> > cannot explain if he never knows this hack's there.  Considering the
> > above, I would prefer hooks, or better to provide a callback (a
> > function pointer that others like AMD can override) to do the
> > translation.  How do you think?
> 
> The HPET does send MSIs, and I'm not sure how much different the
> IOAPIC's message actually is. In any case, modelling it as MSI is
> neither adding incorrectness nor making the code more complex (in fact,
> the contrary is true!). Last but not least, it would be trivial to
> filter out non-PCI MSI sources if we wanted to trace only PCI - because
> we need to identify the origin anyway for remapping purposes. So,
> explicit hooking looks like the wrong way to me.

I am just not sure about the difference between IOAPIC's messages
and MSI ones. For now, they seems very alike. However, I am not sure
whether it would be not alike in the future. E.g., if one day, we
extend APIC bus to support more than 255 CPUs (could it? I do not
know for sure), here if we are with this "MSI layer", we would not
be able to do that, since MSI only support 8 bits for destination ID
field. That's my only worry now. If you (or Radim? or anyone more
experienced on this than me) can confirm that this would never be a
problem, I'd be glad to take the MSI way.

Thanks.

-- peterx

Re: [Qemu-devel] [PATCH v2 12/13] intel_iommu: ioapic: IR support for emulated IOAPIC

[Jan Kiszka]

On 2016-04-12 20:33, Peter Xu wrote:
> On Tue, Apr 12, 2016 at 08:39:02AM -0700, Jan Kiszka wrote:
>> On 2016-04-12 02:02, Peter Xu wrote:
> 
> [...]
> 
>>> Yes, I should consider other x86 platforms like AMD. Thanks to point
>>> out. It seems that there are many places in the patchset that lacks
>>> thorough consideration about this. Will try to fix them in next
>>> version.
>>>
>>> Regarding to the above MSI solution: I'd say it is a good way to
>>> hide everything else behind.  However, since we introduced one extra
>>> layer (MSI) which actually does not exist, not sure there would be
>>> problem too.  Also, I feel it a little bit hacky if we "create" one
>>> MSI out of the air...  For example, if someone tries to capture MSIs
>>> from QEMU inside in the APIC memory writes, he will see something he
>>> cannot explain if he never knows this hack's there.  Considering the
>>> above, I would prefer hooks, or better to provide a callback (a
>>> function pointer that others like AMD can override) to do the
>>> translation.  How do you think?
>>
>> The HPET does send MSIs, and I'm not sure how much different the
>> IOAPIC's message actually is. In any case, modelling it as MSI is
>> neither adding incorrectness nor making the code more complex (in fact,
>> the contrary is true!). Last but not least, it would be trivial to
>> filter out non-PCI MSI sources if we wanted to trace only PCI - because
>> we need to identify the origin anyway for remapping purposes. So,
>> explicit hooking looks like the wrong way to me.
> 
> I am just not sure about the difference between IOAPIC's messages
> and MSI ones. For now, they seems very alike. However, I am not sure
> whether it would be not alike in the future. E.g., if one day, we
> extend APIC bus to support more than 255 CPUs (could it? I do not
> know for sure), here if we are with this "MSI layer", we would not
> be able to do that, since MSI only support 8 bits for destination ID
> field. That's my only worry now. If you (or Radim? or anyone more
> experienced on this than me) can confirm that this would never be a
> problem, I'd be glad to take the MSI way.

That's one of the reason why we need IR: >255 is only possible this way,
because it requires x2APIC and that requires IR (see Intel spec). So,
IOAPIC messages will then always travel via VT-d. No need to worry at all.

Jan

Re: [Qemu-devel] [PATCH v2 12/13] intel_iommu: ioapic: IR support for emulated IOAPIC

[Peter Xu]

On Tue, Apr 12, 2016 at 08:39:21PM -0700, Jan Kiszka wrote:
> On 2016-04-12 20:33, Peter Xu wrote:
> > On Tue, Apr 12, 2016 at 08:39:02AM -0700, Jan Kiszka wrote:
> >> On 2016-04-12 02:02, Peter Xu wrote:
> > 
> > [...]
> > 
> >>> Yes, I should consider other x86 platforms like AMD. Thanks to point
> >>> out. It seems that there are many places in the patchset that lacks
> >>> thorough consideration about this. Will try to fix them in next
> >>> version.
> >>>
> >>> Regarding to the above MSI solution: I'd say it is a good way to
> >>> hide everything else behind.  However, since we introduced one extra
> >>> layer (MSI) which actually does not exist, not sure there would be
> >>> problem too.  Also, I feel it a little bit hacky if we "create" one
> >>> MSI out of the air...  For example, if someone tries to capture MSIs
> >>> from QEMU inside in the APIC memory writes, he will see something he
> >>> cannot explain if he never knows this hack's there.  Considering the
> >>> above, I would prefer hooks, or better to provide a callback (a
> >>> function pointer that others like AMD can override) to do the
> >>> translation.  How do you think?
> >>
> >> The HPET does send MSIs, and I'm not sure how much different the
> >> IOAPIC's message actually is. In any case, modelling it as MSI is
> >> neither adding incorrectness nor making the code more complex (in fact,
> >> the contrary is true!). Last but not least, it would be trivial to
> >> filter out non-PCI MSI sources if we wanted to trace only PCI - because
> >> we need to identify the origin anyway for remapping purposes. So,
> >> explicit hooking looks like the wrong way to me.
> > 
> > I am just not sure about the difference between IOAPIC's messages
> > and MSI ones. For now, they seems very alike. However, I am not sure
> > whether it would be not alike in the future. E.g., if one day, we
> > extend APIC bus to support more than 255 CPUs (could it? I do not
> > know for sure), here if we are with this "MSI layer", we would not
> > be able to do that, since MSI only support 8 bits for destination ID
> > field. That's my only worry now. If you (or Radim? or anyone more
> > experienced on this than me) can confirm that this would never be a
> > problem, I'd be glad to take the MSI way.
> 
> That's one of the reason why we need IR: >255 is only possible this way,
> because it requires x2APIC and that requires IR (see Intel spec). So,
> IOAPIC messages will then always travel via VT-d. No need to worry at all.

Ah, right. When we deliver the MSI, it's in remappable format, so
there is no destination ID at all... Okay, I can take this in
v3. Thanks.

-- peterx

Re: [Qemu-devel] [PATCH v2 12/13] intel_iommu: ioapic: IR support for emulated IOAPIC

[Peter Xu]

On Tue, Apr 12, 2016 at 08:39:21PM -0700, Jan Kiszka wrote:
> On 2016-04-12 20:33, Peter Xu wrote:
> > On Tue, Apr 12, 2016 at 08:39:02AM -0700, Jan Kiszka wrote:
> >> On 2016-04-12 02:02, Peter Xu wrote:
> > 
> > [...]
> > 
> >>> Yes, I should consider other x86 platforms like AMD. Thanks to point
> >>> out. It seems that there are many places in the patchset that lacks
> >>> thorough consideration about this. Will try to fix them in next
> >>> version.
> >>>
> >>> Regarding to the above MSI solution: I'd say it is a good way to
> >>> hide everything else behind.  However, since we introduced one extra
> >>> layer (MSI) which actually does not exist, not sure there would be
> >>> problem too.  Also, I feel it a little bit hacky if we "create" one
> >>> MSI out of the air...  For example, if someone tries to capture MSIs
> >>> from QEMU inside in the APIC memory writes, he will see something he
> >>> cannot explain if he never knows this hack's there.  Considering the
> >>> above, I would prefer hooks, or better to provide a callback (a
> >>> function pointer that others like AMD can override) to do the
> >>> translation.  How do you think?
> >>
> >> The HPET does send MSIs, and I'm not sure how much different the
> >> IOAPIC's message actually is. In any case, modelling it as MSI is
> >> neither adding incorrectness nor making the code more complex (in fact,
> >> the contrary is true!). Last but not least, it would be trivial to
> >> filter out non-PCI MSI sources if we wanted to trace only PCI - because
> >> we need to identify the origin anyway for remapping purposes. So,
> >> explicit hooking looks like the wrong way to me.
> > 
> > I am just not sure about the difference between IOAPIC's messages
> > and MSI ones. For now, they seems very alike. However, I am not sure
> > whether it would be not alike in the future. E.g., if one day, we
> > extend APIC bus to support more than 255 CPUs (could it? I do not
> > know for sure), here if we are with this "MSI layer", we would not
> > be able to do that, since MSI only support 8 bits for destination ID
> > field. That's my only worry now. If you (or Radim? or anyone more
> > experienced on this than me) can confirm that this would never be a
> > problem, I'd be glad to take the MSI way.
> 
> That's one of the reason why we need IR: >255 is only possible this way,
> because it requires x2APIC and that requires IR (see Intel spec). So,
> IOAPIC messages will then always travel via VT-d. No need to worry at all.

One thing I am curious about: I see that in vtd spec 5.1.5.1:

"RTE bits 10:8 is programmed to 000b (Fixed) to force the SHV
(SubHandle Valid) field as Clear in the interrupt address
generated."

So... In real IOMMU hardwares, IOAPIC interrupt will finally be
translated to MSI as well? am I understanding it correctly?

Btw, if to use the way you suggested, the patch content would
possibly be very alike the one you and Rita has posted, which is:

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

I will only pick up those lines I needed in supporting IOAPIC. If
so, do you mind I add your s-o-b as well above mine (maybe add
Rita's too)?

Thanks.

-- peterx

Re: [Qemu-devel] [PATCH v2 12/13] intel_iommu: ioapic: IR support for emulated IOAPIC

[Jan Kiszka]

On 2016-04-13 03:06, Peter Xu wrote:
> On Tue, Apr 12, 2016 at 08:39:21PM -0700, Jan Kiszka wrote:
>> On 2016-04-12 20:33, Peter Xu wrote:
>>> On Tue, Apr 12, 2016 at 08:39:02AM -0700, Jan Kiszka wrote:
>>>> On 2016-04-12 02:02, Peter Xu wrote:
>>>
>>> [...]
>>>
>>>>> Yes, I should consider other x86 platforms like AMD. Thanks to point
>>>>> out. It seems that there are many places in the patchset that lacks
>>>>> thorough consideration about this. Will try to fix them in next
>>>>> version.
>>>>>
>>>>> Regarding to the above MSI solution: I'd say it is a good way to
>>>>> hide everything else behind.  However, since we introduced one extra
>>>>> layer (MSI) which actually does not exist, not sure there would be
>>>>> problem too.  Also, I feel it a little bit hacky if we "create" one
>>>>> MSI out of the air...  For example, if someone tries to capture MSIs
>>>>> from QEMU inside in the APIC memory writes, he will see something he
>>>>> cannot explain if he never knows this hack's there.  Considering the
>>>>> above, I would prefer hooks, or better to provide a callback (a
>>>>> function pointer that others like AMD can override) to do the
>>>>> translation.  How do you think?
>>>>
>>>> The HPET does send MSIs, and I'm not sure how much different the
>>>> IOAPIC's message actually is. In any case, modelling it as MSI is
>>>> neither adding incorrectness nor making the code more complex (in fact,
>>>> the contrary is true!). Last but not least, it would be trivial to
>>>> filter out non-PCI MSI sources if we wanted to trace only PCI - because
>>>> we need to identify the origin anyway for remapping purposes. So,
>>>> explicit hooking looks like the wrong way to me.
>>>
>>> I am just not sure about the difference between IOAPIC's messages
>>> and MSI ones. For now, they seems very alike. However, I am not sure
>>> whether it would be not alike in the future. E.g., if one day, we
>>> extend APIC bus to support more than 255 CPUs (could it? I do not
>>> know for sure), here if we are with this "MSI layer", we would not
>>> be able to do that, since MSI only support 8 bits for destination ID
>>> field. That's my only worry now. If you (or Radim? or anyone more
>>> experienced on this than me) can confirm that this would never be a
>>> problem, I'd be glad to take the MSI way.
>>
>> That's one of the reason why we need IR: >255 is only possible this way,
>> because it requires x2APIC and that requires IR (see Intel spec). So,
>> IOAPIC messages will then always travel via VT-d. No need to worry at all.
> 
> One thing I am curious about: I see that in vtd spec 5.1.5.1:
> 
> "RTE bits 10:8 is programmed to 000b (Fixed) to force the SHV
> (SubHandle Valid) field as Clear in the interrupt address
> generated."
> 
> So... In real IOMMU hardwares, IOAPIC interrupt will finally be
> translated to MSI as well? am I understanding it correctly?

It will be translated into something that the IR unit can receive -
whatever that is technically. Logically, there is no difference to MSIs
received from PCI devices.

> 
> Btw, if to use the way you suggested, the patch content would
> possibly be very alike the one you and Rita has posted, which is:
> 
> https://lists.gnu.org/archive/html/qemu-devel/2016-03/msg01933.html
> 
> I will only pick up those lines I needed in supporting IOAPIC. If
> so, do you mind I add your s-o-b as well above mine (maybe add
> Rita's too)?

If a patch is almost identical, add your [Peter: my changes...] line and
your signed of to it. If it is more modified, claim authorship and just
refer to the original authors in the commit log.

Jan

Re: [Qemu-devel] [PATCH v2 12/13] intel_iommu: ioapic: IR support for emulated IOAPIC

[Peter Xu]

On Wed, Apr 13, 2016 at 07:44:54AM -0700, Jan Kiszka wrote:
[...]
> > One thing I am curious about: I see that in vtd spec 5.1.5.1:
> > 
> > "RTE bits 10:8 is programmed to 000b (Fixed) to force the SHV
> > (SubHandle Valid) field as Clear in the interrupt address
> > generated."
> > 
> > So... In real IOMMU hardwares, IOAPIC interrupt will finally be
> > translated to MSI as well? am I understanding it correctly?
> 
> It will be translated into something that the IR unit can receive -
> whatever that is technically. Logically, there is no difference to MSIs
> received from PCI devices.

Ok. I see there are still differences between IOAPIC and MSI, e.g.,
for IOAPIC entries, it has "Interrupt Input Pin Polarity", which is
bit 13 of the entry, to show whether 1 or 0 is taken as assertion
for level-triggered interrupts. While in MSI, I assume assertion
will be 1 always. Can I take it as "obsolete" and we will never use
it?

If to take IOAPIC entries as MSI messages, all these extra bits will
be dropped (it's dropped in ioapic_service() already, but not sure
whether we will pick them up in the future).

> 
> > 
> > Btw, if to use the way you suggested, the patch content would
> > possibly be very alike the one you and Rita has posted, which is:
> > 
> > https://lists.gnu.org/archive/html/qemu-devel/2016-03/msg01933.html
> > 
> > I will only pick up those lines I needed in supporting IOAPIC. If
> > so, do you mind I add your s-o-b as well above mine (maybe add
> > Rita's too)?
> 
> If a patch is almost identical, add your [Peter: my changes...] line and
> your signed of to it. If it is more modified, claim authorship and just
> refer to the original authors in the commit log.

Ok. Thanks.

-- peterx

Re: [Qemu-devel] [PATCH v2 12/13] intel_iommu: ioapic: IR support for emulated IOAPIC

[Jan Kiszka]

On 2016-04-13 19:46, Peter Xu wrote:
> On Wed, Apr 13, 2016 at 07:44:54AM -0700, Jan Kiszka wrote:
> [...]
>>> One thing I am curious about: I see that in vtd spec 5.1.5.1:
>>>
>>> "RTE bits 10:8 is programmed to 000b (Fixed) to force the SHV
>>> (SubHandle Valid) field as Clear in the interrupt address
>>> generated."
>>>
>>> So... In real IOMMU hardwares, IOAPIC interrupt will finally be
>>> translated to MSI as well? am I understanding it correctly?
>>
>> It will be translated into something that the IR unit can receive -
>> whatever that is technically. Logically, there is no difference to MSIs
>> received from PCI devices.
> 
> Ok. I see there are still differences between IOAPIC and MSI, e.g.,
> for IOAPIC entries, it has "Interrupt Input Pin Polarity", which is
> bit 13 of the entry, to show whether 1 or 0 is taken as assertion
> for level-triggered interrupts. While in MSI, I assume assertion
> will be 1 always. Can I take it as "obsolete" and we will never use
> it?

I can't check details right now, but I would recommend studying in the
specs if any of the *receiver* (APIC and IOMMU) can make sense of that
bit at all, and how. Also study (commit logs) if there is a reason why
the bit is unused.

> 
> If to take IOAPIC entries as MSI messages, all these extra bits will
> be dropped (it's dropped in ioapic_service() already, but not sure
> whether we will pick them up in the future).

What other bits?

Jan

Re: [Qemu-devel] [PATCH v2 12/13] intel_iommu: ioapic: IR support for emulated IOAPIC

[Peter Xu]

On Wed, Apr 13, 2016 at 10:42:26PM -0700, Jan Kiszka wrote:
> On 2016-04-13 19:46, Peter Xu wrote:
> > On Wed, Apr 13, 2016 at 07:44:54AM -0700, Jan Kiszka wrote:
> > [...]
> >>> One thing I am curious about: I see that in vtd spec 5.1.5.1:
> >>>
> >>> "RTE bits 10:8 is programmed to 000b (Fixed) to force the SHV
> >>> (SubHandle Valid) field as Clear in the interrupt address
> >>> generated."
> >>>
> >>> So... In real IOMMU hardwares, IOAPIC interrupt will finally be
> >>> translated to MSI as well? am I understanding it correctly?
> >>
> >> It will be translated into something that the IR unit can receive -
> >> whatever that is technically. Logically, there is no difference to MSIs
> >> received from PCI devices.
> > 
> > Ok. I see there are still differences between IOAPIC and MSI, e.g.,
> > for IOAPIC entries, it has "Interrupt Input Pin Polarity", which is
> > bit 13 of the entry, to show whether 1 or 0 is taken as assertion
> > for level-triggered interrupts. While in MSI, I assume assertion
> > will be 1 always. Can I take it as "obsolete" and we will never use
> > it?
> 
> I can't check details right now, but I would recommend studying in the
> specs if any of the *receiver* (APIC and IOMMU) can make sense of that
> bit at all, and how. Also study (commit logs) if there is a reason why
> the bit is unused.

Thanks for the advices. Will add this in my todo list. I am guessing
that, all devices emulated by QEMU are using 1 as assertions..

It's defined as IOAPIC_LVT_POLARITY in QEMU. As long as I am sure
that current QEMU does not use it, I think I can move on to v3 using
MSI path, at least to make sure I'm not making things worse, or
bringing any functionality loss.

> 
> > 
> > If to take IOAPIC entries as MSI messages, all these extra bits will
> > be dropped (it's dropped in ioapic_service() already, but not sure
> > whether we will pick them up in the future).
> 
> What other bits?

Besides polarity bit, there is another one IOAPIC_LVT_DELIV_STATUS,
which is not used too. But after more careful look, I see this bit
is not too relevant if we are talking about translating IOAPIC
entries into MSI.

Thanks.

-- peterx

[Qemu-devel] [PATCH v2 13/13] 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 when IR is enabled.

The translation process tried to make full use of the helper function
from IOAPIC patch. Several more bits are added to VTDIrq as MSI specific
fields.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/i386/intel_iommu.c          | 172 ++++++++++++++++++++++++++++++++++++++++-
 hw/i386/intel_iommu_internal.h |   2 +
 include/hw/i386/intel_iommu.h  |  34 ++++++++
 include/hw/pci/msi.h           |   4 +
 4 files changed, 211 insertions(+), 1 deletion(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 1dcdc7b..322b5ac 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -44,6 +44,10 @@ static int vtd_dbgflags = VTD_DBGBIT(GENERAL) | VTD_DBGBIT(CSR);
 #define VTD_DPRINTF(what, fmt, ...) do {} while (0)
 #endif
 
+static int vtd_interrupt_remap_msi(IntelIOMMUState *iommu,
+                                   MSIMessage *origin,
+                                   MSIMessage *translated);
+
 static void vtd_define_quad(IntelIOMMUState *s, hwaddr addr, uint64_t val,
                             uint64_t wmask, uint64_t w1cmask)
 {
@@ -1964,12 +1968,70 @@ static const MemoryRegionOps vtd_mem_ops = {
     },
 };
 
+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,
+    },
+};
+
 static Property vtd_properties[] = {
     DEFINE_PROP_UINT32("version", IntelIOMMUState, version, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-
 VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn)
 {
     uintptr_t key = (uintptr_t)bus;
@@ -1995,6 +2057,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");
     }
@@ -2075,6 +2142,7 @@ static int vtd_remap_irq_get(IntelIOMMUState *iommu, uint16_t index, VTDIrq *irq
     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,
@@ -2141,6 +2209,108 @@ int vtd_interrupt_remap_ioapic(IntelIOMMUState *iommu,
     return 0;
 }
 
+/* Generate one MSI message from VTDIrq info */
+static void vtd_generate_msi_message(VTDIrq *irq, MSIMessage *msg_out)
+{
+    VTD_MSIMessage msg;
+
+    bzero(&msg, sizeof(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;
+    /* FIXME: assert always for level mode interrupt */
+    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(iommu && origin && translated);
+
+    if (!iommu->intr_enabled) {
+        memcpy(translated, origin, sizeof(*origin));
+        return 0;
+    }
+
+    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;
+    }
+
+    if (origin->address & MSI_ADDR_HI_MASK) {
+        VTD_DPRINTF(GENERAL, "error: MSI addr high 32 bits nonzero"
+                    " during interrupt remapping: 0x%"PRIx32,
+                    (uint32_t)((origin->address & MSI_ADDR_HI_MASK) >> \
+                    MSI_ADDR_HI_SHIFT));
+        return -VTD_FR_IR_REQ_RSVD;
+    }
+
+    addr.data = origin->address & 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;
+    }
+
+    if (addr.sub_valid != 1) {
+        VTD_DPRINTF(GENERAL, "error: SHV not set for MSI addr: "
+                    "0x%"PRIx32, addr.data);
+        return -VTD_FR_IR_REQ_RSVD;
+    }
+
+    /* TODO: Currently we still do not support compatible mode */
+    if (addr.int_mode != VTD_IR_INT_FORMAT_REMAP) {
+        VTD_DPRINTF(GENERAL, "error: trying to remap MSI entry"
+                    " with compatible format: 0x%"PRIx32,
+                    addr.data);
+        return -VTD_FR_IR_REQ_COMPAT;
+    }
+
+    index = addr.index_h << 15 | addr.index_l;
+
+    ret = vtd_remap_irq_get(iommu, index, &irq);
+    if (ret) {
+        return ret;
+    }
+
+    /*
+     * 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 the initialization. It will also be called when reset, so pay
  * attention when adding new initialization stuff.
  */
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 6a79207..9297eba 100644
--- a/include/hw/i386/intel_iommu.h
+++ b/include/hw/i386/intel_iommu.h
@@ -24,6 +24,7 @@
 #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) \
@@ -57,6 +58,7 @@ 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 {
@@ -77,6 +79,7 @@ struct VTDAddressSpace {
     uint8_t devfn;
     AddressSpace as;
     MemoryRegion iommu;
+    MemoryRegion iommu_ir;      /* Interrupt region: 0xfeeXXXXX */
     IntelIOMMUState *iommu_state;
     VTDContextCacheEntry context_cache_entry;
 };
@@ -154,11 +157,42 @@ union VTD_IR_MSIAddress {
 
 /* 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 */
diff --git a/include/hw/pci/msi.h b/include/hw/pci/msi.h
index 8124908..8e94fca 100644
--- a/include/hw/pci/msi.h
+++ b/include/hw/pci/msi.h
@@ -24,6 +24,10 @@
 #include "qemu-common.h"
 #include "hw/pci/pci.h"
 
+#define  MSI_ADDR_HI_MASK        (0xffffffff00000000ULL)
+#define  MSI_ADDR_HI_SHIFT       (32)
+#define  MSI_ADDR_LO_MASK        (0x00000000ffffffffULL)
+
 struct MSIMessage {
     uint64_t address;
     uint32_t data;
-- 
2.4.3

Re: [Qemu-devel] [PATCH v2 13/13] intel_iommu: Add support for PCI MSI remap

[Michael S. Tsirkin]

On Mon, Apr 11, 2016 at 05:19:23PM +0800, 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 when IR is enabled.
> 
> The translation process tried to make full use of the helper function
> from IOAPIC patch. Several more bits are added to VTDIrq as MSI specific
> fields.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>

So far so good, but I wonder how we are going to
support irqfd with this approach.
It would be nicer to have a single module that
somehow handles both ioapic and non ioapic.

> ---
>  hw/i386/intel_iommu.c          | 172 ++++++++++++++++++++++++++++++++++++++++-
>  hw/i386/intel_iommu_internal.h |   2 +
>  include/hw/i386/intel_iommu.h  |  34 ++++++++
>  include/hw/pci/msi.h           |   4 +
>  4 files changed, 211 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 1dcdc7b..322b5ac 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -44,6 +44,10 @@ static int vtd_dbgflags = VTD_DBGBIT(GENERAL) | VTD_DBGBIT(CSR);
>  #define VTD_DPRINTF(what, fmt, ...) do {} while (0)
>  #endif
>  
> +static int vtd_interrupt_remap_msi(IntelIOMMUState *iommu,
> +                                   MSIMessage *origin,
> +                                   MSIMessage *translated);
> +
>  static void vtd_define_quad(IntelIOMMUState *s, hwaddr addr, uint64_t val,
>                              uint64_t wmask, uint64_t w1cmask)
>  {

don't forward-declare static functions, just arrange them sensibly.

> @@ -1964,12 +1968,70 @@ static const MemoryRegionOps vtd_mem_ops = {
>      },
>  };
>  
> +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,
> +    },
> +};
> +
>  static Property vtd_properties[] = {
>      DEFINE_PROP_UINT32("version", IntelIOMMUState, version, 0),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> -
>  VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn)
>  {
>      uintptr_t key = (uintptr_t)bus;
> @@ -1995,6 +2057,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");
>      }
> @@ -2075,6 +2142,7 @@ static int vtd_remap_irq_get(IntelIOMMUState *iommu, uint16_t index, VTDIrq *irq
>      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,
> @@ -2141,6 +2209,108 @@ int vtd_interrupt_remap_ioapic(IntelIOMMUState *iommu,
>      return 0;
>  }
>  
> +/* Generate one MSI message from VTDIrq info */
> +static void vtd_generate_msi_message(VTDIrq *irq, MSIMessage *msg_out)
> +{
> +    VTD_MSIMessage msg;
> +
> +    bzero(&msg, sizeof(msg));
> +

Just msg = {} will do it.

> +    /* 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;
> +    /* FIXME: assert always for level mode interrupt */

what does this mean?

> +    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(iommu && origin && translated);
> +
> +    if (!iommu->intr_enabled) {
> +        memcpy(translated, origin, sizeof(*origin));
> +        return 0;
> +    }
> +
> +    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;
> +    }
> +
> +    if (origin->address & MSI_ADDR_HI_MASK) {
> +        VTD_DPRINTF(GENERAL, "error: MSI addr high 32 bits nonzero"
> +                    " during interrupt remapping: 0x%"PRIx32,
> +                    (uint32_t)((origin->address & MSI_ADDR_HI_MASK) >> \
> +                    MSI_ADDR_HI_SHIFT));
> +        return -VTD_FR_IR_REQ_RSVD;
> +    }
> +
> +    addr.data = origin->address & 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;
> +    }
> +
> +    if (addr.sub_valid != 1) {
> +        VTD_DPRINTF(GENERAL, "error: SHV not set for MSI addr: "
> +                    "0x%"PRIx32, addr.data);
> +        return -VTD_FR_IR_REQ_RSVD;
> +    }
> +
> +    /* TODO: Currently we still do not support compatible mode */
> +    if (addr.int_mode != VTD_IR_INT_FORMAT_REMAP) {
> +        VTD_DPRINTF(GENERAL, "error: trying to remap MSI entry"
> +                    " with compatible format: 0x%"PRIx32,
> +                    addr.data);
> +        return -VTD_FR_IR_REQ_COMPAT;
> +    }
> +
> +    index = addr.index_h << 15 | addr.index_l;
> +
> +    ret = vtd_remap_irq_get(iommu, index, &irq);
> +    if (ret) {
> +        return ret;
> +    }
> +
> +    /*
> +     * 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 the initialization. It will also be called when reset, so pay
>   * attention when adding new initialization stuff.
>   */
> 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 6a79207..9297eba 100644
> --- a/include/hw/i386/intel_iommu.h
> +++ b/include/hw/i386/intel_iommu.h
> @@ -24,6 +24,7 @@
>  #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) \
> @@ -57,6 +58,7 @@ 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 {
> @@ -77,6 +79,7 @@ struct VTDAddressSpace {
>      uint8_t devfn;
>      AddressSpace as;
>      MemoryRegion iommu;
> +    MemoryRegion iommu_ir;      /* Interrupt region: 0xfeeXXXXX */
>      IntelIOMMUState *iommu_state;
>      VTDContextCacheEntry context_cache_entry;
>  };
> @@ -154,11 +157,42 @@ union VTD_IR_MSIAddress {
>  
>  /* 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 */
> diff --git a/include/hw/pci/msi.h b/include/hw/pci/msi.h
> index 8124908..8e94fca 100644
> --- a/include/hw/pci/msi.h
> +++ b/include/hw/pci/msi.h
> @@ -24,6 +24,10 @@
>  #include "qemu-common.h"
>  #include "hw/pci/pci.h"
>  
> +#define  MSI_ADDR_HI_MASK        (0xffffffff00000000ULL)
> +#define  MSI_ADDR_HI_SHIFT       (32)
> +#define  MSI_ADDR_LO_MASK        (0x00000000ffffffffULL)
> +

I don't think these are needed in msi.h, rename them with
vtd prefix and put them where they are used.

>  struct MSIMessage {
>      uint64_t address;
>      uint32_t data;
> -- 
> 2.4.3

Re: [Qemu-devel] [PATCH v2 13/13] intel_iommu: Add support for PCI MSI remap

[Peter Xu]

Hi, Michael,

On Mon, Apr 11, 2016 at 03:41:29PM +0300, Michael S. Tsirkin wrote:
> On Mon, Apr 11, 2016 at 05:19:23PM +0800, 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 when IR is enabled.
> > 
> > The translation process tried to make full use of the helper function
> > from IOAPIC patch. Several more bits are added to VTDIrq as MSI specific
> > fields.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> 
> So far so good, but I wonder how we are going to
> support irqfd with this approach.

I suppose the support for irqfd can be considered together with
kvm-ioapic support (though if to implement it, I will start from
kvm-ioapic). I haven't thought about it in depth, but currently I
have two plans for it:

- translate interrupt in kernel: firstly, we may need to provide a
  new x86-specific ioctl to tell the kernel the location of the IR
  table. After that, when we got irqfd signal, we translate before
  putting it onto APIC bus. The problem is that (at least), we may
  need to implement the logic twice (both in QEMU and KVM, though
  the logic to parse IR is not very complicated), and everything
  will be doubled in the future (e.g., future IR caches, etc).

- translate interrupt in user space: this will be something like
  Jason's DMAR patchset. When kernel receive IR request (either
  kvm-ioapic, or irqfd), it can ask QEMU about it. Here, we'd better
  introduce IR cache in kernel, and we may further need to enable IR
  cache invalidation in QEMU side, so that when the IR cache is
  polluted, QEMU can let KVM know it.

> It would be nicer to have a single module that
> somehow handles both ioapic and non ioapic.

I will take Jan's advice to handle IOAPIC leveraging MSI
path. That will make things cleaner.

> 
> > ---
> >  hw/i386/intel_iommu.c          | 172 ++++++++++++++++++++++++++++++++++++++++-
> >  hw/i386/intel_iommu_internal.h |   2 +
> >  include/hw/i386/intel_iommu.h  |  34 ++++++++
> >  include/hw/pci/msi.h           |   4 +
> >  4 files changed, 211 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > index 1dcdc7b..322b5ac 100644
> > --- a/hw/i386/intel_iommu.c
> > +++ b/hw/i386/intel_iommu.c
> > @@ -44,6 +44,10 @@ static int vtd_dbgflags = VTD_DBGBIT(GENERAL) | VTD_DBGBIT(CSR);
> >  #define VTD_DPRINTF(what, fmt, ...) do {} while (0)
> >  #endif
> >  
> > +static int vtd_interrupt_remap_msi(IntelIOMMUState *iommu,
> > +                                   MSIMessage *origin,
> > +                                   MSIMessage *translated);
> > +
> >  static void vtd_define_quad(IntelIOMMUState *s, hwaddr addr, uint64_t val,
> >                              uint64_t wmask, uint64_t w1cmask)
> >  {
> 
> don't forward-declare static functions, just arrange them sensibly.

Will do.

> 
> > @@ -1964,12 +1968,70 @@ static const MemoryRegionOps vtd_mem_ops = {
> >      },
> >  };
> >  
> > +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,
> > +    },
> > +};
> > +
> >  static Property vtd_properties[] = {
> >      DEFINE_PROP_UINT32("version", IntelIOMMUState, version, 0),
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> >  
> > -
> >  VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn)
> >  {
> >      uintptr_t key = (uintptr_t)bus;
> > @@ -1995,6 +2057,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");
> >      }
> > @@ -2075,6 +2142,7 @@ static int vtd_remap_irq_get(IntelIOMMUState *iommu, uint16_t index, VTDIrq *irq
> >      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,
> > @@ -2141,6 +2209,108 @@ int vtd_interrupt_remap_ioapic(IntelIOMMUState *iommu,
> >      return 0;
> >  }
> >  
> > +/* Generate one MSI message from VTDIrq info */
> > +static void vtd_generate_msi_message(VTDIrq *irq, MSIMessage *msg_out)
> > +{
> > +    VTD_MSIMessage msg;
> > +
> > +    bzero(&msg, sizeof(msg));
> > +
> 
> Just msg = {} will do it.

Ok.

> 
> > +    /* 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;
> > +    /* FIXME: assert always for level mode interrupt */
> 
> what does this mean?

I will remove above line.

Just to confirm: for level-triggered interrupt, we will always set
msg.level to 1, right?

> 
> > +    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(iommu && origin && translated);
> > +
> > +    if (!iommu->intr_enabled) {
> > +        memcpy(translated, origin, sizeof(*origin));
> > +        return 0;
> > +    }
> > +
> > +    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;
> > +    }
> > +
> > +    if (origin->address & MSI_ADDR_HI_MASK) {
> > +        VTD_DPRINTF(GENERAL, "error: MSI addr high 32 bits nonzero"
> > +                    " during interrupt remapping: 0x%"PRIx32,
> > +                    (uint32_t)((origin->address & MSI_ADDR_HI_MASK) >> \
> > +                    MSI_ADDR_HI_SHIFT));
> > +        return -VTD_FR_IR_REQ_RSVD;
> > +    }
> > +
> > +    addr.data = origin->address & 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;
> > +    }
> > +
> > +    if (addr.sub_valid != 1) {
> > +        VTD_DPRINTF(GENERAL, "error: SHV not set for MSI addr: "
> > +                    "0x%"PRIx32, addr.data);
> > +        return -VTD_FR_IR_REQ_RSVD;
> > +    }
> > +
> > +    /* TODO: Currently we still do not support compatible mode */
> > +    if (addr.int_mode != VTD_IR_INT_FORMAT_REMAP) {
> > +        VTD_DPRINTF(GENERAL, "error: trying to remap MSI entry"
> > +                    " with compatible format: 0x%"PRIx32,
> > +                    addr.data);
> > +        return -VTD_FR_IR_REQ_COMPAT;
> > +    }
> > +
> > +    index = addr.index_h << 15 | addr.index_l;
> > +
> > +    ret = vtd_remap_irq_get(iommu, index, &irq);
> > +    if (ret) {
> > +        return ret;
> > +    }
> > +
> > +    /*
> > +     * 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 the initialization. It will also be called when reset, so pay
> >   * attention when adding new initialization stuff.
> >   */
> > 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 6a79207..9297eba 100644
> > --- a/include/hw/i386/intel_iommu.h
> > +++ b/include/hw/i386/intel_iommu.h
> > @@ -24,6 +24,7 @@
> >  #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) \
> > @@ -57,6 +58,7 @@ 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 {
> > @@ -77,6 +79,7 @@ struct VTDAddressSpace {
> >      uint8_t devfn;
> >      AddressSpace as;
> >      MemoryRegion iommu;
> > +    MemoryRegion iommu_ir;      /* Interrupt region: 0xfeeXXXXX */
> >      IntelIOMMUState *iommu_state;
> >      VTDContextCacheEntry context_cache_entry;
> >  };
> > @@ -154,11 +157,42 @@ union VTD_IR_MSIAddress {
> >  
> >  /* 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 */
> > diff --git a/include/hw/pci/msi.h b/include/hw/pci/msi.h
> > index 8124908..8e94fca 100644
> > --- a/include/hw/pci/msi.h
> > +++ b/include/hw/pci/msi.h
> > @@ -24,6 +24,10 @@
> >  #include "qemu-common.h"
> >  #include "hw/pci/pci.h"
> >  
> > +#define  MSI_ADDR_HI_MASK        (0xffffffff00000000ULL)
> > +#define  MSI_ADDR_HI_SHIFT       (32)
> > +#define  MSI_ADDR_LO_MASK        (0x00000000ffffffffULL)
> > +
> 
> I don't think these are needed in msi.h, rename them with
> vtd prefix and put them where they are used.

Ok.

Thanks.

-- peterx

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

[Michael S. Tsirkin]

On Mon, Apr 11, 2016 at 05:19:10PM +0800, Peter Xu wrote:
> 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 basic network (still do not support kvm-ioapic,
> so we need to specify kernel-irqchip=off here):
> 
> $ qemu-system-x86_64 -M q35,iommu=on,intr=on,kernel-irqchip=off \
>      -enable-kvm -m 1024 -s \
>      -monitor telnet::3333,server,nowait \
>      -netdev user,id=user.0,hostfwd=tcp::5555-:22 \
>      -device virtio-net-pci,netdev=user.0 \
> 	 /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 only two devices are supported:
> 
> - Emulated IOAPIC device
> - PCI Devices
> 
> TODO List:
> 
> - kvm-ioapic support

This is probably a must. I don't think we can merge this
as long as it breaks kvm.

> - vhost support

dataplane as well?

> - pass through device support
> - EIM support
> - IR fault reporting
> - source-id validation for IRTE
> - IRTE cache and better queued invalidation
> - migration support (for IOMMU as general?)
> - more?
> 
> Peter Xu (13):
>   q35: add "int-remap" flag to enable intr
>   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
>   ioapic-common: add iommu for IOAPICCommonState
>   intel_iommu: add IR translation faults defines
>   intel_iommu: ioapic: IR support for emulated IOAPIC
>   intel_iommu: Add support for PCI MSI remap
> 
>  hw/core/machine.c                 |  21 ++
>  hw/i386/acpi-build.c              |  42 ++--
>  hw/i386/intel_iommu.c             | 400 +++++++++++++++++++++++++++++++++++++-
>  hw/i386/intel_iommu_internal.h    |  23 +++
>  hw/intc/ioapic.c                  |  36 +++-
>  hw/intc/ioapic_common.c           |   2 +
>  include/hw/acpi/acpi-defs.h       |  15 ++
>  include/hw/boards.h               |   1 +
>  include/hw/i386/intel_iommu.h     | 120 ++++++++++++
>  include/hw/i386/ioapic_internal.h |   3 +
>  include/hw/pci/msi.h              |   4 +
>  11 files changed, 647 insertions(+), 20 deletions(-)
> 
> -- 
> 2.4.3

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

[Peter Xu]

On Mon, Apr 11, 2016 at 03:32:18PM +0300, Michael S. Tsirkin wrote:
> On Mon, Apr 11, 2016 at 05:19:10PM +0800, Peter Xu wrote:
> > 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 basic network (still do not support kvm-ioapic,
> > so we need to specify kernel-irqchip=off here):
> > 
> > $ qemu-system-x86_64 -M q35,iommu=on,intr=on,kernel-irqchip=off \
> >      -enable-kvm -m 1024 -s \
> >      -monitor telnet::3333,server,nowait \
> >      -netdev user,id=user.0,hostfwd=tcp::5555-:22 \
> >      -device virtio-net-pci,netdev=user.0 \
> > 	 /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 only two devices are supported:
> > 
> > - Emulated IOAPIC device
> > - PCI Devices
> > 
> > TODO List:
> > 
> > - kvm-ioapic support
> 
> This is probably a must. I don't think we can merge this
> as long as it breaks kvm.

But kvm-ioapic might require modification to KVM as well. Do you
think I should add kvm-ioapic into this series as well? Or can I
first submit this part of work without kvm-ioapic (since this work
is not related to KVM at all), then work on another one to support
kvm-ioapic?

> 
> > - vhost support
> 
> dataplane as well?

Do you mean irqfd here?

Thanks.

-- peterx

> 
> > - pass through device support
> > - EIM support
> > - IR fault reporting
> > - source-id validation for IRTE
> > - IRTE cache and better queued invalidation
> > - migration support (for IOMMU as general?)
> > - more?
> > 
> > Peter Xu (13):
> >   q35: add "int-remap" flag to enable intr
> >   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
> >   ioapic-common: add iommu for IOAPICCommonState
> >   intel_iommu: add IR translation faults defines
> >   intel_iommu: ioapic: IR support for emulated IOAPIC
> >   intel_iommu: Add support for PCI MSI remap
> > 
> >  hw/core/machine.c                 |  21 ++
> >  hw/i386/acpi-build.c              |  42 ++--
> >  hw/i386/intel_iommu.c             | 400 +++++++++++++++++++++++++++++++++++++-
> >  hw/i386/intel_iommu_internal.h    |  23 +++
> >  hw/intc/ioapic.c                  |  36 +++-
> >  hw/intc/ioapic_common.c           |   2 +
> >  include/hw/acpi/acpi-defs.h       |  15 ++
> >  include/hw/boards.h               |   1 +
> >  include/hw/i386/intel_iommu.h     | 120 ++++++++++++
> >  include/hw/i386/ioapic_internal.h |   3 +
> >  include/hw/pci/msi.h              |   4 +
> >  11 files changed, 647 insertions(+), 20 deletions(-)
> > 
> > -- 
> > 2.4.3

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

[Jan Kiszka]

On 2016-04-13 00:27, Peter Xu wrote:
> On Mon, Apr 11, 2016 at 03:32:18PM +0300, Michael S. Tsirkin wrote:
>> On Mon, Apr 11, 2016 at 05:19:10PM +0800, Peter Xu wrote:
>>> 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 basic network (still do not support kvm-ioapic,
>>> so we need to specify kernel-irqchip=off here):
>>>
>>> $ qemu-system-x86_64 -M q35,iommu=on,intr=on,kernel-irqchip=off \
>>>      -enable-kvm -m 1024 -s \
>>>      -monitor telnet::3333,server,nowait \
>>>      -netdev user,id=user.0,hostfwd=tcp::5555-:22 \
>>>      -device virtio-net-pci,netdev=user.0 \
>>> 	 /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 only two devices are supported:
>>>
>>> - Emulated IOAPIC device
>>> - PCI Devices
>>>
>>> TODO List:
>>>
>>> - kvm-ioapic support
>>
>> This is probably a must. I don't think we can merge this
>> as long as it breaks kvm.
> 
> But kvm-ioapic might require modification to KVM as well. Do you
> think I should add kvm-ioapic into this series as well? Or can I
> first submit this part of work without kvm-ioapic (since this work
> is not related to KVM at all), then work on another one to support
> kvm-ioapic?

Just go for split irqchip. If we ever need support for in-kernel irqchip
is questionable.

Jan

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

[Peter Xu]

On Wed, Apr 13, 2016 at 07:39:53AM -0700, Jan Kiszka wrote:
> On 2016-04-13 00:27, Peter Xu wrote:
> > On Mon, Apr 11, 2016 at 03:32:18PM +0300, Michael S. Tsirkin wrote:
> >> On Mon, Apr 11, 2016 at 05:19:10PM +0800, Peter Xu wrote:
> >>> 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 basic network (still do not support kvm-ioapic,
> >>> so we need to specify kernel-irqchip=off here):
> >>>
> >>> $ qemu-system-x86_64 -M q35,iommu=on,intr=on,kernel-irqchip=off \
> >>>      -enable-kvm -m 1024 -s \
> >>>      -monitor telnet::3333,server,nowait \
> >>>      -netdev user,id=user.0,hostfwd=tcp::5555-:22 \
> >>>      -device virtio-net-pci,netdev=user.0 \
> >>> 	 /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 only two devices are supported:
> >>>
> >>> - Emulated IOAPIC device
> >>> - PCI Devices
> >>>
> >>> TODO List:
> >>>
> >>> - kvm-ioapic support
> >>
> >> This is probably a must. I don't think we can merge this
> >> as long as it breaks kvm.
> > 
> > But kvm-ioapic might require modification to KVM as well. Do you
> > think I should add kvm-ioapic into this series as well? Or can I
> > first submit this part of work without kvm-ioapic (since this work
> > is not related to KVM at all), then work on another one to support
> > kvm-ioapic?
> 
> Just go for split irqchip. If we ever need support for in-kernel irqchip
> is questionable.

Okay. I can add one more patch to support split irqchip I suppose,
that should be much easier. The idea would be: to translate
interrupts before updating KVM routes in IOAPIC. Also, I may need to
take care of IR cache invalidation this time, since if I do the
above, remapping will be cached in kernel gsi routes.

I suppose irqfd will solve itself too after we support splitted
irqchip, since irqfd should be using the gsi routes. Will do more
test to confirm this.

Thanks.

-- peterx

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

[Alex Williamson]

On Mon, 11 Apr 2016 17:19:10 +0800
Peter Xu <peterx@redhat.com> wrote:

> 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 basic network (still do not support kvm-ioapic,
> so we need to specify kernel-irqchip=off here):
> 
> $ qemu-system-x86_64 -M q35,iommu=on,intr=on,kernel-irqchip=off \
>      -enable-kvm -m 1024 -s \
>      -monitor telnet::3333,server,nowait \
>      -netdev user,id=user.0,hostfwd=tcp::5555-:22 \
>      -device virtio-net-pci,netdev=user.0 \
> 	 /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 only two devices are supported:
> 
> - Emulated IOAPIC device
> - PCI Devices
> 
> TODO List:
> 
> - kvm-ioapic support
> - vhost support
> - pass through device support

vhost and assigned devices should be identical, which boils down to
irqfd support.  I imagine a vfio-pci device works with what you've got
here since kvm irqfd support will be disabled without kernel irqchip
(vfio doesn't depend on it).

Also, patches 1, 2, and 4 seem mis-ordered, I would think we only want
to advertise IR support both to the VM and to the user once it's
complete, otherwise we have versions where the options are enabled yet
don't work, even if only in git.  Otherwise very nice patch breakdown.
Thanks,

Alex

> - EIM support
> - IR fault reporting
> - source-id validation for IRTE
> - IRTE cache and better queued invalidation
> - migration support (for IOMMU as general?)
> - more?
> 
> Peter Xu (13):
>   q35: add "int-remap" flag to enable intr
>   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
>   ioapic-common: add iommu for IOAPICCommonState
>   intel_iommu: add IR translation faults defines
>   intel_iommu: ioapic: IR support for emulated IOAPIC
>   intel_iommu: Add support for PCI MSI remap
> 
>  hw/core/machine.c                 |  21 ++
>  hw/i386/acpi-build.c              |  42 ++--
>  hw/i386/intel_iommu.c             | 400 +++++++++++++++++++++++++++++++++++++-
>  hw/i386/intel_iommu_internal.h    |  23 +++
>  hw/intc/ioapic.c                  |  36 +++-
>  hw/intc/ioapic_common.c           |   2 +
>  include/hw/acpi/acpi-defs.h       |  15 ++
>  include/hw/boards.h               |   1 +
>  include/hw/i386/intel_iommu.h     | 120 ++++++++++++
>  include/hw/i386/ioapic_internal.h |   3 +
>  include/hw/pci/msi.h              |   4 +
>  11 files changed, 647 insertions(+), 20 deletions(-)
> 

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

[Peter Xu]

On Mon, Apr 11, 2016 at 04:19:01PM -0600, Alex Williamson wrote:
> On Mon, 11 Apr 2016 17:19:10 +0800
> Peter Xu <peterx@redhat.com> wrote:
> 
> > 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 basic network (still do not support kvm-ioapic,
> > so we need to specify kernel-irqchip=off here):
> > 
> > $ qemu-system-x86_64 -M q35,iommu=on,intr=on,kernel-irqchip=off \
> >      -enable-kvm -m 1024 -s \
> >      -monitor telnet::3333,server,nowait \
> >      -netdev user,id=user.0,hostfwd=tcp::5555-:22 \
> >      -device virtio-net-pci,netdev=user.0 \
> > 	 /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 only two devices are supported:
> > 
> > - Emulated IOAPIC device
> > - PCI Devices
> > 
> > TODO List:
> > 
> > - kvm-ioapic support
> > - vhost support
> > - pass through device support

Hi, Alex,

> 
> vhost and assigned devices should be identical, which boils down to
> irqfd support.  I imagine a vfio-pci device works with what you've got
> here since kvm irqfd support will be disabled without kernel irqchip
> (vfio doesn't depend on it).

Thanks for the confirmation on vfio-pci part. That would be great if
we can make it work without special care. :)

> 
> Also, patches 1, 2, and 4 seem mis-ordered, I would think we only want
> to advertise IR support both to the VM and to the user once it's
> complete, otherwise we have versions where the options are enabled yet
> don't work, even if only in git.  Otherwise very nice patch breakdown.

Do you mean I should put patch 1 to the end of the series? Since
even with patch 2 and 4, IR won't work as well, and guest will hang
at kernel boot.  I can move patch 1 to the end of series
though. That will make sure that when people see the parameter, they
can use it as well.

Thanks!

-- peterx