[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v3 5/5] qemu: auto-add bridges and allow using them



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]