[PATCH 0/3] amd_iommu: Support Generation of IOMMU XT interrupts

[PATCH 0/3] amd_iommu: Support Generation of IOMMU XT interrupts

[Sairaj Kodilkar]

AMD IOMMU uses MMIO registers 0x170-0x180 to generate the interrupts when guest
has enabled xt support through control register. The guest programs these
registers with appropriate vector and destination ID instead of writing to PCI
MSI capability.

Until now enabling the xt support through command line "xtsup=on" provided
support for 128 bit IRTE. But it has few limitations:

1. It does not consider if guest has actually enabled xt support through MMIO
   control register (0x18). This may cause problems for the guests which do
   not enable this support.
2. The vIOMMU is not capable of generating interrupts using vector and
   destinatio ID in IOMMU x2APIC Control Registers (not supporting event log
   interrupts).

To overcome above limitations, this patch series introduces new internal flag 
"intcapxten" which is set when guest writes "1" to MMIO control register (0x18)
bit 51 (IntCapXTEn) and adds support to generate event log interrupt using
vector and 32 bit destination ID in XT MMIO register 0x170.
-------------------------------------------------------------------------------

Changes since v1:
https://lore.kernel.org/qemu-devel/20251118082403.3455-1-sarunkod@amd.com/t/#m67401571075c42c26cc2560c94c1fc83836c9b20

Patch 1: Return string literals directly instead of copying [AJ]
Patch 2: 
    - Update commit message [AJ]
    - Introduce new subsection for migration compatibility [AJ]
    - Update comment [AJ]
Patch 3: Use ga_enabled flag while setting xten flag [AJ]


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

The patches are based on top of upstream qemu master (a524ba4ae14)

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

Sairaj Kodilkar (3):
  amd_iommu: Use switch case to determine mmio register name
  amd_iommu: Turn on XT support only when guest has enabled it
  amd_iommu: Generate XT interrupts when xt support is enabled

 hw/i386/amd_iommu.c  | 129 ++++++++++++++++++++++++++++---------------
 hw/i386/amd_iommu.h  |  21 ++++++-
 hw/i386/trace-events |   1 +
 3 files changed, 104 insertions(+), 47 deletions(-)

-- 
2.34.1

[PATCH v2 1/3] amd_iommu: Use switch case to determine mmio register name

[Sairaj Kodilkar]

This makes it easier to add new MMIO registers for tracing and removes
the unnecessary complexity introduced by amdvi_mmio_(low/high) array.

Signed-off-by: Sairaj Kodilkar <sarunkod@amd.com>
Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>
---
 hw/i386/amd_iommu.c | 65 +++++++++++++++++++--------------------------
 1 file changed, 27 insertions(+), 38 deletions(-)

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 789e09d6f2bc..62175cc366ac 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -35,28 +35,7 @@
 #include "kvm/kvm_i386.h"
 #include "qemu/iova-tree.h"
 
-/* used AMD-Vi MMIO registers */
-const char *amdvi_mmio_low[] = {
-    "AMDVI_MMIO_DEVTAB_BASE",
-    "AMDVI_MMIO_CMDBUF_BASE",
-    "AMDVI_MMIO_EVTLOG_BASE",
-    "AMDVI_MMIO_CONTROL",
-    "AMDVI_MMIO_EXCL_BASE",
-    "AMDVI_MMIO_EXCL_LIMIT",
-    "AMDVI_MMIO_EXT_FEATURES",
-    "AMDVI_MMIO_PPR_BASE",
-    "UNHANDLED"
-};
-const char *amdvi_mmio_high[] = {
-    "AMDVI_MMIO_COMMAND_HEAD",
-    "AMDVI_MMIO_COMMAND_TAIL",
-    "AMDVI_MMIO_EVTLOG_HEAD",
-    "AMDVI_MMIO_EVTLOG_TAIL",
-    "AMDVI_MMIO_STATUS",
-    "AMDVI_MMIO_PPR_HEAD",
-    "AMDVI_MMIO_PPR_TAIL",
-    "UNHANDLED"
-};
+#define MMIO_REG_TO_STRING(mmio_reg) case mmio_reg: return #mmio_reg
 
 struct AMDVIAddressSpace {
     PCIBus *bus;                /* PCIBus (for bus number)              */
@@ -1484,31 +1463,41 @@ static void amdvi_cmdbuf_run(AMDVIState *s)
     }
 }
 
-static inline uint8_t amdvi_mmio_get_index(hwaddr addr)
-{
-    uint8_t index = (addr & ~0x2000) / 8;
-
-    if ((addr & 0x2000)) {
-        /* high table */
-        index = index >= AMDVI_MMIO_REGS_HIGH ? AMDVI_MMIO_REGS_HIGH : index;
-    } else {
-        index = index >= AMDVI_MMIO_REGS_LOW ? AMDVI_MMIO_REGS_LOW : index;
+static inline
+const char *amdvi_mmio_get_name(hwaddr addr)
+{
+    /* Return MMIO names as string literals */
+    switch (addr) {
+    MMIO_REG_TO_STRING(AMDVI_MMIO_DEVICE_TABLE);
+    MMIO_REG_TO_STRING(AMDVI_MMIO_COMMAND_BASE);
+    MMIO_REG_TO_STRING(AMDVI_MMIO_EVENT_BASE);
+    MMIO_REG_TO_STRING(AMDVI_MMIO_CONTROL);
+    MMIO_REG_TO_STRING(AMDVI_MMIO_EXCL_BASE);
+    MMIO_REG_TO_STRING(AMDVI_MMIO_EXCL_LIMIT);
+    MMIO_REG_TO_STRING(AMDVI_MMIO_EXT_FEATURES);
+    MMIO_REG_TO_STRING(AMDVI_MMIO_COMMAND_HEAD);
+    MMIO_REG_TO_STRING(AMDVI_MMIO_COMMAND_TAIL);
+    MMIO_REG_TO_STRING(AMDVI_MMIO_EVENT_HEAD);
+    MMIO_REG_TO_STRING(AMDVI_MMIO_EVENT_TAIL);
+    MMIO_REG_TO_STRING(AMDVI_MMIO_STATUS);
+    MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_BASE);
+    MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_HEAD);
+    MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_TAIL);
+    default:
+        return "UNHANDLED";
     }
-
-    return index;
 }
 
 static void amdvi_mmio_trace_read(hwaddr addr, unsigned size)
 {
-    uint8_t index = amdvi_mmio_get_index(addr);
-    trace_amdvi_mmio_read(amdvi_mmio_low[index], addr, size, addr & ~0x07);
+    const char *mmio_name = amdvi_mmio_get_name(addr);
+    trace_amdvi_mmio_read(mmio_name, addr, size, addr & ~0x07);
 }
 
 static void amdvi_mmio_trace_write(hwaddr addr, unsigned size, uint64_t val)
 {
-    uint8_t index = amdvi_mmio_get_index(addr);
-    trace_amdvi_mmio_write(amdvi_mmio_low[index], addr, size, val,
-                           addr & ~0x07);
+    const char *mmio_name = amdvi_mmio_get_name(addr);
+    trace_amdvi_mmio_write(mmio_name, addr, size, val, addr & ~0x07);
 }
 
 static uint64_t amdvi_mmio_read(void *opaque, hwaddr addr, unsigned size)
-- 
2.34.1

[PATCH v2 2/3] amd_iommu: Turn on XT support only when guest has enabled it

[Sairaj Kodilkar]

Current code uses 32 bit cpu destination irrespective of the fact that
guest has enabled x2APIC support through control register[XTEn] and
completely depends on command line parameter xtsup=on. This is not a
correct hardware behaviour and can cause problems in the guest which has
not enabled XT mode.

Introduce new flag "xten", which is enabled when guest writes 1 to the
control register bit 50 (XTEn). Also, add a new subsection in
`VMStateDescription` for backward compatibility during vm migration.

Signed-off-by: Sairaj Kodilkar <sarunkod@amd.com>
Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>
---
 hw/i386/amd_iommu.c | 21 +++++++++++++++++++--
 hw/i386/amd_iommu.h |  4 +++-
 2 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 62175cc366ac..850d3920a76d 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -1535,6 +1535,8 @@ static void amdvi_handle_control_write(AMDVIState *s)
     s->cmdbuf_enabled = s->enabled && !!(control &
                         AMDVI_MMIO_CONTROL_CMDBUFLEN);
     s->ga_enabled = !!(control & AMDVI_MMIO_CONTROL_GAEN);
