[PATCH] vfio: Fixup kconfig ordering for VFIO_PCI_CORE

[PATCH] vfio: Fixup kconfig ordering for VFIO_PCI_CORE

[Jason Gunthorpe]

Make VFIO_PCI_CORE the top level menu choice and make it directly
selectable by the user.

This makes a sub menu with all the different PCI driver choices and causes
VFIO_PCI to be enabled by default if the user selects "VFIO support for
PCI devices"

Remove the duplicated 'depends on' from variant drivers and enclose all
the different sub driver choices (including S390 which was wrongly missing
a depends) in a single if block. This makes all the dependencies more
robust.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/vfio/pci/Kconfig           | 17 ++++++++++++-----
 drivers/vfio/pci/hisilicon/Kconfig |  1 -
 drivers/vfio/pci/mlx5/Kconfig      |  1 -
 3 files changed, 12 insertions(+), 7 deletions(-)

Slightly different than as discussed as this seem more robust at the cost of
adding another menu layer.

diff --git a/drivers/vfio/pci/Kconfig b/drivers/vfio/pci/Kconfig
index f9d0c908e738c3..5e9868d5ff1569 100644
--- a/drivers/vfio/pci/Kconfig
+++ b/drivers/vfio/pci/Kconfig
@@ -1,9 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0-only
 if PCI && MMU
-config VFIO_PCI_CORE
-	tristate
-	select VFIO_VIRQFD
-	select IRQ_BYPASS_MANAGER
 
 config VFIO_PCI_MMAP
 	def_bool y if !S390
@@ -11,9 +7,19 @@ config VFIO_PCI_MMAP
 config VFIO_PCI_INTX
 	def_bool y if !S390
 
+config VFIO_PCI_CORE
+	tristate "VFIO support for PCI devices"
+	select VFIO_VIRQFD
+	select IRQ_BYPASS_MANAGER
+	help
+	  Base support for VFIO drivers that support PCI devices. At least one
+	  of the implementation drivers must be selected.
+
+if VFIO_PCI_CORE
+
 config VFIO_PCI
 	tristate "Generic VFIO support for any PCI device"
-	select VFIO_PCI_CORE
+	default y
 	help
 	  Support for the generic PCI VFIO bus driver which can connect any
 	  PCI device to the VFIO framework.
@@ -60,3 +66,4 @@ source "drivers/vfio/pci/mlx5/Kconfig"
 source "drivers/vfio/pci/hisilicon/Kconfig"
 
 endif
+endif
diff --git a/drivers/vfio/pci/hisilicon/Kconfig b/drivers/vfio/pci/hisilicon/Kconfig
index 5daa0f45d2f99b..86826513765062 100644
--- a/drivers/vfio/pci/hisilicon/Kconfig
+++ b/drivers/vfio/pci/hisilicon/Kconfig
@@ -2,7 +2,6 @@
 config HISI_ACC_VFIO_PCI
 	tristate "VFIO PCI support for HiSilicon ACC 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
diff --git a/drivers/vfio/pci/mlx5/Kconfig b/drivers/vfio/pci/mlx5/Kconfig
index 29ba9c504a7560..d36b18d3e21fe7 100644
--- a/drivers/vfio/pci/mlx5/Kconfig
+++ b/drivers/vfio/pci/mlx5/Kconfig
@@ -2,7 +2,6 @@
 config MLX5_VFIO_PCI
 	tristate "VFIO support for MLX5 PCI devices"
 	depends on MLX5_CORE
-	depends on VFIO_PCI_CORE
 	help
 	  This provides migration support for MLX5 devices using the VFIO
 	  framework.

base-commit: 8c1ee346da583718fb0a7791a1f84bdafb103caf
-- 
2.40.1

Re: [PATCH] vfio: Fixup kconfig ordering for VFIO_PCI_CORE

[Alex Williamson]

On Mon, 29 May 2023 14:47:59 -0300
Jason Gunthorpe <jgg@nvidia.com> wrote:

