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: Tue, 6 Jun 2023 15:57:04 -0600	[thread overview]
Message-ID: <20230606155704.037a1f60.alex.williamson@redhat.com> (raw)
In-Reply-To: <ZH9BvcgHvX7HFBAa@nvidia.com>

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