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

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

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

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

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  | 130 ++++++++++++++++++++++++++++---------------
 hw/i386/amd_iommu.h  |   7 ++-
 hw/i386/trace-events |   1 +
 3 files changed, 93 insertions(+), 45 deletions(-)

-- 
2.34.1

[PATCH 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>
---
 hw/i386/amd_iommu.c | 76 +++++++++++++++++++++++----------------------
 1 file changed, 39 insertions(+), 37 deletions(-)

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index d689a06eca46..a9ee7150ef17 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -35,28 +35,13 @@
 #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, name) {\
+    case mmio_reg: \
+        name = #mmio_reg; \
+        break; \
+}
+
+#define MMIO_NAME_SIZE 50
 
 struct AMDVIAddressSpace {
     PCIBus *bus;                /* PCIBus (for bus number)              */
@@ -1484,31 +1469,48 @@ 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 void amdvi_mmio_get_name(hwaddr addr,
+                                       char mmio_name[MMIO_NAME_SIZE])
+{
+    const char *name = NULL;
+
+    switch (addr) {
+    MMIO_REG_TO_STRING(AMDVI_MMIO_DEVICE_TABLE, name)
+    MMIO_REG_TO_STRING(AMDVI_MMIO_COMMAND_BASE, name)
+    MMIO_REG_TO_STRING(AMDVI_MMIO_EVENT_BASE, name)
+    MMIO_REG_TO_STRING(AMDVI_MMIO_CONTROL, name)
+    MMIO_REG_TO_STRING(AMDVI_MMIO_EXCL_BASE, name)
+    MMIO_REG_TO_STRING(AMDVI_MMIO_EXCL_LIMIT, name)
+    MMIO_REG_TO_STRING(AMDVI_MMIO_EXT_FEATURES, name)
+    MMIO_REG_TO_STRING(AMDVI_MMIO_COMMAND_HEAD, name)
+    MMIO_REG_TO_STRING(AMDVI_MMIO_COMMAND_TAIL, name)
+    MMIO_REG_TO_STRING(AMDVI_MMIO_EVENT_HEAD, name)
+    MMIO_REG_TO_STRING(AMDVI_MMIO_EVENT_TAIL, name)
+    MMIO_REG_TO_STRING(AMDVI_MMIO_STATUS, name)
+    MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_BASE, name)
+    MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_HEAD, name)
+    MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_TAIL, name)
+    default:
+        name = "UNHANDLED";
     }
 
-    return index;
+    strncpy(mmio_name, name, MMIO_NAME_SIZE);
 }
 
 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);
+    char mmio_name[MMIO_NAME_SIZE];
+
+    amdvi_mmio_get_name(addr, mmio_name);
+    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);
+    char mmio_name[MMIO_NAME_SIZE];
+
+    amdvi_mmio_get_name(addr, mmio_name);
+    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 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 xt 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).

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

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index a9ee7150ef17..7f08fc31111a 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -1548,6 +1548,7 @@ 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;
 
     /* update the flags depending on the control register */
     if (s->cmdbuf_enabled) {
@@ -2020,7 +2021,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 {
diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
index 302ccca5121f..32467d0bc241 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;      /* Enable x2apic */
 
     /* DMA address translation */
     bool dma_remap;
-- 
2.34.1

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

[Sairaj Kodilkar]

AMD IOMMU uses MMIO registers 0x170-0x180 to generate the interrupts
when guest enables xt support through control register [IntCapXTEn]. 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 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>
---
 hw/i386/amd_iommu.c  | 51 ++++++++++++++++++++++++++++++++++++++------
 hw/i386/amd_iommu.h  |  3 +++
 hw/i386/trace-events |  1 +
 3 files changed, 49 insertions(+), 6 deletions(-)

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 7f08fc31111a..c6bc13c93679 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -200,18 +200,52 @@ static void amdvi_assign_andq(AMDVIState *s, hwaddr addr, uint64_t val)
    amdvi_writeq_raw(s, addr, amdvi_readq(s, addr) & val);
 }
 
+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;
+    };
+};
+
+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)
@@ -1490,6 +1524,7 @@ static inline void amdvi_mmio_get_name(hwaddr addr,
     MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_BASE, name)
     MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_HEAD, name)
     MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_TAIL, name)
+    MMIO_REG_TO_STRING(AMDVI_MMIO_XT_GEN_INTR, name)
     default:
         name = "UNHANDLED";
     }
@@ -1549,6 +1584,7 @@ static void amdvi_handle_control_write(AMDVIState *s)
                         AMDVI_MMIO_CONTROL_CMDBUFLEN);
     s->ga_enabled = !!(control & AMDVI_MMIO_CONTROL_GAEN);
     s->xten = !!(control & AMDVI_MMIO_CONTROL_XTEN) && s->xtsup;
+    s->intcapxten = !!(control & AMDVI_MMIO_CONTROL_INTCAPXTEN) && s->xtsup;
 
     /* update the flags depending on the control register */
     if (s->cmdbuf_enabled) {
@@ -1755,6 +1791,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;
     }
 }
 
diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
index 32467d0bc241..399a4fb748e5 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)
@@ -421,6 +423,7 @@ struct AMDVIState {
     bool ga_enabled;
     bool xtsup;     /* xtsup=on command line */
     bool xten;      /* Enable x2apic */
+    bool intcapxten; /* Enable IOMMU x2apic interrupt generation */
 
     /* DMA address translation */
     bool dma_remap;
diff --git a/hw/i386/trace-events b/hw/i386/trace-events
index b704f4f90c3d..fe7aea4507ae 100644
--- a/hw/i386/trace-events
+++ b/hw/i386/trace-events
@@ -114,6 +114,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 0/3] amd_iommu: Support Generation of IOMMU XT interrupts

[Vasant Hegde]

On 11/18/2025 1:54 PM, Sairaj Kodilkar wrote:
> 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.

I have reviewed this series and it looks good. For entire series,

Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>


-Vasant

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

[Alejandro Jimenez]

Hi Sairaj,

On 11/18/25 3:24 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>
> ---
>   hw/i386/amd_iommu.c | 76 +++++++++++++++++++++++----------------------
>   1 file changed, 39 insertions(+), 37 deletions(-)
> 

[...]

> +#define MMIO_REG_TO_STRING(mmio_reg, name) {\
> +    case mmio_reg: \
> +        name = #mmio_reg; \
> +        break; \
> +}
> +
> +#define MMIO_NAME_SIZE 50
>   
>   struct AMDVIAddressSpace {
>       PCIBus *bus;                /* PCIBus (for bus number)              */
> @@ -1484,31 +1469,48 @@ 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 void amdvi_mmio_get_name(hwaddr addr,
> +                                       char mmio_name[MMIO_NAME_SIZE])
> +{
> +    const char *name = NULL;
> +
> +    switch (addr) {
> +    MMIO_REG_TO_STRING(AMDVI_MMIO_DEVICE_TABLE, name)
> +    MMIO_REG_TO_STRING(AMDVI_MMIO_COMMAND_BASE, name)
> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EVENT_BASE, name)
> +    MMIO_REG_TO_STRING(AMDVI_MMIO_CONTROL, name)
> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EXCL_BASE, name)
> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EXCL_LIMIT, name)
> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EXT_FEATURES, name)
> +    MMIO_REG_TO_STRING(AMDVI_MMIO_COMMAND_HEAD, name)
> +    MMIO_REG_TO_STRING(AMDVI_MMIO_COMMAND_TAIL, name)
> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EVENT_HEAD, name)
> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EVENT_TAIL, name)
> +    MMIO_REG_TO_STRING(AMDVI_MMIO_STATUS, name)
> +    MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_BASE, name)
> +    MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_HEAD, name)
> +    MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_TAIL, name)
> +    default:
> +        name = "UNHANDLED";
>       }
>   
> -    return index;
> +    strncpy(mmio_name, name, MMIO_NAME_SIZE);
>   }

