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

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

[Peter Xu]

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,int_remap=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                 |  20 ++
 hw/i386/acpi-build.c              |  41 +++-
 hw/i386/intel_iommu.c             | 397 +++++++++++++++++++++++++++++++++++++-
 hw/i386/intel_iommu_internal.h    |  25 +++
 hw/intc/ioapic.c                  |  36 +++-
 hw/intc/ioapic_common.c           |   2 +
 hw/pci-host/q35.c                 |   6 +-
 include/hw/acpi/acpi-defs.h       |  15 ++
 include/hw/boards.h               |   1 +
 include/hw/i386/intel_iommu.h     | 121 ++++++++++++
 include/hw/i386/ioapic_internal.h |   3 +
 include/hw/pci/msi.h              |   4 +
 12 files changed, 651 insertions(+), 20 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH 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,int_remap=on

To be more clear, the following command:

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

Will enable Intel IOMMU only, without interrupt remapping support.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/core/machine.c             | 20 ++++++++++++++++++++
 hw/pci-host/q35.c             |  6 ++++--
 include/hw/boards.h           |  1 +
 include/hw/i386/intel_iommu.h |  3 +++
 4 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 6d1a0d8..852cee2 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -298,6 +298,20 @@ static void machine_set_iommu(Object *obj, bool value, Error **errp)
     ms->iommu = value;
 }
 
+static bool machine_get_int_remap(Object *obj, Error **errp)
+{
+    MachineState *ms = MACHINE(obj);
+
+    return ms->int_remap;
+}
+
+static void machine_set_int_remap(Object *obj, bool value, Error **errp)
+{
+    MachineState *ms = MACHINE(obj);
+
+    ms->int_remap = value;
+}
+
 static void machine_set_suppress_vmdesc(Object *obj, bool value, Error **errp)
 {
     MachineState *ms = MACHINE(obj);
@@ -461,6 +475,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, "int-remap",
+                             machine_get_int_remap,
+                             machine_set_int_remap, NULL);
+    object_property_set_description(obj, "int-remap",
+                                    "Set on/off to enable/disable Intel IOMMU INTR",
+                                    NULL);
     object_property_add_bool(obj, "suppress-vmdesc",
                              machine_get_suppress_vmdesc,
                              machine_set_suppress_vmdesc, NULL);
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 115fb8c..7cd4d87 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -434,13 +434,14 @@ static AddressSpace *q35_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
     return &vtd_as->as;
 }
 
-static void mch_init_dmar(MCHPCIState *mch)
+static void mch_init_dmar(MCHPCIState *mch, bool intr_supported)
 {
     PCIBus *pci_bus = PCI_BUS(qdev_get_parent_bus(DEVICE(mch)));
 
     mch->iommu = INTEL_IOMMU_DEVICE(qdev_create(NULL, TYPE_INTEL_IOMMU_DEVICE));
     object_property_add_child(OBJECT(mch), "intel-iommu",
                               OBJECT(mch->iommu), NULL);
+    mch->iommu->intr_supported = intr_supported;
     qdev_init_nofail(DEVICE(mch->iommu));
     sysbus_mmio_map(SYS_BUS_DEVICE(mch->iommu), 0, Q35_HOST_BRIDGE_IOMMU_ADDR);
 
@@ -507,7 +508,8 @@ static void mch_realize(PCIDevice *d, Error **errp)
     }
     /* Intel IOMMU (VT-d) */
     if (object_property_get_bool(qdev_get_machine(), "iommu", NULL)) {
-        mch_init_dmar(mch);
+        mch_init_dmar(mch, object_property_get_bool(qdev_get_machine(),
+                                                    "int-remap", false));
     }
 }
 
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 0f30959..d488e40 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -127,6 +127,7 @@ struct MachineState {
     bool igd_gfx_passthru;
     char *firmware;
     bool iommu;
+    bool int_remap;
     bool suppress_vmdesc;
 
     ram_addr_t ram_size;
diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
index b024ffa..6e52c6b 100644
--- a/include/hw/i386/intel_iommu.h
+++ b/include/hw/i386/intel_iommu.h
@@ -123,6 +123,9 @@ 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_supported;            /* Whether IR is supported */
 };
 
 /* Find the VTD Address space associated with the given bus pointer,
-- 
2.4.3

[Qemu-devel] [PATCH 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          | 31 ++++++++++++++++++++++++-------
 include/hw/i386/intel_iommu.h |  2 ++
 2 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 4554eb8..d9e4f91 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2489,6 +2489,19 @@ build_mcfg_q35(GArray *table_data, GArray *linker, AcpiMcfgInfo *info)
     build_header(linker, table_data, (void *)mcfg, sig, len, 1, NULL, NULL);
 }
 
+static IntelIOMMUState *acpi_get_iommu(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;
+}
+
 static void
 build_dmar_q35(GArray *table_data, GArray *linker)
 {
@@ -2496,10 +2509,19 @@ build_dmar_q35(GArray *table_data, GArray *linker)
 
     AcpiTableDmar *dmar;
     AcpiDmarHardwareUnit *drhd;
+    uint8_t dmar_flags = 0;
+    IntelIOMMUState *intel_iommu = acpi_get_iommu();
+
+    assert(intel_iommu);
+
+    if (intel_iommu->intr_supported) {
+        /* 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));
@@ -2572,12 +2594,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 !!acpi_get_iommu();
 }
 
 static bool acpi_has_nvdimm(void)
diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
index 6e52c6b..83e5a1e 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 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 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          | 4 ++++
 hw/i386/intel_iommu_internal.h | 2 ++
 2 files changed, 6 insertions(+)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 4b0558e..79585d2 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -1961,6 +1961,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 (s->intr_supported) {
+        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 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 d9e4f91..1cefe43 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -76,6 +76,9 @@
 #define ACPI_BUILD_DPRINTF(fmt, ...)
 #endif
 
+/* Default IOAPIC ID */
+#define ACPI_BUILD_IOAPIC_ID 0x0
+
 typedef struct AcpiCpuInfo {
     DECLARE_BITMAP(found_cpus, ACPI_CPU_HOTPLUG_ID_LIMIT);
 } AcpiCpuInfo;
@@ -392,7 +395,6 @@ build_madt(GArray *table_data, GArray *linker, AcpiCpuInfo *cpu)
     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);
@@ -2511,6 +2513,9 @@ build_dmar_q35(GArray *table_data, GArray *linker)
     AcpiDmarHardwareUnit *drhd;
     uint8_t dmar_flags = 0;
     IntelIOMMUState *intel_iommu = acpi_get_iommu();
+    AcpiDmarDeviceScope *scope = NULL;
+    /* Root complex IOAPIC use one path[0] only */
+    uint16_t scope_size = sizeof(*scope) + sizeof(uint16_t);
 
     assert(intel_iommu);
 
@@ -2526,11 +2531,25 @@ build_dmar_q35(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         (0xf0)
+#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 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  |  3 +++
 3 files changed, 58 insertions(+), 1 deletion(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 79585d2..62f0fa7 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -29,7 +29,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);
@@ -899,6 +899,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++;
@@ -1137,6 +1150,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)
 {
@@ -1176,6 +1199,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 */
@@ -1837,6 +1864,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);
@@ -2014,6 +2058,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 83e5a1e..0107488 100644
--- a/include/hw/i386/intel_iommu.h
+++ b/include/hw/i386/intel_iommu.h
@@ -128,6 +128,9 @@ struct IntelIOMMUState {
 
     /* interrupt remapping */
     bool intr_supported;            /* Whether IR is supported */
+    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 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 62f0fa7..f1cb574 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -1179,6 +1179,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)
 {
@@ -1203,6 +1219,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 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 0107488..89781b4 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 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          | 17 ++---------------
 hw/i386/intel_iommu.c         | 13 +++++++++++++
 include/hw/i386/intel_iommu.h |  2 ++
 3 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 1cefe43..3cc7886 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2491,19 +2491,6 @@ build_mcfg_q35(GArray *table_data, GArray *linker, AcpiMcfgInfo *info)
     build_header(linker, table_data, (void *)mcfg, sig, len, 1, NULL, NULL);
 }
 
-static IntelIOMMUState *acpi_get_iommu(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;
-}
-
 static void
 build_dmar_q35(GArray *table_data, GArray *linker)
 {
@@ -2512,7 +2499,7 @@ build_dmar_q35(GArray *table_data, GArray *linker)
     AcpiTableDmar *dmar;
     AcpiDmarHardwareUnit *drhd;
     uint8_t dmar_flags = 0;
-    IntelIOMMUState *intel_iommu = acpi_get_iommu();
+    IntelIOMMUState *intel_iommu = vtd_iommu_get();
     AcpiDmarDeviceScope *scope = NULL;
     /* Root complex IOAPIC use one path[0] only */
     uint16_t scope_size = sizeof(*scope) + sizeof(uint16_t);
@@ -2613,7 +2600,7 @@ static bool acpi_get_mcfg(AcpiMcfgInfo *mcfg)
 
 static bool acpi_has_iommu(void)
 {
-    return !!acpi_get_iommu();
+    return !!vtd_iommu_get();
 }
 
 static bool acpi_has_nvdimm(void)
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index f1cb574..a9cbd7d 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -2000,6 +2000,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 89781b4..bb94fbd 100644
--- a/include/hw/i386/intel_iommu.h
+++ b/include/hw/i386/intel_iommu.h
@@ -197,5 +197,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 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 0a48de2..2c25aaa 100644
--- a/hw/intc/ioapic_common.c
+++ b/hw/intc/ioapic_common.c
@@ -136,6 +136,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 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 | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
index 309833f..c66cb83 100644
--- a/hw/i386/intel_iommu_internal.h
+++ b/hw/i386/intel_iommu_internal.h
@@ -271,6 +271,21 @@ 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 resved
+                                * 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 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 a9cbd7d..089dac9 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -2013,6 +2013,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 bb94fbd..d3ba343 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)
 
