KVM PCI device assignment issues

KVM PCI device assignment issues

[Mark McLoughlin]

Hi,

KVM has support for PCI device assignment using VT-d and AMD IOMMU, but
there are a number of inter-related issues that need some further
discussion:

  - Unbinding devices from any existing device driver before assignment

  - Resetting devices before and after assignment

  - Helping users figure out which devices can actually be assigned

This gets confusing, so some background constraints first:

  - Conventional PCI devices (i.e. PCI/PCI-X, not PCIe) behind the same 
    bridge must be assigned to the same VT-d domain - i.e given device 
    A (0000:0f:1.0) and device B (and 0000:0f:2.0), if you assign 
    device A to guest, you cannot then use device B in the host or 
    another guest.

  - Some newer PCIe devices (and newer conventional PCI devices too via 
    PCI Advanced Features) support Function Level Reset (FLR). This 
    allows a PCI function to be reset without affecting any other 
    functions on that device, or any other devices. This feature is not 
    widespread yet AFAIK - e.g. I've seen it on an audio controller, 
    and it must also be supported by SR-IOV devices.

  - Secondary Bus Reset (SBR) allows software to trigger a reset on all 
    devices (and functions) behind a PCI bridge.

  - A PCI Power Management D-state transition (D3hot to D0) can be used 
    to reset a device (all functions).

  - Some PCI devices don't have page aligned MMIO BARs. These devices 
    (all functions) cannot be safely assigned to guests.

Driver Unbinding
================

Before a device is assigned to a guest, we should make sure that no host
device driver is currently bound to the device.

We can do that with e.g.

 $> echo -n "8086 10de"  > /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

One minor problem with this scheme is that at this point you can't
unbind from pci-stub and trigger a re-probe and have e1000e bind to it.
In order to support that, we need a "remove_id" interface to remove the
dynamic ID.

What we don't support is a way to unbind permanently. Xen has a
pciback.hide module param which tries to achieve this, but you end up
with the inevitable issues around making sure pciback is loaded before
the device driver etc.

Permanent unbinding isn't necessarily needed, but it might help provide
a solution to some of the nastier issues below.

Device Reset
============

Before assigning a device to a guest, it should be reset. The host or a
previous guest may have left the device in an unknown state. Not
resetting can be seen in testing to lead to e.g. "TX Unit Hang" errors
with e1000e devices.

FLR is without doubt the preferable solution here. KVM already
implements this. However, the range of devices which support FLR is
currently quite limited.

If we're assigning devices from behind a PCI/PCI-x bridge (remember all
devices must be assigned together), then we can use SBR to reset them
all together. Clearly, though, one should make sure that all devices
behind that bridge are not in use before doing the reset. We could
implement this with a "reset" sysfs interface for pci-stub - it would
only reset a device using SBR if all devices behind that bridge were
bound to pci-stub.

Where a conventional PCI device is on the root bus, or where a PCIe
device is on the root bus or another bus with multiple devices, we could
use the D-state transition reset. Since this resets all functions on a
device, we would need a similar approach where all functions must be
bound to pci-stub before being reset.

Furthermore, we would need to prevent pci-stub from resetting a device
it is bound to where the device is already assigned to a guest. To
achieve this, we would want KVM to explicitly call in to pci-stub to
mark a device as in use.

The alternatives to such an approach are:

  a) Only support FLR capable devices

  b) Cross our fingers and hope that work without a device reset

  c) Allow a driver to be permanently unbound from a device and require 
     the user to reboot after unbinding before assigning

Filtering
=========

In order to support a sane user interface in management tools, it should
be possible to list all PCI devices on available on a host and filter
out those which cannot be assigned to a guest.

Furthermore, it should be possible to do this without actually affecting
any of the devices - i.e. a "try to unbind and see if we oops" approach
clearly isn't great.

Finally, some management tools would like to be able to do this
filtering given the constraint of a device being reserved for a
currently inactive guest.

This last constraint is the most difficult and points to the logic
needing to be in userland management libraries. Possibly the only sane
kernel space support would be "try to unbind and reset; if it works then
the device is assignable".

Conclusions
===========

Only supporting devices with FLR restricts our user pool far too
severely.

Permanent unbinding is not supportable.

SBR and D-state reset support is doable with the addition of a "reset"
interface to pci-stub and some logic to check that a reset does not
affect devices not already bound to pci-stub.

KVM would need to be able to mark pci-stub bound devices as in use when
assigned to a guest.

We need the opposite to "new_id" to allow dynids to be removed.

The filtering abilities available to userland via kernel interfaces will
be limited. Further logic will need to be implemented in userland.

Cheers,
Mark.

Re: KVM PCI device assignment issues

[Greg KH]

On Fri, Feb 13, 2009 at 04:32:47PM +0000, Mark McLoughlin wrote:
> Driver Unbinding
> ================
> 
> Before a device is assigned to a guest, we should make sure that no host
> device driver is currently bound to the device.
> 
> We can do that with e.g.
> 
>  $> echo -n "8086 10de"  > /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
> 
> One minor problem with this scheme is that at this point you can't
> unbind from pci-stub and trigger a re-probe and have e1000e bind to it.

Are you sure?  It should work if you manually tell the e1000e driver to
bind to it, after unbinding it from the pci-stub driver.

> In order to support that, we need a "remove_id" interface to remove the
> dynamic ID.

Why?

> What we don't support is a way to unbind permanently. Xen has a
> pciback.hide module param which tries to achieve this, but you end up
> with the inevitable issues around making sure pciback is loaded before
> the device driver etc.

What do you mean, unbind "permanently"?  For every reboot?  Or just
within the same boot time?

thanks,

greg k-h

Re: KVM PCI device assignment issues

[Mark McLoughlin]

On Fri, 2009-02-13 at 08:56 -0800, Greg KH wrote:
> On Fri, Feb 13, 2009 at 04:32:47PM +0000, Mark McLoughlin wrote:
> > Driver Unbinding
> > ================
> > 
> > Before a device is assigned to a guest, we should make sure that no host
> > device driver is currently bound to the device.
> > 
> > We can do that with e.g.
> > 
> >  $> echo -n "8086 10de"  > /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
> > 
> > One minor problem with this scheme is that at this point you can't
> > unbind from pci-stub and trigger a re-probe and have e1000e bind to it.
> 
> Are you sure?  It should work if you manually tell the e1000e driver to
> bind to it, after unbinding it from the pci-stub driver.

Yes, that works - I meant using /sys/bus/pci/drivers_probe. The problem
is that it would suck for management tools to have to remember which
device driver it was originally bound to.

> > In order to support that, we need a "remove_id" interface to remove the
> > dynamic ID.
> 
> Why?

Before assignment:

 $> echo -n "8086 10de"  > /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
 $> echo -n "8086 10de"  > /sys/bus/pci/drivers/pci-stub/remove_id

After assignment:

 $> echo -n 0000:00:19.0 > /sys/bus/pci/drivers_probe

> > What we don't support is a way to unbind permanently. Xen has a
> > pciback.hide module param which tries to achieve this, but you end up
> > with the inevitable issues around making sure pciback is loaded before
> > the device driver etc.
> 
> What do you mean, unbind "permanently"?  For every reboot?  Or just
> within the same boot time?

Across reboots, yeah.

Cheers,
Mark.

Re: KVM PCI device assignment issues

[Matthew Wilcox]

On Fri, Feb 13, 2009 at 04:32:47PM +0000, Mark McLoughlin wrote:
> Hi,

You raise some interesting points.  Thanks for doing that rather than
going off and creating a big pile of patches and demanding they be
applied ;-)

> This gets confusing, so some background constraints first:
> 
>   - Conventional PCI devices (i.e. PCI/PCI-X, not PCIe) behind the same 
>     bridge must be assigned to the same VT-d domain - i.e given device 
>     A (0000:0f:1.0) and device B (and 0000:0f:2.0), if you assign 
>     device A to guest, you cannot then use device B in the host or 
>     another guest.

