[PATCH v2 0/5] Interrupt Remap support for emulated amd viommu

[PATCH v2 0/5] Interrupt Remap support for emulated amd viommu

[Santosh Shukla]

Series adds following feature support for emulated amd vIOMMU
1) Pass Through(PT) mode
2) Interrupt Remapping(IR) mode

1) PT mode
Introducing the shared 'nodma' memory region that can be aliased
by all the devices in the PT mode. Shared memory with aliasing
approach will help run VM faster when lot of devices attached to
VM.

2) IR mode
Shared IR memory region with aliasing approach proposed for the
reason mentioned in 1). Also add support to invalidate Interrupt
remaping table(IRT).

Series based on ea9cdbcf3a0b8d5497cddf87990f1b39d8f3bb0a

Testing:
1. nvme/fio testing for VM with > 255 vCPU with xtsup=on and x2apic
enabled
2. Windows Server 2022 VM testing for > 255 vCPU.

Change History:

V2:
- Fixed non-kvm build issue (Reported by Michael Tsirkin)

V1:
- https://lore.kernel.org/all/20240904100257.184851-3-santosh.shukla@amd.com/T/


Suravee Suthikulpanit (5):
  amd_iommu: Rename variable mmio to mr_mmio
  amd_iommu: Add support for pass though mode
  amd_iommu: Use shared memory region for Interrupt Remapping
  amd_iommu: Send notification when invaldate interrupt entry cache
  amd_iommu: Check APIC ID > 255 for XTSup

 hw/i386/acpi-build.c |  4 +-
 hw/i386/amd_iommu.c  | 98 +++++++++++++++++++++++++++++++++++---------
 hw/i386/amd_iommu.h  |  5 ++-
 3 files changed, 85 insertions(+), 22 deletions(-)

-- 
2.43.5

[PATCH v2 1/5] amd_iommu: Rename variable mmio to mr_mmio

[Santosh Shukla]

From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

Rename the MMIO memory region variable 'mmio' to 'mr_mmio'
so to correctly name align with struct AMDVIState::variable type.

No functional change intended.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Santosh Shukla <santosh.shukla@amd.com>
---
 hw/i386/acpi-build.c | 4 ++--
 hw/i386/amd_iommu.c  | 6 +++---
 hw/i386/amd_iommu.h  | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 4967aa745902..5c6920f90d05 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2321,7 +2321,7 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker, const char *oem_id,
     /* Capability offset */
     build_append_int_noprefix(table_data, s->pci.capab_offset, 2);
     /* IOMMU base address */
-    build_append_int_noprefix(table_data, s->mmio.addr, 8);
+    build_append_int_noprefix(table_data, s->mr_mmio.addr, 8);
     /* PCI Segment Group */
     build_append_int_noprefix(table_data, 0, 2);
     /* IOMMU info */
@@ -2356,7 +2356,7 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker, const char *oem_id,
     /* Capability offset */
     build_append_int_noprefix(table_data, s->pci.capab_offset, 2);
     /* IOMMU base address */
-    build_append_int_noprefix(table_data, s->mmio.addr, 8);
+    build_append_int_noprefix(table_data, s->mr_mmio.addr, 8);
     /* PCI Segment Group */
     build_append_int_noprefix(table_data, 0, 2);
     /* IOMMU info */
diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 464f0b666e69..abb64ea507be 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -1598,10 +1598,10 @@ static void amdvi_sysbus_realize(DeviceState *dev, Error **errp)
     x86ms->ioapic_as = amdvi_host_dma_iommu(bus, s, AMDVI_IOAPIC_SB_DEVID);
 
     /* set up MMIO */
-    memory_region_init_io(&s->mmio, OBJECT(s), &mmio_mem_ops, s, "amdvi-mmio",
-                          AMDVI_MMIO_SIZE);
+    memory_region_init_io(&s->mr_mmio, OBJECT(s), &mmio_mem_ops, s,
+                          "amdvi-mmio", AMDVI_MMIO_SIZE);
     memory_region_add_subregion(get_system_memory(), AMDVI_BASE_ADDR,
-                                &s->mmio);
+                                &s->mr_mmio);
     pci_setup_iommu(bus, &amdvi_iommu_ops, s);
     amdvi_init(s);
 }
diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
index 73619fe9eaa7..e5c2ae94f243 100644
--- a/hw/i386/amd_iommu.h
+++ b/hw/i386/amd_iommu.h
@@ -353,7 +353,7 @@ struct AMDVIState {
     uint32_t pprlog_head;        /* ppr log head */
     uint32_t pprlog_tail;        /* ppr log tail */
 
-    MemoryRegion mmio;                 /* MMIO region                  */
+    MemoryRegion mr_mmio;              /* MMIO region                  */
     uint8_t mmior[AMDVI_MMIO_SIZE];    /* read/write MMIO              */
     uint8_t w1cmask[AMDVI_MMIO_SIZE];  /* read/write 1 clear mask      */
     uint8_t romask[AMDVI_MMIO_SIZE];   /* MMIO read/only mask          */
-- 
2.43.5

[PATCH v2 2/5] amd_iommu: Add support for pass though mode

[Santosh Shukla]

From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

Introduce 'nodma' shared memory region to support PT mode
so that for each device, we only create an alias to shared memory
region when DMA-remapping is disabled.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Santosh Shukla <santosh.shukla@amd.com>
---
 hw/i386/amd_iommu.c | 49 ++++++++++++++++++++++++++++++++++++---------
 hw/i386/amd_iommu.h |  2 ++
 2 files changed, 42 insertions(+), 9 deletions(-)

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index abb64ea507be..c5f5103f4911 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -60,8 +60,9 @@ struct AMDVIAddressSpace {
     uint8_t bus_num;            /* bus number                           */
     uint8_t devfn;              /* device function                      */
     AMDVIState *iommu_state;    /* AMDVI - one per machine              */
-    MemoryRegion root;          /* AMDVI Root memory map region */
+    MemoryRegion root;          /* AMDVI Root memory map region         */
     IOMMUMemoryRegion iommu;    /* Device's address translation region  */
+    MemoryRegion iommu_nodma;   /* Alias of shared nodma memory region  */
     MemoryRegion iommu_ir;      /* Device's interrupt remapping region  */
     AddressSpace as;            /* device's corresponding address space */
 };
@@ -1412,6 +1413,7 @@ static AddressSpace *amdvi_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
     AMDVIState *s = opaque;
     AMDVIAddressSpace **iommu_as, *amdvi_dev_as;
     int bus_num = pci_bus_num(bus);
+    X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s);
 
     iommu_as = s->address_spaces[bus_num];
 