@@ -199,5 +213,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

[Qemu-devel] [PATCH 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 089dac9..34aa1fa 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -43,6 +43,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)
 {
@@ -1963,12 +1967,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;
@@ -1994,6 +2056,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");
     }
@@ -2074,6 +2141,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,
@@ -2140,6 +2208,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 c66cb83..8668cc8 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 d3ba343..f4590d9 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 50e452b..14f35c7 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 00/13] IOMMU: Enable interrupt remapping for Intel IOMMU

[Jan Kiszka]

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

Hi Peter,

On 2016-02-19 04:30, Peter Xu wrote:
> This patchset provide very basic functionalities for interrupt
> remapping (IR) support of the emulated Intel IOMMU device.

Interesting. Some questions:

- Were you aware of
http://git.kiszka.org/?p=qemu.git;a=shortlog;h=refs/heads/queues/vtd-intremap,
and can you comment on how this relates to your approach? Mine included
HPET support, e.g.

- Rita Sinha is currently working on integrating my old patches with the
split-irqchip to get KVM working (as an Outreachy project). It's
probably a bit unfortunate to consider a different horse that late in to
project. What do you think, how could we benefit from each other?

- Radim was telling me to look on this as well. How do your efforts
correlate?

> 
> 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,int_remap=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                 |  20 ++
>  hw/i386/acpi-build.c              |  41 +++-
>  hw/i386/intel_iommu.c             | 397 +++++++++++++++++++++++++++++++++++++-
>  hw/i386/intel_iommu_internal.h    |  25 +++
>  hw/intc/ioapic.c                  |  36 +++-
>  hw/intc/ioapic_common.c           |   2 +
>  hw/pci-host/q35.c                 |   6 +-
>  include/hw/acpi/acpi-defs.h       |  15 ++
>  include/hw/boards.h               |   1 +
>  include/hw/i386/intel_iommu.h     | 121 ++++++++++++
>  include/hw/i386/ioapic_internal.h |   3 +
>  include/hw/pci/msi.h              |   4 +
>  12 files changed, 651 insertions(+), 20 deletions(-)
> 

Is there a git tree with your patches somewhere?

Thanks,
Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Peter Xu]

Hi, Jan,

On Fri, Feb 19, 2016 at 07:46:26AM +0100, Jan Kiszka wrote:
> Hi Peter,
> 
> On 2016-02-19 04:30, Peter Xu wrote:
> > This patchset provide very basic functionalities for interrupt
> > remapping (IR) support of the emulated Intel IOMMU device.
> 
> Interesting. Some questions:
> 
> - Were you aware of
> http://git.kiszka.org/?p=qemu.git;a=shortlog;h=refs/heads/queues/vtd-intremap,
> and can you comment on how this relates to your approach? Mine included
> HPET support, e.g.

No. I do not know we have existing works related. I'd say that I am
one of those who are not fan of duplicating works. If I know that
there is existing ones, I would have asked you first.

Actually there are several people within my working team knows that
I would be working on this, and I believe none of us do know your
work too... Or there must be someone telling me so...

Whatever, I am sorry that finally we got some duplicated work. :-(

I have not go deeper into your codes yet, I'd say that most of the
logic is alike, since lots of works are to follow the spec. And the
implementation is slightly different (actually I just got this
memory region idea from Paolo yesterday... instead of a worse one to
declare a specific func ops for MSIs).

What I was going to do is to at least support:

- vhost
- pass through devices

So what I was planning is that, this series will be the start. And
the above two is the first-step goal (and I may need kvm-ioapic as
well though).

With the above, we should be able to run DPDK applications in guests
with either pass-through or vhost-user devices (these should be the
two scenario which is most possibly to be used AFAIU). This is what
I was trying to do. HPET is not with high priority in my todo list.

> 
> - Rita Sinha is currently working on integrating my old patches with the
> split-irqchip to get KVM working (as an Outreachy project). It's
> probably a bit unfortunate to consider a different horse that late in to
> project. What do you think, how could we benefit from each other?

I'd say that I am still new to QEMU community. So this is the first
time I encountered this kind of problem... Do you have any
suggestion?

> 
> - Radim was telling me to look on this as well. How do your efforts
> correlate?

Same as above. Do you have any suggestion on how we'd move on?

One question about your tree: have you posted patches before? and
what's the relationship between your tree and current QEMU master?

> 
> > 
> > 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,int_remap=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                 |  20 ++
> >  hw/i386/acpi-build.c              |  41 +++-
> >  hw/i386/intel_iommu.c             | 397 +++++++++++++++++++++++++++++++++++++-
> >  hw/i386/intel_iommu_internal.h    |  25 +++
> >  hw/intc/ioapic.c                  |  36 +++-
> >  hw/intc/ioapic_common.c           |   2 +
> >  hw/pci-host/q35.c                 |   6 +-
> >  include/hw/acpi/acpi-defs.h       |  15 ++
> >  include/hw/boards.h               |   1 +
> >  include/hw/i386/intel_iommu.h     | 121 ++++++++++++
> >  include/hw/i386/ioapic_internal.h |   3 +
> >  include/hw/pci/msi.h              |   4 +
> >  12 files changed, 651 insertions(+), 20 deletions(-)
> > 
> 
> Is there a git tree with your patches somewhere?

Currently not. If needed, I can try to create one. 

Thanks.
Peter

> 
> Thanks,
> Jan
> 

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

[Jan Kiszka]

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

On 2016-02-19 08:43, Peter Xu wrote:
> Hi, Jan,
> 
> On Fri, Feb 19, 2016 at 07:46:26AM +0100, Jan Kiszka wrote:
>> Hi Peter,
>>
>> On 2016-02-19 04:30, Peter Xu wrote:
>>> This patchset provide very basic functionalities for interrupt
>>> remapping (IR) support of the emulated Intel IOMMU device.
>>
>> Interesting. Some questions:
>>
>> - Were you aware of
>> http://git.kiszka.org/?p=qemu.git;a=shortlog;h=refs/heads/queues/vtd-intremap,
>> and can you comment on how this relates to your approach? Mine included
>> HPET support, e.g.
> 
> No. I do not know we have existing works related. I'd say that I am
> one of those who are not fan of duplicating works. If I know that
> there is existing ones, I would have asked you first.
> 
> Actually there are several people within my working team knows that
> I would be working on this, and I believe none of us do know your
> work too... Or there must be someone telling me so...

I guess we would have to match sets of people know to find out who
forgot to mention the outreachy project ;) - anyway, this can always happen.

> 
> Whatever, I am sorry that finally we got some duplicated work. :-(
> 
> I have not go deeper into your codes yet, I'd say that most of the
> logic is alike, since lots of works are to follow the spec. And the
> implementation is slightly different (actually I just got this
> memory region idea from Paolo yesterday... instead of a worse one to
> declare a specific func ops for MSIs).

I didn't look into your approach in all details yet, and Rita also just
started, she told me. So one question on yours - which looks appealing
from the invasiveness POV - is how you determine the MSI source with
only a single target region? I do find your changes on the IOAPIC, but
none on the PCI infrastructure, which is confusing given that you say
that works, no?

My design introduces a per-source MSI (DMA) target region so that the
IOMMU can do proper remapping by deriving the source device ID from the
targeted region. Then you only need to give the special sources like
IOAPIC and HPET their only regions, and you are done.

> 
> What I was going to do is to at least support:
> 
> - vhost
> - pass through devices

vhost, i.e. virtio, has still problems as current virtio guest drivers
assume that their devices are not under control of any IOMMU unit. I
didn't trace the latest news here, but we were once discussing to
initially exclude virtio devices in DMAR scope descriptions in ACPI
and/or introduce some feature negotiation between guest and host to
identify and reject legacy guest drivers without IOMMU-awareness.

> 
> So what I was planning is that, this series will be the start. And
> the above two is the first-step goal (and I may need kvm-ioapic as
> well though).

KVM support is actually a key thing, that's why we started the project
on integrating the patches with the split irqchip work. There was a
consensus with Paolo long ago that full in-kernel irqchip will no longer
gain any additional support that is needed for IR.

> 
> With the above, we should be able to run DPDK applications in guests
> with either pass-through or vhost-user devices (these should be the
> two scenario which is most possibly to be used AFAIU). This is what
> I was trying to do. HPET is not with high priority in my todo list.
> 
>>
>> - Rita Sinha is currently working on integrating my old patches with the
>> split-irqchip to get KVM working (as an Outreachy project). It's
>> probably a bit unfortunate to consider a different horse that late in to
>> project. What do you think, how could we benefit from each other?
> 
> I'd say that I am still new to QEMU community. So this is the first
> time I encountered this kind of problem... Do you have any
> suggestion?
> 
>>
>> - Radim was telling me to look on this as well. How do your efforts
>> correlate?
> 
> Same as above. Do you have any suggestion on how we'd move on?

Specifically you and Rita should exchange your knowledge on both designs
(here on the list, of course) and try to find an optimal path. We are
unfortunately a bit time-constrained /wrt Outreachy as that project ends
in a couple of weeks. By then, we should have first results for the KVM
integration - not necessarily the perfect ones, but something working
and following the "right" direction.

> 
> One question about your tree: have you posted patches before? and
> what's the relationship between your tree and current QEMU master?

I never posted the patches as they never left the state of "hacks" as
you can quickly spot. However, I think the design is not a hack, but
maybe there is indeed something even smarter.

Rita has rebased the patches recently, maybe you can share your state,
Rita? I did this before, because I'm using them since day #1 to do
regular testing of our Jailhouse hypervisor inside QEMU/KVM. For that
purpose they work very reliably for IOAPIC, HPET and emulated PCI devices.

> 
>>
>>>
>>> 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,int_remap=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                 |  20 ++
>>>  hw/i386/acpi-build.c              |  41 +++-
>>>  hw/i386/intel_iommu.c             | 397 +++++++++++++++++++++++++++++++++++++-
>>>  hw/i386/intel_iommu_internal.h    |  25 +++
>>>  hw/intc/ioapic.c                  |  36 +++-
>>>  hw/intc/ioapic_common.c           |   2 +
>>>  hw/pci-host/q35.c                 |   6 +-
>>>  include/hw/acpi/acpi-defs.h       |  15 ++
>>>  include/hw/boards.h               |   1 +
>>>  include/hw/i386/intel_iommu.h     | 121 ++++++++++++
>>>  include/hw/i386/ioapic_internal.h |   3 +
>>>  include/hw/pci/msi.h              |   4 +
>>>  12 files changed, 651 insertions(+), 20 deletions(-)
>>>
>>
>> Is there a git tree with your patches somewhere?
> 
> Currently not. If needed, I can try to create one. 

Would be helpful for more convenient import and experiments. E.g. I
could quickly through my test setup at them and tell you if they work
for it.

Jan



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Peter Xu]

