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

From: Alex Williamson <alex.williamson@redhat.com>
To: Jason Gunthorpe <jgg@nvidia.com>
Cc: kvm@vger.kernel.org, clg@redhat.com, liulongfang@huawei.com,
	shameerali.kolothum.thodi@huawei.com, yishaih@nvidia.com,
	kevin.tian@intel.com
Subject: Re: [PATCH 1/3] vfio/pci: Cleanup Kconfig
Date: Mon, 5 Jun 2023 13:25:18 -0600	[thread overview]
Message-ID: <20230605132518.2d536373.alex.williamson@redhat.com> (raw)
In-Reply-To: <ZH4U6ElPSC3wIp1E@nvidia.com>

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