Re: [RFC v2 08/11] vfio: Refactor vfio_device_first_open() and _last_close()

From: Yi Liu <yi.l.liu@intel.com>
To: Jason Gunthorpe <jgg@nvidia.com>
Cc: <alex.williamson@redhat.com>, <kevin.tian@intel.com>,
	<eric.auger@redhat.com>, <cohuck@redhat.com>,
	<nicolinc@nvidia.com>, <yi.y.sun@linux.intel.com>,
	<chao.p.peng@linux.intel.com>, <mjrosato@linux.ibm.com>,
	<kvm@vger.kernel.org>
Subject: Re: [RFC v2 08/11] vfio: Refactor vfio_device_first_open() and _last_close()
Date: Fri, 25 Nov 2022 22:06:13 +0800	[thread overview]
Message-ID: <f73ee554-807b-0123-bd93-3e3ba24feaaf@intel.com> (raw)
In-Reply-To: <Y4C28lraaKU1v8NE@nvidia.com>

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@intel.com>
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@intel.com>
---
  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