+    s->xten = !!(control & AMDVI_MMIO_CONTROL_XTEN) && s->xtsup &&
+              s->ga_enabled;
 
     /* update the flags depending on the control register */
     if (s->cmdbuf_enabled) {
@@ -2007,7 +2009,7 @@ static int amdvi_int_remap_ga(AMDVIState *iommu,
     irq->vector = irte.hi.fields.vector;
     irq->dest_mode = irte.lo.fields_remap.dm;
     irq->redir_hint = irte.lo.fields_remap.rq_eoi;
-    if (iommu->xtsup) {
+    if (iommu->xten) {
         irq->dest = irte.lo.fields_remap.destination |
                     (irte.hi.fields.destination_hi << 24);
     } else {
@@ -2390,6 +2392,7 @@ static void amdvi_init(AMDVIState *s)
     s->mmio_enabled = false;
     s->enabled = false;
     s->cmdbuf_enabled = false;
+    s->xten = false;
 
     /* reset MMIO */
     memset(s->mmior, 0, AMDVI_MMIO_SIZE);
@@ -2454,6 +2457,16 @@ static void amdvi_sysbus_reset(DeviceState *dev)
     amdvi_reset_address_translation_all(s);
 }
 
+static const VMStateDescription vmstate_xt = {
+       .name = "amd-iommu-xt",
+       .version_id = 1,
+       .minimum_version_id = 1,
+       .fields = (VMStateField[]) {
+           VMSTATE_BOOL(xten, AMDVIState),
+           VMSTATE_END_OF_LIST()
+       }
+};
+
 static const VMStateDescription vmstate_amdvi_sysbus_migratable = {
     .name = "amd-iommu",
     .version_id = 1,
@@ -2498,7 +2511,11 @@ static const VMStateDescription vmstate_amdvi_sysbus_migratable = {
       VMSTATE_UINT8_ARRAY(romask, AMDVIState, AMDVI_MMIO_SIZE),
       VMSTATE_UINT8_ARRAY(w1cmask, AMDVIState, AMDVI_MMIO_SIZE),
       VMSTATE_END_OF_LIST()
-    }
+    },
+    .subsections = (const VMStateDescription *const []) {
+                    &vmstate_xt,
+                    NULL
+     }
 };
 
 static void amdvi_sysbus_realize(DeviceState *dev, Error **errp)
diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
index 302ccca5121f..e9401f3a5c27 100644
--- a/hw/i386/amd_iommu.h
+++ b/hw/i386/amd_iommu.h
@@ -106,6 +106,7 @@
 #define AMDVI_MMIO_CONTROL_COMWAITINTEN   (1ULL << 4)
 #define AMDVI_MMIO_CONTROL_CMDBUFLEN      (1ULL << 12)
 #define AMDVI_MMIO_CONTROL_GAEN           (1ULL << 17)
+#define AMDVI_MMIO_CONTROL_XTEN           (1ULL << 50)
 
 /* MMIO status register bits */
 #define AMDVI_MMIO_STATUS_CMDBUF_RUN  (1 << 4)
@@ -418,7 +419,8 @@ struct AMDVIState {
 
     /* Interrupt remapping */
     bool ga_enabled;
-    bool xtsup;
+    bool xtsup;     /* xtsup=on command line */
+    bool xten;      /* guest controlled, x2apic mode enabled */
 
     /* DMA address translation */
     bool dma_remap;
-- 
2.34.1

[PATCH v2 3/3] amd_iommu: Generate XT interrupts when xt support is enabled

[Sairaj Kodilkar]

When MMIO 0x18[IntCapXTEn]=1, interrupts originating from the IOMMU itself are
sent based on the programming in XT IOMMU Interrupt Control Registers in MMIO
0x170-0x180 instead of the programming in the IOMMU's MSI capability registers.
The guest programs these registers with appropriate vector and destination
ID instead of writing to PCI MSI capability.

Current AMD vIOMMU is capable of generating interrupts only through PCI
MSI capability and does not care about xt mode. Because of this AMD
vIOMMU cannot generate event log interrupts using XT event log register
(0x170) when the guest has enabled xt mode.

Introduce a new flag "intcapxten" which is set when guest writes control
register [IntCapXTEn] (bit 51) and use vector and destination field in
the XT MMIO register (0x170) to support XT mode.

Signed-off-by: Sairaj Kodilkar <sarunkod@amd.com>
Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>
---
 hw/i386/amd_iommu.c  | 43 +++++++++++++++++++++++++++++++++++++------
 hw/i386/amd_iommu.h  | 17 +++++++++++++++++
 hw/i386/trace-events |  1 +
 3 files changed, 55 insertions(+), 6 deletions(-)

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 850d3920a76d..742ef5d42561 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -194,18 +194,38 @@ static void amdvi_assign_andq(AMDVIState *s, hwaddr addr, uint64_t val)
    amdvi_writeq_raw(s, addr, amdvi_readq(s, addr) & val);
 }
 
+static void amdvi_build_xt_msi_msg(AMDVIState *s, MSIMessage *msg)
+{
+    union mmio_xt_intr xt_reg;
+    struct X86IOMMUIrq irq;
+
+    xt_reg.val = amdvi_readq(s, AMDVI_MMIO_XT_GEN_INTR);
+
+    irq.vector = xt_reg.vector;
+    irq.delivery_mode = xt_reg.delivery_mode;
+    irq.dest_mode = xt_reg.destination_mode;
+    irq.dest = (xt_reg.destination_hi << 24) | xt_reg.destination_lo;
+    irq.trigger_mode = 0;
+    irq.redir_hint = 0;
+
+    x86_iommu_irq_to_msi_message(&irq, msg);
+}
+
 static void amdvi_generate_msi_interrupt(AMDVIState *s)
 {
     MSIMessage msg = {};
-    MemTxAttrs attrs = {
-        .requester_id = pci_requester_id(&s->pci->dev)
-    };
 
-    if (msi_enabled(&s->pci->dev)) {
+    if (s->intcapxten) {
+        trace_amdvi_generate_msi_interrupt("XT GEN");
+        amdvi_build_xt_msi_msg(s, &msg);
+    } else if (msi_enabled(&s->pci->dev)) {
+        trace_amdvi_generate_msi_interrupt("MSI");
         msg = msi_get_message(&s->pci->dev, 0);
-        address_space_stl_le(&address_space_memory, msg.address, msg.data,
-                             attrs, NULL);
+    } else {
+        trace_amdvi_generate_msi_interrupt("NO MSI");
+        return;
     }
+    apic_get_class(NULL)->send_msi(&msg);
 }
 
 static uint32_t get_next_eventlog_entry(AMDVIState *s)
@@ -1483,6 +1503,7 @@ const char *amdvi_mmio_get_name(hwaddr addr)
     MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_BASE);
     MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_HEAD);
     MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_TAIL);
+    MMIO_REG_TO_STRING(AMDVI_MMIO_XT_GEN_INTR);
     default:
         return "UNHANDLED";
     }
@@ -1537,6 +1558,11 @@ static void amdvi_handle_control_write(AMDVIState *s)
     s->ga_enabled = !!(control & AMDVI_MMIO_CONTROL_GAEN);
     s->xten = !!(control & AMDVI_MMIO_CONTROL_XTEN) && s->xtsup &&
               s->ga_enabled;
+    /*
+     * intcapxten does not depend on xten flag because IOMMU spec does not
+     * specify any dependency between these two flags
+     */
+    s->intcapxten = !!(control & AMDVI_MMIO_CONTROL_INTCAPXTEN) && s->xtsup;
 
     /* update the flags depending on the control register */
     if (s->cmdbuf_enabled) {
@@ -1743,6 +1769,9 @@ static void amdvi_mmio_write(void *opaque, hwaddr addr, uint64_t val,
     case AMDVI_MMIO_STATUS:
         amdvi_mmio_reg_write(s, size, val, addr);
         break;
+    case AMDVI_MMIO_XT_GEN_INTR:
+        amdvi_mmio_reg_write(s, size, val, addr);
+        break;
     }
 }
 
@@ -2393,6 +2422,7 @@ static void amdvi_init(AMDVIState *s)
     s->enabled = false;
     s->cmdbuf_enabled = false;
     s->xten = false;
+    s->intcapxten = false;
 
     /* reset MMIO */
     memset(s->mmior, 0, AMDVI_MMIO_SIZE);
@@ -2463,6 +2493,7 @@ static const VMStateDescription vmstate_xt = {
        .minimum_version_id = 1,
        .fields = (VMStateField[]) {
            VMSTATE_BOOL(xten, AMDVIState),
+           VMSTATE_BOOL(intcapxten, AMDVIState),
            VMSTATE_END_OF_LIST()
        }
 };
diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
index e9401f3a5c27..886814770276 100644
--- a/hw/i386/amd_iommu.h
+++ b/hw/i386/amd_iommu.h
@@ -57,6 +57,7 @@
 #define AMDVI_MMIO_EXCL_BASE          0x0020
 #define AMDVI_MMIO_EXCL_LIMIT         0x0028
 #define AMDVI_MMIO_EXT_FEATURES       0x0030
+#define AMDVI_MMIO_XT_GEN_INTR        0x0170
 #define AMDVI_MMIO_COMMAND_HEAD       0x2000
 #define AMDVI_MMIO_COMMAND_TAIL       0x2008
 #define AMDVI_MMIO_EVENT_HEAD         0x2010
@@ -107,6 +108,7 @@
 #define AMDVI_MMIO_CONTROL_CMDBUFLEN      (1ULL << 12)
 #define AMDVI_MMIO_CONTROL_GAEN           (1ULL << 17)
 #define AMDVI_MMIO_CONTROL_XTEN           (1ULL << 50)
+#define AMDVI_MMIO_CONTROL_INTCAPXTEN     (1ULL << 51)
 
 /* MMIO status register bits */
 #define AMDVI_MMIO_STATUS_CMDBUF_RUN  (1 << 4)
@@ -343,6 +345,20 @@ struct irte_ga {
   union irte_ga_hi hi;
 };
 
+union mmio_xt_intr {
+    uint64_t val;
+    struct {
+        uint64_t rsvd_1:2,
+                 destination_mode:1,
+                 rsvd_2:5,
+                 destination_lo:24,
+                 vector:8,
+                 delivery_mode:1,
+                 rsvd_3:15,
+                 destination_hi:8;
+    };
+};
+
 #define TYPE_AMD_IOMMU_DEVICE "amd-iommu"
 OBJECT_DECLARE_SIMPLE_TYPE(AMDVIState, AMD_IOMMU_DEVICE)
 