Is that a limitation of the VT-d / IOMMU setup?

>   - Some newer PCIe devices (and newer conventional PCI devices too via 
>     PCI Advanced Features) support Function Level Reset (FLR). This 
>     allows a PCI function to be reset without affecting any other 
>     functions on that device, or any other devices. This feature is not 
>     widespread yet AFAIK - e.g. I've seen it on an audio controller, 
>     and it must also be supported by SR-IOV devices.

Yes, that's definitely not very widespread yet.  OTOH, we don't need to
worry about disturbing other functions if all devices behind the same
bridge have to be mapped to the same guest.

>   - Secondary Bus Reset (SBR) allows software to trigger a reset on all 
>     devices (and functions) behind a PCI bridge.
> 
>   - A PCI Power Management D-state transition (D3hot to D0) can be used 
>     to reset a device (all functions).

That's not guaranteed according to PCI PM 1.2:

5.4.1. Software Accessible D3 (D3hot)

  When programmed to D0, the function may return to the D0 Initialized
  or D0 Uninitialized state without PCI RST# being asserted. This option
  is determined at design time and allows designs the option of either
  performing an internal reset or not performing an internal reset.

-----

There's also the option that devices in a hotplug PCI slot can have
their power cycled, forcing them into D3cold and then transitioning into
D0 Uninitialised.

>   - Some PCI devices don't have page aligned MMIO BARs. These devices 
>     (all functions) cannot be safely assigned to guests.

We've seen patches to force page alignment on this list ... they haven't
been sufficiently beautiful to be applied yet.

> Driver Unbinding
> ================
> 
> Before a device is assigned to a guest, we should make sure that no host
> device driver is currently bound to the device.
> 
> We can do that with e.g.
> 
>  $> echo -n "8086 10de"  > /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
> 
> One minor problem with this scheme is that at this point you can't
> unbind from pci-stub and trigger a re-probe and have e1000e bind to it.
> In order to support that, we need a "remove_id" interface to remove the
> dynamic ID.

It sounds like you'd be OK with a 'remove_id' interface that only
removes subsequently-added interfaces.

I might suggest a second approach which would be to have an explicit
echo to the bind file ignore the list of ids.  Then you wouldn't need to
'echo -n "8086 10de"' to begin with.

> Device Reset
> ============
> 
> Before assigning a device to a guest, it should be reset. The host or a
> previous guest may have left the device in an unknown state. Not
> resetting can be seen in testing to lead to e.g. "TX Unit Hang" errors
> with e1000e devices.

Really, this is the same problem that kexec has.  Either the driver is
doing insufficient initialisation, or it's not doing tis shutdown
properly.  The former is definitely better than the latter as kexec may
be used from a position of having the driver locked solid and unable to
reset the device.

> If we're assigning devices from behind a PCI/PCI-x bridge (remember all
> devices must be assigned together), then we can use SBR to reset them
> all together. Clearly, though, one should make sure that all devices
> behind that bridge are not in use before doing the reset. We could
> implement this with a "reset" sysfs interface for pci-stub - it would
> only reset a device using SBR if all devices behind that bridge were
> bound to pci-stub.

I don't think this should be part of pci-stub, but rather part of the
PCI core.  I can imagine other uses for being able to reset all devices
behind a bridge that don't involve anything to do with v12n.  So I'd
like to see a /sys/class/pci_bus/*/reset (where * would not include root
busses).

> Where a conventional PCI device is on the root bus, or where a PCIe
> device is on the root bus or another bus with multiple devices, we could
> use the D-state transition reset. Since this resets all functions on a
> device, we would need a similar approach where all functions must be
> bound to pci-stub before being reset.

Even with the caveat above about D0 -> D3hot -> D0 doesn't necessarily
do a full reset, it does seem to be per-function.  For example, this
passage from the SCSI LSI 1010-66 chip docs implies that very strongly:

  Power state D3 is a lower power level than power state D2. A function in
  this state places the LSI53C1010 core in the coma mode. Furthermore,
  the function's soft reset is continually asserted while in power state D3,
  which clears all pending interrupts and 3-states the SCSI bus. In
  addition, the function's PCI Command register is cleared. If both of the
  LSI53C1010 functions are placed in power state D3, the Clock
  Quadrupler is disabled, which results in additional power savings.

> Filtering
> =========
> 
> In order to support a sane user interface in management tools, it should
> be possible to list all PCI devices on available on a host and filter
> out those which cannot be assigned to a guest.

I think this is going to have to have a large userspace component ...
let's see what needs to be done in-kernel.

> Furthermore, it should be possible to do this without actually affecting
> any of the devices - i.e. a "try to unbind and see if we oops" approach
> clearly isn't great.

Well, yes.  I'd even be upset if my network or storage flickered away
briefly while another using was starting to run KVM.

> This last constraint is the most difficult and points to the logic
> needing to be in userland management libraries. Possibly the only sane
> kernel space support would be "try to unbind and reset; if it works then
> the device is assignable".

If we expose a 'reset' file in the /sys/bus/pci/devices/*/ directories
for devices that are resettable, that should be enough, I would think.

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: KVM PCI device assignment issues

[Chris Wright]

* Matthew Wilcox (matthew@wil.cx) wrote:
> On Fri, Feb 13, 2009 at 04:32:47PM +0000, Mark McLoughlin wrote:
> >   - Conventional PCI devices (i.e. PCI/PCI-X, not PCIe) behind the same 
> >     bridge must be assigned to the same VT-d domain - i.e given device 
> >     A (0000:0f:1.0) and device B (and 0000:0f:2.0), if you assign 
> >     device A to guest, you cannot then use device B in the host or 
> >     another guest.
> 
> Is that a limitation of the VT-d / IOMMU setup?

Yes.  The source id will essentially show up as the bridge.

> >   - Some newer PCIe devices (and newer conventional PCI devices too via 
> >     PCI Advanced Features) support Function Level Reset (FLR). This 
> >     allows a PCI function to be reset without affecting any other 
> >     functions on that device, or any other devices. This feature is not 
> >     widespread yet AFAIK - e.g. I've seen it on an audio controller, 
> >     and it must also be supported by SR-IOV devices.
> 
> Yes, that's definitely not very widespread yet.  OTOH, we don't need to
> worry about disturbing other functions if all devices behind the same
> bridge have to be mapped to the same guest.

FLR (when it exists) would work fine for devices not behind a conventional
pci bridge.

> > Driver Unbinding
> > ================
> > 
> > Before a device is assigned to a guest, we should make sure that no host
> > device driver is currently bound to the device.
> > 
> > We can do that with e.g.
> > 
> >  $> echo -n "8086 10de"  > /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
> > 
> > One minor problem with this scheme is that at this point you can't
> > unbind from pci-stub and trigger a re-probe and have e1000e bind to it.
> > In order to support that, we need a "remove_id" interface to remove the
> > dynamic ID.
> 
> It sounds like you'd be OK with a 'remove_id' interface that only
> removes subsequently-added interfaces.
> 
> I might suggest a second approach which would be to have an explicit
> echo to the bind file ignore the list of ids.  Then you wouldn't need to
> 'echo -n "8086 10de"' to begin with.

I tried that first, and it dips into the driver logic, where it wants
to filter via ->match.  Untested patch below _should_ be enough to avoid
adding the id to begin with.

