diff mbox series

[02/10] vfio: Move vfio_device_assign_container() into vfio_device_first_open()

Message ID 2-v1-4991695894d8+211-vfio_iommufd_jgg@nvidia.com (mailing list archive)
State New, archived
Headers show
Series Connect VFIO to IOMMUFD | expand

Commit Message

Jason Gunthorpe Oct. 25, 2022, 6:17 p.m. UTC
The only thing this function does is assert the group has an assigned
container and incrs refcounts.

The overall model we have is that once a conatiner_users refcount is
incremented it cannot be de-assigned from the group -
vfio_group_ioctl_unset_container() will fail and the group FD cannot be
closed.

Thus we do not need to check this on evey device FD open, just the
first. Reorganize the code so that only the first open and last close
manages the container.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/vfio/container.c |  4 ++--
 drivers/vfio/vfio_main.c | 18 ++++++++----------
 2 files changed, 10 insertions(+), 12 deletions(-)

Comments

Tian, Kevin Nov. 1, 2022, 7:38 a.m. UTC | #1
> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Wednesday, October 26, 2022 2:17 AM
> 
> +err_container:
> +	vfio_device_unassign_container(device);
>  err_module_put:
>  	device->kvm = NULL;

err_module_put should be moved after nullifying device->kvm.

otherwise it looks good to me:

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Jason Gunthorpe Nov. 1, 2022, 12:14 p.m. UTC | #2
On Tue, Nov 01, 2022 at 07:38:47AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > Sent: Wednesday, October 26, 2022 2:17 AM
> > 
> > +err_container:
> > +	vfio_device_unassign_container(device);
> >  err_module_put:
> >  	device->kvm = NULL;
> 
> err_module_put should be moved after nullifying device->kvm.
> 
> otherwise it looks good to me:
> 
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>

Done, thanks

Jason
Yi Liu Nov. 1, 2022, 2:37 p.m. UTC | #3
On 2022/10/26 02:17, Jason Gunthorpe wrote:
> The only thing this function does is assert the group has an assigned
> container and incrs refcounts.
> 
> The overall model we have is that once a conatiner_users refcount is

typo.

s/conatiner_users/container_users

> incremented it cannot be de-assigned from the group -
> vfio_group_ioctl_unset_container() will fail and the group FD cannot be
> closed.
> 
> Thus we do not need to check this on evey device FD open, just the

s/evey/every

