[Intel-gfx] [PATCH v12 21/24] vfio: Determine noiommu device in __vfio_register_dev()

[Liu, Yi L]

> From: Jason Gunthorpe <jgg at nvidia.com>
> Sent: Wednesday, June 14, 2023 8:23 PM
> On Wed, Jun 14, 2023 at 06:20:10AM +0000, Tian, Kevin wrote:
> > > From: Liu, Yi L <yi.l.liu at intel.com>
> > > Sent: Wednesday, June 14, 2023 2:14 PM
> > >
> > >
> > > > With that I think Jason's suggestion is to lift that test into main.c:
> > > >
> > > > int vfio_register_group_dev(struct vfio_device *device)
> > > > {
> > > > 	/*
> > > > 	 * VFIO always sets IOMMU_CACHE because we offer no way for
> > > userspace to
> > > > 	 * restore cache coherency. It has to be checked here because it is
> > > only
> > > > 	 * valid for cases where we are using iommu groups.
> > > > 	 */
> > > > 	if (type == VFIO_IOMMU && !vfio_device_is_noiommu(device) &&
> > > > 	    !device_iommu_capable(dev, IOMMU_CAP_CACHE_COHERENCY))
> > > > 		return ERR_PTR(-EINVAL);
> > >
> > > vfio_device_is_noiommu() needs to be called after vfio_device_set_group().
> > > Otherwise, it's always false. So still needs to call it in the
> > > __vfio_register_dev().
> >
> > yes
> 
> Right, but it needs to be in vfio_main.c, not in the group.c - so
> another patch should be added to move it.

I've got a patch as below to move it.

>From 306e71325d255eef34a1c44312bf9cdc8c302faa Mon Sep 17 00:00:00 2001
From: Yi Liu <yi.l.liu at intel.com>
Date: Wed, 14 Jun 2023 00:37:52 -0700
Subject: [PATCH] vfio: Move the IOMMU_CAP_CACHE_COHERENCY check in
 __vfio_register_dev()

The IOMMU_CAP_CACHE_COHERENCY check only applies to the physical devices
that are IOMMU-backed. This change prepares for compiling the vfio_group
infrastructure optionally as cdev does not support the physical devices
that are not IOMMU-backed. This check help to fail the device registration
for such devices if only vfio_group infrastructure is compiled out.

Signed-off-by: Yi Liu <yi.l.liu at intel.com>
---
 drivers/vfio/group.c     | 10 ----------
 drivers/vfio/vfio_main.c | 11 +++++++++++
 2 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/vfio/group.c b/drivers/vfio/group.c
index 41a09a2df690..c2e0128323a7 100644
--- a/drivers/vfio/group.c
+++ b/drivers/vfio/group.c
@@ -687,16 +687,6 @@ static struct vfio_group *vfio_group_find_or_alloc(struct device *dev)
 	if (!iommu_group)
 		return ERR_PTR(-EINVAL);
 
-	/*
-	 * VFIO always sets IOMMU_CACHE because we offer no way for userspace to
-	 * restore cache coherency. It has to be checked here because it is only
-	 * valid for cases where we are using iommu groups.
-	 */
-	if (!device_iommu_capable(dev, IOMMU_CAP_CACHE_COHERENCY)) {
-		iommu_group_put(iommu_group);
-		return ERR_PTR(-EINVAL);
-	}
-
 	mutex_lock(&vfio.group_lock);
 	group = vfio_group_find_from_iommu(iommu_group);
 	if (group) {
diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
index 51c80eb32af6..ffb4585b7f0e 100644
--- a/drivers/vfio/vfio_main.c
+++ b/drivers/vfio/vfio_main.c
@@ -292,6 +292,17 @@ static int __vfio_register_dev(struct vfio_device *device,
 	if (ret)
 		return ret;
 
+	/*
+	 * VFIO always sets IOMMU_CACHE because we offer no way for userspace to
+	 * restore cache coherency. It has to be checked here because it is only
+	 * valid for cases where we are using iommu groups.
+	 */
+	if (type == VFIO_IOMMU && !vfio_device_is_noiommu(device) &&
+	    !device_iommu_capable(device->dev, IOMMU_CAP_CACHE_COHERENCY)) {
+		ret = -EINVAL;
+		goto err_out;
+	}
+
 	ret = vfio_device_add(device);
 	if (ret)
 		goto err_out;
-- 
2.34.1

> I prefer the idea that vfio_device_is_noiommu() works in all the
> kconfig scenarios rather than adding #ifdefs.

But the vfio_group would be empty when CONFIG_VFIO_GROUP is unset.
>From what I got now, when CONFIG_VFIO_GROUP is unset, the stub
function always returns false.

#if IS_ENABLED(CONFIG_VFIO_GROUP)
struct vfio_group {
	...;
};

static inline bool vfio_device_is_noiommu(struct vfio_device *vdev)
{
        return IS_ENABLED(CONFIG_VFIO_NOIOMMU) &&
               vdev->group->type == VFIO_NO_IOMMU;
}
#else
struct vfio_group;

static inline bool vfio_device_is_noiommu(struct vfio_device *vdev)
{
        return false;
}
#endif

Regards,
Yi Liu