[Qemu-devel] [V1 0/4] AMD-Vi Interrupt Remapping

[Qemu-devel] [V1 0/4] AMD-Vi Interrupt Remapping

[David Kiarie]

Hello all,

The following patchset adds AMD-Vi interrupt remapping logic
to Qemu and hooks it onto existing interrupt remapping infrastructure.It has 
a dependency on "Explicit SID for IOAPIC" patchset though.

I would appreciate your feedback!

For quick testing https://github.com/aslaq/qemu IR

David Kiarie (4):
  hw/iommu: Prepare for AMD IOMMU interrupt remapping
  hw/iommu: AMD IOMMU interrupt remapping
  hw/acpi: report IOAPIC on IVRS
  hw/iommu: share common between IOMMUs

 hw/i386/acpi-build.c  |   2 +
 hw/i386/amd_iommu.c   | 226 +++++++++++++++++++++++++++++++++++++++++++++++++-
 hw/i386/amd_iommu.h   |  74 +++++++++++++++++
 hw/i386/intel_iommu.c |   9 --
 hw/i386/trace-events  |   7 ++
 hw/i386/x86-iommu.c   |   8 ++
 6 files changed, 316 insertions(+), 10 deletions(-)

-- 
2.1.4

[Qemu-devel] [V1 1/4] hw/iommu: Prepare for AMD IOMMU interrupt remapping

[David Kiarie]

Introduce macros and trace events for use in AMD IOMMU
interrupt remapping

Signed-off-by: David Kiarie <davidkiarie4@gmail.com>
---
 hw/i386/amd_iommu.h  | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/i386/trace-events |  7 +++++
 2 files changed, 79 insertions(+)

diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
index 5d0b91d..2a7f19e 100644
--- a/hw/i386/amd_iommu.h
+++ b/hw/i386/amd_iommu.h
@@ -177,6 +177,78 @@
 #define AMDVI_IOTLB_MAX_SIZE 1024
 #define AMDVI_DEVID_SHIFT    36
 
+/* interrupt types */
+#define AMDVI_MT_FIXED  0x0
+#define AMDVI_MT_ARBIT  0x1
+#define AMDVI_MT_SMI    0x2
+#define AMDVI_MT_NMI    0x3
+#define AMDVI_MT_INIT   0x4
+#define AMDVI_MT_EXTINT 0x6
+#define AMDVI_MT_LINT1  0xb
+#define AMDVI_MT_LINT0  0xe
+
+/* Ext reg, GA support */
+#define AMDVI_GASUP    (1UL << 7)
+/* MMIO control GA enable bits */
+#define AMDVI_GAEN     (1UL << 17)
+
+/* MSI interrupt type mask */
+#define AMDVI_IR_TYPE_MASK 0x300
+
+/* interrupt destination mode */
+#define AMDVI_IRDEST_MODE_MASK 0x2
+
+/* select MSI data 10:0 bits */
+#define AMDVI_IRTE_INDEX_MASK 0x7ff
+
+/* bits determining whether specific interrupts should be passed
+ * split DTE into 64-bit chunks
+ */
+#define AMDVI_DTE_INTPASS       56
+#define AMDVI_DTE_EINTPASS      57
+#define AMDVI_DTE_NMIPASS       58
+#define AMDVI_DTE_INTCTL        60
+#define AMDVI_DTE_LINT0PASS     62
+#define AMDVI_DTE_LINT1PASS     63
+
+/* interrupt data valid */
+#define AMDVI_IR_VALID          (1UL << 0)
+
+/* interrupt root table mask */
+#define AMDVI_IRTEROOT_MASK     0xffffffffffffc0
+
+/* default IRTE size */
+#define AMDVI_DEFAULT_IRTE_SIZE 0x4
+
+/* IRTE size with GASup enabled */
+#define AMDVI_IRTE_SIZE_GASUP   0x10
+
+#define AMDVI_IRTE_VECTOR_MASK    (0xffU << 16)
+#define AMDVI_IRTE_DEST_MASK      (0xffU << 8)
+#define AMDVI_IRTE_DM_MASK        (0x1U << 6)
+#define AMDVI_IRTE_RQEOI_MASK     (0x1U << 5)
+#define AMDVI_IRTE_INTTYPE_MASK   (0x7U << 2)
+#define AMDVI_IRTE_SUPIOPF_MASK   (0x1U << 1)
+#define AMDVI_IRTE_REMAP_MASK     (0x1U << 0)
+
+#define AMDVI_IR_TABLE_SIZE_MASK 0xfe
+
+/* offsets into MSI data */
+#define AMDVI_MSI_DATA_DM_RSHIFT       0x8
+#define AMDVI_MSI_DATA_LEVEL_RSHIFT    0xe
+#define AMDVI_MSI_DATA_TRM_RSHIFT      0xf
+
+/* offsets into MSI address */
+#define AMDVI_MSI_ADDR_DM_RSHIFT       0x2
+#define AMDVI_MSI_ADDR_RH_RSHIFT       0x3
+#define AMDVI_MSI_ADDR_DEST_RSHIFT     0xc
+
+#define AMDVI_BUS_NUM                  0x0
+/* AMD-Vi specific IOAPIC Device function */
+#define AMDVI_DEVFN_IOAPIC             0xa0
+
+#define AMDVI_LOCAL_APIC_ADDR     0xfee00000
+
 /* extended feature support */
 #define AMDVI_EXT_FEATURES (AMDVI_FEATURE_PREFETCH | AMDVI_FEATURE_PPR | \
         AMDVI_FEATURE_IA | AMDVI_FEATURE_GT | AMDVI_FEATURE_GA | \
diff --git a/hw/i386/trace-events b/hw/i386/trace-events
index 592de3a..5c12c10 100644
--- a/hw/i386/trace-events
+++ b/hw/i386/trace-events
@@ -42,3 +42,10 @@ amdvi_mode_invalid(unsigned level, uint64_t addr)"error: translation level 0x%"P
 amdvi_page_fault(uint64_t addr) "error: page fault accessing guest physical address 0x%"PRIx64
 amdvi_iotlb_hit(uint16_t bus, uint16_t slot, uint16_t func, uint64_t addr, uint64_t txaddr) "hit iotlb devid %02x:%02x.%x gpa 0x%"PRIx64 " hpa 0x%"PRIx64
 amdvi_translation_result(uint16_t bus, uint16_t slot, uint16_t func, uint64_t addr, uint64_t txaddr) "devid: %02x:%02x.%x gpa 0x%"PRIx64 " hpa 0x%"PRIx64
+amdvi_irte_get_fail(uint64_t addr, uint64_t offset) "couldn't access device table entry 0x%"PRIx64" + offset 0x%"PRIx64
+amdvi_invalid_irte_entry(uint16_t devid, uint64_t offset) "devid %x requested IRTE offset 0x%"PRIx64" Outside IR table range"
+amdvi_ir_request(uint32_t data, uint64_t addr, uint16_t sid) "IR request data 0x%"PRIx32" address 0x%"PRIx64" SID %x"
+amdvi_ir_remap(uint32_t data, uint64_t addr, uint16_t sid) "IR remap data 0x%"PRIx32" address 0x%"PRIx64" SID %x"
+amdvi_ir_target_abort(uint32_t data, uint64_t addr, uint16_t sid) "IR target abort data 0x%"PRIx32" address 0x%"PRIx64" SID %x"
+amdvi_ir_write_fail(uint64_t addr, uint32_t data) "fail to write to addr 0x%"PRIx64 " value 0x%"PRIx32
+amdvi_ir_read_fail(uint64_t addr) " fail to read from addr 0x%"PRIx64
-- 
2.1.4

Re: [Qemu-devel] [V1 1/4] hw/iommu: Prepare for AMD IOMMU interrupt remapping

[Valentine Sinitsyn]