On Fri, Feb 19, 2016 at 09:34:40AM +0100, Jan Kiszka wrote:
> On 2016-02-19 08:43, Peter Xu wrote:
> > Hi, Jan,
> > 
> > On Fri, Feb 19, 2016 at 07:46:26AM +0100, Jan Kiszka wrote:
> >> Hi Peter,
> >>
> >> On 2016-02-19 04:30, Peter Xu wrote:
> >>> This patchset provide very basic functionalities for interrupt
> >>> remapping (IR) support of the emulated Intel IOMMU device.
> >>
> >> Interesting. Some questions:
> >>
> >> - Were you aware of
> >> http://git.kiszka.org/?p=qemu.git;a=shortlog;h=refs/heads/queues/vtd-intremap,
> >> and can you comment on how this relates to your approach? Mine included
> >> HPET support, e.g.
> > 
> > No. I do not know we have existing works related. I'd say that I am
> > one of those who are not fan of duplicating works. If I know that
> > there is existing ones, I would have asked you first.
> > 
> > Actually there are several people within my working team knows that
> > I would be working on this, and I believe none of us do know your
> > work too... Or there must be someone telling me so...
> 
> I guess we would have to match sets of people know to find out who
> forgot to mention the outreachy project ;) - anyway, this can always happen.
> 
> > 
> > Whatever, I am sorry that finally we got some duplicated work. :-(
> > 
> > I have not go deeper into your codes yet, I'd say that most of the
> > logic is alike, since lots of works are to follow the spec. And the
> > implementation is slightly different (actually I just got this
> > memory region idea from Paolo yesterday... instead of a worse one to
> > declare a specific func ops for MSIs).
> 
> I didn't look into your approach in all details yet, and Rita also just
> started, she told me. So one question on yours - which looks appealing
> from the invasiveness POV - is how you determine the MSI source with
> only a single target region? I do find your changes on the IOAPIC, but
> none on the PCI infrastructure, which is confusing given that you say
> that works, no?

Do we need to know the source of the MSI interrupt to
translate/deliver it? Maybe I got something missing, but could you
explain why we need it?

What I understand is that, for interrupt remapping, IRT (Interrupt
Remapping Table) is shared within the whole device tree managed by
specific IOMMU (or say, IRE is shared between different source IDs),
which is kinda different from DMAR.

The patch should basically work at least on my machine, and please
feel free to try it if you want. I just created one public tree on
github, link will be pasted below. Also please refer to the cover
letter for the command line to boot the guest. I have not done any
thorough tests, since I just want to first post it out in case there
is anything not right, and both luckily & unluckily... I got to know
that there are people working on it before going deeper. :(

> 
> My design introduces a per-source MSI (DMA) target region so that the
> IOMMU can do proper remapping by deriving the source device ID from the
> targeted region. Then you only need to give the special sources like
> IOAPIC and HPET their only regions, and you are done.

My thought explained as above. Please let me know if I was wrong.

> 
> > 
> > What I was going to do is to at least support:
> > 
> > - vhost
> > - pass through devices
> 
> vhost, i.e. virtio, has still problems as current virtio guest drivers
> assume that their devices are not under control of any IOMMU unit. I
> didn't trace the latest news here, but we were once discussing to
> initially exclude virtio devices in DMAR scope descriptions in ACPI
> and/or introduce some feature negotiation between guest and host to
> identify and reject legacy guest drivers without IOMMU-awareness.
> 
> > 
> > So what I was planning is that, this series will be the start. And
> > the above two is the first-step goal (and I may need kvm-ioapic as
> > well though).
> 
> KVM support is actually a key thing, that's why we started the project
> on integrating the patches with the split irqchip work. There was a
> consensus with Paolo long ago that full in-kernel irqchip will no longer
> gain any additional support that is needed for IR.

Do you have any link to the discussion? I am just curious about it,
thanks in advance.

> 
> > 
> > With the above, we should be able to run DPDK applications in guests
> > with either pass-through or vhost-user devices (these should be the
> > two scenario which is most possibly to be used AFAIU). This is what
> > I was trying to do. HPET is not with high priority in my todo list.
> > 
> >>
> >> - Rita Sinha is currently working on integrating my old patches with the
> >> split-irqchip to get KVM working (as an Outreachy project). It's
> >> probably a bit unfortunate to consider a different horse that late in to
> >> project. What do you think, how could we benefit from each other?
> > 
> > I'd say that I am still new to QEMU community. So this is the first
> > time I encountered this kind of problem... Do you have any
> > suggestion?
> > 
> >>
> >> - Radim was telling me to look on this as well. How do your efforts
> >> correlate?
> > 
> > Same as above. Do you have any suggestion on how we'd move on?
> 
> Specifically you and Rita should exchange your knowledge on both designs
> (here on the list, of course) and try to find an optimal path. We are
> unfortunately a bit time-constrained /wrt Outreachy as that project ends
> in a couple of weeks. By then, we should have first results for the KVM
> integration - not necessarily the perfect ones, but something working
> and following the "right" direction.

Regarding to the current KVM work for this, do we have discussion
thread or link so that I can know more about?

I see that the project does there:

http://qemu-project.org/Outreachy_2015_MayAugust#VT-d_interrupt_emulation_with_KVM_support

Sorry that I failed to notice it before. No matter which to choose, I would
like to help in any way.

> 
> > 
> > One question about your tree: have you posted patches before? and
> > what's the relationship between your tree and current QEMU master?
> 
> I never posted the patches as they never left the state of "hacks" as
> you can quickly spot. However, I think the design is not a hack, but
> maybe there is indeed something even smarter.
> 
> Rita has rebased the patches recently, maybe you can share your state,
> Rita? I did this before, because I'm using them since day #1 to do
> regular testing of our Jailhouse hypervisor inside QEMU/KVM. For that
> purpose they work very reliably for IOAPIC, HPET and emulated PCI devices.
> 
[...]
> 
> Would be helpful for more convenient import and experiments. E.g. I
> could quickly through my test setup at them and tell you if they work
> for it.

I have put codes onto github for better reference:

https://github.com/xzpeter/qemu/tree/vt-d-intr

Thanks.
Peter

> 
> Jan
> 
> 

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

[Paolo Bonzini]

On 19/02/2016 10:29, Peter Xu wrote:
> On Fri, Feb 19, 2016 at 09:34:40AM +0100, Jan Kiszka wrote:
>> On 2016-02-19 08:43, Peter Xu wrote:
>>> Actually there are several people within my working team knows that
>>> I would be working on this, and I believe none of us do know your
>>> work too... Or there must be someone telling me so...
>>
>> I guess we would have to match sets of people know to find out who
>> forgot to mention the outreachy project ;) - anyway, this can always happen.

I knew about the outreachy project and forgot to mention it... but then,
I only learnt about your effort yesterday.  :)

>> I didn't look into your approach in all details yet, and Rita also just
>> started, she told me. So one question on yours - which looks appealing
>> from the invasiveness POV - is how you determine the MSI source with
>> only a single target region? I do find your changes on the IOAPIC, but
>> none on the PCI infrastructure, which is confusing given that you say
>> that works, no?
> 
> Do we need to know the source of the MSI interrupt to
> translate/deliver it? Maybe I got something missing, but could you
> explain why we need it?

I think you're not verifying the SVT, SID and SQ fields in the IRTE.

The source ID can be passed to the IOMMU using the MemTxAttrs mechanism.

>>> So what I was planning is that, this series will be the start. And
>>> the above two is the first-step goal (and I may need kvm-ioapic as
>>> well though).
>>
>> KVM support is actually a key thing, that's why we started the project
>> on integrating the patches with the split irqchip work. There was a
>> consensus with Paolo long ago that full in-kernel irqchip will no longer
>> gain any additional support that is needed for IR.

