Re: [libvirt] [PATCHv2 02/17] conf: pay attention to bus minSlot/maxSlot when autoassigning PCI addresses

On 07/17/2015 02:43 PM, Laine Stump wrote: > The function that auto-assigns PCI addresses was written with the > hardcoded assumptions that any PCI bus would have slots available > starting at 1 and ending at 31. This isn't true for many types of > controller (some have a single slot/port at 0, some have slots/ports > from 0 to 31). This patch updates that function to remove the > hardcoded assumptions. It will properly find/assign addresses for > devices that can only connect to pcie-(root|downstream)-port (which > have minSlot/maxSlot of 0/0) or a pcie-switch-upstream-port (0/31). > > It still will not auto-create a new bus of the proper kind for these > connections when one doesn't exist, that task is for another day. > --- > new in V2 > > src/conf/domain_addr.c | 65 +++++++++++++++++++++++++++++--------------------- > 1 file changed, 38 insertions(+), 27 deletions(-) > > diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c > index 2be98c5..bc09279 100644 > --- a/src/conf/domain_addr.c > +++ b/src/conf/domain_addr.c > @@ -471,24 +471,30 @@ virDomainPCIAddressGetNextSlot(virDomainPCIAddressSetPtr addrs, > virDomainPCIConnectFlags flags) > { > /* default to starting the search for a free slot from > - * 0000:00:00.0 > + * the first slot of domain 0 bus 0... > */ > virDevicePCIAddress a = { 0, 0, 0, 0, false }; > char *addrStr = NULL; > > - /* except if this search is for the exact same type of device as > - * last time, continue the search from the previous match > - */ > - if (flags == addrs->lastFlags) > - a = addrs->lastaddr; > - > if (addrs->nbuses == 0) { > virReportError(VIR_ERR_XML_ERROR, "%s", _("No PCI buses available")); > goto error; > } > > - /* Start the search at the last used bus and slot */ > - for (a.slot++; a.bus < addrs->nbuses; a.bus++) { > + /* ...unless this search is for the exact same type of device as > + * last time, then continue the search from the next slot after > + * the previous match. next slot and possibly first slot of next bus > + */ > + if (flags == addrs->lastFlags) { > + a = addrs->lastaddr; > + if (++a.slot > addrs->buses[a.bus].maxSlot && > + ++a.bus < addrs->nbuses) > + a.slot = addrs->buses[a.bus].minSlot; > + } else { > + a.slot = addrs->buses[0].minSlot; > + } > + > + while (a.bus < addrs->nbuses) { > if (!(addrStr = virDomainPCIAddressAsString(&a))) > goto error; > if (!virDomainPCIAddressFlagsCompatible(&a, addrStr, > @@ -497,29 +503,33 @@ virDomainPCIAddressGetNextSlot(virDomainPCIAddressSetPtr addrs, > VIR_FREE(addrStr); I think with the new logic to use "if / else" rather than "if ... continue;", this VIR_FREE is unnecessary since it's done at then end of the while loop

> VIR_DEBUG("PCI bus %.4x:%.2x is not compatible with the device", > a.domain, a.bus); > - continue; > - } > - for (; a.slot <= VIR_PCI_ADDRESS_SLOT_LAST; a.slot++) { > - if (!virDomainPCIAddressSlotInUse(addrs, &a)) > - goto success; > + } else { > + while (a.slot <= addrs->buses[a.bus].maxSlot) { > + if (!virDomainPCIAddressSlotInUse(addrs, &a)) > + goto success; > > - VIR_DEBUG("PCI slot %.4x:%.2x:%.2x already in use", > - a.domain, a.bus, a.slot); > + VIR_DEBUG("PCI slot %.4x:%.2x:%.2x already in use", > + a.domain, a.bus, a.slot); > + a.slot++; > + } > } > - a.slot = 1; > + if (++a.bus < addrs->nbuses) > + a.slot = addrs->buses[a.bus].minSlot; > VIR_FREE(addrStr); > } > > /* There were no free slots after the last used one */ So essentially we're going to search everything before to see if there's any openings to use. > if (addrs->dryRun) { > - /* a is already set to the first new bus and slot 1 */ > + /* a is already set to the first new bus */ > if (virDomainPCIAddressSetGrow(addrs, &a, flags) < 0) > goto error; > + /* this device will use the first slot of the new bus */ > + a.slot = addrs->buses[a.bus].minSlot; > goto success; > } else if (flags == addrs->lastFlags) { > /* Check the buses from 0 up to the last used one */ > for (a.bus = 0; a.bus <= addrs->lastaddr.bus; a.bus++) { > - addrStr = NULL; > + a.slot = addrs->buses[a.bus].minSlot; > if (!(addrStr = virDomainPCIAddressAsString(&a))) > goto error; > if (!virDomainPCIAddressFlagsCompatible(&a, addrStr, > @@ -527,14 +537,15 @@ virDomainPCIAddressGetNextSlot(virDomainPCIAddressSetPtr addrs, > flags, false, false)) { > VIR_DEBUG("PCI bus %.4x:%.2x is not compatible with the device", > a.domain, a.bus); > - continue; > - } > - for (a.slot = 1; a.slot <= VIR_PCI_ADDRESS_SLOT_LAST; a.slot++) { > - if (!virDomainPCIAddressSlotInUse(addrs, &a)) > - goto success; > - > - VIR_DEBUG("PCI slot %.4x:%.2x:%.2x already in use", > - a.domain, a.bus, a.slot); > + } else { > + while (a.slot <= addrs->buses[a.bus].maxSlot) { > + if (!virDomainPCIAddressSlotInUse(addrs, &a)) > + goto success; > + > + VIR_DEBUG("PCI slot %.4x:%.2x:%.2x already in use", > + a.domain, a.bus, a.slot); > + a.slot++; > + } > } Perhaps preexisting, but one would think a VIR_FREE(addrStr) would be needed here just as it was in the first pass... [Coverity didn't find this either]

ACK with the adjustment John > } > } >