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

[Andrea Bolognani]

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