Agreed.

> Do you have any link to the discussion? I am just curious about it,
> thanks in advance.

I couldn't find anything in kvm@vger.kernel.org, sorry.

>>>> - Radim was telling me to look on this as well. How do your efforts
>>>> correlate?

FWIW, Radim was thinking of interrupt remapping in the kvm-ioapic, which
we have decided to set aside.

Paolo

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

[Jan Kiszka]

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

On 2016-02-19 10:58, Paolo Bonzini wrote:
> 
> 
> On 19/02/2016 10:29, Peter Xu wrote:
>> On Fri, Feb 19, 2016 at 09:34:40AM +0100, Jan Kiszka wrote:
>>> On 2016-02-19 08:43, Peter Xu wrote:
>>>> Actually there are several people within my working team knows that
>>>> I would be working on this, and I believe none of us do know your
>>>> work too... Or there must be someone telling me so...
>>>
>>> I guess we would have to match sets of people know to find out who
>>> forgot to mention the outreachy project ;) - anyway, this can always happen.
> 
> I knew about the outreachy project and forgot to mention it... but then,
> I only learnt about your effort yesterday.  :)
> 
>>> I didn't look into your approach in all details yet, and Rita also just
>>> started, she told me. So one question on yours - which looks appealing
>>> from the invasiveness POV - is how you determine the MSI source with
>>> only a single target region? I do find your changes on the IOAPIC, but
>>> none on the PCI infrastructure, which is confusing given that you say
>>> that works, no?
>>
>> Do we need to know the source of the MSI interrupt to
>> translate/deliver it? Maybe I got something missing, but could you
>> explain why we need it?
> 
> I think you're not verifying the SVT, SID and SQ fields in the IRTE.

Exactly.

> 
> The source ID can be passed to the IOMMU using the MemTxAttrs mechanism.

Ah, that's a nice new channel, resolving the need for using/passing
source-specific target memory regions for this. At least for PCI
devices, it should already be populated with the required information,
others (IOAPIC, HPET) probably require additional work to pass what I
defined as Q35_PSEUDO_BUS_PLATFORM, Q35_PSEUDO_DEVFN_IOAPIC/HPET.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Peter Xu]

On Fri, Feb 19, 2016 at 10:58:12AM +0100, Paolo Bonzini wrote:
> 
> 
> On 19/02/2016 10:29, Peter Xu wrote:
> > On Fri, Feb 19, 2016 at 09:34:40AM +0100, Jan Kiszka wrote:
> >> On 2016-02-19 08:43, Peter Xu wrote:
> >>> Actually there are several people within my working team knows that
> >>> I would be working on this, and I believe none of us do know your
> >>> work too... Or there must be someone telling me so...
> >>
> >> I guess we would have to match sets of people know to find out who
> >> forgot to mention the outreachy project ;) - anyway, this can always happen.
> 
> I knew about the outreachy project and forgot to mention it... but then,
> I only learnt about your effort yesterday.  :)

Yes, it's nobody's bad except myself if the outreachy link is public
for such a long time. I'd need to ask more and do more pre-search on
anything I may work on in the future.

> 
> >> I didn't look into your approach in all details yet, and Rita also just
> >> started, she told me. So one question on yours - which looks appealing
> >> from the invasiveness POV - is how you determine the MSI source with
> >> only a single target region? I do find your changes on the IOAPIC, but
> >> none on the PCI infrastructure, which is confusing given that you say
> >> that works, no?
> > 
> > Do we need to know the source of the MSI interrupt to
> > translate/deliver it? Maybe I got something missing, but could you
> > explain why we need it?
> 
> I think you're not verifying the SVT, SID and SQ fields in the IRTE.

Yes. I have not taken SID verification into consideration, as
mentioned in the cover letter and code comments. I'd say that if
without the MemTxAttrs you mentioned below, current v1 patch does
not suite to add this feature.

> 
> The source ID can be passed to the IOMMU using the MemTxAttrs mechanism.

I see that MemTxAttrs is not enabled yet? It's dropped in all the
accessors like memory_region_write_accessor()?

> 
> >>> So what I was planning is that, this series will be the start. And
> >>> the above two is the first-step goal (and I may need kvm-ioapic as
> >>> well though).
> >>
> >> KVM support is actually a key thing, that's why we started the project
> >> on integrating the patches with the split irqchip work. There was a
> >> consensus with Paolo long ago that full in-kernel irqchip will no longer
> >> gain any additional support that is needed for IR.
> 
> Agreed.
> 
> > Do you have any link to the discussion? I am just curious about it,
> > thanks in advance.
> 
> I couldn't find anything in kvm@vger.kernel.org, sorry.
> 
> >>>> - Radim was telling me to look on this as well. How do your efforts
> >>>> correlate?
> 
> FWIW, Radim was thinking of interrupt remapping in the kvm-ioapic, which
> we have decided to set aside.

Does it mean that we are planning not to support kernel_irqchip for
IR? Any quick reason?

Thanks.
Peter

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

[Peter Xu]

On Fri, Feb 19, 2016 at 11:15:06AM +0100, Jan Kiszka wrote:
> On 2016-02-19 10:58, Paolo Bonzini wrote:
> > I think you're not verifying the SVT, SID and SQ fields in the IRTE.
> 
> Exactly.
> 
> > 
> > The source ID can be passed to the IOMMU using the MemTxAttrs mechanism.
> 
> Ah, that's a nice new channel, resolving the need for using/passing
> source-specific target memory regions for this. At least for PCI
> devices, it should already be populated with the required information,
> others (IOAPIC, HPET) probably require additional work to pass what I
> defined as Q35_PSEUDO_BUS_PLATFORM, Q35_PSEUDO_DEVFN_IOAPIC/HPET.

Jan, Rita, Paolo, 

One more thing to ask: do we have any plan to move on the work to
support vhost/pass-through devices? Do you think we need these too?

Thanks.
Peter

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

[Jan Kiszka]

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

On 2016-02-19 12:34, Peter Xu wrote:
> On Fri, Feb 19, 2016 at 10:58:12AM +0100, Paolo Bonzini wrote:
>>
>> The source ID can be passed to the IOMMU using the MemTxAttrs mechanism.
> 
> I see that MemTxAttrs is not enabled yet? It's dropped in all the
> accessors like memory_region_write_accessor()?

That would be a pity. How much work remains to fix that? Putting Peter
Maydell on CC who apparently started this extension.

...
>> FWIW, Radim was thinking of interrupt remapping in the kvm-ioapic, which
>> we have decided to set aside.
> 
> Does it mean that we are planning not to support kernel_irqchip for
> IR? Any quick reason?

The reason to split up the kernel irqchip is to reduce code complexity
and, thus, attack surface of the kernel-side interface towards the
guest. So, extending the "classic" in-kernel support with yet another
feature is not really supporting that goal.

Jan



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Jan Kiszka]

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

On 2016-02-19 12:39, Peter Xu wrote:
> On Fri, Feb 19, 2016 at 11:15:06AM +0100, Jan Kiszka wrote:
>> On 2016-02-19 10:58, Paolo Bonzini wrote:
>>> I think you're not verifying the SVT, SID and SQ fields in the IRTE.
>>
>> Exactly.
>>
>>>
>>> The source ID can be passed to the IOMMU using the MemTxAttrs mechanism.
>>
>> Ah, that's a nice new channel, resolving the need for using/passing
>> source-specific target memory regions for this. At least for PCI
>> devices, it should already be populated with the required information,
>> others (IOAPIC, HPET) probably require additional work to pass what I
>> defined as Q35_PSEUDO_BUS_PLATFORM, Q35_PSEUDO_DEVFN_IOAPIC/HPET.
> 
> Jan, Rita, Paolo, 
> 
> One more thing to ask: do we have any plan to move on the work to
> support vhost/pass-through devices? Do you think we need these too?

We need them eventually as well, but - unless some of them just work by
chance (unlikely) - that was not in the scope if the ongoing project,
rather a potential add-on for later.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Radim Krčmář]

2016-02-19 12:43+0100, Jan Kiszka:
> On 2016-02-19 12:34, Peter Xu wrote:
>> On Fri, Feb 19, 2016 at 10:58:12AM +0100, Paolo Bonzini wrote:
>>>
>>> The source ID can be passed to the IOMMU using the MemTxAttrs mechanism.
>> 
>> I see that MemTxAttrs is not enabled yet? It's dropped in all the
>> accessors like memory_region_write_accessor()?
> 
> That would be a pity. How much work remains to fix that? Putting Peter
> Maydell on CC who apparently started this extension.

memory_region_dispatch_write() uses write_with_attrs, so I think we only
need to use one for MSI region and add attributes to direct writers
(like HPET and IOAPIC, because normal PCI devices should be passing
correct attributes).

> ...
>>> FWIW, Radim was thinking of interrupt remapping in the kvm-ioapic, which
>>> we have decided to set aside.
> 
>> Does it mean that we are planning not to support kernel_irqchip for
>> IR? Any quick reason?
> 
> The reason to split up the kernel irqchip is to reduce code complexity
> and, thus, attack surface of the kernel-side interface towards the
> guest. So, extending the "classic" in-kernel support with yet another
> feature is not really supporting that goal.

A fear of regressions after forcing people to switch from kernel ioapic
to qemu ioapic is the only argument for in-kernel support.
We'd like to use split irqchip even without IR.

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

[Radim Krčmář]

