[PATCH 0/3] vfio: Cleanup Kconfigs

[PATCH 0/3] vfio: Cleanup Kconfigs

[Alex Williamson]

Create sub-menus for bus drivers and remove artificial dependencies
between vfio-pci and vfio-platform and other variant drivers like
vfio-amba, mlx5-vfio-pci, or hisi-acc-vfio-pci.

This is an alternative proposal vs [1], which attempts to make the
vfio-pci-core module individually selectable, even without built
dependencies, in order to avoid the select per variant module.

The solution presented here is my preference, I don't see the select as
overly burdensome or error prone, we have very good randconfig test
coverage, and I prefer to not allow building module which have no
in-kernel requirements.  Thanks,

Alex

[1]https://lore.kernel.org/all/0-v1-7eacf832787f+86-vfio_pci_kconfig_jgg@nvidia.com/

Alex Williamson (3):
  vfio/pci: Cleanup Kconfig
  vfio/platform: Cleanup Kconfig
  vfio/fsl: Create Kconfig sub-menu

 drivers/vfio/Makefile              |  4 ++--
 drivers/vfio/fsl-mc/Kconfig        |  4 ++++
 drivers/vfio/pci/Kconfig           |  8 ++++++--
 drivers/vfio/pci/hisilicon/Kconfig |  4 ++--
 drivers/vfio/pci/mlx5/Kconfig      |  2 +-
 drivers/vfio/platform/Kconfig      | 17 ++++++++++++++---
 drivers/vfio/platform/Makefile     |  9 +++------
 7 files changed, 32 insertions(+), 16 deletions(-)

-- 
2.39.2

[PATCH 1/3] vfio/pci: Cleanup Kconfig

[Alex Williamson]

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

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

[PATCH 2/3] vfio/platform: Cleanup Kconfig

[Alex Williamson]

Like vfio-pci, there's also a base module here where vfio-amba depends on
vfio-platform, when really it only needs vfio-platform-base.  Create a
sub-menu for platform drivers and a nested menu for reset drivers.  Cleanup
Makefile to make use of new CONFIG_VFIO_PLATFORM_BASE for building the
shared modules and traversing reset modules.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 drivers/vfio/Makefile          |  2 +-
 drivers/vfio/platform/Kconfig  | 17 ++++++++++++++---
 drivers/vfio/platform/Makefile |  9 +++------
 3 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/drivers/vfio/Makefile b/drivers/vfio/Makefile
index 151e816b2ff9..8da44aa1ea16 100644
--- a/drivers/vfio/Makefile
+++ b/drivers/vfio/Makefile
@@ -11,6 +11,6 @@ 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_CORE) += pci/
-obj-$(CONFIG_VFIO_PLATFORM) += platform/
+obj-$(CONFIG_VFIO_PLATFORM_BASE) += platform/
 obj-$(CONFIG_VFIO_MDEV) += mdev/
 obj-$(CONFIG_VFIO_FSL_MC) += fsl-mc/
diff --git a/drivers/vfio/platform/Kconfig b/drivers/vfio/platform/Kconfig
index 331a5920f5ab..6d18faa66a2e 100644
--- a/drivers/vfio/platform/Kconfig
+++ b/drivers/vfio/platform/Kconfig
@@ -1,8 +1,14 @@
 # SPDX-License-Identifier: GPL-2.0-only
+menu "VFIO support for platform devices"
+
+config VFIO_PLATFORM_BASE
+	tristate
+
 config VFIO_PLATFORM
-	tristate "VFIO support for platform devices"
+	tristate "Generic VFIO support for any platform device"
 	depends on ARM || ARM64 || COMPILE_TEST
 	select VFIO_VIRQFD
+	select VFIO_PLATFORM_BASE
 	help
 	  Support for platform devices with VFIO. This is required to make
 	  use of platform devices present on the system using the VFIO
@@ -10,10 +16,11 @@ config VFIO_PLATFORM
 
 	  If you don't know what to do here, say N.
 
-if VFIO_PLATFORM
 config VFIO_AMBA
 	tristate "VFIO support for AMBA devices"
 	depends on ARM_AMBA || COMPILE_TEST
