public inbox for qemu-devel@nongnu.org
 help / color / mirror / Atom feed
* [PATCH v3 0/3] amd_iommu: Support Generation of IOMMU XT interrupts
@ 2026-03-02 11:51 Sairaj Kodilkar
  2026-03-02 11:51 ` [PATCH v3 1/3] amd_iommu: Use switch case to determine mmio register name Sairaj Kodilkar
                   ` (3 more replies)
  0 siblings, 4 replies; 16+ messages in thread
From: Sairaj Kodilkar @ 2026-03-02 11:51 UTC (permalink / raw)
  To: qemu-devel
  Cc: alejandro.j.jimenez, pbonzini, richard.henderson, eduardo, mst,
	marcel.apfelbaum, vasant.hegde, clement.mathieu--drif,
	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 v2:
https://lore.kernel.org/qemu-devel/20260129102814.4488-1-sarunkod@amd.com/
Patch 1:
    - Delete  amdvi_mmio_trace_{read,write} and AMDVI_MMIO_REGS_{LOW,HIGH}
      definitions [AJ]
    - Move MMIO_REG_TO_STRING definition inside amdvi_mmio_get_name() [CM]
Patch 2: Improve commit message [AJ]
Patch 3: Improve commit message and comment [AJ]


Changes since v1:
https://lore.kernel.org/qemu-devel/20251118082403.3455-1-sarunkod@amd.com/
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 07f97d5da04a

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

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  | 144 ++++++++++++++++++++++++++-----------------
 hw/i386/amd_iommu.h  |  25 ++++++--
 hw/i386/trace-events |   1 +
 3 files changed, 108 insertions(+), 62 deletions(-)

-- 
2.34.1



^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v3 1/3] amd_iommu: Use switch case to determine mmio register name
  2026-03-02 11:51 [PATCH v3 0/3] amd_iommu: Support Generation of IOMMU XT interrupts Sairaj Kodilkar
@ 2026-03-02 11:51 ` Sairaj Kodilkar
  2026-03-12 21:59   ` Alejandro Jimenez
  2026-03-02 11:51 ` [PATCH v3 2/3] amd_iommu: Turn on XT support only when guest has enabled it Sairaj Kodilkar
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 16+ messages in thread
From: Sairaj Kodilkar @ 2026-03-02 11:51 UTC (permalink / raw)
  To: qemu-devel
  Cc: alejandro.j.jimenez, pbonzini, richard.henderson, eduardo, mst,
	marcel.apfelbaum, vasant.hegde, clement.mathieu--drif,
	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>
Reviewed-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
---
 hw/i386/amd_iommu.c | 76 ++++++++++++++++-----------------------------
 hw/i386/amd_iommu.h |  4 ---
 2 files changed, 27 insertions(+), 53 deletions(-)

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 789e09d6f2bc..f5aa9c763d02 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -35,29 +35,6 @@
 #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"
-};
-
 struct AMDVIAddressSpace {
     PCIBus *bus;                /* PCIBus (for bus number)              */
     uint8_t devfn;              /* device function                      */
@@ -1484,31 +1461,31 @@ 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) {
+#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_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);
+#undef MMIO_REG_TO_STRING
+    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 +1505,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 +1661,8 @@ 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, offset);
+
     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 302ccca5121f..ca4ff9fffee3 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
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v3 2/3] amd_iommu: Turn on XT support only when guest has enabled it
  2026-03-02 11:51 [PATCH v3 0/3] amd_iommu: Support Generation of IOMMU XT interrupts Sairaj Kodilkar
  2026-03-02 11:51 ` [PATCH v3 1/3] amd_iommu: Use switch case to determine mmio register name Sairaj Kodilkar
@ 2026-03-02 11:51 ` Sairaj Kodilkar
  2026-03-03  9:39   ` Vasant Hegde
  2026-03-02 11:51 ` [PATCH v3 3/3] amd_iommu: Generate XT interrupts when xt support is enabled Sairaj Kodilkar
  2026-03-17  1:10 ` [PATCH v3 0/3] amd_iommu: Support Generation of IOMMU XT interrupts Alejandro Jimenez
  3 siblings, 1 reply; 16+ messages in thread
From: Sairaj Kodilkar @ 2026-03-02 11:51 UTC (permalink / raw)
  To: qemu-devel
  Cc: alejandro.j.jimenez, pbonzini, richard.henderson, eduardo, mst,
	marcel.apfelbaum, vasant.hegde, clement.mathieu--drif,
	Sairaj Kodilkar

Current code uses 32 bit destination ID 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>
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 f5aa9c763d02..4a86e62a3b92 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -1523,6 +1523,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) {
@@ -1996,7 +1998,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 {
@@ -2379,6 +2381,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);
@@ -2443,6 +2446,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,
@@ -2487,7 +2500,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 ca4ff9fffee3..d39019b216af 100644
--- a/hw/i386/amd_iommu.h
+++ b/hw/i386/amd_iommu.h
@@ -102,6 +102,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)
@@ -414,7 +415,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



