Re: [RFC RESEND v5 3/4] tests/qtest/libqos: Add SMMUv3 helper library

[Eric Auger <eric.auger@redhat.com>]

Hi Tao,

On 12/6/25 6:27 AM, Tao Tang wrote:
>
> On 2025/12/6 01:19, Pierrick Bouvier wrote:
>> On 12/5/25 7:03 AM, Tao Tang wrote:
>>> Hi Pierrick,
>>>
>>> On 2025/12/5 07:53, Pierrick Bouvier wrote:
>>>> On 11/26/25 7:45 AM, Tao Tang wrote:
>>>>> Introduce qos-smmuv3, a reusable library for SMMUv3-related qtest
>>>>> operations. This module encapsulates common tasks like:
>>>>>
>>>>> - SMMUv3 initialization (enabling, configuring command/event queues)
>>>>> - Stream Table Entry (STE) and Context Descriptor (CD) setup
>>>>> - Multi-level page table construction (L0-L3 for 4KB granules)
>>>>> - Support for Stage 1, Stage 2, and nested translation modes
>>>>> - Could be easily extended to support multi-space testing
>>>>> infrastructure
>>>>>       (Non-Secure, Secure, Root, Realm)
>>>>>
>>>>> The library provides high-level abstractions that allow test code to
>>>>> focus on IOMMU behavior validation rather than low-level register
>>>>> manipulation and page table encoding. Key features include:
>>>>>
>>>>> - Automatic memory allocation for translation structures with proper
>>>>>       alignment
>>>>> - Helper functions to build valid STEs/CDs for different translation
>>>>>       scenarios
>>>>> - Page table walkers that handle address offset calculations per
>>>>>       security space
>>>>> - Command queue management for SMMU configuration commands
>>>>>
>>>>> This infrastructure is designed to be used by iommu-testdev-based
>>>>> tests
>>>>> and future SMMUv3 test suites, reducing code duplication and
>>>>> improving
>>>>> test maintainability.
>>>>>
>>>>> Signed-off-by: Tao Tang <tangtao1634@phytium.com.cn>
>>>>> ---
>>>>>    tests/qtest/libqos/meson.build  |   3 +
>>>>>    tests/qtest/libqos/qos-smmuv3.c | 731
>>>>> ++++++++++++++++++++++++++++++++
>>>>>    tests/qtest/libqos/qos-smmuv3.h | 267 ++++++++++++
>>>>>    3 files changed, 1001 insertions(+)
>>>>>    create mode 100644 tests/qtest/libqos/qos-smmuv3.c
>>>>>    create mode 100644 tests/qtest/libqos/qos-smmuv3.h
>>>>>
>>>>
>>>> ...
>>>>
>>>>> +
>>>>> +void qsmmu_single_translation(QSMMUTestContext *ctx)
>>>>> +{
>>>>> +    uint32_t config_result;
>>>>> +    uint32_t dma_result;
>>>>> +    bool test_passed;
>>>>> +
>>>>> +    /* Configure SMMU translation */
>>>>> +    config_result = qsmmu_setup_and_enable_translation(ctx);
>>>>> +    if (config_result != 0) {
>>>>> +        g_test_message("Configuration failed: mode=%u status=0x%x",
>>>>> +                       ctx->config.trans_mode, config_result);
>>>>> +        return;
>>>>
>>>> Is that expected to silently return if we can't configure translation?
>>>
>>>
>>> No, it is not intended to silently return on a failed configuration.
>>> Maybe an assertion is a better choice:
>>>
>>>
>>> config_result = qsmmu_setup_and_enable_translation(ctx);
>>>
>>> g_assert_cmpuint(config_result, ==, 0);
>>>
>>
>> Looks good. We should rely on exit code first, and then on verbose
>> log to find what is the problem.
>>
>>>>
>>>>> +    }
>>>>> +
>>>>> +    /* Trigger DMA operation */
>>>>> +    dma_result = qsmmu_trigger_dma(ctx);
>>>>> +    if (dma_result != 0) {
>>>>> +        g_test_message("DMA failed: mode=%u result=0x%x",
>>>>> +                       ctx->config.trans_mode, dma_result);
>>>>> +    } else {
>>>>> +        g_test_message("-> DMA succeeded: mode=%u",
>>>>> ctx->config.trans_mode);
>>>>> +    }
>>>>> +
>>>>> +    /* Validate test result */
>>>>> +    test_passed = qsmmu_validate_test_result(ctx);
>>>>> +    g_assert_true(test_passed);
>>>>> +
>>>>> +    /* Clean up translation state to prepare for the next test */
>>>>> +    qsmmu_cleanup_translation(ctx);
>>>>> +}
>>>>> +
>>>>> +void qsmmu_translation_batch(const QSMMUTestConfig *configs, size_t
>>>>> count,
>>>>> +                             QTestState *qts, QPCIDevice *dev,
>>>>> +                             QPCIBar bar, uint64_t smmu_base)
>>>>> +{
>>>>> +    for (int i = 0; i < count; i++) {
>>>>> +        /* Initialize test memory */
>>>>> +        qtest_memset(qts, configs[i].dma_iova, 0x00,
>>>>> configs[i].dma_len);
>>>>> +        /* Execute each test configuration */
>>>>> +        QSMMUTestContext ctx = {
>>>>> +            .qts = qts,
>>>>> +            .dev = dev,
>>>>> +            .bar = bar,
>>>>> +            .smmu_base = smmu_base,
>>>>> +            .config = configs[i],
>>>>> +            .trans_status = 0,
>>>>> +            .dma_result = 0,
>>>>> +            .sid = dev->devfn,
>>>>> +            .tx_space = qsmmu_sec_sid_to_space(configs[i].sec_sid),
>>>>> +        };
>>>>> +
>>>>> +        qsmmu_single_translation(&ctx);
>>>>> +        g_test_message("--> Test %d completed: mode=%u sec_sid=%u "
>>>>> +                       "status=0x%x result=0x%x", i,
>>>>> configs[i].trans_mode,
>>>>> +                       configs[i].sec_sid, ctx.trans_status,
>>>>> ctx.dma_result);
>>>>> +    }
>>>>> +}
>>>>
>>>> What is the reason for batching operations?
>>>> We are not in a performance critical scenario for running this test,
>>>> so it's probably better to have distinct calls to single_translation.
>>>
>>>
>>> As described in the previous thread [1] , I plan to split the tests so
>>> that each translation mode is exercised by its own qtest. With that
>>> split in place, there is no real need for a qsmmu_translation_batch()
>>> helper anymore, so I refactor it into a qsmmu_run_translation_case
>>> function and drop the inside for-loop.
>>>
>>
>> All good, indeed removes the need for translation_batch.
>>
>>>
>>> [1]
>>> https://lore.kernel.org/qemu-devel/7370070a-c569-4b77-bd1e-6fc749ba9c90@phytium.com.cn/
>>>
>>>
>>>>
>>>> ...
>>>>
>>>> For the rest of the patch, which is quite consequent, congrats. It's
>>>> hard to review all the setup phase here, but knowing it works with the
>>>> current smmuv3 implementation, that's a good proof that it's working
>>>> as expected.
>>>
>>>
>>> Yes, setting up all this infrastructure did take some time, especially
>>> getting the nested mode page tables right (and Secure state-related
>>> configuration which is still in my local repo).
>>>
>>
>> Feel free to start with the current version, and then you'll add
>> secure state related changes as part of your other series.
>>
>>> I really appreciate that you ran the tests yourself and even checked
>>> with a coverage-enabled build to confirm that it exercises the smmuv3
>>> implementation. Thanks again for the thorough review.
>>>
>>
>> In case someone else wants to reproduce:
>> $ export CFLAGS="--coverage"
>> $ ./configure --target-list=aarch64-softmmu
>> $ ninja -C build
>> $ QTEST_QEMU_BINARY=./build/qemu-system-aarch64 \
>>   ./build/tests/qtest/iommu-smmuv3-test
>> $ rm -rf build/coverage_html
>> $ mkdir build/coverage_html
>> $ gcovr \
>>       --gcov-ignore-parse-errors suspicious_hits.warn \
>>       --gcov-ignore-parse-errors negative_hits.warn \
>>       --merge-mode-functions=separate \
>>       --html-details build/coverage_html/index.html \
>>       --filter 'hw/arm/smmu*'
>> $ echo file://$(pwd)/build/coverage_html/index.html
>> # open this in browser by clicking on your terminal
>>
>> If useful for you, you can attach those instructions in your next
>> cover letter, so people can easily reproduce. 

are you ready to maintain that code (esp the lib)? You shall add an
entry in the MAINTAINERS file for those new files I guess.

Eric
>>
>
> Hi Pierrick,
>
> Got it, I’ll keep this series focused on the current non-secure smmuv3
> implementation, and the coverage instructions you shared are very
> helpful — I’ll add them to the next cover letter so that other
> reviewers can easily reproduce the results.
>
> Thanks again for all the detailed review and guidance.
>
> Best regards,
> Tao
>