[PATCH v3 0/6] hw/i386/amd_iommu: Cleanups and fixes

[PATCH v3 0/6] hw/i386/amd_iommu: Cleanups and fixes

[Sairaj Kodilkar]

This series provides few cleanups and fixes for the amd iommu

Changes since v2:
- Used VMSTATE_UNUSED() to maintain migration compatibility when ats_enabled
  flag is removed [Phil].
- Simplified the amdvi_writew [Phil].
v2: https://lore.kernel.org/qemu-devel/2e8f2b72-8fb5-474f-9844-61f306efeb2b@amd.com/

Changes since v1:
- Dropped top two patches which depend on the Alejandro's changes and rebased
  remaining patches on top of v10.1.0-rc0 [Phil].
- Added a patch to fix amdvi_write*() [Ethon].
- Reset event log head and tail when guest writes to event log base register
  [Ethon].
- Considered "evtlog_intr" flag while generating event log interrupt [Ethon].
- Fixed comment [Ethon].
v1: https://lore.kernel.org/qemu-devel/20250716073145.915-1-sarunkod@amd.com/

Base commit: 9e601684dc24a521bb1d23215a63e5c6e79ea0bb (v10.1.0-rc0)

Sairaj Kodilkar (6):
  hw/i386/amd_iommu: Fix MMIO register write tracing
  hw/i386/amd_iommu: Remove unused and wrongly set ats_enabled field
  hw/i386/amd_iommu: Move IOAPIC memory region initialization to the end
  hw/i386/amd_iommu: Fix amdvi_write*()
  hw/i386/amd_iommu: Support MMIO writes to the status register
  hw/i386/amd_iommu: Fix event log generation

 hw/i386/amd_iommu.c | 102 ++++++++++++++++++++++++++++++++++----------
 hw/i386/amd_iommu.h |   2 +-
 2 files changed, 80 insertions(+), 24 deletions(-)

-- 
2.34.1

[PATCH v3 1/6] hw/i386/amd_iommu: Fix MMIO register write tracing

[Sairaj Kodilkar]

Define separate functions to trace MMIO write accesses instead of using
`trace_amdvi_mmio_read()` for both read and write.

Signed-off-by: Sairaj Kodilkar <sarunkod@amd.com>
Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/i386/amd_iommu.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 5a24c17548d4..7fb0bb68f008 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -592,18 +592,31 @@ static void amdvi_cmdbuf_run(AMDVIState *s)
     }
 }
 
-static void amdvi_mmio_trace(hwaddr addr, unsigned size)
+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;
-        trace_amdvi_mmio_read(amdvi_mmio_high[index], addr, size, addr & ~0x07);
     } else {
         index = index >= AMDVI_MMIO_REGS_LOW ? AMDVI_MMIO_REGS_LOW : index;
-        trace_amdvi_mmio_read(amdvi_mmio_low[index], addr, size, addr & ~0x07);
     }
+
+    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)
@@ -623,7 +636,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(addr, size);
+    amdvi_mmio_trace_read(addr, size);
 
     return val;
 }
@@ -770,7 +783,7 @@ static void amdvi_mmio_write(void *opaque, hwaddr addr, uint64_t val,
         return;
     }
 
