<html> <head> <meta content="text/html; charset=utf-8" http-equiv="Content-Type"> </head> <body bgcolor="#FFFFFF" text="#000000"> <div class="moz-cite-prefix">On 10/13/2016 01:43 PM, Laine Stump wrote: </div> <blockquote cite="mid:d9c5386d-1d01-a4dc-944a-b7fee729d679@laine.org" type="cite">On 10/11/2016 05:34 AM, Andrea Bolognani wrote: <blockquote type="cite">On Mon, 2016-10-10 at 15:43 -0400, Laine Stump wrote: <blockquote type="cite"> <blockquote type="cite"> <blockquote type="cite">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. </blockquote> 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 :( </blockquote> 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! </blockquote> Oh, *that* guy! It's always *that* guy, isn't it? :P <blockquote type="cite">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? </blockquote> 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. </blockquote> 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. </blockquote> Okay, now I've reminded myself of what I did. Basically everything necessary for that is the changes to qemuDomainPCIAddressSetCreate() dealing with "lowest*Bridge" (see the patch excerpt below). I would still need to patch this function even if we didn't support "auto-adding dmi-to-pci-bridge when a pci-bridge is present in the config", but it would be slightly simpler. I'll split it into a separate patch so that it's more apparent, and we can decide based on that. <div class="moz-cite-prefix">On 09/20/2016 03:14 PM, Laine Stump wrote: </div> <blockquote cite="mid:1474398864-9039-16-git-send-email-laine@laine.org" type="cite"> <pre wrap="">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; + }</pre> </blockquote> </body> </html>