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

[Laine Stump]

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.