diff mbox series

[1/3] vfio/pci: Cleanup Kconfig

Message ID 20230602213315.2521442-2-alex.williamson@redhat.com (mailing list archive)
State New, archived
Headers show
Series vfio: Cleanup Kconfigs | expand

Commit Message

Alex Williamson June 2, 2023, 9:33 p.m. UTC
It should be possible to select vfio-pci variant drivers without building
vfio-pci itself, which implies each variant driver should select
vfio-pci-core.

Fix the top level vfio Makefile to traverse pci based on vfio-pci-core
rather than vfio-pci.

Mark MMAP and INTX options depending on vfio-pci-core to cleanup resulting
config if core is not enabled.

Push all PCI related vfio options to a sub-menu and make descriptions
consistent.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 drivers/vfio/Makefile              | 2 +-
 drivers/vfio/pci/Kconfig           | 8 ++++++--
 drivers/vfio/pci/hisilicon/Kconfig | 4 ++--
 drivers/vfio/pci/mlx5/Kconfig      | 2 +-
 4 files changed, 10 insertions(+), 6 deletions(-)

Comments

Cédric Le Goater June 5, 2023, 9:21 a.m. UTC | #1
On 6/2/23 23:33, Alex Williamson wrote:
> It should be possible to select vfio-pci variant drivers without building
> vfio-pci itself, which implies each variant driver should select
> vfio-pci-core.
> 
> Fix the top level vfio Makefile to traverse pci based on vfio-pci-core
> rather than vfio-pci.
> 
> Mark MMAP and INTX options depending on vfio-pci-core to cleanup resulting
> config if core is not enabled.
> 
> Push all PCI related vfio options to a sub-menu and make descriptions
> consistent.
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>

Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.

