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

[Jason Gunthorpe <jgg@nvidia.com>]

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:
> > > > It turns out kconfig has problems ensuring the SMMU module and the KUNIT
> > > > module are consistently y/m to allow linking. It will permit KUNIT to be a
> > > > module while SMMU is built in.
> > > > 
> > > > Also, Fedora apparently enables kunit on production kernels.
> > > > 
> > > > So, put the entire kunit in its own module using the
> > > > VISIBLE_IF_KUNIT/EXPORT_SYMBOL_IF_KUNIT machinery. This keeps it out of
> > > > vmlinus on Fedora and makes the kconfig work in the normal way. There is
> > > > no cost if kunit is disabled.
> > > > 
> > > > Fixes: 56e1a4cc2588 ("iommu/arm-smmu-v3: Add unit tests for arm_smmu_write_entry")
> > > > Reported-by: Thorsten Leemhuis <linux@leemhuis.info>
> > > > Link: https://lore.kernel.org/all/aeea8546-5bce-4c51-b506-5d2008e52fef@leemhuis.info
> > > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> > > > ---
> > > >  drivers/iommu/Kconfig                            | 2 +-
> > > >  drivers/iommu/arm/arm-smmu-v3/Makefile           | 3 ++-
> > > >  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c  | 1 +
> > > >  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c | 3 +++
> > > >  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c      | 8 ++++++++
> > > >  5 files changed, 15 insertions(+), 2 deletions(-)
> > > > 
> > > > Joerg, can you pick this to solve the linux-next issue for Thorsten?
> > > > 
> > > > Thanks,
> > > > Jason
> > > > 
> > > > 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.

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

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

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.

Jason