[libvirt] [PATCH 2/2] qemu: allow multiple buses in PCI address alocation

[Laine Stump]

On 02/15/2013 03:22 AM, Ján Tomko wrote:
> Allow allocating addresses with non-zero bus numbers.

while not actually allowing it :-) (since maxbus is never set to
anything > 0, as you say in Patch 0/2). How/when do you envision that
being changed? liguang's patches allow devices with any bus number you
want, and just add pci-bridge devices as necessary, so I don't know if
any explicit setting/checking of a max is necessary (although it might
be something checked for in a virCaps callback function, as certain
machine types might support fewer buses than others or something).

BTW, I think that this entire model of the guest PCI address space
allocation could be useful if moved to its own library in util - it
seems like something that other hypervisors could benefit from.

> ---
>  src/qemu/qemu_command.c | 41 ++++++++++++++++++++++++++++++-----------
>  1 file changed, 30 insertions(+), 11 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index b50e779..f747cfb 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -954,17 +954,25 @@ cleanup:
>  struct _qemuDomainPCIAddressSet {
>      virHashTablePtr used;
>      virDevicePCIAddress lastaddr;
> +    unsigned int maxbus;
>  };
>  
>  
> -static char *qemuPCIAddressAsString(virDevicePCIAddressPtr addr)
> +static char *qemuPCIAddressAsString(qemuDomainPCIAddressSetPtr addrs,
> +                                    virDevicePCIAddressPtr addr)
>  {
>      char *str;
>  
> -    if (addr->domain != 0 ||
> -        addr->bus != 0) {
> +    if (addr->domain != 0) {
>          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                       _("Only PCI domain 0 and bus 0 are available"));
> +                       _("Only PCI domain 0 is available"));
> +        return NULL;
> +    }
> +
> +    if (addr->bus > addrs->maxbus) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Only PCI buses up to %u are available"),
> +                       addrs->maxbus);
>          return NULL;
>      }

I realize that this check was pre-existing, but to me it seems out of
place to have such a check in a function that is merely converting the
PCI address from its components into a string; a marriage of convenience
perhaps?

I think that instead of putting the extra burden of checking limits on
this formatting function, there should instead be a separate function
that checks to make sure all elements are within bounds (and why limit
it to domain and bus? May as well check slot and function as well), and
that function should probably be caps-related (i.e. a callback function
provided in virCaps)

>  
> @@ -999,7 +1007,7 @@ static int qemuCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED,
>          return 0;
>      }
>  
> -    addr = qemuPCIAddressAsString(&info->addr.pci);
> +    addr = qemuPCIAddressAsString(addrs, &info->addr.pci);
>      if (!addr)
>          goto cleanup;
>  
> @@ -1028,7 +1036,7 @@ static int qemuCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED,
>          unsigned int *func = &tmp_addr.function;
>  
>          for (*func = 1; *func < QEMU_PCI_ADDRESS_LAST_FUNCTION; (*func)++) {
> -            addr = qemuPCIAddressAsString(&tmp_addr);
> +            addr = qemuPCIAddressAsString(addrs, &tmp_addr);
>              if (!addr)
>                  goto cleanup;
>  
> @@ -1148,7 +1156,7 @@ static int qemuDomainPCIAddressCheckSlot(qemuDomainPCIAddressSetPtr addrs,
>      unsigned int *func = &(tmp_addr.function);
>  
>      for (*func = 0; *func < QEMU_PCI_ADDRESS_LAST_FUNCTION; (*func)++) {
> -        str = qemuPCIAddressAsString(&tmp_addr);
> +        str = qemuPCIAddressAsString(addrs, &tmp_addr);
>          if (!str)
>              return -1;
>  
> @@ -1168,7 +1176,7 @@ int qemuDomainPCIAddressReserveAddr(qemuDomainPCIAddressSetPtr addrs,
>  {
>      char *str;
>  
> -    str = qemuPCIAddressAsString(addr);
> +    str = qemuPCIAddressAsString(addrs, addr);
>      if (!str)
>          return -1;
>  
> @@ -1243,7 +1251,7 @@ int qemuDomainPCIAddressReleaseAddr(qemuDomainPCIAddressSetPtr addrs,
>      char *str;
>      int ret;
>  
> -    str = qemuPCIAddressAsString(addr);
> +    str = qemuPCIAddressAsString(addrs, addr);
>      if (!str)
>          return -1;
>  
> @@ -1263,7 +1271,7 @@ int qemuDomainPCIAddressReleaseSlot(qemuDomainPCIAddressSetPtr addrs,
>      unsigned int *func = &tmp_addr.function;
>  
>      for (*func = 0; *func < QEMU_PCI_ADDRESS_LAST_FUNCTION; (*func)++) {
> -        str = qemuPCIAddressAsString(&tmp_addr);
> +        str = qemuPCIAddressAsString(addrs, &tmp_addr);
>          if (!str)
>              return -1;
>  
> @@ -1298,19 +1306,30 @@ qemuDomainPCIAddressGetNextSlot(qemuDomainPCIAddressSetPtr addrs,
>      virDevicePCIAddress tmp_addr = addrs->lastaddr;
>      int i;
>      char *addr;
> +    bool next_bus = false;
>  
>      tmp_addr.slot++;
>      for (i = 0; i <= QEMU_PCI_ADDRESS_LAST_SLOT; i++, tmp_addr.slot++) {
>          if (QEMU_PCI_ADDRESS_LAST_SLOT < tmp_addr.slot) {
> +            /* Check all the slots in this bus first */
>              tmp_addr.slot = 0;
> +            next_bus = !next_bus;
>          }
>  
> -        if (!(addr = qemuPCIAddressAsString(&tmp_addr)))
> +        if (!(addr = qemuPCIAddressAsString(addrs, &tmp_addr)))
>              return -1;
>  
>          if (qemuDomainPCIAddressCheckSlot(addrs, &tmp_addr) < 0) {
>              VIR_DEBUG("PCI addr %s already in use", addr);
>              VIR_FREE(addr);
> +            if (i == QEMU_PCI_ADDRESS_LAST_SLOT && next_bus &&
> +                tmp_addr.bus < addrs->maxbus) {
> +                /* Move on to the next bus if this one's full */
> +                i = 0;
> +                tmp_addr.bus++;
> +                tmp_addr.slot = 0;
> +                next_bus = !next_bus;
> +            }
>              continue;
>          }


This function is more confused than necessary. Rather than having the
"next_bus" boolean, wouldn't it be simpler to follow if you just had a
nested loop - inner for slot and outer for bus?

Beyond that (and I guess this is a discussion that's beyond the bounds
of this patch, since the data structure is pre-existing), is a hashtable
really the best choice for representing this? What we really have is a
collection of buses, each with 32 slots x 8 functions; Why not have a
list of 32 byte arrays - each byte is a slot, and each bit within a byte
is a function - just set the bit for any slot/function that is in use. A
new 32 byte array could be added to the list as new buses are put in
use. If there was any extra information stored for each entry then a
hashtable might make sense, but there isn't - the only info stored is
the string used to compute the hash.