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 <[email protected]> >>>>> --- >>>>> 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/[email protected]/ >>> >>> >>>> >>>> ... >>>> >>>> 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 >