> ---
>   drivers/vfio/Makefile              | 2 +-
>   drivers/vfio/pci/Kconfig           | 8 ++++++--
>   drivers/vfio/pci/hisilicon/Kconfig | 4 ++--
>   drivers/vfio/pci/mlx5/Kconfig      | 2 +-
>   4 files changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/vfio/Makefile b/drivers/vfio/Makefile
> index 70e7dcb302ef..151e816b2ff9 100644
> --- a/drivers/vfio/Makefile
> +++ b/drivers/vfio/Makefile
> @@ -10,7 +10,7 @@ vfio-$(CONFIG_VFIO_VIRQFD) += virqfd.o
>   
>   obj-$(CONFIG_VFIO_IOMMU_TYPE1) += vfio_iommu_type1.o
>   obj-$(CONFIG_VFIO_IOMMU_SPAPR_TCE) += vfio_iommu_spapr_tce.o
> -obj-$(CONFIG_VFIO_PCI) += pci/
> +obj-$(CONFIG_VFIO_PCI_CORE) += pci/
>   obj-$(CONFIG_VFIO_PLATFORM) += platform/
>   obj-$(CONFIG_VFIO_MDEV) += mdev/
>   obj-$(CONFIG_VFIO_FSL_MC) += fsl-mc/
> diff --git a/drivers/vfio/pci/Kconfig b/drivers/vfio/pci/Kconfig
> index f9d0c908e738..86bb7835cf3c 100644
> --- a/drivers/vfio/pci/Kconfig
> +++ b/drivers/vfio/pci/Kconfig
> @@ -1,5 +1,7 @@
>   # SPDX-License-Identifier: GPL-2.0-only
> -if PCI && MMU
> +menu "VFIO support for PCI devices"
> +	depends on PCI && MMU
> +
>   config VFIO_PCI_CORE
>   	tristate
>   	select VFIO_VIRQFD
> @@ -7,9 +9,11 @@ config VFIO_PCI_CORE
>   
>   config VFIO_PCI_MMAP
>   	def_bool y if !S390
> +	depends on VFIO_PCI_CORE
>   
>   config VFIO_PCI_INTX
>   	def_bool y if !S390
> +	depends on VFIO_PCI_CORE
>   
>   config VFIO_PCI
>   	tristate "Generic VFIO support for any PCI device"
> @@ -59,4 +63,4 @@ source "drivers/vfio/pci/mlx5/Kconfig"
>   
>   source "drivers/vfio/pci/hisilicon/Kconfig"
>   
> -endif
> +endmenu
> diff --git a/drivers/vfio/pci/hisilicon/Kconfig b/drivers/vfio/pci/hisilicon/Kconfig
> index 5daa0f45d2f9..cbf1c32f6ebf 100644
> --- a/drivers/vfio/pci/hisilicon/Kconfig
> +++ b/drivers/vfio/pci/hisilicon/Kconfig
> @@ -1,13 +1,13 @@
>   # SPDX-License-Identifier: GPL-2.0-only
>   config HISI_ACC_VFIO_PCI
> -	tristate "VFIO PCI support for HiSilicon ACC devices"
> +	tristate "VFIO support for HiSilicon ACC PCI devices"
>   	depends on ARM64 || (COMPILE_TEST && 64BIT)
> -	depends on VFIO_PCI_CORE
>   	depends on PCI_MSI
>   	depends on CRYPTO_DEV_HISI_QM
>   	depends on CRYPTO_DEV_HISI_HPRE
>   	depends on CRYPTO_DEV_HISI_SEC2
>   	depends on CRYPTO_DEV_HISI_ZIP
> +	select VFIO_PCI_CORE
>   	help
>   	  This provides generic PCI support for HiSilicon ACC devices
>   	  using the VFIO framework.
> diff --git a/drivers/vfio/pci/mlx5/Kconfig b/drivers/vfio/pci/mlx5/Kconfig
> index 29ba9c504a75..7088edc4fb28 100644
> --- a/drivers/vfio/pci/mlx5/Kconfig
> +++ b/drivers/vfio/pci/mlx5/Kconfig
> @@ -2,7 +2,7 @@
>   config MLX5_VFIO_PCI
>   	tristate "VFIO support for MLX5 PCI devices"
>   	depends on MLX5_CORE
> -	depends on VFIO_PCI_CORE
> +	select VFIO_PCI_CORE
>   	help
>   	  This provides migration support for MLX5 devices using the VFIO
>   	  framework.
Jason Gunthorpe June 5, 2023, 5:01 p.m. UTC | #2
On Fri, Jun 02, 2023 at 03:33:13PM -0600, Alex Williamson wrote:
> diff --git a/drivers/vfio/Makefile b/drivers/vfio/Makefile
> index 70e7dcb302ef..151e816b2ff9 100644
> --- a/drivers/vfio/Makefile
> +++ b/drivers/vfio/Makefile
> @@ -10,7 +10,7 @@ vfio-$(CONFIG_VFIO_VIRQFD) += virqfd.o
>  
>  obj-$(CONFIG_VFIO_IOMMU_TYPE1) += vfio_iommu_type1.o
>  obj-$(CONFIG_VFIO_IOMMU_SPAPR_TCE) += vfio_iommu_spapr_tce.o
> -obj-$(CONFIG_VFIO_PCI) += pci/
> +obj-$(CONFIG_VFIO_PCI_CORE) += pci/
>  obj-$(CONFIG_VFIO_PLATFORM) += platform/
>  obj-$(CONFIG_VFIO_MDEV) += mdev/
>  obj-$(CONFIG_VFIO_FSL_MC) += fsl-mc/

This makes sense on its own even today

> diff --git a/drivers/vfio/pci/Kconfig b/drivers/vfio/pci/Kconfig
> index f9d0c908e738..86bb7835cf3c 100644
> --- a/drivers/vfio/pci/Kconfig
> +++ b/drivers/vfio/pci/Kconfig
> @@ -1,5 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0-only
> -if PCI && MMU
> +menu "VFIO support for PCI devices"
> +	depends on PCI && MMU


I still think this is excessive, it is normal to hang the makefile
components off the kconfig for the "core". Even VFIO is already doing this:

