[PATCH] vfio: mlx5, pds: add IOMMU_SUPPORT dependency

[PATCH] vfio: mlx5, pds: add IOMMU_SUPPORT dependency

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

Selecting IOMMUFD_DRIVER is not allowed if IOMMUs themselves are not supported:

WARNING: unmet direct dependencies detected for IOMMUFD_DRIVER
  Depends on [n]: IOMMU_SUPPORT [=n]
  Selected by [m]:
  - MLX5_VFIO_PCI [=m] && VFIO [=y] && PCI [=y] && MMU [=y] && MLX5_CORE [=y]

There is no actual build failure, only the warning.

Make the 'select' conditional using the same logic that we have for
INTEL_IOMMU and AMD_IOMMU.

Fixes: 33f6339534287 ("vfio: Move iova_bitmap into iommufd")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/vfio/pci/mlx5/Kconfig | 2 +-
 drivers/vfio/pci/pds/Kconfig  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/vfio/pci/mlx5/Kconfig b/drivers/vfio/pci/mlx5/Kconfig
index c3ced56b77876..c23815c486a75 100644
--- a/drivers/vfio/pci/mlx5/Kconfig
+++ b/drivers/vfio/pci/mlx5/Kconfig
@@ -3,7 +3,7 @@ config MLX5_VFIO_PCI
 	tristate "VFIO support for MLX5 PCI devices"
 	depends on MLX5_CORE
 	select VFIO_PCI_CORE
-	select IOMMUFD_DRIVER
+	select IOMMUFD_DRIVER if IOMMUFD
 	help
 	  This provides migration support for MLX5 devices using the VFIO
 	  framework.
diff --git a/drivers/vfio/pci/pds/Kconfig b/drivers/vfio/pci/pds/Kconfig
index fec9b167c7b9a..6091e11f0e521 100644
--- a/drivers/vfio/pci/pds/Kconfig
+++ b/drivers/vfio/pci/pds/Kconfig
@@ -5,7 +5,7 @@ config PDS_VFIO_PCI
 	tristate "VFIO support for PDS PCI devices"
 	depends on PDS_CORE && PCI_IOV
 	select VFIO_PCI_CORE
-	select IOMMUFD_DRIVER
+	select IOMMUFD_DRIVER if IOMMUFD
 	help
 	  This provides generic PCI support for PDS devices using the VFIO
 	  framework.
-- 
2.39.2

Re: [PATCH] vfio: mlx5, pds: add IOMMU_SUPPORT dependency

[Jason Gunthorpe]

On Mon, Oct 23, 2023 at 01:55:03PM +0200, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> Selecting IOMMUFD_DRIVER is not allowed if IOMMUs themselves are not supported:
> 
> WARNING: unmet direct dependencies detected for IOMMUFD_DRIVER
>   Depends on [n]: IOMMU_SUPPORT [=n]
>   Selected by [m]:
>   - MLX5_VFIO_PCI [=m] && VFIO [=y] && PCI [=y] && MMU [=y] && MLX5_CORE [=y]
> 
> There is no actual build failure, only the warning.
> 
> Make the 'select' conditional using the same logic that we have for
> INTEL_IOMMU and AMD_IOMMU.
> 
> Fixes: 33f6339534287 ("vfio: Move iova_bitmap into iommufd")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/vfio/pci/mlx5/Kconfig | 2 +-
>  drivers/vfio/pci/pds/Kconfig  | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)

But this isn't the logic this wants, it wants to turn on
IOMMUFD_DRIVER if MLX5_VFIO_PCI is turned on.

I think it means IOMMUFD_DRIVER should be lifted out of the
IOMMU_SUPPORT block somehow. I guess just move it into the top of
drivers/iommu/Kconfig?

Jason

Re: [PATCH] vfio: mlx5, pds: add IOMMU_SUPPORT dependency

[Joao Martins]

