RE: RFC: (re-)binding the VFIO platform driver to a platform device

[Bhushan Bharat-R65777]

> -----Original Message-----
> From: kvm-owner@xxxxxxxxxxxxxxx [mailto:kvm-owner@xxxxxxxxxxxxxxx] On Behalf Of
> Kim Phillips
> Sent: Thursday, October 10, 2013 8:36 AM
> To: Wood Scott-B07421
> Cc: Yoder Stuart-B08248; Wood Scott-B07421; christoffer.dall@xxxxxxxxxx;
> alex.williamson@xxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> a.motakis@xxxxxxxxxxxxxxxxxxxxxx; agraf@xxxxxxx; Sethi Varun-B16395; Bhushan
> Bharat-R65777; peter.maydell@xxxxxxxxxx; santosh.shukla@xxxxxxxxxx;
> kvm@xxxxxxxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx
> Subject: Re: RFC: (re-)binding the VFIO platform driver to a platform device
> 
> On Wed, 9 Oct 2013 15:03:19 -0500
> Scott Wood <scottwood@xxxxxxxxxxxxx> wrote:
> 
> > On Wed, 2013-10-09 at 14:44 -0500, Yoder Stuart-B08248 wrote:
> > > > From: Wood Scott-B07421
> > > > Sent: Wednesday, October 09, 2013 2:22 PM
> > > >
> > > > On Wed, 2013-10-09 at 14:02 -0500, Yoder Stuart-B08248 wrote:
> > > > > Have been thinking about this issue some more.  As Scott
> > > > > mentioned,
> 
> thanks for bringing this up again.
> 
> > > > There's already a "bool suppress_bind_attrs" to prevent sysfs
> > > > bind/unbind.  I suggested a similar flag to mean the oppsosite --
> > > > bind
> > > > *only* through sysfs.  Greg KH was skeptical and wanted to see a
> > > > patch before any further discussion.
> > >
> > > Ah, think I understand now...yes that works as well, and would be
> > > less intrustive.   So are you writing a patch? :)
> >
> > I've been meaning to since the previous round of discussion, but I've
> > been busy.  Would someone else be able to test it in the context of
> > using it for VFIO?
> 
> yes - see below.
> 
> > Otherwise, that looks about right, for the driver side (though
> > driver_attach could error out earlier rather than testing it inside
> > the loop).
> 
> I've made the changes you suggested and tested the resulting diff below on an
> arndale board.  I successfully performed the following sequence of commands
> after first changing the i2c@12C80000 node in the device tree to be exclusively
> compatible with "vfio":
> 
> ===
> # ls -l /sys/bus/platform/drivers/vfio-platform/
> total 0
> --w------- 1 root root 4096 Sep 24 19:17 bind
> --w------- 1 root root 4096 Sep 24 19:13 uevent
> --w------- 1 root root 4096 Sep 24 19:18 unbind # ls -l
> /sys/bus/platform/drivers/s3c-i2c total 0
> lrwxrwxrwx 1 root root    0 Sep 24 19:11 12c60000.i2c ->
> ../../../../devices/12c60000.i2c
> lrwxrwxrwx 1 root root    0 Sep 24 19:11 12c90000.i2c ->
> ../../../../devices/12c90000.i2c
> lrwxrwxrwx 1 root root    0 Sep 24 19:20 12ce0000.i2c ->
> ../../../../devices/12ce0000.i2c
> --w------- 1 root root 4096 Sep 24 19:18 bind
> --w------- 1 root root 4096 Sep 24 19:11 uevent
> --w------- 1 root root 4096 Sep 24 19:17 unbind # ls -l
> /sys/devices/12c80000.i2c/driver  # this is the one with the 'vfio' compatible
> ls: cannot access /sys/devices/12c80000.i2c/driver: No such file or directory #
> ls -l /sys/devices/12ce0000.i2c/driver lrwxrwxrwx 1 root root 0 Sep 24 19:18
> /sys/devices/12ce0000.i2c/driver -> ../../bus/platform/drivers/s3c-i2c
> # echo 12ce0000.i2c > /sys/bus/platform/drivers/s3c-i2c/unbind
> # ls -l /sys/devices/12ce0000.i2c/driver
> ls: cannot access /sys/devices/12ce0000.i2c/driver: No such file or directory #
> echo 12ce0000.i2c > /sys/bus/platform/drivers/vfio-platform/bind
> # ls -l /sys/devices/12ce0000.i2c/driver lrwxrwxrwx 1 root root 0 Sep 24 19:21
> /sys/devices/12ce0000.i2c/driver -> ../../bus/platform/drivers/vfio-platform
> # echo 12ce0000.i2c > /sys/bus/platform/drivers/vfio-platform/unbind
> # ls -l /sys/devices/12ce0000.i2c/driver # echo 12ce0000.i2c >
> /sys/bus/platform/drivers/s3c-i2c/bind
> [  722.137524] s3c-i2c 12ce0000.i2c: slave address 0x38 [  722.141037] s3c-i2c
> 12ce0000.i2c: bus frequency set to 65 KHz [  722.150605] s3c-i2c 12ce0000.i2c:
> i2c-8: S3C I2C adapter # ls -l /sys/devices/12ce0000.i2c/driver lrwxrwxrwx 1
> root root 0 Sep 24 19:21 /sys/devices/12ce0000.i2c/driver ->
> ../../bus/platform/drivers/s3c-i2c
> #
> ====
> 
> so it's correctly not allowing 'vfio' driver to bind to a device tree compatible
> it's declared, and it then can bind the i2c @ 12ce0000 device to the vfio-
> platform driver, and unbind and bind it back to the i2c driver.
> 
> For clarity's sake, before this diff, the command:
> 
> echo 12ce0000.i2c > /sys/bus/platform/drivers/vfio-platform/bind
> 
> would error with:
> 
> echo: write error: No such device
> 
> > The other half of fixing the raciness is to ensure that the device
> > doesn't get bound back to a non-VFIO driver (e.g. due to a module load
> > or new_id).  The solution I proposed for that was a similar
> > explicit-bind-only flag for a device, that the user sets through sysfs
> > prior to unbinding.  This would also be useful in non-VFIO contexts to
> > simply say "I don't want to use this device at all".
> 
> I can take a look at doing this if you're still busy.
> 
> Thanks,
> 
> Kim
> 
> diff --git a/drivers/base/bus.c b/drivers/base/bus.c index 73f6c29..da81442
> 100644
> --- a/drivers/base/bus.c
> +++ b/drivers/base/bus.c
> @@ -201,7 +201,8 @@ static ssize_t bind_store(struct device_driver *drv, const
> char *buf,
>  	int err = -ENODEV;
> 
>  	dev = bus_find_device_by_name(bus, NULL, buf);
> -	if (dev && dev->driver == NULL && driver_match_device(drv, dev)) {
> +	if (dev && dev->driver == NULL && (drv->sysfs_bind_only ||
> +					   driver_match_device(drv, dev))) {

Should not we check 
      if (dev && dev->driver == NULL &&
          (device->explicit_bind_only && drv->explicit_bind_only) ||
          driver_match_device(drv, dev)))


>  		if (dev->parent)	/* Needed for USB */
>  			device_lock(dev->parent);
>  		device_lock(dev);
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c index 35fa368..6f85279 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -389,7 +389,7 @@ static int __device_attach(struct device_driver *drv, void
> *data)  {
>  	struct device *dev = data;
> 
> -	if (!driver_match_device(drv, dev))
> +	if (drv->sysfs_bind_only || !driver_match_device(drv, dev))

Likewise .. 

Thanks
-Bharat 

>  		return 0;
> 
>  	return driver_probe_device(drv, dev);
> @@ -476,6 +476,9 @@ static int __driver_attach(struct device *dev, void *data)
>   */
>  int driver_attach(struct device_driver *drv)  {
> +	if (drv->sysfs_bind_only)
> +		return 0;
> +
>  	return bus_for_each_dev(drv->bus, NULL, drv, __driver_attach);  }
> EXPORT_SYMBOL_GPL(driver_attach); diff --git a/drivers/vfio/vfio_platform.c
> b/drivers/vfio/vfio_platform.c index b92d7bb..ba578b2 100644
> --- a/drivers/vfio/vfio_platform.c
> +++ b/drivers/vfio/vfio_platform.c
> @@ -297,6 +297,7 @@ static struct platform_driver vfio_platform_driver = {
>  		.name	= "vfio-platform",
>  		.owner	= THIS_MODULE,
>  		.of_match_table = vfio_platform_match,
> +		.sysfs_bind_only = true,
>  	},
>  };
> 
> diff --git a/include/linux/device.h b/include/linux/device.h index
> 94638ef..a3ae81e 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -199,6 +199,7 @@ extern struct klist *bus_get_device_klist(struct bus_type
> *bus);
>   * @owner:	The module owner.
>   * @mod_name:	Used for built-in modules.
>   * @suppress_bind_attrs: Disables bind/unbind via sysfs.
> + * @sysfs_bind_only: Only allow bind/unbind via sysfs.
>   * @of_match_table: The open firmware table.
>   * @acpi_match_table: The ACPI match table.
>   * @probe:	Called to query the existence of a specific device,
> @@ -232,6 +233,7 @@ struct device_driver {
>  	const char		*mod_name;	/* used for built-in modules */
> 
>  	bool suppress_bind_attrs;	/* disables bind/unbind via sysfs */
> +	bool sysfs_bind_only;		/* only allow bind/unbind via sysfs */
> 
>  	const struct of_device_id	*of_match_table;
>  	const struct acpi_device_id	*acpi_match_table;
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a
> message to majordomo@xxxxxxxxxxxxxxx More majordomo info at
> http://vger.kernel.org/majordomo-info.html


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/