[libvirt] [PATCHv2 02/17] conf: pay attention to bus minSlot/maxSlot when autoassigning PCI addresses

[Laine Stump]

On 07/22/2015 02:50 PM, John Ferlan wrote:
>
> On 07/17/2015 02:43 PM, Laine Stump wrote:
>> The function that auto-assigns PCI addresses was written with the
>> hardcoded assumptions that any PCI bus would have slots available
>> starting at 1 and ending at 31. This isn't true for many types of
>> controller (some have a single slot/port at 0, some have slots/ports
>> from 0 to 31). This patch updates that function to remove the
>> hardcoded assumptions. It will properly find/assign addresses for
>> devices that can only connect to pcie-(root|downstream)-port (which
>> have minSlot/maxSlot of 0/0) or a pcie-switch-upstream-port (0/31).
>>
>> It still will not auto-create a new bus of the proper kind for these
>> connections when one doesn't exist, that task is for another day.
>> ---
>> new in V2
>>
>> src/conf/domain_addr.c | 65 +++++++++++++++++++++++++++++---------------------
>>  1 file changed, 38 insertions(+), 27 deletions(-)
>>
>> diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
>> index 2be98c5..bc09279 100644
>> --- a/src/conf/domain_addr.c
>> +++ b/src/conf/domain_addr.c
>> @@ -471,24 +471,30 @@ virDomainPCIAddressGetNextSlot(virDomainPCIAddressSetPtr addrs,
>>                                 virDomainPCIConnectFlags flags)
>>  {
>>      /* default to starting the search for a free slot from
>> -     * 0000:00:00.0
>> +     * the first slot of domain 0 bus 0...
>>       */
>>      virDevicePCIAddress a = { 0, 0, 0, 0, false };
>>      char *addrStr = NULL;
>>  
>> -    /* except if this search is for the exact same type of device as
>> -     * last time, continue the search from the previous match
>> -     */
>> -    if (flags == addrs->lastFlags)
>> -        a = addrs->lastaddr;
>> -
>>      if (addrs->nbuses == 0) {
>>          virReportError(VIR_ERR_XML_ERROR, "%s", _("No PCI buses available"));
>>          goto error;
>>      }
>>  
>> -    /* Start the search at the last used bus and slot */
>> -    for (a.slot++; a.bus < addrs->nbuses; a.bus++) {
>> +    /* ...unless this search is for the exact same type of device as
>> +     * last time, then continue the search from the next slot after
>> +     * the previous match.
> next slot and possibly first slot of next bus
>
>> +     */
>> +    if (flags == addrs->lastFlags) {
>> +        a = addrs->lastaddr;
>> +        if (++a.slot > addrs->buses[a.bus].maxSlot &&
>> +            ++a.bus < addrs->nbuses)
>> +            a.slot = addrs->buses[a.bus].minSlot;
>> +    } else {
>> +        a.slot = addrs->buses[0].minSlot;
>> +    }
>> +
>> +    while (a.bus < addrs->nbuses) {
>>          if (!(addrStr = virDomainPCIAddressAsString(&a)))
>>              goto error;
>>          if (!virDomainPCIAddressFlagsCompatible(&a, addrStr,
>> @@ -497,29 +503,33 @@ virDomainPCIAddressGetNextSlot(virDomainPCIAddressSetPtr addrs,
>>              VIR_FREE(addrStr);
> I think with the new logic to use "if / else" rather than "if ...
> continue;", this VIR_FREE is unnecessary since it's done at then end of
> the while loop

Correct. Thanks!

>
>>              VIR_DEBUG("PCI bus %.4x:%.2x is not compatible with the device",
>>                        a.domain, a.bus);
>> -            continue;
>> -        }
>> -        for (; a.slot <= VIR_PCI_ADDRESS_SLOT_LAST; a.slot++) {
>> -            if (!virDomainPCIAddressSlotInUse(addrs, &a))
>> -                goto success;
>> +        } else {
>> +            while (a.slot <= addrs->buses[a.bus].maxSlot) {
>> +                if (!virDomainPCIAddressSlotInUse(addrs, &a))
>> +                    goto success;
>>  
>> -            VIR_DEBUG("PCI slot %.4x:%.2x:%.2x already in use",
>> -                      a.domain, a.bus, a.slot);
>> +                VIR_DEBUG("PCI slot %.4x:%.2x:%.2x already in use",
>> +                          a.domain, a.bus, a.slot);
>> +                a.slot++;
>> +            }
>>          }
>> -        a.slot = 1;
>> +        if (++a.bus < addrs->nbuses)
>> +            a.slot = addrs->buses[a.bus].minSlot;
>>          VIR_FREE(addrStr);
>>      }
>>  
>>      /* There were no free slots after the last used one */
> So essentially we're going to search everything before to see if there's
> any openings to use.
>
>>      if (addrs->dryRun) {
>> -        /* a is already set to the first new bus and slot 1 */
>> +        /* a is already set to the first new bus */
>>          if (virDomainPCIAddressSetGrow(addrs, &a, flags) < 0)
>>              goto error;
>> +        /* this device will use the first slot of the new bus */
>> +        a.slot = addrs->buses[a.bus].minSlot;
>>          goto success;
>>      } else if (flags == addrs->lastFlags) {
>>          /* Check the buses from 0 up to the last used one */
>>          for (a.bus = 0; a.bus <= addrs->lastaddr.bus; a.bus++) {
>> -            addrStr = NULL;
>> +            a.slot = addrs->buses[a.bus].minSlot;
>>              if (!(addrStr = virDomainPCIAddressAsString(&a)))
>>                  goto error;
>>              if (!virDomainPCIAddressFlagsCompatible(&a, addrStr,
>> @@ -527,14 +537,15 @@ virDomainPCIAddressGetNextSlot(virDomainPCIAddressSetPtr addrs,
>>                                                      flags, false, false)) {
>>                  VIR_DEBUG("PCI bus %.4x:%.2x is not compatible with the device",
>>                            a.domain, a.bus);
>> -                continue;
>> -            }
>> -            for (a.slot = 1; a.slot <= VIR_PCI_ADDRESS_SLOT_LAST; a.slot++) {
>> -                if (!virDomainPCIAddressSlotInUse(addrs, &a))
>> -                    goto success;
>> -
>> -                VIR_DEBUG("PCI slot %.4x:%.2x:%.2x already in use",
>> -                          a.domain, a.bus, a.slot);
>> +            } else {
>> +                while (a.slot <= addrs->buses[a.bus].maxSlot) {
>> +                    if (!virDomainPCIAddressSlotInUse(addrs, &a))
>> +                        goto success;
>> +
>> +                    VIR_DEBUG("PCI slot %.4x:%.2x:%.2x already in use",
>> +                              a.domain, a.bus, a.slot);
>> +                    a.slot++;
>> +                }
>>              }
> Perhaps preexisting, but one would think a VIR_FREE(addrStr) would be
> needed here just as it was in the first pass...  [Coverity didn't find
> this either]

Yes! Another good catch! I'm surprised that this didn't lead to any
valgrind or coverity reports before now - that loop has been missing a
VIR_FREE(addrStr) for a very long time.

>
> ACK with the adjustment
>
> John
>>          }
>>      }
>>