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