On 2022/11/25 20:37, Jason Gunthorpe wrote:
On Fri, Nov 25, 2022 at 04:57:27PM +0800, Yi Liu wrote:
+static int vfio_device_group_open(struct vfio_device *device)
+{
+ int ret;
+
+ mutex_lock(&device->group->group_lock);
now the group path holds group_lock first, and then device_set->lock.
this is different with existing code. is it acceptable? I had a quick
check with this change, basic test is good. no a-b-b-a locking issue.
I looked for a while and couldn't find a reason why it wouldn't be OK
ok, so updated this commit as below:
From dd236de34a4f736041456fb46bd4a5eab360681b Mon Sep 17 00:00:00 2001
From: Yi Liu <yi.l.liu@xxxxxxxxx>
Date: Wed, 2 Nov 2022 04:42:25 -0700
Subject: [PATCH 08/11] vfio: Refactor vfio_device_open/close()
With this refactor, vfio_device_open/close() is the common code to open
device for the current group path, and also the future vfio device cdev
path. It calls the vfio_device_first_open() and _last_close() to handle
the legacy container path and the compat iommufd path.
Current caller of vfio_device_open/close() are vfio_device_group_open
and vfio_device_group_close(). They take care of group_lock, iommufd_ctx
pointer and also kvm pointer.
Future caller in the vfio device cdev path just need to care about the
iommufd_ctx pointer and kvm pointer. This is not part of this commit.
This prepares for moving group specific code out of vfio_main.c and also
compiling out the group infrastructure in future.
Signed-off-by: Yi Liu <yi.l.liu@xxxxxxxxx>
---
drivers/vfio/vfio_main.c | 133 +++++++++++++++++++++++++--------------
1 file changed, 87 insertions(+), 46 deletions(-)
diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
index 6898a492acc0..b49c48f3bcc8 100644
--- a/drivers/vfio/vfio_main.c
+++ b/drivers/vfio/vfio_main.c
@@ -772,7 +772,38 @@ static bool vfio_assert_device_open(struct vfio_device
*device)
return !WARN_ON_ONCE(!READ_ONCE(device->open_count));
}
-static int vfio_device_first_open(struct vfio_device *device)
+static int vfio_device_group_use_iommu(struct vfio_device *device)
+{
+ struct vfio_group *group = device->group;
+ int ret = 0;
+
+ lockdep_assert_held(&group->group_lock);
+
+ if (WARN_ON(!group->container))
+ return -EINVAL;
+
+ ret = vfio_group_use_container(group);
+ if (ret)
+ return ret;
+ vfio_device_container_register(device);
+ return 0;
+}
+
+static void vfio_device_group_unuse_iommu(struct vfio_device *device)
+{
+ struct vfio_group *group = device->group;
+
+ lockdep_assert_held(&group->group_lock);
+
+ if (WARN_ON(!group->container))
+ return;
+
+ vfio_device_container_unregister(device);
+ vfio_group_unuse_container(group);
+}
+
+static int vfio_device_first_open(struct vfio_device *device,
+ struct iommufd_ctx *iommufd, struct kvm *kvm)
{
int ret;
@@ -781,77 +812,56 @@ static int vfio_device_first_open(struct vfio_device
*device)
if (!try_module_get(device->dev->driver->owner))
return -ENODEV;
- /*
- * Here we pass the KVM pointer with the group under the lock. If the
- * device driver will use it, it must obtain a reference and release it
- * during close_device.
- */
- mutex_lock(&device->group->group_lock);
- if (!vfio_group_has_iommu(device->group)) {
- ret = -EINVAL;
+ if (iommufd)
+ ret = vfio_iommufd_bind(device, iommufd);
+ else
+ ret = vfio_device_group_use_iommu(device);
+ if (ret)
goto err_module_put;
- }
- if (device->group->container) {
- ret = vfio_group_use_container(device->group);
- if (ret)
- goto err_module_put;
- vfio_device_container_register(device);
- } else if (device->group->iommufd) {
- ret = vfio_iommufd_bind(device, device->group->iommufd);
- if (ret)
- goto err_module_put;
- }
-
- device->kvm = device->group->kvm;
+ device->kvm = kvm;
if (device->ops->open_device) {
ret = device->ops->open_device(device);
if (ret)
- goto err_container;
+ goto err_unuse_iommu;
}
- mutex_unlock(&device->group->group_lock);
return 0;
-err_container:
+err_unuse_iommu:
device->kvm = NULL;
- if (device->group->container) {
- vfio_device_container_unregister(device);
- vfio_group_unuse_container(device->group);
- } else if (device->group->iommufd) {
+ if (iommufd)
vfio_iommufd_unbind(device);
- }
+ else
+ vfio_device_group_unuse_iommu(device);
err_module_put:
- mutex_unlock(&device->group->group_lock);
module_put(device->dev->driver->owner);
return ret;
}
-static void vfio_device_last_close(struct vfio_device *device)
+static void vfio_device_last_close(struct vfio_device *device,
+ struct iommufd_ctx *iommufd)
{
lockdep_assert_held(&device->dev_set->lock);
- mutex_lock(&device->group->group_lock);
if (device->ops->close_device)
device->ops->close_device(device);
device->kvm = NULL;
- if (device->group->container) {
- vfio_device_container_unregister(device);
- vfio_group_unuse_container(device->group);
- } else if (device->group->iommufd) {
+ if (iommufd)
vfio_iommufd_unbind(device);
- }
- mutex_unlock(&device->group->group_lock);
+ else
+ vfio_device_group_unuse_iommu(device);
module_put(device->dev->driver->owner);
}
-static int vfio_device_open(struct vfio_device *device)
+static int vfio_device_open(struct vfio_device *device,
+ struct iommufd_ctx *iommufd, struct kvm *kvm)
{
int ret = 0;
mutex_lock(&device->dev_set->lock);
device->open_count++;
if (device->open_count == 1) {
- ret = vfio_device_first_open(device);
+ ret = vfio_device_first_open(device, iommufd, kvm);
if (ret)
device->open_count--;
}
@@ -860,22 +870,53 @@ static int vfio_device_open(struct vfio_device *device)
return ret;
}
-static void vfio_device_close(struct vfio_device *device)
+static void vfio_device_close(struct vfio_device *device,
+ struct iommufd_ctx *iommufd)
{
mutex_lock(&device->dev_set->lock);
vfio_assert_device_open(device);
if (device->open_count == 1)
- vfio_device_last_close(device);
+ vfio_device_last_close(device, iommufd);
device->open_count--;
mutex_unlock(&device->dev_set->lock);
}
+static int vfio_device_group_open(struct vfio_device *device)
+{
+ int ret;
+
+ mutex_lock(&device->group->group_lock);
+ if (!vfio_group_has_iommu(device->group)) {
+ ret = -EINVAL;
+ goto out_unlock;
+ }
+
+ /*
+ * Here we pass the KVM pointer with the group under the lock. If the
+ * device driver will use it, it must obtain a reference and release it
+ * during close_device.
+ */
+ ret = vfio_device_open(device, device->group->iommufd,
+ device->group->kvm);
+
+out_unlock:
+ mutex_unlock(&device->group->group_lock);
+ return ret;
+}
+
+static void vfio_device_group_close(struct vfio_device *device)
+{
+ mutex_lock(&device->group->group_lock);
+ vfio_device_close(device, device->group->iommufd);
+ mutex_unlock(&device->group->group_lock);
+}
+
static struct file *vfio_device_open_file(struct vfio_device *device)
{
struct file *filep;
int ret;
- ret = vfio_device_open(device);
+ ret = vfio_device_group_open(device);
if (ret)
goto err_out;
@@ -904,7 +945,7 @@ static struct file *vfio_device_open_file(struct
vfio_device *device)
return filep;
err_close_device:
- vfio_device_close(device);
+ vfio_device_group_close(device);
err_out:
return ERR_PTR(ret);
}
@@ -1120,7 +1161,7 @@ static int vfio_device_fops_release(struct inode
*inode, struct file *filep)
{
struct vfio_device *device = filep->private_data;
- vfio_device_close(device);
+ vfio_device_group_close(device);
vfio_device_put_registration(device);
--
2.34.1
--
Regards,
Yi Liu
