[libvirt] [PATCHv4 5/5] qemu: auto-add bridges and allow using them

[Eric Blake]

On 04/23/2013 06:47 AM, 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.
> ---
> 
> v4:
> Moved the check for duplicate controller indexes to a separate patch
> Simplified nbuses and max_idx computation
> Only does two-pass allocation of PCI addresses if the machine has a PCI bus
> Does not contain traces of rebasing or spurious whitespace changes
> Tests auto-adding PCI bridges in xml->argv and xml->xml tests.
> 
> @@ -1233,9 +1236,45 @@ static bool qemuPCIAddressValidate(qemuDomainPCIAddressSetPtr addrs ATTRIBUTE_UN
>                         QEMU_PCI_ADDRESS_SLOT_LAST);
>          return false;
>      }
> +    if (addr->slot == 0) {
> +        if (addr->bus) {
> +            virReportError(VIR_ERR_XML_ERROR, "%s",
> +                           _("Slot 0 is unusable on PCI bridges"));
> +        } else {
> +            virReportError(VIR_ERR_XML_ERROR, "%s",
> +                          _("Slot 0 on bus 0 is reserved for the host bridge"));

Indentation is off.

> @@ -1326,15 +1368,53 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
>      qemuDomainObjPrivatePtr priv = NULL;
>  
>      if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) {
> +        int max_idx = -1;

So let's trace what happens if I have XML with no <controller> but I do
use 33 PCI devices and have a capable qemu:

max_idx starts at -1,

>          int nbuses = 0;
>          int i;
> +        int rv;
>  
>          for (i = 0; i < def->ncontrollers; i++) {
> -            if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI)
> -                nbuses++;
> +            if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) {
> +                if (def->controllers[i]->idx > max_idx)
> +                    max_idx = def->controllers[i]->idx;
> +            }
> +        }

If no controllers were specified, it is still at -1,

> +
> +        nbuses = max_idx + 1;

so nbuses is now 0,

> +
> +        if (nbuses > 0 &&
> +            virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCI_BRIDGE)) {

therefore we skip this if,

> +            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 (max_idx > 0) {

we don't error out,

> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                           _("PCI bridges are not supported "
> +                             "by this QEMU binary"));
> +            goto cleanup;
>          }

but we also didn't auto-instantiate any bridges, even if the capability
is supported.  Is that a problem?

ACK if you can answer my question and fix the minor issues.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130424/5d6d6797/attachment-0001.sig>