On 23/10/2023 13:04, Jason Gunthorpe wrote:
> On Mon, Oct 23, 2023 at 01:55:03PM +0200, Arnd Bergmann wrote:
>> From: Arnd Bergmann <arnd@arndb.de>
>>
>> Selecting IOMMUFD_DRIVER is not allowed if IOMMUs themselves are not supported:
>>
>> WARNING: unmet direct dependencies detected for IOMMUFD_DRIVER
>>   Depends on [n]: IOMMU_SUPPORT [=n]
>>   Selected by [m]:
>>   - MLX5_VFIO_PCI [=m] && VFIO [=y] && PCI [=y] && MMU [=y] && MLX5_CORE [=y]
>>
>> There is no actual build failure, only the warning.
>>
>> Make the 'select' conditional using the same logic that we have for
>> INTEL_IOMMU and AMD_IOMMU.
>>
>> Fixes: 33f6339534287 ("vfio: Move iova_bitmap into iommufd")
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> ---
>>  drivers/vfio/pci/mlx5/Kconfig | 2 +-
>>  drivers/vfio/pci/pds/Kconfig  | 2 +-
>>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> But this isn't the logic this wants, it wants to turn on
> IOMMUFD_DRIVER if MLX5_VFIO_PCI is turned on.
> 

Right -- IOMMU drivers need really IOMMUFD (as its usage is driven by IOMMUFD),
whereby vfio pci drivers don't quite need the iommufd support, only the helper
code support, as the vfio UAPI drives VF own dirty tracking.

> I think it means IOMMUFD_DRIVER should be lifted out of the
> IOMMU_SUPPORT block somehow. I guess just move it into the top of
> drivers/iommu/Kconfig?

iommufd Kconfig is only included in the IOMMU_SUPPORT kconfig if clause; so
moving it out from the iommufd kconfig out into iommu kconfig should fix it.
Didn't realize that one can select IOMMU_API yet have IOMMU_SUPPORT unset/unmet.
I'll make the move in v6

	Joao

Re: [PATCH] vfio: mlx5, pds: add IOMMU_SUPPORT dependency

[Arnd Bergmann]

On Mon, Oct 23, 2023, at 14:37, Joao Martins wrote:
> On 23/10/2023 13:04, Jason Gunthorpe wrote:
>> On Mon, Oct 23, 2023 at 01:55:03PM +0200, Arnd Bergmann wrote:
>
> Right -- IOMMU drivers need really IOMMUFD (as its usage is driven by IOMMUFD),
> whereby vfio pci drivers don't quite need the iommufd support, only the helper
> code support, as the vfio UAPI drives VF own dirty tracking.
>
>> I think it means IOMMUFD_DRIVER should be lifted out of the
>> IOMMU_SUPPORT block somehow. I guess just move it into the top of
>> drivers/iommu/Kconfig?
>
> iommufd Kconfig is only included in the IOMMU_SUPPORT kconfig if clause; so
> moving it out from the iommufd kconfig out into iommu kconfig should fix it.
> Didn't realize that one can select IOMMU_API yet have IOMMU_SUPPORT unset/unmet.
> I'll make the move in v6

Are there any useful configurations with IOMMU_API but
not IOMMU_SUPPORT though? My first approach was actually

--- a/drivers/vfio/pci/mlx5/Kconfig
+++ b/drivers/vfio/pci/mlx5/Kconfig
@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0-only
 config MLX5_VFIO_PCI
        tristate "VFIO support for MLX5 PCI devices"
+       depends on IOMMU_SUPPORT
        depends on MLX5_CORE
        select VFIO_PCI_CORE
        select IOMMUFD_DRIVER

before I saw the other construct in the iommu drivers. Maybe that
is the best solution after all.

      Arnd

Re: [PATCH] vfio: mlx5, pds: add IOMMU_SUPPORT dependency

[Jason Gunthorpe]

On Mon, Oct 23, 2023 at 02:55:13PM +0200, Arnd Bergmann wrote:
> On Mon, Oct 23, 2023, at 14:37, Joao Martins wrote:
> > On 23/10/2023 13:04, Jason Gunthorpe wrote:
> >> On Mon, Oct 23, 2023 at 01:55:03PM +0200, Arnd Bergmann wrote:
> >
> > Right -- IOMMU drivers need really IOMMUFD (as its usage is driven by IOMMUFD),
> > whereby vfio pci drivers don't quite need the iommufd support, only the helper
> > code support, as the vfio UAPI drives VF own dirty tracking.
> >
> >> I think it means IOMMUFD_DRIVER should be lifted out of the
> >> IOMMU_SUPPORT block somehow. I guess just move it into the top of
> >> drivers/iommu/Kconfig?
> >
> > iommufd Kconfig is only included in the IOMMU_SUPPORT kconfig if clause; so
> > moving it out from the iommufd kconfig out into iommu kconfig should fix it.
> > Didn't realize that one can select IOMMU_API yet have IOMMU_SUPPORT unset/unmet.
> > I'll make the move in v6
> 
> Are there any useful configurations with IOMMU_API but
> not IOMMU_SUPPORT though? My first approach was actually

