8:19 a.m.
On Mon, 2016-11-07 at 14:50 -0500, Laine Stump wrote:
Previously libvirt would only add pci-bridge devices automatically
when an address was requested for a device that required a legacy PCI
slot and none was available. This patch expands that support to
dmi-to-pci-bridge (which is needed in order to add a pci-bridge on a
machine with a pcie-root), and pcie-root-port (which is needed to add
a hotpluggable PCIe device). It does *not* automatically add
pcie-switch-upstream-ports or pcie-switch-downstream-ports (and
currently there are no plans for that).
[...]
Although libvirt will now automatically create a dmi-to-pci-bridge
when it's needed, the code still remains for now that forces a
dmi-to-pci-bridge on all domains with pcie-root (in
qemuDomainDefAddDefaultDevices()). That will be removed in the next
patch.
s/in the next/in a future/
For now, the pcie-root-ports are added one to a slot, which is a bit
wasteful and means it will fail after 31 total PCIe devices (30 if
there are also some PCI devices), but helps keep the changeset down
for this patch. A future patch will have 8 pcie-root-ports sharing the
functions on a single slot.
[...]
@@ -82,6 +82,30 @@
virDomainPCIControllerModelToConnectType(virDomainControllerModelPCI model)
return 0;
}
+
+static int
+virDomainPCIControllerConnectTypeToModel(virDomainPCIConnectFlags flags)
+{
+ if (flags & VIR_PCI_CONNECT_TYPE_PCIE_ROOT_PORT)
+ return VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT;
Maybe adding an empty line between each branch would make
this a little more readable? Just a thought.
Also too bad these are flags and we can't use a switch()
statement here - the compiler will not warn us if we forget
to handle a VIR_PCI_CONNECT_TYPE in the future :(
[...]
@@ -351,32 +372,73 @@
virDomainPCIAddressSetGrow(virDomainPCIAddressSetPtr addrs,
{
int add;
size_t i;
+ int model;
+ bool needDMIToPCIBridge = false;
add = addr->bus - addrs->nbuses + 1;
- i = addrs->nbuses;
if (add <= 0)
return 0;
- /* auto-grow only works when we're adding plain PCI devices */
- if (!(flags & VIR_PCI_CONNECT_TYPE_PCI_DEVICE)) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Cannot automatically add a new PCI bus for a "
- "device requiring a slot other than standard
PCI."));
+ if (flags & VIR_PCI_CONNECT_TYPE_PCI_DEVICE) {
+ model = VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE;
+
+ /* if there aren't yet any buses that will accept a
+ * pci-bridge, and the caller is asking for one, we'll need to
+ * add a dmi-to-pci-bridge first.
+ */
+ needDMIToPCIBridge = true;
+ for (i = 0; i < addrs->nbuses; i++) {
+ if (addrs->buses[i].flags & VIR_PCI_CONNECT_TYPE_PCI_BRIDGE) {
+ needDMIToPCIBridge = false;
+ break;
+ }
+ }
+ if (needDMIToPCIBridge && add == 1) {
+ /* we need to add at least two buses - one dmi-to-pci,
+ * and the other the requested pci-bridge
+ */
+ add++;
+ addr->bus++;
+ }
This last part got me confused for a bit, so I wouldn't mind
having a more detailed comment here. Something like
We need to add a single pci-bridge to provide the bus our
legacy PCI device will be plugged into; however, we have
also determined that the pci-bridge we're about to add
itself needs to be plugged into a dmi-to-pci-bridge. To
accomodate both requirements, we're going to add both a
dmi-to-pci-bridge and a pci-bridge, and we're going to
bump the bus number of the device by one to account for
the extra controller.
Yeah, that's quite a mouthful :/
+ } else if (flags & VIR_PCI_CONNECT_TYPE_PCI_BRIDGE
&&
+ addrs->buses[0].model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) {
+ model = VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE;
Mh, what about the case where we want to add a pci-bridge
but the machine has a pci-root instead of a pcie-root?
Shouldn't that case be handled as well?
+ } else if (flags & (VIR_PCI_CONNECT_TYPE_PCIE_DEVICE |
+ VIR_PCI_CONNECT_TYPE_PCIE_SWITCH_UPSTREAM_PORT)) {
+ model = VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT;
+ } else {
+ int existingContModel = virDomainPCIControllerConnectTypeToModel(flags);
+
+ if (existingContModel >= 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("a PCI slot is needed to connect a PCI controller
"
+ "model='%s', but none is available, and it
"
+ "cannot be automatically added"),
+ virDomainControllerModelPCITypeToString(existingContModel));
+ } else {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Cannot automatically add a new PCI bus for a "
+ "device with connect flags %.2x"), flags);
+ }
So we error out for dmi-to-pci-bridge and
pci{,e}-expander-bus... Shouldn't we be able to plug either
into into pcie-root or pci-root?
return -1;
}
+ i = addrs->nbuses;
+
if (VIR_EXPAND_N(addrs->buses, addrs->nbuses, add) < 0)
return -1;
- for (; i < addrs->nbuses; i++) {
- /* Any time we auto-add a bus, we will want a multi-slot
- * bus. Currently the only type of bus we will auto-add is a
- * pci-bridge, which is hot-pluggable and provides standard
- * PCI slots.
+ if (needDMIToPCIBridge) {
+ /* first of the new buses is dmi-to-pci-bridge, the
+ * rest are of the requested type
*/
- virDomainPCIAddressBusSetModel(&addrs->buses[i],
- VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE);
+ virDomainPCIAddressBusSetModel(&addrs->buses[i++],
+ VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE);
virDomainPCIAddressBusSetModel() can fail, yet you're not
checking the return value neither here...
}
+
+ for (; i < addrs->nbuses; i++)
+ virDomainPCIAddressBusSetModel(&addrs->buses[i], model);
... nor here.
[...]
@@ -947,7 +934,43 @@ qemuDomainPCIAddressSetCreate(virDomainDefPtr
def,
if (virDomainPCIAddressBusSetModel(&addrs->buses[idx], cont->model)
< 0)
goto error;
- }
+
+ if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT)
+ hasPCIeRoot = true;
+ }
+
+ if (nbuses > 0 && !addrs->buses[0].model) {
+ /* This is just here to replicate a safety measure already in
+ * an older version of this code. In practice, the root bus
+ * should have already been added at index 0 prior to
+ * assigning addresses to devices.
+ */
+ if (virDomainPCIAddressBusSetModel(&addrs->buses[0],
+ VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) <
0)
+ goto error;
+ }
+
+ /* Now fill in a reasonable model for all the buses in the set
+ * that don't yet have a corresponding controller in the domain
+ * config.
+ */
+
No empty line here.
Rest looks good, but no ACK for you quite yet :)
--
Andrea Bolognani / Red Hat / Virtualization