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/[email protected]/ 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 <[email protected]> let me know if you agree with the above approach. Now a couple of comments on the test itself... > Signed-off-by: Costas Argyris <[email protected]> > --- > 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'] > : []) + \