menuconfig VFIO
        tristate "VFIO Non-Privileged userspace driver framework"
        select IOMMU_API
        depends on IOMMUFD || !IOMMUFD
        select INTERVAL_TREE
        select VFIO_CONTAINER if IOMMUFD=n

[..]

obj-$(CONFIG_VFIO) += vfio.o

Jason
Alex Williamson June 5, 2023, 7:25 p.m. UTC | #3
On Mon, 5 Jun 2023 14:01:28 -0300
Jason Gunthorpe <jgg@nvidia.com> wrote:

> On Fri, Jun 02, 2023 at 03:33:13PM -0600, Alex Williamson wrote:
> > diff --git a/drivers/vfio/Makefile b/drivers/vfio/Makefile
> > index 70e7dcb302ef..151e816b2ff9 100644
> > --- a/drivers/vfio/Makefile
> > +++ b/drivers/vfio/Makefile
> > @@ -10,7 +10,7 @@ vfio-$(CONFIG_VFIO_VIRQFD) += virqfd.o
> >  
> >  obj-$(CONFIG_VFIO_IOMMU_TYPE1) += vfio_iommu_type1.o
> >  obj-$(CONFIG_VFIO_IOMMU_SPAPR_TCE) += vfio_iommu_spapr_tce.o
> > -obj-$(CONFIG_VFIO_PCI) += pci/
> > +obj-$(CONFIG_VFIO_PCI_CORE) += pci/
> >  obj-$(CONFIG_VFIO_PLATFORM) += platform/
> >  obj-$(CONFIG_VFIO_MDEV) += mdev/
> >  obj-$(CONFIG_VFIO_FSL_MC) += fsl-mc/  
> 
> This makes sense on its own even today

It's only an academic fix today, currently nothing in pci/ can be
selected without VFIO_PCI.

> > diff --git a/drivers/vfio/pci/Kconfig b/drivers/vfio/pci/Kconfig
> > index f9d0c908e738..86bb7835cf3c 100644
> > --- a/drivers/vfio/pci/Kconfig
> > +++ b/drivers/vfio/pci/Kconfig
> > @@ -1,5 +1,7 @@
> >  # SPDX-License-Identifier: GPL-2.0-only
> > -if PCI && MMU
> > +menu "VFIO support for PCI devices"
> > +	depends on PCI && MMU  
> 
> 
> I still think this is excessive, it is normal to hang the makefile
> components off the kconfig for the "core". Even VFIO is already doing this:
> 
> menuconfig VFIO
>         tristate "VFIO Non-Privileged userspace driver framework"
>         select IOMMU_API
>         depends on IOMMUFD || !IOMMUFD
>         select INTERVAL_TREE
>         select VFIO_CONTAINER if IOMMUFD=n
> 
> [..]
> 
> obj-$(CONFIG_VFIO) += vfio.o

I think the "core" usually does something on its own though without
out-of-tree drivers, so I don't see this as an example of how things
should work as much as it is another target for improvement.

Ideally I think we'd still have a top level menuconfig, but it should
look more like VIRT_DRIVERS, which just enables Makefile traversal and
unhides menu options.  It should be things like VFIO_PCI_CORE or
VFIO_MDEV that actually select VFIO.  We shouldn't build vfio.ko if
there's nothing in-kernel that uses it, nor should we burden the user
with Kconfig options for other intermediate helper modules.  I see the
top level menuconfig as necessary to de-clutter the config, but ideally
users should be selecting config options based on actual functionality,
not just config options to enable other config options.

It looks like there are some non-trivial dependency loops that need to
be resolved if we hide VFIO and make is selected by other modules, so I
don't know that I'll be able to expand this series to include that
right now.  Thanks,