+	select VFIO_VIRQFD
+	select VFIO_PLATFORM_BASE
 	help
 	  Support for ARM AMBA devices with VFIO. This is required to make
 	  use of ARM AMBA devices present on the system using the VFIO
@@ -21,5 +28,9 @@ config VFIO_AMBA
 
 	  If you don't know what to do here, say N.
 
+menu "VFIO platform reset drivers"
+	depends on VFIO_PLATFORM_BASE
+
 source "drivers/vfio/platform/reset/Kconfig"
-endif
+endmenu
+endmenu
diff --git a/drivers/vfio/platform/Makefile b/drivers/vfio/platform/Makefile
index 3f3a24e7c4ef..ee4fb6a82ca8 100644
--- a/drivers/vfio/platform/Makefile
+++ b/drivers/vfio/platform/Makefile
@@ -1,13 +1,10 @@
 # SPDX-License-Identifier: GPL-2.0
 vfio-platform-base-y := vfio_platform_common.o vfio_platform_irq.o
-vfio-platform-y := vfio_platform.o
+obj-$(CONFIG_VFIO_PLATFORM_BASE) += vfio-platform-base.o
+obj-$(CONFIG_VFIO_PLATFORM_BASE) += reset/
 
+vfio-platform-y := vfio_platform.o
 obj-$(CONFIG_VFIO_PLATFORM) += vfio-platform.o
-obj-$(CONFIG_VFIO_PLATFORM) += vfio-platform-base.o
-obj-$(CONFIG_VFIO_PLATFORM) += reset/
 
 vfio-amba-y := vfio_amba.o
-
 obj-$(CONFIG_VFIO_AMBA) += vfio-amba.o
-obj-$(CONFIG_VFIO_AMBA) += vfio-platform-base.o
-obj-$(CONFIG_VFIO_AMBA) += reset/
-- 
2.39.2

[PATCH 3/3] vfio/fsl: Create Kconfig sub-menu

[Alex Williamson]

For consistency with pci and platform, push the vfio-fsl-mc option into a
sub-menu.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 drivers/vfio/fsl-mc/Kconfig | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/vfio/fsl-mc/Kconfig b/drivers/vfio/fsl-mc/Kconfig
index 597d338c5c8a..d2757a1114aa 100644
--- a/drivers/vfio/fsl-mc/Kconfig
+++ b/drivers/vfio/fsl-mc/Kconfig
@@ -1,3 +1,5 @@
+menu "VFIO support for FSL_MC bus devices"
+
 config VFIO_FSL_MC
 	tristate "VFIO support for QorIQ DPAA2 fsl-mc bus devices"
 	depends on FSL_MC_BUS
@@ -8,3 +10,5 @@ config VFIO_FSL_MC
 	  fsl-mc bus devices using the VFIO framework.
 
 	  If you don't know what to do here, say N.
+
+endmenu
-- 
2.39.2

Re: [PATCH 1/3] vfio/pci: Cleanup Kconfig

[Cédric Le Goater]

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.

Re: [PATCH 2/3] vfio/platform: Cleanup Kconfig

[Cédric Le Goater]

