RE: [RFC 06/20] iommu: Add iommu_device_init[exit]_user_dma interfaces

From: Tian, Kevin
Date: Mon Sep 27 2021 - 09:32:41 EST

> From: Jason Gunthorpe
> Sent: Monday, September 27, 2021 9:10 PM
> On Mon, Sep 27, 2021 at 01:00:08PM +0000, Tian, Kevin wrote:
> > > I think for such a narrow usage you should not change the struct
> > > device_driver. Just have pci_stub call a function to flip back to user
> > > mode.
> >
> > Here we want to ensure that kernel dma should be blocked
> > if the group is already marked for user-dma. If we just blindly
> > do it for any driver at this point (as you commented earlier):
> >
> > + ret = iommu_set_kernel_ownership(dev);
> > + if (ret)
> > + return ret;
> >
> > how would pci-stub reach its function to indicate that it doesn't
> > do dma and flip back?
> > Do you envision a simpler policy that no driver can be bound
> > to the group if it's already set for user-dma? what about vfio-pci
> > itself?
> Yes.. I'm not sure there is a good use case to allow the stub drivers
> to load/unload while a VFIO is running. At least, not a strong enough
> one to justify a global change to the driver core..

I'm fine with not loading pci-stub. From the very 1st commit msg
looks pci-stub was introduced before vfio to prevent host driver
loading when doing device assignment with KVM. I'm not sure
whether other usages are built on pci-stub later, but in general it's
not good to position devices in a same group into different usages.

but I'm little worried that even vfio-pci itself cannot be bound now,
which implies that all devices in a group which are intended to be
used by the user must be bound to vfio-pci in a breath before the
user attempts to open any of them, i.e. late-binding and device-
hotplug is disallowed after the initial open. I'm not sure how
important such an usage would be, but it does cause user-tangible
semantics change.


> > > > +static int iommu_dev_viable(struct device *dev, void *data)
> > > > +{
> > > > + enum dma_hint hint = *data;
> > > > + struct device_driver *drv = READ_ONCE(dev->driver);
> > >
> > > Especially since this isn't locked properly or safe.
> >
> > I have the same worry when copying from vfio. Not sure how
> > vfio gets safe with this approach...
> Fixing the locking in vfio_dev_viable is part of deleting the unbound
> list. Once it properly uses the device_lock and doesn't race with the
> driver core like this things are much better. Don't copy this stuff
> into the iommu core without fixing it.

sure. Above was just a quickly-baked sample code to match your

> 23c99131ace926
> 1c395bdf4cc7cf1
> I can't remember if the above is contingent on some of the mdev
> cleanups or not.. Have to get back to it.

my home network has some problem to access above links. Will check it
tomorrow and follow the fix when working on the formal change in
iommu core.