When processing commands, the internal cmdbuf_head state is correctly
reset to 0 on wrap, but the MMIO CmdHeadPtr register was written before
the wrap check, leaving the guest-visible register stuck at the buffer
length instead of 0 after a full command buffer pass.

Move the register write after the wrap check so the MMIO value stays in
sync with the internal state.

The Linux kernel AMD IOMMU driver is not affected by this bug because
it uses COMPLETION_WAIT with a memory store doorbell to detect command
progress.

Add a test to lock this down.

Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3338

Signed-off-by: Costas Argyris <costas.argyris@amd.com>
---
 MAINTAINERS                  |  1 +
 hw/i386/amd_iommu.c          |  2 +-
 tests/qtest/amd-iommu-test.c | 71 ++++++++++++++++++++++++++++++++++++
 tests/qtest/meson.build      |  1 +
 4 files changed, 74 insertions(+), 1 deletion(-)
 create mode 100644 tests/qtest/amd-iommu-test.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 80d28e618d..750d415389 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4052,6 +4052,7 @@ M: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
 R: Sairaj Kodilkar <sarunkod@amd.com>
 S: Supported
 F: hw/i386/amd_iommu*
+F: tests/qtest/amd-iommu-test.c
 
 OpenSBI Firmware
 L: qemu-riscv@nongnu.org
diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 789e09d6f2..ce18cb453f 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -1475,12 +1475,12 @@ static void amdvi_cmdbuf_run(AMDVIState *s)
         trace_amdvi_command_exec(s->cmdbuf_head, s->cmdbuf_tail, s->cmdbuf);
         amdvi_cmdbuf_exec(s);
         s->cmdbuf_head += AMDVI_COMMAND_SIZE;
-        amdvi_writeq_raw(s, AMDVI_MMIO_COMMAND_HEAD, s->cmdbuf_head);
 
         /* wrap head pointer */
         if (s->cmdbuf_head >= s->cmdbuf_len * AMDVI_COMMAND_SIZE) {
             s->cmdbuf_head = 0;
         }
+        amdvi_writeq_raw(s, AMDVI_MMIO_COMMAND_HEAD, s->cmdbuf_head);
     }
 }
 
diff --git a/tests/qtest/amd-iommu-test.c b/tests/qtest/amd-iommu-test.c
new file mode 100644
index 0000000000..0438ee9586
--- /dev/null
+++ b/tests/qtest/amd-iommu-test.c
@@ -0,0 +1,71 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "libqtest.h"
+#include "hw/i386/amd_iommu.h"
+
+#define CMDBUF_ADDR       0x200000
+#define CMDBUF_ENTRIES    256
+#define CMDBUF_LEN_FIELD  8
+
+static inline uint64_t amdvi_reg_readq(QTestState *s, uint64_t offset)
+{
+    return qtest_readq(s, AMDVI_BASE_ADDR + offset);
+}
+
+static inline void amdvi_reg_writeq(QTestState *s, uint64_t offset,
+                                    uint64_t val)
+{
+    qtest_writeq(s, AMDVI_BASE_ADDR + offset, val);
+}
+
+static void test_cmdbuf_head_wrap(void)
+{
+    QTestState *s;
+    uint64_t head;
+    int i;
+    /* 16 bytes per command */
+    struct {
+        uint64_t qw0;
+        uint64_t qw1;
+    } cmdbuf[CMDBUF_ENTRIES];
+
+    s = qtest_init("-M q35 -device amd-iommu");
+
+    /* write 256 COMPLETION_WAIT (no-op) commands to guest RAM */
+    for (i = 0; i < CMDBUF_ENTRIES; i++) {
+        cmdbuf[i].qw0 = (uint64_t)AMDVI_CMD_COMPLETION_WAIT << 60;
+        cmdbuf[i].qw1 = 0;
+    }
+    qtest_memwrite(s, CMDBUF_ADDR, cmdbuf, sizeof(cmdbuf));
+
+    /* point the IOMMU at the command buffer and set its length */
+    amdvi_reg_writeq(s, AMDVI_MMIO_COMMAND_BASE,
+                     CMDBUF_ADDR | ((uint64_t)CMDBUF_LEN_FIELD << 56));
+
+    /* enable the IOMMU and its command buffer processor */
+    amdvi_reg_writeq(s, AMDVI_MMIO_CONTROL,
+                     AMDVI_MMIO_CONTROL_AMDVIEN | AMDVI_MMIO_CONTROL_CMDBUFLEN);
+
+    /* advance tail to the last entry, consuming entries 0..254 */
+    amdvi_reg_writeq(s, AMDVI_MMIO_COMMAND_TAIL,
+                     (CMDBUF_ENTRIES - 1) * AMDVI_COMMAND_SIZE);
+
+    /* wrap tail to 0, consuming entry 255 and completing the buffer */
+    amdvi_reg_writeq(s, AMDVI_MMIO_COMMAND_TAIL, 0);
+
+    /* after consuming all 256 entries the IOMMU must wrap CmdHeadPtr to 0 */
+    head = amdvi_reg_readq(s, AMDVI_MMIO_COMMAND_HEAD);
+    g_assert((head & AMDVI_MMIO_CMDBUF_HEAD_MASK) == 0);
+
+    qtest_quit(s);
+}
+
+int main(int argc, char **argv)
+{
+    g_test_init(&argc, &argv, NULL);
+    qtest_add_func("/q35/amd-iommu/cmdbuf-head-wrap", test_cmdbuf_head_wrap);
+    return g_test_run();
+}
diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index 728dde54b3..67eea5c71a 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -95,6 +95,7 @@ qtests_i386 = \
   (config_all_devices.has_key('CONFIG_SB16') ? ['fuzz-sb16-test'] : []) +                   \
   (config_all_devices.has_key('CONFIG_SDHCI_PCI') ? ['fuzz-sdcard-test'] : []) +            \
   (config_all_devices.has_key('CONFIG_ESP_PCI') ? ['am53c974-test'] : []) +                 \
