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.
> > > A DMI-to-PCI bridge with index='1' has been
added
> > > automatically here...
> > >
> > > ... but it's not being used by any of the devices. So why
> > > would it be added in the first place?
> >
> > That is a *very* good question!
> Can't wait to know the answer! ;)
Oh, now that I've looked in context of the patch ordering, I undestand:
it's because patch 16/18 hasn't been applied yet. I'd forgotten the
ordering...
Right you are :)
--
Andrea Bolognani / Red Hat / Virtualization