Re: [PATCH v11 20/23] vfio: Add VFIO_DEVICE_[AT|DE]TACH_IOMMUFD_PT

[Alex Williamson <alex.williamson@redhat.com>]

On Wed, 24 May 2023 02:12:14 +0000
"Liu, Yi L" <yi.l.liu@intel.com> wrote:

> > From: Alex Williamson <alex.williamson@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@intel.com> wrote:
> >   
> > > > From: Alex Williamson <alex.williamson@redhat.com>
> > > > Sent: Tuesday, May 23, 2023 6:16 AM
> > > >
> > > > On Sat, 13 May 2023 06:28:24 -0700
> > > > Yi Liu <yi.l.liu@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@intel.com>
> > > > > Tested-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> > > > > Signed-off-by: Yi Liu <yi.l.liu@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?

If a user creates an ioas within an iommufd, attaches a device to that
ioas and populates it with mappings, wouldn't the user expect the
device to have access to and honor those mappings?  I think that's the
path we're headed down if we report a successful attach of a noiommu
device to an ioas.

We need to keep in mind that noiommu was meant to be a minimally
intrusive mechanism to provide a dummy vfio IOMMU backend and satisfy
the group requirements, solely for the purpose of making use of the
vfio device interface and without providing any DMA mapping services or
expectations.  IMO, an argument that we need the attach op to succeed in
order to avoid too much disruption in userspace code is nonsense.  On
the contrary, userspace needs to be very aware of this difference and
we shouldn't invest effort trying to make noiommu more convenient to
use.  It's inherently unsafe.

I'm not fond of what a mess noiommu has become with cdev, we're well
beyond the minimal code trickery of the legacy implementation.  I hate
to ask, but could we reiterate our requirements for noiommu as a part of
the native iommufd interface for vfio?  The nested userspace requirement
is gone now that hypervisors have vIOMMU support, so my assumption is
that this is only for bare metal systems without an IOMMU, which
ideally are less and less prevalent.  Are there any noiommu userspaces
that are actually going to adopt the noiommu cdev interface?  What
terrible things happen if noiommu only exists in the vfio group compat
interface to iommufd and at some distant point in the future dies when
that gets disabled?

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

Documentation never hurts.  This is such a specialized use case I'm not
sure we've bothered to do much documentation for noiommu previously.
Thanks,

Alex