On 01/22/2015 05:00 PM, Ján Tomko wrote: > On 01/21/2015 05:50 PM, Erik Skultety wrote: >> 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(-) >> > > This breaks the pci-many test case you added before. > >> 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; >> + > > If you set this to true by default... > >> 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; > > you can do: > if (!fullyReserved(buses[i])) > buses_reserved = false; > Yeah, that one looks better, thanks. > Or just rename the bool to something like 'addExtraEmptySlot' and invert the > condition below. (To stay more positive :)) > >> + } >> + buses_reserved = (resflags && ~((~0) << nbuses)); >> + Not to mention this one should have been bitwise AND instead of logical one anyway. >> 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; >> + } >> + } > > This should IMHO be a separate commit, as it does not fix the case where the > devices exactly fill the buses, but when there are too many. > You're probably right, coming in v2 series. > Also, maybe the error message could say something about 'too many devices with > fixed addressess'? > > Jan > > > > -- > libvir-list mailing list > libvir-list@xxxxxxxxxx > https://www.redhat.com/mailman/listinfo/libvir-list > Erik -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list