Re: [libvirt] [PATCH] Fix the ACS checking in the PCI code.

> diff --git a/src/util/pci.c b/src/util/pci.c > index 26d55b8..f2890bd 100644 > --- a/src/util/pci.c > +++ b/src/util/pci.c > @@ -283,6 +283,7 @@ pciIterDevices(pciIterPredicate predicate, > DIR *dir; > struct dirent *entry; > int ret = 0; > + int rc; > > *matched = NULL; > > @@ -322,11 +323,20 @@ pciIterDevices(pciIterPredicate predicate, > break; > } > > - if (predicate(dev, check, data)) { > + rc = predicate(dev, check, data); > + if (rc < 0) { > + /* the predicate returned an error, bail */ > + pciFreeDevice(check); > + ret = -1; > + break; > + } > + else if (rc == 1) { CodingStyle? $ git grep "else {" | wc -l 1150 $ git grep "else {" | grep -v } | wc -l 67 Guess it's not unprecedented, just rare ;)

> VIR_DEBUG("%s %s: iter matched on %s", dev->id, dev->name, check->name); > *matched = check; > + ret = 1; > break; > } > + > pciFreeDevice(check); > } > closedir(dir); > @@ -510,10 +520,11 @@ pciBusContainsActiveDevices(pciDevice *dev, > > /* Is @check the parent of @dev ? */ > static int > -pciIsParent(pciDevice *dev, pciDevice *check, void *data ATTRIBUTE_UNUSED) > +pciIsParent(pciDevice *dev, pciDevice *check, void *data) > { > uint16_t device_class; > uint8_t header_type, secondary, subordinate; > + pciDevice **best = data; > > if (dev->domain != check->domain) > return 0; > @@ -533,16 +544,54 @@ pciIsParent(pciDevice *dev, pciDevice *check, void *data ATTRIBUTE_UNUSED) > > 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); > + /* if the secondary bus exactly equals the device's bus, then we found > + * the direct parent. No further work is necessary > + */ > + if (dev->bus == secondary) > + return 1; > + > + /* otherwise, SRIOV allows VFs to be on different busses then their PFs. > + * In this case, what we need to do is look for the "best" match; i.e. > + * the most restrictive match that still satisfies all of the conditions. > + */ > + if (dev->bus > secondary && dev->bus <= subordinate) { > + if (*best == NULL) { > + *best = pciGetDevice(check->domain, check->bus, check->slot, > + check->function); > + if (*best == NULL) > + return -1; too bad it's not refcounted, then we'd never hit a failure case here

> + } > + else { CodingStyle? > + /* OK, we had already recorded a previous "best" match for the > + * parent. See if the current device is more restrictive than the > + * best, and if so, make it the new best > + */ > + if (secondary > pciRead8(*best, PCI_SECONDARY_BUS)) { forgot it's not cached, at least this isn't a performance senstitive path > + pciFreeDevice(*best); > + *best = pciGetDevice(check->domain, check->bus, check->slot, > + check->function); > + if (*best == NULL) > + return -1; > + } > + } > + } Yes, should get the right match. > + > + return 0; > } > > -static pciDevice * > -pciGetParentDevice(pciDevice *dev) > +static int > +pciGetParentDevice(pciDevice *dev, pciDevice **parent) > { > - pciDevice *parent = NULL; > - pciIterDevices(pciIsParent, dev, &parent, NULL); > - return parent; > + pciDevice *best = NULL; > + int ret; > + > + *parent = NULL; > + ret = pciIterDevices(pciIsParent, dev, parent, &best); > + if (ret == 1) > + pciFreeDevice(best); > + else if (ret == 0) > + *parent = best; > + return ret; I think we could keep the old interface, but that's really just splitting hairs, and hey...you did the work already ;)

> } > > /* Secondary Bus Reset is our sledgehammer - it resets all > @@ -570,7 +619,8 @@ pciTrySecondaryBusReset(pciDevice *dev, > } > > /* Find the parent bus */ > - parent = pciGetParentDevice(dev); > + if (pciGetParentDevice(dev, &parent) < 0) > + return -1; > if (!parent) { > pciReportError(VIR_ERR_NO_SUPPORT, > _("Failed to find parent device for %s"), > @@ -1377,7 +1427,8 @@ pciDeviceIsBehindSwitchLackingACS(pciDevice *dev) > { > pciDevice *parent; > > - parent = pciGetParentDevice(dev); > + if (pciGetParentDevice(dev, &parent) < 0) > + return -1; > if (!parent) { > /* if we have no parent, and this is the root bus, ACS doesn't come > * into play since devices on the root bus can't P2P without going > @@ -1400,6 +1451,7 @@ pciDeviceIsBehindSwitchLackingACS(pciDevice *dev) > do { > pciDevice *tmp; > int acs; > + int ret; > > acs = pciDeviceDownstreamLacksACS(parent); > > @@ -1412,8 +1464,10 @@ pciDeviceIsBehindSwitchLackingACS(pciDevice *dev) > } > > tmp = parent; > - parent = pciGetParentDevice(parent); > + ret = pciGetParentDevice(parent, &parent); > pciFreeDevice(tmp); > + if (ret < 0) > + return -1; > } while (parent); > > return 0; Acked-by: Chris Wright <chrisw(a)redhat.com&gt;