While I don't believe there is a correctness issue, and it is a clever 
construct to reduce code repetition, I had some concerns with the 
implementation above, mostly on coding style and maintainability. I can 
go into each of the issues, but as I was trying to think of fixes it 
just became easier to write the code so...

I think these changes preserve your original idea while fixing the 
problems and removing unnecessary code. Rather than diff from your 
patch, I'm sharing a diff for the full patch. I am still working through 
the other patches but the upcoming changes should fit in with no issues.
Let me know if you agree with the changes, or if there is something I 
missed.

Alejandro

---
(compile tested only)

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index d689a06eca..6fd9e2670a 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,27 @@ 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 const char *amdvi_mmio_get_name(hwaddr addr)
+{
+    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);
-}
-
-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);
  }

  static uint64_t amdvi_mmio_read(void *opaque, hwaddr addr, unsigned size)
@@ -1528,7 +1503,7 @@ static uint64_t amdvi_mmio_read(void *opaque, 
hwaddr addr, unsigned size)
      } else if (size == 8) {
          val = amdvi_readq(s, addr);
      }
-    amdvi_mmio_trace_read(addr, size);
+    trace_amdvi_mmio_read(amdvi_mmio_get_name(addr), addr, size, addr & 
~0x07);

      return val;
  }
@@ -1684,7 +1659,9 @@ static void amdvi_mmio_write(void *opaque, hwaddr 
addr, uint64_t val,
          return;
      }

-    amdvi_mmio_trace_write(addr, size, val);
+    trace_amdvi_mmio_write(amdvi_mmio_get_name(addr), addr, size, val,
+                          addr & ~0x07);
+
      switch (addr & ~0x07) {
      case AMDVI_MMIO_CONTROL:
          amdvi_mmio_reg_write(s, size, val, addr);
diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
index 302ccca512..ca4ff9fffe 100644
--- a/hw/i386/amd_iommu.h
+++ b/hw/i386/amd_iommu.h
@@ -45,10 +45,6 @@
  #define AMDVI_CAPAB_FLAG_IOTLBSUP     (1 << 24)
  #define AMDVI_CAPAB_INIT_TYPE         (3 << 16)

-/* No. of used MMIO registers */
-#define AMDVI_MMIO_REGS_HIGH  7
-#define AMDVI_MMIO_REGS_LOW   8
-
  /* MMIO registers */
  #define AMDVI_MMIO_DEVICE_TABLE       0x0000
  #define AMDVI_MMIO_COMMAND_BASE       0x0008

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

[Sairaj Kodilkar]

On 11/20/2025 7:06 AM, Alejandro Jimenez wrote:
> Hi Sairaj,
>
> On 11/18/25 3:24 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>
>> ---
>>   hw/i386/amd_iommu.c | 76 +++++++++++++++++++++++----------------------
>>   1 file changed, 39 insertions(+), 37 deletions(-)
>>
>
> [...]
>
>> +#define MMIO_REG_TO_STRING(mmio_reg, name) {\
>> +    case mmio_reg: \
>> +        name = #mmio_reg; \
>> +        break; \
>> +}
>> +
>> +#define MMIO_NAME_SIZE 50
>>     struct AMDVIAddressSpace {
>>       PCIBus *bus;                /* PCIBus (for bus 
>> number)              */
>> @@ -1484,31 +1469,48 @@ 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 void amdvi_mmio_get_name(hwaddr addr,
>> +                                       char mmio_name[MMIO_NAME_SIZE])
>> +{
>> +    const char *name = NULL;
>> +
>> +    switch (addr) {
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_DEVICE_TABLE, name)
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_COMMAND_BASE, name)
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EVENT_BASE, name)
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_CONTROL, name)
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EXCL_BASE, name)
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EXCL_LIMIT, name)
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EXT_FEATURES, name)
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_COMMAND_HEAD, name)
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_COMMAND_TAIL, name)
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EVENT_HEAD, name)
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EVENT_TAIL, name)
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_STATUS, name)
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_BASE, name)
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_HEAD, name)
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_TAIL, name)
>> +    default:
>> +        name = "UNHANDLED";
>>       }
>>   -    return index;
>> +    strncpy(mmio_name, name, MMIO_NAME_SIZE);
>>   }
>
> While I don't believe there is a correctness issue, and it is a clever 
> construct to reduce code repetition, I had some concerns with the 
> implementation above, mostly on coding style and maintainability. I 
> can go into each of the issues, but as I was trying to think of fixes 
> it just became easier to write the code so...
>
> I think these changes preserve your original idea while fixing the 
> problems and removing unnecessary code. Rather than diff from your 
> patch, I'm sharing a diff for the full patch. I am still working 
> through the other patches but the upcoming changes should fit in with 
> no issues.
> Let me know if you agree with the changes, or if there is something I 
> missed.
>
> Alejandro
>
> ---
> (compile tested only)
>
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index d689a06eca..6fd9e2670a 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,27 @@ 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 const char *amdvi_mmio_get_name(hwaddr addr)
> +{
> +    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";
>      }

Hi Alejandro
Although this approach looks good and faster (since you are
returning a pointer without copy), it has a major flaw.
You are returning a pointer to the "local string" which can
lead to all sorts of nasty issues. This is why I am copying
the entire string to the destination.

Thanks
Sairaj

