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

[Jason Gunthorpe <jgg@nvidia.com>]

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