On 2022/11/24 22:56, Jason Gunthorpe wrote:
On Thu, Nov 24, 2022 at 04:26:59AM -0800, Yi Liu wrote:+ kvm = vfio_device_get_group_kvm(device); + if (!kvm) { + ret = -EINVAL; + goto err_unuse_iommu; + }A null kvm is not an error.
makes sense. vfio not only serves for VM but also userspace drivers.
And looking at this along with following cdev patch, I think this organization is cleaner. Make it so the caller of the vfio_device_open does most of the group/device differences. We already have different call chains. keep the iommfd code in vfio_main.c's functions.
ok.
diff --git a/drivers/vfio/group.c b/drivers/vfio/group.c index 3a69839c65ff75..9b511055150cec 100644 --- a/drivers/vfio/group.c +++ b/drivers/vfio/group.c @@ -609,61 +609,32 @@ void vfio_device_group_unregister(struct vfio_device *device)int vfio_device_group_use_iommu(struct vfio_device *device){ + struct vfio_group *group = device->group; int ret = 0;- /*- * 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; - goto out_unlock; - } + lockdep_assert_held(&group->group_lock);- if (device->group->container) {- ret = vfio_group_use_container(device->group); - if (ret) - goto out_unlock; - vfio_device_container_register(device); - } else if (device->group->iommufd) { - ret = vfio_iommufd_bind(device, device->group->iommufd); - } + if (WARN_ON(!group->container)) + return -EINVAL;-out_unlock:- mutex_unlock(&device->group->group_lock); - return ret; + ret = vfio_group_use_container(group); + if (ret) + return ret; + vfio_device_container_register(device); + return 0; }void vfio_device_group_unuse_iommu(struct vfio_device *device)-{ - mutex_lock(&device->group->group_lock); - if (device->group->container) { - vfio_device_container_unregister(device); - vfio_group_unuse_container(device->group); - } else if (device->group->iommufd) { - vfio_iommufd_unbind(device); - } - mutex_unlock(&device->group->group_lock); -} - -struct kvm *vfio_device_get_group_kvm(struct vfio_device *device) { struct vfio_group *group = device->group;- mutex_lock(&group->group_lock);- if (!group->kvm) { - mutex_unlock(&group->group_lock); - return NULL; - } - /* group_lock is released in the vfio_group_put_kvm() */ - return group->kvm; -} + lockdep_assert_held(&group->group_lock);-void vfio_device_put_group_kvm(struct vfio_device *device)-{ - mutex_unlock(&device->group->group_lock); + if (WARN_ON(!group->container)) + return; + + vfio_device_container_unregister(device); + vfio_group_unuse_container(group); }/**diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c index 3108e92a5cb20b..f9386a34d584e2 100644 --- a/drivers/vfio/vfio_main.c +++ b/drivers/vfio/vfio_main.c @@ -364,9 +364,9 @@ 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_first_open(struct vfio_device *device, + struct iommufd_ctx *iommufd, struct kvm *kvm) { - struct kvm *kvm; int ret;lockdep_assert_held(&device->dev_set->lock);@@ -374,54 +374,56 @@ static int vfio_device_first_open(struct vfio_device *device) if (!try_module_get(device->dev->driver->owner)) return -ENODEV;- ret = vfio_device_group_use_iommu(device);+ if (iommufd) + ret = vfio_iommufd_bind(device, iommufd); + else + ret = vfio_device_group_use_iommu(device); if (ret) goto err_module_put;- kvm = vfio_device_get_group_kvm(device);- if (!kvm) { - ret = -EINVAL; - goto err_unuse_iommu; - } - device->kvm = kvm; if (device->ops->open_device) { ret = device->ops->open_device(device); if (ret) - goto err_container; + goto err_unuse_iommu; } - vfio_device_put_group_kvm(device); return 0;-err_container:- device->kvm = NULL; - vfio_device_put_group_kvm(device); err_unuse_iommu: - vfio_device_group_unuse_iommu(device); + if (iommufd) + vfio_iommufd_unbind(device); + else + vfio_device_group_unuse_iommu(device); err_module_put: module_put(device->dev->driver->owner); + device->kvm = NULL; 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);if (device->ops->close_device)device->ops->close_device(device); device->kvm = NULL; - vfio_device_group_unuse_iommu(device); + if (iommufd) + vfio_iommufd_unbind(device); + 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--; } @@ -430,22 +432,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);
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.
+ 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;
+}
+
+void vfio_device_close_group(struct vfio_device *device)
+{
+ mutex_lock(&device->group->group_lock);
+ vfio_device_close(device, device->group->iommufd);
+ mutex_unlock(&device->group->group_lock);
+}
+
above two functions should be put in group.c.
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;@@ -474,7 +507,7 @@ struct file *vfio_device_open_file(struct vfio_device *device)return filep;err_close_device:- vfio_device_close(device); + vfio_device_group_close(device), device->group->iommufd; err_out: return ERR_PTR(ret); } @@ -519,7 +552,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_close_group(device); > vfio_device_put_registration(device);
-- Regards, Yi Liu