Alex
Jason Gunthorpe June 6, 2023, 2:25 p.m. UTC | #4
On Mon, Jun 05, 2023 at 01:25:18PM -0600, Alex Williamson wrote:
> On Mon, 5 Jun 2023 14:01:28 -0300
> Jason Gunthorpe <jgg@nvidia.com> wrote:
> 
> > On Fri, Jun 02, 2023 at 03:33:13PM -0600, Alex Williamson wrote:
> > > diff --git a/drivers/vfio/Makefile b/drivers/vfio/Makefile
> > > index 70e7dcb302ef..151e816b2ff9 100644
> > > --- a/drivers/vfio/Makefile
> > > +++ b/drivers/vfio/Makefile
> > > @@ -10,7 +10,7 @@ vfio-$(CONFIG_VFIO_VIRQFD) += virqfd.o
> > >  
> > >  obj-$(CONFIG_VFIO_IOMMU_TYPE1) += vfio_iommu_type1.o
> > >  obj-$(CONFIG_VFIO_IOMMU_SPAPR_TCE) += vfio_iommu_spapr_tce.o
> > > -obj-$(CONFIG_VFIO_PCI) += pci/
> > > +obj-$(CONFIG_VFIO_PCI_CORE) += pci/
> > >  obj-$(CONFIG_VFIO_PLATFORM) += platform/
> > >  obj-$(CONFIG_VFIO_MDEV) += mdev/
> > >  obj-$(CONFIG_VFIO_FSL_MC) += fsl-mc/  
> > 
> > This makes sense on its own even today
> 
> It's only an academic fix today, currently nothing in pci/ can be
> selected without VFIO_PCI.
> 
> > > diff --git a/drivers/vfio/pci/Kconfig b/drivers/vfio/pci/Kconfig
> > > index f9d0c908e738..86bb7835cf3c 100644
> > > --- a/drivers/vfio/pci/Kconfig
> > > +++ b/drivers/vfio/pci/Kconfig
> > > @@ -1,5 +1,7 @@
> > >  # SPDX-License-Identifier: GPL-2.0-only
> > > -if PCI && MMU
> > > +menu "VFIO support for PCI devices"
> > > +	depends on PCI && MMU  
> > 
> > 
> > I still think this is excessive, it is normal to hang the makefile
> > components off the kconfig for the "core". Even VFIO is already doing this:
> > 
> > menuconfig VFIO
> >         tristate "VFIO Non-Privileged userspace driver framework"
> >         select IOMMU_API
> >         depends on IOMMUFD || !IOMMUFD
> >         select INTERVAL_TREE
> >         select VFIO_CONTAINER if IOMMUFD=n
> > 
> > [..]
> > 
> > obj-$(CONFIG_VFIO) += vfio.o
> 
> I think the "core" usually does something on its own though without
> out-of-tree drivers,

Not really, maybe it creates a sysfs class, but it certainly doesn't
do anything useful unless there is a vfio driver also selected.

> so I don't see this as an example of how things
> should work as much as it is another target for improvement.

It is the common pattern in the kernel, I'm not sure where you are
getting this "improvement" idea from.

> Ideally I think we'd still have a top level menuconfig, but it should
> look more like VIRT_DRIVERS, which just enables Makefile traversal and
> unhides menu options.  It should be things like VFIO_PCI_CORE or
> VFIO_MDEV that actually select VFIO.  

There are many ways to use kconfig, but I think this is not typical
usage and becomes over complicated to solve an unimportant problem.

The menu configs follow the makefiles which is nice and simple to
understand and implement.

Jason
Alex Williamson June 6, 2023, 9:57 p.m. UTC | #5
On Tue, 6 Jun 2023 11:25:01 -0300
Jason Gunthorpe <jgg@nvidia.com> wrote:

