> From: Jason Gunthorpe <jgg@xxxxxxxxxx>
> Sent: Tuesday, February 28, 2023 10:38 PM
>
> On Tue, Feb 28, 2023 at 02:01:36PM +0000, Liu, Yi L wrote:
> > > From: Jason Gunthorpe <jgg@xxxxxxxxxx>
> > > Sent: Tuesday, February 28, 2023 9:44 PM
> > >
> > > On Tue, Feb 28, 2023 at 01:36:24PM +0000, Liu, Yi L wrote:
> > > > > From: Jason Gunthorpe <jgg@xxxxxxxxxx>
> > > > > Sent: Tuesday, February 28, 2023 9:26 PM
> > > > >
> > > > > On Tue, Feb 28, 2023 at 01:22:50PM +0000, Liu, Yi L wrote:
> > > > >
> > > > > > > A null iommufd pointer and a bound df flag is sufficient to see
> that
> > > > > > > it is compat mode.
> > > > > >
> > > > > > Hope df->is_cdev_device suits your expectation.:-) The code will
> look
> > > > > > like below:
> > > > >
> > > > > Yes, this is better.. However I'd suggest 'uses_container' as it is
> > > > > clearer what the special case is
> > > >
> > > > Surely doable. Need to add a helper like below:
> > > >
> > > > bool vfio_device_group_uses_container()
> > > > {
> > > > lockdep_assert_held(&device->group->group_lock);
> > > > return device->group->container;
> > > > }
> > >
> > > It should come from the df.
> > >
> > > If you have a df then by definition:
> > > smp_load_acquire(..) == false - Not bound
> > > df->device->iommufd_ctx != NULL - Using iommufd
> > > df->group->containter != NULL - Using legacy container
> > > all other cases - NO_IOMMU
> > >
> > > No locking required since all these cases after the smp_load_acquire
> > > must be fixed for the lifetime of the df.
> >
> > Do you mean the df->access_granted (introduced in patch 07) or a new
> > flag?
>
> yes
>
> > Following your suggestion, it seems a mandatory requirement to do the
> > smp_load_acquire(..) == false check first, and then call into the
> vfio_device_open()
> > which further calls vfio_device_first_open() to check the iommufd/
> > legacy container/noiommu stuffs. Is it?
>
> Figuring out if an open should happen or not is a different operation,
> you already build exclusion between cdev/group so we don't need to
> care about the open path.
Ok.
> > df->group->containter this may need a helper to avoid decoding group
> > field. May be just store container in df?
>
> At worst a flag, but a helper seems like a good idea anyhow, then it
> can be compiled out
I add a separate commit as below. vfio_device_group_uses_container() is
added.
>From 0ce86e6b71d1884e9f5de30ba23e3aa93cc84db9 Mon Sep 17 00:00:00 2001
From: Yi Liu <yi.l.liu@xxxxxxxxx>
Date: Wed, 1 Mar 2023 02:24:43 -0800
Subject: [PATCH 15/22] vfio: Make vfio_device_first_open() to cover the
noiommu mode in cdev path
vfio_device_first_open() now covers the below two cases:
1) user uses iommufd (e.g. the group path in iommufd compat mode);
2) user uses container (e.g. the group path in legacy mode);
The above two paths have their own noiommu mode support accordingly.
The cdev path also uses iommufd, so for the case user provides a valid
iommufd, this helper is able to support it. But for noiommu mode, the
cdev path just provides a NULL iommufd. So this needs to be able to cover
it. As there is no special things to do for the cdev path in noiommu
mode, it can be covered by simply differentiate it from the container
case. If user is not using iommufd nor container, it is the noiommu
mode.
Signed-off-by: Yi Liu <yi.l.liu@xxxxxxxxx>
---
drivers/vfio/group.c | 5 +++++
drivers/vfio/vfio.h | 1 +
drivers/vfio/vfio_main.c | 19 ++++++++++++++++---
3 files changed, 22 insertions(+), 3 deletions(-)
diff --git a/drivers/vfio/group.c b/drivers/vfio/group.c
index 2a13442add43..ed3ffe7ceb3f 100644
--- a/drivers/vfio/group.c
+++ b/drivers/vfio/group.c
@@ -777,6 +777,11 @@ void vfio_device_group_unregister(struct vfio_device *device)
mutex_unlock(&device->group->device_lock);
}
+bool vfio_device_group_uses_container(struct vfio_device *device)
+{
+ return READ_ONCE(device->group->container);
+}
+
int vfio_device_group_use_iommu(struct vfio_device *device)
{
struct vfio_group *group = device->group;
diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h
index 68d35e1d7b87..e1f5a0310551 100644
--- a/drivers/vfio/vfio.h
+++ b/drivers/vfio/vfio.h
@@ -95,6 +95,7 @@ int vfio_device_set_group(struct vfio_device *device,
void vfio_device_remove_group(struct vfio_device *device);
void vfio_device_group_register(struct vfio_device *device);
void vfio_device_group_unregister(struct vfio_device *device);
+bool vfio_device_group_uses_container(struct vfio_device *device);
int vfio_device_group_use_iommu(struct vfio_device *device);
void vfio_device_group_unuse_iommu(struct vfio_device *device);
void vfio_device_group_close(struct vfio_device_file *df);
diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
index 121a75fadceb..4b5b17e8aaa1 100644
--- a/drivers/vfio/vfio_main.c
+++ b/drivers/vfio/vfio_main.c
@@ -422,9 +422,22 @@ static int vfio_device_first_open(struct vfio_device_file *df)
if (!try_module_get(device->dev->driver->owner))
return -ENODEV;
+ /*
+ * The handling here depends on what the user is using.
+ *
+ * If user uses iommufd in the group compat mode or the
+ * cdev path, call vfio_iommufd_bind().
+ *
+ * If user uses container in the group legacy mode, call
+ * vfio_device_group_use_iommu().
+ *
+ * If user doesn't use iommufd nor container, this is
+ * the noiommufd mode in the cdev path, nothing needs
+ * to be done here just go ahead to open device.
+ */
if (iommufd)
ret = vfio_iommufd_bind(device, iommufd);
- else
+ else if (vfio_device_group_uses_container(device))
ret = vfio_device_group_use_iommu(device);
if (ret)
goto err_module_put;
@@ -439,7 +452,7 @@ static int vfio_device_first_open(struct vfio_device_file *df)
err_unuse_iommu:
if (iommufd)
vfio_iommufd_unbind(device);
- else
+ else if (vfio_device_group_uses_container(device))
vfio_device_group_unuse_iommu(device);
err_module_put:
module_put(device->dev->driver->owner);
@@ -457,7 +470,7 @@ static void vfio_device_last_close(struct vfio_device_file *df)
device->ops->close_device(device);
if (iommufd)
vfio_iommufd_unbind(device);
- else
+ else if (vfio_device_group_uses_container(device))
vfio_device_group_unuse_iommu(device);
module_put(device->dev->driver->owner);
}
--
2.34.1