IOMMU_SUPPORT is just the menu option in kconfig, it doesn't actually
do anything functional as far as I can tell

But you can have IOMMU_API turned on without IOMMU_SUPPORT still on
power

I think the right thing is to combine IOMMU_SUPPORT and IOMMU_API into
the same thing.

Since VFIO already must depend on IOMMU_API it would be sufficient for
this problem too.

Jason

Re: [PATCH] vfio: mlx5, pds: add IOMMU_SUPPORT dependency

[Arnd Bergmann]

On Mon, Oct 23, 2023, at 15:23, Jason Gunthorpe wrote:
> On Mon, Oct 23, 2023 at 02:55:13PM +0200, Arnd Bergmann wrote:
>> On Mon, Oct 23, 2023, at 14:37, Joao Martins wrote:
>> 
>> Are there any useful configurations with IOMMU_API but
>> not IOMMU_SUPPORT though? My first approach was actually
>
> IOMMU_SUPPORT is just the menu option in kconfig, it doesn't actually
> do anything functional as far as I can tell
>
> But you can have IOMMU_API turned on without IOMMU_SUPPORT still on
> power
>
> I think the right thing is to combine IOMMU_SUPPORT and IOMMU_API into
> the same thing.

I've had a closer look now and I think the way it is currently
designed to be used makes some sense: IOMMU implementations almost
universally depend on both a CPU architecture and CONFIG_IOMMU_SUPPORT,
but select IOMMU_API. So if you enable IOMMU_SUPPORT on an
architecture that has no IOMMU implementations, none of the drivers
are visible and nothing happens. Similarly, almost all drivers
using the IOMMU interface depend on IOMMU_API, so they can only
be built if at least one IOMMU driver is configured.

I made a patch to fix the outliers, which fixes the problem
by ensuring that nothing selects IOMMU_API without also depending
on IOMMU_SUPPORT. Unfortunately that introduces circular dependencies
with CONFIG_VIRTIO, which needs a similar patch to ensure that
only VIRTIO transport providers select it, not virtio drivers
(console, gpu, i2c, caif and fs get this one wrong).

> Since VFIO already must depend on IOMMU_API it would be sufficient for
> this problem too.

Right, having all IOMMU users depend on IOMMU_API certainly makes
sense, though at the moment it uses 'select', which is part of
the problem.

    Arnd


diff --git a/drivers/gpu/drm/msm/Kconfig b/drivers/gpu/drm/msm/Kconfig
index ad70b611b44f0..57ebd7f1a3b29 100644
--- a/drivers/gpu/drm/msm/Kconfig
+++ b/drivers/gpu/drm/msm/Kconfig
@@ -5,7 +5,7 @@ config DRM_MSM
 	depends on DRM
 	depends on ARCH_QCOM || SOC_IMX5 || COMPILE_TEST
 	depends on COMMON_CLK
-	depends on IOMMU_SUPPORT
+	depends on IOMMU_API
 	depends on QCOM_AOSS_QMP || QCOM_AOSS_QMP=n
 	depends on QCOM_OCMEM || QCOM_OCMEM=n
 	depends on QCOM_LLCC || QCOM_LLCC=n
diff --git a/drivers/gpu/drm/nouveau/Kconfig b/drivers/gpu/drm/nouveau/Kconfig
index 4a79704b164f7..2902b89a48f17 100644
--- a/drivers/gpu/drm/nouveau/Kconfig
+++ b/drivers/gpu/drm/nouveau/Kconfig
@@ -4,7 +4,7 @@ config DRM_NOUVEAU
 	depends on DRM && PCI && MMU
 	depends on (ACPI_VIDEO && ACPI_WMI && MXM_WMI) || !(ACPI && X86)
 	depends on BACKLIGHT_CLASS_DEVICE
-	select IOMMU_API
+	depends on IOMMU_API
 	select FW_LOADER
 	select DRM_DISPLAY_DP_HELPER
 	select DRM_DISPLAY_HDMI_HELPER
diff --git a/drivers/gpu/drm/panfrost/Kconfig b/drivers/gpu/drm/panfrost/Kconfig
index e6403a9d66ade..acdcad76d92a8 100644
--- a/drivers/gpu/drm/panfrost/Kconfig
+++ b/drivers/gpu/drm/panfrost/Kconfig
@@ -5,9 +5,9 @@ config DRM_PANFROST
 	depends on DRM
 	depends on ARM || ARM64 || COMPILE_TEST
 	depends on !GENERIC_ATOMIC64    # for IOMMU_IO_PGTABLE_LPAE
