[libvirt] [PATCH v6 10/17] qemu: auto-add pcie-root-port/dmi-to-pci-bridge controllers as needed
Laine Stump
laine at laine.org
Sun Nov 13 15:55:37 UTC 2016
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.
More information about the libvir-list
mailing list