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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list