Re: [PATCH v3 02/19] vfio: Move iova_bitmap into iommu core

[Jason Gunthorpe <jgg@nvidia.com>]

On Mon, Oct 16, 2023 at 06:52:50PM +0100, Joao Martins wrote:
> On 16/10/2023 17:34, Jason Gunthorpe wrote:
> > On Mon, Oct 16, 2023 at 05:25:16PM +0100, Joao Martins wrote:
> >>>> I think Jason is describing this would eventually be in a built-in
> >>>> portion of IOMMUFD, but I think currently that built-in portion is
> >>>> IOMMU.  So until we have this IOMMUFD_DRIVER that enables that built-in
> >>>> portion, it seems unnecessarily disruptive to make VFIO select IOMMUFD
> >>>> to get this iova bitmap support.  Thanks,
> >>>
> >>> Right, I'm saying Joao may as well make IOMMUFD_DRIVER right now for
> >>> this
> >>
> >> So far I have this snip at the end.
> >>
> >> Though given that there are struct iommu_domain changes that set a dirty_ops
> >> (which require iova-bitmap).
> > 
> > Drivers which set those ops need to select IOMMUFD_DRIVER..
> > 
> 
> My problem is more of the generic/vfio side (headers and structures of iommu
> core) not really IOMMU driver nor IOMMUFD.

As I said, just don't compile that stuff. If nothing else selects
IOMMFD_DRIVER then the core code has nothing to do.


> >> diff --git a/drivers/iommu/iommufd/Kconfig b/drivers/iommu/iommufd/Kconfig
> >> index 99d4b075df49..96ec013d1192 100644
> >> --- a/drivers/iommu/iommufd/Kconfig
> >> +++ b/drivers/iommu/iommufd/Kconfig
> >> @@ -11,6 +11,13 @@ config IOMMUFD
> >>
> >>           If you don't know what to do here, say N.
> >>
> >> +config IOMMUFD_DRIVER
> >> +       bool "IOMMUFD provides iommu drivers supporting functions"
> >> +       default IOMMU_API
> >> +       help
> >> +         IOMMUFD will provides supporting data structures and helpers to IOMMU
> >> +         drivers.
> > 
> > It is not a 'user selectable' kconfig, just make it
> > 
> > config IOMMUFD_DRIVER
> >        tristate
> >        default n
> > 
> tristate? More like a bool as IOMMU drivers aren't modloadable

tristate, who knows what people will select. If the modular drivers
use it then it is forced to a Y not a M. It is the right way to use kconfig..

> >> --- a/drivers/vfio/Kconfig
> >> +++ b/drivers/vfio/Kconfig
> >> @@ -7,6 +7,7 @@ menuconfig VFIO
> >>         select VFIO_GROUP if SPAPR_TCE_IOMMU || IOMMUFD=n
> >>         select VFIO_DEVICE_CDEV if !VFIO_GROUP
> >>         select VFIO_CONTAINER if IOMMUFD=n
> >> +       select IOMMUFD_DRIVER
> > 
> > As discussed use a if (IS_ENABLED) here and just disable the
> > bitmap code if something else didn't enable it.
> > 
> 
> I'm adding this to vfio_main:
> 
> 	if (!IS_ENABLED(CONFIG_IOMMUFD_DRIVER))
> 		return -EOPNOTSUPP;

Seems right
 
> > VFIO isn't a consumer of it
> > 
> 
> (...) The select IOMMUFD_DRIVER was there because of VFIO PCI vendor drivers not
> VFIO core. 

Those driver should individually select IOMMUFD_DRIVER

for the 'disable bitmap code' I can add ifdef-ry in iova_bitmap.h to
> add scalfold definitions to error-out/nop if CONFIG_IOMMUFD_DRIVER=n when moving
> to iommufd/

Yes that could also be a good approach

> For your suggested scheme to work VFIO PCI drivers still need to select
> IOMMUFD_DRIVER as they require it

Yes, of course

Jason