On 11.08.2016 00:42, David Kiarie wrote:
> Introduce macros and trace events for use in AMD IOMMU
> interrupt remapping
>
> Signed-off-by: David Kiarie <davidkiarie4@gmail.com>
> ---
>  hw/i386/amd_iommu.h  | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/i386/trace-events |  7 +++++
>  2 files changed, 79 insertions(+)
>
> diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
> index 5d0b91d..2a7f19e 100644
> --- a/hw/i386/amd_iommu.h
> +++ b/hw/i386/amd_iommu.h
> @@ -177,6 +177,78 @@
>  #define AMDVI_IOTLB_MAX_SIZE 1024
>  #define AMDVI_DEVID_SHIFT    36
>
> +/* interrupt types */
> +#define AMDVI_MT_FIXED  0x0
> +#define AMDVI_MT_ARBIT  0x1
> +#define AMDVI_MT_SMI    0x2
> +#define AMDVI_MT_NMI    0x3
> +#define AMDVI_MT_INIT   0x4
> +#define AMDVI_MT_EXTINT 0x6
> +#define AMDVI_MT_LINT1  0xb
> +#define AMDVI_MT_LINT0  0xe
> +
> +/* Ext reg, GA support */
> +#define AMDVI_GASUP    (1UL << 7)
> +/* MMIO control GA enable bits */
> +#define AMDVI_GAEN     (1UL << 17)
> +
> +/* MSI interrupt type mask */
> +#define AMDVI_IR_TYPE_MASK 0x300
> +
> +/* interrupt destination mode */
> +#define AMDVI_IRDEST_MODE_MASK 0x2
> +
> +/* select MSI data 10:0 bits */
> +#define AMDVI_IRTE_INDEX_MASK 0x7ff
> +
> +/* bits determining whether specific interrupts should be passed
> + * split DTE into 64-bit chunks
> + */
> +#define AMDVI_DTE_INTPASS       56
> +#define AMDVI_DTE_EINTPASS      57
> +#define AMDVI_DTE_NMIPASS       58
> +#define AMDVI_DTE_INTCTL        60
> +#define AMDVI_DTE_LINT0PASS     62
> +#define AMDVI_DTE_LINT1PASS     63
I suggest adding _SHIFT prefix, to make the meaning clearer.

> +
> +/* interrupt data valid */
> +#define AMDVI_IR_VALID          (1UL << 0)
> +
> +/* interrupt root table mask */
> +#define AMDVI_IRTEROOT_MASK     0xffffffffffffc0
> +
> +/* default IRTE size */
> +#define AMDVI_DEFAULT_IRTE_SIZE 0x4
> +
> +/* IRTE size with GASup enabled */
> +#define AMDVI_IRTE_SIZE_GASUP   0x10
We don't have Virtual APIC, and perhaps this is not needed (see comments 
in the next patch).

> +
> +#define AMDVI_IRTE_VECTOR_MASK    (0xffU << 16)
> +#define AMDVI_IRTE_DEST_MASK      (0xffU << 8)
> +#define AMDVI_IRTE_DM_MASK        (0x1U << 6)
> +#define AMDVI_IRTE_RQEOI_MASK     (0x1U << 5)
> +#define AMDVI_IRTE_INTTYPE_MASK   (0x7U << 2)
> +#define AMDVI_IRTE_SUPIOPF_MASK   (0x1U << 1)
> +#define AMDVI_IRTE_REMAP_MASK     (0x1U << 0)
> +
> +#define AMDVI_IR_TABLE_SIZE_MASK 0xfe
> +
> +/* offsets into MSI data */
> +#define AMDVI_MSI_DATA_DM_RSHIFT       0x8
> +#define AMDVI_MSI_DATA_LEVEL_RSHIFT    0xe
> +#define AMDVI_MSI_DATA_TRM_RSHIFT      0xf
> +
> +/* offsets into MSI address */
> +#define AMDVI_MSI_ADDR_DM_RSHIFT       0x2
> +#define AMDVI_MSI_ADDR_RH_RSHIFT       0x3
> +#define AMDVI_MSI_ADDR_DEST_RSHIFT     0xc
> +
> +#define AMDVI_BUS_NUM                  0x0
> +/* AMD-Vi specific IOAPIC Device function */
> +#define AMDVI_DEVFN_IOAPIC             0xa0
So you decided not to keep these as a single 
AMDVI_SOUTHBRIDGE_IOAPIC_ID, or similar?

Valentine

> +
> +#define AMDVI_LOCAL_APIC_ADDR     0xfee00000
> +
>  /* extended feature support */
>  #define AMDVI_EXT_FEATURES (AMDVI_FEATURE_PREFETCH | AMDVI_FEATURE_PPR | \
>          AMDVI_FEATURE_IA | AMDVI_FEATURE_GT | AMDVI_FEATURE_GA | \
> diff --git a/hw/i386/trace-events b/hw/i386/trace-events
> index 592de3a..5c12c10 100644
> --- a/hw/i386/trace-events
> +++ b/hw/i386/trace-events
> @@ -42,3 +42,10 @@ amdvi_mode_invalid(unsigned level, uint64_t addr)"error: translation level 0x%"P
>  amdvi_page_fault(uint64_t addr) "error: page fault accessing guest physical address 0x%"PRIx64
>  amdvi_iotlb_hit(uint16_t bus, uint16_t slot, uint16_t func, uint64_t addr, uint64_t txaddr) "hit iotlb devid %02x:%02x.%x gpa 0x%"PRIx64 " hpa 0x%"PRIx64
>  amdvi_translation_result(uint16_t bus, uint16_t slot, uint16_t func, uint64_t addr, uint64_t txaddr) "devid: %02x:%02x.%x gpa 0x%"PRIx64 " hpa 0x%"PRIx64
> +amdvi_irte_get_fail(uint64_t addr, uint64_t offset) "couldn't access device table entry 0x%"PRIx64" + offset 0x%"PRIx64
> +amdvi_invalid_irte_entry(uint16_t devid, uint64_t offset) "devid %x requested IRTE offset 0x%"PRIx64" Outside IR table range"
> +amdvi_ir_request(uint32_t data, uint64_t addr, uint16_t sid) "IR request data 0x%"PRIx32" address 0x%"PRIx64" SID %x"
> +amdvi_ir_remap(uint32_t data, uint64_t addr, uint16_t sid) "IR remap data 0x%"PRIx32" address 0x%"PRIx64" SID %x"
> +amdvi_ir_target_abort(uint32_t data, uint64_t addr, uint16_t sid) "IR target abort data 0x%"PRIx32" address 0x%"PRIx64" SID %x"
> +amdvi_ir_write_fail(uint64_t addr, uint32_t data) "fail to write to addr 0x%"PRIx64 " value 0x%"PRIx32
> +amdvi_ir_read_fail(uint64_t addr) " fail to read from addr 0x%"PRIx64
>

[Qemu-devel] [V1 2/4] hw/iommu: AMD IOMMU interrupt remapping

[David Kiarie]

Introduce AMD IOMMU interrupt remapping and hook it onto
the existing interrupt remapping infrastructure

Signed-off-by: David Kiarie <davidkiarie4@gmail.com>
---
 hw/i386/amd_iommu.c | 226 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 hw/i386/amd_iommu.h |   2 +
 2 files changed, 227 insertions(+), 1 deletion(-)

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 5fab9aa..825159b 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -18,12 +18,14 @@
  * with this program; if not, see <http://www.gnu.org/licenses/>.
  *
  * Cache implementation inspired by hw/i386/intel_iommu.c
+ *
  */
 #include "qemu/osdep.h"
 #include <math.h>
 #include "hw/pci/msi.h"
 #include "hw/i386/pc.h"
 #include "hw/i386/amd_iommu.h"
+#include "hw/i386/ioapic_internal.h"
 #include "hw/pci/pci_bus.h"
 #include "trace.h"
 
@@ -660,6 +662,11 @@ static void amdvi_inval_inttable(AMDVIState *s, CMDInvalIntrTable *inval)
         amdvi_log_illegalcom_error(s, inval->type, s->cmdbuf + s->cmdbuf_head);
         return;
     }
+
+    if (s->ir_cache) {
+        x86_iommu_iec_notify_all(X86_IOMMU_DEVICE(s), true, 0, 0);
+    }
+
     trace_amdvi_intr_inval();
 }
 
@@ -1221,6 +1228,207 @@ static IOMMUTLBEntry amdvi_translate(MemoryRegion *iommu, hwaddr addr,
     return ret;
 }
 
+static inline int amdvi_ir_pass(MSIMessage *src, MSIMessage *dst, uint8_t bit,
+                                uint64_t dte)
+{
+    if ((dte & (1UL << bit))) {
+        /* passing interrupt enabled */
+        dst->address = src->address;
+        dst->data = src->data;
+    } else {
+        /* should be target aborted */
+        return -AMDVI_TARGET_ABORT;
+    }
+    return 0;
+}
+
+static int amdvi_remap_ir_intctl(uint64_t dte, uint32_t irte,
+                                 MSIMessage *src, MSIMessage *dst)
+{
+    int ret = 0;
+
+    switch ((dte >> AMDVI_DTE_INTCTL ) & 3UL) {
+    case 1:
+        /* pass */
+        memcpy(dst, src, sizeof(*dst));
+        break;
+    case 2:
+        /* remap */
+        if (irte & AMDVI_IRTE_REMAP_MASK) {
+            /* LOCAL APIC address */
+            dst->address = AMDVI_LOCAL_APIC_ADDR;
+            /* destination mode */
+            dst->address |= (((irte & AMDVI_IRTE_DM_MASK) >> 6) <<
+                    AMDVI_MSI_ADDR_DM_RSHIFT);
+            /* RH */
+            dst->address |= ((irte & AMDVI_IRTE_RQEOI_MASK) >> 5) <<
+                AMDVI_MSI_ADDR_RH_RSHIFT;
+            /* Destination ID */
+            dst->address |= ((irte & AMDVI_IRTE_DEST_MASK) >> 8) <<
+                AMDVI_MSI_ADDR_DEST_RSHIFT;
+            /* construct data - vector */
+            dst->data |= (irte & AMDVI_IRTE_VECTOR_MASK) >> 16;
+            /* Interrupt type */
+            dst->data |= ((irte & AMDVI_IRTE_INTTYPE_MASK) >> 2) <<
+                AMDVI_MSI_DATA_DM_RSHIFT;
+        } else  {
+            ret = -AMDVI_TARGET_ABORT;
+        }
+        break;
+    case 0:
+    case 3:
+    default:
+        ret = -AMDVI_TARGET_ABORT;
+    }
+    return ret;
+}
+/*
+ * We don't support guest virtual APIC so IRTE size will most likely always be 4
+ */
+static int amdvi_irte_get(AMDVIState *s, MSIMessage *src, uint32_t *irte,
+                          uint64_t *dte, uint16_t devid)
+{
+    uint64_t irte_root, offset = devid * AMDVI_DEVTAB_ENTRY_SIZE,
+             irte_size = AMDVI_DEFAULT_IRTE_SIZE,
+             ir_table_size;
+
+    /* check for GASup and if it's enabled */
+    if ((amdvi_readq(s, AMDVI_EXT_FEATURES) & AMDVI_GASUP)
+        && (amdvi_readq(s, AMDVI_MMIO_CONTROL) & AMDVI_GAEN)) {
+        /* set a different IRTE size */
+        irte_size = AMDVI_IRTE_SIZE_GASUP;
+    }
+    if (dma_memory_read(&address_space_memory, s->devtab + offset, dte,
+                        AMDVI_DEVTAB_ENTRY_SIZE)) {
+        trace_amdvi_dte_get_fail(s->devtab, offset);
+        return -AMDVI_DEV_TAB_HW;
+    }
+
+    irte_root = dte[2] & AMDVI_IRTEROOT_MASK;
+    offset = (src->data & AMDVI_IRTE_INDEX_MASK) << 2;
+    ir_table_size = pow(2, dte[2] & AMDVI_IR_TABLE_SIZE_MASK);
+    /* enforce IR table size */
+    if (offset > (ir_table_size * irte_size)) {
+        trace_amdvi_invalid_irte_entry(offset, ir_table_size);
+        return -AMDVI_TARGET_ABORT;
+    }
+    /* read IRTE */
+    if (dma_memory_read(&address_space_memory, irte_root + offset,
+        irte, sizeof(*irte))) {
+        trace_amdvi_irte_get_fail(irte_root, offset);
+        return -AMDVI_DEV_TAB_HW;
+    }
+    return 0;
+}
+
+static int amdvi_int_remap(X86IOMMUState *iommu, MSIMessage *src,
+                           MSIMessage *dst, uint16_t sid)
+{
+    trace_amdvi_ir_request(src->data, src->address, sid);
+
+    AMDVIState *s = AMD_IOMMU_DEVICE(iommu);
+    int ret = 0;
+    uint64_t dte[4];
+    uint32_t ir_enabled, irte;
+
+    ret = amdvi_irte_get(s, src, &irte, dte, sid);
+    if (ret < 0) {
+        goto no_remap;
+    }
+    /* interrupt remapping disabled */
+    if (!(dte[2] & AMDVI_IR_VALID)) {
+        memcpy(dst, src, sizeof(*src));
+        return ret;
+    }
+    switch (src->data & AMDVI_IR_TYPE_MASK) {
+    case AMDVI_MT_FIXED:
+    case AMDVI_MT_ARBIT:
+        ret = amdvi_remap_ir_intctl(dte[2], irte, src, dst);
+        if (ret < 0) {
+            goto no_remap;
+        } else {
+            s->ir_cache = true;
+            trace_amdvi_ir_remap(dst->data, dst->address, sid);
+            return ret;
+        }
+    /* not handling SMI currently */
+    case AMDVI_MT_SMI:
+        goto no_remap;
+    case AMDVI_MT_NMI:
+        ir_enabled = AMDVI_DTE_NMIPASS;
+        break;
+    case AMDVI_MT_INIT:
+        ir_enabled = AMDVI_DTE_INTPASS;
+        break;
+    case AMDVI_MT_EXTINT:
+        ir_enabled = AMDVI_DTE_EINTPASS;
+        break;
+    case AMDVI_MT_LINT1:
+        ir_enabled = AMDVI_DTE_LINT1PASS;
+        break;
+    case AMDVI_MT_LINT0:
+        ir_enabled = AMDVI_DTE_LINT0PASS;
+    }
+
+    ret = amdvi_ir_pass(src, dst, ir_enabled, dte[2]);
+    if (ret < 0){
+        goto no_remap;
+    }
+    s->ir_cache = true;
+    trace_amdvi_ir_remap(dst->data, dst->address, sid);
+    return ret;
+no_remap:
+    memcpy(dst, src, sizeof(*src));
+    trace_amdvi_ir_target_abort(dst->data, dst->address, sid);
+    return ret;
+}
+
+static MemTxResult amdvi_ir_read(void *opaque, hwaddr addr,
+                                 uint64_t *data, unsigned size,
+                                 MemTxAttrs attrs)
+{
+    return MEMTX_OK;
+}
+
+static MemTxResult amdvi_ir_write(void *opaque, hwaddr addr, uint64_t val,
+                                  unsigned size, MemTxAttrs attrs)
+{
+    AMDVIAddressSpace *as = opaque;
+    MSIMessage from = { addr + AMDVI_INT_ADDR_FIRST, val }, to = { 0, 0};
+    uint16_t sid = PCI_BUILD_BDFi(as->bus_num, as->devfn);
+    int ret = 0;
+
+    ret  = amdvi_int_remap(X86_IOMMU_DEVICE(as->iommu_state), &from, &to,
+                           attrs.requester_id);
+
+    if (ret < 0) {
+        trace_amdvi_ir_target_abort(from.data, from.address,
+                                    attrs.requester_id);
+        return MEMTX_ERROR;
+    }
+
+    if(dma_memory_write(&address_space_memory, to.address, &to.data, size)) {
+        trace_amdvi_ir_write_fail(to.address, to.data);
+        return MEMTX_ERROR;
+    }
+
+    return MEMTX_OK;
+}
+
+static const MemoryRegionOps amdvi_ir_ops = {
+    .read_with_attrs = amdvi_ir_read,
+    .write_with_attrs = amdvi_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 AddressSpace *amdvi_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
 {
     AMDVIState *s = opaque;
@@ -1244,6 +1452,12 @@ static AddressSpace *amdvi_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
 
         memory_region_init_iommu(&iommu_as[devfn]->iommu, OBJECT(s),
                                  &s->iommu_ops, "amd-iommu", UINT64_MAX);
+        memory_region_init_io(&iommu_as[devfn]->iommu_ir, OBJECT(s),
+                              &amdvi_ir_ops, iommu_as[devfn], "amd-iommu-ir",
+                              AMDVI_INT_ADDR_SIZE);
+        memory_region_add_subregion(&iommu_as[devfn]->iommu,
+                                    AMDVI_INT_ADDR_FIRST,
+                                    &iommu_as[devfn]->iommu_ir);
         address_space_init(&iommu_as[devfn]->as, &iommu_as[devfn]->iommu,
                            "amd-iommu");
     }
@@ -1292,6 +1506,7 @@ static void amdvi_init(AMDVIState *s)
     s->enabled = false;
     s->ats_enabled = false;
     s->cmdbuf_enabled = false;
+    s->ir_cache = false;
 
     /* reset MMIO */
     memset(s->mmior, 0, AMDVI_MMIO_SIZE);
@@ -1331,11 +1546,15 @@ static void amdvi_realize(DeviceState *dev, Error **err)
     AMDVIState *s = AMD_IOMMU_DEVICE(dev);
     X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(dev);
     PCIBus *bus = PC_MACHINE(qdev_get_machine())->bus;
+    PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
     s->iotlb = g_hash_table_new_full(amdvi_uint64_hash,
                                      amdvi_uint64_equal, g_free, g_free);
 
-    /* This device should take care of IOMMU PCI properties */
+    /* AMD IOMMU has Interrupt Remapping on by default */
+    x86_iommu->intr_supported = true;
     x86_iommu->type = TYPE_AMD;
+
+    /* This device should take care of IOMMU PCI properties */
     qdev_set_parent_bus(DEVICE(&s->pci), &bus->qbus);
     object_property_set_bool(OBJECT(&s->pci), true, "realized", err);
     s->capab_offset = pci_add_capability(&s->pci.dev, AMDVI_CAPAB_ID_SEC, 0,
@@ -1347,9 +1566,13 @@ static void amdvi_realize(DeviceState *dev, Error **err)
     memory_region_init_io(&s->mmio, OBJECT(s), &mmio_mem_ops, s, "amdvi-mmio",
                           AMDVI_MMIO_SIZE);
 
+    x86_iommu->ioapic_bdf = PCI_BUILD_BDF(AMDVI_BUS_NUM,
+             AMDVI_DEVFN_IOAPIC);
+
     sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->mmio);
     sysbus_mmio_map(SYS_BUS_DEVICE(s), 0, AMDVI_BASE_ADDR);
     pci_setup_iommu(bus, amdvi_host_dma_iommu, s);
+    pcms->ioapic_as = amdvi_host_dma_iommu(bus, s, AMDVI_DEVFN_IOAPIC);
     s->devid = object_property_get_int(OBJECT(&s->pci), "addr", err);
     msi_init(&s->pci.dev, 0, 1, true, false, err);
     amdvi_init(s);
@@ -1376,6 +1599,7 @@ static void amdvi_class_init(ObjectClass *klass, void* data)
     dc->vmsd = &vmstate_amdvi;
     dc->hotpluggable = false;
     dc_class->realize = amdvi_realize;
+    dc_class->int_remap = amdvi_int_remap;
 }
 
 static const TypeInfo amdvi = {
diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
index 2a7f19e..f0e23a8 100644
--- a/hw/i386/amd_iommu.h
+++ b/hw/i386/amd_iommu.h
@@ -356,6 +356,8 @@ typedef struct AMDVIState {
     uint32_t evtlog_len;         /* event log length             */
     uint32_t evtlog_head;        /* current IOMMU write position */
     uint32_t evtlog_tail;        /* current Software read position */
+    /* whether we have remapped any interrupts and hence IR cache */
+    bool ir_cache;
 
     /* unused for now */
     hwaddr excl_base;            /* base DVA - IOMMU exclusion range */
-- 
2.1.4

Re: [Qemu-devel] [V1 2/4] hw/iommu: AMD IOMMU interrupt remapping

[Valentine Sinitsyn]

On 11.08.2016 00:42, David Kiarie wrote:
> Introduce AMD IOMMU interrupt remapping and hook it onto
> the existing interrupt remapping infrastructure
>
> Signed-off-by: David Kiarie <davidkiarie4@gmail.com>
> ---
>  hw/i386/amd_iommu.c | 226 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>  hw/i386/amd_iommu.h |   2 +
>  2 files changed, 227 insertions(+), 1 deletion(-)
>
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index 5fab9aa..825159b 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -18,12 +18,14 @@
>   * with this program; if not, see <http://www.gnu.org/licenses/>.
>   *
>   * Cache implementation inspired by hw/i386/intel_iommu.c
> + *
>   */
>  #include "qemu/osdep.h"
>  #include <math.h>
>  #include "hw/pci/msi.h"
>  #include "hw/i386/pc.h"
>  #include "hw/i386/amd_iommu.h"
> +#include "hw/i386/ioapic_internal.h"
>  #include "hw/pci/pci_bus.h"
>  #include "trace.h"
>
> @@ -660,6 +662,11 @@ static void amdvi_inval_inttable(AMDVIState *s, CMDInvalIntrTable *inval)
>          amdvi_log_illegalcom_error(s, inval->type, s->cmdbuf + s->cmdbuf_head);
>          return;
>      }
> +
> +    if (s->ir_cache) {
> +        x86_iommu_iec_notify_all(X86_IOMMU_DEVICE(s), true, 0, 0);
> +    }
> +
>      trace_amdvi_intr_inval();
>  }
>
> @@ -1221,6 +1228,207 @@ static IOMMUTLBEntry amdvi_translate(MemoryRegion *iommu, hwaddr addr,
>      return ret;
>  }
>
> +static inline int amdvi_ir_pass(MSIMessage *src, MSIMessage *dst, uint8_t bit,
> +                                uint64_t dte)
The name is misleading. Actually, this function handles non-vectored 
interrupts (either passes or target aborts them). Maybe call it 
amdvi_ir_handle_non_vectored() ?