> On Mon, Jun 05, 2023 at 01:25:18PM -0600, Alex Williamson wrote:
> > On Mon, 5 Jun 2023 14:01:28 -0300
> > Jason Gunthorpe <jgg@nvidia.com> wrote:
> >   
> > > On Fri, Jun 02, 2023 at 03:33:13PM -0600, Alex Williamson wrote:  
> > > > diff --git a/drivers/vfio/Makefile b/drivers/vfio/Makefile
> > > > index 70e7dcb302ef..151e816b2ff9 100644
> > > > --- a/drivers/vfio/Makefile
> > > > +++ b/drivers/vfio/Makefile
> > > > @@ -10,7 +10,7 @@ vfio-$(CONFIG_VFIO_VIRQFD) += virqfd.o
> > > >  
> > > >  obj-$(CONFIG_VFIO_IOMMU_TYPE1) += vfio_iommu_type1.o
> > > >  obj-$(CONFIG_VFIO_IOMMU_SPAPR_TCE) += vfio_iommu_spapr_tce.o
> > > > -obj-$(CONFIG_VFIO_PCI) += pci/
> > > > +obj-$(CONFIG_VFIO_PCI_CORE) += pci/
> > > >  obj-$(CONFIG_VFIO_PLATFORM) += platform/
> > > >  obj-$(CONFIG_VFIO_MDEV) += mdev/
> > > >  obj-$(CONFIG_VFIO_FSL_MC) += fsl-mc/    
> > > 
> > > This makes sense on its own even today  
> > 
> > It's only an academic fix today, currently nothing in pci/ can be
> > selected without VFIO_PCI.
> >   
> > > > diff --git a/drivers/vfio/pci/Kconfig b/drivers/vfio/pci/Kconfig
> > > > index f9d0c908e738..86bb7835cf3c 100644
> > > > --- a/drivers/vfio/pci/Kconfig
> > > > +++ b/drivers/vfio/pci/Kconfig
> > > > @@ -1,5 +1,7 @@
> > > >  # SPDX-License-Identifier: GPL-2.0-only
> > > > -if PCI && MMU
> > > > +menu "VFIO support for PCI devices"
> > > > +	depends on PCI && MMU    
> > > 
> > > 
> > > I still think this is excessive, it is normal to hang the makefile
> > > components off the kconfig for the "core". Even VFIO is already doing this:
> > > 
> > > menuconfig VFIO
> > >         tristate "VFIO Non-Privileged userspace driver framework"
> > >         select IOMMU_API
> > >         depends on IOMMUFD || !IOMMUFD
> > >         select INTERVAL_TREE
> > >         select VFIO_CONTAINER if IOMMUFD=n
> > > 
> > > [..]
> > > 
> > > obj-$(CONFIG_VFIO) += vfio.o  
> > 
> > I think the "core" usually does something on its own though without
> > out-of-tree drivers,  
> 
> Not really, maybe it creates a sysfs class, but it certainly doesn't
> do anything useful unless there is a vfio driver also selected.

Sorry, I wasn't referring to vfio "core" here, I was thinking more
along the lines of when we include the PCI or IOMMU subsystem there's
a degree of base functionality included there regardless of what
additional options or drivers are selected.  OTOH, if we enable
CONFIG_VFIO without any in-kernel drivers for it, it's simply a waste of
space.

> > so I don't see this as an example of how things
> > should work as much as it is another target for improvement.  
> 
> It is the common pattern in the kernel, I'm not sure where you are
> getting this "improvement" idea from.

Common practice or not, configurations that build and install a module
that has no possibility of an in-kernel user is a waste of time and
space, which leaves room for improvement.
 
> > Ideally I think we'd still have a top level menuconfig, but it should
> > look more like VIRT_DRIVERS, which just enables Makefile traversal and
> > unhides menu options.  It should be things like VFIO_PCI_CORE or
> > VFIO_MDEV that actually select VFIO.    
> 
> There are many ways to use kconfig, but I think this is not typical
> usage and becomes over complicated to solve an unimportant problem.
> 
> The menu configs follow the makefiles which is nice and simple to
> understand and implement.

But leaves open the possibility of building and installing modules that
have no users, which therefore make them open for improvement.  I don't
see anything overly complicated in this series.  We certainly have more
important topics to quibble about than a select or depend, but here we
are.

The current state is that we cannot build vfio-pci-core.ko without
vfio-pci.ko, so there's always an in-kernel user.  The proposal which
allows building vfio-pci-core.ko w/o any in-kernel users is therefore a
regression (imo) prompting this alternative.  CONFIG_VFIO is a separate
pre-existing issue.  Thanks,

