Hi Eric,
On 2026/1/15 20:59, Eric Auger wrote:
Hi Tao,
On 12/24/25 4:46 AM, Tao Tang wrote:
------------------------------<snip>------------------------------
------------------------------<snip>------------------------------
+
+Status
+------
+* Location: ``hw/misc/iommu-testdev.c``
+* Header: ``include/hw/misc/iommu-testdev.h``
+* Build guard: ``CONFIG_IOMMU_TESTDEV``
+
+Device Interface
+----------------
+The device exposes a single PCI BAR0 with 32bit MMIO registers:
+
+* ``ITD_REG_DMA_TRIGGERING`` (0x00): Reading triggers DMA execution
+* ``ITD_REG_DMA_GVA_LO`` (0x04): GVA bits [31:0]
+* ``ITD_REG_DMA_GVA_HI`` (0x08): GVA bits [63:32]
+* ``ITD_REG_DMA_LEN`` (0x0C): DMA transfer length
+* ``ITD_REG_DMA_RESULT`` (0x10): DMA operation result (0=success)
+* ``ITD_REG_DMA_DBELL`` (0x14): Write 1 to arm DMA
I know you provided some explanations to be me earlier about separation
between "arming" and "triggering" but I don't find info in the doc about
what arming practically does and what kind of checks it can enable
compared to the actual trigger. I am not asking for removing it but
maybe document what it aims at.
Thanks for the feedback. I’ll document the intent in the doc.
Today arming only marks the request as armed and sets BUSY; it does not
latch parameters or run checks, so its current effect is intentionally
light. It’s still useful as an explicit gate so qtests can distinguish
NOT_ARMED vs trigger-time failures, and it keeps room for future
extensions (async execution or parameter latching).
+* ``ITD_REG_DMA_ATTRS`` (0x18): DMA attributes which shadow MemTxAttrs format:
+
+ - bit[0]: secure (1=Secure, 0=Non-Secure)
+ - bits[2:1]: address space (0=Non-Secure, 1=Secure)
I was confused by the diff between those 2 fields. If my understanding
is correct they should equal today, until we got some further RME stuff?
Correct?
In MemTxAttrs it is said:
unsigned int secure:1;
/*
* ARM: ArmSecuritySpace. This partially overlaps secure, but it is
* easier to have both fields to assist code that does not understand
* ARMv9 RME, or no specific knowledge of ARM at all (e.g. pflash).
*/
unsigned int space:2;
Yes — today they end up equal in value. But they carry different
semantics: the same numeric value is interpreted differently (e.g.
space==1 means Non-secure, while secure==0 also means Non-secure). I
added both fields to predefine the upcoming split needed for RME,
without implying any RME support in the SMMU yet.
BTW I'll change `bits[2:1]: address space` to be `bits[2:1]:
ArmSecuritySpace` to match the ARM spec as address space is not
particularly accurate.
+ Only these MemTxAttrs fields (``secure`` and ``space``) are consumed today;
+ other bits are reserved but can be wired up easily if future tests need
+ to pass extra attributes.
+
+Translation Setup Workflow
+--------------------------
+``iommu-testdev`` never builds SMMU/AMD-Vi/RISC-V IOMMU structures on its own.
+Architecture-specific construction lives entirely in qtest/libqos helpers.
+Those helpers populate guest memory with page tables/architecture-specific
+structures and program the emulated IOMMU registers directly. See the
+``qsmmu_setup_and_enable_translation()`` function in
+``tests/qtest/libqos/qos-smmuv3.c`` for an example of how SMMUv3 translation
+is set up for this device, which will be introduced in the next commit.
nit: this is rather a commit description comment.
I'll move it into commit message.
+
+DMA Operation Flow
+------------------
+The flow would be split into these steps, mainly for timing control and
+debuggability: qtests can easily exercise and assert distinct paths
+(NOT_ARMED, BAD_LEN, TX/RD failures, mismatch) instead of having all side
+effects hidden behind a single step:
+1. Test programs IOMMU translation tables
+2. Test configures DMA address (GVA_LO/HI), length, and attributes
+3. Test writes 1 to DMA_DBELL to arm the operation
so what does it? is it possible to arm if a transaction is already pending?
Arming just sets the armed gate and BUSY; it doesn’t run checks or start
DMA. Re‑arming while already armed is idempotent: it keeps BUSY and does
not queue a second request.
+4. Test reads DMA_TRIGGERING to execute DMA
+5. Test polls DMA_RESULT:
+
+ - 0x00000000: Success
+ - 0xFFFFFFFE: Busy (still in progress)
+ - 0xDEAD000X: Various error codes
+
+The device performs a write-then-read sequence using a known pattern
+(0x12345678) and verifies data integrity automatically.
+
+Running the qtest
+-----------------
+The SMMUv3 test suite uses this device and covers multiple translation modes::
+
+ cd build-debug
+ QTEST_QEMU_BINARY=./qemu-system-aarch64 \\
+ ./tests/qtest/iommu-smmuv3-test --tap -k
+
+This test suite exercises:
+
+* Stage 1 only translation
+* Stage 2 only translation
+* Nested (Stage 1 + Stage 2) translation
+
+Instantiation
+-------------
+The device is not wired into any board by default. Tests instantiate it
+via QEMU command line::
+
+ -device iommu-testdev
+
+For ARM platforms with SMMUv3::
+
+ -M virt,iommu=smmuv3 -device iommu-testdev
+
+The device will be placed behind the IOMMU automatically.
I guess the device is added on pci.0. so it works by default with
machine wide instantiation but not necessarily with arm-smmuv3 device
which can plugged downstream to a pxb. Maybe reword to avoid confusion.
I’ll reword it in next version.
+
+Limitations
+-----------
+* No realistic PCIe enumeration, MSI/MSI-X, or interrupt handling
+* No ATS/PRI support
+* No actual device functionality beyond DMA test pattern
+* Test-only; not suitable for production or machine realism
+* Address space support (Secure/Root/Realm) is architecture-dependent
+
+See also
+--------
+* ``tests/qtest/iommu-smmuv3-test.c`` — SMMUv3 test suite
+* ``tests/qtest/libqos/qos-smmuv3.{c,h}`` — SMMUv3 test library
+* SMMUv3 emulation: ``hw/arm/smmu*``
diff --git a/hw/misc/Kconfig b/hw/misc/Kconfig
index fccd735c24..b5f6fdbd9c 100644
--- a/hw/misc/Kconfig
+++ b/hw/misc/Kconfig
@@ -25,6 +25,11 @@ config PCI_TESTDEV
default y if TEST_DEVICES
depends on PCI
+config IOMMU_TESTDEV
+ bool
+ default y if TEST_DEVICES
+ depends on PCI
+
config EDU
bool
default y if TEST_DEVICES
diff --git a/hw/misc/iommu-testdev.c b/hw/misc/iommu-testdev.c
new file mode 100644
index 0000000000..2cc1176aa6
--- /dev/null
+++ b/hw/misc/iommu-testdev.c
@@ -0,0 +1,271 @@
+/*
+ * A test device for IOMMU
+ *
+ * Copyright (c) 2025 Phytium Technology
nit 26
I’ll fix the copyright formatting, including other files.
+ *
+ * Author:
+ * Tao Tang <[email protected]>
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "system/address-spaces.h"
+#include "trace.h"
+#include "hw/pci/pci_device.h"
+#include "hw/qdev-properties.h"
+#include "qom/object.h"
+#include "hw/misc/iommu-testdev.h"
+
+#define TYPE_IOMMU_TESTDEV "iommu-testdev"
+OBJECT_DECLARE_SIMPLE_TYPE(IOMMUTestDevState, IOMMU_TESTDEV)
+
+struct IOMMUTestDevState {
+ PCIDevice parent_obj;
+ MemoryRegion bar0;
+ uint64_t dma_vaddr;
+ uint32_t dma_len;
+ uint32_t dma_result;
+ bool dma_pending;
+
+ AddressSpace *dma_as; /* IOMMU-mediated DMA AS for this device */
+ uint32_t dma_attrs_cfg; /* bit0 secure, bits[2:1] space, bit3 unspecified
*/
+};
+
+static void iommu_testdev_maybe_run_dma(IOMMUTestDevState *s)
+{
+ uint32_t expected_val, actual_val;
+ g_autofree uint8_t *write_buf = NULL;
+ g_autofree uint8_t *read_buf = NULL;
+ MemTxResult write_res, read_res;
+ MemTxAttrs attrs;
+ AddressSpace *as;
+
+ if (!s->dma_pending) {
+ s->dma_result = ITD_DMA_ERR_NOT_ARMED;
+ trace_iommu_testdev_dma_result(s->dma_result);
+ return;
+ }
+ trace_iommu_testdev_dma_start();
+
+ s->dma_pending = false;
strange you reset pending before the very access. if I understand
correctly your dma_pending means dma_armed though
You’re right — pending was meant to mean ‘armed’. I’ve changed it so the
flag stays set until the DMA finishes (cleared only at the end), and
renamed it to dma_armed for clarity.
+
+ if (!s->dma_len) {
+ s->dma_result = ITD_DMA_ERR_BAD_LEN;
+ return;
+ }
+
+ write_buf = g_malloc(s->dma_len);
+ read_buf = g_malloc(s->dma_len);
+
+ /* Initialize MemTxAttrs from generic register */
+ attrs.secure = ITD_ATTRS_GET_SECURE(s->dma_attrs_cfg);
+
+ /* The 'space' field in MemTxAttrs is ARM-specific. */
+ attrs.space = ITD_ATTRS_GET_SPACE(s->dma_attrs_cfg);
maybe check they both secure and space are somehow consistent?
I will check the consistency between space and secure.
+
+ as = s->dma_as;
+
+ /* Step 1: Write ITD_DMA_WRITE_VAL to DMA address */
+ trace_iommu_testdev_dma_write(s->dma_vaddr, s->dma_len);
+
+ for (int i = 0; i < s->dma_len; i++) {
+ /* Data is written in little-endian order */
+ write_buf[i] = (ITD_DMA_WRITE_VAL >> ((i % 4) * 8)) & 0xff;
+ }
+ write_res = dma_memory_write(as, s->dma_vaddr, write_buf, s->dma_len,
attrs);
+
+ if (write_res != MEMTX_OK) {
+ s->dma_result = ITD_DMA_ERR_TX_FAIL;
+ trace_iommu_testdev_dma_result(s->dma_result);
+ return;
+ }
+
+ /* Step 2: Read back from the same DMA address */
+ trace_iommu_testdev_dma_read(s->dma_vaddr, s->dma_len);
+
+ read_res = dma_memory_read(as, s->dma_vaddr, read_buf, s->dma_len, attrs);
This assumes the IOMMU translation is correct. If it is bad in both
directions, you may read the same value but does not necessarily means
the translated addr is correct.
Wouldn't it be better to read the mem at expected GPA without going
through the IOMMU?
Sure. I just tried to pass GPA into iommu-testdev and address_space_read
from GPA and then validated the result. It works well and I'll add this
logic in V9. Thanks for your suggestion.
------------------------------<snip>------------------------------
------------------------------<snip>------------------------------
+static void iommu_testdev_mmio_write(void *opaque, hwaddr addr, uint64_t val,
+ unsigned size)
+{
+ IOMMUTestDevState *s = opaque;
+ uint32_t data = val;
+
+ trace_iommu_testdev_mmio_write(addr, val, size);
+
+ switch (addr) {
+ case ITD_REG_DMA_GVA_LO:
+ s->dma_vaddr = (s->dma_vaddr & ~0xffffffffull) | data;
+ break;
+ case ITD_REG_DMA_GVA_HI:
+ s->dma_vaddr = (s->dma_vaddr & 0xffffffffull) |
+ ((uint64_t)data << 32);
+ break;
+ case ITD_REG_DMA_LEN:
+ s->dma_len = data;
+ break;
+ case ITD_REG_DMA_RESULT:
+ s->dma_result = data;
+ break;
+ case ITD_REG_DMA_DBELL:
+ if (data & ITD_DMA_DBELL_ARM) {
+ /* Arm the DMA operation */
+ s->dma_pending = true;
dma_armed?
I'll change it as mentioned earlier.
Thank you again for your thorough and detailed code review.
Best regards,
Tao
