[libvirt] [PATCH 1/3] qemu: Tweak auto adding PCI bridge controller when extending default PCI bus

[Ján Tomko]

On 01/15/2015 02:14 PM, Erik Skultety wrote:
> In case we find out, there are more PCI devices to be connected
> than there are available slots on the default PCI bus, we automatically add a
> new bus and a related PCI bridge controller as well.

This also happened when:
* there was exactly the same amount of PCI devices as available slots
* all the PCI adresses were specified in the XML
[let's call it fully-exhausted XML] which is what you're fixing here.

> As there are no free slots
> left on the default PCI bus, PCI bridge controller gets a free slot on a
> newly created PCI bus which causes qemu to refuse to start the guest.
> This fix introduces a new function qemuDomainPCIBusFullyReserved which
> is checked right before we possibly try to reserve a slot for PCI bridge
> controller.
> 

While this fix is great for the correct use of fully exhausted XML, if you add
one more device to that XML, we generate incorrect QEMU command line:
-device pci-bridge,chassis_nr=1,id=pci.1,bus=pci.1,addr=0x1
instead of erroring out.

> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1132900
> ---
>  src/qemu/qemu_command.c | 24 +++++++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 3346e95..06def5f 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -1445,6 +1445,18 @@ qemuDomainSupportsPCI(virDomainDefPtr def)
>      return false;
>  }
>  
> +static bool
> +qemuDomainPCIBusFullyReserved(virDomainPCIAddressBusPtr bus)
> +{
> +    size_t i;
> +
> +    for (i = bus->minSlot; i <= bus->maxSlot; i++)
> +        if (!bus->slots[i])
> +            return false;
> +
> +    return true;
> +}
> +
>  int
>  qemuDomainAssignPCIAddresses(virDomainDefPtr def,
>                               virQEMUCapsPtr qemuCaps,
> @@ -1479,9 +1491,15 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
>                  goto cleanup;
>              if (qemuAssignDevicePCISlots(def, qemuCaps, addrs) < 0)
>                  goto cleanup;
> -            /* Reserve 1 extra slot for a (potential) bridge */
> -            if (virDomainPCIAddressReserveNextSlot(addrs, &info, flags) < 0)
> -                goto cleanup;

So originally we reserved one empty slot, to make sure we don't generate fully
exhausted XMLs if the bus is full.

> +
> +            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;
> +                }
> +            }
>  

But in this loop you add one for every non-empty bus, which does not seem that
useful - if there is enough space on the buses, then there's no need to
reserve the empty slot at all, because it won't result in generating a new bus
and adding a new bridge. It's only when the buses are full when we want to add
a new one.

If we want to keep the functionality of not-generating fully exhausted XMLs,
the check for full buses should be done after collecting the addresses
specified by the user, but before we assign the non-specified addresses.
That is - separate the static machine-type specific addresses from
AssignDevicePCISlots and check the buses just before assigning the dynamic
addresses.

If we don't, we should delete this loop, since it doesn't really do anything.

As for the incorrect command line generation, it would be nice to check that
the bus we put the bridge on is less than its index and report an XML error
instead of trying to run QEMU.

Jan

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150116/f16ae4da/attachment-0001.sig>