> -
> -    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);
> -}
> -
> -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);
>  }
>
>  static uint64_t amdvi_mmio_read(void *opaque, hwaddr addr, unsigned 
> size)
> @@ -1528,7 +1503,7 @@ static uint64_t amdvi_mmio_read(void *opaque, 
> hwaddr addr, unsigned size)
>      } else if (size == 8) {
>          val = amdvi_readq(s, addr);
>      }
> -    amdvi_mmio_trace_read(addr, size);
> +    trace_amdvi_mmio_read(amdvi_mmio_get_name(addr), addr, size, addr 
> & ~0x07);
>
>      return val;
>  }
> @@ -1684,7 +1659,9 @@ static void amdvi_mmio_write(void *opaque, 
> hwaddr addr, uint64_t val,
>          return;
>      }
>
> -    amdvi_mmio_trace_write(addr, size, val);
> +    trace_amdvi_mmio_write(amdvi_mmio_get_name(addr), addr, size, val,
> +                          addr & ~0x07);
> +
>      switch (addr & ~0x07) {
>      case AMDVI_MMIO_CONTROL:
>          amdvi_mmio_reg_write(s, size, val, addr);
> diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
> index 302ccca512..ca4ff9fffe 100644
> --- a/hw/i386/amd_iommu.h
> +++ b/hw/i386/amd_iommu.h
> @@ -45,10 +45,6 @@
>  #define AMDVI_CAPAB_FLAG_IOTLBSUP     (1 << 24)
>  #define AMDVI_CAPAB_INIT_TYPE         (3 << 16)
>
> -/* No. of used MMIO registers */
> -#define AMDVI_MMIO_REGS_HIGH  7
> -#define AMDVI_MMIO_REGS_LOW   8
> -
>  /* MMIO registers */
>  #define AMDVI_MMIO_DEVICE_TABLE       0x0000
>  #define AMDVI_MMIO_COMMAND_BASE       0x0008
>

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

[Alejandro Jimenez]

On 11/19/25 11:43 PM, Sairaj Kodilkar wrote:
> 
> 
> On 11/20/2025 7:06 AM, Alejandro Jimenez wrote:
>> Hi Sairaj,
>>
>> On 11/18/25 3:24 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>
>>> ---
>>>   hw/i386/amd_iommu.c | 76 +++++++++++++++++++++++----------------------
>>>   1 file changed, 39 insertions(+), 37 deletions(-)
>>>
>>
>> [...]
>>
>>> +#define MMIO_REG_TO_STRING(mmio_reg, name) {\
>>> +    case mmio_reg: \
>>> +        name = #mmio_reg; \
>>> +        break; \
>>> +}
>>> +
>>> +#define MMIO_NAME_SIZE 50
>>>     struct AMDVIAddressSpace {
>>>       PCIBus *bus;                /* PCIBus (for bus 
>>> number)              */
>>> @@ -1484,31 +1469,48 @@ 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 void amdvi_mmio_get_name(hwaddr addr,
>>> +                                       char mmio_name[MMIO_NAME_SIZE])
>>> +{
>>> +    const char *name = NULL;
>>> +
>>> +    switch (addr) {
>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_DEVICE_TABLE, name)
>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_COMMAND_BASE, name)
>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EVENT_BASE, name)
>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_CONTROL, name)
>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EXCL_BASE, name)
>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EXCL_LIMIT, name)
>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EXT_FEATURES, name)
>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_COMMAND_HEAD, name)
>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_COMMAND_TAIL, name)
>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EVENT_HEAD, name)
>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EVENT_TAIL, name)
>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_STATUS, name)
>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_BASE, name)
>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_HEAD, name)
>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_TAIL, name)
>>> +    default:
>>> +        name = "UNHANDLED";
>>>       }
>>>   -    return index;
>>> +    strncpy(mmio_name, name, MMIO_NAME_SIZE);
>>>   }
>>
>> While I don't believe there is a correctness issue, and it is a clever 
>> construct to reduce code repetition, I had some concerns with the 
>> implementation above, mostly on coding style and maintainability. I 
>> can go into each of the issues, but as I was trying to think of fixes 
>> it just became easier to write the code so...
>>
>> I think these changes preserve your original idea while fixing the 
>> problems and removing unnecessary code. Rather than diff from your 
>> patch, I'm sharing a diff for the full patch. I am still working 
>> through the other patches but the upcoming changes should fit in with 
>> no issues.
>> Let me know if you agree with the changes, or if there is something I 
>> missed.
>>
>> Alejandro
>>
>> ---
>> (compile tested only)
>>
>> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
>> index d689a06eca..6fd9e2670a 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,27 @@ 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 const char *amdvi_mmio_get_name(hwaddr addr)
>> +{
>> +    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";
>>      }
> 
> Hi Alejandro
> Although this approach looks good and faster (since you are
> returning a pointer without copy), it has a major flaw.
> You are returning a pointer to the "local string" 