Alex
Jason Gunthorpe June 6, 2023, 11:27 p.m. UTC | #6
On Tue, Jun 06, 2023 at 03:57:04PM -0600, Alex Williamson wrote:
> > Not really, maybe it creates a sysfs class, but it certainly doesn't
> > do anything useful unless there is a vfio driver also selected.
> 
> Sorry, I wasn't referring to vfio "core" here, I was thinking more
> along the lines of when we include the PCI or IOMMU subsystem there's
> a degree of base functionality included there regardless of what
> additional options or drivers are selected.  

Lots of other cases are just like VFIO where it is the subsystem core
that really doesn't do anything. Look at tpm, infiniband, drm, etc

> The current state is that we cannot build vfio-pci-core.ko without
> vfio-pci.ko, so there's always an in-kernel user.  

I think I might have done that, and it wasn't done for that reason.. I
just messed it up and didn't follow the normal pattern - and this
caused these troubles with the wrong/missing depends/selects.

I view following the usual pattern as more valuable than a one off fix
for what is really a systemic issue in kconfig. Which is why I made
the patch to align with how CONFIG_VFIO works :)

Jason
Eric Auger June 7, 2023, 1:33 p.m. UTC | #7
Hi Alex,

On 6/2/23 23:33, Alex Williamson wrote:
> It should be possible to select vfio-pci variant drivers without building
> vfio-pci itself, which implies each variant driver should select
> vfio-pci-core.
> 
> Fix the top level vfio Makefile to traverse pci based on vfio-pci-core
> rather than vfio-pci.
> 
> Mark MMAP and INTX options depending on vfio-pci-core to cleanup resulting
> config if core is not enabled.
> 
> Push all PCI related vfio options to a sub-menu and make descriptions
> consistent.
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>


Eric

> ---
>  drivers/vfio/Makefile              | 2 +-
>  drivers/vfio/pci/Kconfig           | 8 ++++++--
>  drivers/vfio/pci/hisilicon/Kconfig | 4 ++--
>  drivers/vfio/pci/mlx5/Kconfig      | 2 +-
>  4 files changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/vfio/Makefile b/drivers/vfio/Makefile
> index 70e7dcb302ef..151e816b2ff9 100644
> --- a/drivers/vfio/Makefile
> +++ b/drivers/vfio/Makefile
> @@ -10,7 +10,7 @@ vfio-$(CONFIG_VFIO_VIRQFD) += virqfd.o
>  
>  obj-$(CONFIG_VFIO_IOMMU_TYPE1) += vfio_iommu_type1.o
>  obj-$(CONFIG_VFIO_IOMMU_SPAPR_TCE) += vfio_iommu_spapr_tce.o
> -obj-$(CONFIG_VFIO_PCI) += pci/
> +obj-$(CONFIG_VFIO_PCI_CORE) += pci/
>  obj-$(CONFIG_VFIO_PLATFORM) += platform/
>  obj-$(CONFIG_VFIO_MDEV) += mdev/
>  obj-$(CONFIG_VFIO_FSL_MC) += fsl-mc/
> diff --git a/drivers/vfio/pci/Kconfig b/drivers/vfio/pci/Kconfig
> index f9d0c908e738..86bb7835cf3c 100644
> --- a/drivers/vfio/pci/Kconfig
> +++ b/drivers/vfio/pci/Kconfig
> @@ -1,5 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0-only
> -if PCI && MMU
> +menu "VFIO support for PCI devices"
> +	depends on PCI && MMU
> +
>  config VFIO_PCI_CORE
>  	tristate
>  	select VFIO_VIRQFD
> @@ -7,9 +9,11 @@ config VFIO_PCI_CORE
>  
>  config VFIO_PCI_MMAP
>  	def_bool y if !S390
> +	depends on VFIO_PCI_CORE
>  
>  config VFIO_PCI_INTX
>  	def_bool y if !S390
> +	depends on VFIO_PCI_CORE
>  
>  config VFIO_PCI
>  	tristate "Generic VFIO support for any PCI device"
> @@ -59,4 +63,4 @@ source "drivers/vfio/pci/mlx5/Kconfig"
>  
>  source "drivers/vfio/pci/hisilicon/Kconfig"
>  
> -endif
> +endmenu
> diff --git a/drivers/vfio/pci/hisilicon/Kconfig b/drivers/vfio/pci/hisilicon/Kconfig
> index 5daa0f45d2f9..cbf1c32f6ebf 100644
> --- a/drivers/vfio/pci/hisilicon/Kconfig
> +++ b/drivers/vfio/pci/hisilicon/Kconfig
> @@ -1,13 +1,13 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  config HISI_ACC_VFIO_PCI
> -	tristate "VFIO PCI support for HiSilicon ACC devices"
> +	tristate "VFIO support for HiSilicon ACC PCI devices"
>  	depends on ARM64 || (COMPILE_TEST && 64BIT)
> -	depends on VFIO_PCI_CORE
>  	depends on PCI_MSI
>  	depends on CRYPTO_DEV_HISI_QM
>  	depends on CRYPTO_DEV_HISI_HPRE
>  	depends on CRYPTO_DEV_HISI_SEC2
>  	depends on CRYPTO_DEV_HISI_ZIP
> +	select VFIO_PCI_CORE
>  	help
>  	  This provides generic PCI support for HiSilicon ACC devices
>  	  using the VFIO framework.
> diff --git a/drivers/vfio/pci/mlx5/Kconfig b/drivers/vfio/pci/mlx5/Kconfig
> index 29ba9c504a75..7088edc4fb28 100644
> --- a/drivers/vfio/pci/mlx5/Kconfig
> +++ b/drivers/vfio/pci/mlx5/Kconfig
> @@ -2,7 +2,7 @@
>  config MLX5_VFIO_PCI
>  	tristate "VFIO support for MLX5 PCI devices"
>  	depends on MLX5_CORE
> -	depends on VFIO_PCI_CORE
> +	select VFIO_PCI_CORE
>  	help
>  	  This provides migration support for MLX5 devices using the VFIO
>  	  framework.
Alex Williamson June 7, 2023, 5:24 p.m. UTC | #8
On Tue, 6 Jun 2023 20:27:41 -0300
Jason Gunthorpe <jgg@nvidia.com> wrote:

