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

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

[Jason Gunthorpe]

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
diff --git a/drivers/iommu/arm/arm-smmu-v3/Makefile b/drivers/iommu/arm/arm-smmu-v3/Makefile
index 0b97054b3929b7..014a997753a8a2 100644
--- a/drivers/iommu/arm/arm-smmu-v3/Makefile
+++ b/drivers/iommu/arm/arm-smmu-v3/Makefile
@@ -2,5 +2,6 @@
 obj-$(CONFIG_ARM_SMMU_V3) += arm_smmu_v3.o
 arm_smmu_v3-objs-y += arm-smmu-v3.o
 arm_smmu_v3-objs-$(CONFIG_ARM_SMMU_V3_SVA) += arm-smmu-v3-sva.o
-arm_smmu_v3-objs-$(CONFIG_ARM_SMMU_V3_KUNIT_TEST) += arm-smmu-v3-test.o
 arm_smmu_v3-objs := $(arm_smmu_v3-objs-y)
+
+obj-$(CONFIG_ARM_SMMU_V3_KUNIT_TEST) += arm-smmu-v3-test.o
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
index 34a977a0767d46..e490ffb3801545 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
@@ -185,6 +185,7 @@ void arm_smmu_make_sva_cd(struct arm_smmu_cd *target,
 	 */
 	target->data[3] = cpu_to_le64(read_sysreg(mair_el1));
 }
+EXPORT_SYMBOL_IF_KUNIT(arm_smmu_make_sva_cd);
 
 static struct arm_smmu_ctx_desc *arm_smmu_alloc_shared_cd(struct mm_struct *mm)
 {
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c
index 417804392ff089..315e487fd990eb 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c
@@ -463,3 +463,6 @@ static struct kunit_suite arm_smmu_v3_test_module = {
 	.test_cases = arm_smmu_v3_test_cases,
 };
 kunit_test_suites(&arm_smmu_v3_test_module);
+
+MODULE_IMPORT_NS(EXPORTED_FOR_KUNIT_TESTING);
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 4820b8b5dd81b3..ab415e107054c1 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1007,6 +1007,7 @@ void arm_smmu_get_ste_used(const __le64 *ent, __le64 *used_bits)
 	if (cfg == STRTAB_STE_0_CFG_BYPASS)
 		used_bits[1] |= cpu_to_le64(STRTAB_STE_1_SHCFG);
 }