@@ -1436,13 +1438,13 @@ static AddressSpace *amdvi_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
          * Memory region relationships looks like (Address range shows
          * only lower 32 bits to make it short in length...):
          *
-         * |-----------------+-------------------+----------|
-         * | Name            | Address range     | Priority |
-         * |-----------------+-------------------+----------+
-         * | amdvi_root      | 00000000-ffffffff |        0 |
-         * |  amdvi_iommu    | 00000000-ffffffff |        1 |
-         * |  amdvi_iommu_ir | fee00000-feefffff |       64 |
-         * |-----------------+-------------------+----------|
+         * |--------------------+-------------------+----------|
+         * | Name               | Address range     | Priority |
+         * |--------------------+-------------------+----------+
+         * | amdvi-root         | 00000000-ffffffff |        0 |
+         * | amdvi-iommu_nodma  | 00000000-ffffffff |        0 |
+         * | amdvi-iommu_ir     | fee00000-feefffff |       64 |
+         * |--------------------+-------------------+----------|
          */
         memory_region_init_iommu(&amdvi_dev_as->iommu,
                                  sizeof(amdvi_dev_as->iommu),
@@ -1461,7 +1463,25 @@ static AddressSpace *amdvi_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
                                             64);
         memory_region_add_subregion_overlap(&amdvi_dev_as->root, 0,
                                             MEMORY_REGION(&amdvi_dev_as->iommu),
-                                            1);
+                                            0);
+
+        /* Build the DMA Disabled alias to shared memory */
+        memory_region_init_alias(&amdvi_dev_as->iommu_nodma, OBJECT(s),
+                                 "amdvi-sys", &s->mr_sys, 0,
+                                 memory_region_size(&s->mr_sys));
+        memory_region_add_subregion_overlap(&amdvi_dev_as->root, 0,
+                                            &amdvi_dev_as->iommu_nodma,
+                                            0);
+
+        if (!x86_iommu->pt_supported) {
+            memory_region_set_enabled(&amdvi_dev_as->iommu_nodma, false);
+            memory_region_set_enabled(MEMORY_REGION(&amdvi_dev_as->iommu),
+                                      true);
+        } else {
+            memory_region_set_enabled(MEMORY_REGION(&amdvi_dev_as->iommu),
+                                      false);
+            memory_region_set_enabled(&amdvi_dev_as->iommu_nodma, true);
+        }
     }
     return &iommu_as[devfn]->as;
 }
@@ -1602,6 +1622,17 @@ static void amdvi_sysbus_realize(DeviceState *dev, Error **errp)
                           "amdvi-mmio", AMDVI_MMIO_SIZE);
     memory_region_add_subregion(get_system_memory(), AMDVI_BASE_ADDR,
                                 &s->mr_mmio);
+
+    /* Create the share memory regions by all devices */
+    memory_region_init(&s->mr_sys, OBJECT(s), "amdvi-sys", UINT64_MAX);
+
+    /* set up the DMA disabled memory region */
+    memory_region_init_alias(&s->mr_nodma, OBJECT(s),
+                             "amdvi-nodma", get_system_memory(), 0,
+                             memory_region_size(get_system_memory()));
+    memory_region_add_subregion_overlap(&s->mr_sys, 0,
+                                        &s->mr_nodma, 0);
+
     pci_setup_iommu(bus, &amdvi_iommu_ops, s);
     amdvi_init(s);
 }
diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
index e5c2ae94f243..be417e51c4dc 100644
--- a/hw/i386/amd_iommu.h
+++ b/hw/i386/amd_iommu.h
@@ -354,6 +354,8 @@ struct AMDVIState {
     uint32_t pprlog_tail;        /* ppr log tail */
 
     MemoryRegion mr_mmio;              /* MMIO region                  */
+    MemoryRegion mr_sys;
+    MemoryRegion mr_nodma;
     uint8_t mmior[AMDVI_MMIO_SIZE];    /* read/write MMIO              */
     uint8_t w1cmask[AMDVI_MMIO_SIZE];  /* read/write 1 clear mask      */
     uint8_t romask[AMDVI_MMIO_SIZE];   /* MMIO read/only mask          */
-- 
2.43.5

[PATCH v2 3/5] amd_iommu: Use shared memory region for Interrupt Remapping

[Santosh Shukla]

From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

Use shared memory region for interrupt remapping which can be
aliased by all devices.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Santosh Shukla <santosh.shukla@amd.com>
---
 hw/i386/amd_iommu.c | 22 ++++++++++++++--------
 hw/i386/amd_iommu.h |  1 +
 2 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index c5f5103f4911..24fcd561345c 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -1443,7 +1443,7 @@ static AddressSpace *amdvi_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
          * |--------------------+-------------------+----------+
          * | amdvi-root         | 00000000-ffffffff |        0 |
          * | amdvi-iommu_nodma  | 00000000-ffffffff |        0 |
-         * | amdvi-iommu_ir     | fee00000-feefffff |       64 |
+         * | amdvi-iommu_ir     | fee00000-feefffff |        1 |
          * |--------------------+-------------------+----------|
          */
         memory_region_init_iommu(&amdvi_dev_as->iommu,
@@ -1454,13 +1454,6 @@ static AddressSpace *amdvi_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
         memory_region_init(&amdvi_dev_as->root, OBJECT(s),
                            "amdvi_root", UINT64_MAX);
         address_space_init(&amdvi_dev_as->as, &amdvi_dev_as->root, name);
-        memory_region_init_io(&amdvi_dev_as->iommu_ir, OBJECT(s),
-                              &amdvi_ir_ops, s, "amd_iommu_ir",
-                              AMDVI_INT_ADDR_SIZE);
-        memory_region_add_subregion_overlap(&amdvi_dev_as->root,
-                                            AMDVI_INT_ADDR_FIRST,
-                                            &amdvi_dev_as->iommu_ir,
-                                            64);
         memory_region_add_subregion_overlap(&amdvi_dev_as->root, 0,
                                             MEMORY_REGION(&amdvi_dev_as->iommu),
                                             0);
@@ -1472,6 +1465,13 @@ static AddressSpace *amdvi_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
         memory_region_add_subregion_overlap(&amdvi_dev_as->root, 0,
                                             &amdvi_dev_as->iommu_nodma,
                                             0);
+        /* Build the Interrupt Remapping alias to shared memory */
+        memory_region_init_alias(&amdvi_dev_as->iommu_ir, OBJECT(s),
+                                 "amdvi-ir", &s->mr_ir, 0,
+                                 memory_region_size(&s->mr_ir));
+        memory_region_add_subregion_overlap(MEMORY_REGION(&amdvi_dev_as->iommu),
+                                            AMDVI_INT_ADDR_FIRST,
+                                            &amdvi_dev_as->iommu_ir, 1);
 
         if (!x86_iommu->pt_supported) {
             memory_region_set_enabled(&amdvi_dev_as->iommu_nodma, false);
@@ -1633,6 +1633,12 @@ static void amdvi_sysbus_realize(DeviceState *dev, Error **errp)
     memory_region_add_subregion_overlap(&s->mr_sys, 0,
                                         &s->mr_nodma, 0);
 
+    /* set up the Interrupt Remapping memory region */
+    memory_region_init_io(&s->mr_ir, OBJECT(s), &amdvi_ir_ops,
+                          s, "amdvi-ir", AMDVI_INT_ADDR_SIZE);
+    memory_region_add_subregion_overlap(&s->mr_sys, AMDVI_INT_ADDR_FIRST,
+                                        &s->mr_ir, 1);
+
     pci_setup_iommu(bus, &amdvi_iommu_ops, s);
     amdvi_init(s);
 }
diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
index be417e51c4dc..e0dac4d9a96c 100644
--- a/hw/i386/amd_iommu.h
+++ b/hw/i386/amd_iommu.h
@@ -356,6 +356,7 @@ struct AMDVIState {
     MemoryRegion mr_mmio;              /* MMIO region                  */
     MemoryRegion mr_sys;
     MemoryRegion mr_nodma;
+    MemoryRegion mr_ir;
     uint8_t mmior[AMDVI_MMIO_SIZE];    /* read/write MMIO              */
     uint8_t w1cmask[AMDVI_MMIO_SIZE];  /* read/write 1 clear mask      */
     uint8_t romask[AMDVI_MMIO_SIZE];   /* MMIO read/only mask          */
-- 
2.43.5

[PATCH v2 4/5] amd_iommu: Send notification when invaldate interrupt entry cache

[Santosh Shukla]

From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

In order to support AMD IOMMU interrupt remapping emulation with PCI
pass-through devices, QEMU needs to notify VFIO when guest IOMMU driver
updates and invalidate the guest interrupt remapping table (IRT), and
communicate information so that the host IOMMU driver can update
the shadowed interrupt remapping table in the host IOMMU.

Therefore, send notification when guet IOMMU emulates the IRT invalidation
commands.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Santosh Shukla <santosh.shukla@amd.com>
---
 hw/i386/amd_iommu.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 24fcd561345c..9095146525e6 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -431,6 +431,12 @@ static void amdvi_complete_ppr(AMDVIState *s, uint64_t *cmd)
     trace_amdvi_ppr_exec();
 }
 
+static void amdvi_intremap_inval_notify_all(AMDVIState *s, bool global,
+                               uint32_t index, uint32_t mask)
+{
+    x86_iommu_iec_notify_all(X86_IOMMU_DEVICE(s), global, index, mask);
+}
+
 static void amdvi_inval_all(AMDVIState *s, uint64_t *cmd)
 {
     if (extract64(cmd[0], 0, 60) || cmd[1]) {
@@ -438,6 +444,9 @@ static void amdvi_inval_all(AMDVIState *s, uint64_t *cmd)
                                    s->cmdbuf + s->cmdbuf_head);
     }
 
+    /* Notify global invalidation */
+    amdvi_intremap_inval_notify_all(s, true, 0, 0);
+
     amdvi_iotlb_reset(s);
     trace_amdvi_all_inval();
 }
@@ -486,6 +495,9 @@ static void amdvi_inval_inttable(AMDVIState *s, uint64_t *cmd)
         return;
     }
 