2016-02-19 07:46+0100, Jan Kiszka:
> - Rita Sinha is currently working on integrating my old patches with the
> split-irqchip to get KVM working (as an Outreachy project). It's
> probably a bit unfortunate to consider a different horse that late in to
> project. What do you think, how could we benefit from each other?
> 
> - Radim was telling me to look on this as well. How do your efforts
> correlate?

I wasn't aware of Peter's project ...

Peter, my goal is to support x2APIC to get closer to supporting more
than 255 VCPUs.  Please Cc me in future discussions/patches!

Thanks.

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

[Jan Kiszka]

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

On 2016-02-19 10:29, Peter Xu wrote:
>> Would be helpful for more convenient import and experiments. E.g. I
>> could quickly through my test setup at them and tell you if they work
>> for it.
> 
> I have put codes onto github for better reference:
> 
> https://github.com/xzpeter/qemu/tree/vt-d-intr

Just a quick feedback: Interrupts of the root cell (the Linux that hands
over the control to the hypervisor) get stuck when starting Jailhouse.

What basically happens there is that Linux boots up with DMAR off but IR
on, then Jailhouse takes over, turning IR off for a moment, reconfigures
things so that it gains control, and then reenables IR (and also turns
on DMAR). After that, interrupts no longer arrive at that guest, which
is now L2.

Jan



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Marcel Apfelbaum]

On 02/19/2016 05:30 AM, Peter Xu wrote:
> 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,int_remap=on

Hi Peter,

Please be aware that there is an AMD IOMMU emulation series on the list,
so now we will have iommu=intel/amd/off.

As far as I know we prefer int-remap instead of int_remap and also
the "int" prefix reminds me integer, I would use ir=on or the clean interrupt-remapping=on.

>
> To be more clear, the following command:
>
> $ qemu-system-x86_64 -M q35,iommu=on
>
> Will enable Intel IOMMU only, without interrupt remapping support.

Since AMD IOMMU also supports interrupt remapping please sync with the other project.

>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   hw/core/machine.c             | 20 ++++++++++++++++++++
>   hw/pci-host/q35.c             |  6 ++++--
>   include/hw/boards.h           |  1 +
>   include/hw/i386/intel_iommu.h |  3 +++
>   4 files changed, 28 insertions(+), 2 deletions(-)
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 6d1a0d8..852cee2 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -298,6 +298,20 @@ static void machine_set_iommu(Object *obj, bool value, Error **errp)
>       ms->iommu = value;
>   }
>
> +static bool machine_get_int_remap(Object *obj, Error **errp)
> +{
> +    MachineState *ms = MACHINE(obj);
> +
> +    return ms->int_remap;
> +}

I am starting to wonder if "iommu" and now "interrupt-remapping" should be part
of machine and not the pc machine. Both implementations we have are only for the PC machines.

> +
> +static void machine_set_int_remap(Object *obj, bool value, Error **errp)
> +{
> +    MachineState *ms = MACHINE(obj);
> +
> +    ms->int_remap = value;
> +}
> +
>   static void machine_set_suppress_vmdesc(Object *obj, bool value, Error **errp)
>   {
>       MachineState *ms = MACHINE(obj);
> @@ -461,6 +475,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, "int-remap",
> +                             machine_get_int_remap,
> +                             machine_set_int_remap, NULL);
> +    object_property_set_description(obj, "int-remap",
> +                                    "Set on/off to enable/disable Intel IOMMU INTR",
> +                                    NULL);


Here the same, I would rename int-remap to interrupt-remapping and keep in mind
that would not be for Intel IOMMU only.

