On 01/15/2015 02:14 PM, Erik Skultety wrote: > In case we find out, there are more PCI devices to be connected > than there are available slots on the default PCI bus, we automatically add a > new bus and a related PCI bridge controller as well. This also happened when: * there was exactly the same amount of PCI devices as available slots * all the PCI adresses were specified in the XML [let's call it fully-exhausted XML] which is what you're fixing here. > As there are no free slots > left on the default PCI bus, PCI bridge controller gets a free slot on a > newly created PCI bus which causes qemu to refuse to start the guest. > This fix introduces a new function qemuDomainPCIBusFullyReserved which > is checked right before we possibly try to reserve a slot for PCI bridge > controller. > While this fix is great for the correct use of fully exhausted XML, if you add one more device to that XML, we generate incorrect QEMU command line: -device pci-bridge,chassis_nr=1,id=pci.1,bus=pci.1,addr=0x1 instead of erroring out. > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1132900 > --- > src/qemu/qemu_command.c | 24 +++++++++++++++++++++--- > 1 file changed, 21 insertions(+), 3 deletions(-) > > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index 3346e95..06def5f 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -1445,6 +1445,18 @@ qemuDomainSupportsPCI(virDomainDefPtr def) > return false; > } > > +static bool > +qemuDomainPCIBusFullyReserved(virDomainPCIAddressBusPtr bus) > +{ > + size_t i; > + > + for (i = bus->minSlot; i <= bus->maxSlot; i++) > + if (!bus->slots[i]) > + return false; > + > + return true; > +} > + > int > qemuDomainAssignPCIAddresses(virDomainDefPtr def, > virQEMUCapsPtr qemuCaps, > @@ -1479,9 +1491,15 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, > goto cleanup; > if (qemuAssignDevicePCISlots(def, qemuCaps, addrs) < 0) > goto cleanup; > - /* Reserve 1 extra slot for a (potential) bridge */ > - if (virDomainPCIAddressReserveNextSlot(addrs, &info, flags) < 0) > - goto cleanup; So originally we reserved one empty slot, to make sure we don't generate fully exhausted XMLs if the bus is full. > + > + 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; > + } > + } > But in this loop you add one for every non-empty bus, which does not seem that useful - if there is enough space on the buses, then there's no need to reserve the empty slot at all, because it won't result in generating a new bus and adding a new bridge. It's only when the buses are full when we want to add a new one. If we want to keep the functionality of not-generating fully exhausted XMLs, the check for full buses should be done after collecting the addresses specified by the user, but before we assign the non-specified addresses. That is - separate the static machine-type specific addresses from AssignDevicePCISlots and check the buses just before assigning the dynamic addresses. If we don't, we should delete this loop, since it doesn't really do anything. As for the incorrect command line generation, it would be nice to check that the bus we put the bridge on is less than its index and report an XML error instead of trying to run QEMU. Jan
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list