On 07/22/2015 02:50 PM, John Ferlan wrote:
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
Correct. Thanks!
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]
Yes! Another good catch! I'm surprised that this didn't lead to any valgrind or coverity reports before now - that loop has been missing a VIR_FREE(addrStr) for a very long time.
ACK with the adjustment
John
} }