[libvirt] [PATCH v6 10/17] qemu: auto-add pcie-root-port/dmi-to-pci-bridge controllers as needed

[Laine Stump]

On 11/11/2016 11:09 AM, Laine Stump wrote:
> On 11/10/2016 09:10 AM, Andrea Bolognani wrote:
>> On Wed, 2016-11-09 at 15:23 -0500, Laine Stump wrote:
>>>>> +    } else if (flags & VIR_PCI_CONNECT_TYPE_PCI_BRIDGE &&
>>>>> +               addrs->buses[0].model == 
>>>>> VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) {
>>>>> +        model = VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE;
>>>>   Mh, what about the case where we want to add a pci-bridge
>>>> but the machine has a pci-root instead of a pcie-root?
>>>> Shouldn't that case be handled as well?
>>>   It is handled - we'll want to add a pci-bridge if flags &
>>> VIR_PCI_TYPE_PCI_DEVICE is true (i.e. the first clause). In that case,
>>> since pci-root has the flag VIR_PCI_CONNECT_TYPE_PCI_BRIDGE set, the 
>>> for
>>> loop will set neetDMIToPCIBridge = false, so we will end up adding just
>>> the one pci-bridge that we need.
>> Sorry, I was not clear enough.
>>
>> What if the function is called like
>>
>>    virDomainPCIAddressSetGrow(addrs, addr,
>>                               VIR_PCI_CONNECT_TYPE_PCI_BRIDGE)
>>
>> because *the caller* is going through a virDomainDef and,
>> after finding a pci-bridge in it, wants to make sure the
>> address set can actually fit it? The case when
>>
>>    flags & VIR_PCI_CONNECT_TYPE_PCI_BRIDGE
>>
>> can clearly happen, as we're handling it above, but we error
>> out unless the guest has a pcie-root?
>
> Okay, two different scenarios, each for either pci-root or pcie-root:
>
> 1) pci-bridge requested with no address supplied

(or where the address supplied has a bus# exactly 1 greater than the 
current last bus)

>
>    a) pci-root - if there is an open slot it will be assigned to the 
> pci-bridge.

(i.e. ...Grow() would not have been called in the first place)

>        If there isn't an open slot, then there is no chance for 
> success anyway,
>        so the correct error message will be correct:

Sigh. "the *current* error message will be correct.

>
>           a PCI slot is neede to connect a PCI coontller 
> model='pci-bridge'
>           but none is available, and it cannot be automatically added.

Double Sigh. sed -e "s/neede/needed/" -e "s/coontller/controller/"

>
>      (because the only way to add a new open slot is to add a pci-bridge,
>      and that's what we're already trying and failing to do).
>
>    b) pcie-root - if there is an open slot, then we would never get 
> into ...Grow(),
>        if there isn't then we need to add a dmi-to-pci-bridge (because 
> we can
>        only plug into a pci-bridge or a dmi-to-pci-bridge, and we've 
> already established
>        that we can't directly plug in a pci-bridge. So again, behavior 
> is correct
>
> 2) pci-bridge was requested with user-supplied address that is on a 
> nonexistent bus, and that bus number is >1 over the current last bus 
> (i.e. there is a gap in the controller indexes). This would happen if, 
> for example, someone put this in their xml:
>
>         <controller type='pci' index='0' model='pci-root'/>
>         <controller type='pci' index='2' model='pci-bridge'/>
>
> (if libvirt was setting the index, it would have set the index for the 
> pci-bridge to '1').
>
>     a) pci-root - okay, here's where what you're saying would apply - 
> we need to add extra pci-bridges
>         to fill in the "empty space" in the list of controllers.
>
>     b) pcie-root - actually we have the same issue in this case.
>
> Personally I think that it's nonsensical for a user to do that, and 
> trying to support "empty regions" in the PCI address space by 
> auto-adding pci-bridges is pointless busywork, but we have supported 
> it in the past, so I guess we need to now. Sigh.

But wait! Look at the example above - that's *exactly* the odd case that 
patch 11/17 (the RFC patch that we agreed wasn't worthwhile) remedies.

Even without that RFC patch, if you look at the code that creates a 
PCIAddressSet (qemuDomainPCIAddressSetCreate()), you'll see that during 
the initial creation, the unused bus numbers are filled in with 
pcie-root-port or pci-bridge controllers, i.e. the PCIAddressSet is 
never "sparse", and thus virDomainPCIAddressSetGrow() is guaranteed 
never to encounter the case above. So you're asking for a solution to a 
nonexistent problem.

>
>
>> That's the bit that I
>> can't quite wrap my head around.
>
> Because it's a silly thing to support. But we already do, so we have 
> to continue :-(

I retract that - it's not a problem during Grow, only during initial 
creation, and we previously decided that it was a strange corner case 
that is unnecessary in practice and that we don't want to support.