> +{
> +    if ((dte & (1UL << bit))) {
> +        /* passing interrupt enabled */
> +        dst->address = src->address;
> +        dst->data = src->data;
> +    } else {
> +        /* should be target aborted */
> +        return -AMDVI_TARGET_ABORT;
> +    }
> +    return 0;
> +}
> +
> +static int amdvi_remap_ir_intctl(uint64_t dte, uint32_t irte,
> +                                 MSIMessage *src, MSIMessage *dst)
> +{
> +    int ret = 0;
> +
> +    switch ((dte >> AMDVI_DTE_INTCTL ) & 3UL) {
AMDVI_DTE_INTCTL_SHIFT? Yes, I should have mentioned it in a previous 
patch, sorry. Maybe also introduce macros for 3UL and 1, 2, 3 in switch 
branches below.

> +    case 1:
> +        /* pass */
> +        memcpy(dst, src, sizeof(*dst));
> +        break;
> +    case 2:
> +        /* remap */
> +        if (irte & AMDVI_IRTE_REMAP_MASK) {
> +            /* LOCAL APIC address */
> +            dst->address = AMDVI_LOCAL_APIC_ADDR;
> +            /* destination mode */
> +            dst->address |= (((irte & AMDVI_IRTE_DM_MASK) >> 6) <<
> +                    AMDVI_MSI_ADDR_DM_RSHIFT);
> +            /* RH */
> +            dst->address |= ((irte & AMDVI_IRTE_RQEOI_MASK) >> 5) <<
> +                AMDVI_MSI_ADDR_RH_RSHIFT;
> +            /* Destination ID */
> +            dst->address |= ((irte & AMDVI_IRTE_DEST_MASK) >> 8) <<
> +                AMDVI_MSI_ADDR_DEST_RSHIFT;
> +            /* construct data - vector */
> +            dst->data |= (irte & AMDVI_IRTE_VECTOR_MASK) >> 16;
> +            /* Interrupt type */
> +            dst->data |= ((irte & AMDVI_IRTE_INTTYPE_MASK) >> 2) <<
> +                AMDVI_MSI_DATA_DM_RSHIFT;
These bit operations look scary. Did you considered using bitfields or 
wrapping them in macros?
> +        } else  {
> +            ret = -AMDVI_TARGET_ABORT;
> +        }
> +        break;
> +    case 0:
> +    case 3:
In fact, you should report this as event when IR == 1.

> +    default:
> +        ret = -AMDVI_TARGET_ABORT;
> +    }
> +    return ret;
> +}
> +/*
> + * We don't support guest virtual APIC so IRTE size will most likely always be 4
> + */
> +static int amdvi_irte_get(AMDVIState *s, MSIMessage *src, uint32_t *irte,
> +                          uint64_t *dte, uint16_t devid)
> +{
> +    uint64_t irte_root, offset = devid * AMDVI_DEVTAB_ENTRY_SIZE,
> +             irte_size = AMDVI_DEFAULT_IRTE_SIZE,
> +             ir_table_size;
> +
> +    /* check for GASup and if it's enabled */
> +    if ((amdvi_readq(s, AMDVI_EXT_FEATURES) & AMDVI_GASUP)
> +        && (amdvi_readq(s, AMDVI_MMIO_CONTROL) & AMDVI_GAEN)) {
> +        /* set a different IRTE size */
> +        irte_size = AMDVI_IRTE_SIZE_GASUP;
> +    }
As I said, this is likely the only place where we account for Virtual 
APIC. You don't seem to handle Virtual APIC Root in DTE, for instance. 
Maybe drop this incomplete support altogether, and print some warning 
here instead?

> +    if (dma_memory_read(&address_space_memory, s->devtab + offset, dte,
> +                        AMDVI_DEVTAB_ENTRY_SIZE)) {
> +        trace_amdvi_dte_get_fail(s->devtab, offset);
> +        return -AMDVI_DEV_TAB_HW;
> +    }
> +
> +    irte_root = dte[2] & AMDVI_IRTEROOT_MASK;
> +    offset = (src->data & AMDVI_IRTE_INDEX_MASK) << 2;
> +    ir_table_size = pow(2, dte[2] & AMDVI_IR_TABLE_SIZE_MASK);
1 << dte[2] & AMDVI_IR_TABLE_SIZE_MASK ?

> +    /* enforce IR table size */
> +    if (offset > (ir_table_size * irte_size)) {
> +        trace_amdvi_invalid_irte_entry(offset, ir_table_size);
> +        return -AMDVI_TARGET_ABORT;
> +    }
> +    /* read IRTE */
> +    if (dma_memory_read(&address_space_memory, irte_root + offset,
> +        irte, sizeof(*irte))) {
> +        trace_amdvi_irte_get_fail(irte_root, offset);
> +        return -AMDVI_DEV_TAB_HW;
> +    }
> +    return 0;
> +}
> +
> +static int amdvi_int_remap(X86IOMMUState *iommu, MSIMessage *src,
> +                           MSIMessage *dst, uint16_t sid)
> +{
> +    trace_amdvi_ir_request(src->data, src->address, sid);
> +
> +    AMDVIState *s = AMD_IOMMU_DEVICE(iommu);
> +    int ret = 0;
> +    uint64_t dte[4];
> +    uint32_t ir_enabled, irte;
> +
> +    ret = amdvi_irte_get(s, src, &irte, dte, sid);
> +    if (ret < 0) {
> +        goto no_remap;
> +    }
> +    /* interrupt remapping disabled */
> +    if (!(dte[2] & AMDVI_IR_VALID)) {
> +        memcpy(dst, src, sizeof(*src));
> +        return ret;
> +    }
> +    switch (src->data & AMDVI_IR_TYPE_MASK) {
> +    case AMDVI_MT_FIXED:
> +    case AMDVI_MT_ARBIT:
> +        ret = amdvi_remap_ir_intctl(dte[2], irte, src, dst);
> +        if (ret < 0) {
> +            goto no_remap;
> +        } else {
> +            s->ir_cache = true;
> +            trace_amdvi_ir_remap(dst->data, dst->address, sid);
> +            return ret;
> +        }
> +    /* not handling SMI currently */
> +    case AMDVI_MT_SMI:
Maybe also add some warning here so we'd know when to implement SMI Filter.

> +        goto no_remap;
> +    case AMDVI_MT_NMI:
> +        ir_enabled = AMDVI_DTE_NMIPASS;
> +        break;
> +    case AMDVI_MT_INIT:
> +        ir_enabled = AMDVI_DTE_INTPASS;
> +        break;
> +    case AMDVI_MT_EXTINT:
> +        ir_enabled = AMDVI_DTE_EINTPASS;
> +        break;
> +    case AMDVI_MT_LINT1:
> +        ir_enabled = AMDVI_DTE_LINT1PASS;
> +        break;
> +    case AMDVI_MT_LINT0:
> +        ir_enabled = AMDVI_DTE_LINT0PASS;
> +    }
> +
> +    ret = amdvi_ir_pass(src, dst, ir_enabled, dte[2]);
> +    if (ret < 0){
> +        goto no_remap;
> +    }
> +    s->ir_cache = true;
> +    trace_amdvi_ir_remap(dst->data, dst->address, sid);
> +    return ret;
> +no_remap:
> +    memcpy(dst, src, sizeof(*src));
Do you need this memcpy()?  The original request is target aborted as 
soon as this returns.

> +    trace_amdvi_ir_target_abort(dst->data, dst->address, sid);
> +    return ret;
> +}
> +
> +static MemTxResult amdvi_ir_read(void *opaque, hwaddr addr,
> +                                 uint64_t *data, unsigned size,
> +                                 MemTxAttrs attrs)
> +{
> +    return MEMTX_OK;
> +}
> +
> +static MemTxResult amdvi_ir_write(void *opaque, hwaddr addr, uint64_t val,
> +                                  unsigned size, MemTxAttrs attrs)
> +{
> +    AMDVIAddressSpace *as = opaque;
> +    MSIMessage from = { addr + AMDVI_INT_ADDR_FIRST, val }, to = { 0, 0};
> +    uint16_t sid = PCI_BUILD_BDFi(as->bus_num, as->devfn);
'BDFi' reads like a typo

> +    int ret = 0;
> +
> +    ret  = amdvi_int_remap(X86_IOMMU_DEVICE(as->iommu_state), &from, &to,
> +                           attrs.requester_id);
> +
> +    if (ret < 0) {
> +        trace_amdvi_ir_target_abort(from.data, from.address,
> +                                    attrs.requester_id);
> +        return MEMTX_ERROR;
> +    }
> +
> +    if(dma_memory_write(&address_space_memory, to.address, &to.data, size)) {
> +        trace_amdvi_ir_write_fail(to.address, to.data);
> +        return MEMTX_ERROR;
> +    }
> +
> +    return MEMTX_OK;
> +}
> +
> +static const MemoryRegionOps amdvi_ir_ops = {
> +    .read_with_attrs = amdvi_ir_read,
> +    .write_with_attrs = amdvi_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 AddressSpace *amdvi_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
>  {
>      AMDVIState *s = opaque;
> @@ -1244,6 +1452,12 @@ static AddressSpace *amdvi_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
>
>          memory_region_init_iommu(&iommu_as[devfn]->iommu, OBJECT(s),
>                                   &s->iommu_ops, "amd-iommu", UINT64_MAX);
> +        memory_region_init_io(&iommu_as[devfn]->iommu_ir, OBJECT(s),
> +                              &amdvi_ir_ops, iommu_as[devfn], "amd-iommu-ir",
> +                              AMDVI_INT_ADDR_SIZE);
> +        memory_region_add_subregion(&iommu_as[devfn]->iommu,
> +                                    AMDVI_INT_ADDR_FIRST,
> +                                    &iommu_as[devfn]->iommu_ir);
>          address_space_init(&iommu_as[devfn]->as, &iommu_as[devfn]->iommu,
>                             "amd-iommu");
>      }
> @@ -1292,6 +1506,7 @@ static void amdvi_init(AMDVIState *s)
>      s->enabled = false;
>      s->ats_enabled = false;
>      s->cmdbuf_enabled = false;
> +    s->ir_cache = false;
>
>      /* reset MMIO */
>      memset(s->mmior, 0, AMDVI_MMIO_SIZE);
> @@ -1331,11 +1546,15 @@ static void amdvi_realize(DeviceState *dev, Error **err)
>      AMDVIState *s = AMD_IOMMU_DEVICE(dev);
>      X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(dev);
>      PCIBus *bus = PC_MACHINE(qdev_get_machine())->bus;
> +    PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
>      s->iotlb = g_hash_table_new_full(amdvi_uint64_hash,
>                                       amdvi_uint64_equal, g_free, g_free);
>
> -    /* This device should take care of IOMMU PCI properties */
> +    /* AMD IOMMU has Interrupt Remapping on by default */
> +    x86_iommu->intr_supported = true;
>      x86_iommu->type = TYPE_AMD;
> +
> +    /* This device should take care of IOMMU PCI properties */
>      qdev_set_parent_bus(DEVICE(&s->pci), &bus->qbus);
>      object_property_set_bool(OBJECT(&s->pci), true, "realized", err);
>      s->capab_offset = pci_add_capability(&s->pci.dev, AMDVI_CAPAB_ID_SEC, 0,
> @@ -1347,9 +1566,13 @@ static void amdvi_realize(DeviceState *dev, Error **err)
>      memory_region_init_io(&s->mmio, OBJECT(s), &mmio_mem_ops, s, "amdvi-mmio",
>                            AMDVI_MMIO_SIZE);
>
> +    x86_iommu->ioapic_bdf = PCI_BUILD_BDF(AMDVI_BUS_NUM,
> +             AMDVI_DEVFN_IOAPIC);
> +
>      sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->mmio);
>      sysbus_mmio_map(SYS_BUS_DEVICE(s), 0, AMDVI_BASE_ADDR);
>      pci_setup_iommu(bus, amdvi_host_dma_iommu, s);
> +    pcms->ioapic_as = amdvi_host_dma_iommu(bus, s, AMDVI_DEVFN_IOAPIC);
>      s->devid = object_property_get_int(OBJECT(&s->pci), "addr", err);
>      msi_init(&s->pci.dev, 0, 1, true, false, err);
>      amdvi_init(s);
> @@ -1376,6 +1599,7 @@ static void amdvi_class_init(ObjectClass *klass, void* data)
>      dc->vmsd = &vmstate_amdvi;
>      dc->hotpluggable = false;
>      dc_class->realize = amdvi_realize;
> +    dc_class->int_remap = amdvi_int_remap;
>  }
>
>  static const TypeInfo amdvi = {
> diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
> index 2a7f19e..f0e23a8 100644
> --- a/hw/i386/amd_iommu.h
> +++ b/hw/i386/amd_iommu.h
> @@ -356,6 +356,8 @@ typedef struct AMDVIState {
>      uint32_t evtlog_len;         /* event log length             */
>      uint32_t evtlog_head;        /* current IOMMU write position */
>      uint32_t evtlog_tail;        /* current Software read position */
> +    /* whether we have remapped any interrupts and hence IR cache */
> +    bool ir_cache;
>
>      /* unused for now */
>      hwaddr excl_base;            /* base DVA - IOMMU exclusion range */
>

Valentine

Re: [Qemu-devel] [V1 2/4] hw/iommu: AMD IOMMU interrupt remapping

[David Kiarie]

On Fri, Aug 12, 2016 at 11:08 PM, Valentine Sinitsyn <
valentine.sinitsyn@gmail.com> wrote:

> On 11.08.2016 00:42, David Kiarie wrote:
>
>> Introduce AMD IOMMU interrupt remapping and hook it onto
>
>
>> +static inline int amdvi_ir_pass(MSIMessage *src, MSIMessage *dst,
>> uint8_t bit,
>> +                                uint64_t dte)
>>
> The name is misleading. Actually, this function handles non-vectored
> interrupts (either passes or target aborts them). Maybe call it
> amdvi_ir_handle_non_vectored() ?
>
> +{
>> +    if ((dte & (1UL << bit))) {
>> +        /* passing interrupt enabled */
>> +        dst->address = src->address;
>> +        dst->data = src->data;
>> +    } else {
>> +        /* should be target aborted */
>> +        return -AMDVI_TARGET_ABORT;
>> +    }
>> +    return 0;
>> +}
>> +
>> +static int amdvi_remap_ir_intctl(uint64_t dte, uint32_t irte,
>> +                                 MSIMessage *src, MSIMessage *dst)
>> +{
>> +    int ret = 0;
>> +
>> +    switch ((dte >> AMDVI_DTE_INTCTL ) & 3UL) {
>>
> AMDVI_DTE_INTCTL_SHIFT? Yes, I should have mentioned it in a previous
> patch, sorry. Maybe also introduce macros for 3UL and 1, 2, 3 in switch
> branches below.
>
> +    case 1:
>> +        /* pass */
>> +        memcpy(dst, src, sizeof(*dst));
>> +        break;
>> +    case 2:
>> +        /* remap */
>> +        if (irte & AMDVI_IRTE_REMAP_MASK) {
>> +            /* LOCAL APIC address */
>> +            dst->address = AMDVI_LOCAL_APIC_ADDR;
>> +            /* destination mode */
>> +            dst->address |= (((irte & AMDVI_IRTE_DM_MASK) >> 6) <<
>> +                    AMDVI_MSI_ADDR_DM_RSHIFT);
>> +            /* RH */
>> +            dst->address |= ((irte & AMDVI_IRTE_RQEOI_MASK) >> 5) <<
>> +                AMDVI_MSI_ADDR_RH_RSHIFT;
>> +            /* Destination ID */
>> +            dst->address |= ((irte & AMDVI_IRTE_DEST_MASK) >> 8) <<
>> +                AMDVI_MSI_ADDR_DEST_RSHIFT;
>> +            /* construct data - vector */
>> +            dst->data |= (irte & AMDVI_IRTE_VECTOR_MASK) >> 16;
>> +            /* Interrupt type */
>> +            dst->data |= ((irte & AMDVI_IRTE_INTTYPE_MASK) >> 2) <<
>> +                AMDVI_MSI_DATA_DM_RSHIFT;
>>
> These bit operations look scary. Did you considered using bitfields or
> wrapping them in macros?


Will look at introducing macros to cover this comment and the previous one.


>
> +        } else  {
>> +            ret = -AMDVI_TARGET_ABORT;
>> +        }
>> +        break;
>> +    case 0:
>> +    case 3:
>>
> In fact, you should report this as event when IR == 1.
>
> +    default:
>> +        ret = -AMDVI_TARGET_ABORT;
>> +    }
>> +    return ret;
>> +}
>> +/*
>> + * We don't support guest virtual APIC so IRTE size will most likely
>> always be 4
>> + */
>> +static int amdvi_irte_get(AMDVIState *s, MSIMessage *src, uint32_t *irte,
>> +                          uint64_t *dte, uint16_t devid)
>> +{
>> +    uint64_t irte_root, offset = devid * AMDVI_DEVTAB_ENTRY_SIZE,
>> +             irte_size = AMDVI_DEFAULT_IRTE_SIZE,
>> +             ir_table_size;
>> +
>> +    /* check for GASup and if it's enabled */
>> +    if ((amdvi_readq(s, AMDVI_EXT_FEATURES) & AMDVI_GASUP)
>> +        && (amdvi_readq(s, AMDVI_MMIO_CONTROL) & AMDVI_GAEN)) {
>> +        /* set a different IRTE size */
>> +        irte_size = AMDVI_IRTE_SIZE_GASUP;
>> +    }
>>
> As I said, this is likely the only place where we account for Virtual
> APIC. You don't seem to handle Virtual APIC Root in DTE, for instance.
> Maybe drop this incomplete support altogether, and print some warning here
> instead?


I'll drop everything and print a warning instead.


>
>
> +    if (dma_memory_read(&address_space_memory, s->devtab + offset, dte,
>> +                        AMDVI_DEVTAB_ENTRY_SIZE)) {
>> +        trace_amdvi_dte_get_fail(s->devtab, offset);
>> +        return -AMDVI_DEV_TAB_HW;
>> +    }
>> +
>> +    irte_root = dte[2] & AMDVI_IRTEROOT_MASK;
>> +    offset = (src->data & AMDVI_IRTE_INDEX_MASK) << 2;
>> +    ir_table_size = pow(2, dte[2] & AMDVI_IR_TABLE_SIZE_MASK);
>>
> 1 << dte[2] & AMDVI_IR_TABLE_SIZE_MASK ?
>
>
> +    /* enforce IR table size */
>> +    if (offset > (ir_table_size * irte_size)) {
>> +        trace_amdvi_invalid_irte_entry(offset, ir_table_size);
>> +        return -AMDVI_TARGET_ABORT;
>> +    }
>> +    /* read IRTE */
>> +    if (dma_memory_read(&address_space_memory, irte_root + offset,
>> +        irte, sizeof(*irte))) {
>> +        trace_amdvi_irte_get_fail(irte_root, offset);
>> +        return -AMDVI_DEV_TAB_HW;
>> +    }
>> +    return 0;
>> +}
>> +
>> +static int amdvi_int_remap(X86IOMMUState *iommu, MSIMessage *src,
>> +                           MSIMessage *dst, uint16_t sid)
>> +{
>> +    trace_amdvi_ir_request(src->data, src->address, sid);
>> +
>> +    AMDVIState *s = AMD_IOMMU_DEVICE(iommu);
>> +    int ret = 0;
>> +    uint64_t dte[4];
>> +    uint32_t ir_enabled, irte;
>> +
>> +    ret = amdvi_irte_get(s, src, &irte, dte, sid);
>> +    if (ret < 0) {
>> +        goto no_remap;
>> +    }
>> +    /* interrupt remapping disabled */
>> +    if (!(dte[2] & AMDVI_IR_VALID)) {
>> +        memcpy(dst, src, sizeof(*src));
>> +        return ret;
>> +    }
>> +    switch (src->data & AMDVI_IR_TYPE_MASK) {
>> +    case AMDVI_MT_FIXED:
>> +    case AMDVI_MT_ARBIT:
>> +        ret = amdvi_remap_ir_intctl(dte[2], irte, src, dst);
>> +        if (ret < 0) {
>> +            goto no_remap;
>> +        } else {
>> +            s->ir_cache = true;
>> +            trace_amdvi_ir_remap(dst->data, dst->address, sid);
>> +            return ret;
>> +        }
>> +    /* not handling SMI currently */
>> +    case AMDVI_MT_SMI:
>>
> Maybe also add some warning here so we'd know when to implement SMI Filter.
>
>
> +        goto no_remap;
>> +    case AMDVI_MT_NMI:
>> +        ir_enabled = AMDVI_DTE_NMIPASS;
>> +        break;
>> +    case AMDVI_MT_INIT:
>> +        ir_enabled = AMDVI_DTE_INTPASS;
>> +        break;
>> +    case AMDVI_MT_EXTINT:
>> +        ir_enabled = AMDVI_DTE_EINTPASS;
>> +        break;
>> +    case AMDVI_MT_LINT1:
>> +        ir_enabled = AMDVI_DTE_LINT1PASS;
>> +        break;
>> +    case AMDVI_MT_LINT0:
>> +        ir_enabled = AMDVI_DTE_LINT0PASS;
>> +    }
>> +
>> +    ret = amdvi_ir_pass(src, dst, ir_enabled, dte[2]);
>> +    if (ret < 0){
>> +        goto no_remap;
>> +    }
>> +    s->ir_cache = true;
>> +    trace_amdvi_ir_remap(dst->data, dst->address, sid);
>> +    return ret;
>> +no_remap:
>> +    memcpy(dst, src, sizeof(*src));
>>
> Do you need this memcpy()?  The original request is target aborted as soon
> as this returns.
>
> +    trace_amdvi_ir_target_abort(dst->data, dst->address, sid);
>> +    return ret;
>> +}
>> +
>> +static MemTxResult amdvi_ir_read(void *opaque, hwaddr addr,
>> +                                 uint64_t *data, unsigned size,
>> +                                 MemTxAttrs attrs)
>> +{
>> +    return MEMTX_OK;
>> +}
>> +
>> +static MemTxResult amdvi_ir_write(void *opaque, hwaddr addr, uint64_t
>> val,
>> +                                  unsigned size, MemTxAttrs attrs)
>> +{
>> +    AMDVIAddressSpace *as = opaque;
>> +    MSIMessage from = { addr + AMDVI_INT_ADDR_FIRST, val }, to = { 0, 0};
>> +    uint16_t sid = PCI_BUILD_BDFi(as->bus_num, as->devfn);
>>
> 'BDFi' reads like a typo


Right, typo even I am not sure how it got this far.

Overall, thanks for review all comments will be covered in next version.


>
>
> +    int ret = 0;
>> +
>> +    ret  = amdvi_int_remap(X86_IOMMU_DEVICE(as->iommu_state), &from,
>> &to,
>> +                           attrs.requester_id);
>> +
>> +    if (ret < 0) {
>> +        trace_amdvi_ir_target_abort(from.data, from.address,
>> +                                    attrs.requester_id);
>> +        return MEMTX_ERROR;
>> +    }
>> +
>> +    if(dma_memory_write(&address_space_memory, to.address, &to.data,
>> size)) {
>> +        trace_amdvi_ir_write_fail(to.address, to.data);
>> +        return MEMTX_ERROR;
>> +    }
>> +
>> +    return MEMTX_OK;
>> +}
>> +
>> +static const MemoryRegionOps amdvi_ir_ops = {
>> +    .read_with_attrs = amdvi_ir_read,
>> +    .write_with_attrs = amdvi_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 AddressSpace *amdvi_host_dma_iommu(PCIBus *bus, void *opaque, int
>> devfn)
>>  {
>>      AMDVIState *s = opaque;
>> @@ -1244,6 +1452,12 @@ static AddressSpace *amdvi_host_dma_iommu(PCIBus
>> *bus, void *opaque, int devfn)
>>
>>          memory_region_init_iommu(&iommu_as[devfn]->iommu, OBJECT(s),
>>                                   &s->iommu_ops, "amd-iommu", UINT64_MAX);
>> +        memory_region_init_io(&iommu_as[devfn]->iommu_ir, OBJECT(s),
>> +                              &amdvi_ir_ops, iommu_as[devfn],
>> "amd-iommu-ir",
>> +                              AMDVI_INT_ADDR_SIZE);
>> +        memory_region_add_subregion(&iommu_as[devfn]->iommu,
>> +                                    AMDVI_INT_ADDR_FIRST,
>> +                                    &iommu_as[devfn]->iommu_ir);
>>          address_space_init(&iommu_as[devfn]->as,
>> &iommu_as[devfn]->iommu,
>>                             "amd-iommu");
>>      }
>> @@ -1292,6 +1506,7 @@ static void amdvi_init(AMDVIState *s)
>>      s->enabled = false;
>>      s->ats_enabled = false;
>>      s->cmdbuf_enabled = false;
>> +    s->ir_cache = false;
>>
>>      /* reset MMIO */
>>      memset(s->mmior, 0, AMDVI_MMIO_SIZE);
>> @@ -1331,11 +1546,15 @@ static void amdvi_realize(DeviceState *dev, Error
>> **err)
>>      AMDVIState *s = AMD_IOMMU_DEVICE(dev);
>>      X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(dev);
>>      PCIBus *bus = PC_MACHINE(qdev_get_machine())->bus;
>> +    PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
>>      s->iotlb = g_hash_table_new_full(amdvi_uint64_hash,
>>                                       amdvi_uint64_equal, g_free, g_free);
>>
>> -    /* This device should take care of IOMMU PCI properties */
>> +    /* AMD IOMMU has Interrupt Remapping on by default */
>> +    x86_iommu->intr_supported = true;
>>      x86_iommu->type = TYPE_AMD;
>> +
>> +    /* This device should take care of IOMMU PCI properties */
>>      qdev_set_parent_bus(DEVICE(&s->pci), &bus->qbus);
>>      object_property_set_bool(OBJECT(&s->pci), true, "realized", err);
>>      s->capab_offset = pci_add_capability(&s->pci.dev,
>> AMDVI_CAPAB_ID_SEC, 0,
>> @@ -1347,9 +1566,13 @@ static void amdvi_realize(DeviceState *dev, Error
>> **err)
>>      memory_region_init_io(&s->mmio, OBJECT(s), &mmio_mem_ops, s,
>> "amdvi-mmio",
>>                            AMDVI_MMIO_SIZE);
>>
>> +    x86_iommu->ioapic_bdf = PCI_BUILD_BDF(AMDVI_BUS_NUM,
>> +             AMDVI_DEVFN_IOAPIC);
>> +
>>      sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->mmio);
>>      sysbus_mmio_map(SYS_BUS_DEVICE(s), 0, AMDVI_BASE_ADDR);
>>      pci_setup_iommu(bus, amdvi_host_dma_iommu, s);
>> +    pcms->ioapic_as = amdvi_host_dma_iommu(bus, s, AMDVI_DEVFN_IOAPIC);
>>      s->devid = object_property_get_int(OBJECT(&s->pci), "addr", err);
>>      msi_init(&s->pci.dev, 0, 1, true, false, err);
>>      amdvi_init(s);
>> @@ -1376,6 +1599,7 @@ static void amdvi_class_init(ObjectClass *klass,
>> void* data)
>>      dc->vmsd = &vmstate_amdvi;
>>      dc->hotpluggable = false;
>>      dc_class->realize = amdvi_realize;
>> +    dc_class->int_remap = amdvi_int_remap;
>>  }
>>
>>  static const TypeInfo amdvi = {
>> diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
>> index 2a7f19e..f0e23a8 100644
>> --- a/hw/i386/amd_iommu.h
>> +++ b/hw/i386/amd_iommu.h
>> @@ -356,6 +356,8 @@ typedef struct AMDVIState {
>>      uint32_t evtlog_len;         /* event log length             */
>>      uint32_t evtlog_head;        /* current IOMMU write position */
>>      uint32_t evtlog_tail;        /* current Software read position */
>> +    /* whether we have remapped any interrupts and hence IR cache */
>> +    bool ir_cache;
>>
>>      /* unused for now */
>>      hwaddr excl_base;            /* base DVA - IOMMU exclusion range */
>>
>>
> Valentine
>

[Qemu-devel] [V1 3/4] hw/acpi: report IOAPIC on IVRS

[David Kiarie]

Report IOAPIC via IVRS which effectively allows linux AMD-Vi
driver to enable interrupt remapping

Signed-off-by: David Kiarie <davidkiarie4@gmail.com>
---
 hw/i386/acpi-build.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 49bd183..da602c3 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2615,6 +2615,8 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker)
      *   Refer to Spec - Table 95:IVHD Device Entry Type Codes(4-byte)
      */
     build_append_int_noprefix(table_data, 0x0000001, 4);
+    /* IOAPIC represented as an 8-byte entry. Spec v2.62 Tables 97 */
+    build_append_int_noprefix(table_data, 0x0100a000ff000048, 8);
 
     build_header(linker, table_data, (void *)(table_data->data + iommu_start),
                  "IVRS", table_data->len - iommu_start, 1, NULL, NULL);
-- 
2.1.4

Re: [Qemu-devel] [V1 3/4] hw/acpi: report IOAPIC on IVRS

[Valentine Sinitsyn]

On 11.08.2016 00:42, David Kiarie wrote:
> Report IOAPIC via IVRS which effectively allows linux AMD-Vi
> driver to enable interrupt remapping
>
> Signed-off-by: David Kiarie <davidkiarie4@gmail.com>
> ---
>  hw/i386/acpi-build.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 49bd183..da602c3 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2615,6 +2615,8 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker)
>       *   Refer to Spec - Table 95:IVHD Device Entry Type Codes(4-byte)
>       */
>      build_append_int_noprefix(table_data, 0x0000001, 4);
> +    /* IOAPIC represented as an 8-byte entry. Spec v2.62 Tables 97 */
> +    build_append_int_noprefix(table_data, 0x0100a000ff000048, 8);
Nit: bit 3 in DTE Setting is reserved (Table 97 in the spec), while you 
set them all with 0xff.

>
>      build_header(linker, table_data, (void *)(table_data->data + iommu_start),
>                   "IVRS", table_data->len - iommu_start, 1, NULL, NULL);
>

Valentine

Re: [Qemu-devel] [V1 3/4] hw/acpi: report IOAPIC on IVRS

[David Kiarie]

On Fri, Aug 12, 2016 at 11:14 PM, Valentine Sinitsyn <
valentine.sinitsyn@gmail.com> wrote:

> On 11.08.2016 00:42, David Kiarie wrote:
>
>> Report IOAPIC via IVRS which effectively allows linux AMD-Vi
>> driver to enable interrupt remapping
>>
>> Signed-off-by: David Kiarie <davidkiarie4@gmail.com>
>> ---
>>  hw/i386/acpi-build.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index 49bd183..da602c3 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -2615,6 +2615,8 @@ build_amd_iommu(GArray *table_data, BIOSLinker
>> *linker)
>>       *   Refer to Spec - Table 95:IVHD Device Entry Type Codes(4-byte)
>>       */
>>      build_append_int_noprefix(table_data, 0x0000001, 4);
>> +    /* IOAPIC represented as an 8-byte entry. Spec v2.62 Tables 97 */
>> +    build_append_int_noprefix(table_data, 0x0100a000ff000048, 8);
>>
> Nit: bit 3 in DTE Setting is reserved (Table 97 in the spec), while you
> set them all with 0xff.


Noted, thanks!


>
>
>
>>      build_header(linker, table_data, (void *)(table_data->data +
>> iommu_start),
>>                   "IVRS", table_data->len - iommu_start, 1, NULL, NULL);
>>
>>
> Valentine
>
>

[Qemu-devel] [V1 4/4] hw/iommu: share common between IOMMUs

[David Kiarie]

Enabling interrupt remapping with kernel_irqchip=on should
result in error for both VT-d and AMD-Vi

Signed-off-by: David Kiarie <davidkiarie4@gmail.com>
---
 hw/i386/intel_iommu.c | 9 ---------
 hw/i386/x86-iommu.c   | 8 ++++++++
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 7c80907..857f9f1 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -30,7 +30,6 @@
 #include "hw/boards.h"
 #include "hw/i386/x86-iommu.h"
 #include "hw/pci-host/q35.h"
-#include "sysemu/kvm.h"
 
 #define DEBUG_INTEL_IOMMU
 #ifdef DEBUG_INTEL_IOMMU
@@ -2472,14 +2471,6 @@ static void vtd_realize(DeviceState *dev, Error **errp)
         Q35_PSEUDO_DEVFN_IOAPIC;
     /* Pseudo address space under root PCI bus. */
     pcms->ioapic_as = vtd_host_dma_iommu(bus, s, Q35_PSEUDO_DEVFN_IOAPIC);
-
-    /* Currently Intel IOMMU IR only support "kernel-irqchip={off|split}" */
-    if (x86_iommu->intr_supported && kvm_irqchip_in_kernel() &&
-        !kvm_irqchip_is_split()) {
-        error_report("Intel Interrupt Remapping cannot work with "
-                     "kernel-irqchip=on, please use 'split|off'.");
-        exit(1);
-    }
 }
 
 static void vtd_class_init(ObjectClass *klass, void *data)
diff --git a/hw/i386/x86-iommu.c b/hw/i386/x86-iommu.c
index 2278af7..66510f7 100644
--- a/hw/i386/x86-iommu.c
+++ b/hw/i386/x86-iommu.c
@@ -21,6 +21,7 @@
 #include "hw/sysbus.h"
 #include "hw/boards.h"
 #include "hw/i386/x86-iommu.h"
+#include "sysemu/kvm.h"
 #include "qemu/error-report.h"
 #include "trace.h"
 
@@ -84,6 +85,13 @@ static void x86_iommu_realize(DeviceState *dev, Error **errp)
     if (x86_class->realize) {
         x86_class->realize(dev, errp);
     }
+    /* Currently IOMMU IR only support "kernel-irqchip={off|split}" */
+    if (x86_iommu->intr_supported && kvm_irqchip_in_kernel() &&
+        !kvm_irqchip_is_split()) {
+        error_report("Interrupt Remapping cannot work with "
+                     "kernel-irqchip=on, please use 'split|off'.");
+        exit(1);
+    }
 
     x86_iommu_set_default(X86_IOMMU_DEVICE(dev));
 }
