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

[Ján Tomko]

On 04/25/2013 02:39 AM, Eric Blake wrote:
> On 04/23/2013 06:47 AM, Ján Tomko wrote:
>> @@ -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?

No, if the machine has no buses, we would have no place to put the
bridge in. (Unless it's a PCI express machine, but libvirt doesn't know
how to use those yet)

And we'll error out anyway with:
error: XML error: No PCI buses available
either in GetNextSlot called by AssignPCISlots for devices without an
address, or in PCIAddressValidate called by CollectPCIAddress for
explicitly specified PCI addresses.

Jan