On 6/2/23 23:33, Alex Williamson wrote:
> Like vfio-pci, there's also a base module here where vfio-amba depends on
> vfio-platform, when really it only needs vfio-platform-base.  Create a
> sub-menu for platform drivers and a nested menu for reset drivers.  Cleanup
> Makefile to make use of new CONFIG_VFIO_PLATFORM_BASE for building the
> shared modules and traversing reset modules.
> 
> 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/platform/Kconfig  | 17 ++++++++++++++---
>   drivers/vfio/platform/Makefile |  9 +++------
>   3 files changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/vfio/Makefile b/drivers/vfio/Makefile
> index 151e816b2ff9..8da44aa1ea16 100644
> --- a/drivers/vfio/Makefile
> +++ b/drivers/vfio/Makefile
> @@ -11,6 +11,6 @@ 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_CORE) += pci/
> -obj-$(CONFIG_VFIO_PLATFORM) += platform/
> +obj-$(CONFIG_VFIO_PLATFORM_BASE) += platform/
>   obj-$(CONFIG_VFIO_MDEV) += mdev/
>   obj-$(CONFIG_VFIO_FSL_MC) += fsl-mc/
> diff --git a/drivers/vfio/platform/Kconfig b/drivers/vfio/platform/Kconfig
> index 331a5920f5ab..6d18faa66a2e 100644
> --- a/drivers/vfio/platform/Kconfig
> +++ b/drivers/vfio/platform/Kconfig
> @@ -1,8 +1,14 @@
>   # SPDX-License-Identifier: GPL-2.0-only
> +menu "VFIO support for platform devices"
> +
> +config VFIO_PLATFORM_BASE
> +	tristate
> +
>   config VFIO_PLATFORM
> -	tristate "VFIO support for platform devices"
> +	tristate "Generic VFIO support for any platform device"
>   	depends on ARM || ARM64 || COMPILE_TEST
>   	select VFIO_VIRQFD
> +	select VFIO_PLATFORM_BASE
>   	help
>   	  Support for platform devices with VFIO. This is required to make
>   	  use of platform devices present on the system using the VFIO
> @@ -10,10 +16,11 @@ config VFIO_PLATFORM
>   
>   	  If you don't know what to do here, say N.
>   
> -if VFIO_PLATFORM
>   config VFIO_AMBA
>   	tristate "VFIO support for AMBA devices"
>   	depends on ARM_AMBA || COMPILE_TEST
> +	select VFIO_VIRQFD
> +	select VFIO_PLATFORM_BASE
>   	help
>   	  Support for ARM AMBA devices with VFIO. This is required to make
>   	  use of ARM AMBA devices present on the system using the VFIO
> @@ -21,5 +28,9 @@ config VFIO_AMBA
>   
>   	  If you don't know what to do here, say N.
>   
> +menu "VFIO platform reset drivers"
> +	depends on VFIO_PLATFORM_BASE
> +
>   source "drivers/vfio/platform/reset/Kconfig"
> -endif
> +endmenu
> +endmenu
> diff --git a/drivers/vfio/platform/Makefile b/drivers/vfio/platform/Makefile
> index 3f3a24e7c4ef..ee4fb6a82ca8 100644
> --- a/drivers/vfio/platform/Makefile
> +++ b/drivers/vfio/platform/Makefile
> @@ -1,13 +1,10 @@
>   # SPDX-License-Identifier: GPL-2.0
>   vfio-platform-base-y := vfio_platform_common.o vfio_platform_irq.o
> -vfio-platform-y := vfio_platform.o
> +obj-$(CONFIG_VFIO_PLATFORM_BASE) += vfio-platform-base.o
> +obj-$(CONFIG_VFIO_PLATFORM_BASE) += reset/
>   
> +vfio-platform-y := vfio_platform.o
>   obj-$(CONFIG_VFIO_PLATFORM) += vfio-platform.o
> -obj-$(CONFIG_VFIO_PLATFORM) += vfio-platform-base.o
> -obj-$(CONFIG_VFIO_PLATFORM) += reset/
>   
>   vfio-amba-y := vfio_amba.o
> -
>   obj-$(CONFIG_VFIO_AMBA) += vfio-amba.o
> -obj-$(CONFIG_VFIO_AMBA) += vfio-platform-base.o
> -obj-$(CONFIG_VFIO_AMBA) += reset/

Re: [PATCH 3/3] vfio/fsl: Create Kconfig sub-menu

[Cédric Le Goater]

On 6/2/23 23:33, Alex Williamson wrote:
> For consistency with pci and platform, push the vfio-fsl-mc option into a
> sub-menu.
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>

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

Thanks,

C.

> ---
>   drivers/vfio/fsl-mc/Kconfig | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/vfio/fsl-mc/Kconfig b/drivers/vfio/fsl-mc/Kconfig
> index 597d338c5c8a..d2757a1114aa 100644
> --- a/drivers/vfio/fsl-mc/Kconfig
> +++ b/drivers/vfio/fsl-mc/Kconfig
> @@ -1,3 +1,5 @@
> +menu "VFIO support for FSL_MC bus devices"
> +
>   config VFIO_FSL_MC
>   	tristate "VFIO support for QorIQ DPAA2 fsl-mc bus devices"
>   	depends on FSL_MC_BUS
> @@ -8,3 +10,5 @@ config VFIO_FSL_MC
>   	  fsl-mc bus devices using the VFIO framework.
>   
>   	  If you don't know what to do here, say N.
> +
> +endmenu

