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 / VirtualizationFrom 2ce5a69b762fe6767f3289c200cf2ce3acc69a52 Mon Sep 17 00:00:00 2001
From: Andrea Bolognani <abologna@xxxxxxxxxx>
Date: Tue, 25 Oct 2016 18:37:47 +0200
Subject: [PATCH] qemu: Introduce qemuDomainPCIAddressReserve{Addr,Slot}()
---
src/qemu/qemu_domain_address.c | 54 ++++++++++++++++++++++++++++++++----------
1 file changed, 42 insertions(+), 12 deletions(-)
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index 42af435..f17eaa1 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -789,6 +789,30 @@ qemuDomainFillDevicePCIConnectFlags(virDomainDefPtr def,
static int
+qemuDomainPCIAddressReserveAddr(virDomainPCIAddressSetPtr addrs,
+ virPCIDeviceAddressPtr addr,
+ virDomainDeviceInfoPtr info,
+ bool reserveEntireSlot,
+ bool fromConfig)
+{
+ return virDomainPCIAddressReserveAddr(addrs, addr,
+ info->pciConnectFlags,
+ reserveEntireSlot, fromConfig);
+}
+
+
+static int
+qemuDomainPCIAddressReserveSlot(virDomainPCIAddressSetPtr addrs,
+ virPCIDeviceAddressPtr addr,
+ virDomainDeviceInfoPtr info)
+{
+ return qemuDomainPCIAddressReserveAddr(addrs, addr,
+ info,
+ true, false);
+}
+
+
+static int
qemuDomainPCIAddressReserveNextAddr(virDomainPCIAddressSetPtr addrs,
virDomainDeviceInfoPtr dev,
unsigned int function,
@@ -892,8 +916,8 @@ qemuDomainCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED,
entireSlot = (addr->function == 0 &&
addr->multi != VIR_TRISTATE_SWITCH_ON);
- if (virDomainPCIAddressReserveAddr(addrs, addr, info->pciConnectFlags,
- entireSlot, true) < 0)
+ if (qemuDomainPCIAddressReserveAddr(addrs, addr, info,
+ entireSlot, true) < 0)
goto cleanup;
ret = 0;
@@ -1113,7 +1137,8 @@ qemuDomainValidateDevicePCISlotsPIIX3(virDomainDefPtr def,
goto cleanup;
}
} else {
- if (virDomainPCIAddressReserveSlot(addrs, &tmp_addr, flags) < 0)
+ if (qemuDomainPCIAddressReserveSlot(addrs, &tmp_addr,
+ &primaryVideo->info) < 0)
goto cleanup;
primaryVideo->info.addr.pci = tmp_addr;
primaryVideo->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
@@ -1214,8 +1239,9 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def,
assign = true;
}
if (assign) {
- if (virDomainPCIAddressReserveAddr(addrs, &tmp_addr,
- flags, false, true) < 0)
+ if (qemuDomainPCIAddressReserveAddr(addrs, &tmp_addr,
+ &cont->info,
+ false, true) < 0)
goto cleanup;
cont->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
cont->info.addr.pci.domain = 0;
@@ -1237,8 +1263,8 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def,
memset(&tmp_addr, 0, sizeof(tmp_addr));
tmp_addr.slot = 0x1E;
if (!virDomainPCIAddressSlotInUse(addrs, &tmp_addr)) {
- if (virDomainPCIAddressReserveAddr(addrs, &tmp_addr,
- flags, true, false) < 0)
+ if (qemuDomainPCIAddressReserveSlot(addrs, &tmp_addr,
+ &cont->info) < 0)
goto cleanup;
cont->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
cont->info.addr.pci.domain = 0;
@@ -1301,7 +1327,9 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def,
goto cleanup;
}
} else {
- if (virDomainPCIAddressReserveSlot(addrs, &tmp_addr, flags) < 0)
+ if (qemuDomainPCIAddressReserveAddr(addrs, &tmp_addr,
+ &primaryVideo->info,
+ true, true) < 0)
goto cleanup;
primaryVideo->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
primaryVideo->info.addr.pci = tmp_addr;
@@ -1349,7 +1377,9 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def,
!virDeviceInfoPCIAddressWanted(&sound->info)) {
continue;
}
- if (virDomainPCIAddressReserveSlot(addrs, &tmp_addr, flags) < 0)
+ if (qemuDomainPCIAddressReserveAddr(addrs, &tmp_addr,
+ &sound->info,
+ true, true) < 0)
goto cleanup;
sound->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
@@ -1565,9 +1595,9 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def,
if (foundAddr) {
/* Reserve this function on the slot we found */
- if (virDomainPCIAddressReserveAddr(addrs, &addr,
- cont->info.pciConnectFlags,
- false, true) < 0)
+ if (qemuDomainPCIAddressReserveAddr(addrs, &addr,
+ &cont->info,
+ false, true) < 0)
goto error;
cont->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
--
2.7.4
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list