+	depends on IOMMU_API
 	depends on MMU
 	select DRM_SCHED
-	select IOMMU_SUPPORT
 	select IOMMU_IO_PGTABLE_LPAE
 	select DRM_GEM_SHMEM_HELPER
 	select PM_DEVFREQ
diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index 3199fd54b462c..be3a8bf42f6ca 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -3,10 +3,6 @@
 config IOMMU_IOVA
 	tristate
 
-# IOMMU_API always gets selected by whoever wants it.
-config IOMMU_API
-	bool
-
 menuconfig IOMMU_SUPPORT
 	bool "IOMMU Hardware Support"
 	depends on MMU
@@ -483,4 +479,8 @@ config SPRD_IOMMU
 
 	  Say Y here if you want to use the multimedia devices listed above.
 
+# IOMMU_API must be selected by any IOMMU provider
+config IOMMU_API
+	bool
+
 endif # IOMMU_SUPPORT
diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig
index 6bda6dbb48784..8c56189c95b38 100644
--- a/drivers/vfio/Kconfig
+++ b/drivers/vfio/Kconfig
@@ -1,8 +1,8 @@
 # SPDX-License-Identifier: GPL-2.0-only
 menuconfig VFIO
 	tristate "VFIO Non-Privileged userspace driver framework"
-	select IOMMU_API
 	depends on IOMMUFD || !IOMMUFD
+	depends on IOMMU_API
 	select INTERVAL_TREE
 	select VFIO_GROUP if SPAPR_TCE_IOMMU || IOMMUFD=n
 	select VFIO_DEVICE_CDEV if !VFIO_GROUP
diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
index d5989871dd5de..9b0eeffc9a3b3 100644
--- a/drivers/xen/Kconfig
+++ b/drivers/xen/Kconfig
@@ -353,6 +353,7 @@ config XEN_GRANT_DMA_OPS
 config XEN_VIRTIO
 	bool "Xen virtio support"
 	depends on VIRTIO
+	depends on IOMMU_SUPPORT
 	select XEN_GRANT_DMA_OPS
 	select XEN_GRANT_DMA_IOMMU if OF
 	help

Re: [PATCH] vfio: mlx5, pds: add IOMMU_SUPPORT dependency

[Jason Gunthorpe]

On Mon, Oct 23, 2023 at 04:02:11PM +0200, Arnd Bergmann wrote:
> On Mon, Oct 23, 2023, at 15:23, Jason Gunthorpe wrote:
> > On Mon, Oct 23, 2023 at 02:55:13PM +0200, Arnd Bergmann wrote:
> >> On Mon, Oct 23, 2023, at 14:37, Joao Martins wrote:
> >> 
> >> Are there any useful configurations with IOMMU_API but
> >> not IOMMU_SUPPORT though? My first approach was actually
> >
> > IOMMU_SUPPORT is just the menu option in kconfig, it doesn't actually
> > do anything functional as far as I can tell
> >
> > But you can have IOMMU_API turned on without IOMMU_SUPPORT still on
> > power
> >
> > I think the right thing is to combine IOMMU_SUPPORT and IOMMU_API into
> > the same thing.
> 
> I've had a closer look now and I think the way it is currently
> designed to be used makes some sense: IOMMU implementations almost
> universally depend on both a CPU architecture and CONFIG_IOMMU_SUPPORT,
> but select IOMMU_API. So if you enable IOMMU_SUPPORT on an
> architecture that has no IOMMU implementations, none of the drivers
> are visible and nothing happens. Similarly, almost all drivers
> using the IOMMU interface depend on IOMMU_API, so they can only
> be built if at least one IOMMU driver is configured.

Maybe, but I don't think we need such micro-optimization.

If someone selects 'enable IOMMU support' and doesn't turn on any
drivers then they should still get the core API. That is how a lot of
the kconfig stuff typically works in the kernel.

Similarly, if they don't select 'enable IOMMU support' then they
definitely shouldn't quitely get the core API turned on!

> diff --git a/drivers/gpu/drm/nouveau/Kconfig b/drivers/gpu/drm/nouveau/Kconfig
> index 4a79704b164f7..2902b89a48f17 100644
> --- a/drivers/gpu/drm/nouveau/Kconfig
> +++ b/drivers/gpu/drm/nouveau/Kconfig
> @@ -4,7 +4,7 @@ config DRM_NOUVEAU
>  	depends on DRM && PCI && MMU
>  	depends on (ACPI_VIDEO && ACPI_WMI && MXM_WMI) || !(ACPI && X86)
>  	depends on BACKLIGHT_CLASS_DEVICE
> -	select IOMMU_API
> +	depends on IOMMU_API
>  	select FW_LOADER
>  	select DRM_DISPLAY_DP_HELPER
>  	select DRM_DISPLAY_HDMI_HELPER