+  (config_all_devices.has_key('CONFIG_AMD_IOMMU') ? ['amd-iommu-test'] : []) +             \
   (config_all_devices.has_key('CONFIG_VTD') ? ['intel-iommu-test'] : []) +                 \
   (config_all_devices.has_key('CONFIG_VTD') and
    config_all_devices.has_key('CONFIG_IOMMU_TESTDEV') ? ['iommu-intel-test'] : []) +      \
-- 
2.43.0

Hi Costas,

Thank you for working on this issue and adding a test!

On 5/17/26 3:23 PM, Costas Argyris wrote:
> When processing commands, the internal cmdbuf_head state is correctly
> reset to 0 on wrap, but the MMIO CmdHeadPtr register was written before
> the wrap check, leaving the guest-visible register stuck at the buffer
> length instead of 0 after a full command buffer pass.
> 
> Move the register write after the wrap check so the MMIO value stays in
> sync with the internal state.
> 
> The Linux kernel AMD IOMMU driver is not affected by this bug because
> it uses COMPLETION_WAIT with a memory store doorbell to detect command
> progress.
> 
> Add a test to lock this down.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3338
> 

This catches the same CmdHeadPtr wraparound issue that I sent a few days
earlier and Sairaj reviewed:

https://lore.kernel.org/qemu-devel/20260512150044.334867-1-alejandro.j.jimenez@oracle.com/

although I wasn't aware there was a gitlab issue created for it, it was
just something I caught when reviewing a similar patch from Paolo.

So I'd propose that we use this review to commit the test changes you
authored i.e. something like:

tests/qtest: Add amd-iommu command buffer head wrap test

Add a qtest for AMD IOMMU command buffer head pointer wraparound.
The test programs a command buffer, fills it with COMPLETION_WAIT commands,
advances the tail to consume all but the final entry, then wraps the tail
to zero to force the final command to advance CmdHeadPtr past the end of
the buffer. The guest-visible CmdHeadPtr register must then wrap back to zero.

This covers the case fixed by an earlier CmdHeadPtr wraparound patch.

The Linux kernel AMD IOMMU driver is not affected by this bug because it
uses COMPLETION_WAIT with a memory store doorbell to detect command progress.

Signed-off-by: Costas Argyris <costas.argyris@amd.com>

let me know if you agree with the above approach.

Now a couple of comments on the test itself...

> Signed-off-by: Costas Argyris <costas.argyris@amd.com>
> ---
>  MAINTAINERS                  |  1 +
>  hw/i386/amd_iommu.c          |  2 +-
>  tests/qtest/amd-iommu-test.c | 71 ++++++++++++++++++++++++++++++++++++
>  tests/qtest/meson.build      |  1 +
>  4 files changed, 74 insertions(+), 1 deletion(-)
>  create mode 100644 tests/qtest/amd-iommu-test.c
> 

[...]

> +
> +#define CMDBUF_ADDR       0x200000
> +#define CMDBUF_ENTRIES    256
> +#define CMDBUF_LEN_FIELD  8
> +

To reduce magic values and potential errors, we can define CMDBUF_ENTRIES
in terms of CMDBUF_LEN_FIELD i.e. ComLen from MMIO Offset 0008h Command
Buffer Base Address Register in the AMD-Vi spec.

