[Intel-gfx] [PATCH v11 20/23] vfio: Add VFIO_DEVICE_[AT|DE]TACH_IOMMUFD_PT

[Liu, Yi L]

> From: Alex Williamson <alex.williamson at redhat.com>
> Sent: Tuesday, May 23, 2023 11:50 PM
> 
> On Tue, 23 May 2023 01:20:17 +0000
> "Liu, Yi L" <yi.l.liu at intel.com> wrote:
> 
> > > From: Alex Williamson <alex.williamson at redhat.com>
> > > Sent: Tuesday, May 23, 2023 6:16 AM
> > >
> > > On Sat, 13 May 2023 06:28:24 -0700
> > > Yi Liu <yi.l.liu at intel.com> 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.
> > > >
> > > > Tested-by: Yanting Jiang <yanting.jiang at intel.com>
> > > > Tested-by: Shameer Kolothum <shameerali.kolothum.thodi at huawei.com>
> > > > Signed-off-by: Yi Liu <yi.l.liu at intel.com>
> > > > ---
> > > >  drivers/vfio/device_cdev.c | 66 ++++++++++++++++++++++++++++++++++++++
> > > >  drivers/vfio/iommufd.c     | 18 +++++++++++
> > > >  drivers/vfio/vfio.h        | 18 +++++++++++
> > > >  drivers/vfio/vfio_main.c   |  8 +++++
> > > >  include/uapi/linux/vfio.h  | 52 ++++++++++++++++++++++++++++++
> > > >  5 files changed, 162 insertions(+)
> > > >
> > > > diff --git a/drivers/vfio/device_cdev.c b/drivers/vfio/device_cdev.c
> > > > index 291cc678a18b..3f14edb80a93 100644
> > > > --- a/drivers/vfio/device_cdev.c
> > > > +++ b/drivers/vfio/device_cdev.c
> > > > @@ -174,6 +174,72 @@ long vfio_device_ioctl_bind_iommufd(struct
> vfio_device_file
> > > *df,
> > > >  	return ret;
> > > >  }
> > > >
> > > > +int vfio_ioctl_device_attach(struct vfio_device_file *df,
> > > > +			     struct vfio_device_attach_iommufd_pt __user *arg)
> > > > +{
> > > > +	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, arg, minsz))
> > > > +		return -EFAULT;
> > > > +
> > > > +	if (attach.argsz < minsz || attach.flags)
> > > > +		return -EINVAL;
> > > > +
> > > > +	/* ATTACH only allowed for cdev fds */
> > > > +	if (df->group)
> > > > +		return -EINVAL;
> > > > +
> > > > +	mutex_lock(&device->dev_set->lock);
> > > > +	ret = vfio_iommufd_attach(device, &attach.pt_id);
> > > > +	if (ret)
> > > > +		goto out_unlock;
> > > > +
> > > > +	ret = copy_to_user(&arg->pt_id, &attach.pt_id,
> > > > +			   sizeof(attach.pt_id)) ? -EFAULT : 0;
> > > > +	if (ret)
> > > > +		goto out_detach;
> > > > +	mutex_unlock(&device->dev_set->lock);
> > > > +
> > > > +	return 0;
> > > > +
> > > > +out_detach:
> > > > +	vfio_iommufd_detach(device);
> > > > +out_unlock:
> > > > +	mutex_unlock(&device->dev_set->lock);
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +int vfio_ioctl_device_detach(struct vfio_device_file *df,
> > > > +			     struct vfio_device_detach_iommufd_pt __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, arg, minsz))
> > > > +		return -EFAULT;
> > > > +
> > > > +	if (detach.argsz < minsz || detach.flags)
> > > > +		return -EINVAL;
> > > > +
> > > > +	/* DETACH only allowed for cdev fds */
> > > > +	if (df->group)
> > > > +		return -EINVAL;
> > > > +
> > > > +	mutex_lock(&device->dev_set->lock);
> > > > +	vfio_iommufd_detach(device);
> > > > +	mutex_unlock(&device->dev_set->lock);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > >  static char *vfio_device_devnode(const struct device *dev, umode_t *mode)
> > > >  {
> > > >  	return kasprintf(GFP_KERNEL, "vfio/devices/%s", dev_name(dev));
> > > > diff --git a/drivers/vfio/iommufd.c b/drivers/vfio/iommufd.c
> > > > index 83575b65ea01..799ea322a7d4 100644
> > > > --- a/drivers/vfio/iommufd.c
> > > > +++ b/drivers/vfio/iommufd.c
> > > > @@ -112,6 +112,24 @@ void vfio_iommufd_unbind(struct vfio_device_file *df)
> > > >  		vdev->ops->unbind_iommufd(vdev);
> > > >  }
> > > >
> > > > +int vfio_iommufd_attach(struct vfio_device *vdev, u32 *pt_id)
> > > > +{
> > > > +	lockdep_assert_held(&vdev->dev_set->lock);
> > > > +
> > > > +	if (vfio_device_is_noiommu(vdev))
> > > > +		return 0;
> > >
> > > Isn't this an invalid operation for a noiommu cdev, ie. -EINVAL?  We
> > > return success and copy back the provided pt_id, why would a user not
> > > consider it a bug that they can't use whatever value was there with
> > > iommufd?
> >
> > Yes, this is the question I asked in [1]. At that time, it appears to me
> > that better to allow it [2]. Maybe it's more suitable to ask it here.
> 
> From an API perspective it seems wrong.  We return success without
> doing anything.  A user would be right to consider it a bug that the
> attach operation works but there's not actually any association to the
> IOAS.  Thanks,

The current version is kind of tradeoff based on prior remarks when
I asked the question. As prior comment[2], it appears to me the attach
shall success for noiommu devices as well, but per your remark it seems
not in plan. So anyway, we may just fail the attach/detach for noiommu
devices. Is it?

btw. Should we document it somewhere as well? E.g. noiommu userspace
does not support attach/detach? Userspace should know it is opening
noiommu devices.

Regards,
Yi Liu