Like here, nouveau should still be compilable even if no iommu driver
was selected, and it should compile on arches without iommu drivers at
all.

Jason

Re: [PATCH] vfio: mlx5, pds: add IOMMU_SUPPORT dependency

[Arnd Bergmann]

On Mon, Oct 23, 2023, at 16:19, Jason Gunthorpe wrote:
> On Mon, Oct 23, 2023 at 04:02:11PM +0200, Arnd Bergmann wrote:
>> On Mon, Oct 23, 2023, at 15:23, Jason Gunthorpe wrote:
>> >
>> > I think the right thing is to combine IOMMU_SUPPORT and IOMMU_API into
>> > the same thing.
>> 
>> I've had a closer look now and I think the way it is currently
>> designed to be used makes some sense: IOMMU implementations almost
>> universally depend on both a CPU architecture and CONFIG_IOMMU_SUPPORT,
>> but select IOMMU_API. So if you enable IOMMU_SUPPORT on an
>> architecture that has no IOMMU implementations, none of the drivers
>> are visible and nothing happens. Similarly, almost all drivers
>> using the IOMMU interface depend on IOMMU_API, so they can only
>> be built if at least one IOMMU driver is configured.
>
> Maybe, but I don't think we need such micro-optimization.
>
> If someone selects 'enable IOMMU support' and doesn't turn on any
> drivers then they should still get the core API. That is how a lot of
> the kconfig stuff typically works in the kernel.

Agreed, that would be fine as well, and I agree it is less confusing.
A similar approach as in iommu is used in a couple of other subsystems
(regmap, gpiolib, sound, mfd, phy, virtio) where each provider selects
the subsystem as a library, but I'm not a huge fan of this either.
It's usually just easier to not make fundamental changes like this.

In this case, we can just use 'depends on' for one of the two
symbols everywhere and use to control both the providers and
consumers.

> Similarly, if they don't select 'enable IOMMU support' then they
> definitely shouldn't quitely get the core API turned on!
...
>> diff --git a/drivers/gpu/drm/nouveau/Kconfig b/drivers/gpu/drm/nouveau/Kconfig
>> index 4a79704b164f7..2902b89a48f17 100644
>> --- a/drivers/gpu/drm/nouveau/Kconfig
>> +++ b/drivers/gpu/drm/nouveau/Kconfig
>> @@ -4,7 +4,7 @@ config DRM_NOUVEAU
>>  	depends on DRM && PCI && MMU
>>  	depends on (ACPI_VIDEO && ACPI_WMI && MXM_WMI) || !(ACPI && X86)
>>  	depends on BACKLIGHT_CLASS_DEVICE
>> -	select IOMMU_API
>> +	depends on IOMMU_API
>>  	select FW_LOADER
>>  	select DRM_DISPLAY_DP_HELPER
>>  	select DRM_DISPLAY_HDMI_HELPER
>
> Like here, nouveau should still be compilable even if no iommu driver
> was selected, and it should compile on arches without iommu drivers at
> all.

Right, so with my draft patch, we can't build nouveau without
having an IOMMU driver, which matches the original intention
behind the Kconfig logic, while your suggestion would add the
same dependency here but still allow it to be compile tested
on target systems with no IOMMU. A minor downside of your
approach is that you end up building drivers (without
CONFIG_COMPILE_TEST) that currently exclude because we know
they will never work.

     Arnd

Re: [PATCH] vfio: mlx5, pds: add IOMMU_SUPPORT dependency

[Jason Gunthorpe]