> > Furthermore, it should be possible to do this without actually affecting
> > any of the devices - i.e. a "try to unbind and see if we oops" approach
> > clearly isn't great.
> 
> Well, yes.  I'd even be upset if my network or storage flickered away
> briefly while another using was starting to run KVM.
> 
> > This last constraint is the most difficult and points to the logic
> > needing to be in userland management libraries. Possibly the only sane
> > kernel space support would be "try to unbind and reset; if it works then
> > the device is assignable".
> 
> If we expose a 'reset' file in the /sys/bus/pci/devices/*/ directories
> for devices that are resettable, that should be enough, I would think.

Yes, currently it's only internal and it's not robust given the reset
constraints.

thanks,
-chris
--

 drivers/base/base.h |    1 +
 drivers/base/bus.c  |    2 +-
 drivers/base/dd.c   |   15 +++++++++++++--
 3 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/base/base.h b/drivers/base/base.h
index 0a5f055..60dc346 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -86,6 +86,7 @@ extern void bus_remove_driver(struct device_driver *drv);
 
 extern void driver_detach(struct device_driver *drv);
 extern int driver_probe_device(struct device_driver *drv, struct device *dev);
+extern int driver_bind_probe_device(struct device_driver *drv, struct device *dev);
 
 extern void sysdev_shutdown(void);
 extern int sysdev_suspend(pm_message_t state);
diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index 83f32b8..ad28338 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -202,7 +202,7 @@ static ssize_t driver_bind(struct device_driver *drv,
 		if (dev->parent)	/* Needed for USB */
 			down(&dev->parent->sem);
 		down(&dev->sem);
-		err = driver_probe_device(drv, dev);
+		err = driver_bind_probe_device(drv, dev);
 		up(&dev->sem);
 		if (dev->parent)
 			up(&dev->parent->sem);
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 315bed8..fba6463 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -184,13 +184,14 @@ int driver_probe_done(void)
  * This function must be called with @dev->sem held.  When called for a
  * USB interface, @dev->parent->sem must be held as well.
  */