@@ -421,6 +437,7 @@ struct AMDVIState {
     bool ga_enabled;
     bool xtsup;     /* xtsup=on command line */
     bool xten;      /* guest controlled, x2apic mode enabled */
+    bool intcapxten; /* guest controlled, IOMMU x2apic interrupts enabled */
 
     /* DMA address translation */
     bool dma_remap;
diff --git a/hw/i386/trace-events b/hw/i386/trace-events
index 5fa5e93b68dc..a1dfade20f18 100644
--- a/hw/i386/trace-events
+++ b/hw/i386/trace-events
@@ -118,6 +118,7 @@ amdvi_ir_intctl(uint8_t val) "int_ctl 0x%"PRIx8
 amdvi_ir_target_abort(const char *str) "%s"
 amdvi_ir_delivery_mode(const char *str) "%s"
 amdvi_ir_irte_ga_val(uint64_t hi, uint64_t lo) "hi 0x%"PRIx64" lo 0x%"PRIx64
+amdvi_generate_msi_interrupt(const char *str) "Mode: %s"
 
 # vmport.c
 vmport_register(unsigned char command, void *func, void *opaque) "command: 0x%02x func: %p opaque: %p"
-- 
2.34.1

Re: [PATCH v2 1/3] amd_iommu: Use switch case to determine mmio register name

[Alejandro Jimenez]

On 1/29/26 5:28 AM, Sairaj Kodilkar wrote:
> This makes it easier to add new MMIO registers for tracing and removes
> the unnecessary complexity introduced by amdvi_mmio_(low/high) array.
> 
> Signed-off-by: Sairaj Kodilkar <sarunkod@amd.com>
> Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  hw/i386/amd_iommu.c | 65 +++++++++++++++++++--------------------------
>  1 file changed, 27 insertions(+), 38 deletions(-)
> 

I'd like to also remove the unused AMDVI_MMIO_REGS_{LOW,HIGH} definitions
and amdvi_mmio_trace_{read,write} helpers as I did on the example diff in:

https://lore.kernel.org/qemu-devel/eaf49cf3-e56b-40f1-974d-207969c7371e@oracle.com/

assuming you agree with it, no need to send a new revision, I can add those
changes to the current patch.

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

> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index 789e09d6f2bc..62175cc366ac 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -35,28 +35,7 @@
>  #include "kvm/kvm_i386.h"
>  #include "qemu/iova-tree.h"
>  
> -/* used AMD-Vi MMIO registers */
> -const char *amdvi_mmio_low[] = {
> -    "AMDVI_MMIO_DEVTAB_BASE",
> -    "AMDVI_MMIO_CMDBUF_BASE",
> -    "AMDVI_MMIO_EVTLOG_BASE",
> -    "AMDVI_MMIO_CONTROL",
> -    "AMDVI_MMIO_EXCL_BASE",
> -    "AMDVI_MMIO_EXCL_LIMIT",
> -    "AMDVI_MMIO_EXT_FEATURES",
> -    "AMDVI_MMIO_PPR_BASE",
> -    "UNHANDLED"
> -};
> -const char *amdvi_mmio_high[] = {
> -    "AMDVI_MMIO_COMMAND_HEAD",
> -    "AMDVI_MMIO_COMMAND_TAIL",
> -    "AMDVI_MMIO_EVTLOG_HEAD",
> -    "AMDVI_MMIO_EVTLOG_TAIL",
> -    "AMDVI_MMIO_STATUS",
> -    "AMDVI_MMIO_PPR_HEAD",
> -    "AMDVI_MMIO_PPR_TAIL",
> -    "UNHANDLED"
> -};
> +#define MMIO_REG_TO_STRING(mmio_reg) case mmio_reg: return #mmio_reg
>  
>  struct AMDVIAddressSpace {
>      PCIBus *bus;                /* PCIBus (for bus number)              */
> @@ -1484,31 +1463,41 @@ static void amdvi_cmdbuf_run(AMDVIState *s)
>      }
>  }
>  
> -static inline uint8_t amdvi_mmio_get_index(hwaddr addr)
> -{
> -    uint8_t index = (addr & ~0x2000) / 8;
> -
> -    if ((addr & 0x2000)) {
> -        /* high table */
> -        index = index >= AMDVI_MMIO_REGS_HIGH ? AMDVI_MMIO_REGS_HIGH : index;
> -    } else {
> -        index = index >= AMDVI_MMIO_REGS_LOW ? AMDVI_MMIO_REGS_LOW : index;
> +static inline
> +const char *amdvi_mmio_get_name(hwaddr addr)
> +{
> +    /* Return MMIO names as string literals */
> +    switch (addr) {
> +    MMIO_REG_TO_STRING(AMDVI_MMIO_DEVICE_TABLE);
> +    MMIO_REG_TO_STRING(AMDVI_MMIO_COMMAND_BASE);
> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EVENT_BASE);
> +    MMIO_REG_TO_STRING(AMDVI_MMIO_CONTROL);
> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EXCL_BASE);
> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EXCL_LIMIT);
> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EXT_FEATURES);
> +    MMIO_REG_TO_STRING(AMDVI_MMIO_COMMAND_HEAD);
> +    MMIO_REG_TO_STRING(AMDVI_MMIO_COMMAND_TAIL);
> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EVENT_HEAD);
> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EVENT_TAIL);
> +    MMIO_REG_TO_STRING(AMDVI_MMIO_STATUS);
> +    MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_BASE);
> +    MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_HEAD);
> +    MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_TAIL);
> +    default:
> +        return "UNHANDLED";
>      }
> -
> -    return index;
>  }
>  
>  static void amdvi_mmio_trace_read(hwaddr addr, unsigned size)
>  {
> -    uint8_t index = amdvi_mmio_get_index(addr);
> -    trace_amdvi_mmio_read(amdvi_mmio_low[index], addr, size, addr & ~0x07);
> +    const char *mmio_name = amdvi_mmio_get_name(addr);
> +    trace_amdvi_mmio_read(mmio_name, addr, size, addr & ~0x07);
>  }
>  
>  static void amdvi_mmio_trace_write(hwaddr addr, unsigned size, uint64_t val)
>  {
> -    uint8_t index = amdvi_mmio_get_index(addr);
> -    trace_amdvi_mmio_write(amdvi_mmio_low[index], addr, size, val,
> -                           addr & ~0x07);
> +    const char *mmio_name = amdvi_mmio_get_name(addr);
> +    trace_amdvi_mmio_write(mmio_name, addr, size, val, addr & ~0x07);
>  }
>  
>  static uint64_t amdvi_mmio_read(void *opaque, hwaddr addr, unsigned size)

Re: [PATCH v2 1/3] amd_iommu: Use switch case to determine mmio register name

[CLEMENT MATHIEU--DRIF]

On Thu, 2026-01-29 at 15:58 +0530, Sairaj Kodilkar wrote:
> This makes it easier to add new MMIO registers for tracing and removes  
> the unnecessary complexity introduced by amdvi_mmio_(low/high) array.
> 
> Signed-off-by: Sairaj Kodilkar <[sarunkod@amd.com](mailto:sarunkod@amd.com)>  
> Reviewed-by: Vasant Hegde <[vasant.hegde@amd.com](mailto:vasant.hegde@amd.com)>  
> ---  
>  hw/i386/amd_iommu.c | 65 +++++++++++++++++++--------------------------  
>  1 file changed, 27 insertions(+), 38 deletions(-)
> 
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c  
> index 789e09d6f2bc..62175cc366ac 100644  
> --- a/hw/i386/amd_iommu.c  
> +++ b/hw/i386/amd_iommu.c  
> @@ -35,28 +35,7 @@  
>  #include "kvm/kvm_i386.h"  
>  #include "qemu/iova-tree.h"  
>    
> -/* used AMD-Vi MMIO registers */  
> -const char *amdvi_mmio_low[] = {  
> -    "AMDVI_MMIO_DEVTAB_BASE",  
> -    "AMDVI_MMIO_CMDBUF_BASE",  
> -    "AMDVI_MMIO_EVTLOG_BASE",  
> -    "AMDVI_MMIO_CONTROL",  
> -    "AMDVI_MMIO_EXCL_BASE",  
> -    "AMDVI_MMIO_EXCL_LIMIT",  
> -    "AMDVI_MMIO_EXT_FEATURES",  
> -    "AMDVI_MMIO_PPR_BASE",  
> -    "UNHANDLED"  
> -};  
> -const char *amdvi_mmio_high[] = {  
> -    "AMDVI_MMIO_COMMAND_HEAD",  
> -    "AMDVI_MMIO_COMMAND_TAIL",  
> -    "AMDVI_MMIO_EVTLOG_HEAD",  
> -    "AMDVI_MMIO_EVTLOG_TAIL",  
> -    "AMDVI_MMIO_STATUS",  
> -    "AMDVI_MMIO_PPR_HEAD",  
> -    "AMDVI_MMIO_PPR_TAIL",  
> -    "UNHANDLED"  
> -};  
> +#define MMIO_REG_TO_STRING(mmio_reg) case mmio_reg: return #mmio_reg  

Hi Sairaj,

Shouldn't we define this inside the mmio_get_name function and undef it after the return statement?  
I think it would be cleanup to make the scope of this a bit smaller as it is specifically written for this function.  

>    
>  struct AMDVIAddressSpace {  
>      PCIBus *bus;                /* PCIBus (for bus number)              */  
> @@ -1484,31 +1463,41 @@ static void amdvi_cmdbuf_run(AMDVIState *s)  
>      }  
>  }  
>    
> -static inline uint8_t amdvi_mmio_get_index(hwaddr addr)  
> -{  
> -    uint8_t index = (addr & ~0x2000) / 8;  
> -  
> -    if ((addr & 0x2000)) {  
> -        /* high table */  
> -        index = index >= AMDVI_MMIO_REGS_HIGH ? AMDVI_MMIO_REGS_HIGH : index;  
> -    } else {  
> -        index = index >= AMDVI_MMIO_REGS_LOW ? AMDVI_MMIO_REGS_LOW : index;  
> +static inline  
> +const char *amdvi_mmio_get_name(hwaddr addr)  
> +{  
> +    /* Return MMIO names as string literals */  
> +    switch (addr) {  
> +    MMIO_REG_TO_STRING(AMDVI_MMIO_DEVICE_TABLE);  
> +    MMIO_REG_TO_STRING(AMDVI_MMIO_COMMAND_BASE);  
> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EVENT_BASE);  
> +    MMIO_REG_TO_STRING(AMDVI_MMIO_CONTROL);  
> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EXCL_BASE);  
> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EXCL_LIMIT);  
> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EXT_FEATURES);  
> +    MMIO_REG_TO_STRING(AMDVI_MMIO_COMMAND_HEAD);  
> +    MMIO_REG_TO_STRING(AMDVI_MMIO_COMMAND_TAIL);  
> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EVENT_HEAD);  
> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EVENT_TAIL);  
> +    MMIO_REG_TO_STRING(AMDVI_MMIO_STATUS);  
> +    MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_BASE);  
> +    MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_HEAD);  
> +    MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_TAIL);  
> +    default:  
> +        return "UNHANDLED";  
>      }  
> -  
> -    return index;  
>  }  
>    
>  static void amdvi_mmio_trace_read(hwaddr addr, unsigned size)  
>  {  
> -    uint8_t index = amdvi_mmio_get_index(addr);  
> -    trace_amdvi_mmio_read(amdvi_mmio_low[index], addr, size, addr & ~0x07);  
> +    const char *mmio_name = amdvi_mmio_get_name(addr);  
> +    trace_amdvi_mmio_read(mmio_name, addr, size, addr & ~0x07);  
>  }  
>    
>  static void amdvi_mmio_trace_write(hwaddr addr, unsigned size, uint64_t val)  
>  {  
> -    uint8_t index = amdvi_mmio_get_index(addr);  
> -    trace_amdvi_mmio_write(amdvi_mmio_low[index], addr, size, val,  
> -                           addr & ~0x07);  
> +    const char *mmio_name = amdvi_mmio_get_name(addr);  
> +    trace_amdvi_mmio_write(mmio_name, addr, size, val, addr & ~0x07);  
>  }  
>    
>  static uint64_t amdvi_mmio_read(void *opaque, hwaddr addr, unsigned size)

