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

[Jason Gunthorpe <jgg@nvidia.com>]

On Wed, Oct 18, 2023 at 01:48:04PM +0100, Joao Martins wrote:
> On 18/10/2023 13:03, Jason Gunthorpe wrote:
> > On Wed, Oct 18, 2023 at 11:19:07AM +0100, Joao Martins wrote:
> >> On 16/10/2023 19:05, Jason Gunthorpe wrote:
> >>> 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:
> >>>>>> 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..
> >>>
> >> Making it tristate will break build bisection in this module with errors like this:
> >>
> >> [I say bisection, because aftewards when we put IOMMU drivers in the mix, these
> >> are always builtin, so it ends up selecting IOMMU_DRIVER=y.]
> >>
> >> ERROR: modpost: missing MODULE_LICENSE() in drivers/iommu/iommufd/iova_bitmap.o
> >>
> >> iova_bitmap is no module, and making it tristate allows to build it as a module
> >> as long as one of the selectors of is a module. 'bool' is actually more accurate
> >> to what it is builtin or not.
> > 
> > It is a module if you make it tristate, add the MODULE_LICENSE
> 
> It's not just that. It can't work as a module when CONFIG_VFIO=y and another
> user is CONFIG_MLX5_VFIO_PCI=m. CONFIG_VFIO uses the API so this is that case
> where IS_ENABLED(CONFIG_IOMMUFD_DRIVER) evaluates to true but it is only
> technically used by a module so it doesn't link it in. 

Ah! There is a well known kconfig technique for this too:
  depends on m || IOMMUFD_DRIVER != m
or 
  depends on IOMMUFD_DRIVER || IOMMUFD_DRIVER = n

On the VFIO module.

> I would like to reiterate that there's no actual module user, making a bool is a
> bit more clear on its usage on what it actually is (you would need IOMMU drivers
> to be modules, which I think is a big gamble that is happening anytime soon?)

This is all true too, but my thinking was to allow VFIO to use it
without having an IOMMU driver compiled in that supports dirty
tracking. eg for embedded cases.

Jason