[PATCH] iommu/arm-smmu-v3: Make the kunit into a module

[Will Deacon]

On Tue, May 07, 2024 at 11:33:21AM -0300, Jason Gunthorpe wrote:
> On Tue, May 07, 2024 at 03:22:48PM +0100, Will Deacon wrote:
> > On Tue, May 07, 2024 at 11:09:46AM -0300, Jason Gunthorpe wrote:
> > > On Tue, May 07, 2024 at 02:58:17PM +0100, Will Deacon wrote:
> > > > On Tue, May 07, 2024 at 10:21:10AM -0300, Jason Gunthorpe wrote:
> > > > > diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> > > > > index 66325210c8c986..c04584be30893f 100644
> > > > > --- a/drivers/iommu/Kconfig
> > > > > +++ b/drivers/iommu/Kconfig
> > > > > @@ -415,7 +415,7 @@ config ARM_SMMU_V3_SVA
> > > > >  	  and PRI.
> > > > >  
> > > > >  config ARM_SMMU_V3_KUNIT_TEST
> > > > > -	bool "KUnit tests for arm-smmu-v3 driver"  if !KUNIT_ALL_TESTS
> > > > > +	tristate "KUnit tests for arm-smmu-v3 driver"  if !KUNIT_ALL_TESTS
> > > > >  	depends on KUNIT
> > > > >  	depends on ARM_SMMU_V3_SVA
> > > > >  	default KUNIT_ALL_TESTS
> > > > 
> > > > Would it work to leave this as 'bool' and have something like:
> > > > 
> > > > 	depends on KUNIT=y
> > > 
> > > Yes, there is a version like this (depends on KUNIT = ARM_SMMU_V3),
> > > but it made kconfig act a little weird and hide the symbols.
> > 
> > Which symbols were hidden in which cases? Having ARM_SMMU_V3_KUNIT_TEST
> > disappear from menuconfig when the dependency isn't satisifed sounds
> > fine to me, but you make it sound like it was something else.
> 
> That isn't normal, it should show up as [ ] and be limited to M, not
> be hidden completely.

Sorry, but please can you explain a little more? I'm having to guess at
a lot of the details here. Are you saying that the option disappears
even when ARM_SMMU_V3=m?

> > > > instead? That would be a lot simpler and avoids all the conditional
> > > > symbol exports.
> > > 
> > > But then Fedora is linking this code into their production kernel
> > > which doesn't seem right.
> > 
> > Well, only if they select CONFIG_ARM_SMMU_V3_KUNIT_TEST, right? There
> > also seems to be precedence for that if I grep around:
> 
> Thorsten said they are selecting that..

I find that hard to believe given that the option didn't exist until
about week ago and isn't in any released kernels :/

> > drivers/base/Kconfig:	depends on KUNIT=y
> > drivers/fpga/tests/Kconfig:	depends on FPGA=y && FPGA_REGION=y && FPGA_BRIDGE=y && KUNIT=y && MODULES=n
> > drivers/gpu/drm/xe/Kconfig:	depends on DRM && PCI && MMU && (m || (y && KUNIT=y))
> > drivers/mmc/host/Kconfig:	depends on (MMC_SDHCI_OF_ASPEED=m || KUNIT=y)
> > drivers/net/ethernet/microchip/vcap/Kconfig:	depends on KUNIT=y && VCAP=y && y
> > drivers/thunderbolt/Kconfig:	depends on USB4 && KUNIT=y
> > drivers/virt/nitro_enclaves/Kconfig:	depends on NITRO_ENCLAVES && KUNIT=y
> > fs/Kconfig.binfmt:	depends on KUNIT=y && BINFMT_ELF=y
> > lib/Kconfig.debug:	depends on KUNIT=y
> > lib/Kconfig.debug:	depends on KUNIT=y
> > lib/Kconfig.debug:	depends on RUST && KUNIT=y
> > mm/damon/Kconfig:	depends on DAMON && KUNIT=y
> > mm/damon/Kconfig:	depends on DAMON_VADDR && KUNIT=y
> > mm/damon/Kconfig:	depends on DAMON_SYSFS && KUNIT=y
> > mm/damon/Kconfig:	depends on DAMON_DBGFS && KUNIT=y
> > net/mctp/Kconfig:        depends on MCTP=y && KUNIT=y
> > security/landlock/Kconfig:	depends on KUNIT=y
> 
> These cases don't seem to involve a module, eg BINFMT_ELF can't be a
> module.

The DRM_XE one is tristate and has this interesting variant:

	depends on ... && (m || (y && KUNIT=y))

> I see the majority of kunit setups have it put the test into its own
> module.
> 
> What's the issue here? This is the majority way to do kunit, I just
> made a mistake not doing it fully when Mostafa brought it up.

I just think that EXPORT_SYMBOL_IF_KUNIT looks like a pretty horrible
hack and it would be nice to avoid it entirely with a one-liner Kconfig
change, even if that slightly limits the configurations in which the
unit tests are available.

Will