[Intel-gfx] [PATCH v5 17/19] vfio: Add VFIO_DEVICE_AT[DE]TACH_IOMMUFD_PT

[Liu, Yi L]

> From: Jason Gunthorpe <jgg at nvidia.com>
> Sent: Tuesday, February 28, 2023 2:39 AM
> 
> On Mon, Feb 27, 2023 at 03:11:33AM -0800, Yi Liu wrote:
> > This adds ioctl for userspace to attach device cdev fd to and detach
> > from IOAS/hw_pagetable managed by iommufd.
> >
> >     VFIO_DEVICE_ATTACH_IOMMUFD_PT: attach vfio device to IOAS,
> hw_pagetable
> > 				   managed by iommufd. Attach can be
> > 				   undo by
> VFIO_DEVICE_DETACH_IOMMUFD_PT
> > 				   or device fd close.
> >     VFIO_DEVICE_DETACH_IOMMUFD_PT: detach vfio device from the
> current attached
> > 				   IOAS or hw_pagetable managed by
> iommufd.
> >
> > Signed-off-by: Yi Liu <yi.l.liu at intel.com>
> > Reviewed-by: Kevin Tian <kevin.tian at intel.com>
> > ---
> >  drivers/vfio/device_cdev.c | 76
> ++++++++++++++++++++++++++++++++++++++
> >  drivers/vfio/vfio.h        | 16 ++++++++
> >  drivers/vfio/vfio_main.c   |  8 ++++
> >  include/uapi/linux/vfio.h  | 52 ++++++++++++++++++++++++++
> >  4 files changed, 152 insertions(+)
> >
> > diff --git a/drivers/vfio/device_cdev.c b/drivers/vfio/device_cdev.c
> > index 37f80e368551..5b5a249a6612 100644
> > --- a/drivers/vfio/device_cdev.c
> > +++ b/drivers/vfio/device_cdev.c
> > @@ -191,6 +191,82 @@ long vfio_device_ioctl_bind_iommufd(struct
> vfio_device_file *df,
> >  	return ret;
> >  }
> >
> > +int vfio_ioctl_device_attach(struct vfio_device_file *df,
> > +			     void __user *arg)
> 
> This should be
> 
> struct vfio_device_attach_iommufd_pt __user *arg

Got it.

> > +{
> > +	struct vfio_device *device = df->device;
> > +	struct vfio_device_attach_iommufd_pt attach;
> > +	unsigned long minsz;
> > +	int ret;
> > +
> > +	minsz = offsetofend(struct vfio_device_attach_iommufd_pt, pt_id);
> > +
> > +	if (copy_from_user(&attach, (void __user *)arg, minsz))
> 
> No cast

Yes.

> > +		return -EFAULT;
> > +
> > +	if (attach.argsz < minsz || attach.flags ||
> > +	    attach.pt_id == IOMMUFD_INVALID_ID)
> > +		return -EINVAL;
> > +
> > +	if (!device->ops->bind_iommufd)
> > +		return -ENODEV;
> > +
> > +	mutex_lock(&device->dev_set->lock);
> > +	if (df->noiommu) {
> > +		ret = -EINVAL;
> > +		goto out_unlock;
> > +	}
> > +
> > +	ret = device->ops->attach_ioas(device, &attach.pt_id);
> > +	if (ret)
> > +		goto out_unlock;
> > +
> > +	ret = copy_to_user((void __user *)arg +
> > +			   offsetofend(struct
> vfio_device_attach_iommufd_pt, flags),
> 
> This should just be &arg->flags

Yes, can use arg->xxx here. I guess you mean &arg->pt_id.

> 
> > +			   &attach.pt_id,
> > +			   sizeof(attach.pt_id)) ? -EFAULT : 0;
> 
> Also:
> 
> static_assert(__same_type(arg->flags), attach.pt_id);

Got it. but s/arg->flags/arg->pt_id/

> > +	if (ret)
> > +		goto out_detach;
> > +	mutex_unlock(&device->dev_set->lock);
> > +
> > +	return 0;
> > +
> > +out_detach:
> > +	device->ops->detach_ioas(device);
> 
> 
> > +out_unlock:
> > +	mutex_unlock(&device->dev_set->lock);
> > +	return ret;
> > +}
> > +
> > +int vfio_ioctl_device_detach(struct vfio_device_file *df,
> > +			     void __user *arg)
> > +{
> > +	struct vfio_device *device = df->device;
> > +	struct vfio_device_detach_iommufd_pt detach;
> > +	unsigned long minsz;
> > +
> > +	minsz = offsetofend(struct vfio_device_detach_iommufd_pt, flags);
> > +
> > +	if (copy_from_user(&detach, (void __user *)arg, minsz))
> > +		return -EFAULT;
> 
> Same comments here

Sure.
 
> > +
> > +	if (detach.argsz < minsz || detach.flags)
> > +		return -EINVAL;
> > +
> > +	if (!device->ops->bind_iommufd)
> > +		return -ENODEV;
> > +
> > +	mutex_lock(&device->dev_set->lock);
> > +	if (df->noiommu) {
> > +		mutex_unlock(&device->dev_set->lock);
> > +		return -EINVAL;
> > +	}
> 
> This seems strange. no iommu mode should have a NULL dev->iommufctx.
> Why do we have a df->noiommu at all?

This is due to the vfio_device_first_open(). Detail as below comment (part of
patch 0016).

+	/*
+	 * For group/container path, iommufd pointer is NULL when comes
+	 * into this helper. Its noiommu support is handled by
+	 * vfio_device_group_use_iommu()
+	 *
+	 * For iommufd compat mode, iommufd pointer here is a valid value.
+	 * Its noiommu support is in vfio_iommufd_bind().
+	 *
+	 * For device cdev path, iommufd pointer here is a valid value for
+	 * normal cases, but it is NULL if it's noiommu. Check df->noiommu
+	 * to differentiate cdev noiommu from the group/container path which
+	 * also passes NULL iommufd pointer in. If set then do nothing.
+	 */
 	if (iommufd)
 		ret = vfio_iommufd_bind(device, iommufd, dev_id, pt_id);
-	else
+	else if (!df->noiommu)
 		ret = vfio_device_group_use_iommu(device);
 	if (ret)
 		goto err_module_put;

Regards,
Yi Liu