Not quite. While you are right this could be issue in the initial 
version if you returned the local *name, it is not a problem above since 
the strings created using the macro stringification operator (#) are 
literal strings and not local i.e. they do not live in the stack frame 
that gets destroyed when the function returns. In all cases the returned 
pointer will point to a string literal that is available for the life of 
the program in the (ro)data section.
You can check it yourself by building the binary and looking at the data 
section with something like  objdump, but that would only show some 
fragments due to alignment of the output. After a bit of searching, it 
looks like readelf has an option that works best:

$ readelf -p .rodata build/qemu-system-x86_64 | grep AMDVI_MMIO
   [ a1c37]  AMDVI_MMIO_DEVICE_TABLE
   [ a1c4f]  AMDVI_MMIO_COMMAND_BASE
   [ a1c67]  AMDVI_MMIO_EVENT_BASE
   [ a1c7d]  AMDVI_MMIO_CONTROL
   [ a1c90]  AMDVI_MMIO_EXCL_BASE
   [ a1ca5]  AMDVI_MMIO_EXCL_LIMIT
   [ a1cbb]  AMDVI_MMIO_EXT_FEATURES
   [ a1cd3]  AMDVI_MMIO_COMMAND_HEAD
   [ a1ceb]  AMDVI_MMIO_COMMAND_TAIL
   [ a1d03]  AMDVI_MMIO_EVENT_HEAD
   [ a1d19]  AMDVI_MMIO_EVENT_TAIL
   [ a1d2f]  AMDVI_MMIO_STATUS
   [ a1d41]  AMDVI_MMIO_PPR_BASE
   [ a1d55]  AMDVI_MMIO_PPR_HEAD
   [ a1d69]  AMDVI_MMIO_PPR_TAIL


which can
> lead to all sorts of nasty issues. This is why I am copying
> the entire string to the destination.
> 
FYI, this copy is one of the reasons that made me look for an 
alternative implemention. Using strncpy is explicitly forbidden in the 
QEMU coding style:
https://qemu-project.gitlab.io/qemu/devel/style.html#string-manipulation
and while there are alternatives, in this case the copy is not really 
necessary.

Alejandro

> Thanks
> Sairaj
> 
>> -
>> -    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);
>> -}
>> -
>> -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);
>>  }
>>
>>  static uint64_t amdvi_mmio_read(void *opaque, hwaddr addr, unsigned 
>> size)
>> @@ -1528,7 +1503,7 @@ static uint64_t amdvi_mmio_read(void *opaque, 
>> hwaddr addr, unsigned size)
>>      } else if (size == 8) {
>>          val = amdvi_readq(s, addr);
>>      }
>> -    amdvi_mmio_trace_read(addr, size);
>> +    trace_amdvi_mmio_read(amdvi_mmio_get_name(addr), addr, size, addr 
>> & ~0x07);
>>
>>      return val;
>>  }
>> @@ -1684,7 +1659,9 @@ static void amdvi_mmio_write(void *opaque, 
>> hwaddr addr, uint64_t val,
>>          return;
>>      }
>>
>> -    amdvi_mmio_trace_write(addr, size, val);
>> +    trace_amdvi_mmio_write(amdvi_mmio_get_name(addr), addr, size, val,
>> +                          addr & ~0x07);
>> +
>>      switch (addr & ~0x07) {
>>      case AMDVI_MMIO_CONTROL:
>>          amdvi_mmio_reg_write(s, size, val, addr);
>> diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
>> index 302ccca512..ca4ff9fffe 100644
>> --- a/hw/i386/amd_iommu.h
>> +++ b/hw/i386/amd_iommu.h
>> @@ -45,10 +45,6 @@
>>  #define AMDVI_CAPAB_FLAG_IOTLBSUP     (1 << 24)
>>  #define AMDVI_CAPAB_INIT_TYPE         (3 << 16)
>>
>> -/* No. of used MMIO registers */
>> -#define AMDVI_MMIO_REGS_HIGH  7
>> -#define AMDVI_MMIO_REGS_LOW   8
>> -
>>  /* MMIO registers */
>>  #define AMDVI_MMIO_DEVICE_TABLE       0x0000
>>  #define AMDVI_MMIO_COMMAND_BASE       0x0008
>>
> 
> 
> 

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

[Sairaj Kodilkar]

On 11/20/2025 7:01 PM, Alejandro Jimenez wrote:
>
>
> On 11/19/25 11:43 PM, Sairaj Kodilkar wrote:
>>
>>
>> On 11/20/2025 7:06 AM, Alejandro Jimenez wrote:
>>> Hi Sairaj,
>>>
>>> On 11/18/25 3:24 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>
>>>> ---
>>>>   hw/i386/amd_iommu.c | 76 
>>>> +++++++++++++++++++++++----------------------
>>>>   1 file changed, 39 insertions(+), 37 deletions(-)
>>>>
>>>
>>> [...]
>>>
>>>> +#define MMIO_REG_TO_STRING(mmio_reg, name) {\
>>>> +    case mmio_reg: \
>>>> +        name = #mmio_reg; \
>>>> +        break; \
>>>> +}
>>>> +
>>>> +#define MMIO_NAME_SIZE 50
>>>>     struct AMDVIAddressSpace {
>>>>       PCIBus *bus;                /* PCIBus (for bus 
>>>> number)              */
>>>> @@ -1484,31 +1469,48 @@ 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 void amdvi_mmio_get_name(hwaddr addr,
>>>> +                                       char 
>>>> mmio_name[MMIO_NAME_SIZE])
>>>> +{
>>>> +    const char *name = NULL;
>>>> +
>>>> +    switch (addr) {
>>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_DEVICE_TABLE, name)
>>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_COMMAND_BASE, name)
>>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EVENT_BASE, name)
>>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_CONTROL, name)
>>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EXCL_BASE, name)
>>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EXCL_LIMIT, name)
>>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EXT_FEATURES, name)
>>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_COMMAND_HEAD, name)
>>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_COMMAND_TAIL, name)
>>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EVENT_HEAD, name)
>>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EVENT_TAIL, name)
>>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_STATUS, name)
>>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_BASE, name)
>>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_HEAD, name)
>>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_TAIL, name)
>>>> +    default:
>>>> +        name = "UNHANDLED";
>>>>       }
>>>>   -    return index;
>>>> +    strncpy(mmio_name, name, MMIO_NAME_SIZE);
>>>>   }
>>>
>>> While I don't believe there is a correctness issue, and it is a 
>>> clever construct to reduce code repetition, I had some concerns with 
>>> the implementation above, mostly on coding style and 
>>> maintainability. I can go into each of the issues, but as I was 
>>> trying to think of fixes it just became easier to write the code so...
>>>
>>> I think these changes preserve your original idea while fixing the 
>>> problems and removing unnecessary code. Rather than diff from your 
>>> patch, I'm sharing a diff for the full patch. I am still working 
>>> through the other patches but the upcoming changes should fit in 
>>> with no issues.
>>> Let me know if you agree with the changes, or if there is something 
>>> I missed.
>>>
>>> Alejandro
>>>
>>> ---
>>> (compile tested only)
>>>
>>> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
>>> index d689a06eca..6fd9e2670a 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,27 @@ 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 const char *amdvi_mmio_get_name(hwaddr addr)
>>> +{
>>> +    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";
>>>      }
>>
>> Hi Alejandro
>> Although this approach looks good and faster (since you are
>> returning a pointer without copy), it has a major flaw.
>> You are returning a pointer to the "local string" 
>
> Not quite. While you are right this could be issue in the initial 
> version if you returned the local *name, it is not a problem above 
> since the strings created using the macro stringification operator (#) 
> are literal strings and not local i.e. they do not live in the stack 
> frame that gets destroyed when the function returns. In all cases the 
> returned pointer will point to a string literal that is available for 
> the life of the program in the (ro)data section.
> You can check it yourself by building the binary and looking at the 
> data section with something like  objdump, but that would only show 
> some fragments due to alignment of the output. After a bit of 
> searching, it looks like readelf has an option that works best:
>
> $ readelf -p .rodata build/qemu-system-x86_64 | grep AMDVI_MMIO
>   [ a1c37]  AMDVI_MMIO_DEVICE_TABLE
>   [ a1c4f]  AMDVI_MMIO_COMMAND_BASE
>   [ a1c67]  AMDVI_MMIO_EVENT_BASE
>   [ a1c7d]  AMDVI_MMIO_CONTROL
>   [ a1c90]  AMDVI_MMIO_EXCL_BASE
>   [ a1ca5]  AMDVI_MMIO_EXCL_LIMIT
>   [ a1cbb]  AMDVI_MMIO_EXT_FEATURES
>   [ a1cd3]  AMDVI_MMIO_COMMAND_HEAD
>   [ a1ceb]  AMDVI_MMIO_COMMAND_TAIL
>   [ a1d03]  AMDVI_MMIO_EVENT_HEAD
>   [ a1d19]  AMDVI_MMIO_EVENT_TAIL
>   [ a1d2f]  AMDVI_MMIO_STATUS
>   [ a1d41]  AMDVI_MMIO_PPR_BASE
>   [ a1d55]  AMDVI_MMIO_PPR_HEAD
>   [ a1d69]  AMDVI_MMIO_PPR_TAIL
>
>
> which can
>> lead to all sorts of nasty issues. This is why I am copying
>> the entire string to the destination.
>>
> FYI, this copy is one of the reasons that made me look for an 
> alternative implemention. Using strncpy is explicitly forbidden in the 
> QEMU coding style:
> https://qemu-project.gitlab.io/qemu/devel/style.html#string-manipulation
> and while there are alternatives, in this case the copy is not really 
> necessary.
>
> Alejandro

Ahh right I missed that its string literal and will be stored in ro data 
section. Thanks for clarifying the things
I am in favor of this approach. Will do this in the next patch version. 
Could you please provide your signed-off-by ?

Thanks
Sairaj

.../...

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

[Alejandro Jimenez]

On 11/21/25 12:20 AM, Sairaj Kodilkar wrote:
> 
> 
> On 11/20/2025 7:01 PM, Alejandro Jimenez wrote:
>>
>>
>> On 11/19/25 11:43 PM, Sairaj Kodilkar wrote:
>>>
>>>
>>> On 11/20/2025 7:06 AM, Alejandro Jimenez wrote:
>>>> Hi Sairaj,
>>>>
>>>> On 11/18/25 3:24 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>
>>>>> ---
>>>>>   hw/i386/amd_iommu.c | 76 ++++++++++++++++++++++ 
>>>>> +----------------------
>>>>>   1 file changed, 39 insertions(+), 37 deletions(-)
>>>>>
>>>>

[...]


> I am in favor of this approach. Will do this in the next patch version. 
> Could you please provide your signed-off-by ?
> 

No need. I suggested some adjustments, but I think your original idea 
still stands and Vasant's R-b still applies (unless he has an objection 
to the proposed changes...).

I'm still reviewing the other patches in the series, but might not get 
to reply until early next week.

Thank you,
Alejandro

> Thanks
> Sairaj
> 
> .../...

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

[Alejandro Jimenez]

Hi Sairaj,

I have a couple of suggestions, and one addition I believe is needed in 
the code, but overall looks good.

On 11/18/25 3:24 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"

I think it fits the language used by the spec slightly better.

> guest has enabled xt support through control register[XTEn] and

a guest has enabled x2APIC support ...

