On Mon, 2016-11-07 at 14:50 -0500, Laine Stump wrote:
Andrea had the right idea when he disabled the "reserve an
extra
unused slot" bit for aarch64/virt.
You bet I did! :P
For *any* PCI Express-based
machine, it is pointless since 1) an extra legacy-PCI slot can't be
used for hotplug, since hotplug into legacy PCI slots doesn't work on
PCI Express machinetypes, and 2) even for "coldplug" expansion,
everybody will want to expand using Express controllers, not legacy
PCI.
This patch eliminates the extra slot reserve unless the system has a
pci-root (i.e. legacy PCI)
[...]
@@ -1817,23 +1815,35 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr
def,
addrs) < 0)
goto cleanup;
- for (i = 0; i < addrs->nbuses; i++) {
- if (!qemuDomainPCIBusFullyReserved(&addrs->buses[i]))
- buses_reserved = false;
- }
-
- /* Reserve 1 extra slot for a (potential) bridge only if buses
- * are not fully reserved yet.
+ /* For domains that have pci-root, reserve 1 extra slot for a
+ * (potential) bridge (for future expansion) only if buses are
+ * not fully reserved yet (if all buses are fully reserved
+ * with manually/previously assigned addresses, any attempt to
+ * reserve an extra slot would fail anyway. But if all buses
+ * are *not* fully reserved, this extra reservation might push
+ * the config to add a new pci-bridge to plug into the final
+ * available slot, thus preserving the ability to expand)
*
- * We don't reserve the extra slot for aarch64 mach-virt guests
- * either because we want to be able to have pure virtio-mmio
- * guests, and reserving this slot would force us to add at least
- * a dmi-to-pci-bridge to an otherwise PCI-free topology
+ * We only do this for those domains that have pci-root, since
+ * those with pcie-root will usually want to expand using PCIe
+ * controllers, which we will do after assigning addresses for
+ * all *actual* devices.
*/
- if (!buses_reserved &&
I feel like I mentioned this in a previous review, but just
in case I didn't: the declaration for buses_reserved should
be moved to this inner scope, as it's not used at all
outside of it.
- !qemuDomainMachineIsVirt(def) &&
- qemuDomainPCIAddressReserveNextSlot(addrs, &info) < 0)
- goto cleanup;
+
+ if (qemuDomainMachineHasPCIRoot(def)) {
+ info.pciConnectFlags = (VIR_PCI_CONNECT_HOTPLUGGABLE |
+ VIR_PCI_CONNECT_TYPE_PCI_DEVICE);
info almost falls into the same bucket, except I see later
in the series it will be reused to make sure an empty,
hotplug-capable PCIe slot is availabe to newly-defined
guests.
I would argue that, even then, both scopes are narrow enough
that it would still be nicer to define info twice close to
its usages than sharing a single variable with a very wide
scope. That said, I leave the final call on this one entirely
up to you :)
ACK with the scope(s?) adjusted.
--
Andrea Bolognani / Red Hat / Virtualization