[PATCH 1/2] PCI: allow pci driver to support only dynids

[PATCH 1/2] PCI: allow pci driver to support only dynids

[Chris Wright]

commit b41d6cf38e27 (PCI: Check dynids driver_data value for validity)
requires all drivers to include an id table to try and match
driver_data.  Before validating driver_data check driver has an id
table.

Cc: Jean Delvare <khali@linux-fr.org>
Cc: Milton Miller <miltonm@bga.com>
Signed-off-by: Chris Wright <chrisw@sous-sol.org>
---
 drivers/pci/pci-driver.c |   20 +++++++++++---------
 1 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index b4cdd69..0a5edbe 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -48,7 +48,7 @@ store_new_id(struct device_driver *driver, const char *buf, size_t count)
 		subdevice=PCI_ANY_ID, class=0, class_mask=0;
 	unsigned long driver_data=0;
 	int fields=0;
-	int retval;
+	int retval=0;
 
 	fields = sscanf(buf, "%x %x %x %x %x %x %lx",
 			&vendor, &device, &subvendor, &subdevice,
@@ -58,16 +58,18 @@ store_new_id(struct device_driver *driver, const char *buf, size_t count)
 
 	/* Only accept driver_data values that match an existing id_table
 	   entry */
-	retval = -EINVAL;
-	while (ids->vendor || ids->subvendor || ids->class_mask) {
-		if (driver_data == ids->driver_data) {
-			retval = 0;
-			break;
+	if (ids) {
+		retval = -EINVAL;
+		while (ids->vendor || ids->subvendor || ids->class_mask) {
+			if (driver_data == ids->driver_data) {
+				retval = 0;
+				break;
+			}
+			ids++;
 		}
-		ids++;
+		if (retval)	/* No match */
+			return retval;
 	}
-	if (retval)	/* No match */
-		return retval;
 
 	dynid = kzalloc(sizeof(*dynid), GFP_KERNEL);
 	if (!dynid)
-- 
1.6.0.3

[PATCH 2/2] PCI: pci-stub module to reserve pci device

[Chris Wright]

When doing device assignment with KVM there's currently nothing to
protect the device from having a driver in the host as well as the guest.
This trivial module just binds the pci device on the host to a stub
driver so that a real host driver can't bind to the device.  It has no
pci id table, it supports only dynamic ids.

 # echo "8086 10f5" > /sys/bus/pci/drivers/pci-stub/new_id
 # echo -n 0000:00:19.0 > /sys/bus/pci/drivers/e1000e/unbind
 # echo -n 0000:00:19.0 > /sys/bus/pci/drivers/pci-stub/bind
 # ls -l /sys/bus/pci/devices/0000:00:19.0/driver
 lrwxrwxrwx 1 root root 0 2008-11-25 19:10 /sys/bus/pci/devices/0000:00:19.0/driver -> ../../../bus/pci/drivers/pci-stub

Cc: "Kay, Allen M" <allen.m.kay@intel.com>
Cc: "Nakajima, Jun" <jun.nakajima@intel.com>
Signed-off-by: Chris Wright <chrisw@sous-sol.org>
---
 drivers/pci/Kconfig    |    9 +++++++++
 drivers/pci/Makefile   |    2 ++
 drivers/pci/pci-stub.c |   38 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 49 insertions(+), 0 deletions(-)
 create mode 100644 drivers/pci/pci-stub.c

diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
index e1ca425..f6183df 100644
--- a/drivers/pci/Kconfig
+++ b/drivers/pci/Kconfig
@@ -42,6 +42,15 @@ config PCI_DEBUG
 
 	  When in doubt, say N.
 
+config PCI_STUB
+	tristate "PCI Stub driver"
+	depends on PCI
+	help
+	  Say Y or M here if you want be able to reserve a PCI device
+	  when it is going to be assigned to a guest.
+
+	  When in doubt, say N.
+
 config HT_IRQ
 	bool "Interrupts on hypertransport devices"
 	default y
diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
index af3bfe2..3d07ce2 100644
--- a/drivers/pci/Makefile
+++ b/drivers/pci/Makefile
@@ -53,6 +53,8 @@ obj-$(CONFIG_HOTPLUG) += setup-bus.o
 
 obj-$(CONFIG_PCI_SYSCALL) += syscall.o
 
+obj-$(CONFIG_PCI_STUB) += pci-stub.o
+
 ifeq ($(CONFIG_PCI_DEBUG),y)
 EXTRA_CFLAGS += -DDEBUG
 endif
diff --git a/drivers/pci/pci-stub.c b/drivers/pci/pci-stub.c
new file mode 100644
index 0000000..8b19df6
--- /dev/null
+++ b/drivers/pci/pci-stub.c
@@ -0,0 +1,38 @@
+/* pci-stub - simple stub driver to reserve a pci device
+ *
+ * Copyright (C) 2008 Red Hat, Inc.
+ * Author:
+ * 	Chris Wright
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.
+ */
+
+#include <linux/module.h>
+#include <linux/pci.h>
+
+static int pci_stub_probe(struct pci_dev *dev, const struct pci_device_id *id)
+{
+	return 0;
+}
+
+static struct pci_driver stub_driver = {
+	.name		= "pci-stub",
+	.id_table	= NULL,	/* only dynamic id's */
+	.probe		= pci_stub_probe,
+};
+
+static int __init pci_stub_init(void)
+{
+	return pci_register_driver(&stub_driver);
+}
+
+static void __exit pci_stub_exit(void)
+{
+	pci_unregister_driver(&stub_driver);
+}
+
+module_init(pci_stub_init);
+module_exit(pci_stub_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Chris Wright <chrisw@sous-sol.org>");
-- 
1.6.0.3

Re: [PATCH 2/2] PCI: pci-stub module to reserve pci device

[Greg KH]

On Tue, Nov 25, 2008 at 07:38:39PM -0800, Chris Wright wrote:
> When doing device assignment with KVM there's currently nothing to
> protect the device from having a driver in the host as well as the guest.
> This trivial module just binds the pci device on the host to a stub
> driver so that a real host driver can't bind to the device.  It has no
> pci id table, it supports only dynamic ids.
> 
>  # echo "8086 10f5" > /sys/bus/pci/drivers/pci-stub/new_id
>  # echo -n 0000:00:19.0 > /sys/bus/pci/drivers/e1000e/unbind
>  # echo -n 0000:00:19.0 > /sys/bus/pci/drivers/pci-stub/bind
>  # ls -l /sys/bus/pci/devices/0000:00:19.0/driver
>  lrwxrwxrwx 1 root root 0 2008-11-25 19:10 /sys/bus/pci/devices/0000:00:19.0/driver -> ../../../bus/pci/drivers/pci-stub

You might want to put this somewhere in the .c or documentation files
somewhere as well.

> 
> Cc: "Kay, Allen M" <allen.m.kay@intel.com>
> Cc: "Nakajima, Jun" <jun.nakajima@intel.com>
> Signed-off-by: Chris Wright <chrisw@sous-sol.org>
> ---
>  drivers/pci/Kconfig    |    9 +++++++++
>  drivers/pci/Makefile   |    2 ++
>  drivers/pci/pci-stub.c |   38 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 49 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/pci/pci-stub.c
> 
> diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
> index e1ca425..f6183df 100644
> --- a/drivers/pci/Kconfig
> +++ b/drivers/pci/Kconfig
> @@ -42,6 +42,15 @@ config PCI_DEBUG
>  
>  	  When in doubt, say N.
>  
> +config PCI_STUB
> +	tristate "PCI Stub driver"
> +	depends on PCI
> +	help
> +	  Say Y or M here if you want be able to reserve a PCI device
> +	  when it is going to be assigned to a guest.

"guest operating system"?  Otherwise, just "guest" doesn't mean much
here in this context.

Other than that minor thing, looks great.

Acked-by: Greg Kroah-Hartman <gregkh@suse.de>

thanks,

greg k-h

Re: [PATCH 2/2] PCI: pci-stub module to reserve pci device

[Chris Wright]

* Greg KH (greg@kroah.com) wrote:
> On Tue, Nov 25, 2008 at 07:38:39PM -0800, Chris Wright wrote:
> > When doing device assignment with KVM there's currently nothing to
> > protect the device from having a driver in the host as well as the guest.
> > This trivial module just binds the pci device on the host to a stub
> > driver so that a real host driver can't bind to the device.  It has no
> > pci id table, it supports only dynamic ids.
> > 
> >  # echo "8086 10f5" > /sys/bus/pci/drivers/pci-stub/new_id
> >  # echo -n 0000:00:19.0 > /sys/bus/pci/drivers/e1000e/unbind
> >  # echo -n 0000:00:19.0 > /sys/bus/pci/drivers/pci-stub/bind
> >  # ls -l /sys/bus/pci/devices/0000:00:19.0/driver
> >  lrwxrwxrwx 1 root root 0 2008-11-25 19:10 /sys/bus/pci/devices/0000:00:19.0/driver -> ../../../bus/pci/drivers/pci-stub
> 
> You might want to put this somewhere in the .c or documentation files
> somewhere as well.

OK, I'll add this to the .c file for now.

> > Cc: "Kay, Allen M" <allen.m.kay@intel.com>
> > Cc: "Nakajima, Jun" <jun.nakajima@intel.com>
> > Signed-off-by: Chris Wright <chrisw@sous-sol.org>
> > ---
> >  drivers/pci/Kconfig    |    9 +++++++++
> >  drivers/pci/Makefile   |    2 ++
> >  drivers/pci/pci-stub.c |   38 ++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 49 insertions(+), 0 deletions(-)
> >  create mode 100644 drivers/pci/pci-stub.c
> > 
> > diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
> > index e1ca425..f6183df 100644
> > --- a/drivers/pci/Kconfig
> > +++ b/drivers/pci/Kconfig
> > @@ -42,6 +42,15 @@ config PCI_DEBUG
> >  
> >  	  When in doubt, say N.
> >  
> > +config PCI_STUB
> > +	tristate "PCI Stub driver"
> > +	depends on PCI
> > +	help
> > +	  Say Y or M here if you want be able to reserve a PCI device
> > +	  when it is going to be assigned to a guest.
> 
> "guest operating system"?  Otherwise, just "guest" doesn't mean much
> here in this context.

fixed

> Other than that minor thing, looks great.
> 
> Acked-by: Greg Kroah-Hartman <gregkh@suse.de>

thanks,
-chris

[PATCH 2/2 v2] PCI: pci-stub module to reserve pci device

[Chris Wright]

When doing device assignment with KVM there's currently nothing to
protect the device from having a driver in the host as well as the guest.
This trivial module just binds the pci device on the host to a stub
driver so that a real host driver can't bind to the device.  It has no
pci id table, it supports only dynamic ids.

 # echo "8086 10f5" > /sys/bus/pci/drivers/pci-stub/new_id
 # echo -n 0000:00:19.0 > /sys/bus/pci/drivers/e1000e/unbind
 # echo -n 0000:00:19.0 > /sys/bus/pci/drivers/pci-stub/bind
 # ls -l /sys/bus/pci/devices/0000:00:19.0/driver
 lrwxrwxrwx 1 root root 0 2008-11-25 19:10 /sys/bus/pci/devices/0000:00:19.0/driver -> ../../../bus/pci/drivers/pci-stub

Cc: "Kay, Allen M" <allen.m.kay@intel.com>
Cc: "Nakajima, Jun" <jun.nakajima@intel.com>
Signed-off-by: Chris Wright <chrisw@sous-sol.org>
Acked-by: Greg Kroah-Hartman <gregkh@suse.de>
---
 drivers/pci/Kconfig    |    9 +++++++++
 drivers/pci/Makefile   |    2 ++
 drivers/pci/pci-stub.c |   47 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 58 insertions(+), 0 deletions(-)
 create mode 100644 drivers/pci/pci-stub.c

diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
index e1ca425..2a4501d 100644
--- a/drivers/pci/Kconfig
+++ b/drivers/pci/Kconfig
@@ -42,6 +42,15 @@ config PCI_DEBUG
 
 	  When in doubt, say N.
 
+config PCI_STUB
+	tristate "PCI Stub driver"
+	depends on PCI
+	help
+	  Say Y or M here if you want be able to reserve a PCI device
+	  when it is going to be assigned to a guest operating system.
+
+	  When in doubt, say N.
+
 config HT_IRQ
 	bool "Interrupts on hypertransport devices"
 	default y
diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
index af3bfe2..3d07ce2 100644
--- a/drivers/pci/Makefile
+++ b/drivers/pci/Makefile
@@ -53,6 +53,8 @@ obj-$(CONFIG_HOTPLUG) += setup-bus.o
 
 obj-$(CONFIG_PCI_SYSCALL) += syscall.o
 
+obj-$(CONFIG_PCI_STUB) += pci-stub.o
+
 ifeq ($(CONFIG_PCI_DEBUG),y)
 EXTRA_CFLAGS += -DDEBUG
 endif
diff --git a/drivers/pci/pci-stub.c b/drivers/pci/pci-stub.c
new file mode 100644
index 0000000..74fbec0
--- /dev/null
+++ b/drivers/pci/pci-stub.c
@@ -0,0 +1,47 @@
+/* pci-stub - simple stub driver to reserve a pci device
+ *
+ * Copyright (C) 2008 Red Hat, Inc.
+ * Author:
+ * 	Chris Wright
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.
+ *
+ * Usage is simple, allocate a new id to the stub driver and bind the
+ * device to it.  For example:
+ * 
+ * # echo "8086 10f5" > /sys/bus/pci/drivers/pci-stub/new_id
+ * # echo -n 0000:00:19.0 > /sys/bus/pci/drivers/e1000e/unbind
+ * # echo -n 0000:00:19.0 > /sys/bus/pci/drivers/pci-stub/bind
+ * # ls -l /sys/bus/pci/devices/0000:00:19.0/driver
+ * .../0000:00:19.0/driver -> ../../../bus/pci/drivers/pci-stub
+ */
+
+#include <linux/module.h>
+#include <linux/pci.h>
+
+static int pci_stub_probe(struct pci_dev *dev, const struct pci_device_id *id)
+{
+	return 0;
+}
+
+static struct pci_driver stub_driver = {
+	.name		= "pci-stub",
+	.id_table	= NULL,	/* only dynamic id's */
+	.probe		= pci_stub_probe,
+};
+
+static int __init pci_stub_init(void)
+{
+	return pci_register_driver(&stub_driver);
+}
+
+static void __exit pci_stub_exit(void)
+{
+	pci_unregister_driver(&stub_driver);
+}
+
+module_init(pci_stub_init);
+module_exit(pci_stub_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Chris Wright <chrisw@sous-sol.org>");
-- 
1.6.0.3

Re: [PATCH 1/2] PCI: allow pci driver to support only dynids

[Jean Delvare]

Hi Chris,

On Tue, 25 Nov 2008 19:36:10 -0800, Chris Wright wrote:
> commit b41d6cf38e27 (PCI: Check dynids driver_data value for validity)
> requires all drivers to include an id table to try and match
> driver_data.  Before validating driver_data check driver has an id
> table.

Sorry for missing this case.

> Cc: Jean Delvare <khali@linux-fr.org>
> Cc: Milton Miller <miltonm@bga.com>
> Signed-off-by: Chris Wright <chrisw@sous-sol.org>
> ---
>  drivers/pci/pci-driver.c |   20 +++++++++++---------
>  1 files changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index b4cdd69..0a5edbe 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -48,7 +48,7 @@ store_new_id(struct device_driver *driver, const char *buf, size_t count)
>  		subdevice=PCI_ANY_ID, class=0, class_mask=0;
>  	unsigned long driver_data=0;
>  	int fields=0;
> -	int retval;
> +	int retval=0;
>  
>  	fields = sscanf(buf, "%x %x %x %x %x %x %lx",
>  			&vendor, &device, &subvendor, &subdevice,
> @@ -58,16 +58,18 @@ store_new_id(struct device_driver *driver, const char *buf, size_t count)
>  
>  	/* Only accept driver_data values that match an existing id_table
>  	   entry */
> -	retval = -EINVAL;
> -	while (ids->vendor || ids->subvendor || ids->class_mask) {
> -		if (driver_data == ids->driver_data) {
> -			retval = 0;
> -			break;
> +	if (ids) {
> +		retval = -EINVAL;
> +		while (ids->vendor || ids->subvendor || ids->class_mask) {
> +			if (driver_data == ids->driver_data) {
> +				retval = 0;
> +				break;
> +			}
> +			ids++;
>  		}
> -		ids++;
> +		if (retval)	/* No match */
> +			return retval;
>  	}
> -	if (retval)	/* No match */
> -		return retval;
>  
>  	dynid = kzalloc(sizeof(*dynid), GFP_KERNEL);
>  	if (!dynid)

Do we really want to let the user add a new id to a driver which didn't
have any? Wouldn't it be safer to simply not create the new_id sysfs
file if driver->id_table is NULL? That's a one-line change:

Signed-off-by: Jean Delvare <khali@linux-fr.org>
---
 drivers/pci/pci-driver.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- linux-2.6.28-rc6.orig/drivers/pci/pci-driver.c	2008-10-24 09:28:14.000000000 +0200
+++ linux-2.6.28-rc6/drivers/pci/pci-driver.c	2008-11-26 09:23:46.000000000 +0100
@@ -113,7 +113,7 @@ static int
 pci_create_newid_file(struct pci_driver *drv)
 {
 	int error = 0;
-	if (drv->probe != NULL)
+	if (drv->probe != NULL && drv->id_table != NULL)
 		error = driver_create_file(&drv->driver, &driver_attr_new_id);
 	return error;
 }

As a side note, I am curious what PCI driver we do have which has
driver->probe defined but no driver->id_table. Did you hit an actual
issue or are you fixing a theoretical one?

Thanks,
-- 
Jean Delvare

Re: [PATCH 1/2] PCI: allow pci driver to support only dynids

[Chris Wright]

* Jean Delvare (khali@linux-fr.org) wrote:
> As a side note, I am curious what PCI driver we do have which has
> driver->probe defined but no driver->id_table. Did you hit an actual
> issue or are you fixing a theoretical one?

Such a driver was patch 2/2.

http://lkml.org/lkml/2008/11/26/11

It's meant to have only dynamic ids, worked fine on 2.6.27 and oopsed
from NULL id-> deref on current git.

thanks,
-chris

Re: [PATCH 1/2] PCI: allow pci driver to support only dynids

[Jean Delvare]

Hi Chris,

On Wed, 26 Nov 2008 07:19:40 -0800, Chris Wright wrote:
> * Jean Delvare (khali@linux-fr.org) wrote:
> > As a side note, I am curious what PCI driver we do have which has
> > driver->probe defined but no driver->id_table. Did you hit an actual
> > issue or are you fixing a theoretical one?
> 
> Such a driver was patch 2/2.
> 
> http://lkml.org/lkml/2008/11/26/11
> 
> It's meant to have only dynamic ids, worked fine on 2.6.27 and oopsed
> from NULL id-> deref on current git.

Ah, OK, then your patch makes full sense and mine doesn't, sorry for
the noise. Feel free to add my Acked-by on your patch.

-- 
Jean Delvare

Re: [PATCH 1/2] PCI: allow pci driver to support only dynids

[Chris Wright]

* Jean Delvare (khali@linux-fr.org) wrote:
> Ah, OK, then your patch makes full sense and mine doesn't, sorry for
> the noise. Feel free to add my Acked-by on your patch.

Thanks Jean.
-chris

Re: [PATCH 1/2] PCI: allow pci driver to support only dynids

[Jesse Barnes]

On Tuesday, November 25, 2008 7:36 pm Chris Wright wrote:
> commit b41d6cf38e27 (PCI: Check dynids driver_data value for validity)
> requires all drivers to include an id table to try and match
> driver_data.  Before validating driver_data check driver has an id
> table.
>
> Cc: Jean Delvare <khali@linux-fr.org>
> Cc: Milton Miller <miltonm@bga.com>
> Signed-off-by: Chris Wright <chrisw@sous-sol.org>

Applied these to my linux-next branch, thanks Chris.

-- 
Jesse Barnes, Intel Open Source Technology Center