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

[Joao Martins <joao.m.martins@oracle.com>]

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.

> Perhaps (at least for ARM) they should even be coded
> 
>  select IOMMUFD_DRIVER if IOMMUFD
> 
> And then #ifdef out the dirty tracking bits so embedded systems don't
> get the bloat with !IOMMUFD
>
Right

>> 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

> ie the only way to get it is to build a driver that will consume it.
> 
>> diff --git a/drivers/iommu/iommufd/Makefile b/drivers/iommu/iommufd/Makefile
>> index 8aeba81800c5..34b446146961 100644
>> --- a/drivers/iommu/iommufd/Makefile
>> +++ b/drivers/iommu/iommufd/Makefile
>> @@ -11,3 +11,4 @@ iommufd-y := \
>>  iommufd-$(CONFIG_IOMMUFD_TEST) += selftest.o
>>
>>  obj-$(CONFIG_IOMMUFD) += iommufd.o
>> +obj-$(CONFIG_IOMMUFD_DRIVER) += iova_bitmap.o
> 
> Right..
> 
>> diff --git a/drivers/vfio/iova_bitmap.c b/drivers/iommu/iommufd/iova_bitmap.c
>> similarity index 100%
>> rename from drivers/vfio/iova_bitmap.c
>> rename to drivers/iommu/iommufd/iova_bitmap.c
>> diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig
>> index 6bda6dbb4878..1db519cce815 100644
>> --- 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;


> VFIO isn't a consumer of it
> 

(...) The select IOMMUFD_DRIVER was there because of VFIO PCI vendor drivers not
VFIO core. 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/

> The question you are asking is on the driver side implementing it, and
> it should be conditional if IOMMUFD is turned on.

For the IOMMU driver side sure IOMMUFD should be required as it's the sole entry
point of this

But as discussed earlier IOMMUFD=m|y dependency really only matters for IOMMU
dirty tracking, but not for VFIO device dirty tracking.

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