On Tue, 2016-10-25 at 09:49 -0400, Laine Stump wrote:
Set pciConnectFlags in each device's DeviceInfo and then use those flags later when validating existing addresses in qemuDomainCollectPCIAddress() and when assigning new addresses with qemuDomainPCIAddressReserveNextAddr() (rather than scattering the logic about which devices need which type of slot all over the place). Note that the exact flags set by qemuDomainDeviceCalculatePCIConnectFlags() are different from the flags previously set manually in qemuDomainCollectPCIAddress(), but this doesn't matter because all validation of addresses in that case ignores the setting of the HOTPLUGGABLE flag, and treats PCIE_DEVICE and PCI_DEVICE the same (this lax checking was done on purpose, because there are some things that we want to allow the user to specify manually, e.g. assigning a PCIe device to a PCI slot, that we *don't* ever want libvirt to do automatically. The flag settings that we *really* want to match are 1) the old flag settings in qemuDomainAssignDevicePCISlots() (which is HOTPLUGGABLE | PCI_DEVICE for everything except PCI controllers) and the new flag settings done
[...] and 2) the new [...]
by qemuDomainDeviceCalculatePCIConnectFlags() (which are currently exactly that - HOTPLUGGABLE | PCI_DEVICE for everything except PCI controllers). --- src/qemu/qemu_domain_address.c | 205 +++++++++++++++-------------------------- 1 file changed, 74 insertions(+), 131 deletions(-) diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index 602179c..d731b19 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -721,7 +721,7 @@ qemuDomainFillDevicePCIConnectFlagsIter(virDomainDefPtr def ATTRIBUTE_UNUSED, * failure would be some internal problem with * virDomainDeviceInfoIterate()) */ -static int ATTRIBUTE_UNUSED +static int qemuDomainFillAllPCIConnectFlags(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) {
You should remove ATTRIBUTE_UNUSED from qemuDomainFillDevicePCIConnectFlags() as well. [...]
@@ -926,7 +857,7 @@ qemuDomainCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED, entireSlot = (addr->function == 0 && addr->multi != VIR_TRISTATE_SWITCH_ON); - if (virDomainPCIAddressReserveAddr(addrs, addr, flags, + if (virDomainPCIAddressReserveAddr(addrs, addr, info->pciConnectFlags, entireSlot, true) < 0)
Would it be cleaner to have a qemuDomainPCIAddressReserveAddr() function that takes @info directly? It would be used only a single time, so it could very well be argued that it would be overkill... On the other hand, it would be neat not to use virDomainPCIAddressReserve*() functions at all in the qemu driver and rely solely on the wrappers instead. Speaking of which, even with the full series applied there are still a bunch of uses of virDomainPCIAddressReserveAddr() and virDomainPCIAddressReserveSlot(), mostly in qemuDomainValidateDevicePCISlots{PIIX3,Q35}(). The attached patch makes all of them go away, except a few that actually make sense because they set aside addresses for potential future devices and as such don't have access to a virDomainDeviceInfo yet. I'm not saying we should deal with this right away: I'd rather go back later to tidy up the whole thing than hold up the series or go through another round of reviews for what is ultimately a cosmetic issue. [...]
@@ -1878,24 +1795,50 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, */ if (!buses_reserved && !qemuDomainMachineIsVirt(def) && - qemuDomainPCIAddressReserveNextSlot(addrs, &info, flags) < 0) + qemuDomainPCIAddressReserveNextSlot(addrs, &info) < 0) goto cleanup; if (qemuDomainAssignDevicePCISlots(def, qemuCaps, addrs) < 0) goto cleanup; for (i = 1; i < addrs->nbuses; i++) { + virDomainDeviceDef dev; + int contIndex; virDomainPCIAddressBusPtr bus = &addrs->buses[i]; if ((rv = virDomainDefMaybeAddController( def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, i, bus->model)) < 0) goto cleanup; - /* If we added a new bridge, we will need one more address */ - if (rv > 0 && - qemuDomainPCIAddressReserveNextSlot(addrs, &info, flags) < 0) + + if (rv == 0) + continue; /* no new controller added */
Alternatively, you could turn this into if (rv > 0) { virDomainDeviceDef dev; int contIndex; /* The code below */ } but I leave up to you whether to go one way or the other.
+ + /* We did add a new controller, so we will need one more + * address (and we need to set the new controller's + * pciConnectFlags) + */ + contIndex = virDomainControllerFind(def, + VIR_DOMAIN_CONTROLLER_TYPE_PCI, + i); + if (contIndex < 0) { + /* this should never happen - we just added it */ + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not find auto-added %s controller " + "with index %zu"), + virDomainControllerModelPCITypeToString(bus->model), + i); + goto cleanup; + } + dev.type = VIR_DOMAIN_DEVICE_CONTROLLER; + dev.data.controller = def->controllers[contIndex]; + /* set connect flags so it will be properly addressed */ + qemuDomainFillDevicePCIConnectFlags(def, &dev, qemuCaps); + if (qemuDomainPCIAddressReserveNextSlot(addrs, + &dev.data.controller->info) < 0) goto cleanup; } + nbuses = addrs->nbuses; virDomainPCIAddressSetFree(addrs); addrs = NULL;
ACK -- Andrea Bolognani / Red Hat / Virtualization