On Mon, Oct 23, 2023 at 04:35:15PM +0200, Arnd Bergmann wrote:
> >> diff --git a/drivers/gpu/drm/nouveau/Kconfig b/drivers/gpu/drm/nouveau/Kconfig
> >> index 4a79704b164f7..2902b89a48f17 100644
> >> --- a/drivers/gpu/drm/nouveau/Kconfig
> >> +++ b/drivers/gpu/drm/nouveau/Kconfig
> >> @@ -4,7 +4,7 @@ config DRM_NOUVEAU
> >>  	depends on DRM && PCI && MMU
> >>  	depends on (ACPI_VIDEO && ACPI_WMI && MXM_WMI) || !(ACPI && X86)
> >>  	depends on BACKLIGHT_CLASS_DEVICE
> >> -	select IOMMU_API
> >> +	depends on IOMMU_API
> >>  	select FW_LOADER
> >>  	select DRM_DISPLAY_DP_HELPER
> >>  	select DRM_DISPLAY_HDMI_HELPER
> >
> > Like here, nouveau should still be compilable even if no iommu driver
> > was selected, and it should compile on arches without iommu drivers at
> > all.
> 
> Right, so with my draft patch, we can't build nouveau without
> having an IOMMU driver, which matches the original intention
> behind the Kconfig logic, while your suggestion would add the
> same dependency here but still allow it to be compile tested
> on target systems with no IOMMU. A minor downside of your
> approach is that you end up building drivers (without
> CONFIG_COMPILE_TEST) that currently exclude because we know
> they will never work.

I wonder how true that is, even nouveau only seems to have this for
some tegra specific situation. The driver broadly does work without an
iommu. (even weirder that already seems to have IS_ENABLED so I don't
know what this is for)

I'd prefer clarity over these kinds of optimizations..

Jason

Re: [PATCH] vfio: mlx5, pds: add IOMMU_SUPPORT dependency

[Arnd Bergmann]

On Mon, Oct 23, 2023, at 16:43, Jason Gunthorpe wrote:
> On Mon, Oct 23, 2023 at 04:35:15PM +0200, Arnd Bergmann wrote:
>> >> diff --git a/drivers/gpu/drm/nouveau/Kconfig b/drivers/gpu/drm/nouveau/Kconfig
>> >> index 4a79704b164f7..2902b89a48f17 100644
>> >> --- a/drivers/gpu/drm/nouveau/Kconfig
>> >> +++ b/drivers/gpu/drm/nouveau/Kconfig
>> >> @@ -4,7 +4,7 @@ config DRM_NOUVEAU
>> >>  	depends on DRM && PCI && MMU
>> >>  	depends on (ACPI_VIDEO && ACPI_WMI && MXM_WMI) || !(ACPI && X86)
>> >>  	depends on BACKLIGHT_CLASS_DEVICE
>> >> -	select IOMMU_API
>> >> +	depends on IOMMU_API
>> >>  	select FW_LOADER
>> >>  	select DRM_DISPLAY_DP_HELPER
>> >>  	select DRM_DISPLAY_HDMI_HELPER
>> >
>> > Like here, nouveau should still be compilable even if no iommu driver
>> > was selected, and it should compile on arches without iommu drivers at
>> > all.
>> 
>> Right, so with my draft patch, we can't build nouveau without
>> having an IOMMU driver, which matches the original intention
>> behind the Kconfig logic, while your suggestion would add the
>> same dependency here but still allow it to be compile tested
>> on target systems with no IOMMU. A minor downside of your
>> approach is that you end up building drivers (without
>> CONFIG_COMPILE_TEST) that currently exclude because we know
>> they will never work.
>
> I wonder how true that is, even nouveau only seems to have this for
> some tegra specific situation. The driver broadly does work without an
> iommu. (even weirder that already seems to have IS_ENABLED so I don't
> know what this is for)
>
> I'd prefer clarity over these kinds of optimizations..