I think it is better to replace "xt" above with "x2APIC", which 
describes what the XT feature is/does.

> 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).
> 
> Signed-off-by: Sairaj Kodilkar <sarunkod@amd.com>
> ---
>   hw/i386/amd_iommu.c | 3 ++-
>   hw/i386/amd_iommu.h | 4 +++-
>   2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index a9ee7150ef17..7f08fc31111a 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -1548,6 +1548,7 @@ 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;
>   

I think we should also include a new xten field in 
vmstate_amdvi_sysbus_migratable, to ensure the remapping behavior stays 
consistent after migration. i.e.

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 9bf36ef608..5940011ef1 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -2452,6 +2452,7 @@ static const VMStateDescription 
vmstate_amdvi_sysbus_migratable = {
        /* Updated in  amdvi_handle_control_write() */
        VMSTATE_BOOL(enabled, AMDVIState),
        VMSTATE_BOOL(ga_enabled, AMDVIState),
+      VMSTATE_BOOL(xten, AMDVIState),
        /* bool ats_enabled is obsolete */
        VMSTATE_UNUSED(1), /* was ats_enabled */
        VMSTATE_BOOL(cmdbuf_enabled, AMDVIState),

There is more work to do there, but this seems straightforward.

>       /* update the flags depending on the control register */
>       if (s->cmdbuf_enabled) {
> @@ -2020,7 +2021,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 {
> diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
> index 302ccca5121f..32467d0bc241 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;      /* Enable x2apic */
>   

I'd reword the comment to indicate this what the guest toggles for 
enabling x2apic. e.g.

bool xten;      /* guest controlled, x2apic mode enabled */

I am aware that other fields that are also "guest controlled" don't have 
similar comments. My idea is to highlight that the xten is "toggled" at 
runtime, and is different from xtsup, which is a capability inherent to 
the hardware being emulated and set during initialization.

Thank you,
Alejandro

>       /* DMA address translation */
>       bool dma_remap;

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

[Sairaj Kodilkar]

On 11/26/2025 4:34 AM, Alejandro Jimenez wrote:
> Hi Sairaj,
>
> I have a couple of suggestions, and one addition I believe is needed 
> in the code, but overall looks good.
>
> On 11/18/25 3:24 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"
>
> I think it fits the language used by the spec slightly better.
>
>> guest has enabled xt support through control register[XTEn] and
>
> a guest has enabled x2APIC support ...
>
> I think it is better to replace "xt" above with "x2APIC", which 
> describes what the XT feature is/does.
>
>> 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).
>>
>> Signed-off-by: Sairaj Kodilkar <sarunkod@amd.com>
>> ---
>>   hw/i386/amd_iommu.c | 3 ++-
>>   hw/i386/amd_iommu.h | 4 +++-
>>   2 files changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
>> index a9ee7150ef17..7f08fc31111a 100644
>> --- a/hw/i386/amd_iommu.c
>> +++ b/hw/i386/amd_iommu.c
>> @@ -1548,6 +1548,7 @@ 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;
>
> I think we should also include a new xten field in 
> vmstate_amdvi_sysbus_migratable, to ensure the remapping behavior 
> stays consistent after migration. i.e.

Right I completely missed this

>
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index 9bf36ef608..5940011ef1 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -2452,6 +2452,7 @@ static const VMStateDescription 
> vmstate_amdvi_sysbus_migratable = {
>        /* Updated in  amdvi_handle_control_write() */
>        VMSTATE_BOOL(enabled, AMDVIState),
>        VMSTATE_BOOL(ga_enabled, AMDVIState),
> +      VMSTATE_BOOL(xten, AMDVIState),
>        /* bool ats_enabled is obsolete */
>        VMSTATE_UNUSED(1), /* was ats_enabled */
>        VMSTATE_BOOL(cmdbuf_enabled, AMDVIState),
>
> There is more work to do there, but this seems straightforward.
>
>>       /* update the flags depending on the control register */
>>       if (s->cmdbuf_enabled) {
>> @@ -2020,7 +2021,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 {
>> diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
>> index 302ccca5121f..32467d0bc241 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;      /* Enable x2apic */
>
> I'd reword the comment to indicate this what the guest toggles for 
> enabling x2apic. e.g.
>
> bool xten;      /* guest controlled, x2apic mode enabled */
>
> I am aware that other fields that are also "guest controlled" don't 
> have similar comments. My idea is to highlight that the xten is 
> "toggled" at runtime, and is different from xtsup, which is a 
> capability inherent to the hardware being emulated and set during 
> initialization.

Yep that makes sense

Thanks
Sairaj

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

[Sairaj Kodilkar]

On 11/26/2025 4:34 AM, Alejandro Jimenez wrote:
> Hi Sairaj,
>
> I have a couple of suggestions, and one addition I believe is needed 
> in the code, but overall looks good.
>
> On 11/18/25 3:24 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"
>
> I think it fits the language used by the spec slightly better.
>
>> guest has enabled xt support through control register[XTEn] and
>
> a guest has enabled x2APIC support ...
>
> I think it is better to replace "xt" above with "x2APIC", which 
> describes what the XT feature is/does.
>
>> 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).
>>
>> Signed-off-by: Sairaj Kodilkar <sarunkod@amd.com>
>> ---
>>   hw/i386/amd_iommu.c | 3 ++-
>>   hw/i386/amd_iommu.h | 4 +++-
>>   2 files changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
>> index a9ee7150ef17..7f08fc31111a 100644
>> --- a/hw/i386/amd_iommu.c
>> +++ b/hw/i386/amd_iommu.c
>> @@ -1548,6 +1548,7 @@ 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;
>
> I think we should also include a new xten field in 
> vmstate_amdvi_sysbus_migratable, to ensure the remapping behavior 
> stays consistent after migration. i.e.
>
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index 9bf36ef608..5940011ef1 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -2452,6 +2452,7 @@ static const VMStateDescription 
> vmstate_amdvi_sysbus_migratable = {
>        /* Updated in  amdvi_handle_control_write() */
>        VMSTATE_BOOL(enabled, AMDVIState),
>        VMSTATE_BOOL(ga_enabled, AMDVIState),
> +      VMSTATE_BOOL(xten, AMDVIState),
>        /* bool ats_enabled is obsolete */
>        VMSTATE_UNUSED(1), /* was ats_enabled */
>        VMSTATE_BOOL(cmdbuf_enabled, AMDVIState),

Hi Alejandro,

I don't think adding a VMSTATE_BOOL directly is a good idea because 
it'll break backward
compatibility (kernel running on older qemu won't be able to migrate on 
newer qemu because
change in the stream length of the migration data). In this case we will 
have to increment
the min_version_number to stop migration from older qemu
I am thinking to add a new subsection as it'll not break the 
compatibility. Let me know what
do you think about this.

Something like this...

static const VMStateDescription vmstate_xt = {
       .name = "amd-iommu-xt",
       .version_id = 1,
       .minimum_version_id = 1,
       .fields = (VMStateField[]) {
           VMSTATE_BOOL(xten, AMDVIState),
       }
};

static const VMStateDescription vmstate_amdvi_sysbus_migratable = {
     ....
     .subsections = (const VMStateDescription *const []) {
                    &vmstate_xt,
                    NULL
     }
};

I have tested above code.

Thanks
Sairaj

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

[Alejandro Jimenez]

On 11/26/25 7:13 AM, Sairaj Kodilkar wrote:
> 
> 
> On 11/26/2025 4:34 AM, Alejandro Jimenez wrote:
>> Hi Sairaj,
>>
>> I have a couple of suggestions, and one addition I believe is needed 
>> in the code, but overall looks good.
>>
>> On 11/18/25 3:24 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"
>>
>> I think it fits the language used by the spec slightly better.
>>
>>> guest has enabled xt support through control register[XTEn] and
>>
>> a guest has enabled x2APIC support ...
>>
>> I think it is better to replace "xt" above with "x2APIC", which 
>> describes what the XT feature is/does.
>>
>>> 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).
>>>
>>> Signed-off-by: Sairaj Kodilkar <sarunkod@amd.com>
>>> ---
>>>   hw/i386/amd_iommu.c | 3 ++-
>>>   hw/i386/amd_iommu.h | 4 +++-
>>>   2 files changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
>>> index a9ee7150ef17..7f08fc31111a 100644
>>> --- a/hw/i386/amd_iommu.c
>>> +++ b/hw/i386/amd_iommu.c
>>> @@ -1548,6 +1548,7 @@ 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;
>>
>> I think we should also include a new xten field in 
>> vmstate_amdvi_sysbus_migratable, to ensure the remapping behavior 
>> stays consistent after migration. i.e.
>>
>> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
>> index 9bf36ef608..5940011ef1 100644
>> --- a/hw/i386/amd_iommu.c
>> +++ b/hw/i386/amd_iommu.c
>> @@ -2452,6 +2452,7 @@ static const VMStateDescription 
>> vmstate_amdvi_sysbus_migratable = {
>>        /* Updated in  amdvi_handle_control_write() */
>>        VMSTATE_BOOL(enabled, AMDVIState),
>>        VMSTATE_BOOL(ga_enabled, AMDVIState),
>> +      VMSTATE_BOOL(xten, AMDVIState),
>>        /* bool ats_enabled is obsolete */
>>        VMSTATE_UNUSED(1), /* was ats_enabled */
>>        VMSTATE_BOOL(cmdbuf_enabled, AMDVIState),
> 
> Hi Alejandro,
> 
> I don't think adding a VMSTATE_BOOL directly is a good idea because 
> it'll break backward
> compatibility (kernel running on older qemu won't be able to migrate on 
> newer qemu because
> change in the stream length of the migration data). In this case we will 
> have to increment
> the min_version_number to stop migration from older qemu
> I am thinking to add a new subsection as it'll not break the 
> compatibility. Let me know what
> do you think about this.
> 

You are right, the subsection approach is the right way of doing this.
Given the lack of updates to the code until very recently, I think it is 
unlikely anyone is using migration... but since the feature is 
officially available, we need to do the right thing.

A problem remains if we need to support migration from newer to older 
(i.e. missing xten) QEMU versions. Then we need to have a .needed() 
method to avoid sending the subsection, or otherwise the migration fails 
due to the dest (i.e. the old version) not knowing about it. We can try 
to find a workaround, but my thinking is that the added complexity is 
not worth it, again considering the extremely low likelihood of actually 
breaking a user setup.

I don't know if there is a precedent for this type of choice, so let me 
do some research, and hopefully others in thread will also comment with 
their opinion/guidance.

Thank you,
Alejandro

> Something like this...
> 
> static const VMStateDescription vmstate_xt = {
>        .name = "amd-iommu-xt",
>        .version_id = 1,
>        .minimum_version_id = 1,
>        .fields = (VMStateField[]) {
>            VMSTATE_BOOL(xten, AMDVIState),
>        }
> };
> 
> static const VMStateDescription vmstate_amdvi_sysbus_migratable = {
>      ....
>      .subsections = (const VMStateDescription *const []) {
>                     &vmstate_xt,
>                     NULL
>      }
> };
> 
> I have tested above code.
> 
> Thanks
> Sairaj

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

[Sairaj Kodilkar]

On 11/27/2025 3:46 AM, Alejandro Jimenez wrote:
>
>
> On 11/26/25 7:13 AM, Sairaj Kodilkar wrote:
>>
>>
>> On 11/26/2025 4:34 AM, Alejandro Jimenez wrote:
>>> Hi Sairaj,
>>>
>>> I have a couple of suggestions, and one addition I believe is needed 
>>> in the code, but overall looks good.
>>>
>>> On 11/18/25 3:24 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"
>>>
>>> I think it fits the language used by the spec slightly better.
>>>
>>>> guest has enabled xt support through control register[XTEn] and
>>>
>>> a guest has enabled x2APIC support ...
>>>
>>> I think it is better to replace "xt" above with "x2APIC", which 
>>> describes what the XT feature is/does.
>>>
>>>> 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).
>>>>
>>>> Signed-off-by: Sairaj Kodilkar <sarunkod@amd.com>
>>>> ---
>>>>   hw/i386/amd_iommu.c | 3 ++-
>>>>   hw/i386/amd_iommu.h | 4 +++-
>>>>   2 files changed, 5 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
>>>> index a9ee7150ef17..7f08fc31111a 100644
>>>> --- a/hw/i386/amd_iommu.c
>>>> +++ b/hw/i386/amd_iommu.c
>>>> @@ -1548,6 +1548,7 @@ 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;
>>>
>>> I think we should also include a new xten field in 
>>> vmstate_amdvi_sysbus_migratable, to ensure the remapping behavior 
>>> stays consistent after migration. i.e.
>>>
>>> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
>>> index 9bf36ef608..5940011ef1 100644
>>> --- a/hw/i386/amd_iommu.c
>>> +++ b/hw/i386/amd_iommu.c
>>> @@ -2452,6 +2452,7 @@ static const VMStateDescription 
>>> vmstate_amdvi_sysbus_migratable = {
>>>        /* Updated in  amdvi_handle_control_write() */
>>>        VMSTATE_BOOL(enabled, AMDVIState),
>>>        VMSTATE_BOOL(ga_enabled, AMDVIState),
>>> +      VMSTATE_BOOL(xten, AMDVIState),
>>>        /* bool ats_enabled is obsolete */
>>>        VMSTATE_UNUSED(1), /* was ats_enabled */
>>>        VMSTATE_BOOL(cmdbuf_enabled, AMDVIState),
>>
>> Hi Alejandro,
>>
>> I don't think adding a VMSTATE_BOOL directly is a good idea because 
>> it'll break backward
>> compatibility (kernel running on older qemu won't be able to migrate 
>> on newer qemu because
>> change in the stream length of the migration data). In this case we 
>> will have to increment
>> the min_version_number to stop migration from older qemu
>> I am thinking to add a new subsection as it'll not break the 
>> compatibility. Let me know what
>> do you think about this.
>>
>
> You are right, the subsection approach is the right way of doing this.
> Given the lack of updates to the code until very recently, I think it 
> is unlikely anyone is using migration... but since the feature is 
> officially available, we need to do the right thing.
>
> A problem remains if we need to support migration from newer to older 
> (i.e. missing xten) QEMU versions. Then we need to have a .needed() 
> method to avoid sending the subsection, or otherwise the migration 
> fails due to the dest (i.e. the old version) not knowing about it. We 
> can try to find a workaround, but my thinking is that the added 
> complexity is not worth it, again considering the extremely low 
> likelihood of actually breaking a user setup.
>
> I don't know if there is a precedent for this type of choice, so let 
> me do some research, and hopefully others in thread will also comment 
> with their opinion/guidance.
>

I think .needed only useful when the field is rarely used. For our use 
case the guest will always
try to enable IOMMU XT interrupts. So we will anyway endup breaking the 
migration from newer
to older.

I think we can forbid the migration from newer to older. As you said the 
added complexity is not
worth it.

Thanks
Sairaj

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

[Alejandro Jimenez]

Hi Sairaj,

On 11/18/25 3:24 AM, Sairaj Kodilkar wrote:
> AMD IOMMU uses MMIO registers 0x170-0x180 to generate the interrupts
> when guest enables xt support through control register [IntCapXTEn]. The

"the interrupts" is a bit vague, considering that it is only a specific 
type of interrupts that originate from the IOMMU itself.
I think the quote from the spec is pretty clear and we might want to use 
just that:

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

I'd prefer if this was documented in the code comments, but I would go a 
step further and mention the specific interrupts, e.g.:

The mapping to MMIO offsets and interrupts controlled by each is as follows:

XT IOMMU General Interrupt Control Register (0x170): Event Log exception 
interrupts and Completion wait interrupts

XT IOMMU PPR Interrupt Control Register (0x178): PPR Log exception 
interrupts

XT IOMMU GA Log Interrupt Control Register (0x180): GA Log exception 
interrupts (irrelevant in vIOMMU case)


> 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 when the guest has enabled
> xt mode.
> 

At first I thought that statement was not correct. If that were the 
case, I was wondering why we don't currently have issues with Completion 
Wait, since they are also controlled by MMIO 0x170 offset. But the Linux 
driver doesn't rely on the completion interrupt AFAICT, it just sets the 
completion store bit and monitors the semaphore to detect the 
completion. We might not be so lucky with other OSs, so good catch.

> Introduce a new flag "intcapxten" which is set when guest writes control

And similarly to the xten case in PATCH 2, we also need to migrate this 
new field. It can be added to the your proposed vmstate_xt.

> 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>
> ---
>   hw/i386/amd_iommu.c  | 51 ++++++++++++++++++++++++++++++++++++++------
>   hw/i386/amd_iommu.h  |  3 +++
>   hw/i386/trace-events |  1 +
>   3 files changed, 49 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index 7f08fc31111a..c6bc13c93679 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -200,18 +200,52 @@ static void amdvi_assign_andq(AMDVIState *s, hwaddr addr, uint64_t val)
>      amdvi_writeq_raw(s, addr, amdvi_readq(s, addr) & val);
>   }
>   
> +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;
> +    };
> +};

We should define this union mmio_xt_intr in amd_iommu.h header, right 
after struct irte_ga.

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

Based on my reading from the MSI details, hardcoding redir_hint=0 
results in the dest_mode field essentially being a nop, since the 
messages will be delivered in physical mode i.e. only to the APIC ID in 
the dest field.

 From Intel's SDM Vol3, 12.11.1 Message Address Register Format:

• If RH is 0, then the DM bit is ignored and the message is sent ahead 
independent of whether the physical or logical destination mode is used.

I am not sure if the current implementations of AMD IOMMU drivers ever 
use the logical mode, but I am thinking we should at least catch that 
case with a warning e.g.
if (xt_reg.destination_mode) {
     error_report_once(...
}

A follow up question, since you chose to explicitly set redir_hint to 0, 
and something that bothers me about the remap functions is that they set:

irq->redir_hint = irte.lo.fields_remap.rq_eoi;

where AFAICT, redir_hint and rq_eoi are semantically different and 
control unrelated behaviors. So I've been wondering why these 
assignments are done. No need to answer this specifically, but if you 
have a better understanding of it please let me know.


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

This is great. The method of writing to the address space directly still 
confuses me, using the APIC helper for MSI delivery seems to be the 
appropriate way.

>   }
>   
>   static uint32_t get_next_eventlog_entry(AMDVIState *s)
> @@ -1490,6 +1524,7 @@ static inline void amdvi_mmio_get_name(hwaddr addr,
>       MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_BASE, name)
>       MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_HEAD, name)
>       MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_TAIL, name)
> +    MMIO_REG_TO_STRING(AMDVI_MMIO_XT_GEN_INTR, name)
>       default:
>           name = "UNHANDLED";
>       }
> @@ -1549,6 +1584,7 @@ static void amdvi_handle_control_write(AMDVIState *s)
>                           AMDVI_MMIO_CONTROL_CMDBUFLEN);
>       s->ga_enabled = !!(control & AMDVI_MMIO_CONTROL_GAEN);
>       s->xten = !!(control & AMDVI_MMIO_CONTROL_XTEN) && s->xtsup;
> +    s->intcapxten = !!(control & AMDVI_MMIO_CONTROL_INTCAPXTEN) && s->xtsup;
>   
I think that s->intcapxten should check for s->xten instead of s->xtsup, 
that way we create only one dependency on the input parameter, and the 
rest flows logically from what the guest driver configures.

