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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list