On 04/22/2013 12:43 PM, Ján Tomko wrote:
Add a "dry run" address allocation to figure out how many bridges will be needed for all the devices without explicit addresses.
Auto-add just enough bridges to put all the devices on, or up to the bridge with the largest specified index. ---
In addition to Laine's comments,
+ virBitmapPtr bitmap = NULL;
/* verify init path for container based domains */ if (STREQ(def->os.type, "exe") && !def->os.init) { @@ -2653,11 +2656,30 @@ virDomainDefPostParseInternal(virDomainDefPtr def, } }
- return 0; + /* Verify that PCI controller indexes are unique */ + bitmap = virBitmapNew(def->ncontrollers);
This limits the bitmap to the number of controllers that I passed in, but your commit message makes it sound like I can pass in a controller for index 1 and index 2 while letting the code auto-insert the controller for index 0. If that's true, then...
+ for (i = 0; i < def->ncontrollers; i++) { + if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) { + ignore_value(virBitmapGetBit(bitmap, def->controllers[i]->idx, &b));
...attempting to get bit 2 (based on the explicit 2 in def->controllers[i]->idx) will fail as out-of-range,...
+ if (b) { + virReportError(VIR_ERR_XML_ERROR, + _("Multiple PCI controllers with index %d"), + def->controllers[i]->idx); + goto cleanup; + } + ignore_value(virBitmapSetBit(bitmap, def->controllers[i]->idx)); + } + }
...and the attempt to set will also fail. Which means that a similar example of passing in two controllers that both try to use index 2, and let 0 and 1 be auto-populated, won't detect the collision. Do we know what the maximum index will be? Is it time to add a growable bitmap? Should we separate this duplicate detection code into a separate patch?
+/* Ensure addr fits in the address set, by expanding it if needed. + * Return value: + * -1 = OOM + * 0 = no action performed + * >0 = number of buses added + */ +static int +qemuDomainPCIAddressSetGrow(qemuDomainPCIAddressSetPtr addrs, + virDevicePCIAddressPtr addr)
Indentation is off.
@@ -1326,15 +1369,54 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, qemuDomainObjPrivatePtr priv = NULL;
if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { + int max_idx = 0; int nbuses = 0; int i; + int rv;
for (i = 0; i < def->ncontrollers; i++) { - if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) + if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) { + if (def->controllers[i]->idx > max_idx) + max_idx = def->controllers[i]->idx; nbuses++; + } + }
This looks like a more accurate determination of the max bus number; should you move the duplicate detection here instead?
+ + if (nbuses > 0 && max_idx >= nbuses) + nbuses = max_idx + 1;
You change nbuses, but only if it is already > 1, but then...
+ + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCI_BRIDGE)) { + virDomainDeviceInfo info; + /* 1st pass to figure out how many PCI bridges we need */ + if (!(addrs = qemuDomainPCIAddressSetCreate(def, nbuses, true))) + goto cleanup; + if (qemuAssignDevicePCISlots(def, qemuCaps, addrs) < 0) + goto cleanup; + /* Reserve 1 extra slot for a (potential) bridge */ + if (qemuDomainPCIAddressSetNextAddr(addrs, &info) < 0) + goto cleanup; + + for (i = 1; i < addrs->nbuses; i++) { + if ((rv = virDomainDefMaybeAddController( + def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, + i, VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE)) < 0) + goto cleanup; + /* If we added a new bridge, we will need one more address */ + if (rv > 0 && qemuDomainPCIAddressSetNextAddr(addrs, &info) < 0) + goto cleanup; + } + nbuses = addrs->nbuses; + qemuDomainPCIAddressSetFree(addrs); + addrs = NULL; + + } else if (nbuses > 1) {
...read nbuses if the capability is not present. Would it be any simpler to just change this to 'else if (max_idx > 0)'?
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("PCI bridges are not supported " + "by this QEMU binary")); + goto cleanup; }
- if (!(addrs = qemuDomainPCIAddressSetCreate(def, nbuses))) + if (!(addrs = qemuDomainPCIAddressSetCreate(def, nbuses, false))) goto cleanup;
if (qemuAssignDevicePCISlots(def, qemuCaps, addrs) < 0)
@@ -1519,41 +1609,58 @@ void qemuDomainPCIAddressSetFree(qemuDomainPCIAddressSetPtr addrs) VIR_FREE(addrs); }
- static int
I think we've been settling on two blank lines between functions lately, although I'm not bothered if you leave this change in.
qemuDomainPCIAddressGetNextSlot(qemuDomainPCIAddressSetPtr addrs, virDevicePCIAddressPtr next_addr) { - virDevicePCIAddress tmp_addr = addrs->lastaddr; - int i; - char *addr; + virDevicePCIAddress a = addrs->lastaddr;
- tmp_addr.slot++; - for (i = 0; i < QEMU_PCI_ADDRESS_SLOT_LAST; i++, tmp_addr.slot++) { - if (QEMU_PCI_ADDRESS_SLOT_LAST <= tmp_addr.slot) { - tmp_addr.slot = 0; - } + if (addrs->nbuses == 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", _("No PCI buses available")); + return -1; + }
addrs->nbuses should always be >= 1, now that we allocate it, right? Is it possible to hit this error?
- if (!(addr = qemuPCIAddressAsString(&tmp_addr))) - return -1; + /* Start the search at the last used bus and slot */ + for (a.slot++; a.bus < addrs->nbuses; a.bus++) { + for ( ; a.slot < QEMU_PCI_ADDRESS_SLOT_LAST; a.slot++) { + if (!qemuDomainPCIAddressSlotInUse(addrs, &a)) + goto success;
- if (qemuDomainPCIAddressSlotInUse(addrs, &tmp_addr)) { - VIR_DEBUG("PCI addr %s already in use", addr); - VIR_FREE(addr); - continue; + VIR_DEBUG("PCI slot %.4x:%.2x:%.2x already in use", + a.domain, a.bus, a.slot); } + a.slot = 1; + }
- VIR_DEBUG("Found free PCI addr %s", addr); - VIR_FREE(addr); + /* There were no free slots after the last used one */ + if (addrs->dryRun) { + /* a is already set to the first new bus and slot 1 */ + if (qemuDomainPCIAddressSetGrow(addrs, &a) < 0) + return -1; + goto success; + } else { + /* Check the buses from 0 up to the last used one */ + for (a.bus = 0; a.bus <= addrs->lastaddr.bus; a.bus++) { + for (a.slot = 1; a.slot < QEMU_PCI_ADDRESS_SLOT_LAST; a.slot++) { + if (!qemuDomainPCIAddressSlotInUse(addrs, &a)) + goto success;
- addrs->lastaddr = tmp_addr; - *next_addr = tmp_addr; - return 0; + VIR_DEBUG("PCI slot %.4x:%.2x:%.2x already in use", + a.domain, a.bus, a.slot); + } + + } }
This wraparound logic was definitely easier to read than the v2 attempt.
virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("No more available PCI addresses")); return -1; + +success: + VIR_DEBUG("Found free PCI slot %.4x:%.2x:%.2x", + a.domain, a.bus, a.slot); + *next_addr = a; + return 0; }
int qemuDomainPCIAddressSetNextAddr(qemuDomainPCIAddressSetPtr addrs,
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org