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

From: Eric Auger <eric.auger@redhat.com>
To: Alex Williamson <alex.williamson@redhat.com>
Cc: kvm@vger.kernel.org, jgg@nvidia.com, clg@redhat.com
Subject: Re: [PATCH 2/3] vfio/platform: Cleanup Kconfig
Date: Thu, 8 Jun 2023 10:51:10 +0200	[thread overview]
Message-ID: <f2bd4708-c14e-ff22-d27e-4ad9897b5de2@redhat.com> (raw)
In-Reply-To: <20230607130421.4e1b7ced.alex.williamson@redhat.com>


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
>