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

[Alex Williamson <alex.williamson@redhat.com>]

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