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

John Ferlan jferlan at redhat.com
Wed Jul 22 18:50:38 UTC 2015



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

>              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]

ACK with the adjustment

John
>          }
>      }
> 




More information about the libvir-list mailing list