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

From: Mostafa Saleh <smostafa@google.com>
To: Jason Gunthorpe <jgg@nvidia.com>
Cc: iommu@lists.linux.dev, Joerg Roedel <joro@8bytes.org>,
	linux-arm-kernel@lists.infradead.org,
	Robin Murphy <robin.murphy@arm.com>,
	Will Deacon <will@kernel.org>, Eric Auger <eric.auger@redhat.com>,
	Moritz Fischer <mdf@kernel.org>,
	Moritz Fischer <moritzf@google.com>,
	Michael Shavit <mshavit@google.com>,
	Nicolin Chen <nicolinc@nvidia.com>,
	patches@lists.linux.dev,
	Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
Subject: Re: [PATCH v7 9/9] iommu/arm-smmu-v3: Add unit tests for arm_smmu_write_entry
Date: Sat, 27 Apr 2024 22:33:55 +0000	[thread overview]
Message-ID: <Zi19U2MufT9W3Jxe@google.com> (raw)
In-Reply-To: <20240422142429.GE49823@nvidia.com>

On Mon, Apr 22, 2024 at 11:24:29AM -0300, Jason Gunthorpe wrote:
> 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?
> 

Sounds good, I guess that option will be only used for development.

Thanks,
Mostafa

> > > @@ -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