On that topic (I should have mentioned this on patch 2), I think that it 
might be reasonable to make s->xten also conditional on whether 
s->ga_enabled is true. The spec just says they should both be set:

"In systems with x2APIC enabled, software must set MMIO 0x18[XTEn]=1 and 
MMIO 0x18[GAEn]=1.This enables the use of the 128-bit IRTE format with 
32-bit destination field. Even if Guest Virtual APIC
will not be used, software must set MMIO 0x18[GAEn]=1.
"

my understanding is that we can't support X2APIC mode without 128-bit 
IRTE format (which is controlled by ga_enabled), so it makes sense to 
block X2APIC (i.e. xten) unless ga_enabled is already set.

So in the end we'd have something like:

s->ga_enabled = !!(control & AMDVI_MMIO_CONTROL_GAEN);
s->xten = !!(control & AMDVI_MMIO_CONTROL_XTEN) &&
	  s->xtsup && s->ga_enabled;  // this should go in PATCH 2
s->intcapxten = !!(control & AMDVI_MMIO_CONTROL_INTCAPXTEN) && s->xten;

What do you think?

Thank you,
Alejandro

>       /* update the flags depending on the control register */
>       if (s->cmdbuf_enabled) {
> @@ -1755,6 +1791,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;
>       }
>   }
>   
> diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
> index 32467d0bc241..399a4fb748e5 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)
> @@ -421,6 +423,7 @@ struct AMDVIState {
>       bool ga_enabled;
>       bool xtsup;     /* xtsup=on command line */
>       bool xten;      /* Enable x2apic */
> +    bool intcapxten; /* Enable IOMMU x2apic interrupt generation */
>   
>       /* DMA address translation */
>       bool dma_remap;
> diff --git a/hw/i386/trace-events b/hw/i386/trace-events
> index b704f4f90c3d..fe7aea4507ae 100644
> --- a/hw/i386/trace-events
> +++ b/hw/i386/trace-events
> @@ -114,6 +114,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 3/3] amd_iommu: Generate XT interrupts when xt support is enabled