^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v3 3/3] amd_iommu: Generate XT interrupts when xt support is enabled
  2026-03-02 11:51 [PATCH v3 0/3] amd_iommu: Support Generation of IOMMU XT interrupts Sairaj Kodilkar
  2026-03-02 11:51 ` [PATCH v3 1/3] amd_iommu: Use switch case to determine mmio register name Sairaj Kodilkar
  2026-03-02 11:51 ` [PATCH v3 2/3] amd_iommu: Turn on XT support only when guest has enabled it Sairaj Kodilkar
@ 2026-03-02 11:51 ` Sairaj Kodilkar
  2026-03-03  9:47   ` Vasant Hegde
  2026-03-17  1:10 ` [PATCH v3 0/3] amd_iommu: Support Generation of IOMMU XT interrupts Alejandro Jimenez
  3 siblings, 1 reply; 16+ messages in thread
From: Sairaj Kodilkar @ 2026-03-02 11:51 UTC (permalink / raw)
  To: qemu-devel
  Cc: alejandro.j.jimenez, pbonzini, richard.henderson, eduardo, mst,
	marcel.apfelbaum, vasant.hegde, clement.mathieu--drif,
	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 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>
Reviewed-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
---
 hw/i386/amd_iommu.c  | 47 ++++++++++++++++++++++++++++++++++++++------
 hw/i386/amd_iommu.h  | 17 ++++++++++++++++
 hw/i386/trace-events |  1 +
 3 files changed, 59 insertions(+), 6 deletions(-)

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 4a86e62a3b92..bd24fdee7cfe 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -192,18 +192,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)
@@ -1482,6 +1502,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);
 #undef MMIO_REG_TO_STRING
     default:
         return "UNHANDLED";
@@ -1525,6 +1546,15 @@ 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 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).
+     */
+    s->intcapxten = !!(control & AMDVI_MMIO_CONTROL_INTCAPXTEN) && s->xtsup;
 
     /* update the flags depending on the control register */
     if (s->cmdbuf_enabled) {
@@ -1732,6 +1762,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;
     }
 }
 
@@ -2382,6 +2415,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);
@@ -2452,6 +2486,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 d39019b216af..72139bb0c70b 100644
--- a/hw/i386/amd_iommu.h
+++ b/hw/i386/amd_iommu.h
@@ -53,6 +53,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
@@ -103,6 +104,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)
@@ -339,6 +341,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)
 
@@ -417,6 +433,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



^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 2/3] amd_iommu: Turn on XT support only when guest has enabled it
  2026-03-02 11:51 ` [PATCH v3 2/3] amd_iommu: Turn on XT support only when guest has enabled it Sairaj Kodilkar
@ 2026-03-03  9:39   ` Vasant Hegde
  2026-03-03 10:20     ` Sairaj Kodilkar
  0 siblings, 1 reply; 16+ messages in thread
From: Vasant Hegde @ 2026-03-03  9:39 UTC (permalink / raw)
  To: Sairaj Kodilkar, qemu-devel
  Cc: alejandro.j.jimenez, pbonzini, richard.henderson, eduardo, mst,
	marcel.apfelbaum, clement.mathieu--drif



On 3/2/2026 5:21 PM, Sairaj Kodilkar wrote:
> Current code uses 32 bit destination ID 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>
> 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 f5aa9c763d02..4a86e62a3b92 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -1523,6 +1523,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) {
> @@ -1996,7 +1998,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 {
> @@ -2379,6 +2381,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);
> @@ -2443,6 +2446,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,

Wrong indentation. Otherwise patch looks good.

-Vasant



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 3/3] amd_iommu: Generate XT interrupts when xt support is enabled
  2026-03-02 11:51 ` [PATCH v3 3/3] amd_iommu: Generate XT interrupts when xt support is enabled Sairaj Kodilkar
@ 2026-03-03  9:47   ` Vasant Hegde
  2026-03-03  9:55     ` Sairaj Kodilkar
  0 siblings, 1 reply; 16+ messages in thread
From: Vasant Hegde @ 2026-03-03  9:47 UTC (permalink / raw)
  To: Sairaj Kodilkar, qemu-devel
  Cc: alejandro.j.jimenez, pbonzini, richard.henderson, eduardo, mst,
	marcel.apfelbaum, clement.mathieu--drif



On 3/2/2026 5:21 PM, 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 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>
> Reviewed-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
> ---
>  hw/i386/amd_iommu.c  | 47 ++++++++++++++++++++++++++++++++++++++------
>  hw/i386/amd_iommu.h  | 17 ++++++++++++++++
>  hw/i386/trace-events |  1 +
>  3 files changed, 59 insertions(+), 6 deletions(-)
> 

.../...

>  
>      /* update the flags depending on the control register */
>      if (s->cmdbuf_enabled) {
> @@ -1732,6 +1762,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;
>      }
>  }
>  
> @@ -2382,6 +2415,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);
> @@ -2452,6 +2486,7 @@ static const VMStateDescription vmstate_xt = {
>         .minimum_version_id = 1,
>         .fields = (VMStateField[]) {
>             VMSTATE_BOOL(xten, AMDVIState),
> +           VMSTATE_BOOL(intcapxten, AMDVIState),

Do we need to increase the version no?

-Vasant



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 3/3] amd_iommu: Generate XT interrupts when xt support is enabled
  2026-03-03  9:47   ` Vasant Hegde
