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

[Yi Liu]

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 <j...@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....@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