| Message ID | 1477414421.3434.14.camel@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
On Tue, 2016-10-25 at 14:08 -0400, Laine Stump wrote: > > [...] > > > @@ -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(). Sorry, I haven't looked at any of your follow-up series at all yet. Disregard my comments then :) -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
From 2ce5a69b762fe6767f3289c200cf2ce3acc69a52 Mon Sep 17 00:00:00 2001
From: Andrea Bolognani <abologna@redhat.com>
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