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

[Chris Lalancette]

On 07/31/10 - 11:18:05AM, Chris Wright wrote:
> > 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 ;)

Heh, yeah, that's a habit of mine.  libvirt does tend to do the other style
but *shrug*

> 
> >              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

Yeah, this is the part of the patch I'm least happy about, but this should
do the right thing.

> 
> > +        }
> > +        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 ;)

Well, I really need the "int" return value here to distinguish the error case,
which is why I ended up changing it here.  It's an internal interface, so
I'm not too sad :).

> 
> >  }
> >  
> >  /* 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 at redhat.com>

Thanks, I've pushed this now.

-- 
Chris Lalancette