> Make VFIO_PCI_CORE the top level menu choice and make it directly
> selectable by the user.
> 
> This makes a sub menu with all the different PCI driver choices and causes
> VFIO_PCI to be enabled by default if the user selects "VFIO support for
> PCI devices"
> 
> Remove the duplicated 'depends on' from variant drivers and enclose all
> the different sub driver choices (including S390 which was wrongly missing
> a depends) in a single if block. This makes all the dependencies more
> robust.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/vfio/pci/Kconfig           | 17 ++++++++++++-----
>  drivers/vfio/pci/hisilicon/Kconfig |  1 -
>  drivers/vfio/pci/mlx5/Kconfig      |  1 -
>  3 files changed, 12 insertions(+), 7 deletions(-)
> 
> Slightly different than as discussed as this seem more robust at the cost of
> adding another menu layer.
> 
> diff --git a/drivers/vfio/pci/Kconfig b/drivers/vfio/pci/Kconfig
> index f9d0c908e738c3..5e9868d5ff1569 100644
> --- a/drivers/vfio/pci/Kconfig
> +++ b/drivers/vfio/pci/Kconfig
> @@ -1,9 +1,5 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  if PCI && MMU
> -config VFIO_PCI_CORE
> -	tristate
> -	select VFIO_VIRQFD
> -	select IRQ_BYPASS_MANAGER
>  
>  config VFIO_PCI_MMAP
>  	def_bool y if !S390
> @@ -11,9 +7,19 @@ config VFIO_PCI_MMAP
>  config VFIO_PCI_INTX
>  	def_bool y if !S390
>  
> +config VFIO_PCI_CORE
> +	tristate "VFIO support for PCI devices"
> +	select VFIO_VIRQFD
> +	select IRQ_BYPASS_MANAGER
> +	help
> +	  Base support for VFIO drivers that support PCI devices. At least one
> +	  of the implementation drivers must be selected.

As enforced by what?

This is just adding one more layer of dependencies in order to select
the actual endpoint driver that is actually what anyone cares about.
Despite the above help text, I think this also allows a kernel to be
built with vfio-pci-core and nothing in-kernel that uses it.

I don't see why we wouldn't just make each of the variant drivers
select VFIO_PCI_CORE.  Thanks,

Alex

> +
> +if VFIO_PCI_CORE
> +
>  config VFIO_PCI
>  	tristate "Generic VFIO support for any PCI device"
> -	select VFIO_PCI_CORE
> +	default y
>  	help
>  	  Support for the generic PCI VFIO bus driver which can connect any
>  	  PCI device to the VFIO framework.
> @@ -60,3 +66,4 @@ source "drivers/vfio/pci/mlx5/Kconfig"
>  source "drivers/vfio/pci/hisilicon/Kconfig"
>  
>  endif
> +endif
> diff --git a/drivers/vfio/pci/hisilicon/Kconfig b/drivers/vfio/pci/hisilicon/Kconfig
> index 5daa0f45d2f99b..86826513765062 100644
> --- a/drivers/vfio/pci/hisilicon/Kconfig
> +++ b/drivers/vfio/pci/hisilicon/Kconfig
> @@ -2,7 +2,6 @@
>  config HISI_ACC_VFIO_PCI
>  	tristate "VFIO PCI support for HiSilicon ACC 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
> diff --git a/drivers/vfio/pci/mlx5/Kconfig b/drivers/vfio/pci/mlx5/Kconfig
> index 29ba9c504a7560..d36b18d3e21fe7 100644
> --- a/drivers/vfio/pci/mlx5/Kconfig
> +++ b/drivers/vfio/pci/mlx5/Kconfig
> @@ -2,7 +2,6 @@
>  config MLX5_VFIO_PCI
>  	tristate "VFIO support for MLX5 PCI devices"
>  	depends on MLX5_CORE
> -	depends on VFIO_PCI_CORE
>  	help
>  	  This provides migration support for MLX5 devices using the VFIO
>  	  framework.
> 
> base-commit: 8c1ee346da583718fb0a7791a1f84bdafb103caf

