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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