Re: [PATCH v2 1/3] amd_iommu: Use switch case to determine mmio register name

[Sairaj Kodilkar]

On 1/30/2026 1:09 PM, CLEMENT MATHIEU--DRIF wrote:
>
> On Thu, 2026-01-29 at 15:58 +0530, Sairaj Kodilkar wrote:
>> This makes it easier to add new MMIO registers for tracing and removes
>> the unnecessary complexity introduced by amdvi_mmio_(low/high) array.
>>
>> Signed-off-by: Sairaj Kodilkar <[sarunkod@amd.com](mailto:sarunkod@amd.com)>
>> Reviewed-by: Vasant Hegde <[vasant.hegde@amd.com](mailto:vasant.hegde@amd.com)>
>> ---
>>   hw/i386/amd_iommu.c | 65 +++++++++++++++++++--------------------------
>>   1 file changed, 27 insertions(+), 38 deletions(-)
>>
>> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
>> index 789e09d6f2bc..62175cc366ac 100644
>> --- a/hw/i386/amd_iommu.c
>> +++ b/hw/i386/amd_iommu.c
>> @@ -35,28 +35,7 @@
>>   #include "kvm/kvm_i386.h"
>>   #include "qemu/iova-tree.h"
>>     
>> -/* used AMD-Vi MMIO registers */
>> -const char *amdvi_mmio_low[] = {
>> -    "AMDVI_MMIO_DEVTAB_BASE",
>> -    "AMDVI_MMIO_CMDBUF_BASE",
>> -    "AMDVI_MMIO_EVTLOG_BASE",
>> -    "AMDVI_MMIO_CONTROL",
>> -    "AMDVI_MMIO_EXCL_BASE",
>> -    "AMDVI_MMIO_EXCL_LIMIT",
>> -    "AMDVI_MMIO_EXT_FEATURES",
>> -    "AMDVI_MMIO_PPR_BASE",
>> -    "UNHANDLED"
>> -};
>> -const char *amdvi_mmio_high[] = {
>> -    "AMDVI_MMIO_COMMAND_HEAD",
>> -    "AMDVI_MMIO_COMMAND_TAIL",
>> -    "AMDVI_MMIO_EVTLOG_HEAD",
>> -    "AMDVI_MMIO_EVTLOG_TAIL",
>> -    "AMDVI_MMIO_STATUS",
>> -    "AMDVI_MMIO_PPR_HEAD",
>> -    "AMDVI_MMIO_PPR_TAIL",
>> -    "UNHANDLED"
>> -};
>> +#define MMIO_REG_TO_STRING(mmio_reg) case mmio_reg: return #mmio_reg
> Hi Sairaj,
>
> Shouldn't we define this inside the mmio_get_name function and undef it after the return statement?
> I think it would be cleanup to make the scope of this a bit smaller as it is specifically written for this function.

Hi
I think this is probably okay as its unlikely to cause any issues in future.

Thanks
-Sairaj

>
>>     
>>   struct AMDVIAddressSpace {
>>       PCIBus *bus;                /* PCIBus (for bus number)              */
>> @@ -1484,31 +1463,41 @@ static void amdvi_cmdbuf_run(AMDVIState *s)
>>       }
>>   }
>>     
>> -static inline uint8_t amdvi_mmio_get_index(hwaddr addr)
>> -{
>> -    uint8_t index = (addr & ~0x2000) / 8;
>> -
>> -    if ((addr & 0x2000)) {
>> -        /* high table */
>> -        index = index >= AMDVI_MMIO_REGS_HIGH ? AMDVI_MMIO_REGS_HIGH : index;
>> -    } else {
>> -        index = index >= AMDVI_MMIO_REGS_LOW ? AMDVI_MMIO_REGS_LOW : index;
>> +static inline
>> +const char *amdvi_mmio_get_name(hwaddr addr)
>> +{
>> +    /* Return MMIO names as string literals */
>> +    switch (addr) {
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_DEVICE_TABLE);
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_COMMAND_BASE);
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EVENT_BASE);
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_CONTROL);
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EXCL_BASE);
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EXCL_LIMIT);
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EXT_FEATURES);
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_COMMAND_HEAD);
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_COMMAND_TAIL);
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EVENT_HEAD);
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EVENT_TAIL);
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_STATUS);
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_BASE);
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_HEAD);
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_TAIL);
>> +    default:
>> +        return "UNHANDLED";
>>       }
>> -
>> -    return index;
>>   }
>>     
>>   static void amdvi_mmio_trace_read(hwaddr addr, unsigned size)
>>   {
>> -    uint8_t index = amdvi_mmio_get_index(addr);
>> -    trace_amdvi_mmio_read(amdvi_mmio_low[index], addr, size, addr & ~0x07);
>> +    const char *mmio_name = amdvi_mmio_get_name(addr);
>> +    trace_amdvi_mmio_read(mmio_name, addr, size, addr & ~0x07);
>>   }
>>     
>>   static void amdvi_mmio_trace_write(hwaddr addr, unsigned size, uint64_t val)
>>   {
>> -    uint8_t index = amdvi_mmio_get_index(addr);
>> -    trace_amdvi_mmio_write(amdvi_mmio_low[index], addr, size, val,
>> -                           addr & ~0x07);
>> +    const char *mmio_name = amdvi_mmio_get_name(addr);
>> +    trace_amdvi_mmio_write(mmio_name, addr, size, val, addr & ~0x07);
>>   }
>>     
>>   static uint64_t amdvi_mmio_read(void *opaque, hwaddr addr, unsigned size)

Re: [PATCH v2 1/3] amd_iommu: Use switch case to determine mmio register name

[Sairaj Kodilkar]

On 1/30/2026 3:20 AM, Alejandro Jimenez wrote:
>
> On 1/29/26 5:28 AM, Sairaj Kodilkar wrote:
>> This makes it easier to add new MMIO registers for tracing and removes
>> the unnecessary complexity introduced by amdvi_mmio_(low/high) array.
>>
>> Signed-off-by: Sairaj Kodilkar <sarunkod@amd.com>
>> Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>
>> ---
>>   hw/i386/amd_iommu.c | 65 +++++++++++++++++++--------------------------
>>   1 file changed, 27 insertions(+), 38 deletions(-)
>>
> I'd like to also remove the unused AMDVI_MMIO_REGS_{LOW,HIGH} definitions
> and amdvi_mmio_trace_{read,write} helpers as I did on the example diff in:
>
> https://lore.kernel.org/qemu-devel/eaf49cf3-e56b-40f1-974d-207969c7371e@oracle.com/
>
> assuming you agree with it, no need to send a new revision, I can add those
> changes to the current patch.

Sure go ahead,

Thanks
Sairaj

>
> Otherwise:
> Reviewed-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
>
>> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
>> index 789e09d6f2bc..62175cc366ac 100644
>> --- a/hw/i386/amd_iommu.c
>> +++ b/hw/i386/amd_iommu.c
>> @@ -35,28 +35,7 @@
>>   #include "kvm/kvm_i386.h"
>>   #include "qemu/iova-tree.h"
>>   
>> -/* used AMD-Vi MMIO registers */
>> -const char *amdvi_mmio_low[] = {
>> -    "AMDVI_MMIO_DEVTAB_BASE",
>> -    "AMDVI_MMIO_CMDBUF_BASE",
>> -    "AMDVI_MMIO_EVTLOG_BASE",
>> -    "AMDVI_MMIO_CONTROL",
>> -    "AMDVI_MMIO_EXCL_BASE",
>> -    "AMDVI_MMIO_EXCL_LIMIT",
>> -    "AMDVI_MMIO_EXT_FEATURES",
>> -    "AMDVI_MMIO_PPR_BASE",
>> -    "UNHANDLED"
>> -};
>> -const char *amdvi_mmio_high[] = {
>> -    "AMDVI_MMIO_COMMAND_HEAD",
>> -    "AMDVI_MMIO_COMMAND_TAIL",
>> -    "AMDVI_MMIO_EVTLOG_HEAD",
>> -    "AMDVI_MMIO_EVTLOG_TAIL",
>> -    "AMDVI_MMIO_STATUS",
>> -    "AMDVI_MMIO_PPR_HEAD",
>> -    "AMDVI_MMIO_PPR_TAIL",
>> -    "UNHANDLED"
>> -};
>> +#define MMIO_REG_TO_STRING(mmio_reg) case mmio_reg: return #mmio_reg
>>   
>>   struct AMDVIAddressSpace {
>>       PCIBus *bus;                /* PCIBus (for bus number)              */
>> @@ -1484,31 +1463,41 @@ static void amdvi_cmdbuf_run(AMDVIState *s)
>>       }
>>   }
>>   
>> -static inline uint8_t amdvi_mmio_get_index(hwaddr addr)
>> -{
>> -    uint8_t index = (addr & ~0x2000) / 8;
>> -
>> -    if ((addr & 0x2000)) {
>> -        /* high table */
>> -        index = index >= AMDVI_MMIO_REGS_HIGH ? AMDVI_MMIO_REGS_HIGH : index;
>> -    } else {
>> -        index = index >= AMDVI_MMIO_REGS_LOW ? AMDVI_MMIO_REGS_LOW : index;
>> +static inline
>> +const char *amdvi_mmio_get_name(hwaddr addr)
>> +{
>> +    /* Return MMIO names as string literals */
>> +    switch (addr) {
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_DEVICE_TABLE);
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_COMMAND_BASE);
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EVENT_BASE);
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_CONTROL);
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EXCL_BASE);
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EXCL_LIMIT);
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EXT_FEATURES);
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_COMMAND_HEAD);
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_COMMAND_TAIL);
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EVENT_HEAD);
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EVENT_TAIL);
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_STATUS);
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_BASE);
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_HEAD);
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_TAIL);
>> +    default:
>> +        return "UNHANDLED";
>>       }
>> -
>> -    return index;
>>   }
>>   
>>   static void amdvi_mmio_trace_read(hwaddr addr, unsigned size)
>>   {
>> -    uint8_t index = amdvi_mmio_get_index(addr);
>> -    trace_amdvi_mmio_read(amdvi_mmio_low[index], addr, size, addr & ~0x07);
>> +    const char *mmio_name = amdvi_mmio_get_name(addr);
>> +    trace_amdvi_mmio_read(mmio_name, addr, size, addr & ~0x07);
>>   }
>>   
>>   static void amdvi_mmio_trace_write(hwaddr addr, unsigned size, uint64_t val)
>>   {
>> -    uint8_t index = amdvi_mmio_get_index(addr);
>> -    trace_amdvi_mmio_write(amdvi_mmio_low[index], addr, size, val,
>> -                           addr & ~0x07);
>> +    const char *mmio_name = amdvi_mmio_get_name(addr);
>> +    trace_amdvi_mmio_write(mmio_name, addr, size, val, addr & ~0x07);
>>   }
>>   
>>   static uint64_t amdvi_mmio_read(void *opaque, hwaddr addr, unsigned size)

