Re: [libvirt] [PATCH 3/5] qemu: Fix auto-adding PCI bridge when all slots are reserved

Commit 93c8ca tried to fix the issue with auto-adding of a PCI bridge controller, it worked well when the slots were reserved by devices with user defined addresses without any other devices with unspecified PCI addresses present in the XML. However, if another device with unspecified PCI addresses appeared in the domain's XML, the same problem occurred as the BZ below references. This patch fixes the issue by not creating any additional bridges when not necessary. Now let's say all the buses corresponding to the number of PCI controllers present in the domain's XML got fully reserved by devices with user defined addresses. If there are no other devices present in the XML, no need to create both an additional PCI bus and a bridge controller. If however there are still some PCI devices waiting to get a slot assigned by qemuAssignDevicePCISlots, this means an additional bus needs to be created along with a corresponding bridge controller. This scenario now results in an reasonable error instead of generating wrong qemu command line. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1132900 --- src/qemu/qemu_command.c | 42 ++++++++++++++++++++++++++++++++++-------- 1 file changed, 34 insertions(+), 8 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index cdf02a6..99b06ee 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1472,6 +1472,9 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, int nbuses = 0; size_t i; int rv; + int resflags = 0; + bool buses_reserved = false; +

virDomainPCIConnectFlags flags = VIR_PCI_CONNECT_TYPE_PCI; for (i = 0; i < def->ncontrollers; i++) { @@ -1490,17 +1493,22 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, /* 1st pass to figure out how many PCI bridges we need */ if (!(addrs = qemuDomainPCIAddressSetCreate(def, nbuses, true))) goto cleanup; + + for (i = 0; i < nbuses; i++) {

+ if (qemuDomainPCIBusFullyReserved(&addrs->buses[i])) + resflags |= 1 << i;

+ } + buses_reserved = (resflags && ~((~0) << nbuses)); + if (qemuAssignDevicePCISlots(def, qemuCaps, addrs) < 0) goto cleanup; - for (i = 0; i < addrs->nbuses; i++) { - if (!qemuDomainPCIBusFullyReserved(&addrs->buses[i])) { - - /* Reserve 1 extra slot for a (potential) bridge */ - if (virDomainPCIAddressReserveNextSlot(addrs, &info, flags) < 0) - goto cleanup; - } - } + /* Reserve 1 extra slot for a (potential) bridge only if buses + * are not fully reserved yet + */ + if (!buses_reserved && + virDomainPCIAddressReserveNextSlot(addrs, &info, flags) < 0) + goto cleanup; for (i = 1; i < addrs->nbuses; i++) { virDomainPCIAddressBusPtr bus = &addrs->buses[i]; @@ -1532,6 +1540,24 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, if (qemuAssignDevicePCISlots(def, qemuCaps, addrs) < 0) goto cleanup; } + + for (i = 0; i < def->ncontrollers; i++) { + /* check if every PCI bridge controller's ID is greater than + * the bus it is placed onto + */ + virDomainControllerDefPtr cont = def->controllers[i]; + int idx = cont->idx; + int bus = cont->info.addr.pci.bus; + if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && + cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE && + idx <= bus) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("failed to create PCI bridge " + "on bus %d: bus is fully reserved"), + bus); + goto cleanup; + } + }