@ 2026-03-03  9:55     ` Sairaj Kodilkar
  2026-03-13  0:16       ` Alejandro Jimenez
  0 siblings, 1 reply; 16+ messages in thread
From: Sairaj Kodilkar @ 2026-03-03  9:55 UTC (permalink / raw)
  To: Vasant Hegde, qemu-devel
  Cc: sarunkod, alejandro.j.jimenez, pbonzini, richard.henderson,
	eduardo, mst, marcel.apfelbaum, clement.mathieu--drif



On 3/3/2026 3:17 PM, Vasant Hegde wrote:
>
> On 3/2/2026 5:21 PM, 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 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>
>> Reviewed-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
>> ---
>>   hw/i386/amd_iommu.c  | 47 ++++++++++++++++++++++++++++++++++++++------
>>   hw/i386/amd_iommu.h  | 17 ++++++++++++++++
>>   hw/i386/trace-events |  1 +
>>   3 files changed, 59 insertions(+), 6 deletions(-)
>>
> .../...
>
>>   
>>       /* update the flags depending on the control register */
>>       if (s->cmdbuf_enabled) {
>> @@ -1732,6 +1762,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;
>>       }
>>   }
>>   
>> @@ -2382,6 +2415,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);
>> @@ -2452,6 +2486,7 @@ static const VMStateDescription vmstate_xt = {
>>          .minimum_version_id = 1,
>>          .fields = (VMStateField[]) {
>>              VMSTATE_BOOL(xten, AMDVIState),
>> +           VMSTATE_BOOL(intcapxten, AMDVIState),
> Do we need to increase the version no?

No, because we have introduced a separate subsection, the older and newer
qemu are compatible.

Thanks
Sairaj


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 2/3] amd_iommu: Turn on XT support only when guest has enabled it
  2026-03-03  9:39   ` Vasant Hegde
@ 2026-03-03 10:20     ` Sairaj Kodilkar
  0 siblings, 0 replies; 16+ messages in thread
From: Sairaj Kodilkar @ 2026-03-03 10:20 UTC (permalink / raw)
  To: Vasant Hegde, qemu-devel
  Cc: sarunkod, alejandro.j.jimenez, pbonzini, richard.henderson,
	eduardo, mst, marcel.apfelbaum, clement.mathieu--drif



On 3/3/2026 3:09 PM, Vasant Hegde wrote:
>
> On 3/2/2026 5:21 PM, Sairaj Kodilkar wrote:
>> Current code uses 32 bit destination ID 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>
>> 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 f5aa9c763d02..4a86e62a3b92 100644
>> --- a/hw/i386/amd_iommu.c
>> +++ b/hw/i386/amd_iommu.c
>> @@ -1523,6 +1523,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) {
>> @@ -1996,7 +1998,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 {
>> @@ -2379,6 +2381,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);
>> @@ -2443,6 +2446,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,
> Wrong indentation. Otherwise patch looks good.

Ahh right, Not sure why checkpatch.pl missed it.

Thanks
Sairaj

>
> -Vasant
>



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 1/3] amd_iommu: Use switch case to determine mmio register name
  2026-03-02 11:51 ` [PATCH v3 1/3] amd_iommu: Use switch case to determine mmio register name Sairaj Kodilkar
@ 2026-03-12 21:59   ` Alejandro Jimenez
  2026-03-13  0:36     ` Alejandro Jimenez
  2026-03-13  4:49     ` Sairaj Kodilkar
  0 siblings, 2 replies; 16+ messages in thread
From: Alejandro Jimenez @ 2026-03-12 21:59 UTC (permalink / raw)
  To: Sairaj Kodilkar, qemu-devel
  Cc: pbonzini, richard.henderson, eduardo, mst, marcel.apfelbaum,
	vasant.hegde, clement.mathieu--drif

Hi Sairaj,

On 3/2/26 6:51 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>
> Reviewed-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
> ---
>  hw/i386/amd_iommu.c | 76 ++++++++++++++++-----------------------------
>  hw/i386/amd_iommu.h |  4 ---
>  2 files changed, 27 insertions(+), 53 deletions(-)
> 
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index 789e09d6f2bc..f5aa9c763d02 100644

[...]

> -
> -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 +1505,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 +1661,8 @@ 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, offset);
> +

There is a small change in the trace output here from the previous patches,
and it is because the use of offset in the last argument vs the previous
patches which still use (addr & ~0x07).
My understanding is that the offset the trace uses is the MMIO offset
(which gives a mapping between the name of the register we print and its
offset in the MMIO space).

The offset variable that amdvi_mmio_write() calculates earlier is the byte
offset within the MMIO register, and it is needed for those special cases
(i.e. hacks) where a guest driver does not write with a full 64-bit write
to the register, so that we need to delay the side effect until the MMIO
register has been fully updated. That is my understanding of the code that
does:

1709         if (offset || (size == 8)) {

1710             amdvi_handle_devtab_write(s);

1711         }


i.e. don't apply the architectural side effects unless we write to the full
64-bit register at once, or offset is not zero. This last condition meaning
we do word writes, and write to the high word last i.e. offset !=0.

So in short, I will fix the call to trace_amdvi_mmio_write() to continue to
use (addr & ~0x07), and will also change the switch condition in
amdvi_mmio_get_name() to use the same.
No need to send another revision, but I just wanted to highlight the
difference so you are aware of the change.

