Re: [PATCH v6 09/17] [ACKED] qemu: only force an available legacy-PCI slot on domains with pci-root

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]