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

[Konrad Rzeszutek Wilk <konrad.wilk-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>]

On Wed, Mar 26, 2014 at 04:09:21PM -0600, Alex Williamson wrote:
> On Wed, 2014-03-26 at 10:21 -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,
> 
> Here's one way this might work for PCI; note that we can do this
> entirely in the bus driver for PCI.  Bind/unbind would go like this:
> 
> # bind device to vfio-pci
> echo vfio-pci > /sys/bus/pci/devices/0000\:03\:00.0/preferred_driver
> echo 0000:03:00.0 > /sys/bus/pci/devices/0000\:03\:00.0/driver/unbind
> echo 0000:03:00.0 > /sys/bus/pci/drivers_probe
> 
> # bind device back to host driver
> echo > /sys/bus/pci/devices/0000\:03\:00.0/preferred_driver
> echo 0000:03:00.0 > /sys/bus/pci/devices/0000\:03\:00.0/driver/unbind
> echo 0000:03:00.0 > /sys/bus/pci/drivers_probe
> 
> When preferred_driver is set for a device it will match and bind only to
> a driver with a matching name.  This also means we can write random
> strings here to avoid a device being bound to any driver if we want.
> 
> In the example patch below I've put the preferred_driver in the struct
> pci_dev, but if this mechanism were adopted by multiple devices perhaps
> we could add it to struct device.  Would something like this work for
> platform devices?
> 
> Note 1, the below is just the core PCI driver change to support this,
> there's some trivial collateral damage from changing an exported
> function not shown here for brevity.
> 
> Note 2, PCI passes a struct pci_device_id to the driver probe function
> which would be NULL in the preferred driver case of the example below.
> We'd need to dynamically create one of these when calling the probe
> function to make this practical for drivers that use that data.  Thanks,

That is I think a much easier way. Thought I would just call
it 'override' instead of preferred_driver, since well, that is its
intent.