-- 
2.1.4

Re: [Qemu-devel] [V1 0/4] AMD-Vi Interrupt Remapping

[David Kiarie]

On Wed, Aug 10, 2016 at 10:42 PM, David Kiarie <davidkiarie4@gmail.com>
wrote:

Sorry, forgot to cc Peter.

Hello all,
>
> The following patchset adds AMD-Vi interrupt remapping logic
> to Qemu and hooks it onto existing interrupt remapping infrastructure.It
> has
> a dependency on "Explicit SID for IOAPIC" patchset though.
>
> I would appreciate your feedback!
>
> For quick testing https://github.com/aslaq/qemu IR
>
> David Kiarie (4):
>   hw/iommu: Prepare for AMD IOMMU interrupt remapping
>   hw/iommu: AMD IOMMU interrupt remapping
>   hw/acpi: report IOAPIC on IVRS
>   hw/iommu: share common between IOMMUs
>
>  hw/i386/acpi-build.c  |   2 +
>  hw/i386/amd_iommu.c   | 226 ++++++++++++++++++++++++++++++
> +++++++++++++++++++-
>  hw/i386/amd_iommu.h   |  74 +++++++++++++++++
>  hw/i386/intel_iommu.c |   9 --
>  hw/i386/trace-events  |   7 ++
>  hw/i386/x86-iommu.c   |   8 ++
>  6 files changed, 316 insertions(+), 10 deletions(-)
>
> --
> 2.1.4
>
>