Thank you,
Alejandro


>      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 302ccca5121f..ca4ff9fffee3 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



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 3/3] amd_iommu: Generate XT interrupts when xt support is enabled
  2026-03-03  9:55     ` Sairaj Kodilkar
@ 2026-03-13  0:16       ` Alejandro Jimenez
  2026-03-13  5:20         ` Sairaj Kodilkar
  0 siblings, 1 reply; 16+ messages in thread
From: Alejandro Jimenez @ 2026-03-13  0:16 UTC (permalink / raw)
  To: Sairaj Kodilkar, Vasant Hegde, qemu-devel
  Cc: pbonzini, richard.henderson, eduardo, mst, marcel.apfelbaum,
	clement.mathieu--drif



On 3/3/26 4:55 AM, Sairaj Kodilkar wrote:
> 
> 
> On 3/3/2026 3:17 PM, Vasant Hegde wrote:
>>
>> On 3/2/2026 5:21 PM, 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 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>
>>> Reviewed-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
>>> ---
>>>   hw/i386/amd_iommu.c  | 47 ++++++++++++++++++++++++++++++++++++++------
>>>   hw/i386/amd_iommu.h  | 17 ++++++++++++++++
>>>   hw/i386/trace-events |  1 +
>>>   3 files changed, 59 insertions(+), 6 deletions(-)
>>>
>> .../...
>>
>>>         /* update the flags depending on the control register */
>>>       if (s->cmdbuf_enabled) {
>>> @@ -1732,6 +1762,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;
>>>       }
>>>   }
>>>   @@ -2382,6 +2415,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);
>>> @@ -2452,6 +2486,7 @@ static const VMStateDescription vmstate_xt = {
>>>          .minimum_version_id = 1,
>>>          .fields = (VMStateField[]) {
>>>              VMSTATE_BOOL(xten, AMDVIState),
>>> +           VMSTATE_BOOL(intcapxten, AMDVIState),
>> Do we need to increase the version no?
> 
> No, because we have introduced a separate subsection, the older and newer
> qemu are compatible.
> 
I thought that was the case because the changes will still be part of the
same "release". I don't believe we guarantee migration compatibility
between random/intermediate commits, but...

If we are going to follow the guidelines strictly then I think Vasant's
observation is correct. The patch changes the layout of the subsection so
we are in the same scenario that lead us to include a subsection to begin with.

Because the two new values are still part of the same xt support domain, I
think it makes sense to keep them in the same subsection and the simplest
way is to do:

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

That change is on top of your current patch. There seems to be precedent
for this based on my search in the git log. If you are ok with this
approach let me know and I'll apply it, no need to send a new revision.

Thank you,
Alejandro


> Thanks
> Sairaj



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 1/3] amd_iommu: Use switch case to determine mmio register name
  2026-03-12 21:59   ` Alejandro Jimenez
@ 2026-03-13  0:36     ` Alejandro Jimenez
  2026-03-13  4:49     ` Sairaj Kodilkar
  1 sibling, 0 replies; 16+ messages in thread
From: Alejandro Jimenez @ 2026-03-13  0:36 UTC (permalink / raw)
  To: Sairaj Kodilkar, qemu-devel
  Cc: pbonzini, richard.henderson, eduardo, mst, marcel.apfelbaum,
	vasant.hegde, clement.mathieu--drif



On 3/12/26 5:59 PM, Alejandro Jimenez wrote:
> Hi Sairaj,
> 
> On 3/2/26 6:51 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>
>> Reviewed-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
>> ---
>>  hw/i386/amd_iommu.c | 76 ++++++++++++++++-----------------------------
>>  hw/i386/amd_iommu.h |  4 ---
>>  2 files changed, 27 insertions(+), 53 deletions(-)
>>
>> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
>> index 789e09d6f2bc..f5aa9c763d02 100644
> 
> [...]
> 
>> -
>> -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 +1505,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 +1661,8 @@ 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, offset);
>> +
> 
> There is a small change in the trace output here from the previous patches,
> and it is because the use of offset in the last argument vs the previous
> patches which still use (addr & ~0x07).
> My understanding is that the offset the trace uses is the MMIO offset
> (which gives a mapping between the name of the register we print and its
> offset in the MMIO space).
> 
> The offset variable that amdvi_mmio_write() calculates earlier is the byte
> offset within the MMIO register, and it is needed for those special cases
> (i.e. hacks) where a guest driver does not write with a full 64-bit write
> to the register, so that we need to delay the side effect until the MMIO
> register has been fully updated. That is my understanding of the code that
> does:
> 
> 1709         if (offset || (size == 8)) {
> 
> 1710             amdvi_handle_devtab_write(s);
> 
> 1711         }
> 
> 
> i.e. don't apply the architectural side effects unless we write to the full
> 64-bit register at once, or offset is not zero. This last condition meaning
> we do word writes, and write to the high word last i.e. offset !=0.
> 
To clarify because I made a mistake above and this is confusing enough:

I should have written dword (i.e. 32-bits) instead of word. So the hack is
accounting for two 32-bit writes, first to low and then to higher 32bits of
these registers mapped to the MMIO space. We should probably guard against
more finely grained accesses, since IIUC the spec says byte accesses are
possible... but that is a different can of worms.