-    amdvi_mmio_trace(addr, size);
+    amdvi_mmio_trace_write(addr, size, val);
     switch (addr & ~0x07) {
     case AMDVI_MMIO_CONTROL:
         amdvi_mmio_reg_write(s, size, val, addr);
-- 
2.34.1

[PATCH v3 2/6] hw/i386/amd_iommu: Remove unused and wrongly set ats_enabled field

[Sairaj Kodilkar]

The ats_enabled field is set using HTTUNEN, which is wrong.
Fix this by removing the field as it is never used.

Fixes: d29a09ca68428 ("hw/i386: Introduce AMD IOMMU")
Signed-off-by: Sairaj Kodilkar <sarunkod@amd.com>
Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>
---
 hw/i386/amd_iommu.c | 5 ++---
 hw/i386/amd_iommu.h | 1 -
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 7fb0bb68f008..d4e10d63a606 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -646,7 +646,6 @@ static void amdvi_handle_control_write(AMDVIState *s)
     unsigned long control = amdvi_readq(s, AMDVI_MMIO_CONTROL);
     s->enabled = !!(control & AMDVI_MMIO_CONTROL_AMDVIEN);
 
-    s->ats_enabled = !!(control & AMDVI_MMIO_CONTROL_HTTUNEN);
     s->evtlog_enabled = s->enabled && !!(control &
                         AMDVI_MMIO_CONTROL_EVENTLOGEN);
 
@@ -1555,7 +1554,6 @@ static void amdvi_init(AMDVIState *s)
     s->excl_allow = false;
     s->mmio_enabled = false;
     s->enabled = false;
-    s->ats_enabled = false;
     s->cmdbuf_enabled = false;
 
     /* reset MMIO */
@@ -1626,7 +1624,8 @@ static const VMStateDescription vmstate_amdvi_sysbus_migratable = {
       /* Updated in  amdvi_handle_control_write() */
       VMSTATE_BOOL(enabled, AMDVIState),
       VMSTATE_BOOL(ga_enabled, AMDVIState),
-      VMSTATE_BOOL(ats_enabled, AMDVIState),
+      /* bool ats_enabled is obsolete */
+      VMSTATE_UNUSED(1),
       VMSTATE_BOOL(cmdbuf_enabled, AMDVIState),
       VMSTATE_BOOL(completion_wait_intr, AMDVIState),
       VMSTATE_BOOL(evtlog_enabled, AMDVIState),
diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
index 8b42913ed8da..67078c6f1e22 100644
--- a/hw/i386/amd_iommu.h
+++ b/hw/i386/amd_iommu.h
@@ -322,7 +322,6 @@ struct AMDVIState {
     uint64_t mmio_addr;
 
     bool enabled;                /* IOMMU enabled                */
-    bool ats_enabled;            /* address translation enabled  */
     bool cmdbuf_enabled;         /* command buffer enabled       */
     bool evtlog_enabled;         /* event log enabled            */
     bool excl_enabled;
-- 
2.34.1

[PATCH v3 3/6] hw/i386/amd_iommu: Move IOAPIC memory region initialization to the end

[Sairaj Kodilkar]

Setting up IOAPIC memory region requires mr_sys and mr_ir. Currently
these two memory regions are setup after the initializing the IOAPIC
memory region, which cause `amdvi_host_dma_iommu()` to use unitialized
mr_sys and mr_ir.

Move the IOAPIC memory region initialization to the end in order to use
the mr_sys and mr_ir regions after they are fully initialized.

Fixes: 577c470f4326 ("x86_iommu/amd: Prepare for interrupt remap support")
Signed-off-by: Sairaj Kodilkar <sarunkod@amd.com>
Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>
---
 hw/i386/amd_iommu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index d4e10d63a606..1ffd1375570c 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -1698,9 +1698,6 @@ static void amdvi_sysbus_realize(DeviceState *dev, Error **errp)
     s->iotlb = g_hash_table_new_full(amdvi_uint64_hash,
                                      amdvi_uint64_equal, g_free, g_free);
 
-    /* Pseudo address space under root PCI bus. */
-    x86ms->ioapic_as = amdvi_host_dma_iommu(bus, s, AMDVI_IOAPIC_SB_DEVID);
-
     /* set up MMIO */
     memory_region_init_io(&s->mr_mmio, OBJECT(s), &mmio_mem_ops, s,
                           "amdvi-mmio", AMDVI_MMIO_SIZE);
@@ -1723,6 +1720,9 @@ static void amdvi_sysbus_realize(DeviceState *dev, Error **errp)
     memory_region_add_subregion_overlap(&s->mr_sys, AMDVI_INT_ADDR_FIRST,
                                         &s->mr_ir, 1);
 
+    /* Pseudo address space under root PCI bus. */
+    x86ms->ioapic_as = amdvi_host_dma_iommu(bus, s, AMDVI_IOAPIC_SB_DEVID);
+
     if (kvm_enabled() && x86ms->apic_id_limit > 255 && !s->xtsup) {
         error_report("AMD IOMMU with x2APIC configuration requires xtsup=on");
         exit(EXIT_FAILURE);
-- 
2.34.1

[PATCH v3 4/6] hw/i386/amd_iommu: Fix amdvi_write*()

[Sairaj Kodilkar]

amdvi_write*() function do not preserve the older values of W1C bits in
the MMIO register. This results in all W1C bits set to 0, when guest
tries to reset a single bit by writing 1 to it. Fix this by preserving
W1C bits in the old value of the MMIO register.

Fixes: d29a09ca68428 ("hw/i386: Introduce AMD IOMMU")
Suggested-by: Ethan MILON <ethan.milon@eviden.com>
Signed-off-by: Sairaj Kodilkar <sarunkod@amd.com>
---
 hw/i386/amd_iommu.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 1ffd1375570c..ea0cb0e21fae 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -123,8 +123,13 @@ static void amdvi_writew(AMDVIState *s, hwaddr addr, uint16_t val)
     uint16_t romask = lduw_le_p(&s->romask[addr]);
     uint16_t w1cmask = lduw_le_p(&s->w1cmask[addr]);
     uint16_t oldval = lduw_le_p(&s->mmior[addr]);
+
+    uint16_t oldval_preserved = oldval & (romask | w1cmask);
+    uint16_t newval_write = val & ~romask;
+    uint16_t newval_w1c_set = val & w1cmask;
+
     stw_le_p(&s->mmior[addr],
-            ((oldval & romask) | (val & ~romask)) & ~(val & w1cmask));
+             (oldval_preserved | newval_write) & ~newval_w1c_set);
 }
 
 static void amdvi_writel(AMDVIState *s, hwaddr addr, uint32_t val)
@@ -132,8 +137,13 @@ static void amdvi_writel(AMDVIState *s, hwaddr addr, uint32_t val)
     uint32_t romask = ldl_le_p(&s->romask[addr]);
     uint32_t w1cmask = ldl_le_p(&s->w1cmask[addr]);
     uint32_t oldval = ldl_le_p(&s->mmior[addr]);
+
+    uint32_t oldval_preserved = oldval & (romask | w1cmask);
+    uint32_t newval_write = val & ~romask;
+    uint32_t newval_w1c_set = val & w1cmask;
+
     stl_le_p(&s->mmior[addr],
-            ((oldval & romask) | (val & ~romask)) & ~(val & w1cmask));
+             (oldval_preserved | newval_write) & ~newval_w1c_set);
 }
 
 static void amdvi_writeq(AMDVIState *s, hwaddr addr, uint64_t val)
