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

From: Eric Auger <eric.auger@redhat.com>
To: Alex Williamson <alex.williamson@redhat.com>, kvm@vger.kernel.org
Cc: jgg@nvidia.com, clg@redhat.com
Subject: Re: [PATCH 2/3] vfio/platform: Cleanup Kconfig
Date: Wed, 7 Jun 2023 15:32:07 +0200	[thread overview]
Message-ID: <7b4b8592-7857-b437-da06-2a6854fbf36b@redhat.com> (raw)
In-Reply-To: <20230602213315.2521442-3-alex.williamson@redhat.com>

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/