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

[Tian, Kevin]

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

Alex?

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

> 
> https://github.com/jgunthorpe/linux/commit/fa6abb318ccca114da12c0b5b1
> 23c99131ace926
> https://github.com/jgunthorpe/linux/commit/45980bd90b023d1eea56df70d
> 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.

Thanks
Kevin