@@ -141,8 +151,13 @@ static void amdvi_writeq(AMDVIState *s, hwaddr addr, uint64_t val)
     uint64_t romask = ldq_le_p(&s->romask[addr]);
     uint64_t w1cmask = ldq_le_p(&s->w1cmask[addr]);
     uint64_t oldval = ldq_le_p(&s->mmior[addr]);
+
+    uint64_t oldval_preserved = oldval & (romask | w1cmask);
+    uint64_t newval_write = val & ~romask;
+    uint64_t newval_w1c_set = val & w1cmask;
+
     stq_le_p(&s->mmior[addr],
-            ((oldval & romask) | (val & ~romask)) & ~(val & w1cmask));
+             (oldval_preserved | newval_write) & ~newval_w1c_set);
 }
 
 /* OR a 64-bit register with a 64-bit value */
-- 
2.34.1

[PATCH v3 5/6] hw/i386/amd_iommu: Support MMIO writes to the status register

[Sairaj Kodilkar]

Support the writes to the status register so that guest can reset the
EventOverflow, EventLogInt, ComWaitIntr, etc bits after servicing the
respective interrupt.

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

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index ea0cb0e21fae..681a46c3ceb8 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -862,6 +862,9 @@ static void amdvi_mmio_write(void *opaque, hwaddr addr, uint64_t val,
         amdvi_mmio_reg_write(s, size, val, addr);
         amdvi_handle_pprtail_write(s);
         break;
+    case AMDVI_MMIO_STATUS:
+        amdvi_mmio_reg_write(s, size, val, addr);
+        break;
     }
 }
 