+    /* Notify global invalidation */
+    amdvi_intremap_inval_notify_all(s, true, 0, 0);
+
     trace_amdvi_intr_inval();
 }
 
-- 
2.43.5

[PATCH v2 5/5] amd_iommu: Check APIC ID > 255 for XTSup

[Santosh Shukla]

From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

The XTSup mode enables x2APIC support for AMD IOMMU, which is needed
to support vcpu w/ APIC ID > 255.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Santosh Shukla <santosh.shukla@amd.com>
---
v2:
- Fixed non-kvm build issue by adding a check for kvm_irqchip_is_split()

 hw/i386/amd_iommu.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 9095146525e6..24eebf053df0 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -32,6 +32,7 @@
 #include "trace.h"
 #include "hw/i386/apic-msidef.h"
 #include "hw/qdev-properties.h"
+#include "kvm/kvm_i386.h"
 
 /* used AMD-Vi MMIO registers */
 const char *amdvi_mmio_low[] = {
@@ -1651,6 +1652,16 @@ static void amdvi_sysbus_realize(DeviceState *dev, Error **errp)
     memory_region_add_subregion_overlap(&s->mr_sys, AMDVI_INT_ADDR_FIRST,
                                         &s->mr_ir, 1);
 
+    /* AMD IOMMU with x2APIC mode requires xtsup=on */
+    if (x86ms->apic_id_limit > 255 && !s->xtsup) {
+        error_report("AMD IOMMU with x2APIC confguration requires xtsup=on");
+        exit(EXIT_FAILURE);
+    }
+    if (s->xtsup && kvm_irqchip_is_split() && !kvm_enable_x2apic()) {
+        error_report("AMD IOMMU xt=on requires support on the KVM side");
+        exit(EXIT_FAILURE);
+    }
+
     pci_setup_iommu(bus, &amdvi_iommu_ops, s);
     amdvi_init(s);
 }
-- 
2.43.5

Re: [PATCH v2 2/5] amd_iommu: Add support for pass though mode

[Alejandro Jimenez]

Hi Santosh,

On 9/16/24 10:31, Santosh Shukla wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> 
> Introduce 'nodma' shared memory region to support PT mode
> so that for each device, we only create an alias to shared memory
> region when DMA-remapping is disabled.
> 
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Signed-off-by: Santosh Shukla <santosh.shukla@amd.com>
> ---
>   hw/i386/amd_iommu.c | 49 ++++++++++++++++++++++++++++++++++++---------
>   hw/i386/amd_iommu.h |  2 ++
>   2 files changed, 42 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index abb64ea507be..c5f5103f4911 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -60,8 +60,9 @@ struct AMDVIAddressSpace {
>       uint8_t bus_num;            /* bus number                           */
>       uint8_t devfn;              /* device function                      */
>       AMDVIState *iommu_state;    /* AMDVI - one per machine              */
> -    MemoryRegion root;          /* AMDVI Root memory map region */
> +    MemoryRegion root;          /* AMDVI Root memory map region         */
>       IOMMUMemoryRegion iommu;    /* Device's address translation region  */
> +    MemoryRegion iommu_nodma;   /* Alias of shared nodma memory region  */
>       MemoryRegion iommu_ir;      /* Device's interrupt remapping region  */
>       AddressSpace as;            /* device's corresponding address space */
>   };
> @@ -1412,6 +1413,7 @@ static AddressSpace *amdvi_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
>       AMDVIState *s = opaque;
>       AMDVIAddressSpace **iommu_as, *amdvi_dev_as;
>       int bus_num = pci_bus_num(bus);
> +    X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s);
>   
>       iommu_as = s->address_spaces[bus_num];
>   
> @@ -1436,13 +1438,13 @@ static AddressSpace *amdvi_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
>            * Memory region relationships looks like (Address range shows
>            * only lower 32 bits to make it short in length...):
>            *
> -         * |-----------------+-------------------+----------|
> -         * | Name            | Address range     | Priority |
> -         * |-----------------+-------------------+----------+
> -         * | amdvi_root      | 00000000-ffffffff |        0 |
> -         * |  amdvi_iommu    | 00000000-ffffffff |        1 |
> -         * |  amdvi_iommu_ir | fee00000-feefffff |       64 |
> -         * |-----------------+-------------------+----------|
> +         * |--------------------+-------------------+----------|
> +         * | Name               | Address range     | Priority |
> +         * |--------------------+-------------------+----------+
> +         * | amdvi-root         | 00000000-ffffffff |        0 |
> +         * | amdvi-iommu_nodma  | 00000000-ffffffff |        0 |
> +         * | amdvi-iommu_ir     | fee00000-feefffff |       64 |
> +         * |--------------------+-------------------+----------|

Minor nit: I would keep the original indentation here to help reinforce the concept that iommu_nodma and iommu_ir are meant to be sub-regions under the root container. It would also be great if the table could show that they are mutually exclusive based on whether passthrough is in use, but that is probably too much to include in this format.

