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

On Thu, 2016-10-13 at 13:43 -0400, Laine Stump wrote: >>>>> + if (nbuses > 0 && !addrs->buses[0].model) { >>>>> + if (virDomainPCIAddressBusSetModel(&addrs->buses[0], >>>>> + VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) < 0) >>>>> + goto error; >>>>> + } >>>> >>>> Shouldn't we use either PCI_ROOT or PCIE_ROOT based on the >>>> machine type here? And set hasPCIeRoot *after* doing so? >>>> >>>> Sorry for the questions, I guess this is the point in the >>>> patch where I got a bit lost :( >> >> I'm afraid you missed this question :) > > Sorry about the omission. I've tried to limit the code that decides > whether or not there should be a pci-root or a pcie-root to the one > place when default controllers are being added (I forget the name of the > function right now), I guess you mean qemuDomainDefAddDefaultDevices()? That's the function where pci{,e}-root is added, if not already present in the configuration. > and after that only decide based on whether a > pci-root or pcie-root really is there in the config. My subconscious > reasoning for this is that the extra calisthenics around supporting > aarch64/virt with PCI(e) vs with mmio has made me wonder if there might > be machinetypes in the future that could support one type of root bus or > another (or none) according to what is in the config, rather than having > a root bus just builtin to the machine. I don't know if that would ever > happen, but if it did, this code would work without change - only the > function adding the default controllers would need to be changed. > > (Note that I used the same logic when deciding how to right > qemuDomainMachineHasPCI[e]Root())(still not sure about that name, but it > can always be changed later to remove the "Machine" if someone doesn't > like it) That makes sense. My point is that the code above, if I'm reading it correctly, sets the model of bus 0 to PCI_ROOT if it's not already set. But 1) qemuDomainDefAddDefaultDevices() mentioned above should already have added pci{,e}-root to @def 2) if that's not the case, we should use either PCI_ROOT or PCIE_ROOT based on what's appropriate for the machine type Looking at the code in qemuDomainDefAddDefaultDevices() it seems like we would never find ourselves in the situation where pci{,e}-root is needed but not present in @def by the time qemuDomainPCIAddressSetCreate() is called, so I think that chunk of code should just be removed.