-- 
2.34.1

[PATCH v3 6/6] hw/i386/amd_iommu: Fix event log generation

[Sairaj Kodilkar]

Current event logging code is broken, because of following issues

1. The code uses '|' instead of '&' to test the bit field, which causes
   vIOMMU to generate overflow interrupt for every log entry.
2. Code does not update the eventlog tail MMIO register after adding an
   entry to the buffer, because of which guest cannot process new
   entries (as head == tail means buffer is empty).
3. Compares eventlog tail (which is byte offset in the buffer) to
   eventlog length (which is number of maximum entries in the buffer).
   This causes vIOMMU to generate only fix number of event logs, after
   which it keeps on generating overflow interrupts, without
   actually resetting the log buffer.
4. Updates ComWaitInt instead of EventLogInt bitfield in Status
   register. Guest checks this field to see if there are new event log
   entries in the buffer.
5. Does not reset event log head and tail pointers when guest writes to
   eventlog base register.

Fix above issues, so that guest can process event log entries.

Fixes: d29a09ca68428 ("hw/i386: Introduce AMD IOMMU")
Signed-off-by: Sairaj Kodilkar <sarunkod@amd.com>
Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>
---
 hw/i386/amd_iommu.c | 44 +++++++++++++++++++++++++++++++++++---------
 hw/i386/amd_iommu.h |  1 +
 2 files changed, 36 insertions(+), 9 deletions(-)

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 681a46c3ceb8..bcc21c3d7930 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -160,10 +160,10 @@ static void amdvi_writeq(AMDVIState *s, hwaddr addr, uint64_t val)
              (oldval_preserved | newval_write) & ~newval_w1c_set);
 }
 
-/* OR a 64-bit register with a 64-bit value */
+/* AND a 64-bit register with a 64-bit value */
 static bool amdvi_test_mask(AMDVIState *s, hwaddr addr, uint64_t val)
 {
-    return amdvi_readq(s, addr) | val;
+    return amdvi_readq(s, addr) & val;
 }
 
 /* OR a 64-bit register with a 64-bit value storing result in the register */
@@ -192,19 +192,31 @@ static void amdvi_generate_msi_interrupt(AMDVIState *s)
     }
 }
 
