Re: [libvirt] [PATCH 15/26] qemu: Automatically pick index and model for pci-root controllers

> @@ -1856,6 +1856,7 @@ qemuDomainSupportsPCI(virDomainDefPtr def, >   >  static void >  qemuDomainPCIControllerSetDefaultModelName(virDomainControllerDefPtr cont, > +                                           virDomainDefPtr def, >                                             virQEMUCapsPtr qemuCaps) >  { >      int *modelName = &cont->opts.pciopts.modelName; > @@ -1892,6 +1893,9 @@ qemuDomainPCIControllerSetDefaultModelName(virDomainControllerDefPtr cont, >          *modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PXB_PCIE; >          break; >      case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: > +        if (qemuDomainIsPSeries(def)) > +            *modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE; > +        break;   Just to verify - *all* the pci-roots on pSeries will be spapr-pci-host-bridge, even the first one?

>      case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: >      case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: >          break; > @@ -1900,6 +1904,30 @@ qemuDomainPCIControllerSetDefaultModelName(virDomainControllerDefPtr cont, >   >   >  static int > +qemuDomainAddressFindNewIndex(virDomainDefPtr def)   To help avoid confusion between the target index and the index at the toplevel, can we call this qemuDomainAddressFindNewTargetIndex()? (yeah, I know that's really long)

> +{ > +    int ret = 1; > +    size_t i; > + > +    for (i = 0; i < def->ncontrollers; i++) { > +        virDomainControllerDefPtr cont = def->controllers[i]; > + > +        if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) { > +            if (cont->opts.pciopts.idx >= ret) > +                ret = cont->opts.pciopts.idx + 1; > +        }   I know it would be idiotic to do so, but what if someone removed one of the pci-root controllers, and then later added a new one? You'd end up with an unused index where the earlier one was removed (and it would never be filled in). Maybe you should instead start at 0, and scan all the existing controllers for 0, then for 1, then for 2, etc, until you find the lowest target index value that isn't used.

> @@ -2196,9 +2224,34 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, >                      goto cleanup; >                  } >                  break; > +            case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: > +                if (!qemuDomainIsPSeries(def)) > +                    break; > +                if (options->idx == -1) { > +                    if (cont->idx == 0) { > +                        /* The pcie-root controller with controller index 0   *really* unimportant, but I think the above should say "pci-root" rather than "pcie-root" :-)

> +                         * must always be assigned target index 0, because > +                         * it represents the implicit PHB which is treated > +                         * differently than all other PHBs */ > +                        options->idx = 0; > +                    } else { > +                        /* For all other PHBs the target index doesn't need > +                         * to match the controller index or have any > +                         * particular value, really */   How is it used then? Just directly put on qemu commandline? And it's allowed to have gaps in the index numbers of the PHBs?

> +                        options->idx = qemuDomainAddressFindNewIndex(def); > +                    } > +                } > +                if (options->idx == -1) { > +                    virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > +                                   _("No valid index is available to " > +                                     "auto-assign to bus %d. Must be " > +                                     "manually assigned"),   I guess if you checked for unused indexes inside the limits of currently used indexes (as I suggested above) then the part about "Must be manually assigned" wouldn't be true - if we couldn't find an unused index, then that's the end of it; there's no possible value to manually assign either.