The code in libvirt that does ACS checking has a pretty serious bug that was essentially rendering the check useless. When trying to assign a device, we have to check that all bridges upstream of this device support ACS. That means that we have to find the parent of the current device, check for ACS, then find the parent of that device, check for ACS, etc. However, the code to find the parent of the device had a much too relaxed check. It would iterate through all PCI devices on the system, looking for a device that had a range of busses that included the current device's bus. That's not correct, though. Depending on how we iterated through the list of PCI devices, we could first find the *topmost* bridge in the system; since it necessarily had a range of busses including the current device's bus, we would only ever check the topmost bridge, and not check any of the intermediate bridges. This patch is simple in that it looks for the PCI device whose secondary device *exactly* equals the bus of the device we are looking for. That means that one, and only one bridge will be found, and it will be the correct device. Note that this also caused a fairly serious bug in the secondary bus reset code, where we could erroneously reset the topmost bus instead of the inner bus. This patch was tested by me on a 4-port NIC with a bridge without ACS (where assignment failed), a 4-port NIC with a bridge with ACS (where assignment succeeded), and a 2-port NIC with no bridges (where assignment succeeded). Signed-off-by: Chris Lalancette <clalance(a)redhat.com&gt; --- src/util/pci.c | 5 ++--- 1 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/util/pci.c b/src/util/pci.c index 26d55b8..df30e04 100644 --- a/src/util/pci.c +++ b/src/util/pci.c @@ -513,7 +513,7 @@ static int pciIsParent(pciDevice *dev, pciDevice *check, void *data ATTRIBUTE_UNUSED) { uint16_t device_class; - uint8_t header_type, secondary, subordinate; + uint8_t header_type, secondary; if (dev->domain != check->domain) return 0; @@ -529,12 +529,11 @@ pciIsParent(pciDevice *dev, pciDevice *check, void *data ATTRIBUTE_UNUSED) return 0; secondary = pciRead8(check, PCI_SECONDARY_BUS); - subordinate = pciRead8(check, PCI_SUBORDINATE_BUS); VIR_DEBUG("%s %s: found parent device %s", dev->id, dev->name, check->name); /* No, it's superman! */ - return (dev->bus >= secondary && dev->bus <= subordinate); + return (dev->bus == secondary); } static pciDevice *

If detecting the FLR flag of a pci device fails, then we could run into the situation of trying to close a file descriptor twice, once in pciInitDevice() and once in pciFreeDevice(). Fix that by removing the pciCloseConfig() in pciInitDevice() and just letting pciFreeDevice() handle it. Thanks to Chris Wright for pointing out this problem. While we are at it, fix an error check. While it would actually work as-is (since success returns 0), it's still more clear to check for < 0 (as the rest of the code does). Signed-off-by: Chris Lalancette <clalance(a)redhat.com&gt; --- src/qemu/qemu_driver.c | 2 +- src/util/pci.c | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b4c468a..9436e2a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8010,7 +8010,7 @@ static int qemudDomainAttachHostPciDevice(struct qemud_driver *driver, return -1; } - if (qemuPrepareHostdevPCIDevices(driver, &hostdev, 1)) + if (qemuPrepareHostdevPCIDevices(driver, &hostdev, 1) < 0) return -1; if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) { diff --git a/src/util/pci.c b/src/util/pci.c index 14ef058..26d55b8 100644 --- a/src/util/pci.c +++ b/src/util/pci.c @@ -189,8 +189,10 @@ pciCloseConfig(pciDevice *dev) if (!dev) return; - if (dev->fd >= 0) + if (dev->fd >= 0) { close(dev->fd); + dev->fd = -1; + } } static int @@ -673,10 +675,8 @@ pciInitDevice(pciDevice *dev) dev->pcie_cap_pos = pciFindCapabilityOffset(dev, PCI_CAP_ID_EXP); dev->pci_pm_cap_pos = pciFindCapabilityOffset(dev, PCI_CAP_ID_PM); flr = pciDetectFunctionLevelReset(dev); - if (flr < 0) { - pciCloseConfig(dev); + if (flr < 0) return flr; - } dev->has_flr = flr; dev->has_pm_reset = pciDetectPowerManagementReset(dev); dev->initted = 1;