Re: [libvirt] [PATCH v3 15/18] qemu: auto-add pcie-root-port/dmi-to-pci-bridge controllers as needed

On 10/11/2016 05:34 AM, Andrea Bolognani wrote: > On Mon, 2016-10-10 at 15:43 -0400, Laine Stump wrote: >>>> 3) Although it is strongly discouraged, it is legal for a pci-bridge >>>> to be directly plugged into pcie-root, and we don't want to >>>> auto-add a dmi-to-pci-bridge if there is already a pci-bridge >>>> that's been forced directly into pcie-root. Finally, >>>> although I >>>> fail to see the utility of it, it is legal to have >>>> something like >>>> this in the xml: >>>> <controller type='pci' model='pcie-root' index='0'/> >>>> <controller type='pci' model='pci-bridge' index='2'/> >>>> and that will lead to an automatically added >>>> dmi-to-pci-bridge at >>>> index=1 (to give the unaddressed pci-bridge a proper place >>>> to plug >>>> in): >>>> <controller type='pci' model='dmi-to-pci-bridge' >>>> index='1'/> >>>> (for example, see the existing test case >>>> "usb-controller-default-q35"). This is handled in >>>> qemuDomainPCIAddressSetCreate() when it's adding in >>>> controllers to >>>> fill holes in the indexes. >>> I wonder how this "feature" came to be... It seems to be the >>> reason for quite a bit of convoluted code down below, which >>> we could avoid if this had never worked at all. As is so >>> often the case, too late for that :( >> Maybe not. The only place I ever saw that was in the above test case, >> and another named "usb-controller-explicit-q35", and the author of both >> of those tests was (wait for it!), no, not Albert Einstein. Andrea >> Bolognani! > Oh, *that* guy! It's always *that* guy, isn't it? :P > >> The only reason it worked in the past was because we always >> automatically added the dmi-to-pci-bridge very early in the post-parse >> processing. This implies that any saved config anywhere will already >> have the necessary dmi-to-pci-bridge at index='1', so we only need to >> preserve the behavior for new domain definitions that have a pci-bridge >> at index='2' but nothing at index='1'. >> Since you're the only person documented to have ever created a config >> like that, and it would only be problematic if someone tried to create >> another new config, maybe we should just stop accidentally >> supporting it >> and count it as a bug that's being fixed. What's your opinion? > Given the evidence you're presenting, I'm all for getting > rid of it, especially since it will make the code below > much simpler and hence more maintainable. > > Considering how critical that part of libvirt is, anything > we can do to make it leaner and meaner is going to be a huge > win in the long run. I'm looking back through the code and wondering how to simplify it - it may be that the alternate method I had initially used (which failed that one test) is just as complicated as what I have now; unfortunately I didn't save it.

diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index 1ff80e3..a9c4c32 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -797,6 +797,11 @@ qemuDomainPCIAddressSetCreate(virDomainDefPtr def, { virDomainPCIAddressSetPtr addrs; size_t i; + bool hasPCIeRoot = false; + int lowestDMIToPCIBridge = nbuses; + int lowestUnaddressedPCIBridge = nbuses; + int lowestAddressedPCIBridge = nbuses; + virDomainControllerModelPCI defaultModel; if ((addrs = virDomainPCIAddressSetAlloc(nbuses)) == NULL) return NULL; @@ -804,38 +809,84 @@ qemuDomainPCIAddressSetCreate(virDomainDefPtr def, addrs->nbuses = nbuses; addrs->dryRun = dryRun; - /* As a safety measure, set default model='pci-root' for first pci - * controller and 'pci-bridge' for all subsequent. After setting - * those defaults, then scan the config and set the actual model - * for all addrs[idx]->bus that already have a corresponding - * controller in the config. - * - */ - if (nbuses > 0) - virDomainPCIAddressBusSetModel(&addrs->buses[0], - VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT); - for (i = 1; i < nbuses; i++) { - virDomainPCIAddressBusSetModel(&addrs->buses[i], - VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE); - } - for (i = 0; i < def->ncontrollers; i++) { - size_t idx = def->controllers[i]->idx; + virDomainControllerDefPtr cont = def->controllers[i]; + size_t idx = cont->idx; - if (def->controllers[i]->type != VIR_DOMAIN_CONTROLLER_TYPE_PCI) + if (cont->type != VIR_DOMAIN_CONTROLLER_TYPE_PCI) continue; if (idx >= addrs->nbuses) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Inappropriate new pci controller index %zu " - "not found in addrs"), idx); + "exceeds addrs array length"), idx); goto error; } - if (virDomainPCIAddressBusSetModel(&addrs->buses[idx], - def->controllers[i]->model) < 0) + if (virDomainPCIAddressBusSetModel(&addrs->buses[idx], cont->model) < 0) goto error; + + /* we'll use all this info later to determine if we need + * to add a dmi-to-pci-bridge due to unaddressed pci-bridge controllers + */ + if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) { + hasPCIeRoot = true; + } else if (cont->model == + VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE) { + if (lowestDMIToPCIBridge > idx) + lowestDMIToPCIBridge = idx; + } else if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE) { + if (virDeviceInfoPCIAddressWanted(&cont->info)) { + if (lowestUnaddressedPCIBridge > idx) + lowestUnaddressedPCIBridge = idx; + } else { + if (lowestAddressedPCIBridge > idx) + lowestAddressedPCIBridge = idx; + } } + } + + if (nbuses > 0 && !addrs->buses[0].model) { + if (virDomainPCIAddressBusSetModel(&addrs->buses[0], + VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) < 0) + goto error; + } + + /* Now fill in a reasonable model for all the buses in the set + * that don't yet have a corresponding controller in the domain + * config. In the rare (and actually fairly idiotic, but still + * allowed for some reason) case that a domain has 1) a pcie-root + * at index 0, 2)*no* dmi-to-pci-bridge (or pci-bridge that was + * manually addressed to sit directly on pcie-root), and 3) does + * have an unaddressed pci-bridge at an index > 1, then we need to + * add a dmi-to-pci-bridge. + */ + + if (!hasPCIeRoot) + defaultModel = VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE; + + else if (lowestUnaddressedPCIBridge < MIN(lowestAddressedPCIBridge, + lowestDMIToPCIBridge)) + defaultModel = VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE; + else + defaultModel = VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT; + + for (i = 1; i < addrs->nbuses; i++) { + + if (addrs->buses[i].model) + continue; + + if (virDomainPCIAddressBusSetModel(&addrs->buses[i], defaultModel) < 0) + goto error; + + VIR_DEBUG("Auto-adding <controller type='pci' model='%s' index='%zu'/>", + virDomainControllerModelPCITypeToString(defaultModel), i); + /* only add a single dmi-to-pci-bridge, then add pcie-root-port + * for any other unspecified controller indexes. + */ + if (hasPCIeRoot) + defaultModel = VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT; + }