Re: [Qemu-devel] [V1 0/4] AMD-Vi Interrupt Remapping

[Valentine Sinitsyn]

Hi David,

On 11.08.2016 00:42, David Kiarie wrote:
> Hello all,
>
> The following patchset adds AMD-Vi interrupt remapping logic
> to Qemu and hooks it onto existing interrupt remapping infrastructure.It has
> a dependency on "Explicit SID for IOAPIC" patchset though.
>
> I would appreciate your feedback!
Nice work! I'll look into it deeper in a few days.

In the meantime, I'm not able to apply this on top of your V15 IOMMU 
series applied to the current master. Am I missing anything?

Valentine

>
> For quick testing https://github.com/aslaq/qemu IR
>
> David Kiarie (4):
>   hw/iommu: Prepare for AMD IOMMU interrupt remapping
>   hw/iommu: AMD IOMMU interrupt remapping
>   hw/acpi: report IOAPIC on IVRS
>   hw/iommu: share common between IOMMUs
>
>  hw/i386/acpi-build.c  |   2 +
>  hw/i386/amd_iommu.c   | 226 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  hw/i386/amd_iommu.h   |  74 +++++++++++++++++
>  hw/i386/intel_iommu.c |   9 --
>  hw/i386/trace-events  |   7 ++
>  hw/i386/x86-iommu.c   |   8 ++
>  6 files changed, 316 insertions(+), 10 deletions(-)
>

