1:50 p.m.
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
}
}