Re: [PATCH 1/3] vfio/pci: Cleanup Kconfig

[Jason Gunthorpe]

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

Re: [PATCH 1/3] vfio/pci: Cleanup Kconfig

[Alex Williamson]

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

Re: [PATCH 1/3] vfio/pci: Cleanup Kconfig

[Jason Gunthorpe]

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

Re: [PATCH 1/3] vfio/pci: Cleanup Kconfig

[Alex Williamson]

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

Re: [PATCH 1/3] vfio/pci: Cleanup Kconfig

[Jason Gunthorpe]

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

Re: [PATCH 2/3] vfio/platform: Cleanup Kconfig

[Eric Auger]

Hi Alex,

On 6/2/23 23:33, Alex Williamson wrote:
> Like vfio-pci, there's also a base module here where vfio-amba depends on
> vfio-platform, when really it only needs vfio-platform-base.  Create a
> sub-menu for platform drivers and a nested menu for reset drivers.  Cleanup
> Makefile to make use of new CONFIG_VFIO_PLATFORM_BASE for building the
> shared modules and traversing reset modules.
>
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
>  drivers/vfio/Makefile          |  2 +-
>  drivers/vfio/platform/Kconfig  | 17 ++++++++++++++---
>  drivers/vfio/platform/Makefile |  9 +++------
>  3 files changed, 18 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/vfio/Makefile b/drivers/vfio/Makefile
> index 151e816b2ff9..8da44aa1ea16 100644
> --- a/drivers/vfio/Makefile
> +++ b/drivers/vfio/Makefile
> @@ -11,6 +11,6 @@ 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_CORE) += pci/
> -obj-$(CONFIG_VFIO_PLATFORM) += platform/
> +obj-$(CONFIG_VFIO_PLATFORM_BASE) += platform/
>  obj-$(CONFIG_VFIO_MDEV) += mdev/
>  obj-$(CONFIG_VFIO_FSL_MC) += fsl-mc/
> diff --git a/drivers/vfio/platform/Kconfig b/drivers/vfio/platform/Kconfig
> index 331a5920f5ab..6d18faa66a2e 100644
> --- a/drivers/vfio/platform/Kconfig
> +++ b/drivers/vfio/platform/Kconfig
> @@ -1,8 +1,14 @@
>  # SPDX-License-Identifier: GPL-2.0-only
> +menu "VFIO support for platform devices"
> +
> +config VFIO_PLATFORM_BASE
> +	tristate
> +
>  config VFIO_PLATFORM
> -	tristate "VFIO support for platform devices"
> +	tristate "Generic VFIO support for any platform device"
>  	depends on ARM || ARM64 || COMPILE_TEST
I wonder if we couldn't put those dependencies at the menu level. I
guess this also applies to AMBA. And just leave 'depends on ARM_AMBA ' in

config VFIO_AMBA?

>  	select VFIO_VIRQFD
> +	select VFIO_PLATFORM_BASE
>  	help
>  	  Support for platform devices with VFIO. This is required to make
>  	  use of platform devices present on the system using the VFIO
> @@ -10,10 +16,11 @@ config VFIO_PLATFORM
>  
>  	  If you don't know what to do here, say N.
>  
> -if VFIO_PLATFORM
>  config VFIO_AMBA
>  	tristate "VFIO support for AMBA devices"
>  	depends on ARM_AMBA || COMPILE_TEST
> +	select VFIO_VIRQFD
> +	select VFIO_PLATFORM_BASE
>  	help
>  	  Support for ARM AMBA devices with VFIO. This is required to make
>  	  use of ARM AMBA devices present on the system using the VFIO
> @@ -21,5 +28,9 @@ config VFIO_AMBA
>  
>  	  If you don't know what to do here, say N.
>  
> +menu "VFIO platform reset drivers"
> +	depends on VFIO_PLATFORM_BASE
I wonder if this shouldn't depend on VFIO_PLATFORM instead?
There are no amba reset devices at the moment so why whould be compile
them if VFIO_AMBA is set (which is unlikely by the way)?