[Sairaj Kodilkar]

On 12/3/2025 11:39 PM, Alejandro Jimenez wrote:

Hi alejandro,

> Hi Sairaj,
>
> On 11/18/25 3:24 AM, Sairaj Kodilkar wrote:
>> AMD IOMMU uses MMIO registers 0x170-0x180 to generate the interrupts
>> when guest enables xt support through control register [IntCapXTEn]. The
>
> "the interrupts" is a bit vague, considering that it is only a 
> specific type of interrupts that originate from the IOMMU itself.
> I think the quote from the spec is pretty clear and we might want to 
> use just that:
>
> "
> 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.
> "
>
> I'd prefer if this was documented in the code comments, but I would go 
> a step further and mention the specific interrupts, e.g.:
>
> The mapping to MMIO offsets and interrupts controlled by each is as 
> follows:
>
> XT IOMMU General Interrupt Control Register (0x170): Event Log 
> exception interrupts and Completion wait interrupts
>
> XT IOMMU PPR Interrupt Control Register (0x178): PPR Log exception 
> interrupts
>
> XT IOMMU GA Log Interrupt Control Register (0x180): GA Log exception 
> interrupts (irrelevant in vIOMMU case)
>
>

I think we can avoid mentioning other two registers (PPR and GAlog) as 
we do not plan to support these
features as of now.

