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..
>
Got it (and thanks for the patience)
>>>> --- 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
>
OK -- this is the part that wasn't clear straight away.
So individually per driver not on VFIO_PCI_CORE in which these drivers depend
on? A lot of the dirty tracking stuff gets steered via what VFIO_PCI_CORE
allows, perhaps I can put the IOMMUFD_DRIVER selection there?
>> 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
Here's a diff, naturally AMD/Intel kconfigs would get a select IOMMUFD_DRIVER as
well later in the series
diff --git a/drivers/iommu/iommufd/Kconfig b/drivers/iommu/iommufd/Kconfig
index 99d4b075df49..4c6cb96a4b28 100644
--- a/drivers/iommu/iommufd/Kconfig
+++ b/drivers/iommu/iommufd/Kconfig
@@ -11,6 +11,10 @@ config IOMMUFD
If you don't know what to do here, say N.
+config IOMMUFD_DRIVER
+ tristate
+ default n
+
if IOMMUFD
config IOMMUFD_VFIO_CONTAINER
bool "IOMMUFD provides the VFIO container /dev/vfio/vfio"
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
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/Makefile b/drivers/vfio/Makefile
index c82ea032d352..68c05705200f 100644
--- a/drivers/vfio/Makefile
+++ b/drivers/vfio/Makefile
@@ -1,8 +1,7 @@
# SPDX-License-Identifier: GPL-2.0
obj-$(CONFIG_VFIO) += vfio.o
-vfio-y += vfio_main.o \
- iova_bitmap.o
+vfio-y += vfio_main.o
vfio-$(CONFIG_VFIO_DEVICE_CDEV) += device_cdev.o
vfio-$(CONFIG_VFIO_GROUP) += group.o
vfio-$(CONFIG_IOMMUFD) += iommufd.o
diff --git a/drivers/vfio/pci/mlx5/Kconfig b/drivers/vfio/pci/mlx5/Kconfig
index 7088edc4fb28..c3ced56b7787 100644
--- a/drivers/vfio/pci/mlx5/Kconfig
+++ b/drivers/vfio/pci/mlx5/Kconfig
@@ -3,6 +3,7 @@ config MLX5_VFIO_PCI
tristate "VFIO support for MLX5 PCI devices"
depends on MLX5_CORE
select VFIO_PCI_CORE
+ select IOMMUFD_DRIVER
help
This provides migration support for MLX5 devices using the VFIO
framework.
diff --git a/drivers/vfio/pci/pds/Kconfig b/drivers/vfio/pci/pds/Kconfig
index 407b3fd32733..fff368a8183b 100644
--- a/drivers/vfio/pci/pds/Kconfig
+++ b/drivers/vfio/pci/pds/Kconfig
@@ -5,6 +5,7 @@ config PDS_VFIO_PCI
tristate "VFIO support for PDS PCI devices"
depends on PDS_CORE
select VFIO_PCI_CORE
+ select IOMMUFD_DRIVER
help
This provides generic PCI support for PDS devices using the VFIO
framework.
diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
index 40732e8ed4c6..93b0c2b377e1 100644
--- a/drivers/vfio/vfio_main.c
+++ b/drivers/vfio/vfio_main.c
@@ -1095,6 +1095,9 @@ static int vfio_device_log_read_and_clear(struct
iova_bitmap *iter,
{
struct vfio_device *device = opaque;
+ if (!IS_ENABLED(CONFIG_IOMMUFD_DRIVER))
+ return -EOPNOTSUPP;
+
return device->log_ops->log_read_and_clear(device, iova, length, iter);
}
@@ -1111,6 +1114,9 @@ vfio_ioctl_device_feature_logging_report(struct
vfio_device *device,
u64 iova_end;
int ret;
+ if (!IS_ENABLED(CONFIG_IOMMUFD_DRIVER))
+ return -EOPNOTSUPP;
+
if (!device->log_ops)
return -ENOTTY;
diff --git a/include/linux/iova_bitmap.h b/include/linux/iova_bitmap.h
index c006cf0a25f3..1c338f5e5b7a 100644
--- a/include/linux/iova_bitmap.h
+++ b/include/linux/iova_bitmap.h
@@ -7,6 +7,7 @@
#define _IOVA_BITMAP_H_
#include <linux/types.h>
+#include <linux/errno.h>
struct iova_bitmap;
@@ -14,6 +15,7 @@ typedef int (*iova_bitmap_fn_t)(struct iova_bitmap *bitmap,
unsigned long iova, size_t length,
void *opaque);
+#if IS_ENABLED(CONFIG_IOMMUFD_DRIVER)
struct iova_bitmap *iova_bitmap_alloc(unsigned long iova, size_t length,
unsigned long page_size,
u64 __user *data);
@@ -22,5 +24,29 @@ int iova_bitmap_for_each(struct iova_bitmap *bitmap, void
*opaque,
iova_bitmap_fn_t fn);
void iova_bitmap_set(struct iova_bitmap *bitmap,
unsigned long iova, size_t length);
+#else
+static inline struct iova_bitmap *iova_bitmap_alloc(unsigned long iova,
+ size_t length,
+ unsigned long page_size,
+ u64 __user *data)
+{
+ return NULL;
+}
+
+static inline void iova_bitmap_free(struct iova_bitmap *bitmap)
+{
+}
+
+static inline int iova_bitmap_for_each(struct iova_bitmap *bitmap, void *opaque,
+ iova_bitmap_fn_t fn)
+{
+ return -EOPNOTSUPP;
+}
+
+static inline void iova_bitmap_set(struct iova_bitmap *bitmap,
+ unsigned long iova, size_t length)
+{
+}
+#endif
#endif
