[libvirt] [PATCH 3/5] qemu: Fix auto-adding PCI bridge when all slots are reserved

[Ján Tomko]

On 01/21/2015 05:50 PM, Erik Skultety wrote:
> Commit 93c8ca tried to fix the issue with auto-adding of a PCI bridge
> controller, it worked well when the slots were reserved by devices with
> user defined addresses without any other devices with unspecified PCI addresses
> present in the XML.
> However, if another device with unspecified PCI addresses appeared in
> the domain's XML, the same problem occurred as the BZ below references.
> 
> This patch fixes the issue by not creating any additional bridges when not
> necessary.
> Now let's say all the buses corresponding to the number of PCI controllers
> present in the domain's XML got fully reserved by devices with user defined
> addresses. If there are no other devices present in the XML, no need to
> create both an additional PCI bus and a bridge controller.
> If however there are still some PCI devices waiting to get a slot assigned
> by qemuAssignDevicePCISlots, this means an additional bus needs to
> be created along with a corresponding bridge controller.
> This scenario now results in an reasonable error instead of generating wrong qemu
> command line.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1132900
> ---
>  src/qemu/qemu_command.c | 42 ++++++++++++++++++++++++++++++++++--------
>  1 file changed, 34 insertions(+), 8 deletions(-)
> 

This breaks the pci-many test case you added before.

> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index cdf02a6..99b06ee 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -1472,6 +1472,9 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
>          int nbuses = 0;
>          size_t i;
>          int rv;
> +        int resflags = 0;
> +        bool buses_reserved = false;
> +

If you set this to true by default...

>          virDomainPCIConnectFlags flags = VIR_PCI_CONNECT_TYPE_PCI;
>  
>          for (i = 0; i < def->ncontrollers; i++) {
> @@ -1490,17 +1493,22 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
>              /* 1st pass to figure out how many PCI bridges we need */
>              if (!(addrs = qemuDomainPCIAddressSetCreate(def, nbuses, true)))
>                  goto cleanup;
> +
> +            for (i = 0; i < nbuses; i++) {

> +                if (qemuDomainPCIBusFullyReserved(&addrs->buses[i]))
> +                    resflags |= 1 << i;

you can do:
if (!fullyReserved(buses[i]))
  buses_reserved = false;

Or just rename the bool to something like 'addExtraEmptySlot' and invert the
condition below. (To stay more positive :))

> +            }
> +            buses_reserved = (resflags && ~((~0) << nbuses));
> +
>              if (qemuAssignDevicePCISlots(def, qemuCaps, addrs) < 0)
>                  goto cleanup;
>  
> -            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;
> -                }
> -            }
> +            /* Reserve 1 extra slot for a (potential) bridge only if buses
> +             * are not fully reserved yet
> +             */
> +            if (!buses_reserved &&
> +                virDomainPCIAddressReserveNextSlot(addrs, &info, flags) < 0)
> +                goto cleanup;
>  
>              for (i = 1; i < addrs->nbuses; i++) {
>                  virDomainPCIAddressBusPtr bus = &addrs->buses[i];
> @@ -1532,6 +1540,24 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
>              if (qemuAssignDevicePCISlots(def, qemuCaps, addrs) < 0)
>                  goto cleanup;
>          }
> +
> +        for (i = 0; i < def->ncontrollers; i++) {
> +            /* check if every PCI bridge controller's ID is greater than
> +             * the bus it is placed onto
> +             */
> +            virDomainControllerDefPtr cont = def->controllers[i];
> +            int idx = cont->idx;
> +            int bus = cont->info.addr.pci.bus;
> +            if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI &&
> +                cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE &&
> +                idx <= bus) {
> +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                               _("failed to create PCI bridge "
> +                                 "on bus %d: bus is fully reserved"),
> +                               bus);
> +                goto cleanup;
> +            }
> +        }

This should IMHO be a separate commit, as it does not fix the case where the
devices exactly fill the buses, but when there are too many.

Also, maybe the error message could say something about 'too many devices with
fixed addressess'?

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/20150122/831e5b9b/attachment-0001.sig>