Re: [RFC v2 2/2] tests/qtest: add SMMUv3 smoke test using smmu-testdev DMA source

[Pierrick Bouvier <pierrick.bouvier@linaro.org>]

Hi Tao,

On 2025-10-27 16:26, Tao Tang wrote:
> Hi Alex,
> 
> On 2025/10/23 19:02, Alex Bennée wrote:
>> tangtao1634 <tangtao1634@phytium.com.cn> writes:
>>
>>> From: Tao Tang <tangtao1634@phytium.com.cn>
>>>
>>> Introduce a bare-metal qtest that drives the new smmu-testdev to exercise
>>> the SMMUv3 emulation without guest firmware or drivers. The test programs
>>> a minimal Non-Secure context (STE/CD/PTE), triggers a DMA, and asserts
>>> translation results.
>>>
>>> Motivation
>>> ----------
>>> SMMU testing in emulation often requires a large software stack and a
>>> realistic PCIe fabric, which adds flakiness and obscures failures. This
>>> qtest keeps the surface small and deterministic by using a hermetic DMA
>>> source that feeds the SMMU directly.
>>>
>>> What the test covers
>>> --------------------
>>> * Builds a Non-Secure STE/CD/PTE for a chosen stream_id/ssid.
>>> * Primes source and destination host buffers.
>>> * Kicks a DMA via smmu-testdev and waits for completion.
>>> * Verifies translated access and payload equality.
>>>
>>> Non-goals and scope limits
>>> --------------------------
>>> * Secure bank flows are omitted because Secure SMMU support is still RFC.
>>>     A local Secure test exists and can be posted once the upstream series
>>>     lands.
>>> * PCIe discovery, MSI/INTx, ATS/PRI, and driver bring-up are out of scope
>>>     as smmu-testdev is not a realistic PCIe Endpoint nor a platform device.
>>>
>>> Rationale for a dedicated path
>>> ------------------------------
>>> Using a generic PCI or virtio device would still require driver init and a
>>> richer bus model, undermining determinism for this focused purpose. This
>>> qtest, paired with smmu-testdev, keeps failures attributable to the SMMU
>>> translation path.
>>>
>>> Finally we document the smmu-testdev device in docs/specs.
>>>
>>> Signed-off-by: Tao Tang <tangtao1634@phytium.com.cn>
>>> ---
>>> ------------------------------<snip>------------------------------
>>>
>>>
>>>
>>> ------------------------------<snip>------------------------------
>>> +
>>> +    /* Find device by vendor/device ID to avoid slot surprises. */
>>> +    dev = NULL;
>> might as well init when you declare.
>>
>>> +    g_assert_nonnull(dev);
>> surely g_assert(dev) would do.
>>
>>> +    const uint32_t modes[] = { 0u, 1u, 2u }; /* Stage1, Stage2, Nested stage */
>>> +    const SMMUTestDevSpace spaces[] = { STD_SPACE_NONSECURE };
>> top of block.
> 
> 
> Thank you very much for your valuable feedback. Also I will refactor
> these codes with the guide of summarized plans as described in patch #1.
> 
>>
>>> +    /* Use attrs-DMA path for end-to-end */
>>> +    qpci_io_writel(dev, bar, STD_REG_DMA_MODE, 1);
>>> +    for (size_t mi = 0; mi < sizeof(modes) / sizeof(modes[0]); mi++) {
>>> +        const SMMUTestDevSpace *s1_set = NULL;
>>> +        size_t s1_count = 0;
>>> +        const SMMUTestDevSpace *s2_set = NULL;
>>> +        size_t s2_count = 0;
>>> +
>>> +        switch (modes[mi]) {
>>> +        case 0u:
>>> +        case 1u:
>>> +        case 2u:
>>> +            s1_set = spaces;
>>> +            s1_count = sizeof(spaces) / sizeof(spaces[0]);
>>> +            s2_set = spaces;
>>> +            s2_count = sizeof(spaces) / sizeof(spaces[0]);
>>> +            break;
>>> +        default:
>>> +            g_assert_not_reached();
>>> +        }
>>> +
>>> +        for (size_t si = 0; si < s1_count; si++) {
>>> +            for (size_t sj = 0; sj < s2_count; sj++) {
>>> +                qpci_io_writel(dev, bar, STD_REG_TRANS_MODE, modes[mi]);
>>> +                qpci_io_writel(dev, bar, STD_REG_S1_SPACE, s1_set[si]);
>>> +                qpci_io_writel(dev, bar, STD_REG_S2_SPACE, s2_set[sj]);
>>> +                qpci_io_writel(dev, bar, STD_REG_TRANS_DBELL, 0x2);
>>> +                qpci_io_writel(dev, bar, STD_REG_TRANS_DBELL, 0x1);
>>> +
>>> +                uint32_t st = qpci_io_readl(dev, bar,
>>> +                                            STD_REG_TRANS_STATUS);
>>> +                g_test_message("build: stage=%s s1=%s s2=%s status=0x%x",
>>> +                                std_mode_to_str(modes[mi]),
>>> +                                std_space_to_str(s1_set[si]),
>>> +                                std_space_to_str(s2_set[sj]), st);
>>> +                /* Program SMMU registers in selected control bank. */
>>> +                smmu_prog_minimal(qts, s1_set[si]);
>>> +
>>> +                /* End-to-end DMA using tx_space per mode. */
>>> +                SMMUTestDevSpace tx_space =
>>> +                    (modes[mi] == 0u) ? s1_set[si] : s2_set[sj];
>>> +                uint32_t dma_attrs = ((uint32_t)tx_space << 1);
>>> +                qpci_io_writel(dev, bar, STD_REG_DMA_ATTRS,
>>> +                                dma_attrs);
>>> +                qpci_io_writel(dev, bar, STD_REG_DMA_DBELL, 1);
>>> +                /* Wait for DMA completion and assert success. */
>>> +                {
>>> +                    uint32_t dr = poll_dma_result(dev, bar, qts);
>>> +                    uint32_t exp = expected_dma_result(modes[mi],
>>> +                                                        spaces[si],
>>> +                                                        spaces[sj]);
>>> +                    g_assert_cmpuint(dr, ==, exp);
>>> +                    g_test_message("polling end. attrs=0x%x res=0x%x",
>>> +                                   dma_attrs, dr);
>>> +                }
>>> +                /* Clear CD/STE/PTE built by the device for next round. */
>>> +                qpci_io_writel(dev, bar, STD_REG_TRANS_CLEAR, 1);
>>> +                g_test_message("clear cache end.");
>>> +            }
>>> +        }
>>> +    }
>> I suspect this function could be broken up a bit as new tests are added
>> and functionality shared?
> 
> 
> Sure. I've actually been thinking along the same lines. As I plan for
> future tests, I'm considering how best to organize the test cases given
> the numerous combinations of features we'll need to cover. For example,
> beyond iterating through security states and translation stages, we will
> also need to test many other parameters, such as:
> 
> - Linear vs. two-level Stream Tables
> 
> - Different Output Address Sizes (Although only support 44bits in
> current SMMU implementation)
>

