On Mon, Sep 03, 2012 at 04:57:01PM +0800, Osier Yang wrote:

Previously it refuses to do the secondary bus reset as long as there is(are) devices/functions behind the same bus, regardless of whether the devices/functions are being used or not. And it only save and restore the device itself's PCI config space.

But later it was changed to allow the secondary bus reset as long as the devices/functions behind the same bus are not being used. Unfortunately, it still just saves and restores the device itself's PCI config space. It means we will lose the PCI config space for the devices share same bus when doing passthrough.

Also, (hope my guess is right) as it assumes the secondary reset is allowed unless the device doesn't have devices/functions behind the same bus, so it only reads the bridge control register from the device, but not the parent.

This patch fixes the problem by finding out all the devices/functions behind the same bus of the device to be reset, and save/restore PCI config space for all of them. And read the bridge control register from the device's parent (bridge) before resetting.

* src/util/pci.c: - New helper pciSharesBus to check if two devices share same bus.

- New helper pciDevicesShareBus to return a list containg all of the devices/functions which share same bus with the device

- pciTrySecondaryBusReset: Save and restore PCI config space for all the devices/functions behind the same bus; Read the bridge control register from the device's parent instead before resetting. --- src/util/pci.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++++++++--- 1 files changed, 75 insertions(+), 5 deletions(-)

diff --git a/src/util/pci.c b/src/util/pci.c index 0742d07..1a9777a 100644 --- a/src/util/pci.c +++ b/src/util/pci.c @@ -517,6 +517,39 @@ pciBusContainsActiveDevices(pciDevice *dev, return active; }

+/* + * Check if the @dev and @check share bus. + */ +static int +pciSharesBus(pciDevice *dev, pciDevice *check, void *data ATTRIBUTE_UNUSED) +{ + if ((dev->domain == check->domain) && + (dev->bus == check->bus) && + (dev->slot == check->slot)) + return 1; + + return 0; +}

I'm a bit puzzled ... seems to me the function check if the devices share the same slot on the same bus, not just sharing of the same bus

+/* + * Return all the devices/functions share same bus with @dev + * as a list. + */ +static pciDeviceList * +pciDevicesShareBus(pciDevice *dev) +{ + pciDevice *match = NULL; + pciDeviceList *pcis = NULL; + + if (!(pcis = pciDeviceListNew())) + return NULL; + + if (pciIterDevices(pciSharesBus, dev, &match, NULL)) + pciDeviceListAdd(pcis, match); + + return pcis; +} + /* Is @check the parent of @dev ? */ static int pciIsParent(pciDevice *dev, pciDevice *check, void *data) @@ -604,6 +637,9 @@ pciTrySecondaryBusReset(pciDevice *dev, uint8_t config_space[PCI_CONF_LEN]; uint16_t ctl; int ret = -1; + pciDeviceList *list = NULL; + uint8_t (*config_spaces)[PCI_CONF_LEN]; + int i;

/* Refuse to do a secondary bus reset if there are other * devices/functions behind the bus are used by the host @@ -628,10 +664,7 @@ pciTrySecondaryBusReset(pciDevice *dev,

VIR_DEBUG("%s %s: doing a secondary bus reset", dev->id, dev->name);

- /* Save and restore the device's config space; we only do this - * for the supplied device since we refuse to do a reset if there - * are multiple devices/functions - */ + /* Save the device's config space */ if (pciRead(dev, 0, config_space, PCI_CONF_LEN) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to read PCI config space for %s"), @@ -639,10 +672,29 @@ pciTrySecondaryBusReset(pciDevice *dev, goto out; }

+ /* Save the config space of devices behind the same bus */ + if ((list = pciDevicesShareBus(dev))) { + if (VIR_ALLOC_N(config_spaces, list->count) < 0) { + virReportOOMError(); + goto out; + } + + for (i = 0; i < list->count; i++) { + pciDevice *pci = list->devs[i]; + + if (pciRead(pci, 0, config_spaces[i], PCI_CONF_LEN) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to read PCI config space for %s"), + pci->name); + goto out; + } + } + } + /* Read the control register, set the reset flag, wait 200ms, * unset the reset flag and wait 200ms. */ - ctl = pciRead16(dev, PCI_BRIDGE_CONTROL); + ctl = pciRead16(parent, PCI_BRIDGE_CONTROL);

pciWrite16(parent, PCI_BRIDGE_CONTROL, ctl | PCI_BRIDGE_CTL_RESET);

@@ -652,14 +704,32 @@ pciTrySecondaryBusReset(pciDevice *dev,

usleep(200 * 1000); /* sleep 200ms */

+ /* Restore the device's config space */ if (pciWrite(dev, 0, config_space, PCI_CONF_LEN) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to restore PCI config space for %s"), dev->name); goto out; } + + /* Restore the config space of devices behind the same bus */ + if (list) { + for (i = 0; i < list->count; i++) { + pciDevice *pci = list->devs[i]; + + if (pciWrite(pci, 0, config_spaces[i], PCI_CONF_LEN) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to restore PCI config space for %s"), + pci->name); + goto out; + } + } + } + ret = 0; out: + pciDeviceListFree(list); + VIR_FREE(config_spaces); pciFreeDevice(parent); return ret; }

Otherwise this seems to make sense to me, but it would be good if someone else could review too before ACK'ing as this is rather sensitive code ACK/2 Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/