Alejandro
> +
> +        if (!x86_iommu->pt_supported) {
> +            memory_region_set_enabled(&amdvi_dev_as->iommu_nodma, false);
> +            memory_region_set_enabled(MEMORY_REGION(&amdvi_dev_as->iommu),
> +                                      true);
> +        } else {
> +            memory_region_set_enabled(MEMORY_REGION(&amdvi_dev_as->iommu),
> +                                      false);
> +            memory_region_set_enabled(&amdvi_dev_as->iommu_nodma, true);
> +        }
>       }
>       return &iommu_as[devfn]->as;
>   }
> @@ -1602,6 +1622,17 @@ static void amdvi_sysbus_realize(DeviceState *dev, Error **errp)
>                             "amdvi-mmio", AMDVI_MMIO_SIZE);
>       memory_region_add_subregion(get_system_memory(), AMDVI_BASE_ADDR,
>                                   &s->mr_mmio);
> +
> +    /* Create the share memory regions by all devices */
> +    memory_region_init(&s->mr_sys, OBJECT(s), "amdvi-sys", UINT64_MAX);
> +
> +    /* set up the DMA disabled memory region */
> +    memory_region_init_alias(&s->mr_nodma, OBJECT(s),
> +                             "amdvi-nodma", get_system_memory(), 0,
> +                             memory_region_size(get_system_memory()));
> +    memory_region_add_subregion_overlap(&s->mr_sys, 0,
> +                                        &s->mr_nodma, 0);
> +
>       pci_setup_iommu(bus, &amdvi_iommu_ops, s);
>       amdvi_init(s);
>   }
> diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
> index e5c2ae94f243..be417e51c4dc 100644
> --- a/hw/i386/amd_iommu.h
> +++ b/hw/i386/amd_iommu.h
> @@ -354,6 +354,8 @@ struct AMDVIState {
>       uint32_t pprlog_tail;        /* ppr log tail */
>   
>       MemoryRegion mr_mmio;              /* MMIO region                  */
> +    MemoryRegion mr_sys;
> +    MemoryRegion mr_nodma;
>       uint8_t mmior[AMDVI_MMIO_SIZE];    /* read/write MMIO              */
>       uint8_t w1cmask[AMDVI_MMIO_SIZE];  /* read/write 1 clear mask      */
>       uint8_t romask[AMDVI_MMIO_SIZE];   /* MMIO read/only mask          */

Re: [PATCH v2 4/5] amd_iommu: Send notification when invaldate interrupt entry cache

[Alejandro Jimenez]

In subject:
s/invaldate/invalidate/

On 9/16/24 10:31, Santosh Shukla wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> 
> In order to support AMD IOMMU interrupt remapping emulation with PCI
> pass-through devices, QEMU needs to notify VFIO when guest IOMMU driver
> updates and invalidate the guest interrupt remapping table (IRT), and
> communicate information so that the host IOMMU driver can update
> the shadowed interrupt remapping table in the host IOMMU.
> 
> Therefore, send notification when guet

s/guet/guest

Alejandro

  IOMMU emulates the IRT invalidation
> commands.
> 
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Signed-off-by: Santosh Shukla <santosh.shukla@amd.com>
> ---
>   hw/i386/amd_iommu.c | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
> 
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index 24fcd561345c..9095146525e6 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -431,6 +431,12 @@ static void amdvi_complete_ppr(AMDVIState *s, uint64_t *cmd)
>       trace_amdvi_ppr_exec();
>   }
>   
> +static void amdvi_intremap_inval_notify_all(AMDVIState *s, bool global,
> +                               uint32_t index, uint32_t mask)
> +{
> +    x86_iommu_iec_notify_all(X86_IOMMU_DEVICE(s), global, index, mask);
> +}
> +
>   static void amdvi_inval_all(AMDVIState *s, uint64_t *cmd)
>   {
>       if (extract64(cmd[0], 0, 60) || cmd[1]) {
> @@ -438,6 +444,9 @@ static void amdvi_inval_all(AMDVIState *s, uint64_t *cmd)
>                                      s->cmdbuf + s->cmdbuf_head);
>       }
>   
> +    /* Notify global invalidation */
> +    amdvi_intremap_inval_notify_all(s, true, 0, 0);
> +
>       amdvi_iotlb_reset(s);
>       trace_amdvi_all_inval();
>   }
> @@ -486,6 +495,9 @@ static void amdvi_inval_inttable(AMDVIState *s, uint64_t *cmd)
>           return;
>       }
>   
> +    /* Notify global invalidation */
> +    amdvi_intremap_inval_notify_all(s, true, 0, 0);
> +
>       trace_amdvi_intr_inval();
>   }
>   

Re: [PATCH v2 5/5] amd_iommu: Check APIC ID > 255 for XTSup

[Alejandro Jimenez]

On 9/16/24 10:31, Santosh Shukla wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> 
> The XTSup mode enables x2APIC support for AMD IOMMU, which is needed
> to support vcpu w/ APIC ID > 255.
> 
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Signed-off-by: Santosh Shukla <santosh.shukla@amd.com>
> ---
> v2:
> - Fixed non-kvm build issue by adding a check for kvm_irqchip_is_split()
> 
>   hw/i386/amd_iommu.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
> 
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index 9095146525e6..24eebf053df0 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -32,6 +32,7 @@
>   #include "trace.h"
>   #include "hw/i386/apic-msidef.h"
>   #include "hw/qdev-properties.h"
> +#include "kvm/kvm_i386.h"
>   
>   /* used AMD-Vi MMIO registers */
>   const char *amdvi_mmio_low[] = {
> @@ -1651,6 +1652,16 @@ static void amdvi_sysbus_realize(DeviceState *dev, Error **errp)
>       memory_region_add_subregion_overlap(&s->mr_sys, AMDVI_INT_ADDR_FIRST,
>                                           &s->mr_ir, 1);
>   
> +    /* AMD IOMMU with x2APIC mode requires xtsup=on */
> +    if (x86ms->apic_id_limit > 255 && !s->xtsup) {
> +        error_report("AMD IOMMU with x2APIC confguration requires xtsup=on");
> +        exit(EXIT_FAILURE);
> +    }
> +    if (s->xtsup && kvm_irqchip_is_split() && !kvm_enable_x2apic()) {
> +        error_report("AMD IOMMU xt=on requires support on the KVM side");

Use "xtsup=on" on the error message, "xt" is not a valid option IIUC.

Alejandro

> +        exit(EXIT_FAILURE);
> +    }
> +
>       pci_setup_iommu(bus, &amdvi_iommu_ops, s);
>       amdvi_init(s);
>   }

