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

[Ján Tomko]

On 02/21/13 17:12, Laine Stump wrote:
> 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).

The idea was to change it when creating the PCI address set based on the
number of bridges in the domain definition (or machine type, if it
implies more buses).

liguang's patch only adds bridges for explicitly specified bus numbers,
we could still run out of buses if there are enough devices without a
specified address or if we use hotplug. Both of these sound convenient,
but I'm not sure if there is a pretty solution.

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

Slot and device values are checked by the XML parsing code right now,
and looking at all the callers of qemuPCIAddressAsString, only a few of
them can have wrong domain or bus value.

Maybe we should print the values in hexadecimal instead of decimal, to
match the XML, as the strings are also used in error messages.


>> @@ -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?

And now I realize it's not even able to use the previous buses.

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

That sounds much better than the hash table.

Jan