[PATCH 0/3] A couple improvements when auto-adding PCI controllers
The 1st patch fixes a tiny omission from a feature added several years ago, which slightly improves the error message when you attempt to add a pcie-to-pci-bridge controller to a domain that has no PCIe bus. The 2nd improves that error message a bit more, along with improving some comments in the code. The 3rd eliminates said error message completely when you're adding a pcie-to-pci-bridge controller to a domain that *does* support PCIe, but doesn't currently have an open pcie-root-port to plug in the pcie-to-pci-bridge (by auto-adding another pcie-root-port). The impetus for these was https://issues.redhat.com/browse/RHEL-62032 Laine Stump (3): conf: add forgotten clause to virDomainPCIControllerConectTypeToModel() conf: improve error message when a PCI controller can't be auto-added conf: auto-add a pcie-root-port when needed while plugging in pcie-to-pci-bridge src/conf/domain_addr.c | 43 +++++++++++++++---- .../pcie-root-port-too-many.x86_64-latest.err | 2 +- 2 files changed, 35 insertions(+), 10 deletions(-) -- 2.51.0
When building the PCI topology of a domain that has PCI devices with no assigned PCI addresses, the function virDomainPCIAddressSetGrow() will attempt to add a new PCI controller with the appropriate type of slot to connect a device that doesn't have a PCI address. In some cases this isn't possible (for example, if the device you are attempting to add to the topology requires a type of connection only provided by some controller that *itself* requires a connection of a type not available for the given architecture/machinetype, e.g. if you want to add a pcie-root-port to a domain with a machine type that has a pci-root (no PCIE)). In those cases, an error message is logged by using virDomainPCIControllerConectTypeToModel() to extract the type of device from the "flags" that are sent to virDomainPCIAddressSetGrow(). However, if virDomainPCIControllerConectTypeToModel() doesn't find a device type in the connection flags, it will return -1, and virDomainPCIAddressSetGrow() will log the very generic: Cannot automatically add a new PCI bus for a device with connect flags nnnn Both of these functions were written prior to libvirt adding support for the pcie-to-pci-bridge controller, and when that support was added (in 2018), a new if() clause wasn't added to virDomainPCIControllerConectTypeToModel(). Seven (!) years later, this omission was noticed by someone adding a new test case to regression testing. This patches remedies the earlier omission, so that now when someone tries to add a pcie-to-pci-bridge controller to a domain that doesn't have PCIE, the message will be: a PCI slot is needed to connect a PCI controller model='pcie-to-pci-bridge', but none is available, and it cannot be automatically added This is still not an ideal error message, but is arguably better. (NB: Unfortunately it isn't possible to use a switch() statement with no default case (in order to catch a similar error in the future), since we are converting from bitmapped flags. Fortunately, we haven't needed to add a new PCI controller type in the last 7 1/2 years :-) Resolves: https://issues.redhat.com/browse/RHEL-62032 Fixes: 542f05e7756cc5614eb2ee7b0bac9aabb7aa016c Signed-off-by: Laine Stump <laine@redhat.com> --- src/conf/domain_addr.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index 8dfa8feca0..7d58e2222a 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -286,6 +286,9 @@ virDomainPCIControllerConnectTypeToModel(virDomainPCIConnectFlags flags) if (flags & VIR_PCI_CONNECT_TYPE_PCI_BRIDGE) return VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE; + if (flags & VIR_PCI_CONNECT_TYPE_PCIE_TO_PCI_BRIDGE) + return VIR_DOMAIN_CONTROLLER_MODEL_PCIE_TO_PCI_BRIDGE; + /* some connect types don't correspond to a controller model */ return -1; } -- 2.51.0
Log a slightly different message when the missing-but-required slot is conventional PCI vs PCIe. Also correct/improve the comments about why auto-add of a PCI controller isn't supported when we're trying to create a slot for various different pci controllers. Signed-off-by: Laine Stump <laine@redhat.com> --- src/conf/domain_addr.c | 37 +++++++++++++++---- .../pcie-root-port-too-many.x86_64-latest.err | 2 +- 2 files changed, 30 insertions(+), 9 deletions(-) diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index 7d58e2222a..ef1b2bd69a 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -704,7 +704,7 @@ virDomainPCIAddressSetGrow(virDomainPCIAddressSet *addrs, * and we can't automatically decide which numa node to * associate it with) * - * VIR_CONNECT_TYPE_PCIE_SWITCH_DOWNSTREAM_PORT - we ndon't + * VIR_CONNECT_TYPE_PCIE_SWITCH_DOWNSTREAM_PORT - we don't * support this, because it can only plug into an * upstream-port, and the upstream port might need a * root-port; supporting this extra layer needlessly @@ -717,20 +717,41 @@ virDomainPCIAddressSetGrow(virDomainPCIAddressSet *addrs, * devices on the host, and are also outside the scope of our * "automatic-bus-expansion". * - * VIR_PCI_CONNECT_TYPE_PCI_BRIDGE (when the root bus is - * pci-root) - see the comment above in the case that handles - * adding a slot for pci-bridge to a guest with pcie-root. + * VIR_PCI_CONNECT_TYPE_PCI_BRIDGE if the root bus is pci-root + * but there are no free slots already available to plug in a + * pci-bridge, then the battle is already lost - the only way + * to get another open slot would be to auto-add a pci-bridge + * device, but that's what we're already trying to do - by + * definition either we wouldn't get here in the first place, + * or it's already too late. + * + * Alternatively if the root bus *isn't* pci-root, then + * either the root bus is pcie-root (in which case the user + * should be using a pcie-to-pci-bridge instead), or there is + * no PCI supported *at all*, and in both of those cases we + * should fail. + * + * Additionally, we obviously can't auto-add any type of PCIe + * controller if the root bus is pci-root, or if there is no + * PCI supported at all. * */ int existingContModel = virDomainPCIControllerConnectTypeToModel(flags); if (existingContModel >= 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("a PCI slot is needed to connect a PCI controller model='%1$s', but none is available, and it cannot be automatically added"), - virDomainControllerModelPCITypeToString(existingContModel)); + if (existingContModel == VIR_PCI_CONNECT_TYPE_PCI_EXPANDER_BUS || + existingContModel == VIR_PCI_CONNECT_TYPE_PCI_BRIDGE) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("a conventional PCI slot is needed to connect a PCI controller model='%1$s', but none is available, and it cannot be automatically added"), + virDomainControllerModelPCITypeToString(existingContModel)); + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("a PCIe slot is needed to connect a PCI controller model='%1$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 %1$.2x"), + _("Cannot automatically add a new PCI bus for a device with unrecognized connect flags %1$.2x"), flags); } return -1; diff --git a/tests/qemuxmlconfdata/pcie-root-port-too-many.x86_64-latest.err b/tests/qemuxmlconfdata/pcie-root-port-too-many.x86_64-latest.err index 9b24cfb7e0..e1d543a50a 100644 --- a/tests/qemuxmlconfdata/pcie-root-port-too-many.x86_64-latest.err +++ b/tests/qemuxmlconfdata/pcie-root-port-too-many.x86_64-latest.err @@ -1 +1 @@ -internal error: a PCI slot is needed to connect a PCI controller model='pcie-root-port', but none is available, and it cannot be automatically added +internal error: a PCIe slot is needed to connect a PCI controller model='pcie-root-port', but none is available, and it cannot be automatically added -- 2.51.0
This will almost surely never come up during any normal operation[*], which is likely why this wasn't done when pcie-to-pci-bridge support was added back in the before-fore times. It's pretty simple to support though - a pcie-to-pci-bridge plugs into a pcie-root-port just like any other pcie device, and if there isn't an open slot on an existing pcie-root-port, we can just add one. ([*] in real life, a pcie-to-pci-bridge is only auto-added by libvirt itself, while this function is dealing with the followup to *user added* devices. Also each pcie-to-pci-bridge has 32 slots, and it's unlikely a domain with pcie support would be wanting more than 32 conventional PCI (i.e. not PCIe) devices) Signed-off-by: Laine Stump <laine@redhat.com> --- src/conf/domain_addr.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index ef1b2bd69a..6aa5ac1b9f 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -691,7 +691,8 @@ virDomainPCIAddressSetGrow(virDomainPCIAddressSet *addrs, } } } else if (flags & (VIR_PCI_CONNECT_TYPE_PCIE_DEVICE | - VIR_PCI_CONNECT_TYPE_PCIE_SWITCH_UPSTREAM_PORT)) { + VIR_PCI_CONNECT_TYPE_PCIE_SWITCH_UPSTREAM_PORT | + VIR_PCI_CONNECT_TYPE_PCIE_TO_PCI_BRIDGE)) { model = VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT; } else { /* The types of devices that we can't auto-add a controller for: -- 2.51.0
participants (1)
-
Laine Stump