Hopefully less confusing that before...
Alejandro

> So in short, I will fix the call to trace_amdvi_mmio_write() to continue to
> use (addr & ~0x07), and will also change the switch condition in
> amdvi_mmio_get_name() to use the same.
> No need to send another revision, but I just wanted to highlight the
> difference so you are aware of the change.
> 
> Thank you,
> Alejandro
> 
> 
>>      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 302ccca5121f..ca4ff9fffee3 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
> 
> 



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 1/3] amd_iommu: Use switch case to determine mmio register name
  2026-03-12 21:59   ` Alejandro Jimenez
  2026-03-13  0:36     ` Alejandro Jimenez
@ 2026-03-13  4:49     ` Sairaj Kodilkar
  1 sibling, 0 replies; 16+ messages in thread
From: Sairaj Kodilkar @ 2026-03-13  4:49 UTC (permalink / raw)
  To: Alejandro Jimenez, qemu-devel
  Cc: sarunkod, pbonzini, richard.henderson, eduardo, mst,
	marcel.apfelbaum, vasant.hegde, clement.mathieu--drif



On 3/13/2026 3:29 AM, Alejandro Jimenez wrote:
> Hi Sairaj,
>
> On 3/2/26 6:51 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>
>> Reviewed-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
>> ---
>>   hw/i386/amd_iommu.c | 76 ++++++++++++++++-----------------------------
>>   hw/i386/amd_iommu.h |  4 ---
>>   2 files changed, 27 insertions(+), 53 deletions(-)
>>
>> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
>> index 789e09d6f2bc..f5aa9c763d02 100644
> [...]
>
>> -
>> -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 +1505,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 +1661,8 @@ 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, offset);
>> +
> There is a small change in the trace output here from the previous patches,
> and it is because the use of offset in the last argument vs the previous
> patches which still use (addr & ~0x07).
> My understanding is that the offset the trace uses is the MMIO offset
> (which gives a mapping between the name of the register we print and its
> offset in the MMIO space).
>
> The offset variable that amdvi_mmio_write() calculates earlier is the byte
> offset within the MMIO register, and it is needed for those special cases
> (i.e. hacks) where a guest driver does not write with a full 64-bit write
> to the register, so that we need to delay the side effect until the MMIO
> register has been fully updated. That is my understanding of the code that
> does:
>
> 1709         if (offset || (size == 8)) {
>
> 1710             amdvi_handle_devtab_write(s);
>
> 1711         }
>
>
> i.e. don't apply the architectural side effects unless we write to the full
> 64-bit register at once, or offset is not zero. This last condition meaning
> we do word writes, and write to the high word last i.e. offset !=0.
>
> So in short, I will fix the call to trace_amdvi_mmio_write() to continue to
> use (addr & ~0x07), and will also change the switch condition in
> amdvi_mmio_get_name() to use the same.
> No need to send another revision, but I just wanted to highlight the
> difference so you are aware of the change.
>
> Thank you,
> Alejandro
>

Hey Alejandro,

Thanks, for pointing this out. I got confused because of the same naming
i.e. offset in both the cases and missed '~'.
I should have used addr & ~0x07 directly.

Thanks
Sairaj

>>       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 302ccca5121f..ca4ff9fffee3 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



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 3/3] amd_iommu: Generate XT interrupts when xt support is enabled
  2026-03-13  0:16       ` Alejandro Jimenez
@ 2026-03-13  5:20         ` Sairaj Kodilkar
  2026-03-13 16:17           ` Alejandro Jimenez
  0 siblings, 1 reply; 16+ messages in thread
From: Sairaj Kodilkar @ 2026-03-13  5:20 UTC (permalink / raw)
  To: Alejandro Jimenez, Vasant Hegde, qemu-devel
  Cc: sarunkod, pbonzini, richard.henderson, eduardo, mst,
	marcel.apfelbaum, clement.mathieu--drif



