| Message ID | 20231023115520.3530120-1-arnd@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio: mlx5, pds: add IOMMU_SUPPORT dependency | expand |
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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.