Re: [Qemu-devel] [V1 0/4] AMD-Vi Interrupt Remapping

[David Kiarie]

On Thu, Aug 11, 2016 at 9:39 AM, Valentine Sinitsyn <
valentine.sinitsyn@gmail.com> wrote:

> Hi David,
>
> On 11.08.2016 00:42, David Kiarie wrote:
>
>> Hello all,
>>
>> The following patchset adds AMD-Vi interrupt remapping logic
>> to Qemu and hooks it onto existing interrupt remapping infrastructure.It
>> has
>> a dependency on "Explicit SID for IOAPIC" patchset though.
>>
>> I would appreciate your feedback!
>>
> Nice work! I'll look into it deeper in a few days.
>
> In the meantime, I'm not able to apply this on top of your V15 IOMMU
> series applied to the current master. Am I missing anything?


I think it got stale already. You can test from here though
https://github.com/aslaq/qemu v15


>
> Valentine
>
>
>
>> For quick testing https://github.com/aslaq/qemu IR
>>
>> David Kiarie (4):
>>   hw/iommu: Prepare for AMD IOMMU interrupt remapping
>>   hw/iommu: AMD IOMMU interrupt remapping
>>   hw/acpi: report IOAPIC on IVRS
>>   hw/iommu: share common between IOMMUs
>>
>>  hw/i386/acpi-build.c  |   2 +
>>  hw/i386/amd_iommu.c   | 226 ++++++++++++++++++++++++++++++
>> +++++++++++++++++++-
>>  hw/i386/amd_iommu.h   |  74 +++++++++++++++++
>>  hw/i386/intel_iommu.c |   9 --
>>  hw/i386/trace-events  |   7 ++
>>  hw/i386/x86-iommu.c   |   8 ++
>>  6 files changed, 316 insertions(+), 10 deletions(-)
>>
>>