On 3/13/2026 5:46 AM, Alejandro Jimenez wrote:
>
> On 3/3/26 4:55 AM, Sairaj Kodilkar wrote:
>>
>> On 3/3/2026 3:17 PM, Vasant Hegde wrote:
>>> On 3/2/2026 5:21 PM, 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 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>
>>>> Reviewed-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
>>>> ---
>>>>    hw/i386/amd_iommu.c  | 47 ++++++++++++++++++++++++++++++++++++++------
>>>>    hw/i386/amd_iommu.h  | 17 ++++++++++++++++
>>>>    hw/i386/trace-events |  1 +
>>>>    3 files changed, 59 insertions(+), 6 deletions(-)
>>>>
>>> .../...
>>>
>>>>          /* update the flags depending on the control register */
>>>>        if (s->cmdbuf_enabled) {
>>>> @@ -1732,6 +1762,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;
>>>>        }
>>>>    }
>>>>    @@ -2382,6 +2415,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);
>>>> @@ -2452,6 +2486,7 @@ static const VMStateDescription vmstate_xt = {
>>>>           .minimum_version_id = 1,
>>>>           .fields = (VMStateField[]) {
>>>>               VMSTATE_BOOL(xten, AMDVIState),
>>>> +           VMSTATE_BOOL(intcapxten, AMDVIState),
>>> Do we need to increase the version no?
>> No, because we have introduced a separate subsection, the older and newer
>> qemu are compatible.
>>
> I thought that was the case because the changes will still be part of the
> same "release". I don't believe we guarantee migration compatibility
> between random/intermediate commits, but...
>
> If we are going to follow the guidelines strictly then I think Vasant's
> observation is correct. The patch changes the layout of the subsection so
> we are in the same scenario that lead us to include a subsection to begin with.
>
> Because the two new values are still part of the same xt support domain, I
> think it makes sense to keep them in the same subsection and the simplest
> way is to do:
>
>   static const VMStateDescription vmstate_xt = {
>       .name = "amd-iommu-xt",
> -    .version_id = 1,
> +    .version_id = 2,
>       .minimum_version_id = 1,
>       .fields = (VMStateField[]) {
>           VMSTATE_BOOL(xten, AMDVIState),
> -        VMSTATE_BOOL(intcapxten, AMDVIState),
> +        VMSTATE_BOOL_V(intcapxten, AMDVIState, 2),
>           VMSTATE_END_OF_LIST()
>       }
>   };
>
> That change is on top of your current patch. There seems to be precedent
> for this based on my search in the git log. If you are ok with this
> approach let me know and I'll apply it, no need to send a new revision.

I am not sure if it is really useful here. Because without this patch
xt-support will not work and migrating from vm which only has first two
patches to the vm which has all three patches does not make sense to me.
I think we should treat three patches as a single unit, let me know what
do you think about this ?

Or we can do something like [1], which introduces new subsections
for each capbility.
https://lore.kernel.org/qemu-devel/20180119050005.29392-1-sjitindarsingh@gmail.com/

But this makes sense only when you can use each capability separately.

Thanks
Sairaj
>
> Thank you,
> Alejandro
>
>
>> Thanks
>> Sairaj



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 3/3] amd_iommu: Generate XT interrupts when xt support is enabled
  2026-03-13  5:20         ` Sairaj Kodilkar
@ 2026-03-13 16:17           ` Alejandro Jimenez
  2026-03-16  7:04             ` Sairaj Kodilkar
  0 siblings, 1 reply; 16+ messages in thread
From: Alejandro Jimenez @ 2026-03-13 16:17 UTC (permalink / raw)
  To: Sairaj Kodilkar, Vasant Hegde, qemu-devel
  Cc: pbonzini, richard.henderson, eduardo, mst, marcel.apfelbaum,
	clement.mathieu--drif



On 3/13/26 1:20 AM, Sairaj Kodilkar wrote:
> 
> 
> On 3/13/2026 5:46 AM, Alejandro Jimenez wrote:
>>
>> On 3/3/26 4:55 AM, Sairaj Kodilkar wrote:
>>>
>>> On 3/3/2026 3:17 PM, Vasant Hegde wrote:
>>>> On 3/2/2026 5:21 PM, 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 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>
>>>>> Reviewed-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
>>>>> ---
>>>>>    hw/i386/amd_iommu.c  | 47 ++++++++++++++++++++++++++++++++++++++------
>>>>>    hw/i386/amd_iommu.h  | 17 ++++++++++++++++
>>>>>    hw/i386/trace-events |  1 +
>>>>>    3 files changed, 59 insertions(+), 6 deletions(-)
>>>>>
>>>> .../...
>>>>
>>>>>          /* update the flags depending on the control register */
>>>>>        if (s->cmdbuf_enabled) {
>>>>> @@ -1732,6 +1762,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;
>>>>>        }
>>>>>    }
>>>>>    @@ -2382,6 +2415,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);
>>>>> @@ -2452,6 +2486,7 @@ static const VMStateDescription vmstate_xt = {
>>>>>           .minimum_version_id = 1,
>>>>>           .fields = (VMStateField[]) {
>>>>>               VMSTATE_BOOL(xten, AMDVIState),
>>>>> +           VMSTATE_BOOL(intcapxten, AMDVIState),
>>>> Do we need to increase the version no?
>>> No, because we have introduced a separate subsection, the older and newer
>>> qemu are compatible.
>>>
>> I thought that was the case because the changes will still be part of the
>> same "release". I don't believe we guarantee migration compatibility
>> between random/intermediate commits, but...
>>
>> If we are going to follow the guidelines strictly then I think Vasant's
>> observation is correct. The patch changes the layout of the subsection so
>> we are in the same scenario that lead us to include a subsection to begin
>> with.
>>
>> Because the two new values are still part of the same xt support domain, I
>> think it makes sense to keep them in the same subsection and the simplest
>> way is to do:
>>
>>   static const VMStateDescription vmstate_xt = {
>>       .name = "amd-iommu-xt",
>> -    .version_id = 1,
>> +    .version_id = 2,
>>       .minimum_version_id = 1,
>>       .fields = (VMStateField[]) {
>>           VMSTATE_BOOL(xten, AMDVIState),
>> -        VMSTATE_BOOL(intcapxten, AMDVIState),
>> +        VMSTATE_BOOL_V(intcapxten, AMDVIState, 2),
>>           VMSTATE_END_OF_LIST()
>>       }
>>   };
>>
>> That change is on top of your current patch. There seems to be precedent
>> for this based on my search in the git log. If you are ok with this
>> approach let me know and I'll apply it, no need to send a new revision.
> 
> I am not sure if it is really useful here. Because without this patch
> xt-support will not work and migrating from vm which only has first two
> patches to the vm which has all three patches does not make sense to me.

I agree it doesn't make much sense from a practical standpoint, unless we
are bisecting a migration issue, and we don't want to fail between these
two commits.
But again, adding a new field to the subsection does change the payload
that is sent "on the wire", and increasing the version like in my example
above is the minimum change that keeps it all fully correct for migrations
going forward (just like we ensured when adding the subsection in the first
place).

> I think we should treat three patches as a single unit, let me know what
> do you think about this ?
> 
I considered just moving the creation of the entire subsection to the last
patch, but I think it is unnecessary. The approach of incrementing the
version ID above doesn't have any downsides other than little additional
complexity, and it is the "technically correct" way to do it.

I don't want to introduce new subsections, since that has basically the
same effect as the method I proposed, and it makes more sense to keep all
of the XT-related fields in the same subsection (appropriately named
"amd-iommu-xt")

So unless you have any correctness concerns, we should go with my initial
suggestion (actually it was Vasant's). I get the usefulness is limited, but
it is the proper way to do this based on my interpretation of the docs.

Thank you,
Alejandro

> Or we can do something like [1], which introduces new subsections
> for each capbility.
> https://lore.kernel.org/qemu-devel/20180119050005.29392-1-
> sjitindarsingh@gmail.com/
> 
> But this makes sense only when you can use each capability separately.
> 
> Thanks
> Sairaj
>>
>> Thank you,
>> Alejandro
>>
>>
>>> Thanks
>>> Sairaj
> 



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 3/3] amd_iommu: Generate XT interrupts when xt support is enabled
  2026-03-13 16:17           ` Alejandro Jimenez
@ 2026-03-16  7:04             ` Sairaj Kodilkar
  0 siblings, 0 replies; 16+ messages in thread
From: Sairaj Kodilkar @ 2026-03-16  7:04 UTC (permalink / raw)
  To: Alejandro Jimenez, Vasant Hegde, qemu-devel
  Cc: sarunkod, pbonzini, richard.henderson, eduardo, mst,
	marcel.apfelbaum, clement.mathieu--drif

>>>>>> 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 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>
>>>>>> Reviewed-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
>>>>>> ---
>>>>>>     hw/i386/amd_iommu.c  | 47 ++++++++++++++++++++++++++++++++++++++------
>>>>>>     hw/i386/amd_iommu.h  | 17 ++++++++++++++++
>>>>>>     hw/i386/trace-events |  1 +
>>>>>>     3 files changed, 59 insertions(+), 6 deletions(-)
>>>>>>
>>>>> .../...
>>>>>
>>>>>>           /* update the flags depending on the control register */
>>>>>>         if (s->cmdbuf_enabled) {
>>>>>> @@ -1732,6 +1762,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;
>>>>>>         }
>>>>>>     }
>>>>>>     @@ -2382,6 +2415,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);
>>>>>> @@ -2452,6 +2486,7 @@ static const VMStateDescription vmstate_xt = {
>>>>>>            .minimum_version_id = 1,
>>>>>>            .fields = (VMStateField[]) {
>>>>>>                VMSTATE_BOOL(xten, AMDVIState),
>>>>>> +           VMSTATE_BOOL(intcapxten, AMDVIState),
>>>>> Do we need to increase the version no?
>>>> No, because we have introduced a separate subsection, the older and newer
>>>> qemu are compatible.
>>>>
>>> I thought that was the case because the changes will still be part of the
>>> same "release". I don't believe we guarantee migration compatibility
>>> between random/intermediate commits, but...
>>>
>>> If we are going to follow the guidelines strictly then I think Vasant's
>>> observation is correct. The patch changes the layout of the subsection so
>>> we are in the same scenario that lead us to include a subsection to begin
>>> with.
>>>
>>> Because the two new values are still part of the same xt support domain, I
>>> think it makes sense to keep them in the same subsection and the simplest
>>> way is to do:
>>>
>>>    static const VMStateDescription vmstate_xt = {
>>>        .name = "amd-iommu-xt",
>>> -    .version_id = 1,
>>> +    .version_id = 2,
>>>        .minimum_version_id = 1,
>>>        .fields = (VMStateField[]) {
>>>            VMSTATE_BOOL(xten, AMDVIState),
>>> -        VMSTATE_BOOL(intcapxten, AMDVIState),
>>> +        VMSTATE_BOOL_V(intcapxten, AMDVIState, 2),
>>>            VMSTATE_END_OF_LIST()
>>>        }
>>>    };
>>>
>>> That change is on top of your current patch. There seems to be precedent
>>> for this based on my search in the git log. If you are ok with this
>>> approach let me know and I'll apply it, no need to send a new revision.
>> I am not sure if it is really useful here. Because without this patch
>> xt-support will not work and migrating from vm which only has first two
>> patches to the vm which has all three patches does not make sense to me.
> I agree it doesn't make much sense from a practical standpoint, unless we
> are bisecting a migration issue, and we don't want to fail between these
> two commits.
> But again, adding a new field to the subsection does change the payload
> that is sent "on the wire", and increasing the version like in my example
> above is the minimum change that keeps it all fully correct for migrations
> going forward (just like we ensured when adding the subsection in the first
> place).
>
>> I think we should treat three patches as a single unit, let me know what
>> do you think about this ?
>>
> I considered just moving the creation of the entire subsection to the last
> patch, but I think it is unnecessary. The approach of incrementing the
> version ID above doesn't have any downsides other than little additional
> complexity, and it is the "technically correct" way to do it.
>
> I don't want to introduce new subsections, since that has basically the
> same effect as the method I proposed, and it makes more sense to keep all
> of the XT-related fields in the same subsection (appropriately named
> "amd-iommu-xt")
>
> So unless you have any correctness concerns, we should go with my initial
> suggestion (actually it was Vasant's). I get the usefulness is limited, but
> it is the proper way to do this based on my interpretation of the docs.
Hi Alejandro,

I don't have any correctness concerns. I was trying to understand pros and
cons of this method. I am ok with this change.

Thanks
Sairaj




^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 0/3] amd_iommu: Support Generation of IOMMU XT interrupts
  2026-03-02 11:51 [PATCH v3 0/3] amd_iommu: Support Generation of IOMMU XT interrupts Sairaj Kodilkar
                   ` (2 preceding siblings ...)
  2026-03-02 11:51 ` [PATCH v3 3/3] amd_iommu: Generate XT interrupts when xt support is enabled Sairaj Kodilkar
@ 2026-03-17  1:10 ` Alejandro Jimenez
  3 siblings, 0 replies; 16+ messages in thread
From: Alejandro Jimenez @ 2026-03-17  1:10 UTC (permalink / raw)
  To: Sairaj Kodilkar, qemu-devel, mst
  Cc: pbonzini, richard.henderson, eduardo, marcel.apfelbaum,
	vasant.hegde, clement.mathieu--drif

Hi Sairaj, Michael,

On 3/2/26 6:51 AM, 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.
> 

Thank you for your work on this series, and being responsive to all the
feedback.

I reviewed this latest round later than I should have. I had mistakenly
marked the soft-freeze date in my calendar to be later in the cycle, so I
was late getting to review/test this last revision.

A case can be made that this series should be merged even after the change
freeze, since it addresses a potential correctness issue with the interrupt
generation, and the cleanup/refactoring/migration components are supporting
this fix. However, given where we are in the cycle and the fact we have not
seen any crashes/problems directly attributed to the XT support, I think we
can defer to 11.1, and merge once the window opens in late April. No need
to resend the series, it is already in my staging branch.

I'm including Michael on the direct recipients in case he has other
preference. I have the pull request with this series ready if there is
need/opportunity to merge earlier.

Thank you,
Alejandro

> -------------------------------------------------------------------------------
> 
> Changes since v2:
> https://lore.kernel.org/qemu-devel/20260129102814.4488-1-sarunkod@amd.com/
> Patch 1:
>     - Delete  amdvi_mmio_trace_{read,write} and AMDVI_MMIO_REGS_{LOW,HIGH}
>       definitions [AJ]
>     - Move MMIO_REG_TO_STRING definition inside amdvi_mmio_get_name() [CM]
> Patch 2: Improve commit message [AJ]
> Patch 3: Improve commit message and comment [AJ]
> 
> 
> Changes since v1:
> https://lore.kernel.org/qemu-devel/20251118082403.3455-1-sarunkod@amd.com/
> 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 07f97d5da04a
> 
> -------------------------------------------------------------------------------
> 
> 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  | 144 ++++++++++++++++++++++++++-----------------
>  hw/i386/amd_iommu.h  |  25 ++++++--
>  hw/i386/trace-events |   1 +
>  3 files changed, 108 insertions(+), 62 deletions(-)
> 



^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2026-03-17  1:11 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-03-02 11:51 [PATCH v3 0/3] amd_iommu: Support Generation of IOMMU XT interrupts Sairaj Kodilkar
2026-03-02 11:51 ` [PATCH v3 1/3] amd_iommu: Use switch case to determine mmio register name Sairaj Kodilkar
2026-03-12 21:59   ` Alejandro Jimenez
2026-03-13  0:36     ` Alejandro Jimenez
2026-03-13  4:49     ` Sairaj Kodilkar
2026-03-02 11:51 ` [PATCH v3 2/3] amd_iommu: Turn on XT support only when guest has enabled it Sairaj Kodilkar
2026-03-03  9:39   ` Vasant Hegde
2026-03-03 10:20     ` Sairaj Kodilkar
2026-03-02 11:51 ` [PATCH v3 3/3] amd_iommu: Generate XT interrupts when xt support is enabled Sairaj Kodilkar
2026-03-03  9:47   ` Vasant Hegde
2026-03-03  9:55     ` Sairaj Kodilkar
2026-03-13  0:16       ` Alejandro Jimenez
2026-03-13  5:20         ` Sairaj Kodilkar
2026-03-13 16:17           ` Alejandro Jimenez
2026-03-16  7:04             ` Sairaj Kodilkar
2026-03-17  1:10 ` [PATCH v3 0/3] amd_iommu: Support Generation of IOMMU XT interrupts Alejandro Jimenez

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox