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
a) pci-root - if there is an open slot it will be assigned to the
pci-bridge.
If there isn't an open slot, then there is no chance for success
anyway,
so the correct 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.
(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.
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 :-(
+ } else if (flags & (VIR_PCI_CONNECT_TYPE_PCIE_DEVICE |
+ VIR_PCI_CONNECT_TYPE_PCIE_SWITCH_UPSTREAM_PORT)) {
+ model = VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT;
+ } else {
+ int existingContModel = virDomainPCIControllerConnectTypeToModel(flags);
+
+ if (existingContModel >= 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("a PCI slot is needed to connect a PCI controller "
+ "model='%s', but none is available, and it "
+ "cannot be automatically added"),
+ virDomainControllerModelPCITypeToString(existingContModel));
+ } else {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Cannot automatically add a new PCI bus for a "
+ "device with connect flags %.2x"), flags);
+ }
So we error out for dmi-to-pci-bridge and
pci{,e}-expander-bus... Shouldn't we be able to plug either
into into pcie-root or pci-root?
Exactly. And since those buses already exist from the start, and can't
be added later, there wouldn't ever be a situation where we needed to
automatically grow the bus structure due to one of those devices and
growing could actually do anything (i.e. you can't add a pcie-root or
pci-root).
I'm clearly missing something here :(
Don't we (potentially) grow the address set using this
function every time we reserve an address for an endpoint
device or a controller?
No. It is only called when we look for an open slot that matches the
connectFlags of the device/controller, and don't find any open slot with
compatible flags.
Including when we're going through
a virDomainDef, which might contain a dmi-to-pci-bridge
or a pci{,e}-expander-bus?
Sure, it can contain one of those. But the only place you can plug in a
pci[e]-expander-bus is pci[e]-root, and you can't add a new pci[e]-root,
so if you've looked for an empty slot and there isn't one, then you have
an error. Since the ...Grow() function is only called if you've already
failed to find an existing appropriate slot, it's correct for it to give
an error.
A dmi-to-pci-bridge is *slightly* different - it could also be connected
to a pcie-expander-bus. But we don't automatically add those either,
since they're really intended for supporting NUMA (and anyway, if there
is no free slot in pcie-root to plug in a dmi-to-pci-bridge, then there
is *also* no free slot to plug in a pcie-expander-bus). So again, if
you've looked for an open slot to plug in a dmi-to-pci-bridge and
couldn't find one, then when you call this function there is nothing
productive it can do, so it's proper for it to fail and log an error.
I'm not expecting the pci{,e}-root case to be handled, of
course, but I can't figure out why we seem to be skipping
over a bunch of perfectly valid cases.
Because they're not valid cases.
- virDomainPCIAddressBusSetModel(&addrs->buses[i],
- VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE);
+ virDomainPCIAddressBusSetModel(&addrs->buses[i++],
+ VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE);
virDomainPCIAddressBusSetModel() can fail, yet you're not
checking the return value neither here...
Yes, but the only way it can fail is if an unknown controller model is
sent to it, and we can see by simple physical inspection that that isn't
the case. If this was calling off to a function in some other file where
we didn't know just how simple it is and couldn't easily see that it
would never fail, then I would agree that we should check, but in this
case it's superfluous - if this function had an ATTRIBUTE_RETURN_CHECK
on it, I would have put these calls inside ignore_value().
[...]
}
+
+ for (; i < addrs->nbuses; i++)
+ virDomainPCIAddressBusSetModel(&addrs->buses[i], model);
... nor here.
Same for this - the controller model is one of just a couple hardcoded
values set within the same function, not sent in from somewhere else,
and the function we're calling is a very simple function in the same file.
[...]
@@ -947,7 +934,43 @@ qemuDomainPCIAddressSetCreate(virDomainDefPtr def,
if (virDomainPCIAddressBusSetModel(&addrs->buses[idx], cont->model) < 0)
goto error;
- }
Since I'm sure you'll bring up the fact that I *am* checking the return
value of virDomainPCIAddressBusSetModel here (and everywhere else in
qemu_domain_address.c), I'll point out that these calls are made from
another file in another directory, so I'm being overly paranoid, not on
any real factual basis, but just because it feels right :-)
I get where you're coming from, but I don't think I agree
with your conclusions.
Sure, virDomainPCIAddressBusSetModel() might be a trivial
function *now*, but we can't guarantee the same will be
true in the future. Sure, the model we pass to it from
inside virDomainPCIAddressSetGrow() is one of very few
known-good values, but we might eg. read it from a less
reliable source somewhere down the line.
My point is, by not checking the return value we're
implicitly embedding some knowledge about
virDomainPCIAddressSetGrow() inside
virDomainPCIAddressBusSetModel(), thus creating a strong
relationship between the two functions. We should avoid
creating such relationships, because they make the code
much more difficult to reason about and might lead to
unknowingly introducing subtle bugs.
I think that putting in error checking/recovery code for such simple
cases is just adding more code to maintain and get wrong for no gain.
(similar to checking the return from virBlahTypeToString() in cases
where we know the enum value has already been validated). But I'll cede
your point and add return checks on all the calls for
virDomainPCIAddressBusSetModel().
Rest looks good, but no ACK for you quite yet :)
So far I've changed comments and whitespace. You're welcome to dispute
my opinion about checking return value from the SetModel function, but
otherwise there's only cosmetic differences.
I would never hold my ACK hostage of a purely cosmetic
issue :)
However, as explained above, I'm still unable to wrap my
head around some details, and until I do I'm not comfortable
ACKing the patch. Hopefully you'll be able to help me clear
out any remaining doubts :)
I'll add the return value checks, and add support for calling ...Grow()
with an address > 1 over the current bus count.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list