Re: [PATCH 1/4] vfio: selftests: Introduce vfio_dma_mapping_perf_test

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Format String Mismatch (Undefined Behavior) for 64-bit `u64` values.
- [Low] Unused struct `iommu_mapping` definition.
--

commit ed41e119412fd95d65a62de77183ae8dfc8f2449
Author: Aaron Lewis <aaronlewis@google.com>

vfio: selftests: Introduce vfio_dma_mapping_perf_test

Introduce a tool to aid in latency testing rather than a traditional
pass/fail test. Start with a basic test that performs a simple DMA
map/unmap and reports the duration of each operation.

> diff --git a/tools/testing/selftests/vfio/vfio_dma_mapping_perf_test.c b/tools/testing/selftests/vfio/vfio_dma_mapping_perf_test.c
> new file mode 100644
> index 0000000000000..87d84450fc47e
> --- /dev/null
> +++ b/tools/testing/selftests/vfio/vfio_dma_mapping_perf_test.c
> @@ -0,0 +1,124 @@
> +// SPDX-License-Identifier: GPL-2.0-only
[ ... ]
> +static const char *device_bdf;
> +
> +struct iommu_mapping {
> +	u64 pgd;
> +	u64 p4d;
> +	u64 pud;
> +	u64 pmd;
> +	u64 pte;
> +};

[Severity: Low]
This isn't a bug, but struct iommu_mapping is defined and never used in this
file. Is it needed, or is it left over from earlier iterations?

[ ... ]
> +TEST_F(vfio_dma_mapping_perf_test, dma_map_unmap)
> +{
> +	const u64 size = SZ_1G;
> +	const int flags = variant->mmap_flags;
> +	struct dma_region region;
> +	struct timespec start;
> +	u64 unmapped;
> +	int rc;
[ ... ]
> +	timer_start(&start);
> +	iommu_map(self->iommu, &region);
> +	printf("Mapped HVA %p (size %luG) at IOVA 0x%lx in %.2lfms\n",
> +	       region.vaddr, size / SZ_1G, region.iova, timer_elapsed_ms(start));

[Severity: Medium]
Does this printf format string mismatch cause stack misalignment on 32-bit
platforms?

Because size / SZ_1G and region.iova are 64-bit u64 values, using %lu and %lx
instead of %llu and %llx can misalign the variadic arguments. On 32-bit
systems, %lu expects a 32-bit value, so printf will misread the 64-bit integer,
corrupting the subsequent double argument and producing garbage output for
timer_elapsed_ms().

> +	ASSERT_EQ(region.iova, to_iova(self->device, region.vaddr));
> +
> +	timer_start(&start);
> +	rc = __iommu_unmap(self->iommu, &region, &unmapped);
> +	printf("Unmapped IOVA 0x%lx in %.2lfms\n", region.iova, timer_elapsed_ms(start));

[Severity: Medium]
Similarly here, does region.iova need %llx to avoid stack misalignment issues
with the subsequent double on 32-bit platforms?

> +	ASSERT_EQ(rc, 0);
> +	ASSERT_EQ(unmapped, region.size);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701203311.326798-1-aaronlewis@google.com?part=1