Re: [PATCH v7 9/9] iommu/arm-smmu-v3: Add unit tests for arm_smmu_write_entry

Jason Gunthorpe
2024-04-22T14:24:59Z

On Fri, Apr 19, 2024 at 09:24:52PM +0000, Mostafa Saleh wrote:
> > +static void arm_smmu_test_make_sva_release_cd(struct arm_smmu_cd *cd,
> > + unsigned int asid)
> > +{
> > + struct arm_smmu_master master = {
> > + .smmu = &smmu,
> > + };
> > +
> > + arm_smmu_make_sva_cd(cd, &master, NULL, asid);
> > +}
> > +
>
> The test doesn’t build with SVA disabled, it fails with:
> aarch64-linux-gnu-ld: Unexpected GOT/PLT entries detected!
> aarch64-linux-gnu-ld: Unexpected run-time procedure linkages detected!
> aarch64-linux-gnu-ld: drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-test.o: in function `arm_smmu_test_make_sva_release_cd':
> .../linux/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c:409:(.text+0x17c): undefined reference to `arm_smmu_make_sva_cd'
> aarch64-linux-gnu-ld: drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-test.o: in function `arm_smmu_test_make_sva_cd':
> .../linux/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c:399:(.text+0x230): undefined reference to `arm_smmu_make_sva_cd'
>
> I belive this check should be guarded under SVA.

Ugh yes, 0-day just hit this too.

I'm just going to do this:

config ARM_SMMU_V3_KUNIT_TEST
bool "KUnit tests for arm-smmu-v3 driver" if !KUNIT_ALL_TESTS
depends on KUNIT
+ depends on ARM_SMMU_V3_SVA
default KUNIT_ALL_TESTS
help
Enable this option to unit-test arm-smmu-v3 driver functions.

Instead of adding #ifdefs. No reason not to test the whole driver?

> > @@ -980,7 +968,7 @@ void arm_smmu_tlb_inv_asid(struct arm_smmu_device *smmu, u16 asid)
> > * would be nice if this was complete according to the spec, but minimally it
> > * has to capture the bits this driver uses.
> > */
> > -static void arm_smmu_get_ste_used(const __le64 *ent, __le64 *used_bits)
> > +void arm_smmu_get_ste_used(const __le64 *ent, __le64 *used_bits)
>
> IMO we should not export all these low level functions unconditionally.
> KUNIT already defines “VISIBLE_IF_KUNIT” which sets symbols to be static
> if CONFIG_KUNIT is not enabled. Or maybe even guard it for this test
> like what btrfs does with “EXPORT_FOR_TESTS”

Sure, that doesn't look like too much trouble long term.

Thanks,
Jason