Eric
> +
>  source "drivers/vfio/platform/reset/Kconfig"
> -endif
> +endmenu
> +endmenu
> diff --git a/drivers/vfio/platform/Makefile b/drivers/vfio/platform/Makefile
> index 3f3a24e7c4ef..ee4fb6a82ca8 100644
> --- a/drivers/vfio/platform/Makefile
> +++ b/drivers/vfio/platform/Makefile
> @@ -1,13 +1,10 @@
>  # SPDX-License-Identifier: GPL-2.0
>  vfio-platform-base-y := vfio_platform_common.o vfio_platform_irq.o
> -vfio-platform-y := vfio_platform.o
> +obj-$(CONFIG_VFIO_PLATFORM_BASE) += vfio-platform-base.o
> +obj-$(CONFIG_VFIO_PLATFORM_BASE) += reset/
>  
> +vfio-platform-y := vfio_platform.o
>  obj-$(CONFIG_VFIO_PLATFORM) += vfio-platform.o
> -obj-$(CONFIG_VFIO_PLATFORM) += vfio-platform-base.o
> -obj-$(CONFIG_VFIO_PLATFORM) += reset/
>  
>  vfio-amba-y := vfio_amba.o
> -
>  obj-$(CONFIG_VFIO_AMBA) += vfio-amba.o
> -obj-$(CONFIG_VFIO_AMBA) += vfio-platform-base.o
> -obj-$(CONFIG_VFIO_AMBA) += reset/

Re: [PATCH 1/3] vfio/pci: Cleanup Kconfig

[Eric Auger]

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.

Re: [PATCH 3/3] vfio/fsl: Create Kconfig sub-menu

[Eric Auger]

Hi Alex,

On 6/2/23 23:33, Alex Williamson wrote:
> For consistency with pci and platform, push the vfio-fsl-mc option into a
> sub-menu.
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
>  drivers/vfio/fsl-mc/Kconfig | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/vfio/fsl-mc/Kconfig b/drivers/vfio/fsl-mc/Kconfig
> index 597d338c5c8a..d2757a1114aa 100644
> --- a/drivers/vfio/fsl-mc/Kconfig
> +++ b/drivers/vfio/fsl-mc/Kconfig
> @@ -1,3 +1,5 @@
> +menu "VFIO support for FSL_MC bus devices"
> +
>  config VFIO_FSL_MC
>  	tristate "VFIO support for QorIQ DPAA2 fsl-mc bus devices"
>  	depends on FSL_MC_BUS
> @@ -8,3 +10,5 @@ config VFIO_FSL_MC
>  	  fsl-mc bus devices using the VFIO framework.
>  
>  	  If you don't know what to do here, say N.
> +
> +endmenu
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric

Re: [PATCH 1/3] vfio/pci: Cleanup Kconfig

[Alex Williamson]

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

Re: [PATCH 2/3] vfio/platform: Cleanup Kconfig

[Alex Williamson]

On Wed, 7 Jun 2023 15:32:07 +0200
Eric Auger <eric.auger@redhat.com> wrote:

> Hi Alex,
> 
> On 6/2/23 23:33, Alex Williamson wrote:
> > Like vfio-pci, there's also a base module here where vfio-amba depends on
> > vfio-platform, when really it only needs vfio-platform-base.  Create a
> > sub-menu for platform drivers and a nested menu for reset drivers.  Cleanup
> > Makefile to make use of new CONFIG_VFIO_PLATFORM_BASE for building the
> > shared modules and traversing reset modules.
> >
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > ---
> >  drivers/vfio/Makefile          |  2 +-
> >  drivers/vfio/platform/Kconfig  | 17 ++++++++++++++---
> >  drivers/vfio/platform/Makefile |  9 +++------
> >  3 files changed, 18 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/vfio/Makefile b/drivers/vfio/Makefile
> > index 151e816b2ff9..8da44aa1ea16 100644
> > --- a/drivers/vfio/Makefile
> > +++ b/drivers/vfio/Makefile
> > @@ -11,6 +11,6 @@ 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_CORE) += pci/
> > -obj-$(CONFIG_VFIO_PLATFORM) += platform/
> > +obj-$(CONFIG_VFIO_PLATFORM_BASE) += platform/
> >  obj-$(CONFIG_VFIO_MDEV) += mdev/
> >  obj-$(CONFIG_VFIO_FSL_MC) += fsl-mc/
> > diff --git a/drivers/vfio/platform/Kconfig b/drivers/vfio/platform/Kconfig
> > index 331a5920f5ab..6d18faa66a2e 100644
> > --- a/drivers/vfio/platform/Kconfig
> > +++ b/drivers/vfio/platform/Kconfig
> > @@ -1,8 +1,14 @@
> >  # SPDX-License-Identifier: GPL-2.0-only
> > +menu "VFIO support for platform devices"
> > +
> > +config VFIO_PLATFORM_BASE
> > +	tristate
> > +
> >  config VFIO_PLATFORM
> > -	tristate "VFIO support for platform devices"
> > +	tristate "Generic VFIO support for any platform device"
> >  	depends on ARM || ARM64 || COMPILE_TEST  
> I wonder if we couldn't put those dependencies at the menu level. I
> guess this also applies to AMBA. And just leave 'depends on ARM_AMBA ' in
> 
> config VFIO_AMBA?

Yup, we could, something like:

menu "VFIO support for platform devices"
	depends on ARM || ARM64 || COMPILE_TEST

And we could move VFIO_VIRQFD to VFIO_PLATFORM_BASE

config VFIO_PLATFORM_BASE
	tristate
	select VFIO_VIRQFD

VFIO_AMBA would then only depend on ARM_AMBA and both would select
VFIO_PLATFORM_BASE.

> 
> >  	select VFIO_VIRQFD
> > +	select VFIO_PLATFORM_BASE
> >  	help
> >  	  Support for platform devices with VFIO. This is required to make
> >  	  use of platform devices present on the system using the VFIO
> > @@ -10,10 +16,11 @@ config VFIO_PLATFORM
> >  
> >  	  If you don't know what to do here, say N.
> >  
> > -if VFIO_PLATFORM
> >  config VFIO_AMBA
> >  	tristate "VFIO support for AMBA devices"
> >  	depends on ARM_AMBA || COMPILE_TEST
> > +	select VFIO_VIRQFD
> > +	select VFIO_PLATFORM_BASE
> >  	help
> >  	  Support for ARM AMBA devices with VFIO. This is required to make
> >  	  use of ARM AMBA devices present on the system using the VFIO
> > @@ -21,5 +28,9 @@ config VFIO_AMBA
> >  
> >  	  If you don't know what to do here, say N.
> >  
> > +menu "VFIO platform reset drivers"
> > +	depends on VFIO_PLATFORM_BASE  
> I wonder if this shouldn't depend on VFIO_PLATFORM instead?
> There are no amba reset devices at the moment so why whould be compile
> them if VFIO_AMBA is set (which is unlikely by the way)?

I did see that AMBA sets reset_required = false, but at the same time
the handling of reset modules is in the base driver, so if there were
an AMBA reset driver, wouldn't it also live in the reset/ directory?
It seems like we'd therefore want to traverse into reset/Kconfig, but
maybe if all the current config options in there are non-AMBA we should
wrap them in 'if VFIO_PLATFORM' (or 'depends on' for each, but the 'if'
is marginally cleaner).  Thanks,

Alex

Re: [PATCH 2/3] vfio/platform: Cleanup Kconfig

[Eric Auger]