> On Tue, Jun 06, 2023 at 03:57:04PM -0600, Alex Williamson wrote:
> > > Not really, maybe it creates a sysfs class, but it certainly doesn't
> > > do anything useful unless there is a vfio driver also selected.  
> > 
> > Sorry, I wasn't referring to vfio "core" here, I was thinking more
> > along the lines of when we include the PCI or IOMMU subsystem there's
> > a degree of base functionality included there regardless of what
> > additional options or drivers are selected.    
> 
> Lots of other cases are just like VFIO where it is the subsystem core
> that really doesn't do anything. Look at tpm, infiniband, drm, etc

This is getting a bit absurd, the build system should not be building
modules that have no users.  Maybe it's not a high enough priority to go
to excessive lengths to prevent it, but I don't see that we're doing
that here.
 
> > The current state is that we cannot build vfio-pci-core.ko without
> > vfio-pci.ko, so there's always an in-kernel user.    
> 
> I think I might have done that, and it wasn't done for that reason.. I
> just messed it up and didn't follow the normal pattern - and this
> caused these troubles with the wrong/missing depends/selects.

I didn't assume this was intentional, but the result of requiring a
built user of vfio-pci-core is not entirely bad.

> I view following the usual pattern as more valuable than a one off fix
> for what is really a systemic issue in kconfig. Which is why I made
> the patch to align with how CONFIG_VFIO works :)

Is using a menu and having drivers select the config option for the
core module they depend on really that unusual?  This all seems like
Kconfig 101.

Perhaps we should be more sensitive to this in vfio than other parts of
the kernel exactly because we're providing a userspace driver
interface.  We should disable as much as we can of that interface when
there are no in-kernel users of it.

I'm failing to see how "this is the way we do things" makes it correct
when we can trivially eliminate the possibility of building this
particular shared module when it has no users.  Thanks,

