On 10/14/2016 01:20 PM, Andrea Bolognani wrote:
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.
Truthfully the only reason it's there at all is because there was similar code originally (which also was surely never needed). Even so, I'm nervous about totally removing the check for unset model even though a visual inspection of the current code tells us it won't be needed. So instead, I'm going to turn it into an internal error condition.