Hi Alex,
On 6/7/23 21:04, Alex Williamson wrote:
> On Wed, 7 Jun 2023 15:32:07 +0200
> Eric Auger <eric.auger@redhat.com> wrote:
>
>> Hi Alex,
>>
>> On 6/2/23 23:33, Alex Williamson wrote:
>>> Like vfio-pci, there's also a base module here where vfio-amba depends on
>>> vfio-platform, when really it only needs vfio-platform-base.  Create a
>>> sub-menu for platform drivers and a nested menu for reset drivers.  Cleanup
>>> Makefile to make use of new CONFIG_VFIO_PLATFORM_BASE for building the
>>> shared modules and traversing reset modules.
>>>
>>> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
>>> ---
>>>  drivers/vfio/Makefile          |  2 +-
>>>  drivers/vfio/platform/Kconfig  | 17 ++++++++++++++---
>>>  drivers/vfio/platform/Makefile |  9 +++------
>>>  3 files changed, 18 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/vfio/Makefile b/drivers/vfio/Makefile
>>> index 151e816b2ff9..8da44aa1ea16 100644
>>> --- a/drivers/vfio/Makefile
>>> +++ b/drivers/vfio/Makefile
>>> @@ -11,6 +11,6 @@ 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_CORE) += pci/
>>> -obj-$(CONFIG_VFIO_PLATFORM) += platform/
>>> +obj-$(CONFIG_VFIO_PLATFORM_BASE) += platform/
>>>  obj-$(CONFIG_VFIO_MDEV) += mdev/
>>>  obj-$(CONFIG_VFIO_FSL_MC) += fsl-mc/
>>> diff --git a/drivers/vfio/platform/Kconfig b/drivers/vfio/platform/Kconfig
>>> index 331a5920f5ab..6d18faa66a2e 100644
>>> --- a/drivers/vfio/platform/Kconfig
>>> +++ b/drivers/vfio/platform/Kconfig
>>> @@ -1,8 +1,14 @@
>>>  # SPDX-License-Identifier: GPL-2.0-only
>>> +menu "VFIO support for platform devices"
>>> +
>>> +config VFIO_PLATFORM_BASE
>>> +	tristate
>>> +
>>>  config VFIO_PLATFORM
>>> -	tristate "VFIO support for platform devices"
>>> +	tristate "Generic VFIO support for any platform device"
>>>  	depends on ARM || ARM64 || COMPILE_TEST  
>> I wonder if we couldn't put those dependencies at the menu level. I
>> guess this also applies to AMBA. And just leave 'depends on ARM_AMBA ' in
>>
>> config VFIO_AMBA?
> Yup, we could, something like:
>
> menu "VFIO support for platform devices"
> 	depends on ARM || ARM64 || COMPILE_TEST
>
> And we could move VFIO_VIRQFD to VFIO_PLATFORM_BASE
>
> config VFIO_PLATFORM_BASE
> 	tristate
> 	select VFIO_VIRQFD
>
> VFIO_AMBA would then only depend on ARM_AMBA and both would select
> VFIO_PLATFORM_BASE.
>
>>>  	select VFIO_VIRQFD
>>> +	select VFIO_PLATFORM_BASE
>>>  	help
>>>  	  Support for platform devices with VFIO. This is required to make
>>>  	  use of platform devices present on the system using the VFIO
>>> @@ -10,10 +16,11 @@ config VFIO_PLATFORM
>>>  
>>>  	  If you don't know what to do here, say N.
>>>  
>>> -if VFIO_PLATFORM
>>>  config VFIO_AMBA
>>>  	tristate "VFIO support for AMBA devices"
>>>  	depends on ARM_AMBA || COMPILE_TEST
>>> +	select VFIO_VIRQFD
>>> +	select VFIO_PLATFORM_BASE
>>>  	help
>>>  	  Support for ARM AMBA devices with VFIO. This is required to make
>>>  	  use of ARM AMBA devices present on the system using the VFIO
>>> @@ -21,5 +28,9 @@ config VFIO_AMBA
>>>  
>>>  	  If you don't know what to do here, say N.
>>>  
>>> +menu "VFIO platform reset drivers"
>>> +	depends on VFIO_PLATFORM_BASE  
>> I wonder if this shouldn't depend on VFIO_PLATFORM instead?
>> There are no amba reset devices at the moment so why whould be compile
>> them if VFIO_AMBA is set (which is unlikely by the way)?
> I did see that AMBA sets reset_required = false, but at the same time
> the handling of reset modules is in the base driver, so if there were
> an AMBA reset driver, wouldn't it also live in the reset/ directory?
Yes I guess so.
> It seems like we'd therefore want to traverse into reset/Kconfig, but
> maybe if all the current config options in there are non-AMBA we should
> wrap them in 'if VFIO_PLATFORM' (or 'depends on' for each, but the 'if'
> is marginally cleaner).  Thanks,
Yes your v2 makes sense to me.

Eric
>
> Alex
>