[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: Wednesday, May 24, 2023 11:32 PM
> 
> On Wed, 24 May 2023 02:12:14 +0000
> "Liu, Yi L" <yi.l.liu at intel.com> wrote:
> 
> > > 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:
> > > > >
> > > > > >  	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.

makes sense. Let's just fail attach/detach for noiommu devices.

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

vIOMMU may introduce some performance deduction if there
are frequent map/unmap. As far as I know, some cloud service
providers are more willing to use noiommu mode within VM.
Besides the performance consideration, using a booting a VM
without vIOMMU is supposed to be more robust. But I'm not
sure if the noiommu userspace will adapt to cdev noiommu.
Perhaps yes if group may be deprecated in future.

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

Seems no, I didn't find special documentation for noiommu. Perhaps
a comment in the source code is enough. Depends on your taste.

Regards,
Yi Liu