#define CMDBUF_LEN_FIELD  8
#define CMDBUF_ENTRIES    (1U << CMDBUF_LEN_FIELD)

We are choosing to test with the minimum valid value of CMDBUF_LEN_FIELD=8,
but there are other valid values [8..15], and this keeps the test correct
for any of them.

> +static inline uint64_t amdvi_reg_readq(QTestState *s, uint64_t offset)
> +{
> +    return qtest_readq(s, AMDVI_BASE_ADDR + offset);
> +}
> +
> +static inline void amdvi_reg_writeq(QTestState *s, uint64_t offset,
> +                                    uint64_t val)
> +{
> +    qtest_writeq(s, AMDVI_BASE_ADDR + offset, val);
> +}
> +
> +static void test_cmdbuf_head_wrap(void)
> +{
> +    QTestState *s;
> +    uint64_t head;
> +    int i;
> +    /* 16 bytes per command */
> +    struct {
> +        uint64_t qw0;
> +        uint64_t qw1;
> +    } cmdbuf[CMDBUF_ENTRIES];
> +

The line below adds a dependency on Q35, but that is not necessarily always
available with AMD_IOMMU. So we should add a safety check like VT-d does on
their test i.e.

+    if (!qtest_has_machine("q35")) {
+        g_test_skip("q35 machine not available");
+        return;
+    }

Thank you,
Alejandro

> +    s = qtest_init("-M q35 -device amd-iommu");
> +
> +    /* write 256 COMPLETION_WAIT (no-op) commands to guest RAM */
> +    for (i = 0; i < CMDBUF_ENTRIES; i++) {
> +        cmdbuf[i].qw0 = (uint64_t)AMDVI_CMD_COMPLETION_WAIT << 60;
> +        cmdbuf[i].qw1 = 0;
> +    }
> +    qtest_memwrite(s, CMDBUF_ADDR, cmdbuf, sizeof(cmdbuf));
> +
> +    /* point the IOMMU at the command buffer and set its length */
> +    amdvi_reg_writeq(s, AMDVI_MMIO_COMMAND_BASE,
> +                     CMDBUF_ADDR | ((uint64_t)CMDBUF_LEN_FIELD << 56));
> +
> +    /* enable the IOMMU and its command buffer processor */
> +    amdvi_reg_writeq(s, AMDVI_MMIO_CONTROL,
> +                     AMDVI_MMIO_CONTROL_AMDVIEN | AMDVI_MMIO_CONTROL_CMDBUFLEN);
> +
> +    /* advance tail to the last entry, consuming entries 0..254 */
> +    amdvi_reg_writeq(s, AMDVI_MMIO_COMMAND_TAIL,
> +                     (CMDBUF_ENTRIES - 1) * AMDVI_COMMAND_SIZE);
> +
> +    /* wrap tail to 0, consuming entry 255 and completing the buffer */
> +    amdvi_reg_writeq(s, AMDVI_MMIO_COMMAND_TAIL, 0);
> +
> +    /* after consuming all 256 entries the IOMMU must wrap CmdHeadPtr to 0 */
> +    head = amdvi_reg_readq(s, AMDVI_MMIO_COMMAND_HEAD);
> +    g_assert((head & AMDVI_MMIO_CMDBUF_HEAD_MASK) == 0);
> +
> +    qtest_quit(s);
> +}
> +
> +int main(int argc, char **argv)
> +{
> +    g_test_init(&argc, &argv, NULL);
> +    qtest_add_func("/q35/amd-iommu/cmdbuf-head-wrap", test_cmdbuf_head_wrap);
> +    return g_test_run();
> +}
> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
> index 728dde54b3..67eea5c71a 100644
> --- a/tests/qtest/meson.build
> +++ b/tests/qtest/meson.build
> @@ -95,6 +95,7 @@ qtests_i386 = \
>    (config_all_devices.has_key('CONFIG_SB16') ? ['fuzz-sb16-test'] : []) +                   \
>    (config_all_devices.has_key('CONFIG_SDHCI_PCI') ? ['fuzz-sdcard-test'] : []) +            \
>    (config_all_devices.has_key('CONFIG_ESP_PCI') ? ['am53c974-test'] : []) +                 \
> +  (config_all_devices.has_key('CONFIG_AMD_IOMMU') ? ['amd-iommu-test'] : []) +             \
>    (config_all_devices.has_key('CONFIG_VTD') ? ['intel-iommu-test'] : []) +                 \
>    (config_all_devices.has_key('CONFIG_VTD') and
>     config_all_devices.has_key('CONFIG_IOMMU_TESTDEV') ? ['iommu-intel-test'] : []) +      \