Re: [PATCH v2 1/3] amd_iommu: Use switch case to determine mmio register name

[Alejandro Jimenez]

On 1/30/26 3:04 AM, Sairaj Kodilkar wrote:
> 
> 
> On 1/30/2026 1:09 PM, CLEMENT MATHIEU--DRIF wrote:
>>
>> On Thu, 2026-01-29 at 15:58 +0530, Sairaj Kodilkar wrote:
>>> This makes it easier to add new MMIO registers for tracing and removes
>>> the unnecessary complexity introduced by amdvi_mmio_(low/high) array.
>>>
>>> Signed-off-by: Sairaj Kodilkar <[sarunkod@amd.com]
>>> (mailto:sarunkod@amd.com)>
>>> Reviewed-by: Vasant Hegde <[vasant.hegde@amd.com]
>>> (mailto:vasant.hegde@amd.com)>
>>> ---
>>>   hw/i386/amd_iommu.c | 65 +++++++++++++++++++--------------------------
>>>   1 file changed, 27 insertions(+), 38 deletions(-)
>>>
>>> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
>>> index 789e09d6f2bc..62175cc366ac 100644
>>> --- a/hw/i386/amd_iommu.c
>>> +++ b/hw/i386/amd_iommu.c
>>> @@ -35,28 +35,7 @@
>>>   #include "kvm/kvm_i386.h"
>>>   #include "qemu/iova-tree.h"
>>>     -/* used AMD-Vi MMIO registers */
>>> -const char *amdvi_mmio_low[] = {
>>> -    "AMDVI_MMIO_DEVTAB_BASE",
>>> -    "AMDVI_MMIO_CMDBUF_BASE",
>>> -    "AMDVI_MMIO_EVTLOG_BASE",
>>> -    "AMDVI_MMIO_CONTROL",
>>> -    "AMDVI_MMIO_EXCL_BASE",
>>> -    "AMDVI_MMIO_EXCL_LIMIT",
>>> -    "AMDVI_MMIO_EXT_FEATURES",
>>> -    "AMDVI_MMIO_PPR_BASE",
>>> -    "UNHANDLED"
>>> -};
>>> -const char *amdvi_mmio_high[] = {
>>> -    "AMDVI_MMIO_COMMAND_HEAD",
>>> -    "AMDVI_MMIO_COMMAND_TAIL",
>>> -    "AMDVI_MMIO_EVTLOG_HEAD",
>>> -    "AMDVI_MMIO_EVTLOG_TAIL",
>>> -    "AMDVI_MMIO_STATUS",
>>> -    "AMDVI_MMIO_PPR_HEAD",
>>> -    "AMDVI_MMIO_PPR_TAIL",
>>> -    "UNHANDLED"
>>> -};
>>> +#define MMIO_REG_TO_STRING(mmio_reg) case mmio_reg: return #mmio_reg
>> Hi Sairaj,
>>
>> Shouldn't we define this inside the mmio_get_name function and undef it
>> after the return statement?
>> I think it would be cleanup to make the scope of this a bit smaller as it
>> is specifically written for this function.
> 

I agree with the above. I think it is a good idea given the ad-hoc nature
of this macro to keep the definition and its usage together and undef it
right after to avoid any confusion.

I was a bit reluctant when I proposed the macro because it affects control
flow (the kernel coding style frowns on that even if QEMU doesn't
explicitly forbids it), but I think this is clean and easy to parse:

static inline
const char *amdvi_mmio_get_name(hwaddr addr)
{
    /* Return MMIO names as string literals */
    switch (addr) {
#define MMIO_REG_TO_STRING(mmio_reg) case mmio_reg: return #mmio_reg
    MMIO_REG_TO_STRING(AMDVI_MMIO_DEVICE_TABLE);

[...]
    MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_TAIL);
#undef MMIO_REG_TO_STRING
    default:
        return "UNHANDLED";
    }
}

Sairaj: if you don't want to sign off on this specific pattern, I can send
a patch for it and we can review it separately.

Alejandro

> Hi
> I think this is probably okay as its unlikely to cause any issues in future.
> 
> Thanks
> -Sairaj
> 
>>
>>>       struct AMDVIAddressSpace {
>>>       PCIBus *bus;                /* PCIBus (for bus
>>> number)              */
>>> @@ -1484,31 +1463,41 @@ static void amdvi_cmdbuf_run(AMDVIState *s)
>>>       }
>>>   }
>>>     -static inline uint8_t amdvi_mmio_get_index(hwaddr addr)
>>> -{
>>> -    uint8_t index = (addr & ~0x2000) / 8;
>>> -
>>> -    if ((addr & 0x2000)) {
>>> -        /* high table */
>>> -        index = index >= AMDVI_MMIO_REGS_HIGH ? AMDVI_MMIO_REGS_HIGH :
>>> index;
>>> -    } else {
>>> -        index = index >= AMDVI_MMIO_REGS_LOW ? AMDVI_MMIO_REGS_LOW :
>>> index;
>>> +static inline
>>> +const char *amdvi_mmio_get_name(hwaddr addr)
>>> +{
>>> +    /* Return MMIO names as string literals */
>>> +    switch (addr) {
>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_DEVICE_TABLE);
>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_COMMAND_BASE);
>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EVENT_BASE);
>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_CONTROL);
>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EXCL_BASE);
>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EXCL_LIMIT);
>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EXT_FEATURES);
>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_COMMAND_HEAD);
>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_COMMAND_TAIL);
>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EVENT_HEAD);
>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EVENT_TAIL);
>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_STATUS);
>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_BASE);
>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_HEAD);
>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_TAIL);
>>> +    default:
>>> +        return "UNHANDLED";
>>>       }
>>> -
>>> -    return index;
>>>   }
>>>       static void amdvi_mmio_trace_read(hwaddr addr, unsigned size)
>>>   {
>>> -    uint8_t index = amdvi_mmio_get_index(addr);
>>> -    trace_amdvi_mmio_read(amdvi_mmio_low[index], addr, size, addr &
>>> ~0x07);
>>> +    const char *mmio_name = amdvi_mmio_get_name(addr);
>>> +    trace_amdvi_mmio_read(mmio_name, addr, size, addr & ~0x07);
>>>   }
>>>       static void amdvi_mmio_trace_write(hwaddr addr, unsigned size,
>>> uint64_t val)
>>>   {
>>> -    uint8_t index = amdvi_mmio_get_index(addr);
>>> -    trace_amdvi_mmio_write(amdvi_mmio_low[index], addr, size, val,
>>> -                           addr & ~0x07);
>>> +    const char *mmio_name = amdvi_mmio_get_name(addr);
>>> +    trace_amdvi_mmio_write(mmio_name, addr, size, val, addr & ~0x07);
>>>   }
>>>       static uint64_t amdvi_mmio_read(void *opaque, hwaddr addr,
>>> unsigned size)
> 

Re: [PATCH v2 2/3] amd_iommu: Turn on XT support only when guest has enabled it

[Alejandro Jimenez]

One minor nit that I mentioned on v1:

On 1/29/26 5:28 AM, Sairaj Kodilkar wrote:
> Current code uses 32 bit cpu destination irrespective of the fact that

s/"32 bit cpu destination"/"32-bit destination ID"

> guest has enabled x2APIC support through control register[XTEn] and
> completely depends on command line parameter xtsup=on. This is not a
> correct hardware behaviour and can cause problems in the guest which has
> not enabled XT mode.
> 
> Introduce new flag "xten", which is enabled when guest writes 1 to the
> control register bit 50 (XTEn). Also, add a new subsection in
> `VMStateDescription` for backward compatibility during vm migration.
> 
> Signed-off-by: Sairaj Kodilkar <sarunkod@amd.com>
> Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>

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