> first. Reorganize the code so that only the first open and last close
> manages the container.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>   drivers/vfio/container.c |  4 ++--
>   drivers/vfio/vfio_main.c | 18 ++++++++----------
>   2 files changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/vfio/container.c b/drivers/vfio/container.c
> index d74164abbf401d..dd79a66ec62cad 100644
> --- a/drivers/vfio/container.c
> +++ b/drivers/vfio/container.c
> @@ -531,11 +531,11 @@ int vfio_device_assign_container(struct vfio_device *device)
>   
>   void vfio_device_unassign_container(struct vfio_device *device)
>   {
> -	mutex_lock(&device->group->group_lock);
> +	lockdep_assert_held_write(&device->group->group_lock);
> +
>   	WARN_ON(device->group->container_users <= 1);
>   	device->group->container_users--;
>   	fput(device->group->opened_file);
> -	mutex_unlock(&device->group->group_lock);
>   }
>   
>   /*
> diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
> index d043383fc3ba2b..204443ba3b3cd9 100644
> --- a/drivers/vfio/vfio_main.c
> +++ b/drivers/vfio/vfio_main.c
> @@ -749,16 +749,22 @@ static int vfio_device_first_open(struct vfio_device *device)
>   	 * it during close_device.
>   	 */
>   	mutex_lock(&device->group->group_lock);
> +	ret = vfio_device_assign_container(device);
> +	if (ret)
> +		goto err_module_put;
> +
>   	device->kvm = device->group->kvm;
>   	if (device->ops->open_device) {
>   		ret = device->ops->open_device(device);
>   		if (ret)
> -			goto err_module_put;
> +			goto err_container;
>   	}
>   	vfio_device_container_register(device);
>   	mutex_unlock(&device->group->group_lock);
>   	return 0;
>   
> +err_container:
> +	vfio_device_unassign_container(device);
>   err_module_put:
>   	device->kvm = NULL;
>   	mutex_unlock(&device->group->group_lock);
> @@ -775,6 +781,7 @@ static void vfio_device_last_close(struct vfio_device *device)
>   	if (device->ops->close_device)
>   		device->ops->close_device(device);
>   	device->kvm = NULL;
> +	vfio_device_unassign_container(device);
>   	mutex_unlock(&device->group->group_lock);
>   	module_put(device->dev->driver->owner);
>   }
> @@ -784,12 +791,6 @@ static struct file *vfio_device_open(struct vfio_device *device)
>   	struct file *filep;
>   	int ret;
>   
> -	mutex_lock(&device->group->group_lock);
> -	ret = vfio_device_assign_container(device);
> -	mutex_unlock(&device->group->group_lock);
> -	if (ret)
> -		return ERR_PTR(ret);
> -
>   	mutex_lock(&device->dev_set->lock);
>   	device->open_count++;
>   	if (device->open_count == 1) {
> @@ -833,7 +834,6 @@ static struct file *vfio_device_open(struct vfio_device *device)
>   err_unassign_container:

should the err_unassign_container be renamed to be err_dec_count?

other parts look good to me.

Reviewed-by: Yi Liu <yi.l.liu@intel.com>

>   	device->open_count--;
>   	mutex_unlock(&device->dev_set->lock);
> -	vfio_device_unassign_container(device);
>   	return ERR_PTR(ret);
>   }
>   
> @@ -1040,8 +1040,6 @@ static int vfio_device_fops_release(struct inode *inode, struct file *filep)
>   	device->open_count--;
>   	mutex_unlock(&device->dev_set->lock);
>   
> -	vfio_device_unassign_container(device);
> -
>   	vfio_device_put_registration(device);
>   
>   	return 0;
Jason Gunthorpe Nov. 1, 2022, 5:37 p.m. UTC | #4
On Tue, Nov 01, 2022 at 10:37:14PM +0800, Yi Liu wrote:
> > @@ -784,12 +791,6 @@ static struct file *vfio_device_open(struct vfio_device *device)
> >   	struct file *filep;
> >   	int ret;
> > -	mutex_lock(&device->group->group_lock);
> > -	ret = vfio_device_assign_container(device);
> > -	mutex_unlock(&device->group->group_lock);
> > -	if (ret)
> > -		return ERR_PTR(ret);
> > -
> >   	mutex_lock(&device->dev_set->lock);
> >   	device->open_count++;
> >   	if (device->open_count == 1) {
> > @@ -833,7 +834,6 @@ static struct file *vfio_device_open(struct vfio_device *device)
> >   err_unassign_container:
> 
> should the err_unassign_container be renamed to be err_dec_count?

Yes, I went with err_unlock

Thanks,
Jason
diff mbox series

Patch

diff --git a/drivers/vfio/container.c b/drivers/vfio/container.c
index d74164abbf401d..dd79a66ec62cad 100644
--- a/drivers/vfio/container.c
+++ b/drivers/vfio/container.c
@@ -531,11 +531,11 @@  int vfio_device_assign_container(struct vfio_device *device)
 
 void vfio_device_unassign_container(struct vfio_device *device)
 {
-	mutex_lock(&device->group->group_lock);
+	lockdep_assert_held_write(&device->group->group_lock);
+
 	WARN_ON(device->group->container_users <= 1);
 	device->group->container_users--;
 	fput(device->group->opened_file);
-	mutex_unlock(&device->group->group_lock);
 }
 
 /*
diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
index d043383fc3ba2b..204443ba3b3cd9 100644
--- a/drivers/vfio/vfio_main.c
+++ b/drivers/vfio/vfio_main.c
@@ -749,16 +749,22 @@  static int vfio_device_first_open(struct vfio_device *device)
 	 * it during close_device.
 	 */
 	mutex_lock(&device->group->group_lock);
+	ret = vfio_device_assign_container(device);
+	if (ret)
+		goto err_module_put;
+
 	device->kvm = device->group->kvm;
 	if (device->ops->open_device) {
 		ret = device->ops->open_device(device);
 		if (ret)
-			goto err_module_put;
+			goto err_container;
 	}
 	vfio_device_container_register(device);
 	mutex_unlock(&device->group->group_lock);
 	return 0;
 
+err_container:
+	vfio_device_unassign_container(device);
 err_module_put:
 	device->kvm = NULL;
 	mutex_unlock(&device->group->group_lock);
@@ -775,6 +781,7 @@  static void vfio_device_last_close(struct vfio_device *device)
 	if (device->ops->close_device)
 		device->ops->close_device(device);
 	device->kvm = NULL;
+	vfio_device_unassign_container(device);
 	mutex_unlock(&device->group->group_lock);
 	module_put(device->dev->driver->owner);
 }
@@ -784,12 +791,6 @@  static struct file *vfio_device_open(struct vfio_device *device)
 	struct file *filep;
 	int ret;
 
-	mutex_lock(&device->group->group_lock);
-	ret = vfio_device_assign_container(device);
-	mutex_unlock(&device->group->group_lock);
-	if (ret)
-		return ERR_PTR(ret);
-
 	mutex_lock(&device->dev_set->lock);
 	device->open_count++;
 	if (device->open_count == 1) {
@@ -833,7 +834,6 @@  static struct file *vfio_device_open(struct vfio_device *device)
 err_unassign_container:
 	device->open_count--;
 	mutex_unlock(&device->dev_set->lock);
-	vfio_device_unassign_container(device);
 	return ERR_PTR(ret);
 }
 
@@ -1040,8 +1040,6 @@  static int vfio_device_fops_release(struct inode *inode, struct file *filep)
 	device->open_count--;
 	mutex_unlock(&device->dev_set->lock);
 
-	vfio_device_unassign_container(device);
-
 	vfio_device_put_registration(device);
 
 	return 0;