+static uint32_t get_next_eventlog_entry(AMDVIState *s)
+{
+    uint32_t evtlog_size = s->evtlog_len * AMDVI_EVENT_LEN;
+    return (s->evtlog_tail + AMDVI_EVENT_LEN) % evtlog_size;
+}
+
 static void amdvi_log_event(AMDVIState *s, uint64_t *evt)
 {
+    uint32_t evtlog_tail_next;
+
     /* event logging not enabled */
     if (!s->evtlog_enabled || amdvi_test_mask(s, AMDVI_MMIO_STATUS,
         AMDVI_MMIO_STATUS_EVT_OVF)) {
         return;
     }
 
+    evtlog_tail_next = get_next_eventlog_entry(s);
+
     /* event log buffer full */
-    if (s->evtlog_tail >= s->evtlog_len) {
-        amdvi_assign_orq(s, AMDVI_MMIO_STATUS, AMDVI_MMIO_STATUS_EVT_OVF);
-        /* generate interrupt */
-        amdvi_generate_msi_interrupt(s);
+    if (evtlog_tail_next == s->evtlog_head) {
+        /* generate overflow interrupt */
+        if (s->evtlog_intr) {
+            amdvi_assign_orq(s, AMDVI_MMIO_STATUS, AMDVI_MMIO_STATUS_EVT_OVF);
+            amdvi_generate_msi_interrupt(s);
+        }
         return;
     }
 
@@ -213,9 +225,13 @@ static void amdvi_log_event(AMDVIState *s, uint64_t *evt)
         trace_amdvi_evntlog_fail(s->evtlog, s->evtlog_tail);
     }
 
-    s->evtlog_tail += AMDVI_EVENT_LEN;
-    amdvi_assign_orq(s, AMDVI_MMIO_STATUS, AMDVI_MMIO_STATUS_COMP_INT);
-    amdvi_generate_msi_interrupt(s);
+    s->evtlog_tail = evtlog_tail_next;
+    amdvi_writeq_raw(s, AMDVI_MMIO_EVENT_TAIL, s->evtlog_tail);
+
+    if (s->evtlog_intr) {
+        amdvi_assign_orq(s, AMDVI_MMIO_STATUS, AMDVI_MMIO_STATUS_EVENT_INT);
+        amdvi_generate_msi_interrupt(s);
+    }
 }
 
 static void amdvi_setevent_bits(uint64_t *buffer, uint64_t value, int start,
@@ -731,9 +747,19 @@ static inline void amdvi_handle_excllim_write(AMDVIState *s)
 static inline void amdvi_handle_evtbase_write(AMDVIState *s)
 {
     uint64_t val = amdvi_readq(s, AMDVI_MMIO_EVENT_BASE);
+
+    if (amdvi_readq(s, AMDVI_MMIO_STATUS) & AMDVI_MMIO_STATUS_EVENT_INT)
+        /* Do not reset if eventlog interrupt bit is set*/
+        return;
+
     s->evtlog = val & AMDVI_MMIO_EVTLOG_BASE_MASK;
     s->evtlog_len = 1UL << (amdvi_readq(s, AMDVI_MMIO_EVTLOG_SIZE_BYTE)
                     & AMDVI_MMIO_EVTLOG_SIZE_MASK);
+
+    /* clear tail and head pointer to 0 when event base is updated */
+    s->evtlog_tail = s->evtlog_head = 0;
+    amdvi_writeq_raw(s, AMDVI_MMIO_EVENT_HEAD, s->evtlog_head);
+    amdvi_writeq_raw(s, AMDVI_MMIO_EVENT_TAIL, s->evtlog_tail);
 }
 
 static inline void amdvi_handle_evttail_write(AMDVIState *s)
diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
index 67078c6f1e22..2476296c4902 100644
--- a/hw/i386/amd_iommu.h
+++ b/hw/i386/amd_iommu.h
@@ -111,6 +111,7 @@
 #define AMDVI_MMIO_STATUS_CMDBUF_RUN  (1 << 4)
 #define AMDVI_MMIO_STATUS_EVT_RUN     (1 << 3)
 #define AMDVI_MMIO_STATUS_COMP_INT    (1 << 2)
+#define AMDVI_MMIO_STATUS_EVENT_INT   (1 << 1)
 #define AMDVI_MMIO_STATUS_EVT_OVF     (1 << 0)
 
 #define AMDVI_CMDBUF_ID_BYTE              0x07
-- 
2.34.1

Re: [PATCH v3 2/6] hw/i386/amd_iommu: Remove unused and wrongly set ats_enabled field

[Philippe Mathieu-Daudé]

On 1/8/25 08:05, Sairaj Kodilkar wrote:
> The ats_enabled field is set using HTTUNEN, which is wrong.
> Fix this by removing the field as it is never used.
> 
> Fixes: d29a09ca68428 ("hw/i386: Introduce AMD IOMMU")
> Signed-off-by: Sairaj Kodilkar <sarunkod@amd.com>
> Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>   hw/i386/amd_iommu.c | 5 ++---
>   hw/i386/amd_iommu.h | 1 -
>   2 files changed, 2 insertions(+), 4 deletions(-)


> @@ -1626,7 +1624,8 @@ static const VMStateDescription vmstate_amdvi_sysbus_migratable = {
>         /* Updated in  amdvi_handle_control_write() */
>         VMSTATE_BOOL(enabled, AMDVIState),
>         VMSTATE_BOOL(ga_enabled, AMDVIState),
> -      VMSTATE_BOOL(ats_enabled, AMDVIState),
> +      /* bool ats_enabled is obsolete */
> +      VMSTATE_UNUSED(1),

          VMSTATE_UNUSED(1), /* was ats_enabled */

Otherwise,

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v3 0/6] hw/i386/amd_iommu: Cleanups and fixes

[Michael Tokarev]

On 01.08.2025 09:05, Sairaj Kodilkar wrote:
> This series provides few cleanups and fixes for the amd iommu
> 
> Changes since v2:
> - Used VMSTATE_UNUSED() to maintain migration compatibility when ats_enabled
>    flag is removed [Phil].
> - Simplified the amdvi_writew [Phil].
> v2: https://lore.kernel.org/qemu-devel/2e8f2b72-8fb5-474f-9844-61f306efeb2b@amd.com/
> 
> Changes since v1:
> - Dropped top two patches which depend on the Alejandro's changes and rebased
>    remaining patches on top of v10.1.0-rc0 [Phil].
> - Added a patch to fix amdvi_write*() [Ethon].
> - Reset event log head and tail when guest writes to event log base register
>    [Ethon].
> - Considered "evtlog_intr" flag while generating event log interrupt [Ethon].
> - Fixed comment [Ethon].
> v1: https://lore.kernel.org/qemu-devel/20250716073145.915-1-sarunkod@amd.com/
> 
> Base commit: 9e601684dc24a521bb1d23215a63e5c6e79ea0bb (v10.1.0-rc0)
> 
> Sairaj Kodilkar (6):
>    hw/i386/amd_iommu: Fix MMIO register write tracing
>    hw/i386/amd_iommu: Remove unused and wrongly set ats_enabled field
>    hw/i386/amd_iommu: Move IOAPIC memory region initialization to the end
>    hw/i386/amd_iommu: Fix amdvi_write*()
>    hw/i386/amd_iommu: Support MMIO writes to the status register
>    hw/i386/amd_iommu: Fix event log generation
> 
>   hw/i386/amd_iommu.c | 102 ++++++++++++++++++++++++++++++++++----------
>   hw/i386/amd_iommu.h |   2 +-
>   2 files changed, 80 insertions(+), 24 deletions(-)

Hi!

Is there anything there worth to pick up for qemu 10.0.x stable series?
(the "Move IOAPIC memory init" does not apply to 10.0 already).

Thanks,

/mjt

Re: [PATCH v3 0/6] hw/i386/amd_iommu: Cleanups and fixes

[Sairaj Kodilkar]

On 8/2/2025 11:11 AM, Michael Tokarev wrote:
> On 01.08.2025 09:05, Sairaj Kodilkar wrote:
>> This series provides few cleanups and fixes for the amd iommu
>>
>> Changes since v2:
>> - Used VMSTATE_UNUSED() to maintain migration compatibility when 
>> ats_enabled
>>    flag is removed [Phil].
>> - Simplified the amdvi_writew [Phil].
>> v2: https://lore.kernel.org/qemu- 
>> devel/2e8f2b72-8fb5-474f-9844-61f306efeb2b@amd.com/
>>
>> Changes since v1:
>> - Dropped top two patches which depend on the Alejandro's changes and 
>> rebased
>>    remaining patches on top of v10.1.0-rc0 [Phil].
>> - Added a patch to fix amdvi_write*() [Ethon].
>> - Reset event log head and tail when guest writes to event log base 
>> register
>>    [Ethon].
>> - Considered "evtlog_intr" flag while generating event log interrupt 
>> [Ethon].
>> - Fixed comment [Ethon].
>> v1: https://lore.kernel.org/qemu-devel/20250716073145.915-1- 
>> sarunkod@amd.com/
>>
>> Base commit: 9e601684dc24a521bb1d23215a63e5c6e79ea0bb (v10.1.0-rc0)
>>
>> Sairaj Kodilkar (6):
>>    hw/i386/amd_iommu: Fix MMIO register write tracing
>>    hw/i386/amd_iommu: Remove unused and wrongly set ats_enabled field
>>    hw/i386/amd_iommu: Move IOAPIC memory region initialization to the end
>>    hw/i386/amd_iommu: Fix amdvi_write*()
>>    hw/i386/amd_iommu: Support MMIO writes to the status register
>>    hw/i386/amd_iommu: Fix event log generation
>>
>>   hw/i386/amd_iommu.c | 102 ++++++++++++++++++++++++++++++++++----------
>>   hw/i386/amd_iommu.h |   2 +-
>>   2 files changed, 80 insertions(+), 24 deletions(-)
> 
> Hi!
> 
> Is there anything there worth to pick up for qemu 10.0.x stable series?
> (the "Move IOAPIC memory init" does not apply to 10.0 already).

Hi MJT,

I will backport the patch manually and send it to qemu stable mailing list

Thanks
Sairaj

Re: [PATCH v3 0/6] hw/i386/amd_iommu: Cleanups and fixes

[Michael Tokarev]

On 04.08.2025 14:06, Sairaj Kodilkar wrote:
...
>>> Sairaj Kodilkar (6):
>>>    hw/i386/amd_iommu: Fix MMIO register write tracing
>>>    hw/i386/amd_iommu: Remove unused and wrongly set ats_enabled field
>>>    hw/i386/amd_iommu: Move IOAPIC memory region initialization to the 
>>> end
>>>    hw/i386/amd_iommu: Fix amdvi_write*()
>>>    hw/i386/amd_iommu: Support MMIO writes to the status register
>>>    hw/i386/amd_iommu: Fix event log generation
>>>
>>>   hw/i386/amd_iommu.c | 102 ++++++++++++++++++++++++++++++++++----------
>>>   hw/i386/amd_iommu.h |   2 +-
>>>   2 files changed, 80 insertions(+), 24 deletions(-)
>>
>> Hi!
>>
>> Is there anything there worth to pick up for qemu 10.0.x stable series?
>> (the "Move IOAPIC memory init" does not apply to 10.0 already).
> 
> Hi MJT,
> 
> I will backport the patch manually and send it to qemu stable mailing list

Hi!

There's no need to back-port it, -- there's just minor context fix
required, here:

+++ b/hw/i386/amd_iommu.c
@@ -1693,19 +1693,16 @@ static void amdvi_sysbus_realize(DeviceState 
*dev, Error **errp)
...

      s->iotlb = g_hash_table_new_full(amdvi_uint64_hash,
                                       amdvi_uint64_equal, g_free, g_free);

-    /* Pseudo address space under root PCI bus. */
-    x86ms->ioapic_as = amdvi_host_dma_iommu(bus, s, AMDVI_IOAPIC_SB_DEVID);
-
      /* set up MMIO */
      memory_region_init_io(&s->mr_mmio, OBJECT(s), &mmio_mem_ops, s,
                            "amdvi-mmio", AMDVI_MMIO_SIZE);

the "s->iotlb = g_hash_table_new_full" part were added by commit
f864a3235ea1d1 "hw/i386/amd_iommu: Isolate AMDVI-PCI from amd-iommu
device to allow full control over the PCI device creation".  It should
be okay to just remove the 3 marked lines from here (to be moved to
the right place).

My question was not about back-porting this commit, but more about
the set of commits which needs to be picked up for the stable series.

I picked up this commit.  Please let me know if there are other changes
needed to be picked up.

Thanks,

/mjt

Re: [PATCH v3 0/6] hw/i386/amd_iommu: Cleanups and fixes

[Sairaj Kodilkar]

On 8/6/2025 12:28 PM, Michael Tokarev wrote:
> On 04.08.2025 14:06, Sairaj Kodilkar wrote:
> ...
>>>> Sairaj Kodilkar (6):
>>>>    hw/i386/amd_iommu: Fix MMIO register write tracing
>>>>    hw/i386/amd_iommu: Remove unused and wrongly set ats_enabled field
>>>>    hw/i386/amd_iommu: Move IOAPIC memory region initialization to 
>>>> the end
>>>>    hw/i386/amd_iommu: Fix amdvi_write*()
>>>>    hw/i386/amd_iommu: Support MMIO writes to the status register
>>>>    hw/i386/amd_iommu: Fix event log generation
>>>>
>>>>   hw/i386/amd_iommu.c | 102 +++++++++++++++++++++++++++++++++ 
>>>> +----------
>>>>   hw/i386/amd_iommu.h |   2 +-
>>>>   2 files changed, 80 insertions(+), 24 deletions(-)
>>>
>>> Hi!
>>>
>>> Is there anything there worth to pick up for qemu 10.0.x stable series?
>>> (the "Move IOAPIC memory init" does not apply to 10.0 already).
>>
>> Hi MJT,
>>
>> I will backport the patch manually and send it to qemu stable mailing 
>> list
> 
> Hi!
> 
> There's no need to back-port it, -- there's just minor context fix
> required, here:
> 
> +++ b/hw/i386/amd_iommu.c
> @@ -1693,19 +1693,16 @@ static void amdvi_sysbus_realize(DeviceState 
> *dev, Error **errp)
> ...
> 
>       s->iotlb = g_hash_table_new_full(amdvi_uint64_hash,
>                                        amdvi_uint64_equal, g_free, g_free);
> 
> -    /* Pseudo address space under root PCI bus. */
> -    x86ms->ioapic_as = amdvi_host_dma_iommu(bus, s, 
> AMDVI_IOAPIC_SB_DEVID);
> -
>       /* set up MMIO */
>       memory_region_init_io(&s->mr_mmio, OBJECT(s), &mmio_mem_ops, s,
>                             "amdvi-mmio", AMDVI_MMIO_SIZE);
> 
> the "s->iotlb = g_hash_table_new_full" part were added by commit
> f864a3235ea1d1 "hw/i386/amd_iommu: Isolate AMDVI-PCI from amd-iommu
> device to allow full control over the PCI device creation".  It should
> be okay to just remove the 3 marked lines from here (to be moved to
> the right place).
> 
> My question was not about back-porting this commit, but more about
> the set of commits which needs to be picked up for the stable series.
> 
> I picked up this commit.  Please let me know if there are other changes
> needed to be picked up.
> 

Hi MJT

I think this one is sufficient.

Thanks
Sairaj

Re: [PATCH v3 1/6] hw/i386/amd_iommu: Fix MMIO register write tracing

[vsntk18]

>-static void amdvi_mmio_trace(hwaddr addr, unsigned size)
>+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;
>-        trace_amdvi_mmio_read(amdvi_mmio_high[index], addr, size, addr & ~0x07);
>     } else {
>         index = index >= AMDVI_MMIO_REGS_LOW ? AMDVI_MMIO_REGS_LOW : index;
>-        trace_amdvi_mmio_read(amdvi_mmio_low[index], addr, size, addr & ~0x07);
>     }
>+
>+    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,

Shouldn't you be picking between amdvi_mmio_low and amdvi_mmio_high in the
above 2 fuctions depending on the addr value?

Re: [PATCH v3 1/6] hw/i386/amd_iommu: Fix MMIO register write tracing

[Sairaj Kodilkar]

On 9/30/2025 3:06 PM, vsntk18@gmail.com wrote:
>> -static void amdvi_mmio_trace(hwaddr addr, unsigned size)
>> +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;
>> -        trace_amdvi_mmio_read(amdvi_mmio_high[index], addr, size, addr & ~0x07);
>>      } else {
>>          index = index >= AMDVI_MMIO_REGS_LOW ? AMDVI_MMIO_REGS_LOW : index;
>> -        trace_amdvi_mmio_read(amdvi_mmio_low[index], addr, size, addr & ~0x07);
>>      }
>> +
>> +    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,
> Shouldn't you be picking between amdvi_mmio_low and amdvi_mmio_high in the
> above 2 fuctions depending on the addr value?
Yep I realized that after the patches were merged. I have included the 
fix in the next cleanup series

Thanks
Sairaj