>       object_property_add_bool(obj, "suppress-vmdesc",
>                                machine_get_suppress_vmdesc,
>                                machine_set_suppress_vmdesc, NULL);
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index 115fb8c..7cd4d87 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -434,13 +434,14 @@ static AddressSpace *q35_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
>       return &vtd_as->as;
>   }
>
> -static void mch_init_dmar(MCHPCIState *mch)
> +static void mch_init_dmar(MCHPCIState *mch, bool intr_supported)
>   {
>       PCIBus *pci_bus = PCI_BUS(qdev_get_parent_bus(DEVICE(mch)));
>
>       mch->iommu = INTEL_IOMMU_DEVICE(qdev_create(NULL, TYPE_INTEL_IOMMU_DEVICE));
>       object_property_add_child(OBJECT(mch), "intel-iommu",
>                                 OBJECT(mch->iommu), NULL);
> +    mch->iommu->intr_supported = intr_supported;
>       qdev_init_nofail(DEVICE(mch->iommu));
>       sysbus_mmio_map(SYS_BUS_DEVICE(mch->iommu), 0, Q35_HOST_BRIDGE_IOMMU_ADDR);
>
> @@ -507,7 +508,8 @@ static void mch_realize(PCIDevice *d, Error **errp)
>       }
>       /* Intel IOMMU (VT-d) */
>       if (object_property_get_bool(qdev_get_machine(), "iommu", NULL)) {
> -        mch_init_dmar(mch);
> +        mch_init_dmar(mch, object_property_get_bool(qdev_get_machine(),
> +                                                    "int-remap", false));


Here you can directly call qdev_get_machine()->interrupt-remapping instead of object_property_get_bool.


Thanks,
Marcel

>       }
>   }
>
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 0f30959..d488e40 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -127,6 +127,7 @@ struct MachineState {
>       bool igd_gfx_passthru;
>       char *firmware;
>       bool iommu;
> +    bool int_remap;
>       bool suppress_vmdesc;
>
>       ram_addr_t ram_size;
> diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
> index b024ffa..6e52c6b 100644
> --- a/include/hw/i386/intel_iommu.h
> +++ b/include/hw/i386/intel_iommu.h
> @@ -123,6 +123,9 @@ 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_supported;            /* Whether IR is supported */
>   };
>
>   /* Find the VTD Address space associated with the given bus pointer,
>

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

[Marcel Apfelbaum]

On 02/19/2016 05:30 AM, Peter Xu wrote:
> In ACPI DMA remapping report structure, enable INTR flag when specified.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   hw/i386/acpi-build.c          | 31 ++++++++++++++++++++++++-------
>   include/hw/i386/intel_iommu.h |  2 ++
>   2 files changed, 26 insertions(+), 7 deletions(-)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 4554eb8..d9e4f91 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2489,6 +2489,19 @@ build_mcfg_q35(GArray *table_data, GArray *linker, AcpiMcfgInfo *info)
>       build_header(linker, table_data, (void *)mcfg, sig, len, 1, NULL, NULL);
>   }
>
> +static IntelIOMMUState *acpi_get_iommu(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;
> +}
> +
>   static void
>   build_dmar_q35(GArray *table_data, GArray *linker)
>   {
> @@ -2496,10 +2509,19 @@ build_dmar_q35(GArray *table_data, GArray *linker)
>
>       AcpiTableDmar *dmar;
>       AcpiDmarHardwareUnit *drhd;
> +    uint8_t dmar_flags = 0;
> +    IntelIOMMUState *intel_iommu = acpi_get_iommu();
> +
> +    assert(intel_iommu);
> +
> +    if (intel_iommu->intr_supported) {

Hi,

It seems intr_supported duplicates the same field you have in machine.
You can pass the machine to build_dmar_q35 and get rid of the extra field.

Thanks,
Marcel


> +        /* 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));
> @@ -2572,12 +2594,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 !!acpi_get_iommu();
>   }
>
>   static bool acpi_has_nvdimm(void)
> diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
> index 6e52c6b..83e5a1e 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;
>

Re: [Qemu-devel] [PATCH 05/13] acpi: add DMAR scope definition for root IOAPIC

[Marcel Apfelbaum]

On 02/19/2016 05:30 AM, Peter Xu wrote:
> 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 d9e4f91..1cefe43 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -76,6 +76,9 @@
>   #define ACPI_BUILD_DPRINTF(fmt, ...)
>   #endif
>
> +/* Default IOAPIC ID */
> +#define ACPI_BUILD_IOAPIC_ID 0x0
> +
>   typedef struct AcpiCpuInfo {
>       DECLARE_BITMAP(found_cpus, ACPI_CPU_HOTPLUG_ID_LIMIT);
>   } AcpiCpuInfo;
> @@ -392,7 +395,6 @@ build_madt(GArray *table_data, GArray *linker, AcpiCpuInfo *cpu)
>       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);
> @@ -2511,6 +2513,9 @@ build_dmar_q35(GArray *table_data, GArray *linker)
>       AcpiDmarHardwareUnit *drhd;
>       uint8_t dmar_flags = 0;
>       IntelIOMMUState *intel_iommu = acpi_get_iommu();
> +    AcpiDmarDeviceScope *scope = NULL;
> +    /* Root complex IOAPIC use one path[0] only */
> +    uint16_t scope_size = sizeof(*scope) + sizeof(uint16_t);
>
>       assert(intel_iommu);
>
> @@ -2526,11 +2531,25 @@ build_dmar_q35(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         (0xf0)
> +#define ACPI_IOAPIC_DEVFN_IR       (0x00)

Hi,

How do you know for sure there is no bus (or bus & device) having the number 0xf0 in the system?
Now that we support multiple Root Complexes, using the  pxb-pcie device we can simply add a PCI root bus like this:
     -device pxb-pcie,bus_nr=0xf0
or we can add enough switches to get to this number.

You could dynamically query for an unused PCI bus, but the number would change between the runs.
Or, I suppose you can reserve a slot on bus 0 for that. It is interesting how it works on a real machine.

Thanks,
Marcel


> +    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;
>
>

Re: [Qemu-devel] [PATCH 05/13] acpi: add DMAR scope definition for root IOAPIC

[Marcel Apfelbaum]

On 02/21/2016 01:38 PM, Marcel Apfelbaum wrote:
> On 02/19/2016 05:30 AM, Peter Xu wrote:
>> 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 d9e4f91..1cefe43 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -76,6 +76,9 @@
>>   #define ACPI_BUILD_DPRINTF(fmt, ...)
>>   #endif
>>
>> +/* Default IOAPIC ID */
>> +#define ACPI_BUILD_IOAPIC_ID 0x0
>> +
>>   typedef struct AcpiCpuInfo {
>>       DECLARE_BITMAP(found_cpus, ACPI_CPU_HOTPLUG_ID_LIMIT);
>>   } AcpiCpuInfo;
>> @@ -392,7 +395,6 @@ build_madt(GArray *table_data, GArray *linker, AcpiCpuInfo *cpu)
>>       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);
>> @@ -2511,6 +2513,9 @@ build_dmar_q35(GArray *table_data, GArray *linker)
>>       AcpiDmarHardwareUnit *drhd;
>>       uint8_t dmar_flags = 0;
>>       IntelIOMMUState *intel_iommu = acpi_get_iommu();
>> +    AcpiDmarDeviceScope *scope = NULL;
>> +    /* Root complex IOAPIC use one path[0] only */
>> +    uint16_t scope_size = sizeof(*scope) + sizeof(uint16_t);
>>
>>       assert(intel_iommu);
>>
>> @@ -2526,11 +2531,25 @@ build_dmar_q35(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         (0xf0)
>> +#define ACPI_IOAPIC_DEVFN_IR       (0x00)
>
> Hi,
>
> How do you know for sure there is no bus (or bus & device) having the number 0xf0 in the system?
> Now that we support multiple Root Complexes, using the  pxb-pcie device we can simply add a PCI root bus like this:
>      -device pxb-pcie,bus_nr=0xf0
> or we can add enough switches to get to this number.
>
> You could dynamically query for an unused PCI bus, but the number would change between the runs.
> Or, I suppose you can reserve a slot on bus 0 for that. It is interesting how it works on a real machine.

thinking about it more, maybe we should let the firmware to assign the bus/dev/fun for the IO APIC?

Thanks,
Marcel

>
> Thanks,
> Marcel
>
>
>> +    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;
>>
>>
>

Re: [Qemu-devel] [PATCH 05/13] acpi: add DMAR scope definition for root IOAPIC

[Jan Kiszka]

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

On 2016-02-21 13:08, Marcel Apfelbaum wrote:
> On 02/21/2016 01:38 PM, Marcel Apfelbaum wrote:
>> On 02/19/2016 05:30 AM, Peter Xu wrote:
>>> 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 d9e4f91..1cefe43 100644
>>> --- a/hw/i386/acpi-build.c
>>> +++ b/hw/i386/acpi-build.c
>>> @@ -76,6 +76,9 @@
>>>   #define ACPI_BUILD_DPRINTF(fmt, ...)
>>>   #endif
>>>
>>> +/* Default IOAPIC ID */
>>> +#define ACPI_BUILD_IOAPIC_ID 0x0
>>> +
>>>   typedef struct AcpiCpuInfo {
>>>       DECLARE_BITMAP(found_cpus, ACPI_CPU_HOTPLUG_ID_LIMIT);
>>>   } AcpiCpuInfo;
>>> @@ -392,7 +395,6 @@ build_madt(GArray *table_data, GArray *linker,
>>> AcpiCpuInfo *cpu)
>>>       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);
>>> @@ -2511,6 +2513,9 @@ build_dmar_q35(GArray *table_data, GArray *linker)
>>>       AcpiDmarHardwareUnit *drhd;
>>>       uint8_t dmar_flags = 0;
>>>       IntelIOMMUState *intel_iommu = acpi_get_iommu();
>>> +    AcpiDmarDeviceScope *scope = NULL;
>>> +    /* Root complex IOAPIC use one path[0] only */
>>> +    uint16_t scope_size = sizeof(*scope) + sizeof(uint16_t);
>>>
>>>       assert(intel_iommu);
>>>
>>> @@ -2526,11 +2531,25 @@ build_dmar_q35(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         (0xf0)
>>> +#define ACPI_IOAPIC_DEVFN_IR       (0x00)
>>
>> Hi,
>>
>> How do you know for sure there is no bus (or bus & device) having the
>> number 0xf0 in the system?
>> Now that we support multiple Root Complexes, using the  pxb-pcie
>> device we can simply add a PCI root bus like this:
>>      -device pxb-pcie,bus_nr=0xf0
>> or we can add enough switches to get to this number.
>>
>> You could dynamically query for an unused PCI bus, but the number
>> would change between the runs.
>> Or, I suppose you can reserve a slot on bus 0 for that. It is
>> interesting how it works on a real machine.
> 
> thinking about it more, maybe we should let the firmware to assign the
> bus/dev/fun for the IO APIC?

We have the same problem over with VT-d and IR.

I don't think the firmware is not the right place, otherwise there would
be an interface in hw to adjust that parameters. I think we should
simply make sure that the qemu user cannot assign devices to those
addresses as they are reserved for the platform devices (the HPET
requires another ID), or even reserve the hole bus for the platform.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [PATCH 05/13] acpi: add DMAR scope definition for root IOAPIC

[Marcel Apfelbaum]

On 02/21/2016 03:40 PM, Jan Kiszka wrote:
> On 2016-02-21 13:08, Marcel Apfelbaum wrote:
>> On 02/21/2016 01:38 PM, Marcel Apfelbaum wrote:
>>> On 02/19/2016 05:30 AM, Peter Xu wrote:
>>>> 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 d9e4f91..1cefe43 100644
>>>> --- a/hw/i386/acpi-build.c
>>>> +++ b/hw/i386/acpi-build.c
>>>> @@ -76,6 +76,9 @@
>>>>    #define ACPI_BUILD_DPRINTF(fmt, ...)
>>>>    #endif
>>>>
>>>> +/* Default IOAPIC ID */
>>>> +#define ACPI_BUILD_IOAPIC_ID 0x0
>>>> +
>>>>    typedef struct AcpiCpuInfo {
>>>>        DECLARE_BITMAP(found_cpus, ACPI_CPU_HOTPLUG_ID_LIMIT);
>>>>    } AcpiCpuInfo;
>>>> @@ -392,7 +395,6 @@ build_madt(GArray *table_data, GArray *linker,
>>>> AcpiCpuInfo *cpu)
>>>>        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);
>>>> @@ -2511,6 +2513,9 @@ build_dmar_q35(GArray *table_data, GArray *linker)
>>>>        AcpiDmarHardwareUnit *drhd;
>>>>        uint8_t dmar_flags = 0;
>>>>        IntelIOMMUState *intel_iommu = acpi_get_iommu();
>>>> +    AcpiDmarDeviceScope *scope = NULL;
>>>> +    /* Root complex IOAPIC use one path[0] only */
>>>> +    uint16_t scope_size = sizeof(*scope) + sizeof(uint16_t);
>>>>
>>>>        assert(intel_iommu);
>>>>
>>>> @@ -2526,11 +2531,25 @@ build_dmar_q35(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         (0xf0)
>>>> +#define ACPI_IOAPIC_DEVFN_IR       (0x00)
>>>
>>> Hi,
>>>
>>> How do you know for sure there is no bus (or bus & device) having the
>>> number 0xf0 in the system?
>>> Now that we support multiple Root Complexes, using the  pxb-pcie
>>> device we can simply add a PCI root bus like this:
>>>       -device pxb-pcie,bus_nr=0xf0
>>> or we can add enough switches to get to this number.
>>>
>>> You could dynamically query for an unused PCI bus, but the number
>>> would change between the runs.
>>> Or, I suppose you can reserve a slot on bus 0 for that. It is
>>> interesting how it works on a real machine.
>>
>> thinking about it more, maybe we should let the firmware to assign the
>> bus/dev/fun for the IO APIC?
>
> We have the same problem over with VT-d and IR.
>
> I don't think the firmware is not the right place, otherwise there would
> be an interface in hw to adjust that parameters. I think we should
> simply make sure that the qemu user cannot assign devices to those
> addresses as they are reserved for the platform devices (the HPET
> requires another ID), or even reserve the hole bus for the platform.


I understand, but it is the firmware that assign addresses, not QEMU/user.
We need a way to tell the firmware not to use a certain address range, we can do that
with fw_config I suppose.

Reserving a bus might be easier, we take the bus number out from host bridges CRS
ranges and each platform device can be assigned a slot. We would need to select the bus carefully
to not collide with other PCI hierarchies. Maybe bus 0xFF.

Thanks,
Marcel

>
> Jan
>

Re: [Qemu-devel] [PATCH 11/13] intel_iommu: add IR translation faults defines

[Marcel Apfelbaum]

On 02/19/2016 05:30 AM, Peter Xu wrote:
> 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 | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
>
> diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
> index 309833f..c66cb83 100644
> --- a/hw/i386/intel_iommu_internal.h
> +++ b/hw/i386/intel_iommu_internal.h
> @@ -271,6 +271,21 @@ 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 resved
> +                                * fields set */

Hi,

Minor comments:
resved -> reserved

I would keep comments single line when possible
even on top of the code line.


Thanks,
Marcel

> +    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.
>

Re: [Qemu-devel] [PATCH 05/13] acpi: add DMAR scope definition for root IOAPIC

[Jan Kiszka]

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

On 2016-02-21 16:54, Marcel Apfelbaum wrote:
> On 02/21/2016 03:40 PM, Jan Kiszka wrote:
>> On 2016-02-21 13:08, Marcel Apfelbaum wrote:
>>> On 02/21/2016 01:38 PM, Marcel Apfelbaum wrote:
>>>> On 02/19/2016 05:30 AM, Peter Xu wrote:
>>>>> 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 d9e4f91..1cefe43 100644
>>>>> --- a/hw/i386/acpi-build.c
>>>>> +++ b/hw/i386/acpi-build.c
>>>>> @@ -76,6 +76,9 @@
>>>>>    #define ACPI_BUILD_DPRINTF(fmt, ...)
>>>>>    #endif
>>>>>
>>>>> +/* Default IOAPIC ID */
>>>>> +#define ACPI_BUILD_IOAPIC_ID 0x0
>>>>> +
>>>>>    typedef struct AcpiCpuInfo {
>>>>>        DECLARE_BITMAP(found_cpus, ACPI_CPU_HOTPLUG_ID_LIMIT);
>>>>>    } AcpiCpuInfo;
>>>>> @@ -392,7 +395,6 @@ build_madt(GArray *table_data, GArray *linker,
>>>>> AcpiCpuInfo *cpu)
>>>>>        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);
>>>>> @@ -2511,6 +2513,9 @@ build_dmar_q35(GArray *table_data, GArray
>>>>> *linker)
>>>>>        AcpiDmarHardwareUnit *drhd;
>>>>>        uint8_t dmar_flags = 0;
>>>>>        IntelIOMMUState *intel_iommu = acpi_get_iommu();
>>>>> +    AcpiDmarDeviceScope *scope = NULL;
>>>>> +    /* Root complex IOAPIC use one path[0] only */
>>>>> +    uint16_t scope_size = sizeof(*scope) + sizeof(uint16_t);
>>>>>
>>>>>        assert(intel_iommu);
>>>>>
>>>>> @@ -2526,11 +2531,25 @@ build_dmar_q35(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         (0xf0)
>>>>> +#define ACPI_IOAPIC_DEVFN_IR       (0x00)
>>>>
>>>> Hi,
>>>>
>>>> How do you know for sure there is no bus (or bus & device) having the
>>>> number 0xf0 in the system?
>>>> Now that we support multiple Root Complexes, using the  pxb-pcie
>>>> device we can simply add a PCI root bus like this:
>>>>       -device pxb-pcie,bus_nr=0xf0
>>>> or we can add enough switches to get to this number.
>>>>
>>>> You could dynamically query for an unused PCI bus, but the number
>>>> would change between the runs.
>>>> Or, I suppose you can reserve a slot on bus 0 for that. It is
>>>> interesting how it works on a real machine.
>>>
>>> thinking about it more, maybe we should let the firmware to assign the
>>> bus/dev/fun for the IO APIC?
>>
>> We have the same problem over with VT-d and IR.
>>
>> I don't think the firmware is not the right place, otherwise there would
>> be an interface in hw to adjust that parameters. I think we should
>> simply make sure that the qemu user cannot assign devices to those
>> addresses as they are reserved for the platform devices (the HPET
>> requires another ID), or even reserve the hole bus for the platform.
> 
> 
> I understand, but it is the firmware that assign addresses, not QEMU/user.

Right, buses are chosen by the firmware, but device address are
(optionally) under user control.

> We need a way to tell the firmware not to use a certain address range,
> we can do that
> with fw_config I suppose.

The pseudo addresses of IOAPIC and HPET are part of ACPI tables, both on
AMD and Intel. Firmware can evaluate them easily.

> 
> Reserving a bus might be easier, we take the bus number out from host
> bridges CRS
> ranges and each platform device can be assigned a slot. We would need to
> select the bus carefully
> to not collide with other PCI hierarchies. Maybe bus 0xFF.

Yes, that's the bus number I once chose for Intel / Q35.

Jan



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Peter Xu]

On Sun, Feb 21, 2016 at 12:38:38PM +0200, Marcel Apfelbaum wrote:
> On 02/19/2016 05:30 AM, Peter Xu wrote:
> >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,int_remap=on
> 
> Hi Peter,
> 
> Please be aware that there is an AMD IOMMU emulation series on the list,
> so now we will have iommu=intel/amd/off.
> 
> As far as I know we prefer int-remap instead of int_remap and also
> the "int" prefix reminds me integer, I would use ir=on or the clean interrupt-remapping=on.

Hi, Marcel,

Really appreciated for all of your review comments for the series!

As I worked on this without noticing that Jan/Rita is working on
this too, I may need to halt here and wait for her patches. So
please ignore this patchset for now, and let's all wait for Rita's
first version.

Sorry for wasting your time on this!

Thanks!
Peter

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

[Peter Xu]

On Fri, Feb 19, 2016 at 05:38:48PM +0100, Radim Krčmář wrote:
> 2016-02-19 07:46+0100, Jan Kiszka:
> > - Rita Sinha is currently working on integrating my old patches with the
> > split-irqchip to get KVM working (as an Outreachy project). It's
> > probably a bit unfortunate to consider a different horse that late in to
> > project. What do you think, how could we benefit from each other?
> > 
> > - Radim was telling me to look on this as well. How do your efforts
> > correlate?
> 
> I wasn't aware of Peter's project ...
> 
> Peter, my goal is to support x2APIC to get closer to supporting more
> than 255 VCPUs.  Please Cc me in future discussions/patches!

Sure!

Peter

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

[Marcel Apfelbaum]

On 02/23/2016 05:48 AM, Peter Xu wrote:
> On Sun, Feb 21, 2016 at 12:38:38PM +0200, Marcel Apfelbaum wrote:
>> On 02/19/2016 05:30 AM, Peter Xu wrote:
>>> 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,int_remap=on
>>
>> Hi Peter,
>>
>> Please be aware that there is an AMD IOMMU emulation series on the list,
>> so now we will have iommu=intel/amd/off.
>>
>> As far as I know we prefer int-remap instead of int_remap and also
>> the "int" prefix reminds me integer, I would use ir=on or the clean interrupt-remapping=on.
>
> Hi, Marcel,
>
> Really appreciated for all of your review comments for the series!
>
> As I worked on this without noticing that Jan/Rita is working on
> this too, I may need to halt here and wait for her patches. So
> please ignore this patchset for now, and let's all wait for Rita's
> first version.
>

Hi Peter,

> Sorry for wasting your time on this!


No problem, really, it happens :)
Now you can much easier review their work and improve it as you see fit.

Good luck,
Marcel

>
> Thanks!
> Peter
>

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

[Peter Xu]

It's a long time from previous post... However I will start to pick
this up. Several questions on the comments...

On Sun, Feb 21, 2016 at 12:38:38PM +0200, Marcel Apfelbaum wrote:
> On 02/19/2016 05:30 AM, Peter Xu wrote:
> >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,int_remap=on
> 
> Hi Peter,
> 
> Please be aware that there is an AMD IOMMU emulation series on the list,
> so now we will have iommu=intel/amd/off.

I had a look on the AMD series. I see that x-iommu-type is
introduced to specify AMD IOMMUs. So maybe I should keep this
interface unchanged for now?

> 
> As far as I know we prefer int-remap instead of int_remap and also
> the "int" prefix reminds me integer, I would use ir=on or the clean interrupt-remapping=on.

I suppose "ir" is a little bit hard for people to think about
interrupt remaping, and "interrupt-remapping" might be too long. If
you would not mind, I'd like to use "intr" in next version.

[...]

> >+static bool machine_get_int_remap(Object *obj, Error **errp)
> >+{
> >+    MachineState *ms = MACHINE(obj);
> >+
> >+    return ms->int_remap;
> >+}
> 
> I am starting to wonder if "iommu" and now "interrupt-remapping" should be part
> of machine and not the pc machine. Both implementations we have are only for the PC machines.

Do you mean that "both implementation we have are for the machines"
rather than PC machines? It seems that both of us are modifying
machine_initfn() rather than pc_machine_initfn()? Am I missing
anything?

> 
> >+
> >+static void machine_set_int_remap(Object *obj, bool value, Error **errp)
> >+{
> >+    MachineState *ms = MACHINE(obj);
> >+
> >+    ms->int_remap = value;
> >+}
> >+
> >  static void machine_set_suppress_vmdesc(Object *obj, bool value, Error **errp)
> >  {
> >      MachineState *ms = MACHINE(obj);
> >@@ -461,6 +475,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, "int-remap",
> >+                             machine_get_int_remap,
> >+                             machine_set_int_remap, NULL);
> >+    object_property_set_description(obj, "int-remap",
> >+                                    "Set on/off to enable/disable Intel IOMMU INTR",
> >+                                    NULL);
> 
> 
> Here the same, I would rename int-remap to interrupt-remapping and keep in mind
> that would not be for Intel IOMMU only.

Will keep that in mind. Thanks.

However, we do not need to specify the type here, right? Since we
can specify the type of IOMMU via x-iommu-type, and the type of IR
will always follow the type of IOMMU.

> 
> >      object_property_add_bool(obj, "suppress-vmdesc",
> >                               machine_get_suppress_vmdesc,
> >                               machine_set_suppress_vmdesc, NULL);
> >diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> >index 115fb8c..7cd4d87 100644
> >--- a/hw/pci-host/q35.c
> >+++ b/hw/pci-host/q35.c
> >@@ -434,13 +434,14 @@ static AddressSpace *q35_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
> >      return &vtd_as->as;
> >  }
> >
> >-static void mch_init_dmar(MCHPCIState *mch)
> >+static void mch_init_dmar(MCHPCIState *mch, bool intr_supported)
> >  {
> >      PCIBus *pci_bus = PCI_BUS(qdev_get_parent_bus(DEVICE(mch)));
> >
> >      mch->iommu = INTEL_IOMMU_DEVICE(qdev_create(NULL, TYPE_INTEL_IOMMU_DEVICE));
> >      object_property_add_child(OBJECT(mch), "intel-iommu",
> >                                OBJECT(mch->iommu), NULL);
> >+    mch->iommu->intr_supported = intr_supported;
> >      qdev_init_nofail(DEVICE(mch->iommu));
> >      sysbus_mmio_map(SYS_BUS_DEVICE(mch->iommu), 0, Q35_HOST_BRIDGE_IOMMU_ADDR);
> >
> >@@ -507,7 +508,8 @@ static void mch_realize(PCIDevice *d, Error **errp)
> >      }
> >      /* Intel IOMMU (VT-d) */
> >      if (object_property_get_bool(qdev_get_machine(), "iommu", NULL)) {
> >-        mch_init_dmar(mch);
> >+        mch_init_dmar(mch, object_property_get_bool(qdev_get_machine(),
> >+                                                    "int-remap", false));
> 
> 
> Here you can directly call qdev_get_machine()->interrupt-remapping instead of object_property_get_bool.

Will do.

Thanks!

-- peterx

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

[Peter Xu]

On Sun, Feb 21, 2016 at 01:05:24PM +0200, Marcel Apfelbaum wrote:
[...]
> >  static void
> >  build_dmar_q35(GArray *table_data, GArray *linker)
> >  {
> >@@ -2496,10 +2509,19 @@ build_dmar_q35(GArray *table_data, GArray *linker)
> >
> >      AcpiTableDmar *dmar;
> >      AcpiDmarHardwareUnit *drhd;
> >+    uint8_t dmar_flags = 0;
> >+    IntelIOMMUState *intel_iommu = acpi_get_iommu();
> >+
> >+    assert(intel_iommu);
> >+
> >+    if (intel_iommu->intr_supported) {
> 
> Hi,
> 
> It seems intr_supported duplicates the same field you have in machine.
> You can pass the machine to build_dmar_q35 and get rid of the extra field.

You are right.  Will drop intr_supported in all the patches.

Thanks.

-- peterx

Re: [Qemu-devel] [PATCH 05/13] acpi: add DMAR scope definition for root IOAPIC

[Peter Xu]

On Sun, Feb 21, 2016 at 05:01:02PM +0100, Jan Kiszka wrote:
> >> We have the same problem over with VT-d and IR.
> >>
> >> I don't think the firmware is not the right place, otherwise there would
> >> be an interface in hw to adjust that parameters. I think we should
> >> simply make sure that the qemu user cannot assign devices to those
> >> addresses as they are reserved for the platform devices (the HPET
> >> requires another ID), or even reserve the hole bus for the platform.
> > 
> > 
> > I understand, but it is the firmware that assign addresses, not QEMU/user.
> 
> Right, buses are chosen by the firmware, but device address are
> (optionally) under user control.
> 
> > We need a way to tell the firmware not to use a certain address range,
> > we can do that
> > with fw_config I suppose.
> 
> The pseudo addresses of IOAPIC and HPET are part of ACPI tables, both on
> AMD and Intel. Firmware can evaluate them easily.

Hi, Jan, Marcel,

Could you give me some hint on how to achieve this using fw_config
(restriction on PCI bus numbers)?  I failed to find it out myself.

Thanks in advance.

-- peterx

Re: [Qemu-devel] [PATCH 11/13] intel_iommu: add IR translation faults defines

[Peter Xu]

On Sun, Feb 21, 2016 at 05:56:07PM +0200, Marcel Apfelbaum wrote:
> >+
> >+    /*
> >+     * Interrupt remapping transition faults
> >+     */
> >+    VTD_FR_IR_REQ_RSVD = 0x20, /* One or more IR request resved
> >+                                * fields set */
> 
> Hi,
> 
> Minor comments:
> resved -> reserved
> 
> I would keep comments single line when possible
> even on top of the code line.

Will fix these in next version.

Thanks.

-- peterx

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

[Marcel Apfelbaum]

On 04/08/2016 10:30 AM, Peter Xu wrote:
> It's a long time from previous post... However I will start to pick
> this up. Several questions on the comments...
>
> On Sun, Feb 21, 2016 at 12:38:38PM +0200, Marcel Apfelbaum wrote:
>> On 02/19/2016 05:30 AM, Peter Xu wrote:
>>> 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,int_remap=on
>>
>> Hi Peter,
>>
>> Please be aware that there is an AMD IOMMU emulation series on the list,
>> so now we will have iommu=intel/amd/off.
>

Hi Peter,

> I had a look on the AMD series. I see that x-iommu-type is
> introduced to specify AMD IOMMUs. So maybe I should keep this
> interface unchanged for now?
>

I agree

>>
>> As far as I know we prefer int-remap instead of int_remap and also
>> the "int" prefix reminds me integer, I would use ir=on or the clean interrupt-remapping=on.
>
> I suppose "ir" is a little bit hard for people to think about
> interrupt remaping, and "interrupt-remapping" might be too long. If
> you would not mind, I'd like to use "intr" in next version.
>

I would go with "interrupt-remapping", but is only me. If "intr"
is OK for the others developers, I would be OK with it.

> [...]
>
>>> +static bool machine_get_int_remap(Object *obj, Error **errp)
>>> +{
>>> +    MachineState *ms = MACHINE(obj);
>>> +
>>> +    return ms->int_remap;
>>> +}
>>
>> I am starting to wonder if "iommu" and now "interrupt-remapping" should be part
>> of machine and not the pc machine. Both implementations we have are only for the PC machines.
>
> Do you mean that "both implementation we have are for the machines"
> rather than PC machines? It seems that both of us are modifying
> machine_initfn() rather than pc_machine_initfn()? Am I missing
> anything?
>

I meant that IOMMU implementation should be in PC Machines, and not in the base Machine class
because the only implementations we have are for x86. Not all architectures support IOMMU.
But it is out of the scope of this series, I think. It is only a thought :)

>>
>>> +
>>> +static void machine_set_int_remap(Object *obj, bool value, Error **errp)
>>> +{
>>> +    MachineState *ms = MACHINE(obj);
>>> +
>>> +    ms->int_remap = value;
>>> +}
>>> +
>>>   static void machine_set_suppress_vmdesc(Object *obj, bool value, Error **errp)
>>>   {
>>>       MachineState *ms = MACHINE(obj);
>>> @@ -461,6 +475,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, "int-remap",
>>> +                             machine_get_int_remap,
>>> +                             machine_set_int_remap, NULL);
>>> +    object_property_set_description(obj, "int-remap",
>>> +                                    "Set on/off to enable/disable Intel IOMMU INTR",
>>> +                                    NULL);
>>
>>
>> Here the same, I would rename int-remap to interrupt-remapping and keep in mind
>> that would not be for Intel IOMMU only.
>
> Will keep that in mind. Thanks.
>
> However, we do not need to specify the type here, right? Since we
> can specify the type of IOMMU via x-iommu-type, and the type of IR
> will always follow the type of IOMMU.
>

right


Thanks,
Marcel

>>
>>>       object_property_add_bool(obj, "suppress-vmdesc",
>>>                                machine_get_suppress_vmdesc,
>>>                                machine_set_suppress_vmdesc, NULL);
>>> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
>>> index 115fb8c..7cd4d87 100644
>>> --- a/hw/pci-host/q35.c
>>> +++ b/hw/pci-host/q35.c
>>> @@ -434,13 +434,14 @@ static AddressSpace *q35_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
>>>       return &vtd_as->as;
>>>   }
>>>
>>> -static void mch_init_dmar(MCHPCIState *mch)
>>> +static void mch_init_dmar(MCHPCIState *mch, bool intr_supported)
>>>   {
>>>       PCIBus *pci_bus = PCI_BUS(qdev_get_parent_bus(DEVICE(mch)));
>>>
>>>       mch->iommu = INTEL_IOMMU_DEVICE(qdev_create(NULL, TYPE_INTEL_IOMMU_DEVICE));
>>>       object_property_add_child(OBJECT(mch), "intel-iommu",
>>>                                 OBJECT(mch->iommu), NULL);
>>> +    mch->iommu->intr_supported = intr_supported;
>>>       qdev_init_nofail(DEVICE(mch->iommu));
>>>       sysbus_mmio_map(SYS_BUS_DEVICE(mch->iommu), 0, Q35_HOST_BRIDGE_IOMMU_ADDR);
>>>
>>> @@ -507,7 +508,8 @@ static void mch_realize(PCIDevice *d, Error **errp)
>>>       }
>>>       /* Intel IOMMU (VT-d) */
>>>       if (object_property_get_bool(qdev_get_machine(), "iommu", NULL)) {
>>> -        mch_init_dmar(mch);
>>> +        mch_init_dmar(mch, object_property_get_bool(qdev_get_machine(),
>>> +                                                    "int-remap", false));
>>
>>
>> Here you can directly call qdev_get_machine()->interrupt-remapping instead of object_property_get_bool.
>
> Will do.
>
> Thanks!
>
> -- peterx
>