> ---
>  hw/i386/amd_iommu.c | 21 +++++++++++++++++++--
>  hw/i386/amd_iommu.h |  4 +++-
>  2 files changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index 62175cc366ac..850d3920a76d 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -1535,6 +1535,8 @@ static void amdvi_handle_control_write(AMDVIState *s)
>      s->cmdbuf_enabled = s->enabled && !!(control &
>                          AMDVI_MMIO_CONTROL_CMDBUFLEN);
>      s->ga_enabled = !!(control & AMDVI_MMIO_CONTROL_GAEN);
> +    s->xten = !!(control & AMDVI_MMIO_CONTROL_XTEN) && s->xtsup &&
> +              s->ga_enabled;
>  
>      /* update the flags depending on the control register */
>      if (s->cmdbuf_enabled) {
> @@ -2007,7 +2009,7 @@ static int amdvi_int_remap_ga(AMDVIState *iommu,
>      irq->vector = irte.hi.fields.vector;
>      irq->dest_mode = irte.lo.fields_remap.dm;
>      irq->redir_hint = irte.lo.fields_remap.rq_eoi;
> -    if (iommu->xtsup) {
> +    if (iommu->xten) {
>          irq->dest = irte.lo.fields_remap.destination |
>                      (irte.hi.fields.destination_hi << 24);
>      } else {
> @@ -2390,6 +2392,7 @@ static void amdvi_init(AMDVIState *s)
>      s->mmio_enabled = false;
>      s->enabled = false;
>      s->cmdbuf_enabled = false;
> +    s->xten = false;
>  
>      /* reset MMIO */
>      memset(s->mmior, 0, AMDVI_MMIO_SIZE);
> @@ -2454,6 +2457,16 @@ static void amdvi_sysbus_reset(DeviceState *dev)
>      amdvi_reset_address_translation_all(s);
>  }
>  
> +static const VMStateDescription vmstate_xt = {
> +       .name = "amd-iommu-xt",
> +       .version_id = 1,
> +       .minimum_version_id = 1,
> +       .fields = (VMStateField[]) {
> +           VMSTATE_BOOL(xten, AMDVIState),
> +           VMSTATE_END_OF_LIST()
> +       }
> +};
> +
>  static const VMStateDescription vmstate_amdvi_sysbus_migratable = {
>      .name = "amd-iommu",
>      .version_id = 1,
> @@ -2498,7 +2511,11 @@ static const VMStateDescription vmstate_amdvi_sysbus_migratable = {
>        VMSTATE_UINT8_ARRAY(romask, AMDVIState, AMDVI_MMIO_SIZE),
>        VMSTATE_UINT8_ARRAY(w1cmask, AMDVIState, AMDVI_MMIO_SIZE),
>        VMSTATE_END_OF_LIST()
> -    }
> +    },
> +    .subsections = (const VMStateDescription *const []) {
> +                    &vmstate_xt,
> +                    NULL
> +     }
>  };
>  
>  static void amdvi_sysbus_realize(DeviceState *dev, Error **errp)
> diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
> index 302ccca5121f..e9401f3a5c27 100644
> --- a/hw/i386/amd_iommu.h
> +++ b/hw/i386/amd_iommu.h
> @@ -106,6 +106,7 @@
>  #define AMDVI_MMIO_CONTROL_COMWAITINTEN   (1ULL << 4)
>  #define AMDVI_MMIO_CONTROL_CMDBUFLEN      (1ULL << 12)
>  #define AMDVI_MMIO_CONTROL_GAEN           (1ULL << 17)
> +#define AMDVI_MMIO_CONTROL_XTEN           (1ULL << 50)
>  
>  /* MMIO status register bits */
>  #define AMDVI_MMIO_STATUS_CMDBUF_RUN  (1 << 4)
> @@ -418,7 +419,8 @@ struct AMDVIState {
>  
>      /* Interrupt remapping */
>      bool ga_enabled;
> -    bool xtsup;
> +    bool xtsup;     /* xtsup=on command line */
> +    bool xten;      /* guest controlled, x2apic mode enabled */
>  
>      /* DMA address translation */
>      bool dma_remap;

Re: [PATCH v2 3/3] amd_iommu: Generate XT interrupts when xt support is enabled

[Alejandro Jimenez]

On 1/29/26 5:28 AM, Sairaj Kodilkar wrote:
> When MMIO 0x18[IntCapXTEn]=1, interrupts originating from the IOMMU itself are
> sent based on the programming in XT IOMMU Interrupt Control Registers in MMIO
> 0x170-0x180 instead of the programming in the IOMMU's MSI capability registers.
> The guest programs these registers with appropriate vector and destination
> ID instead of writing to PCI MSI capability.
> 
> Current AMD vIOMMU is capable of generating interrupts only through PCI
> MSI capability and does not care about xt mode. Because of this AMD
> vIOMMU cannot generate event log interrupts using XT event log register
> (0x170) when the guest has enabled xt mode.

I see that you slightly modified this from the initial version, but I would
leave it as it was i.e.:

"Because of this AMD vIOMMU cannot generate event log interrupts when the
guest has enabled xt mode."

There is no "XT event log register (0x170)", it is the "MMIO Offset 0x170h
XT IOMMU General Interrupt Control Register" as you mention in the next
sentence.

> 
> Introduce a new flag "intcapxten" which is set when guest writes control
> register [IntCapXTEn] (bit 51) and use vector and destination field in
> the XT MMIO register (0x170) to support XT mode.
> 
> Signed-off-by: Sairaj Kodilkar <sarunkod@amd.com>
> Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  hw/i386/amd_iommu.c  | 43 +++++++++++++++++++++++++++++++++++++------
>  hw/i386/amd_iommu.h  | 17 +++++++++++++++++
>  hw/i386/trace-events |  1 +
>  3 files changed, 55 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index 850d3920a76d..742ef5d42561 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -194,18 +194,38 @@ static void amdvi_assign_andq(AMDVIState *s, hwaddr addr, uint64_t val)
>     amdvi_writeq_raw(s, addr, amdvi_readq(s, addr) & val);
>  }
>  
> +static void amdvi_build_xt_msi_msg(AMDVIState *s, MSIMessage *msg)
> +{
> +    union mmio_xt_intr xt_reg;
> +    struct X86IOMMUIrq irq;
> +
> +    xt_reg.val = amdvi_readq(s, AMDVI_MMIO_XT_GEN_INTR);
> +
> +    irq.vector = xt_reg.vector;
> +    irq.delivery_mode = xt_reg.delivery_mode;
> +    irq.dest_mode = xt_reg.destination_mode;
> +    irq.dest = (xt_reg.destination_hi << 24) | xt_reg.destination_lo;
> +    irq.trigger_mode = 0;
> +    irq.redir_hint = 0;
> +
> +    x86_iommu_irq_to_msi_message(&irq, msg);
> +}
> +
>  static void amdvi_generate_msi_interrupt(AMDVIState *s)
>  {
>      MSIMessage msg = {};
> -    MemTxAttrs attrs = {
> -        .requester_id = pci_requester_id(&s->pci->dev)
> -    };
>  
> -    if (msi_enabled(&s->pci->dev)) {
> +    if (s->intcapxten) {
> +        trace_amdvi_generate_msi_interrupt("XT GEN");
> +        amdvi_build_xt_msi_msg(s, &msg);
> +    } else if (msi_enabled(&s->pci->dev)) {
> +        trace_amdvi_generate_msi_interrupt("MSI");
>          msg = msi_get_message(&s->pci->dev, 0);
> -        address_space_stl_le(&address_space_memory, msg.address, msg.data,
> -                             attrs, NULL);
> +    } else {
> +        trace_amdvi_generate_msi_interrupt("NO MSI");
> +        return;
>      }
> +    apic_get_class(NULL)->send_msi(&msg);
>  }
>  
>  static uint32_t get_next_eventlog_entry(AMDVIState *s)
> @@ -1483,6 +1503,7 @@ const char *amdvi_mmio_get_name(hwaddr addr)
>      MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_BASE);
>      MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_HEAD);
>      MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_TAIL);
> +    MMIO_REG_TO_STRING(AMDVI_MMIO_XT_GEN_INTR);
>      default:
>          return "UNHANDLED";
>      }
> @@ -1537,6 +1558,11 @@ static void amdvi_handle_control_write(AMDVIState *s)
>      s->ga_enabled = !!(control & AMDVI_MMIO_CONTROL_GAEN);
>      s->xten = !!(control & AMDVI_MMIO_CONTROL_XTEN) && s->xtsup &&
>                s->ga_enabled;
> +    /*
> +     * intcapxten does not depend on xten flag because IOMMU spec does not
> +     * specify any dependency between these two flags
> +     */

I'd like to modify the comment to add more info/breadcrumbs e.g.:

"
IntCapXTEn controls whether IOMMU-originated interrupts are sent based on
the information in XT IOMMU Interrupt Control Registers rather than the
IOMMU’s MSI capability registers. Therefore it requires IOMMU x2APIC
support capabilities (i.e. XTSup=1), but it is independent of whether a
driver chooses to enable x2APIC mode for interrupt remapping (i.e. XTEn=1)
"

This conveys your reasoning for not linking intcapxten and xten, while also
giving the exact search terms to query the spec and find the relevant
sections. Any concerns with this?

Otherwise,

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