Re: [PATCH v2 0/5] Interrupt Remap support for emulated amd viommu

[Alejandro Jimenez]

Hi Santosh,


On 9/16/24 10:31, Santosh Shukla wrote:
> Series adds following feature support for emulated amd vIOMMU
> 1) Pass Through(PT) mode
> 2) Interrupt Remapping(IR) mode
> 
> 1) PT mode
> Introducing the shared 'nodma' memory region that can be aliased
> by all the devices in the PT mode. Shared memory with aliasing
> approach will help run VM faster when lot of devices attached to
> VM.
> 
> 2) IR mode
> Shared IR memory region with aliasing approach proposed for the
> reason mentioned in 1). Also add support to invalidate Interrupt
> remaping table(IRT).
> 
> Series based on ea9cdbcf3a0b8d5497cddf87990f1b39d8f3bb0a
> 
> Testing:
> 1. nvme/fio testing for VM with > 255 vCPU with xtsup=on and x2apic
> enabled
> 2. Windows Server 2022 VM testing for > 255 vCPU.

Tested on EPYC Genoa launching a guest with 380 vCPUs, with VFIO passthrough NIC, using "-device amd-iommu,intremap=on,xtsup=on,pt=on"

I pointed out a few minor nits, of which I think the most important is to correct the error message on PATCH 5/5. With that addressed:

Reviewed-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>

Thank you.

> 
> Change History:
> 
> V2:
> - Fixed non-kvm build issue (Reported by Michael Tsirkin)
> 
> V1:
> - https://lore.kernel.org/all/20240904100257.184851-3-santosh.shukla@amd.com/T/
> 
> 
> Suravee Suthikulpanit (5):
>    amd_iommu: Rename variable mmio to mr_mmio
>    amd_iommu: Add support for pass though mode
>    amd_iommu: Use shared memory region for Interrupt Remapping
>    amd_iommu: Send notification when invaldate interrupt entry cache
>    amd_iommu: Check APIC ID > 255 for XTSup
> 
>   hw/i386/acpi-build.c |  4 +-
>   hw/i386/amd_iommu.c  | 98 +++++++++++++++++++++++++++++++++++---------
>   hw/i386/amd_iommu.h  |  5 ++-
>   3 files changed, 85 insertions(+), 22 deletions(-)
> 

Re: [PATCH v2 4/5] amd_iommu: Send notification when invaldate interrupt entry cache

[Shukla, Santosh]

On 9/21/2024 1:56 AM, Alejandro Jimenez wrote:
> In subject:
> s/invaldate/invalidate/
> 
> On 9/16/24 10:31, Santosh Shukla wrote:
>> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>
>> In order to support AMD IOMMU interrupt remapping emulation with PCI
>> pass-through devices, QEMU needs to notify VFIO when guest IOMMU driver
>> updates and invalidate the guest interrupt remapping table (IRT), and
>> communicate information so that the host IOMMU driver can update
>> the shadowed interrupt remapping table in the host IOMMU.
>>
>> Therefore, send notification when guet
> 
> s/guet/guest
> 
> Alejandro

V3. Thanks

> 
>  IOMMU emulates the IRT invalidation
>> commands.
>>
>> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> Signed-off-by: Santosh Shukla <santosh.shukla@amd.com>
>> ---
>>   hw/i386/amd_iommu.c | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
>> index 24fcd561345c..9095146525e6 100644
>> --- a/hw/i386/amd_iommu.c
>> +++ b/hw/i386/amd_iommu.c
>> @@ -431,6 +431,12 @@ static void amdvi_complete_ppr(AMDVIState *s, uint64_t *cmd)
>>       trace_amdvi_ppr_exec();
>>   }
>>   +static void amdvi_intremap_inval_notify_all(AMDVIState *s, bool global,
>> +                               uint32_t index, uint32_t mask)
>> +{
>> +    x86_iommu_iec_notify_all(X86_IOMMU_DEVICE(s), global, index, mask);
>> +}
>> +
>>   static void amdvi_inval_all(AMDVIState *s, uint64_t *cmd)
>>   {
>>       if (extract64(cmd[0], 0, 60) || cmd[1]) {
>> @@ -438,6 +444,9 @@ static void amdvi_inval_all(AMDVIState *s, uint64_t *cmd)
>>                                      s->cmdbuf + s->cmdbuf_head);
>>       }
>>   +    /* Notify global invalidation */
>> +    amdvi_intremap_inval_notify_all(s, true, 0, 0);
>> +
>>       amdvi_iotlb_reset(s);
>>       trace_amdvi_all_inval();
>>   }
>> @@ -486,6 +495,9 @@ static void amdvi_inval_inttable(AMDVIState *s, uint64_t *cmd)
>>           return;
>>       }
>>   +    /* Notify global invalidation */
>> +    amdvi_intremap_inval_notify_all(s, true, 0, 0);
>> +
>>       trace_amdvi_intr_inval();
>>   }
>>   