-int driver_probe_device(struct device_driver *drv, struct device *dev)
+static int __driver_probe_device(struct device_driver *drv, struct device *dev,
+				 bool force)
 {
 	int ret = 0;
 
 	if (!device_is_registered(dev))
 		return -ENODEV;
-	if (drv->bus->match && !drv->bus->match(dev, drv))
+	if (!force && drv->bus->match && !drv->bus->match(dev, drv))
 		goto done;
 
 	pr_debug("bus: '%s': %s: matched device %s with driver %s\n",
@@ -202,6 +203,16 @@ done:
 	return ret;
 }
 
+int driver_probe_bind_device(struct device_driver *drv, struct device *dev)
+{
+	return __driver_probe_device(drv, dev, 1);
+}
+
+int driver_probe_device(struct device_driver *drv, struct device *dev)
+{
+	return __driver_probe_device(drv, dev, 0);
+}
+
 static int __device_attach(struct device_driver *drv, void *data)
 {
 	struct device *dev = data;

Re: KVM PCI device assignment issues

[Chris Wright]

* Chris Wright (chrisw@redhat.com) wrote:
> * Matthew Wilcox (matthew@wil.cx) wrote:
> > I might suggest a second approach which would be to have an explicit
> > echo to the bind file ignore the list of ids.  Then you wouldn't need to
> > 'echo -n "8086 10de"' to begin with.
> 
> I tried that first, and it dips into the driver logic, where it wants
> to filter via ->match.  Untested patch below _should_ be enough to avoid
> adding the id to begin with.

OK, after making it actually compile.  Still gets trapped into generic
logic, this time in pci core.  I'm starting to remember why dynid looked
like the better option.

pci_device_probe
  __pci_device_probe
    pci_match_device()  <-- fails

[PATCH] pci: add remove_id sysfs entry

[Chris Wright]

This adds a remove_id sysfs entry to allow users of new_id to later
remove the added dynid.  One use case is management tools that want to
dynamically bind/unbind devices to pci-stub driver while devices are
assigned to KVM guests.  Rather than having to track which driver was
originally bound to the driver, a mangement tool can simply:

# echo "8086 10f5" > /sys/bus/pci/drivers/pci-stub/new_id
# echo -n 0000:00:19.0 > /sys/bus/pci/devices/0000:00:19.0/driver/unbind
# echo -n 0000:00:19.0 > /sys/bus/pci/drivers/pci-stub/bind

Guest uses device

# echo "8086 10f5" > /sys/bus/pci/drivers/pci-stub/remove_id
# echo 0000:00:19.0 > /sys/bus/pci/drivers_probe

Signed-off-by: Chris Wright <chrisw@sous-sol.org>
---
 drivers/pci/pci-driver.c |   80 ++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 78 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 93eac14..87a5ddb 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -99,6 +99,52 @@ store_new_id(struct device_driver *driver, const char *buf, size_t count)
 }
 static DRIVER_ATTR(new_id, S_IWUSR, NULL, store_new_id);
 
+/**
+ * store_remove_id - remove a PCI device ID from this driver
+ * @driver: target device driver
+ * @buf: buffer for scanning device ID data
+ * @count: input size
+ *
+ * Removes a dynamic pci device ID to this driver.
+ */
+static ssize_t
+store_remove_id(struct device_driver *driver, const char *buf, size_t count)
+{
+	struct pci_dynid *dynid, *n;
+	struct pci_driver *pdrv = to_pci_driver(driver);
+	__u32 vendor, device, subvendor = PCI_ANY_ID,
+		subdevice = PCI_ANY_ID, class = 0, class_mask = 0;
+	int fields = 0;
+	int retval = -ENODEV;
+
+	fields = sscanf(buf, "%x %x %x %x %x %x",
+			&vendor, &device, &subvendor, &subdevice,
+			&class, &class_mask);
+	if (fields < 2)
+		return -EINVAL;
+
+	spin_lock(&pdrv->dynids.lock);
+	list_for_each_entry_safe(dynid, n, &pdrv->dynids.list, node) {
+		struct pci_device_id *id = &dynid->id;
+		if ((id->vendor == vendor) &&
+		    (id->device == device) &&
+		    (subvendor == PCI_ANY_ID || id->subvendor == subvendor) &&
+		    (subdevice == PCI_ANY_ID || id->subdevice == subdevice) &&
+		    !((id->class ^ class) & class_mask)) {
+			list_del(&dynid->node);
+			kfree(dynid);
+			retval = 0;
+			break;
+		}
+	}
+	spin_unlock(&pdrv->dynids.lock);
+
+	if (retval)
+		return retval;
+	return count;
+}
+static DRIVER_ATTR(remove_id, S_IWUSR, NULL, store_remove_id);
+
 static void
 pci_free_dynids(struct pci_driver *drv)
 {
@@ -125,6 +171,20 @@ static void pci_remove_newid_file(struct pci_driver *drv)
 {
 	driver_remove_file(&drv->driver, &driver_attr_new_id);
 }
+
+static int
+pci_create_removeid_file(struct pci_driver *drv)
+{
+	int error = 0;
+	if (drv->probe != NULL)
+		error = driver_create_file(&drv->driver,&driver_attr_remove_id);
+	return error;
+}
+
+static void pci_remove_removeid_file(struct pci_driver *drv)
+{
+	driver_remove_file(&drv->driver, &driver_attr_remove_id);
+}
 #else /* !CONFIG_HOTPLUG */
 static inline void pci_free_dynids(struct pci_driver *drv) {}
 static inline int pci_create_newid_file(struct pci_driver *drv)
@@ -132,6 +192,11 @@ static inline int pci_create_newid_file(struct pci_driver *drv)
 	return 0;
 }
 static inline void pci_remove_newid_file(struct pci_driver *drv) {}
+static inline int pci_create_removeid_file(struct pci_driver *drv)
+{
+	return 0;
+}
+static inline void pci_remove_removeid_file(struct pci_driver *drv) {}
 #endif
 
 /**
@@ -852,13 +917,23 @@ int __pci_register_driver(struct pci_driver *drv, struct module *owner,
 	/* register with core */
 	error = driver_register(&drv->driver);
 	if (error)
-		return error;
+		goto out;
 
 	error = pci_create_newid_file(drv);
 	if (error)
-		driver_unregister(&drv->driver);
+		goto out_newid;
 
+	error = pci_create_removeid_file(drv);
+	if (error)
+		goto out_removeid;
+out:
 	return error;
+
+out_removeid:
+	pci_remove_newid_file(drv);
+out_newid:
+	driver_unregister(&drv->driver);
+	goto out;
 }
 
 /**
@@ -874,6 +949,7 @@ int __pci_register_driver(struct pci_driver *drv, struct module *owner,
 void
 pci_unregister_driver(struct pci_driver *drv)
 {
+	pci_remove_removeid_file(drv);
 	pci_remove_newid_file(drv);
 	driver_unregister(&drv->driver);
 	pci_free_dynids(drv);

Re: [PATCH] pci: add remove_id sysfs entry

[Greg KH]

On Fri, Feb 13, 2009 at 06:12:44PM -0800, Chris Wright wrote:
> This adds a remove_id sysfs entry to allow users of new_id to later
> remove the added dynid.  One use case is management tools that want to
> dynamically bind/unbind devices to pci-stub driver while devices are
> assigned to KVM guests.  Rather than having to track which driver was
> originally bound to the driver, a mangement tool can simply:
> 
> # echo "8086 10f5" > /sys/bus/pci/drivers/pci-stub/new_id

As you're adding a new sysfs file, can you please document it in
Documentation/ABI?  I know the original bind and unbind files aren't
documented there, they were created before the ABI/ directory showed up.

It would be nice to add them as well if anyone wants to :)

thanks,

greg k-h

Re: [PATCH] pci: add remove_id sysfs entry

[Chris Wright]

* Greg KH (greg@kroah.com) wrote:
> On Fri, Feb 13, 2009 at 06:12:44PM -0800, Chris Wright wrote:
> > This adds a remove_id sysfs entry to allow users of new_id to later
> > remove the added dynid.  One use case is management tools that want to
> > dynamically bind/unbind devices to pci-stub driver while devices are
> > assigned to KVM guests.  Rather than having to track which driver was
> > originally bound to the driver, a mangement tool can simply:
> > 
> > # echo "8086 10f5" > /sys/bus/pci/drivers/pci-stub/new_id
> 
> As you're adding a new sysfs file, can you please document it in
> Documentation/ABI?  I know the original bind and unbind files aren't
> documented there, they were created before the ABI/ directory showed up.
> 
> It would be nice to add them as well if anyone wants to :)

Sure, I'll take a stab at that.

[PATCH 1/2] PCI: add some sysfs ABI docs

[Chris Wright]

Add sysfs ABI docs for driver entries bind, unbind and new_id.  These
entries are pretty old, from 2.6.0 onwards AFAIK, so this documents
current behaviour.

Signed-off-by: Chris Wright <chrisw@sous-sol.org>
---
 Documentation/ABI/testing/sysfs-bus-pci |   41 ++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

--- a/Documentation/ABI/testing/sysfs-bus-pci
+++ b/Documentation/ABI/testing/sysfs-bus-pci
@@ -1,3 +1,44 @@
+What:		/sys/bus/pci/drivers/.../bind
+Date:		December 2003
+Contact:	linux-pci@vger.kernel.org
+Description:
+		Writing a device location to this file will cause
+		the driver to attempt to bind to the device found at
+		this location.	This is useful for overriding default
+		bindings.  The format for the location is: DDDD:BB:DD.F.
+		That is Domain:Bus:Device.Function and is the same as
+		found in /sys/bus/pci/devices/.  For example:
+		# echo 0000:00:19.0 > /sys/bus/pci/drivers/foo/bind
+
+What:		/sys/bus/pci/drivers/.../unbind
+Date:		December 2003
+Contact:	linux-pci@vger.kernel.org
+Description:
+		Writing a device location to this file will cause the
+		driver to attempt to unbind from the device found at
+		this location.	This may be useful when overriding default
+		bindings.  The format for the location is: DDDD:BB:DD.F.
+		That is Domain:Bus:Device.Function and is the same as
+		found in /sys/bus/pci/devices/. For example:
+		# echo 0000:00:19.0 > /sys/bus/pci/drivers/foo/unbind
+
+What:		/sys/bus/pci/drivers/.../new_id
+Date:		December 2003
+Contact:	linux-pci@vger.kernel.org
+Description:
+		Writing a device ID to this file will attempt to
+		dynamically add a new device ID to a PCI device driver.
+		This may allow the driver to support more hardware than
+		was included in the driver's static device ID support
+		table at compile time.  The format for the device ID is:
+		VVVV DDDD SVVV SDDD CCCC MMMM PPPP.  That is Vendor ID,
+		Device ID, Subsystem Vendor ID, Subsystem Device ID,
+		Class, Class Mask, and Private Driver Data.  The Vendor ID
+		and Device ID fields are required, the rest are optional.
+		Upon successfully adding an ID, the driver will probe
+		for the device and attempt to bind to it.  For example:
+		# echo 8086 10f5 > /sys/bus/pci/drivers/foo/new_id
+
 What:		/sys/bus/pci/devices/.../vpd
 Date:		February 2008
 Contact:	Ben Hutchings <bhutchings@solarflare.com>

[PATCH 2/2] PCI: add remove_id sysfs entry

[Chris Wright]

This adds a remove_id sysfs entry to allow users of new_id to later
remove the added dynid.  One use case is management tools that want to
dynamically bind/unbind devices to pci-stub driver while devices are
assigned to KVM guests.  Rather than having to track which driver was
originally bound to the driver, a mangement tool can simply:

# echo "8086 10f5" > /sys/bus/pci/drivers/pci-stub/new_id
# echo -n 0000:00:19.0 > /sys/bus/pci/devices/0000:00:19.0/driver/unbind
# echo -n 0000:00:19.0 > /sys/bus/pci/drivers/pci-stub/bind

Guest uses device

# echo "8086 10f5" > /sys/bus/pci/drivers/pci-stub/remove_id
# echo 0000:00:19.0 > /sys/bus/pci/drivers_probe

Signed-off-by: Chris Wright <chrisw@sous-sol.org>
---
 Documentation/ABI/testing/sysfs-bus-pci |   16 ++++++
 drivers/pci/pci-driver.c                |   80 +++++++++++++++++++++++++++++++-
 2 files changed, 94 insertions(+), 2 deletions(-)

--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -99,6 +99,52 @@ store_new_id(struct device_driver *drive
 }
 static DRIVER_ATTR(new_id, S_IWUSR, NULL, store_new_id);
 
+/**
+ * store_remove_id - remove a PCI device ID from this driver
+ * @driver: target device driver
+ * @buf: buffer for scanning device ID data
+ * @count: input size
+ *
+ * Removes a dynamic pci device ID to this driver.
+ */
+static ssize_t
+store_remove_id(struct device_driver *driver, const char *buf, size_t count)
+{
+	struct pci_dynid *dynid, *n;
+	struct pci_driver *pdrv = to_pci_driver(driver);
+	__u32 vendor, device, subvendor = PCI_ANY_ID,
+		subdevice = PCI_ANY_ID, class = 0, class_mask = 0;
+	int fields = 0;
+	int retval = -ENODEV;
+
+	fields = sscanf(buf, "%x %x %x %x %x %x",
+			&vendor, &device, &subvendor, &subdevice,
+			&class, &class_mask);
+	if (fields < 2)
+		return -EINVAL;
+
+	spin_lock(&pdrv->dynids.lock);
+	list_for_each_entry_safe(dynid, n, &pdrv->dynids.list, node) {
+		struct pci_device_id *id = &dynid->id;
+		if ((id->vendor == vendor) &&
+		    (id->device == device) &&
+		    (subvendor == PCI_ANY_ID || id->subvendor == subvendor) &&
+		    (subdevice == PCI_ANY_ID || id->subdevice == subdevice) &&
+		    !((id->class ^ class) & class_mask)) {
+			list_del(&dynid->node);
+			kfree(dynid);
+			retval = 0;
+			break;
+		}
+	}
+	spin_unlock(&pdrv->dynids.lock);
+
+	if (retval)
+		return retval;
+	return count;
+}
+static DRIVER_ATTR(remove_id, S_IWUSR, NULL, store_remove_id);
+
 static void
 pci_free_dynids(struct pci_driver *drv)
 {
@@ -125,6 +171,20 @@ static void pci_remove_newid_file(struct
 {
 	driver_remove_file(&drv->driver, &driver_attr_new_id);
 }
+
+static int
+pci_create_removeid_file(struct pci_driver *drv)
+{
+	int error = 0;
+	if (drv->probe != NULL)
+		error = driver_create_file(&drv->driver,&driver_attr_remove_id);
+	return error;
+}
+
+static void pci_remove_removeid_file(struct pci_driver *drv)
+{
+	driver_remove_file(&drv->driver, &driver_attr_remove_id);
+}
 #else /* !CONFIG_HOTPLUG */
 static inline void pci_free_dynids(struct pci_driver *drv) {}
 static inline int pci_create_newid_file(struct pci_driver *drv)
@@ -132,6 +192,11 @@ static inline int pci_create_newid_file(
 	return 0;
 }
 static inline void pci_remove_newid_file(struct pci_driver *drv) {}
+static inline int pci_create_removeid_file(struct pci_driver *drv)
+{
+	return 0;
+}
+static inline void pci_remove_removeid_file(struct pci_driver *drv) {}
 #endif
 
 /**
@@ -852,13 +917,23 @@ int __pci_register_driver(struct pci_dri
 	/* register with core */
 	error = driver_register(&drv->driver);
 	if (error)
-		return error;
+		goto out;
 
 	error = pci_create_newid_file(drv);
 	if (error)
-		driver_unregister(&drv->driver);
+		goto out_newid;
 
+	error = pci_create_removeid_file(drv);
+	if (error)
+		goto out_removeid;
+out:
 	return error;
+
+out_removeid:
+	pci_remove_newid_file(drv);
+out_newid:
+	driver_unregister(&drv->driver);
+	goto out;
 }
 
 /**
@@ -874,6 +949,7 @@ int __pci_register_driver(struct pci_dri
 void
 pci_unregister_driver(struct pci_driver *drv)
 {
+	pci_remove_removeid_file(drv);
 	pci_remove_newid_file(drv);
 	driver_unregister(&drv->driver);
 	pci_free_dynids(drv);
--- a/Documentation/ABI/testing/sysfs-bus-pci
+++ b/Documentation/ABI/testing/sysfs-bus-pci
@@ -39,6 +39,22 @@ Description:
 		for the device and attempt to bind to it.  For example:
 		# echo 8086 10f5 > /sys/bus/pci/drivers/foo/new_id
 
+What:		/sys/bus/pci/drivers/.../remove_id
+Date:		February 2009
+Contact:	Chris Wright <chrisw@sous-sol.org>
+Description:
+		Writing a device ID to this file will remove an ID
+		that was dynamically added via the new_id sysfs entry.
+		The format for the device ID is:
+		VVVV DDDD SVVV SDDD CCCC MMMM.	That is Vendor ID, Device
+		ID, Subsystem Vendor ID, Subsystem Device ID, Class,
+		and Class Mask.  The Vendor ID and Device ID fields are
+		required, the rest are optional.  After successfully
+		removing an ID, the driver will no longer support the
+		device.  This is useful to ensure auto probing won't
+		match the driver to the device.  For example:
+		# echo 8086 10f5 > /sys/bus/pci/drivers/foo/remove_id
+
 What:		/sys/bus/pci/devices/.../vpd
 Date:		February 2008
 Contact:	Ben Hutchings <bhutchings@solarflare.com>

Re: [PATCH 1/2] PCI: add some sysfs ABI docs

[Greg KH]

On Mon, Feb 23, 2009 at 06:17:25PM -0800, Chris Wright wrote:
> Add sysfs ABI docs for driver entries bind, unbind and new_id.  These
> entries are pretty old, from 2.6.0 onwards AFAIK, so this documents
> current behaviour.
> 
> Signed-off-by: Chris Wright <chrisw@sous-sol.org>
> ---
>  Documentation/ABI/testing/sysfs-bus-pci |   41 ++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
> 
> --- a/Documentation/ABI/testing/sysfs-bus-pci
> +++ b/Documentation/ABI/testing/sysfs-bus-pci
> @@ -1,3 +1,44 @@
> +What:		/sys/bus/pci/drivers/.../bind
> +Date:		December 2003
> +Contact:	linux-pci@vger.kernel.org
> +Description:
> +		Writing a device location to this file will cause
> +		the driver to attempt to bind to the device found at
> +		this location.	This is useful for overriding default
> +		bindings.  The format for the location is: DDDD:BB:DD.F.
> +		That is Domain:Bus:Device.Function and is the same as
> +		found in /sys/bus/pci/devices/.  For example:
> +		# echo 0000:00:19.0 > /sys/bus/pci/drivers/foo/bind

Don't you need 'echo -n' instead?  Or did we fix that problem?  Or is
that just for the new_id file?

If so, feel free to ignore the comment and add:
	Acked-by: Greg Kroah-Hartman <gregkh@suse.de>

Thanks a lot for doing this, it is much needed.

greg k-h

Re: [PATCH 2/2] PCI: add remove_id sysfs entry

[Greg KH]

On Mon, Feb 23, 2009 at 06:18:29PM -0800, Chris Wright wrote:
> This adds a remove_id sysfs entry to allow users of new_id to later
> remove the added dynid.  One use case is management tools that want to
> dynamically bind/unbind devices to pci-stub driver while devices are
> assigned to KVM guests.  Rather than having to track which driver was
> originally bound to the driver, a mangement tool can simply:
> 
> # echo "8086 10f5" > /sys/bus/pci/drivers/pci-stub/new_id
> # echo -n 0000:00:19.0 > /sys/bus/pci/devices/0000:00:19.0/driver/unbind
> # echo -n 0000:00:19.0 > /sys/bus/pci/drivers/pci-stub/bind

Ah, you do need -n with unbind and bind, you might want to change your
documentation :)

thanks,

greg k-h

Re: [PATCH 1/2] PCI: add some sysfs ABI docs

[Chris Wright]

* Greg KH (greg@kroah.com) wrote:
> On Mon, Feb 23, 2009 at 06:17:25PM -0800, Chris Wright wrote:
> > +		# echo 0000:00:19.0 > /sys/bus/pci/drivers/foo/bind
> 
> Don't you need 'echo -n' instead?  Or did we fix that problem?  Or is
> that just for the new_id file?

Not any more, bind/unbind using sysfs_streq now (I think as of 2.6.28).
And new_id doesn't need it either.

> If so, feel free to ignore the comment and add:
> 	Acked-by: Greg Kroah-Hartman <gregkh@suse.de>
> 
> Thanks a lot for doing this, it is much needed.

Re: [PATCH 2/2] PCI: add remove_id sysfs entry

[Chris Wright]

* Greg KH (greg@kroah.com) wrote:
> On Mon, Feb 23, 2009 at 06:18:29PM -0800, Chris Wright wrote:
> > This adds a remove_id sysfs entry to allow users of new_id to later
> > remove the added dynid.  One use case is management tools that want to
> > dynamically bind/unbind devices to pci-stub driver while devices are
> > assigned to KVM guests.  Rather than having to track which driver was
> > originally bound to the driver, a mangement tool can simply:
> > 
> > # echo "8086 10f5" > /sys/bus/pci/drivers/pci-stub/new_id
> > # echo -n 0000:00:19.0 > /sys/bus/pci/devices/0000:00:19.0/driver/unbind
> > # echo -n 0000:00:19.0 > /sys/bus/pci/drivers/pci-stub/bind
> 
> Ah, you do need -n with unbind and bind, you might want to change your
> documentation :)

Heh, it is confusing.  I'll just drop the -n from the changelog here,
and add a note to the docs that mention older kernels may need -n.

thanks,
-chris

Re: [PATCH 2/2] PCI: add remove_id sysfs entry

[Greg KH]

On Mon, Feb 23, 2009 at 09:33:13PM -0800, Chris Wright wrote:
> * Greg KH (greg@kroah.com) wrote:
> > On Mon, Feb 23, 2009 at 06:18:29PM -0800, Chris Wright wrote:
> > > This adds a remove_id sysfs entry to allow users of new_id to later
> > > remove the added dynid.  One use case is management tools that want to
> > > dynamically bind/unbind devices to pci-stub driver while devices are
> > > assigned to KVM guests.  Rather than having to track which driver was
> > > originally bound to the driver, a mangement tool can simply:
> > > 
> > > # echo "8086 10f5" > /sys/bus/pci/drivers/pci-stub/new_id
> > > # echo -n 0000:00:19.0 > /sys/bus/pci/devices/0000:00:19.0/driver/unbind
> > > # echo -n 0000:00:19.0 > /sys/bus/pci/drivers/pci-stub/bind
> > 
> > Ah, you do need -n with unbind and bind, you might want to change your
> > documentation :)
> 
> Heh, it is confusing.  I'll just drop the -n from the changelog here,
> and add a note to the docs that mention older kernels may need -n.

Sounds good to me.

thanks,

greg k-h

[PATCH 1/2 v2] PCI: add some sysfs ABI docs

[Chris Wright]

Add sysfs ABI docs for driver entries bind, unbind and new_id.  These
entries are pretty old, from 2.6.0 onwards AFAIK, so this documents
current behaviour.

Signed-off-by: Chris Wright <chrisw@sous-sol.org>
Acked-by: Greg Kroah-Hartman <gregkh@suse.de>
---
v2
 - add note re: older kernels and echo -n
 - add ack from gregkh

 Documentation/ABI/testing/sysfs-bus-pci |   43 ++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

--- a/Documentation/ABI/testing/sysfs-bus-pci
+++ b/Documentation/ABI/testing/sysfs-bus-pci
@@ -1,3 +1,46 @@
+What:		/sys/bus/pci/drivers/.../bind
+Date:		December 2003
+Contact:	linux-pci@vger.kernel.org
+Description:
+		Writing a device location to this file will cause
+		the driver to attempt to bind to the device found at
+		this location.	This is useful for overriding default
+		bindings.  The format for the location is: DDDD:BB:DD.F.
+		That is Domain:Bus:Device.Function and is the same as
+		found in /sys/bus/pci/devices/.  For example:
+		# echo 0000:00:19.0 > /sys/bus/pci/drivers/foo/bind
+		(Note: kernels before 2.6.28 may require echo -n).
+
+What:		/sys/bus/pci/drivers/.../unbind
+Date:		December 2003
+Contact:	linux-pci@vger.kernel.org
+Description:
+		Writing a device location to this file will cause the
+		driver to attempt to unbind from the device found at
+		this location.	This may be useful when overriding default
+		bindings.  The format for the location is: DDDD:BB:DD.F.
+		That is Domain:Bus:Device.Function and is the same as
+		found in /sys/bus/pci/devices/. For example:
+		# echo 0000:00:19.0 > /sys/bus/pci/drivers/foo/unbind
+		(Note: kernels before 2.6.28 may require echo -n).
+
+What:		/sys/bus/pci/drivers/.../new_id
+Date:		December 2003
+Contact:	linux-pci@vger.kernel.org
+Description:
+		Writing a device ID to this file will attempt to
+		dynamically add a new device ID to a PCI device driver.
+		This may allow the driver to support more hardware than
+		was included in the driver's static device ID support
+		table at compile time.  The format for the device ID is:
+		VVVV DDDD SVVV SDDD CCCC MMMM PPPP.  That is Vendor ID,
+		Device ID, Subsystem Vendor ID, Subsystem Device ID,
+		Class, Class Mask, and Private Driver Data.  The Vendor ID
+		and Device ID fields are required, the rest are optional.
+		Upon successfully adding an ID, the driver will probe
+		for the device and attempt to bind to it.  For example:
+		# echo "8086 10f5" > /sys/bus/pci/drivers/foo/new_id
+
 What:		/sys/bus/pci/devices/.../vpd
 Date:		February 2008
 Contact:	Ben Hutchings <bhutchings@solarflare.com>

[PATCH 2/2 v2] PCI: add remove_id sysfs entry

[Chris Wright]

This adds a remove_id sysfs entry to allow users of new_id to later
remove the added dynid.  One use case is management tools that want to
dynamically bind/unbind devices to pci-stub driver while devices are
assigned to KVM guests.  Rather than having to track which driver was
originally bound to the driver, a mangement tool can simply:

# echo "8086 10f5" > /sys/bus/pci/drivers/pci-stub/new_id
# echo 0000:00:19.0 > /sys/bus/pci/devices/0000:00:19.0/driver/unbind
# echo 0000:00:19.0 > /sys/bus/pci/drivers/pci-stub/bind

Guest uses device

# echo "8086 10f5" > /sys/bus/pci/drivers/pci-stub/remove_id
# echo 0000:00:19.0 > /sys/bus/pci/drivers_probe

Signed-off-by: Chris Wright <chrisw@sous-sol.org>
---
v2
 - update changelog to match ABI docs (remove echo -n)

 Documentation/ABI/testing/sysfs-bus-pci |   16 ++++++
 drivers/pci/pci-driver.c                |   80 +++++++++++++++++++++++++++++++-
 2 files changed, 94 insertions(+), 2 deletions(-)

--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -99,6 +99,52 @@ store_new_id(struct device_driver *drive
 }
 static DRIVER_ATTR(new_id, S_IWUSR, NULL, store_new_id);
 
+/**
+ * store_remove_id - remove a PCI device ID from this driver
+ * @driver: target device driver
+ * @buf: buffer for scanning device ID data
+ * @count: input size
+ *
+ * Removes a dynamic pci device ID to this driver.
+ */
+static ssize_t
+store_remove_id(struct device_driver *driver, const char *buf, size_t count)
+{
+	struct pci_dynid *dynid, *n;
+	struct pci_driver *pdrv = to_pci_driver(driver);
+	__u32 vendor, device, subvendor = PCI_ANY_ID,
+		subdevice = PCI_ANY_ID, class = 0, class_mask = 0;
+	int fields = 0;
+	int retval = -ENODEV;
+
+	fields = sscanf(buf, "%x %x %x %x %x %x",
+			&vendor, &device, &subvendor, &subdevice,
+			&class, &class_mask);
+	if (fields < 2)
+		return -EINVAL;
+
+	spin_lock(&pdrv->dynids.lock);
+	list_for_each_entry_safe(dynid, n, &pdrv->dynids.list, node) {
+		struct pci_device_id *id = &dynid->id;
+		if ((id->vendor == vendor) &&
+		    (id->device == device) &&
+		    (subvendor == PCI_ANY_ID || id->subvendor == subvendor) &&
+		    (subdevice == PCI_ANY_ID || id->subdevice == subdevice) &&
+		    !((id->class ^ class) & class_mask)) {
+			list_del(&dynid->node);
+			kfree(dynid);
+			retval = 0;
+			break;
+		}
+	}
+	spin_unlock(&pdrv->dynids.lock);
+
+	if (retval)
+		return retval;
+	return count;
+}
+static DRIVER_ATTR(remove_id, S_IWUSR, NULL, store_remove_id);
+
 static void
 pci_free_dynids(struct pci_driver *drv)
 {
@@ -125,6 +171,20 @@ static void pci_remove_newid_file(struct
 {
 	driver_remove_file(&drv->driver, &driver_attr_new_id);
 }
+
+static int
+pci_create_removeid_file(struct pci_driver *drv)
+{
+	int error = 0;
+	if (drv->probe != NULL)
+		error = driver_create_file(&drv->driver,&driver_attr_remove_id);
+	return error;
+}
+
+static void pci_remove_removeid_file(struct pci_driver *drv)
+{
+	driver_remove_file(&drv->driver, &driver_attr_remove_id);
+}
 #else /* !CONFIG_HOTPLUG */
 static inline void pci_free_dynids(struct pci_driver *drv) {}
 static inline int pci_create_newid_file(struct pci_driver *drv)
@@ -132,6 +192,11 @@ static inline int pci_create_newid_file(
 	return 0;
 }
 static inline void pci_remove_newid_file(struct pci_driver *drv) {}
+static inline int pci_create_removeid_file(struct pci_driver *drv)
+{
+	return 0;
+}
+static inline void pci_remove_removeid_file(struct pci_driver *drv) {}
 #endif
 
 /**
@@ -852,13 +917,23 @@ int __pci_register_driver(struct pci_dri
 	/* register with core */
 	error = driver_register(&drv->driver);
 	if (error)
-		return error;
+		goto out;
 
 	error = pci_create_newid_file(drv);
 	if (error)
-		driver_unregister(&drv->driver);
+		goto out_newid;
 
+	error = pci_create_removeid_file(drv);
+	if (error)
+		goto out_removeid;
+out:
 	return error;
+
+out_removeid:
+	pci_remove_newid_file(drv);
+out_newid:
+	driver_unregister(&drv->driver);
+	goto out;
 }
 
 /**
@@ -874,6 +949,7 @@ int __pci_register_driver(struct pci_dri
 void
 pci_unregister_driver(struct pci_driver *drv)
 {
+	pci_remove_removeid_file(drv);
 	pci_remove_newid_file(drv);
 	driver_unregister(&drv->driver);
 	pci_free_dynids(drv);
--- a/Documentation/ABI/testing/sysfs-bus-pci
+++ b/Documentation/ABI/testing/sysfs-bus-pci
@@ -41,6 +41,22 @@ Description:
 		for the device and attempt to bind to it.  For example:
 		# echo "8086 10f5" > /sys/bus/pci/drivers/foo/new_id
 
+What:		/sys/bus/pci/drivers/.../remove_id
+Date:		February 2009
+Contact:	Chris Wright <chrisw@sous-sol.org>
+Description:
+		Writing a device ID to this file will remove an ID
+		that was dynamically added via the new_id sysfs entry.
+		The format for the device ID is:
+		VVVV DDDD SVVV SDDD CCCC MMMM.	That is Vendor ID, Device
+		ID, Subsystem Vendor ID, Subsystem Device ID, Class,
+		and Class Mask.  The Vendor ID and Device ID fields are
+		required, the rest are optional.  After successfully
+		removing an ID, the driver will no longer support the
+		device.  This is useful to ensure auto probing won't
+		match the driver to the device.  For example:
+		# echo "8086 10f5" > /sys/bus/pci/drivers/foo/remove_id
+
 What:		/sys/bus/pci/devices/.../vpd
 Date:		February 2008
 Contact:	Ben Hutchings <bhutchings@solarflare.com>

Re: Re: KVM PCI device assignment issues

[Zhao, Yu]

Matthew Wilcox wrote:
> On Fri, Feb 13, 2009 at 04:32:47PM +0000, Mark McLoughlin wrote:
>>   - Secondary Bus Reset (SBR) allows software to trigger a reset on all
>>     devices (and functions) behind a PCI bridge.
>>
>>   - A PCI Power Management D-state transition (D3hot to D0) can be used
>>     to reset a device (all functions).
> 
> That's not guaranteed according to PCI PM 1.2:
> 
> 5.4.1. Software Accessible D3 (D3hot)
> 
>   When programmed to D0, the function may return to the D0 Initialized
>   or D0 Uninitialized state without PCI RST# being asserted. This option
>   is determined at design time and allows designs the option of either
>   performing an internal reset or not performing an internal reset.

The No_Soft_Reset bit in the PMCSR indicates which option is chosen at 
design time:

Section 3.2.4. says:
Value at Reset: Device specific
Read/Write: Read Only
When set (“1”), this bit indicates that devices transitioning from D3hot 
to D0 because of PowerState commands do not perform an internal reset. 
Configuration Context is preserved. Upon transition from the D3hot to 
the D0 Initialized state, no additional operating system intervention is 
required to preserve Configuration Context beyond writing the PowerState 
bits. When clear (“0”), devices do perform an internal reset upon 
transitioning from D3hot to D0 via software control of the PowerState 
bits. Configuration Context is lost when performing the soft reset. Upon 
  transition from the D3hot to the D0 state, full reinitialization 
sequence is needed to return the device to D0 Initialized. Regardless of 
this bit, devices that transition from D3hot to D0 by a system or bus 
segment reset will return to the device state D0 Uninitialized with only 
PME context preserved if PME is supported and enabled.

So the reset is guaranteed if the bit is 0.

And I checked the devices on my machine, all of them who have PM perform 
internal reset when transiting from D3hot to D0 (GeForce 7300, Myri-10G, 
E1000 82567, ICH10 SATA, EHCI, etc.)

Re: [PATCH 1/2 v2] PCI: add some sysfs ABI docs

[Jesse Barnes]

On Monday, February 23, 2009 9:50:35 pm Chris Wright wrote:
> Add sysfs ABI docs for driver entries bind, unbind and new_id.  These
> entries are pretty old, from 2.6.0 onwards AFAIK, so this documents
> current behaviour.
>
> Signed-off-by: Chris Wright <chrisw@sous-sol.org>
> Acked-by: Greg Kroah-Hartman <gregkh@suse.de>

Applied to my for-linus tree, thanks.

-- 
Jesse Barnes, Intel Open Source Technology Center

RE: [PATCH 2/2 v2] PCI: add remove_id sysfs entry

[Han, Weidong]

Chris Wright wrote:
> This adds a remove_id sysfs entry to allow users of new_id to later
> remove the added dynid.  One use case is management tools that want to
> dynamically bind/unbind devices to pci-stub driver while devices are
> assigned to KVM guests.  Rather than having to track which driver was
> originally bound to the driver, a mangement tool can simply:
> 
> # echo "8086 10f5" > /sys/bus/pci/drivers/pci-stub/new_id
> # echo 0000:00:19.0 > /sys/bus/pci/devices/0000:00:19.0/driver/unbind
> # echo 0000:00:19.0 > /sys/bus/pci/drivers/pci-stub/bind
> 
> Guest uses device
> 
> # echo "8086 10f5" > /sys/bus/pci/drivers/pci-stub/remove_id
> # echo 0000:00:19.0 > /sys/bus/pci/drivers_probe
> 

After above two commands, I found device 00:19.0 was still bound to pci-stub, and doesn't work. I found it needs following unbind command between remove_id and drivers_probe to make it work with original driver:
  # echo "8086 10f5" > /sys/bus/pci/drivers/pci-stub/unbind

Chris, did it also happen on your side? or did I miss something? 

Regards,
Weidong

> Signed-off-by: Chris Wright <chrisw@sous-sol.org>
> ---
> v2
>  - update changelog to match ABI docs (remove echo -n)
> 
>  Documentation/ABI/testing/sysfs-bus-pci |   16 ++++++
>  drivers/pci/pci-driver.c                |   80
>  +++++++++++++++++++++++++++++++- 2 files changed, 94 insertions(+),
> 2 deletions(-) 
> 
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -99,6 +99,52 @@ store_new_id(struct device_driver *drive
>  }
>  static DRIVER_ATTR(new_id, S_IWUSR, NULL, store_new_id);
> 
> +/**
> + * store_remove_id - remove a PCI device ID from this driver
> + * @driver: target device driver
> + * @buf: buffer for scanning device ID data
> + * @count: input size
> + *
> + * Removes a dynamic pci device ID to this driver.
> + */
> +static ssize_t
> +store_remove_id(struct device_driver *driver, const char *buf,
> size_t count) +{
> +	struct pci_dynid *dynid, *n;
> +	struct pci_driver *pdrv = to_pci_driver(driver);
> +	__u32 vendor, device, subvendor = PCI_ANY_ID,
> +		subdevice = PCI_ANY_ID, class = 0, class_mask = 0;
> +	int fields = 0;
> +	int retval = -ENODEV;
> +
> +	fields = sscanf(buf, "%x %x %x %x %x %x",
> +			&vendor, &device, &subvendor, &subdevice,
> +			&class, &class_mask);
> +	if (fields < 2)
> +		return -EINVAL;
> +
> +	spin_lock(&pdrv->dynids.lock);
> +	list_for_each_entry_safe(dynid, n, &pdrv->dynids.list, node) {
> +		struct pci_device_id *id = &dynid->id;
> +		if ((id->vendor == vendor) &&
> +		    (id->device == device) &&
> +		    (subvendor == PCI_ANY_ID || id->subvendor == subvendor) &&
> +		    (subdevice == PCI_ANY_ID || id->subdevice == subdevice) &&
> +		    !((id->class ^ class) & class_mask)) {
> +			list_del(&dynid->node);
> +			kfree(dynid);
> +			retval = 0;
> +			break;
> +		}
> +	}
> +	spin_unlock(&pdrv->dynids.lock);
> +
> +	if (retval)
> +		return retval;
> +	return count;
> +}
> +static DRIVER_ATTR(remove_id, S_IWUSR, NULL, store_remove_id);
> +
>  static void
>  pci_free_dynids(struct pci_driver *drv)
>  {
> @@ -125,6 +171,20 @@ static void pci_remove_newid_file(struct
>  {
>  	driver_remove_file(&drv->driver, &driver_attr_new_id);
>  }
> +
> +static int
> +pci_create_removeid_file(struct pci_driver *drv)
> +{
> +	int error = 0;
> +	if (drv->probe != NULL)
> +		error = driver_create_file(&drv->driver,&driver_attr_remove_id);
> +	return error;
> +}
> +
> +static void pci_remove_removeid_file(struct pci_driver *drv)
> +{
> +	driver_remove_file(&drv->driver, &driver_attr_remove_id);
> +}
>  #else /* !CONFIG_HOTPLUG */
>  static inline void pci_free_dynids(struct pci_driver *drv) {}
>  static inline int pci_create_newid_file(struct pci_driver *drv)
> @@ -132,6 +192,11 @@ static inline int pci_create_newid_file(
>  	return 0;
>  }
>  static inline void pci_remove_newid_file(struct pci_driver *drv) {}
> +static inline int pci_create_removeid_file(struct pci_driver *drv)
> +{
> +	return 0;
> +}
> +static inline void pci_remove_removeid_file(struct pci_driver *drv)
>  {} #endif
> 
>  /**
> @@ -852,13 +917,23 @@ int __pci_register_driver(struct pci_dri
>  	/* register with core */
>  	error = driver_register(&drv->driver);
>  	if (error)
> -		return error;
> +		goto out;
> 
>  	error = pci_create_newid_file(drv);
>  	if (error)
> -		driver_unregister(&drv->driver);
> +		goto out_newid;
> 
> +	error = pci_create_removeid_file(drv);
> +	if (error)
> +		goto out_removeid;
> +out:
>  	return error;
> +
> +out_removeid:
> +	pci_remove_newid_file(drv);
> +out_newid:
> +	driver_unregister(&drv->driver);
> +	goto out;
>  }
> 
>  /**
> @@ -874,6 +949,7 @@ int __pci_register_driver(struct pci_dri
>  void
>  pci_unregister_driver(struct pci_driver *drv)
>  {
> +	pci_remove_removeid_file(drv);
>  	pci_remove_newid_file(drv);
>  	driver_unregister(&drv->driver);
>  	pci_free_dynids(drv);
> --- a/Documentation/ABI/testing/sysfs-bus-pci
> +++ b/Documentation/ABI/testing/sysfs-bus-pci
> @@ -41,6 +41,22 @@ Description:
>  		for the device and attempt to bind to it.  For example:
>  		# echo "8086 10f5" > /sys/bus/pci/drivers/foo/new_id
> 
> +What:		/sys/bus/pci/drivers/.../remove_id
> +Date:		February 2009
> +Contact:	Chris Wright <chrisw@sous-sol.org>
> +Description:
> +		Writing a device ID to this file will remove an ID
> +		that was dynamically added via the new_id sysfs entry.
> +		The format for the device ID is:
> +		VVVV DDDD SVVV SDDD CCCC MMMM.	That is Vendor ID, Device
> +		ID, Subsystem Vendor ID, Subsystem Device ID, Class,
> +		and Class Mask.  The Vendor ID and Device ID fields are
> +		required, the rest are optional.  After successfully
> +		removing an ID, the driver will no longer support the
> +		device.  This is useful to ensure auto probing won't
> +		match the driver to the device.  For example:
> +		# echo "8086 10f5" > /sys/bus/pci/drivers/foo/remove_id
> +
>  What:		/sys/bus/pci/devices/.../vpd
>  Date:		February 2008
>  Contact:	Ben Hutchings <bhutchings@solarflare.com>

Re: [PATCH 2/2 v2] PCI: add remove_id sysfs entry

[Chris Wright]

* Han, Weidong (weidong.han@intel.com) wrote:
> Chris Wright wrote:
> > This adds a remove_id sysfs entry to allow users of new_id to later
> > remove the added dynid.  One use case is management tools that want to
> > dynamically bind/unbind devices to pci-stub driver while devices are
> > assigned to KVM guests.  Rather than having to track which driver was
> > originally bound to the driver, a mangement tool can simply:
> > 
> > # echo "8086 10f5" > /sys/bus/pci/drivers/pci-stub/new_id
> > # echo 0000:00:19.0 > /sys/bus/pci/devices/0000:00:19.0/driver/unbind
> > # echo 0000:00:19.0 > /sys/bus/pci/drivers/pci-stub/bind
> > 
> > Guest uses device
> > 
> > # echo "8086 10f5" > /sys/bus/pci/drivers/pci-stub/remove_id
> > # echo 0000:00:19.0 > /sys/bus/pci/drivers_probe
> 
> After above two commands, I found device 00:19.0 was still bound to pci-stub, and doesn't work. I found it needs following unbind command between remove_id and drivers_probe to make it work with original driver:
>   # echo "8086 10f5" > /sys/bus/pci/drivers/pci-stub/unbind
> 
> Chris, did it also happen on your side? or did I miss something? 

You're right.  I just forgot to put that in the changelog.

thanks,
-chris

Re: [PATCH 2/2 v2] PCI: add remove_id sysfs entry

[Jesse Barnes]

On Mon, 23 Feb 2009 21:52:23 -0800
Chris Wright <chrisw@sous-sol.org> wrote:

> This adds a remove_id sysfs entry to allow users of new_id to later
> remove the added dynid.  One use case is management tools that want to
> dynamically bind/unbind devices to pci-stub driver while devices are
> assigned to KVM guests.  Rather than having to track which driver was
> originally bound to the driver, a mangement tool can simply:
> 
> # echo "8086 10f5" > /sys/bus/pci/drivers/pci-stub/new_id
> # echo 0000:00:19.0 > /sys/bus/pci/devices/0000:00:19.0/driver/unbind
> # echo 0000:00:19.0 > /sys/bus/pci/drivers/pci-stub/bind
> 
> Guest uses device
> 
> # echo "8086 10f5" > /sys/bus/pci/drivers/pci-stub/remove_id
> # echo 0000:00:19.0 > /sys/bus/pci/drivers_probe
> 
> Signed-off-by: Chris Wright <chrisw@sous-sol.org>

Applied to linux-next, thanks Chris.

-- 
Jesse Barnes, Intel Open Source Technology Center