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.