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.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
