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

[Andrea Bolognani]

On Tue, 2017-06-13 at 21:52 -0400, Laine Stump wrote:
> > @@ -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?

Yup.

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

Sure.

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

Zero is going to be used for the default PHB, so I'd have to
start from one instead. But I like the idea :)

> > @@ -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" :-)

It's not unimportant! Comments that disagree with the code
can cause a lot of confusion. Good catch :)

> > +                         * 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?

Yes to both. I can't think of a single sensible reason why
you'd want to have gaps, but it's a valid configuration.

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

Indeed.

I just realized I'm not making sure user-assigned target
indexes are contained in the allowed range either, so I'll
add that as well.

-- 
Andrea Bolognani / Red Hat / Virtualization