Re: [PATCH v2 5/5] amd_iommu: Check APIC ID > 255 for XTSup

[Shukla, Santosh]

On 9/21/2024 1:56 AM, Alejandro Jimenez wrote:
> 
> 
> On 9/16/24 10:31, Santosh Shukla wrote:
>> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>
>> The XTSup mode enables x2APIC support for AMD IOMMU, which is needed
>> to support vcpu w/ APIC ID > 255.
>>
>> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> Signed-off-by: Santosh Shukla <santosh.shukla@amd.com>
>> ---
>> v2:
>> - Fixed non-kvm build issue by adding a check for kvm_irqchip_is_split()
>>
>>   hw/i386/amd_iommu.c | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
>> index 9095146525e6..24eebf053df0 100644
>> --- a/hw/i386/amd_iommu.c
>> +++ b/hw/i386/amd_iommu.c
>> @@ -32,6 +32,7 @@
>>   #include "trace.h"
>>   #include "hw/i386/apic-msidef.h"
>>   #include "hw/qdev-properties.h"
>> +#include "kvm/kvm_i386.h"
>>     /* used AMD-Vi MMIO registers */
>>   const char *amdvi_mmio_low[] = {
>> @@ -1651,6 +1652,16 @@ static void amdvi_sysbus_realize(DeviceState *dev, Error **errp)
>>       memory_region_add_subregion_overlap(&s->mr_sys, AMDVI_INT_ADDR_FIRST,
>>                                           &s->mr_ir, 1);
>>   +    /* AMD IOMMU with x2APIC mode requires xtsup=on */
>> +    if (x86ms->apic_id_limit > 255 && !s->xtsup) {
>> +        error_report("AMD IOMMU with x2APIC confguration requires xtsup=on");
>> +        exit(EXIT_FAILURE);
>> +    }
>> +    if (s->xtsup && kvm_irqchip_is_split() && !kvm_enable_x2apic()) {
>> +        error_report("AMD IOMMU xt=on requires support on the KVM side");
> 
> Use "xtsup=on" on the error message, "xt" is not a valid option IIUC.
> 

Yes - V3, thank you for spotting.

Thanks,
Santosh

> Alejandro
> 
>> +        exit(EXIT_FAILURE);
>> +    }
>> +
>>       pci_setup_iommu(bus, &amdvi_iommu_ops, s);
>>       amdvi_init(s);
>>   }

Re: [PATCH v2 0/5] Interrupt Remap support for emulated amd viommu

[Shukla, Santosh]

Hi Alejandro,

On 9/21/2024 2:09 AM, Alejandro Jimenez wrote:
> Hi Santosh,
> 
> 
> On 9/16/24 10:31, Santosh Shukla wrote:
>> Series adds following feature support for emulated amd vIOMMU
>> 1) Pass Through(PT) mode
>> 2) Interrupt Remapping(IR) mode
>>
>> 1) PT mode
>> Introducing the shared 'nodma' memory region that can be aliased
>> by all the devices in the PT mode. Shared memory with aliasing
>> approach will help run VM faster when lot of devices attached to
>> VM.
>>
>> 2) IR mode
>> Shared IR memory region with aliasing approach proposed for the
>> reason mentioned in 1). Also add support to invalidate Interrupt
>> remaping table(IRT).
>>
>> Series based on ea9cdbcf3a0b8d5497cddf87990f1b39d8f3bb0a
>>
>> Testing:
>> 1. nvme/fio testing for VM with > 255 vCPU with xtsup=on and x2apic
>> enabled
>> 2. Windows Server 2022 VM testing for > 255 vCPU.
> 
> Tested on EPYC Genoa launching a guest with 380 vCPUs, with VFIO passthrough NIC, using "-device amd-iommu,intremap=on,xtsup=on,pt=on"
> 
> I pointed out a few minor nits, of which I think the most important is to correct the error message on PATCH 5/5. With that addressed:
> 
> Reviewed-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
> 
> Thank you.

Thank you for reviewing and comments.

Regards,
Santosh

> 
>>
>> Change History:
>>
>> V2:
>> - Fixed non-kvm build issue (Reported by Michael Tsirkin)
>>
>> V1:
>> - https://lore.kernel.org/all/20240904100257.184851-3-santosh.shukla@amd.com/T/
>>
>>
>> Suravee Suthikulpanit (5):
>>    amd_iommu: Rename variable mmio to mr_mmio
>>    amd_iommu: Add support for pass though mode
>>    amd_iommu: Use shared memory region for Interrupt Remapping
>>    amd_iommu: Send notification when invaldate interrupt entry cache
>>    amd_iommu: Check APIC ID > 255 for XTSup
>>
>>   hw/i386/acpi-build.c |  4 +-
>>   hw/i386/amd_iommu.c  | 98 +++++++++++++++++++++++++++++++++++---------
>>   hw/i386/amd_iommu.h  |  5 ++-
>>   3 files changed, 85 insertions(+), 22 deletions(-)
>>

Re: [PATCH v2 2/5] amd_iommu: Add support for pass though mode

[Shukla, Santosh]