>> 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 when the guest has enabled
>> xt mode.
>>
>
> At first I thought that statement was not correct. If that were the 
> case, I was wondering why we don't currently have issues with 
> Completion Wait, since they are also controlled by MMIO 0x170 offset. 
> But the Linux driver doesn't rely on the completion interrupt AFAICT, 
> it just sets the completion store bit and monitors the semaphore to 
> detect the completion. We might not be so lucky with other OSs, so 
> good catch.
>

Maybe this patch qualifies for the fixes tag ?

>> Introduce a new flag "intcapxten" which is set when guest writes control
>
> And similarly to the xten case in PATCH 2, we also need to migrate 
> this new field. It can be added to the your proposed vmstate_xt.

Right

>
>> 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>
>> ---
>>   hw/i386/amd_iommu.c  | 51 ++++++++++++++++++++++++++++++++++++++------
>>   hw/i386/amd_iommu.h  |  3 +++
>>   hw/i386/trace-events |  1 +
>>   3 files changed, 49 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
>> index 7f08fc31111a..c6bc13c93679 100644
>> --- a/hw/i386/amd_iommu.c
>> +++ b/hw/i386/amd_iommu.c
>> @@ -200,18 +200,52 @@ static void amdvi_assign_andq(AMDVIState *s, 
>> hwaddr addr, uint64_t val)
>>      amdvi_writeq_raw(s, addr, amdvi_readq(s, addr) & val);
>>   }
>>   +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;
>> +    };
>> +};
>
> We should define this union mmio_xt_intr in amd_iommu.h header, right 
> after struct irte_ga.
>

Right.

>> +
>> +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;
>> +
>
> Based on my reading from the MSI details, hardcoding redir_hint=0 
> results in the dest_mode field essentially being a nop, since the 
> messages will be delivered in physical mode i.e. only to the APIC ID 
> in the dest field.
>
> From Intel's SDM Vol3, 12.11.1 Message Address Register Format:
>
> • If RH is 0, then the DM bit is ignored and the message is sent ahead 
> independent of whether the physical or logical destination mode is used.
>

This statement sounds ambiguious. My understanding is that the RH field 
is used to determine
the "lowest priority processor" among all possible cpus that can receive 
the interrupt. This means
that in case of physical destination mode, it should forward the 
interrupt to the cpu with matching
destination mode but in case of logical destination mode it should 
forward the interrupt to the
cpu with lowest priority.

I confirmed this by looking at kernel KVM function --> 
kvm_irq_delivery_to_apic(). So I think the
what SDM is trying to tell by "DM bit is ignored is that" is that it 
will not try to find the cpu
with lowest priority interrupt but rather forward it to the APIC 
hardware directly (i.e. to all the
logical processors in case of logical destination mode).

So we don't need to add a error report here.

> I am not sure if the current implementations of AMD IOMMU drivers ever 
> use the logical mode, but I am thinking we should at least catch that 
> case with a warning e.g.
> if (xt_reg.destination_mode) {
>     error_report_once(...
> }
>
> A follow up question, since you chose to explicitly set redir_hint to 
> 0, and something that bothers me about the remap functions is that 
> they set:
>
> irq->redir_hint = irte.lo.fields_remap.rq_eoi;
>
> where AFAICT, redir_hint and rq_eoi are semantically different and 
> control unrelated behaviors. So I've been wondering why these 
> assignments are done. No need to answer this specifically, but if you 
> have a better understanding of it please let me know.
>

Currently I don't  have idea about this. The specification does not 
provide any relation between
rq_eoi and redir_hint.


>
>> + 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);
>
> This is great. The method of writing to the address space directly 
> still confuses me, using the APIC helper for MSI delivery seems to be 
> the appropriate way.
>
>>   }
>>     static uint32_t get_next_eventlog_entry(AMDVIState *s)
>> @@ -1490,6 +1524,7 @@ static inline void amdvi_mmio_get_name(hwaddr 
>> addr,
>>       MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_BASE, name)
>>       MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_HEAD, name)
>>       MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_TAIL, name)
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_XT_GEN_INTR, name)
>>       default:
>>           name = "UNHANDLED";
>>       }
>> @@ -1549,6 +1584,7 @@ static void 
>> amdvi_handle_control_write(AMDVIState *s)
>>                           AMDVI_MMIO_CONTROL_CMDBUFLEN);
>>       s->ga_enabled = !!(control & AMDVI_MMIO_CONTROL_GAEN);
>>       s->xten = !!(control & AMDVI_MMIO_CONTROL_XTEN) && s->xtsup;
>> +    s->intcapxten = !!(control & AMDVI_MMIO_CONTROL_INTCAPXTEN) && 
>> s->xtsup;
> I think that s->intcapxten should check for s->xten instead of 
> s->xtsup, that way we create only one dependency on the input 
> parameter, and the rest flows logically from what the guest driver 
> configures.

I did think about it quite a bit, the specs does not provide any dependency
between intcapxten and xtsup, that's why I avoided.

>
> On that topic (I should have mentioned this on patch 2), I think that 
> it might be reasonable to make s->xten also conditional on whether 
> s->ga_enabled is true. The spec just says they should both be set:
>
> "In systems with x2APIC enabled, software must set MMIO 0x18[XTEn]=1 
> and MMIO 0x18[GAEn]=1.This enables the use of the 128-bit IRTE format 
> with 32-bit destination field. Even if Guest Virtual APIC
> will not be used, software must set MMIO 0x18[GAEn]=1.
> "
>
> my understanding is that we can't support X2APIC mode without 128-bit 
> IRTE format (which is controlled by ga_enabled), so it makes sense to 
> block X2APIC (i.e. xten) unless ga_enabled is already set.

Yep I agree with this one, we can enforce ga_enabled but then we cannot 
have

  s->intcapxten = !!(control & AMDVI_MMIO_CONTROL_INTCAPXTEN) && s->xten;

because it'll create a dependency between s->intcapxten and s->ga_enabled


>
> So in the end we'd have something like:
>
> s->ga_enabled = !!(control & AMDVI_MMIO_CONTROL_GAEN);
> s->xten = !!(control & AMDVI_MMIO_CONTROL_XTEN) &&
>       s->xtsup && s->ga_enabled;  // this should go in PATCH 2
> s->intcapxten = !!(control & AMDVI_MMIO_CONTROL_INTCAPXTEN) && s->xten;
>
> What do you think?
>

PS: just a small formatting suggestion: Please keep the charater per 
line within
~72 so that its easier to read on the email client

Thanks
Sairaj