[libvirt] [PATCH 03/11] conf: Remove dubious code from virDomainPCIAddressSetGrow()

[Andrea Bolognani]

On Tue, 2018-04-03 at 22:44 -0400, Laine Stump wrote:
> On 04/03/2018 07:12 PM, John Ferlan wrote:
> > Since Laine added this code - perhaps calling his name out on the CC
> > list will allow him to appear and answer the question.

Fair point. I kinda just assumed Laine would be the only one
crazy^Wbrave enough to attempt a review for this series :P

> > > -    } else if (flags & VIR_PCI_CONNECT_TYPE_PCI_BRIDGE &&
> > > -               addrs->buses[0].model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) {
> > > -        /* NB: if the root bus is pci-root, and we couldn't find an
> > > -         * open place to connect a pci-bridge, then there is nothing
> > > -         * we can do (since the only way to gain a new slot that
> > > -         * accepts a pci-bridge is to add *a pci-bridge* (which is the
> > > -         * reason we're here in the first place!)
> > > -         */
> > > -        model = VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE;
> 
> This is saying that if the device you need a slot for is a pci-bridge
> (or has exactly the same connection requirements as a pci-bridge) and if
> the root bus is pcie-root then you need to add a dmi-to-pci-bridge (you
> wouldn't be in this function in the first place if you already *had* a
> dmi-to-pci-bridge, so there's no need to check if you have one). This
> would happen if someone manually added a pci-bridge device to a pure
> pcie topology.
> 
> Normally I would say that this should stay, as I believe we *should* try
> to support partially-specified topologies as much as possible (since
> you've dealt with libvirt-qe bugzilla reports, you too know of some of
> the odd scenarios they come up with - e.g. removing one controller from
> a working config.).

I'm not sure I agree here. There are just way too many corner cases
for us to hope we can deal with all of them reasonably, let alone
without introducing heaps of fragile and difficult to understand
code, and supporting some of them can give users the impression that
*all* of them are going to work, no matter how insane.

I'd rather back the reasonable, supportable use cases with rock-solid
code and error out when the user is asking too much of libvirt.

> However, currently when an un-addressed pci-bridge is encountered (which
> is the only time we should get to this code), the search for a slot that
> accepts a pci-bridge will find an empty slot on the *that same
> pci-bridge* and we end up logging an error indicating that (I forget the
> exact error) - I could *swear* that at some point in the past it wasn't
> dead code, and that it actually helped to resolve a bug filed by
> libvirt-qe, but experimentation shows that certainly isn't the case now.

This matches my understanding, and my experience after playing with
it for a fairly long time.

> In the meantime, if I remove that code (and don't apply any of your
> patches) then a pure pcie domain *can* be successfully edited to add a
> single pci-bridge (as long as you specify an index, *and* there is an
> unused index of a smaller value), but what ends up in the
> "proper/validated" config is a pci-bridge that is plugged directly into
> a pcie-root-port (which is also wrong, but silently so).
> 
> So I guess I reserve my judgement until I see what happens with your
> entire series applied, which I'll do tomorrow morning.

Looking forward to that :)

-- 
Andrea Bolognani / Red Hat / Virtualization