Alex
diff mbox series

Patch

diff --git a/drivers/vfio/Makefile b/drivers/vfio/Makefile
index 70e7dcb302ef..151e816b2ff9 100644
--- a/drivers/vfio/Makefile
+++ b/drivers/vfio/Makefile
@@ -10,7 +10,7 @@  vfio-$(CONFIG_VFIO_VIRQFD) += virqfd.o
 
 obj-$(CONFIG_VFIO_IOMMU_TYPE1) += vfio_iommu_type1.o
 obj-$(CONFIG_VFIO_IOMMU_SPAPR_TCE) += vfio_iommu_spapr_tce.o
-obj-$(CONFIG_VFIO_PCI) += pci/
+obj-$(CONFIG_VFIO_PCI_CORE) += pci/
 obj-$(CONFIG_VFIO_PLATFORM) += platform/
 obj-$(CONFIG_VFIO_MDEV) += mdev/
 obj-$(CONFIG_VFIO_FSL_MC) += fsl-mc/
diff --git a/drivers/vfio/pci/Kconfig b/drivers/vfio/pci/Kconfig
index f9d0c908e738..86bb7835cf3c 100644
--- a/drivers/vfio/pci/Kconfig
+++ b/drivers/vfio/pci/Kconfig
@@ -1,5 +1,7 @@ 
 # SPDX-License-Identifier: GPL-2.0-only
-if PCI && MMU
+menu "VFIO support for PCI devices"
+	depends on PCI && MMU
+
 config VFIO_PCI_CORE
 	tristate
 	select VFIO_VIRQFD
@@ -7,9 +9,11 @@  config VFIO_PCI_CORE
 
 config VFIO_PCI_MMAP
 	def_bool y if !S390
+	depends on VFIO_PCI_CORE
 
 config VFIO_PCI_INTX
 	def_bool y if !S390
+	depends on VFIO_PCI_CORE
 
 config VFIO_PCI
 	tristate "Generic VFIO support for any PCI device"
@@ -59,4 +63,4 @@  source "drivers/vfio/pci/mlx5/Kconfig"
 
 source "drivers/vfio/pci/hisilicon/Kconfig"
 
-endif
+endmenu
diff --git a/drivers/vfio/pci/hisilicon/Kconfig b/drivers/vfio/pci/hisilicon/Kconfig
index 5daa0f45d2f9..cbf1c32f6ebf 100644
--- a/drivers/vfio/pci/hisilicon/Kconfig
+++ b/drivers/vfio/pci/hisilicon/Kconfig
@@ -1,13 +1,13 @@ 
 # SPDX-License-Identifier: GPL-2.0-only
 config HISI_ACC_VFIO_PCI
-	tristate "VFIO PCI support for HiSilicon ACC devices"
+	tristate "VFIO support for HiSilicon ACC PCI devices"
 	depends on ARM64 || (COMPILE_TEST && 64BIT)
-	depends on VFIO_PCI_CORE
 	depends on PCI_MSI
 	depends on CRYPTO_DEV_HISI_QM
 	depends on CRYPTO_DEV_HISI_HPRE
 	depends on CRYPTO_DEV_HISI_SEC2
 	depends on CRYPTO_DEV_HISI_ZIP
+	select VFIO_PCI_CORE
 	help
 	  This provides generic PCI support for HiSilicon ACC devices
 	  using the VFIO framework.
diff --git a/drivers/vfio/pci/mlx5/Kconfig b/drivers/vfio/pci/mlx5/Kconfig
index 29ba9c504a75..7088edc4fb28 100644
--- a/drivers/vfio/pci/mlx5/Kconfig
+++ b/drivers/vfio/pci/mlx5/Kconfig
@@ -2,7 +2,7 @@ 
 config MLX5_VFIO_PCI
 	tristate "VFIO support for MLX5 PCI devices"
 	depends on MLX5_CORE
-	depends on VFIO_PCI_CORE
+	select VFIO_PCI_CORE
 	help
 	  This provides migration support for MLX5 devices using the VFIO
 	  framework.