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

[Jason Gunthorpe <jgg@nvidia.com>]

On Wed, May 08, 2024 at 05:53:33PM +0100, Will Deacon wrote:
> 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?

If you set ARM_SMMU_V3=y then you don't even get the chance to select
ARM_SMMU_V3_KUNIT_TEST, even though KUNIT_TEST=m is set.

This is not normal or desired, it makes it invisible. The menus look
like this:

All built in:
   [*]   ARM Ltd. System MMU Version 3 (SMMUv3) Support
   [*]     Shared Virtual Addressing support for the ARM SMMUv3
   [ ]       KUnit tests for arm-smmu-v3 driver      

Kunit modular (this patch):
   [*]   ARM Ltd. System MMU Version 3 (SMMUv3) Support
   [*]     Shared Virtual Addressing support for the ARM SMMUv3
   [ ]       KUnit tests for arm-smmu-v3 driver      

Kunit modular (alternative):
   [*]   ARM Ltd. System MMU Version 3 (SMMUv3) Support
   [*]     Shared Virtual Addressing support for the ARM SMMUv3

It just goes away.

> > 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 :/

It happens automatically because this:

config ARM_SMMU_V3_KUNIT_TEST
	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
               ^^^^^^^^^^^^^^^^

They don't need to change their .config. olddefconfig will turn it on
automatically. This is why Thorsten hit it without doing anything
special. Kunit seems to be designed in this automatic way.

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

Yeah, that is one of the novel ways to write the 'compatible
modularity but not force enabled' check

I suspect alot of these just predate the EXPORT_SYMBOL_IF_KUNIT
infrastructure and should probably just be moved into
modules.. modules clearly work better with kunit's ecosystem.

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

Let's not second guess the kunit maintainers, we are just users here..

With this patch the SMMU kunit will be run by Fedora as part of their
testing system since it will be built. I suspect similar is true for
other distros. I think that alone is worth accepting this "hack".

Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel