[libvirt] [PATCH 3/5] qemu: Fix auto-adding PCI bridge when all slots are reserved

[Erik Skultety]

On 01/22/2015 05:00 PM, Ján Tomko wrote:
> On 01/21/2015 05:50 PM, Erik Skultety wrote:
>> Commit 93c8ca tried to fix the issue with auto-adding of a PCI bridge
>> controller, it worked well when the slots were reserved by devices with
>> user defined addresses without any other devices with unspecified PCI addresses
>> present in the XML.
>> However, if another device with unspecified PCI addresses appeared in
>> the domain's XML, the same problem occurred as the BZ below references.
>>
>> This patch fixes the issue by not creating any additional bridges when not
>> necessary.
>> Now let's say all the buses corresponding to the number of PCI controllers
>> present in the domain's XML got fully reserved by devices with user defined
>> addresses. If there are no other devices present in the XML, no need to
>> create both an additional PCI bus and a bridge controller.
>> If however there are still some PCI devices waiting to get a slot assigned
>> by qemuAssignDevicePCISlots, this means an additional bus needs to
>> be created along with a corresponding bridge controller.
>> This scenario now results in an reasonable error instead of generating wrong qemu
>> command line.
>>
>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1132900
>> ---
>>  src/qemu/qemu_command.c | 42 ++++++++++++++++++++++++++++++++++--------
>>  1 file changed, 34 insertions(+), 8 deletions(-)
>>
> 
> This breaks the pci-many test case you added before.
> 
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index cdf02a6..99b06ee 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -1472,6 +1472,9 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
>>          int nbuses = 0;
>>          size_t i;
>>          int rv;
>> +        int resflags = 0;
>> +        bool buses_reserved = false;
>> +
> 
> If you set this to true by default...
> 
>>          virDomainPCIConnectFlags flags = VIR_PCI_CONNECT_TYPE_PCI;
>>  
>>          for (i = 0; i < def->ncontrollers; i++) {
>> @@ -1490,17 +1493,22 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
>>              /* 1st pass to figure out how many PCI bridges we need */
>>              if (!(addrs = qemuDomainPCIAddressSetCreate(def, nbuses, true)))
>>                  goto cleanup;
>> +
>> +            for (i = 0; i < nbuses; i++) {
> 
>> +                if (qemuDomainPCIBusFullyReserved(&addrs->buses[i]))
>> +                    resflags |= 1 << i;
> 
> you can do:
> if (!fullyReserved(buses[i]))
>   buses_reserved = false;
> 

Yeah, that one looks better, thanks.

> Or just rename the bool to something like 'addExtraEmptySlot' and invert the
> condition below. (To stay more positive :))
> 
>> +            }
>> +            buses_reserved = (resflags && ~((~0) << nbuses));
>> +

Not to mention this one should have been bitwise AND instead of logical
one anyway.

>>              if (qemuAssignDevicePCISlots(def, qemuCaps, addrs) < 0)
>>                  goto cleanup;
>>  
>> -            for (i = 0; i < addrs->nbuses; i++) {
>> -                if (!qemuDomainPCIBusFullyReserved(&addrs->buses[i])) {
>> -
>> -                    /* Reserve 1 extra slot for a (potential) bridge */
>> -                    if (virDomainPCIAddressReserveNextSlot(addrs, &info, flags) < 0)
>> -                        goto cleanup;
>> -                }
>> -            }
>> +            /* Reserve 1 extra slot for a (potential) bridge only if buses
>> +             * are not fully reserved yet
>> +             */
>> +            if (!buses_reserved &&
>> +                virDomainPCIAddressReserveNextSlot(addrs, &info, flags) < 0)
>> +                goto cleanup;
>>  
>>              for (i = 1; i < addrs->nbuses; i++) {
>>                  virDomainPCIAddressBusPtr bus = &addrs->buses[i];
>> @@ -1532,6 +1540,24 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
>>              if (qemuAssignDevicePCISlots(def, qemuCaps, addrs) < 0)
>>                  goto cleanup;
>>          }
>> +
>> +        for (i = 0; i < def->ncontrollers; i++) {
>> +            /* check if every PCI bridge controller's ID is greater than
>> +             * the bus it is placed onto
>> +             */
>> +            virDomainControllerDefPtr cont = def->controllers[i];
>> +            int idx = cont->idx;
>> +            int bus = cont->info.addr.pci.bus;
>> +            if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI &&
>> +                cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE &&
>> +                idx <= bus) {
>> +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> +                               _("failed to create PCI bridge "
>> +                                 "on bus %d: bus is fully reserved"),
>> +                               bus);
>> +                goto cleanup;
>> +            }
>> +        }
> 
> This should IMHO be a separate commit, as it does not fix the case where the
> devices exactly fill the buses, but when there are too many.
> 

You're probably right, coming in v2 series.

> Also, maybe the error message could say something about 'too many devices with
> fixed addressess'?
> 
> Jan
> 
> 
> 
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
> 
Erik