+EXPORT_SYMBOL_IF_KUNIT(arm_smmu_get_ste_used);
 
 /*
  * Figure out if we can do a hitless update of entry to become target. Returns a
@@ -1141,6 +1142,7 @@ void arm_smmu_write_entry(struct arm_smmu_entry_writer *writer, __le64 *entry,
 			entry_set(writer, entry, target, 0, NUM_ENTRY_QWORDS));
 	}
 }
+EXPORT_SYMBOL_IF_KUNIT(arm_smmu_write_entry);
 
 static void arm_smmu_sync_cd(struct arm_smmu_master *master,
 			     int ssid, bool leaf)
@@ -1268,6 +1270,7 @@ void arm_smmu_get_cd_used(const __le64 *ent, __le64 *used_bits)
 		used_bits[1] &= ~cpu_to_le64(CTXDESC_CD_1_TTB0_MASK);
 	}
 }
+EXPORT_SYMBOL_IF_KUNIT(arm_smmu_get_cd_used);
 
 static void arm_smmu_cd_writer_sync_entry(struct arm_smmu_entry_writer *writer)
 {
@@ -1332,6 +1335,7 @@ void arm_smmu_make_s1_cd(struct arm_smmu_cd *target,
 				      CTXDESC_CD_1_TTB0_MASK);
 	target->data[3] = cpu_to_le64(pgtbl_cfg->arm_lpae_s1_cfg.mair);
 }
+EXPORT_SYMBOL_IF_KUNIT(arm_smmu_make_s1_cd);
 
 void arm_smmu_clear_cd(struct arm_smmu_master *master, ioasid_t ssid)
 {
@@ -1515,6 +1519,7 @@ void arm_smmu_make_abort_ste(struct arm_smmu_ste *target)
 		STRTAB_STE_0_V |
 		FIELD_PREP(STRTAB_STE_0_CFG, STRTAB_STE_0_CFG_ABORT));
 }
+EXPORT_SYMBOL_IF_KUNIT(arm_smmu_make_abort_ste);
 
 VISIBLE_IF_KUNIT
 void arm_smmu_make_bypass_ste(struct arm_smmu_device *smmu,
@@ -1529,6 +1534,7 @@ void arm_smmu_make_bypass_ste(struct arm_smmu_device *smmu,
 		target->data[1] = cpu_to_le64(FIELD_PREP(STRTAB_STE_1_SHCFG,
 							 STRTAB_STE_1_SHCFG_INCOMING));
 }
+EXPORT_SYMBOL_IF_KUNIT(arm_smmu_make_bypass_ste);
 
 VISIBLE_IF_KUNIT
 void arm_smmu_make_cdtable_ste(struct arm_smmu_ste *target,
@@ -1580,6 +1586,7 @@ void arm_smmu_make_cdtable_ste(struct arm_smmu_ste *target,
 			cpu_to_le64(FIELD_PREP(STRTAB_STE_2_S2VMID, 0));
 	}
 }
+EXPORT_SYMBOL_IF_KUNIT(arm_smmu_make_cdtable_ste);
 
 VISIBLE_IF_KUNIT
 void arm_smmu_make_s2_domain_ste(struct arm_smmu_ste *target,
@@ -1627,6 +1634,7 @@ void arm_smmu_make_s2_domain_ste(struct arm_smmu_ste *target,
 	target->data[3] = cpu_to_le64(pgtbl_cfg->arm_lpae_s2_cfg.vttbr &
 				      STRTAB_STE_3_S2TTB_MASK);
 }
+EXPORT_SYMBOL_IF_KUNIT(arm_smmu_make_s2_domain_ste);
 
 /*
  * This can safely directly manipulate the STE memory without a sync sequence

base-commit: 36bb003ed6860aa478c0503a6c44725064679085
-- 
2.43.2

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

[Will Deacon]

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

instead? That would be a lot simpler and avoids all the conditional
symbol exports.

Will

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

[Jason Gunthorpe]

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.

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

I avoided doing this from the start because I didn't know about
EXPORT_SYMBOL_IF_KUNIT and didn't want to make it costly.

Jason

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

[Will Deacon]

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.

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

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

Will

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

[Jason Gunthorpe]

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

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

[Thorsten Leemhuis]

On 07.05.24 15:21, 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.

FWIW, as "CONFIG_KUNIT=m" in case that's relevant for the discussion.
The modules then are put into a separate package.

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

I didn't test this yesterday, as it sounded like the approach would be
revisited. But as that might turn out to not be the case I applied the
patch on today's -next to see if it fixed the issue. That did the trick
afaics (assuming nobody fixed this through other measures!). Hence feel
free to add

Tested-by: Thorsten Leemhuis <linux@leemhuis.info>

Thx Jason for handling this so quickly!

Ciao, Thorsten

Re: [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

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

[Jason Gunthorpe]

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

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

[Will Deacon]

On Wed, May 08, 2024 at 03:04:23PM -0300, Jason Gunthorpe wrote:
> 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:
> > 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

Bah, and it looks like this has its own set of issues anyway:

https://lore.kernel.org/all/20240224121528.1972719-1-arnd@kernel.org/

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

Fair enough. I'll suck it up given that this seems to be the new and
improved way of dealing with this problem. I'm just a bit paranoid about
exporting symbols (even within a namespace), probably thanks to my time
working on Android.

Will

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

[Jason Gunthorpe]

On Thu, May 09, 2024 at 04:23:22PM +0100, Will Deacon wrote:
> On Wed, May 08, 2024 at 03:04:23PM -0300, Jason Gunthorpe wrote:
> > 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:
> > > 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
> 
> Bah, and it looks like this has its own set of issues anyway:
> 
> https://lore.kernel.org/all/20240224121528.1972719-1-arnd@kernel.org/

:\

> > 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.
> 
> Fair enough. I'll suck it up given that this seems to be the new and
> improved way of dealing with this problem. I'm just a bit paranoid about
> exporting symbols (even within a namespace), probably thanks to my time
> working on Android.

Indeed, this is the primary reason I kept Michael's original
integration as non-modular and didn't want to do this when Mostafa
first suggested it.. kunit is new to me as well.

Thanks,
Jason

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

[Will Deacon]

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?

The one-liner Kconfig change didn't surface, so:

Acked-by: Will Deacon <will@kernel.org>

Will

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

[Joerg Roedel]

On Tue, May 07, 2024 at 10:21:10AM -0300, Jason Gunthorpe wrote:
>  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?

Applied, thanks.