1:08 p.m.
On 10/25/2016 12:53 PM, Andrea Bolognani wrote:
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?
Actually in a later series (the one that cleans up the *Slot() vs
*Addr() naming), I eliminated all but one of the
qemuDomainPCIAddressReserve*() functions anyway. After that series,
there are only two *PCIAddressReserve*() functions used in this file:
qemuDomainPCIAddressReserveNextAddr() (21 times), and
virDomainPCIAddressReserveAddr() (12 times). The latter can't have a
nice flags-removing wrapper added in qemu_domain_address.c (like the
former does) because it often is called with a bare address - no DeviceInfo
(Well, I don't know, maybe it could be done by reorganizing some of the
calls, I'll have to look at it).
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}().
Yeah, my later series turns all of those into
virDomainPCIAddressReserveAddr().
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.
I like the reduced indent level of doing it this way :-)
> +
> + /* 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