Re: mechanism to allow a driver to bind to any device

[Alex Williamson <alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

On Wed, 2014-03-26 at 13:04 -0400, Konrad Rzeszutek Wilk wrote:
> On Wed, Mar 26, 2014 at 10:49:52AM -0600, Alex Williamson wrote:
> > On Wed, 2014-03-26 at 12:32 -0400, Konrad Rzeszutek Wilk wrote:
> > > On Wed, Mar 26, 2014 at 10:21:02AM -0600, Alex Williamson wrote:
> > > > On Wed, 2014-03-26 at 23:06 +0800, Alexander Graf wrote:
> > > > > 
> > > > > > Am 26.03.2014 um 22:40 schrieb Konrad Rzeszutek Wilk <konrad.wilk-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>:
> > > > > > 
> > > > > >> On Wed, Mar 26, 2014 at 01:40:32AM +0000, Stuart Yoder wrote:
> > > > > >> Hi Greg,
> > > > > >> 
> > > > > >> We (Linaro, Freescale, Virtual Open Systems) are trying get an issue
> > > > > >> closed that has been perculating for a while around creating a mechanism
> > > > > >> that will allow kernel drivers like vfio can bind to devices of any type.
> > > > > >> 
> > > > > >> This thread with you:
> > > > > >> http://www.spinics.net/lists/kvm-arm/msg08370.html
> > > > > >> ...seems to have died out, so am trying to get your response
> > > > > >> and will summarize again.  Vfio drivers in the kernel (regardless of
> > > > > >> bus type) need to bind to devices of any type.  The driver's function
> > > > > >> is to simply export hardware resources of any type to user space.
> > > > > >> 
> > > > > >> There are several approaches that have been proposed:
> > > > > > 
> > > > > > You seem to have missed the one I proposed.
> > > > > >> 
> > > > > >>   1.  new_id -- (current approach) the user explicitly registers
> > > > > >>       each new device type with the vfio driver using the new_id
> > > > > >>       mechanism.
> > > > > >> 
> > > > > >>       Problem: multiple drivers will be resident that handle the
> > > > > >>       same device type...and there is nothing user space hotplug
> > > > > >>       infrastructure can do to help.
> > > > > >> 
> > > > > >>   2.  "any id" -- the vfio driver could specify a wildcard match
> > > > > >>       of some kind in its ID match table which would allow it to
> > > > > >>       match and bind to any possible device id.  However,
> > > > > >>       we don't want the vfio driver grabbing _all_ devices...just the ones we
> > > > > >>       explicitly want to pass to user space.
> > > > > >> 
> > > > > >>       The proposed patch to support this was to create a new flag
> > > > > >>       "sysfs_bind_only" in struct device_driver.  When this flag
> > > > > >>       is set, the driver can only bind to devices via the sysfs
> > > > > >>       bind file.  This would allow the wildcard match to work.
> > > > > >> 
> > > > > >>       Patch is here:
> > > > > >>       https://lkml.org/lkml/2013/12/3/253
> > > > > >> 
> > > > > >>   3.  "Driver initiated explicit bind" -- with this approach the
> > > > > >>       vfio driver would create a private 'bind' sysfs object
> > > > > >>       and the user would echo the requested device into it:
> > > > > >> 
> > > > > >>       echo 0001:03:00.0 > /sys/bus/pci/drivers/vfio-pci/vfio_bind
> > > > > >> 
> > > > > >>       In order to make that work, the driver would need to call
> > > > > >>       driver_probe_device() and thus we need this patch:
> > > > > >>       https://lkml.org/lkml/2014/2/8/175
> > > > > > 
> > > > > > 4). Use the 'unbind' (from the original device) and 'bind' to vfio driver.
> > > > > 
> > > > > This is approach 2, no?
> > > > > 
> > > > > > 
> > > > > > Which I think is what is currently being done. Why is that not sufficient?
> > > > > 
> > > > > How would 'bind to vfio driver' look like?
> > > > > 
> > > > > > The only thing I see in the URL is " That works, but it is ugly."
> > > > > > There is some mention of race but I don't see how - if you do the 'unbind'
> > > > > > on the original driver and then bind the BDF to the VFIO how would you get
> > > > > > a race?
> > > > > 
> > > > > Typically on PCI, you do a
> > > > > 
> > > > >   - add wildcard (pci id) match to vfio driver
> > > > >   - unbind driver
> > > > >   -> reprobe
> > > > >   -> device attaches to vfio driver because it is the least recent match
> > > > >   - remove wildcard match from vfio driver
> > > > > 
> > > > > If in between you hotplug add a card of the same type, it gets attached to vfio - even though the logical "default driver" would be the device specific driver.
> > > > 
> > > > I've mentioned drivers_autoprobe in the past, but I'm not sure we're
> > > > really factoring it into the discussion.  drivers_autoprobe allows us to
> > > > toggle two points:
> > > > 
> > > > a) When a new device is added whether we automatically give drivers a
> > > > try at binding to it
> > > > 
> > > > b) When a new driver is added whether it gets to try to bind to anything
> > > > in the system
> > > > 
> > > > So we do have a mechanism to avoid the race, but the problem is that it
> > > > becomes the responsibility of userspace to:
> > > > 
> > > > 1) turn off drivers_autoprobe
> > > > 2) unbind/new_id/bind/remove_id
> > > > 3) turn on drivers_autoprobe
> > > > 4) call drivers_probe for anything added between 1) & 3)
> > > > 
> > > > Is the question about the ugliness of the current solution whether it's
> > > > unreasonable to ask userspace to do this?
> > > > 
> > > > What we seem to be asking for above is more like an autoprobe flag per
> > > > driver where there's some way for this special driver to opt out of auto
> > > > probing.  Option 2. in Stuart's list does this by short-cutting ID
> > > > matching so that a "match" is only found when using the sysfs bind path,
> > > > option 3. enables a way for a driver to expose their own sysfs entry
> > > > point for binding.  The latter feels particularly chaotic since drivers
> > > > get to make-up their own bind mechanism.
> > > > 
> > > > Another twist I'll throw in is that devices can be hot added to IOMMU
> > > > groups that are in-use by userspace.  When that happens we'd like to be
> > > > able to disable driver autoprobe of the device to avoid a host driver
> > > > automatically binding to the device.  I wonder if instead of looking at
> > > > the problem from the driver perspective, if we were to instead look at
> > > > it from the device perspective if we might find a solution that would
> > > > address both.  For instance, if devices had a driver_probe_id property
> > > > that was by default set to their bus specific ID match ("$VENDOR
> > > > $DEVICE" on PCI) could we use that to write new match IDs so that a
> > > > device could only bind to a given driver?  Effectively we could then
> > > > bind either using the current method of adding to the list of IDs a
> > > > driver will match of changing the ID that a device would match.  Does
> > > > that get us anywhere?  Thanks,
> > > 
> > > The other option for this is to having some sort of priority on the 
> > > device probing with hotplugging.
> > > 
> > > That is you can could do the following:
> > > 
> > >  1) add the device vendor/model in vfio
> > >  2) unbind the BDF from the original driver.
> > >  3) hotplug happens - any new device that has the device vendor/model gets
> > >    owned by vfio instead of the original device.
> > 
> > This doesn't help the device-added-to-inuse-group problem though because
> > we have no idea if the new device would have the same vendor/model as
> > other devices in the group.  By making the device probe ID modifiable,
> 
> Um, you add a hotplugged PCI device in a group that is in usage?

Sure, what if your IOMMU group is an entire conventional PCI
sub-hierarchy that supports hotplug.

> > vfio can watch the IOMMU group notifiers and change the probe ID of new
> 
> Ewwww.

How is that so terrible?  Is it worse than BUG()?

> > devices to either prevent the host driver from claiming them or to allow
> > vfio to claim them.  At the same time we change the problem from "this
> > driver can attach to this kind of device" to "this device can attach to
> > that driver", which also solves Stuart's problem.  Thanks,
> > 
> > Alex
> > 
> > >  4). bind the BDF to the vfio.
> > > 
> > > Granted that is a bit silly too - as the admin might want to have the new
> > > hotplugged device be owned by the native driver.
> > > 
> > > In which case, why not just switch out from using device vendor/model
> > > to just using BDF values?
> 
> Which would still solve the problem. The user-space would just have to
> reassign the device to the vfio group.

Sorry, I'm not really seeing how your proposal is different than what we
have already.  The steps above are exactly what we have today.  The
'new_slot' entry mentioned in reply to Stuart seems to be nothing more
than a PCI specific shortcut for new_id, but nothing substantive changes
about the binding path.  Thanks,

Alex