> +    s->intcapxten = !!(control & AMDVI_MMIO_CONTROL_INTCAPXTEN) && s->xtsup;
>  
>      /* update the flags depending on the control register */
>      if (s->cmdbuf_enabled) {
> @@ -1743,6 +1769,9 @@ static void amdvi_mmio_write(void *opaque, hwaddr addr, uint64_t val,
>      case AMDVI_MMIO_STATUS:
>          amdvi_mmio_reg_write(s, size, val, addr);
>          break;
> +    case AMDVI_MMIO_XT_GEN_INTR:
> +        amdvi_mmio_reg_write(s, size, val, addr);
> +        break;
>      }
>  }
>  
> @@ -2393,6 +2422,7 @@ static void amdvi_init(AMDVIState *s)
>      s->enabled = false;
>      s->cmdbuf_enabled = false;
>      s->xten = false;
> +    s->intcapxten = false;
>  
>      /* reset MMIO */
>      memset(s->mmior, 0, AMDVI_MMIO_SIZE);
> @@ -2463,6 +2493,7 @@ static const VMStateDescription vmstate_xt = {
>         .minimum_version_id = 1,
>         .fields = (VMStateField[]) {
>             VMSTATE_BOOL(xten, AMDVIState),
> +           VMSTATE_BOOL(intcapxten, AMDVIState),
>             VMSTATE_END_OF_LIST()
>         }
>  };
> diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
> index e9401f3a5c27..886814770276 100644
> --- a/hw/i386/amd_iommu.h
> +++ b/hw/i386/amd_iommu.h
> @@ -57,6 +57,7 @@
>  #define AMDVI_MMIO_EXCL_BASE          0x0020
>  #define AMDVI_MMIO_EXCL_LIMIT         0x0028
>  #define AMDVI_MMIO_EXT_FEATURES       0x0030
> +#define AMDVI_MMIO_XT_GEN_INTR        0x0170
>  #define AMDVI_MMIO_COMMAND_HEAD       0x2000
>  #define AMDVI_MMIO_COMMAND_TAIL       0x2008
>  #define AMDVI_MMIO_EVENT_HEAD         0x2010
> @@ -107,6 +108,7 @@
>  #define AMDVI_MMIO_CONTROL_CMDBUFLEN      (1ULL << 12)
>  #define AMDVI_MMIO_CONTROL_GAEN           (1ULL << 17)
>  #define AMDVI_MMIO_CONTROL_XTEN           (1ULL << 50)
> +#define AMDVI_MMIO_CONTROL_INTCAPXTEN     (1ULL << 51)
>  
>  /* MMIO status register bits */
>  #define AMDVI_MMIO_STATUS_CMDBUF_RUN  (1 << 4)
> @@ -343,6 +345,20 @@ struct irte_ga {
>    union irte_ga_hi hi;
>  };
>  
> +union mmio_xt_intr {
> +    uint64_t val;
> +    struct {
> +        uint64_t rsvd_1:2,
> +                 destination_mode:1,
> +                 rsvd_2:5,
> +                 destination_lo:24,
> +                 vector:8,
> +                 delivery_mode:1,
> +                 rsvd_3:15,
> +                 destination_hi:8;
> +    };
> +};
> +
>  #define TYPE_AMD_IOMMU_DEVICE "amd-iommu"
>  OBJECT_DECLARE_SIMPLE_TYPE(AMDVIState, AMD_IOMMU_DEVICE)
>  
> @@ -421,6 +437,7 @@ struct AMDVIState {
>      bool ga_enabled;
>      bool xtsup;     /* xtsup=on command line */
>      bool xten;      /* guest controlled, x2apic mode enabled */
> +    bool intcapxten; /* guest controlled, IOMMU x2apic interrupts enabled */
>  
>      /* DMA address translation */
>      bool dma_remap;
> diff --git a/hw/i386/trace-events b/hw/i386/trace-events
> index 5fa5e93b68dc..a1dfade20f18 100644
> --- a/hw/i386/trace-events
> +++ b/hw/i386/trace-events
> @@ -118,6 +118,7 @@ amdvi_ir_intctl(uint8_t val) "int_ctl 0x%"PRIx8
>  amdvi_ir_target_abort(const char *str) "%s"
>  amdvi_ir_delivery_mode(const char *str) "%s"
>  amdvi_ir_irte_ga_val(uint64_t hi, uint64_t lo) "hi 0x%"PRIx64" lo 0x%"PRIx64
> +amdvi_generate_msi_interrupt(const char *str) "Mode: %s"
>  
>  # vmport.c
>  vmport_register(unsigned char command, void *func, void *opaque) "command: 0x%02x func: %p opaque: %p"

Re: [PATCH v2 1/3] amd_iommu: Use switch case to determine mmio register name

[Sairaj Kodilkar]

On 2/2/2026 8:47 PM, Alejandro Jimenez wrote:
>
> On 1/30/26 3:04 AM, Sairaj Kodilkar wrote:
>>
>> On 1/30/2026 1:09 PM, CLEMENT MATHIEU--DRIF wrote:
>>> On Thu, 2026-01-29 at 15:58 +0530, Sairaj Kodilkar wrote:
>>>> This makes it easier to add new MMIO registers for tracing and removes
>>>> the unnecessary complexity introduced by amdvi_mmio_(low/high) array.
>>>>
>>>> Signed-off-by: Sairaj Kodilkar <[sarunkod@amd.com]
>>>> (mailto:sarunkod@amd.com)>
>>>> Reviewed-by: Vasant Hegde <[vasant.hegde@amd.com]
>>>> (mailto:vasant.hegde@amd.com)>
>>>> ---
>>>>    hw/i386/amd_iommu.c | 65 +++++++++++++++++++--------------------------
>>>>    1 file changed, 27 insertions(+), 38 deletions(-)
>>>>
>>>> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
>>>> index 789e09d6f2bc..62175cc366ac 100644
>>>> --- a/hw/i386/amd_iommu.c
>>>> +++ b/hw/i386/amd_iommu.c
>>>> @@ -35,28 +35,7 @@
>>>>    #include "kvm/kvm_i386.h"
>>>>    #include "qemu/iova-tree.h"
>>>>      -/* used AMD-Vi MMIO registers */
>>>> -const char *amdvi_mmio_low[] = {
>>>> -    "AMDVI_MMIO_DEVTAB_BASE",
>>>> -    "AMDVI_MMIO_CMDBUF_BASE",
>>>> -    "AMDVI_MMIO_EVTLOG_BASE",
>>>> -    "AMDVI_MMIO_CONTROL",
>>>> -    "AMDVI_MMIO_EXCL_BASE",
>>>> -    "AMDVI_MMIO_EXCL_LIMIT",
>>>> -    "AMDVI_MMIO_EXT_FEATURES",
>>>> -    "AMDVI_MMIO_PPR_BASE",
>>>> -    "UNHANDLED"
>>>> -};
>>>> -const char *amdvi_mmio_high[] = {
>>>> -    "AMDVI_MMIO_COMMAND_HEAD",
>>>> -    "AMDVI_MMIO_COMMAND_TAIL",
>>>> -    "AMDVI_MMIO_EVTLOG_HEAD",
>>>> -    "AMDVI_MMIO_EVTLOG_TAIL",
>>>> -    "AMDVI_MMIO_STATUS",
>>>> -    "AMDVI_MMIO_PPR_HEAD",
>>>> -    "AMDVI_MMIO_PPR_TAIL",
>>>> -    "UNHANDLED"
>>>> -};
>>>> +#define MMIO_REG_TO_STRING(mmio_reg) case mmio_reg: return #mmio_reg
>>> Hi Sairaj,
>>>
>>> Shouldn't we define this inside the mmio_get_name function and undef it
>>> after the return statement?
>>> I think it would be cleanup to make the scope of this a bit smaller as it
>>> is specifically written for this function.
> I agree with the above. I think it is a good idea given the ad-hoc nature
> of this macro to keep the definition and its usage together and undef it
> right after to avoid any confusion.
>
> I was a bit reluctant when I proposed the macro because it affects control
> flow (the kernel coding style frowns on that even if QEMU doesn't
> explicitly forbids it), but I think this is clean and easy to parse:
>
> static inline
> const char *amdvi_mmio_get_name(hwaddr addr)
> {
>      /* Return MMIO names as string literals */
>      switch (addr) {
> #define MMIO_REG_TO_STRING(mmio_reg) case mmio_reg: return #mmio_reg
>      MMIO_REG_TO_STRING(AMDVI_MMIO_DEVICE_TABLE);
>
> [...]
>      MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_TAIL);
> #undef MMIO_REG_TO_STRING
>      default:
>          return "UNHANDLED";
>      }
> }
>
> Sairaj: if you don't want to sign off on this specific pattern, I can send
> a patch for it and we can review it separately.
>
> Alejandro

Sure, I'll send the V3 by tommorrow with all the changes

Thanks
Sairaj

>
>> Hi
>> I think this is probably okay as its unlikely to cause any issues in future.
>>
>> Thanks
>> -Sairaj
>>
>>>>        struct AMDVIAddressSpace {
>>>>        PCIBus *bus;                /* PCIBus (for bus
>>>> number)              */
>>>> @@ -1484,31 +1463,41 @@ static void amdvi_cmdbuf_run(AMDVIState *s)
>>>>        }
>>>>    }
>>>>      -static inline uint8_t amdvi_mmio_get_index(hwaddr addr)
>>>> -{
>>>> -    uint8_t index = (addr & ~0x2000) / 8;
>>>> -
>>>> -    if ((addr & 0x2000)) {
>>>> -        /* high table */
>>>> -        index = index >= AMDVI_MMIO_REGS_HIGH ? AMDVI_MMIO_REGS_HIGH :
>>>> index;
>>>> -    } else {
>>>> -        index = index >= AMDVI_MMIO_REGS_LOW ? AMDVI_MMIO_REGS_LOW :
>>>> index;
>>>> +static inline
>>>> +const char *amdvi_mmio_get_name(hwaddr addr)
>>>> +{
>>>> +    /* Return MMIO names as string literals */
>>>> +    switch (addr) {
>>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_DEVICE_TABLE);
>>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_COMMAND_BASE);
>>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EVENT_BASE);
>>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_CONTROL);
>>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EXCL_BASE);
>>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EXCL_LIMIT);
>>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EXT_FEATURES);
>>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_COMMAND_HEAD);
>>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_COMMAND_TAIL);
>>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EVENT_HEAD);
>>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EVENT_TAIL);
>>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_STATUS);
>>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_BASE);
>>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_HEAD);
>>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_TAIL);
>>>> +    default:
>>>> +        return "UNHANDLED";
>>>>        }
>>>> -
>>>> -    return index;
>>>>    }
>>>>        static void amdvi_mmio_trace_read(hwaddr addr, unsigned size)
>>>>    {
>>>> -    uint8_t index = amdvi_mmio_get_index(addr);
>>>> -    trace_amdvi_mmio_read(amdvi_mmio_low[index], addr, size, addr &
>>>> ~0x07);
>>>> +    const char *mmio_name = amdvi_mmio_get_name(addr);
>>>> +    trace_amdvi_mmio_read(mmio_name, addr, size, addr & ~0x07);
>>>>    }
>>>>        static void amdvi_mmio_trace_write(hwaddr addr, unsigned size,
>>>> uint64_t val)
>>>>    {
>>>> -    uint8_t index = amdvi_mmio_get_index(addr);
>>>> -    trace_amdvi_mmio_write(amdvi_mmio_low[index], addr, size, val,
>>>> -                           addr & ~0x07);
>>>> +    const char *mmio_name = amdvi_mmio_get_name(addr);
>>>> +    trace_amdvi_mmio_write(mmio_name, addr, size, val, addr & ~0x07);
>>>>    }
>>>>        static uint64_t amdvi_mmio_read(void *opaque, hwaddr addr,
>>>> unsigned size)

Re: [PATCH v2 3/3] amd_iommu: Generate XT interrupts when xt support is enabled

[Sairaj Kodilkar]

