Re: [Intel-gfx] [PATCH 02/10] vfio: Move vfio_device_assign_container() into vfio_device_first_open()

Yi Liu
2022-11-01T14:37:03Z

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;

--
Regards,
Yi Liu