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

On 11/10/2016 09:10 AM, Andrea Bolognani wrote: 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. Because it's a silly thing to support. But we already do, so we have to continue :-( 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. 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. Because they're not valid cases. 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(). I'll add the return value checks, and add support for calling ...Grow() with an address > 1 over the current bus count.