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

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.

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

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.

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.