Re: [PATCH] vfio: Fixup kconfig ordering for VFIO_PCI_CORE

[Jason Gunthorpe]

On Thu, Jun 01, 2023 at 02:42:38PM -0600, Alex Williamson wrote:
> On Mon, 29 May 2023 14:47:59 -0300
> > +config VFIO_PCI_CORE
> > +	tristate "VFIO support for PCI devices"
> > +	select VFIO_VIRQFD
> > +	select IRQ_BYPASS_MANAGER
> > +	help
> > +	  Base support for VFIO drivers that support PCI devices. At least one
> > +	  of the implementation drivers must be selected.
> 
> As enforced by what?

Doesn't need to be enforced. Probably should have said "should"

> This is just adding one more layer of dependencies in order to select
> the actual endpoint driver that is actually what anyone cares about.

This is making the kconfig more logical and the menu structure better
organized. We eliminate the need for the drivers to set special
depends because the if covers them all.

> I don't see why we wouldn't just make each of the variant drivers
> select VFIO_PCI_CORE.  Thanks,

It can be done, but it seems more fragile.

Jason

Re: [PATCH] vfio: Fixup kconfig ordering for VFIO_PCI_CORE

[Alex Williamson]

On Thu, 1 Jun 2023 17:48:27 -0300
Jason Gunthorpe <jgg@nvidia.com> wrote:

> On Thu, Jun 01, 2023 at 02:42:38PM -0600, Alex Williamson wrote:
> > On Mon, 29 May 2023 14:47:59 -0300  
> > > +config VFIO_PCI_CORE
> > > +	tristate "VFIO support for PCI devices"
> > > +	select VFIO_VIRQFD
> > > +	select IRQ_BYPASS_MANAGER
> > > +	help
> > > +	  Base support for VFIO drivers that support PCI devices. At least one
> > > +	  of the implementation drivers must be selected.  
> > 
> > As enforced by what?  
> 
> Doesn't need to be enforced. Probably should have said "should"
> 
> > This is just adding one more layer of dependencies in order to select
> > the actual endpoint driver that is actually what anyone cares about.  
> 
> This is making the kconfig more logical and the menu structure better
> organized. We eliminate the need for the drivers to set special
> depends because the if covers them all.

We can create a menu with a dummy config option, ex:

config VFIO_PCI_DRIVERS
        bool "VFIO support for PCI devices"
        default y

if VFIO_PCI_DRIVERS
...

So the menu organization itself isn't sufficient for me to make
VFIO_PCI_CORE to root of that menu.

The flip side of requiring drivers to set a "special depends" is that
it's possible we might have a PCI variant driver that doesn't use
VFIO_PCI_CORE.  It would be a hard sell, but it's possible.  It also
shouldn't be terribly subtle to the existing variant drivers relying on
vfio-pci-core that this dependency exists.

Allowing VFIO_PCI_CORE without building an in-kernel module that
depends on it seals the deal for me that this is the wrong approach
though.

> > I don't see why we wouldn't just make each of the variant drivers
> > select VFIO_PCI_CORE.  Thanks,  
> 
> It can be done, but it seems more fragile.

How so?  Thanks,

Alex

Re: [PATCH] vfio: Fixup kconfig ordering for VFIO_PCI_CORE

[Jason Gunthorpe]

On Thu, Jun 01, 2023 at 03:48:57PM -0600, Alex Williamson wrote:
> Allowing VFIO_PCI_CORE without building an in-kernel module that
> depends on it seals the deal for me that this is the wrong approach
> though.

I don't think that is abnormal, we have constructs like this around,
kconfig has its limits.

> > > I don't see why we wouldn't just make each of the variant drivers
> > > select VFIO_PCI_CORE.  Thanks,  
> > 
> > It can be done, but it seems more fragile.
> 
> How so?

The if stanza in the core's kconfig does the right thing automatically
without any need to think about the driver kconfigs.

Jason