It does look like a mistake in ee8642162a9e ("drm/nouveau: fix build
error without CONFIG_IOMMU_API"), which was done in response
to a compile-time failure that was also addressed in commit 
0008d0c3b1ab ("iommu: Define dev_iommu_fwspec_get() for
!CONFIG_IOMMU_API") in a different way that made it possible to
use NOUVEAU without the IOMMU API again.

For drm/panfrost and drm/msm the dependency on IOMMU_SUPPORT
is different, as those apparently just want to select
CONFIG_IOMMU_IO_PGTABLE but can build without IOMMU_API.
This can also be handled in different ways, but it's a
separate problem.

     Arnd

Re: [PATCH] vfio: mlx5, pds: add IOMMU_SUPPORT dependency

[Jason Gunthorpe]

On Mon, Oct 23, 2023 at 01:37:28PM +0100, Joao Martins wrote:

> iommufd Kconfig is only included in the IOMMU_SUPPORT kconfig if clause; so
> moving it out from the iommufd kconfig out into iommu kconfig should fix it.
> Didn't realize that one can select IOMMU_API yet have IOMMU_SUPPORT unset/unmet.
> I'll make the move in v6

I think this is some cruft that accumulated over the years, it doesn't
make alot of sense that there are two kconfigs now..

Jason

Re: [PATCH] vfio: mlx5, pds: add IOMMU_SUPPORT dependency

[Joao Martins]

On 23/10/2023 14:12, Jason Gunthorpe wrote:
> On Mon, Oct 23, 2023 at 01:37:28PM +0100, Joao Martins wrote:
> 
>> iommufd Kconfig is only included in the IOMMU_SUPPORT kconfig if clause; so
>> moving it out from the iommufd kconfig out into iommu kconfig should fix it.
>> Didn't realize that one can select IOMMU_API yet have IOMMU_SUPPORT unset/unmet.
>> I'll make the move in v6
> 
> I think this is some cruft that accumulated over the years, it doesn't
> make alot of sense that there are two kconfigs now..

To be specific what I meant to move is the IOMMUFD_DRIVER kconfig part, not the
whole iommufd Kconfig [in the patch introducing the problem] e.g.

diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index 2b12b583ef4b..5cc869db1b79 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -7,6 +7,10 @@ config IOMMU_IOVA
 config IOMMU_API
        bool

+config IOMMUFD_DRIVER
+       bool
+       default n
+
 menuconfig IOMMU_SUPPORT
        bool "IOMMU Hardware Support"
        depends on MMU
diff --git a/drivers/iommu/iommufd/Kconfig b/drivers/iommu/iommufd/Kconfig
index 1fa543204e89..99d4b075df49 100644
--- a/drivers/iommu/iommufd/Kconfig
+++ b/drivers/iommu/iommufd/Kconfig
@@ -11,10 +11,6 @@ config IOMMUFD

          If you don't know what to do here, say N.

-config IOMMUFD_DRIVER
-       bool
-       default n
-
 if IOMMUFD
 config IOMMUFD_VFIO_CONTAINER
        bool "IOMMUFD provides the VFIO container /dev/vfio/vfio"

(...) or in alternative, do similar to this patch except that it's:

	select IOMMUFD_DRIVER if IOMMU_SUPPORT

In the mlx5/pds vfio drivers.

Perhaps the merging of IOMMU_API with IOMMU_SUPPORT should be best done separately?

	Joao

Re: [PATCH] vfio: mlx5, pds: add IOMMU_SUPPORT dependency

[Jason Gunthorpe]

On Mon, Oct 23, 2023 at 06:50:05PM +0100, Joao Martins wrote:
> On 23/10/2023 14:12, Jason Gunthorpe wrote:
> > On Mon, Oct 23, 2023 at 01:37:28PM +0100, Joao Martins wrote:
> > 
> >> iommufd Kconfig is only included in the IOMMU_SUPPORT kconfig if clause; so
> >> moving it out from the iommufd kconfig out into iommu kconfig should fix it.
> >> Didn't realize that one can select IOMMU_API yet have IOMMU_SUPPORT unset/unmet.
> >> I'll make the move in v6
> > 
> > I think this is some cruft that accumulated over the years, it doesn't
> > make alot of sense that there are two kconfigs now..
> 
> To be specific what I meant to move is the IOMMUFD_DRIVER kconfig part, not the
> whole iommufd Kconfig [in the patch introducing the problem] e.g.

yes

> Perhaps the merging of IOMMU_API with IOMMU_SUPPORT should be best done separately?

Yes

Jason

Re: [PATCH] vfio: mlx5, pds: add IOMMU_SUPPORT dependency

[Arnd Bergmann]

On Mon, Oct 23, 2023, at 19:50, Joao Martins wrote:
> On 23/10/2023 14:12, Jason Gunthorpe wrote:
>> On Mon, Oct 23, 2023 at 01:37:28PM +0100, Joao Martins wrote:
>
> To be specific what I meant to move is the IOMMUFD_DRIVER kconfig part, not the
> whole iommufd Kconfig [in the patch introducing the problem] e.g.
>
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index 2b12b583ef4b..5cc869db1b79 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -7,6 +7,10 @@ config IOMMU_IOVA
>  config IOMMU_API
>         bool
>
> +config IOMMUFD_DRIVER
> +       bool
> +       default n
> +
>  menuconfig IOMMU_SUPPORT
>         bool "IOMMU Hardware Support"
>         depends on MMU
> diff --git a/drivers/iommu/iommufd/Kconfig b/drivers/iommu/iommufd/Kconfig
> index 1fa543204e89..99d4b075df49 100644
> --- a/drivers/iommu/iommufd/Kconfig
> +++ b/drivers/iommu/iommufd/Kconfig
> @@ -11,10 +11,6 @@ config IOMMUFD
>
>           If you don't know what to do here, say N.
>
> -config IOMMUFD_DRIVER
> -       bool
> -       default n
> -
>  if IOMMUFD
>  config IOMMUFD_VFIO_CONTAINER
>         bool "IOMMUFD provides the VFIO container /dev/vfio/vfio"
>
> (...) or in alternative, do similar to this patch except that it's:
>
> 	select IOMMUFD_DRIVER if IOMMU_SUPPORT
>
> In the mlx5/pds vfio drivers.

If I understand it right, we have two providers (AMD and
Intel iommu) and two consumers (mlx5 and pds) for this
interface, so we probably don't want to use 'select' for
both sides here.

As with CONFIG_IOMMU_API, two two logical options are
to either have a hidden symbol selected by the providers
that the consumers depend on, or have a user-visible
symbol and use 'depends on IOMMUFD_DRIVER' for both
the providers and the consumers.

Either way, I think the problem with the warning goes
away.

> Perhaps the merging of IOMMU_API with IOMMU_SUPPORT should
> be best done separately?

Right, that part should be improved as well, but it's
not causing other problems at the moment.

     Arnd

Re: [PATCH] vfio: mlx5, pds: add IOMMU_SUPPORT dependency

[Joao Martins]

On 23/10/2023 19:46, Arnd Bergmann wrote:
> On Mon, Oct 23, 2023, at 19:50, Joao Martins wrote:
>> On 23/10/2023 14:12, Jason Gunthorpe wrote:
>>> On Mon, Oct 23, 2023 at 01:37:28PM +0100, Joao Martins wrote:
>>
>> To be specific what I meant to move is the IOMMUFD_DRIVER kconfig part, not the
>> whole iommufd Kconfig [in the patch introducing the problem] e.g.
>>
>> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
>> index 2b12b583ef4b..5cc869db1b79 100644
>> --- a/drivers/iommu/Kconfig
>> +++ b/drivers/iommu/Kconfig
>> @@ -7,6 +7,10 @@ config IOMMU_IOVA
>>  config IOMMU_API
>>         bool
>>
>> +config IOMMUFD_DRIVER
>> +       bool
>> +       default n
>> +
>>  menuconfig IOMMU_SUPPORT
>>         bool "IOMMU Hardware Support"
>>         depends on MMU
>> diff --git a/drivers/iommu/iommufd/Kconfig b/drivers/iommu/iommufd/Kconfig
>> index 1fa543204e89..99d4b075df49 100644
>> --- a/drivers/iommu/iommufd/Kconfig
>> +++ b/drivers/iommu/iommufd/Kconfig
>> @@ -11,10 +11,6 @@ config IOMMUFD
>>
>>           If you don't know what to do here, say N.
>>
>> -config IOMMUFD_DRIVER
>> -       bool
>> -       default n
>> -
>>  if IOMMUFD
>>  config IOMMUFD_VFIO_CONTAINER
>>         bool "IOMMUFD provides the VFIO container /dev/vfio/vfio"
>>
>> (...) or in alternative, do similar to this patch except that it's:
>>
>> 	select IOMMUFD_DRIVER if IOMMU_SUPPORT
>>
>> In the mlx5/pds vfio drivers.
> 
> If I understand it right, we have two providers (AMD and
> Intel iommu) and two consumers (mlx5 and pds) for this
> interface, so we probably don't want to use 'select' for
> both sides here.
> 
It's not quite one consumes the other.

IOMMU drivers use iova-bitmap, and are providers of the IOMMU support to IOMMUFD
usage.

The mlx5/pds (and VFIO too if either is enabled) consume iova-bitmap (thus
select IOMMUFD_DRIVER where the code is now being moved under) but they aren't
tied to the IOMMU support of it.

This is what we are trying to capture in the kconfig and thus structured it
this way.

> As with CONFIG_IOMMU_API, two two logical options are
> to either have a hidden symbol selected by the providers
> that the consumers depend on, or have a user-visible
> symbol and use 'depends on IOMMUFD_DRIVER' for both
> the providers and the consumers.
> 
> Either way, I think the problem with the warning goes
> away.
> 
>> Perhaps the merging of IOMMU_API with IOMMU_SUPPORT should
>> be best done separately?
> 
> Right, that part should be improved as well, but it's
> not causing other problems at the moment.
> 
>      Arnd