Reading through this, I start to wonder if we will not end up rewriting 
a full SMMU driver by accident. The problem with SMMU development is 
that from the outside, it seems to be "just a device translating DMA 
accesses". In reality, the "just" means we have a stateful device, 
configured from possibly different parts in a software stack. For 
example, with Realms, TF-A, RMM, and kernel all contribute to this state.

A possible analogy would be if we used a QTest device to test QEMU MMU 
implementation, instead of simply relying on running a kernel exercising 
this code.

That said, it's still useful for some basic scenarios, but I'm not sure 
it's the ultimate answer for complex use cases, and thus, it should not 
try to cover it.
As well, this brings the question of which kind of solution we would 
need for that. It seems that one need would be to check the SMMU "state" 
from user space, which moves the problem on having a driver able to poke 
this state.

> My question to you and the wider group is, how far should we go in
> covering these combinations for an initial smoke test? The current loops
> for security state and translation stage cover the basics, but I'm
> wondering if we should aim for more complexity at this stage, or if
> that's a task for future patches. I'd be very interested to hear
> everyone's opinion on the right scope.
>

We have to start somewhere, so something simple and not trying to solve 
all use cases is the right approach. It can even just be read/write 
config/registers before trying to add any DMA scenario.

> In any case, your suggestion to break the current test logic into
> smaller, shared functions is definitely the right first step to manage
> the structure. I will refactor the code accordingly in the next version.
> 
> Thanks again for the valuable suggestion!
> 
> Best regards,
> 
> Tao
>