Thank you for prototyping it!
> 
> Alex
> 
> Signed-off-by: Alex Williamson <alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> 
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index d911e0c..9425920 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -203,17 +203,23 @@ ATTRIBUTE_GROUPS(pci_drv);
>   * Deprecated, don't use this as it will not catch any dynamic ids
>   * that a driver might want to check for.
>   */
> -const struct pci_device_id *pci_match_id(const struct pci_device_id *ids,
> -					 struct pci_dev *dev)
> +int pci_match_id(const struct pci_device_id *ids, struct pci_dev *dev,
> +		 const struct pci_device_id **id)
>  {
> +	if (id)
> +		*id = NULL;
> +
>  	if (ids) {
>  		while (ids->vendor || ids->subvendor || ids->class_mask) {
> -			if (pci_match_one_device(ids, dev))
> -				return ids;
> +			if (pci_match_one_device(ids, dev)) {
> +				if (id)
> +					*id = ids;
> +				return 1;
> +			}
>  			ids++;
>  		}
>  	}
> -	return NULL;
> +	return 0;
>  }
>  
>  /**
> @@ -225,22 +231,30 @@ const struct pci_device_id *pci_match_id(const struct pci_device_id *ids,
>   * system is in its list of supported devices.  Returns the matching
>   * pci_device_id structure or %NULL if there is no match.
>   */
> -static const struct pci_device_id *pci_match_device(struct pci_driver *drv,
> -						    struct pci_dev *dev)
> +static int pci_match_device(struct pci_driver *drv, struct pci_dev *dev,
> +			    const struct pci_device_id **id)
>  {
>  	struct pci_dynid *dynid;
>  
> +	if (id)
> +		*id = NULL;
> +
> +	if (dev->preferred_driver)
> +		return !strcmp(drv->name, dev->preferred_driver);
> +
>  	/* Look at the dynamic ids first, before the static ones */
>  	spin_lock(&drv->dynids.lock);
>  	list_for_each_entry(dynid, &drv->dynids.list, node) {
>  		if (pci_match_one_device(&dynid->id, dev)) {
>  			spin_unlock(&drv->dynids.lock);
> -			return &dynid->id;
> +			if (id)
> +				*id = &dynid->id;
> +			return 1;
>  		}
>  	}
>  	spin_unlock(&drv->dynids.lock);
>  
> -	return pci_match_id(drv->id_table, dev);
> +	return pci_match_id(drv->id_table, dev, id);
>  }
>  
>  struct drv_dev_and_id {
> @@ -342,8 +356,7 @@ __pci_device_probe(struct pci_driver *drv, struct pci_dev *pci_dev)
>  	if (!pci_dev->driver && drv->probe) {
>  		error = -ENODEV;
>  
> -		id = pci_match_device(drv, pci_dev);
> -		if (id)
> +		if (pci_match_device(drv, pci_dev, &id))
>  			error = pci_call_probe(drv, pci_dev, id);
>  		if (error >= 0)
>  			error = 0;
> @@ -1272,17 +1285,12 @@ static int pci_bus_match(struct device *dev, struct device_driver *drv)
>  {
>  	struct pci_dev *pci_dev = to_pci_dev(dev);
>  	struct pci_driver *pci_drv;
> -	const struct pci_device_id *found_id;
>  
>  	if (!pci_dev->match_driver)
>  		return 0;
>  
>  	pci_drv = to_pci_driver(drv);
> -	found_id = pci_match_device(pci_drv, pci_dev);
> -	if (found_id)
> -		return 1;
> -
> -	return 0;
> +	return pci_match_device(pci_drv, pci_dev, NULL);
>  }
>  
>  /**
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 4e0acef..d6075f8 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -222,6 +222,46 @@ static ssize_t enabled_show(struct device *dev,
>  }
>  static DEVICE_ATTR_RW(enabled);
>  
> +static ssize_t preferred_driver_store(struct device *dev,
> +				      struct device_attribute *attr,
> +				      const char *buf, size_t count)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	char *preferred_driver, *old = pdev->preferred_driver;
> +
> +	if (count > PATH_MAX)
> +		return -EINVAL;
> +
> +	preferred_driver = kstrndup(buf, count, GFP_KERNEL);
> +	if (!preferred_driver)
> +		return -ENOMEM;
> +
> +	while (strlen(preferred_driver) &&
> +	       preferred_driver[strlen(preferred_driver) - 1] == '\n')
> +		preferred_driver[strlen(preferred_driver) - 1] = '\0';
> +
> +	if (strlen(preferred_driver)) {
> +		pdev->preferred_driver = preferred_driver;
> +	} else {
> +		kfree(preferred_driver);
> +		pdev->preferred_driver = NULL;
> +	}
> +			
> +	if (old)
> +		kfree(old);
> +
> +	return count;
> +}
> +
> +static ssize_t preferred_driver_show(struct device *dev,
> +				     struct device_attribute *attr, char *buf)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +
> +	return sprintf(buf, "%s\n", pdev->preferred_driver);
> +}
> +static DEVICE_ATTR_RW(preferred_driver);
> +
>  #ifdef CONFIG_NUMA
>  static ssize_t
>  numa_node_show(struct device *dev, struct device_attribute *attr, char *buf)
> @@ -521,6 +561,7 @@ static struct attribute *pci_dev_attrs[] = {
>  #if defined(CONFIG_PM_RUNTIME) && defined(CONFIG_ACPI)
>  	&dev_attr_d3cold_allowed.attr,
>  #endif
> +	&dev_attr_preferred_driver.attr,
>  	NULL,
>  };
>  
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index aab57b4..6fecb0a 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -365,6 +365,7 @@ struct pci_dev {
>  #endif
>  	phys_addr_t rom; /* Physical address of ROM if it's not from the BAR */
>  	size_t romlen; /* Length of ROM if it's not from the BAR */
> +	char *preferred_driver; /* Preferred driver, supercedes ID matching */
>  };
>  
>  static inline struct pci_dev *pci_physfn(struct pci_dev *dev)
> @@ -1111,8 +1112,8 @@ int pci_add_dynid(struct pci_driver *drv,
>  		  unsigned int subvendor, unsigned int subdevice,
>  		  unsigned int class, unsigned int class_mask,
>  		  unsigned long driver_data);
> -const struct pci_device_id *pci_match_id(const struct pci_device_id *ids,
> -					 struct pci_dev *dev);
> +int pci_match_id(const struct pci_device_id *ids, struct pci_dev *dev,
> +		 const struct pci_device_id **id);
>  int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max,
>  		    int pass);
>  
> 
> 
> _______________________________________________
> iommu mailing list
> iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu