9:55 a.m.
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.