Re: [PATCH v3 03/16] iommufd/viommu: Add IOMMU_VDEVICE_ALLOC ioctl

[Nicolin Chen <nicolinc@xxxxxxxxxx>]

On Thu, Oct 17, 2024 at 03:52:30PM -0300, Jason Gunthorpe wrote:
> > +	if (viommu->ops && viommu->ops->vdevice_alloc)
> > +		vdev = viommu->ops->vdevice_alloc(viommu, idev->dev, virt_id);
> > +	else
> > +		vdev = __iommufd_vdevice_alloc(ucmd->ictx, sizeof(*vdev));
> > +	if (IS_ERR(vdev)) {
> > +		rc = PTR_ERR(vdev);
> > +		goto out_unlock_igroup;
> > +	}
> > +
> > +	vdev->idev = idev;
> > +	vdev->id = virt_id;
> > +	vdev->viommu = viommu;
> > +
> > +	idev->vdev = vdev;
> > +	refcount_inc(&idev->obj.users);
> > +	refcount_inc(&viommu->obj.users);
> > +
> > +	curr = xa_cmpxchg(&viommu->vdevs, virt_id, NULL, vdev, GFP_KERNEL);
> > +	if (curr) {
> > +		rc = xa_err(curr) ? : -EBUSY;
> > +		goto out_abort;
> > +	}
> > +
> > +	cmd->out_vdevice_id = vdev->obj.id;
> > +	rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd));
> > +	if (rc)
> > +		goto out_abort;
> > +	iommufd_object_finalize(ucmd->ictx, &vdev->obj);
> > +	goto out_unlock_igroup;
> > +
> > +out_abort:
> > +	iommufd_object_abort_and_destroy(ucmd->ictx, &vdev->obj);
> 
> But be mindful of this abort, it doesn't want to be inside the lock if
> it also gets the lock.. fail_nth should be updated to cover these new
> ioctls to look for tricky things like that

I added an abort() beside destroy():

+void iommufd_vdevice_abort(struct iommufd_object *obj)
+{
+       struct iommufd_vdevice *old, *vdev =
+               container_of(obj, struct iommufd_vdevice, obj);
+       struct iommufd_viommu *viommu = vdev->viommu;
+       struct iommufd_device *idev = vdev->idev;
+
+       lockdep_assert_not_held(&idev->igroup->lock);
+
+       if (viommu->ops && viommu->ops->vdevice_free)
+               viommu->ops->vdevice_free(vdev);
+
+       old = xa_cmpxchg(&viommu->vdevs, vdev->id, vdev, NULL, GFP_KERNEL);
+       if (old)
+               WARN_ON(old != vdev);
+
+       refcount_dec(&viommu->obj.users);
+       refcount_dec(&idev->obj.users);
+       idev->vdev = NULL;
+}
+
+void iommufd_vdevice_destroy(struct iommufd_object *obj)
+{
+       struct iommufd_vdevice *vdev =
+               container_of(obj, struct iommufd_vdevice, obj);
+
+       mutex_lock(&vdev->idev->igroup->lock);
+       iommufd_vdevice_abort(obj);
+       mutex_unlock(&vdev->idev->igroup->lock);
+}
----------------------------------------------------------

And I added fail_nth coverage for IOMMU_VIOMMU/VDEVICE_ALLOC cmds.

Thanks
Nicolin