On 9/21/2024 1:56 AM, Alejandro Jimenez wrote:
> Hi Santosh,
> 
> On 9/16/24 10:31, Santosh Shukla wrote:
>> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>
>> Introduce 'nodma' shared memory region to support PT mode
>> so that for each device, we only create an alias to shared memory
>> region when DMA-remapping is disabled.
>>
>> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> Signed-off-by: Santosh Shukla <santosh.shukla@amd.com>
>> ---
>>   hw/i386/amd_iommu.c | 49 ++++++++++++++++++++++++++++++++++++---------
>>   hw/i386/amd_iommu.h |  2 ++
>>   2 files changed, 42 insertions(+), 9 deletions(-)
>>
>> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
>> index abb64ea507be..c5f5103f4911 100644
>> --- a/hw/i386/amd_iommu.c
>> +++ b/hw/i386/amd_iommu.c
>> @@ -60,8 +60,9 @@ struct AMDVIAddressSpace {
>>       uint8_t bus_num;            /* bus number                           */
>>       uint8_t devfn;              /* device function                      */
>>       AMDVIState *iommu_state;    /* AMDVI - one per machine              */
>> -    MemoryRegion root;          /* AMDVI Root memory map region */
>> +    MemoryRegion root;          /* AMDVI Root memory map region         */
>>       IOMMUMemoryRegion iommu;    /* Device's address translation region  */
>> +    MemoryRegion iommu_nodma;   /* Alias of shared nodma memory region  */
>>       MemoryRegion iommu_ir;      /* Device's interrupt remapping region  */
>>       AddressSpace as;            /* device's corresponding address space */
>>   };
>> @@ -1412,6 +1413,7 @@ static AddressSpace *amdvi_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
>>       AMDVIState *s = opaque;
>>       AMDVIAddressSpace **iommu_as, *amdvi_dev_as;
>>       int bus_num = pci_bus_num(bus);
>> +    X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s);
>>         iommu_as = s->address_spaces[bus_num];
>>   @@ -1436,13 +1438,13 @@ static AddressSpace *amdvi_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
>>            * Memory region relationships looks like (Address range shows
>>            * only lower 32 bits to make it short in length...):
>>            *
>> -         * |-----------------+-------------------+----------|
>> -         * | Name            | Address range     | Priority |
>> -         * |-----------------+-------------------+----------+
>> -         * | amdvi_root      | 00000000-ffffffff |        0 |
>> -         * |  amdvi_iommu    | 00000000-ffffffff |        1 |
>> -         * |  amdvi_iommu_ir | fee00000-feefffff |       64 |
>> -         * |-----------------+-------------------+----------|
>> +         * |--------------------+-------------------+----------|
>> +         * | Name               | Address range     | Priority |
>> +         * |--------------------+-------------------+----------+
>> +         * | amdvi-root         | 00000000-ffffffff |        0 |
>> +         * | amdvi-iommu_nodma  | 00000000-ffffffff |        0 |
>> +         * | amdvi-iommu_ir     | fee00000-feefffff |       64 |
>> +         * |--------------------+-------------------+----------|
> 
> Minor nit: I would keep the original indentation here to help reinforce the concept that iommu_nodma and iommu_ir are meant to be sub-regions under the root container. It would also be great if the table could show that they are 
 V3 - Thank you for pointing that out.

- Santosh

mutually exclusive based on whether passthrough is in use, but that is probably too much to include in this format.
> 
> Alejandro
>> +
>> +        if (!x86_iommu->pt_supported) {
>> +            memory_region_set_enabled(&amdvi_dev_as->iommu_nodma, false);
>> +            memory_region_set_enabled(MEMORY_REGION(&amdvi_dev_as->iommu),
>> +                                      true);
>> +        } else {
>> +            memory_region_set_enabled(MEMORY_REGION(&amdvi_dev_as->iommu),
>> +                                      false);
>> +            memory_region_set_enabled(&amdvi_dev_as->iommu_nodma, true);
>> +        }
>>       }
>>       return &iommu_as[devfn]->as;
>>   }
>> @@ -1602,6 +1622,17 @@ static void amdvi_sysbus_realize(DeviceState *dev, Error **errp)
>>                             "amdvi-mmio", AMDVI_MMIO_SIZE);
>>       memory_region_add_subregion(get_system_memory(), AMDVI_BASE_ADDR,
>>                                   &s->mr_mmio);
>> +
>> +    /* Create the share memory regions by all devices */
>> +    memory_region_init(&s->mr_sys, OBJECT(s), "amdvi-sys", UINT64_MAX);
>> +
>> +    /* set up the DMA disabled memory region */
>> +    memory_region_init_alias(&s->mr_nodma, OBJECT(s),
>> +                             "amdvi-nodma", get_system_memory(), 0,
>> +                             memory_region_size(get_system_memory()));
>> +    memory_region_add_subregion_overlap(&s->mr_sys, 0,
>> +                                        &s->mr_nodma, 0);
>> +
>>       pci_setup_iommu(bus, &amdvi_iommu_ops, s);
>>       amdvi_init(s);
>>   }
>> diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
>> index e5c2ae94f243..be417e51c4dc 100644
>> --- a/hw/i386/amd_iommu.h
>> +++ b/hw/i386/amd_iommu.h
>> @@ -354,6 +354,8 @@ struct AMDVIState {
>>       uint32_t pprlog_tail;        /* ppr log tail */
>>         MemoryRegion mr_mmio;              /* MMIO region                  */
>> +    MemoryRegion mr_sys;
>> +    MemoryRegion mr_nodma;
>>       uint8_t mmior[AMDVI_MMIO_SIZE];    /* read/write MMIO              */
>>       uint8_t w1cmask[AMDVI_MMIO_SIZE];  /* read/write 1 clear mask      */
>>       uint8_t romask[AMDVI_MMIO_SIZE];   /* MMIO read/only mask          */