On 2/3/2026 4:59 AM, Alejandro Jimenez wrote:
>
> On 1/29/26 5:28 AM, Sairaj Kodilkar wrote:
>> When MMIO 0x18[IntCapXTEn]=1, interrupts originating from the IOMMU itself are
>> sent based on the programming in XT IOMMU Interrupt Control Registers in MMIO
>> 0x170-0x180 instead of the programming in the IOMMU's MSI capability registers.
>> The guest programs these registers with appropriate vector and destination
>> ID instead of writing to PCI MSI capability.
>>
>> Current AMD vIOMMU is capable of generating interrupts only through PCI
>> MSI capability and does not care about xt mode. Because of this AMD
>> vIOMMU cannot generate event log interrupts using XT event log register
>> (0x170) when the guest has enabled xt mode.
> I see that you slightly modified this from the initial version, but I would
> leave it as it was i.e.:
>
> "Because of this AMD vIOMMU cannot generate event log interrupts when the
> guest has enabled xt mode."
>
> There is no "XT event log register (0x170)", it is the "MMIO Offset 0x170h
> XT IOMMU General Interrupt Control Register" as you mention in the next
> sentence.

Sure

>> Introduce a new flag "intcapxten" which is set when guest writes control
>> register [IntCapXTEn] (bit 51) and use vector and destination field in
>> the XT MMIO register (0x170) to support XT mode.
>>
>> Signed-off-by: Sairaj Kodilkar <sarunkod@amd.com>
>> Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>
>> ---
>>   hw/i386/amd_iommu.c  | 43 +++++++++++++++++++++++++++++++++++++------
>>   hw/i386/amd_iommu.h  | 17 +++++++++++++++++
>>   hw/i386/trace-events |  1 +
>>   3 files changed, 55 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
>> index 850d3920a76d..742ef5d42561 100644
>> --- a/hw/i386/amd_iommu.c
>> +++ b/hw/i386/amd_iommu.c
>> @@ -194,18 +194,38 @@ static void amdvi_assign_andq(AMDVIState *s, hwaddr addr, uint64_t val)
>>      amdvi_writeq_raw(s, addr, amdvi_readq(s, addr) & val);
>>   }
>>   
>> +static void amdvi_build_xt_msi_msg(AMDVIState *s, MSIMessage *msg)
>> +{
>> +    union mmio_xt_intr xt_reg;
>> +    struct X86IOMMUIrq irq;
>> +
>> +    xt_reg.val = amdvi_readq(s, AMDVI_MMIO_XT_GEN_INTR);
>> +
>> +    irq.vector = xt_reg.vector;
>> +    irq.delivery_mode = xt_reg.delivery_mode;
>> +    irq.dest_mode = xt_reg.destination_mode;
>> +    irq.dest = (xt_reg.destination_hi << 24) | xt_reg.destination_lo;
>> +    irq.trigger_mode = 0;
>> +    irq.redir_hint = 0;
>> +
>> +    x86_iommu_irq_to_msi_message(&irq, msg);
>> +}
>> +
>>   static void amdvi_generate_msi_interrupt(AMDVIState *s)
>>   {
>>       MSIMessage msg = {};
>> -    MemTxAttrs attrs = {
>> -        .requester_id = pci_requester_id(&s->pci->dev)
>> -    };
>>   
>> -    if (msi_enabled(&s->pci->dev)) {
>> +    if (s->intcapxten) {
>> +        trace_amdvi_generate_msi_interrupt("XT GEN");
>> +        amdvi_build_xt_msi_msg(s, &msg);
>> +    } else if (msi_enabled(&s->pci->dev)) {
>> +        trace_amdvi_generate_msi_interrupt("MSI");
>>           msg = msi_get_message(&s->pci->dev, 0);
>> -        address_space_stl_le(&address_space_memory, msg.address, msg.data,
>> -                             attrs, NULL);
>> +    } else {
>> +        trace_amdvi_generate_msi_interrupt("NO MSI");
>> +        return;
>>       }
>> +    apic_get_class(NULL)->send_msi(&msg);
>>   }
>>   
>>   static uint32_t get_next_eventlog_entry(AMDVIState *s)
>> @@ -1483,6 +1503,7 @@ const char *amdvi_mmio_get_name(hwaddr addr)
>>       MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_BASE);
>>       MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_HEAD);
>>       MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_TAIL);
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_XT_GEN_INTR);
>>       default:
>>           return "UNHANDLED";
>>       }
>> @@ -1537,6 +1558,11 @@ static void amdvi_handle_control_write(AMDVIState *s)
>>       s->ga_enabled = !!(control & AMDVI_MMIO_CONTROL_GAEN);
>>       s->xten = !!(control & AMDVI_MMIO_CONTROL_XTEN) && s->xtsup &&
>>                 s->ga_enabled;
>> +    /*
>> +     * intcapxten does not depend on xten flag because IOMMU spec does not
>> +     * specify any dependency between these two flags
>> +     */
> I'd like to modify the comment to add more info/breadcrumbs e.g.:
>
> "
> IntCapXTEn controls whether IOMMU-originated interrupts are sent based on
> the information in XT IOMMU Interrupt Control Registers rather than the
> IOMMU’s MSI capability registers. Therefore it requires IOMMU x2APIC
> support capabilities (i.e. XTSup=1), but it is independent of whether a
> driver chooses to enable x2APIC mode for interrupt remapping (i.e. XTEn=1)
> "
>
> This conveys your reasoning for not linking intcapxten and xten, while also
> giving the exact search terms to query the spec and find the relevant
> sections. Any concerns with this?
>
> Otherwise,
>
> Reviewed-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>

Hi
Thanks for suggestion. I will expand the comment.

Also I hope the confusion around RH bit is clear now

Thanks
Sairaj

>> +    s->intcapxten = !!(control & AMDVI_MMIO_CONTROL_INTCAPXTEN) && s->xtsup;
>>   
>>       /* update the flags depending on the control register */
>>       if (s->cmdbuf_enabled) {
>> @@ -1743,6 +1769,9 @@ static void amdvi_mmio_write(void *opaque, hwaddr addr, uint64_t val,
>>       case AMDVI_MMIO_STATUS:
>>           amdvi_mmio_reg_write(s, size, val, addr);
>>           break;
>> +    case AMDVI_MMIO_XT_GEN_INTR:
>> +        amdvi_mmio_reg_write(s, size, val, addr);
>> +        break;
>>       }
>>   }
>>   
>> @@ -2393,6 +2422,7 @@ static void amdvi_init(AMDVIState *s)
>>       s->enabled = false;
>>       s->cmdbuf_enabled = false;
>>       s->xten = false;
>> +    s->intcapxten = false;
>>   
>>       /* reset MMIO */
>>       memset(s->mmior, 0, AMDVI_MMIO_SIZE);
>> @@ -2463,6 +2493,7 @@ static const VMStateDescription vmstate_xt = {
>>          .minimum_version_id = 1,
>>          .fields = (VMStateField[]) {
>>              VMSTATE_BOOL(xten, AMDVIState),
>> +           VMSTATE_BOOL(intcapxten, AMDVIState),
>>              VMSTATE_END_OF_LIST()
>>          }
>>   };
>> diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
>> index e9401f3a5c27..886814770276 100644
>> --- a/hw/i386/amd_iommu.h
>> +++ b/hw/i386/amd_iommu.h
>> @@ -57,6 +57,7 @@
>>   #define AMDVI_MMIO_EXCL_BASE          0x0020
>>   #define AMDVI_MMIO_EXCL_LIMIT         0x0028
>>   #define AMDVI_MMIO_EXT_FEATURES       0x0030
>> +#define AMDVI_MMIO_XT_GEN_INTR        0x0170
>>   #define AMDVI_MMIO_COMMAND_HEAD       0x2000
>>   #define AMDVI_MMIO_COMMAND_TAIL       0x2008
>>   #define AMDVI_MMIO_EVENT_HEAD         0x2010
>> @@ -107,6 +108,7 @@
>>   #define AMDVI_MMIO_CONTROL_CMDBUFLEN      (1ULL << 12)
>>   #define AMDVI_MMIO_CONTROL_GAEN           (1ULL << 17)
>>   #define AMDVI_MMIO_CONTROL_XTEN           (1ULL << 50)
>> +#define AMDVI_MMIO_CONTROL_INTCAPXTEN     (1ULL << 51)
>>   
>>   /* MMIO status register bits */
>>   #define AMDVI_MMIO_STATUS_CMDBUF_RUN  (1 << 4)
>> @@ -343,6 +345,20 @@ struct irte_ga {
>>     union irte_ga_hi hi;
>>   };
>>   
>> +union mmio_xt_intr {
>> +    uint64_t val;
>> +    struct {
>> +        uint64_t rsvd_1:2,
>> +                 destination_mode:1,
>> +                 rsvd_2:5,
>> +                 destination_lo:24,
>> +                 vector:8,
>> +                 delivery_mode:1,
>> +                 rsvd_3:15,
>> +                 destination_hi:8;
>> +    };
>> +};
>> +
>>   #define TYPE_AMD_IOMMU_DEVICE "amd-iommu"
>>   OBJECT_DECLARE_SIMPLE_TYPE(AMDVIState, AMD_IOMMU_DEVICE)
>>   
>> @@ -421,6 +437,7 @@ struct AMDVIState {
>>       bool ga_enabled;
>>       bool xtsup;     /* xtsup=on command line */
>>       bool xten;      /* guest controlled, x2apic mode enabled */
>> +    bool intcapxten; /* guest controlled, IOMMU x2apic interrupts enabled */
>>   
>>       /* DMA address translation */
>>       bool dma_remap;
>> diff --git a/hw/i386/trace-events b/hw/i386/trace-events
>> index 5fa5e93b68dc..a1dfade20f18 100644
>> --- a/hw/i386/trace-events
>> +++ b/hw/i386/trace-events
>> @@ -118,6 +118,7 @@ amdvi_ir_intctl(uint8_t val) "int_ctl 0x%"PRIx8
>>   amdvi_ir_target_abort(const char *str) "%s"
>>   amdvi_ir_delivery_mode(const char *str) "%s"
>>   amdvi_ir_irte_ga_val(uint64_t hi, uint64_t lo) "hi 0x%"PRIx64" lo 0x%"PRIx64
>> +amdvi_generate_msi_interrupt(const char *str) "Mode: %s"
>>   
>>   # vmport.c
>>   vmport_register(unsigned char command, void *func, void *opaque) "command: 0x%02x func: %p opaque: %p"