[libvirt] [PATCH v4 21/25] [NEW RFC] qemu: if pci-bridge is in PCIe config w/o dmi-to-pci-bridge, add it

[Laine Stump]

I'm undecided if it is worthwhile to add this...

Up until now it has been legal to have something like this in the xml:

  <controller type='pci' model='pcie-root' index='0'/>
  <controller type='pci' model='pci-bridge' index='2'/>

(for example, see the existing test case
"usb-controller-default-q35").  This is handled in
qemuDomainPCIAddressSetCreate() when it's adding in controllers to
fill holes in the indexes.)

Since we have always added the following to every config:

  <controller type='pci' model='dmi-to-pci-bridge' index='1'/>

there has always been a proper place for the unaddressed pci-bridge to
plug in.

A upcoming commit will eliminate the forced adding of a
dmi-to-pci-bridge to *every* Q35 domain config, though. This would
mean the above-mentioned test case (and one other) would begin to
fail. There are two possible solutions to this problem:

1) change the test cases, and made the above construct an error (note
that in the future it will work just fine to not list *any* pci
controller, and they will all be auto-added as needed).

or

2) This patch, which specifically looks for the case where we have a
pci-bridge (but no dmi-to-pci-bridge) in a PCIe domain config and
auto-adds the necessary dmi-to-pci-bridge. This can only work in the
case that the pci-bridge has been given an index such that there is an
unused index with a lower value for the dmi-to-pci-bridge to occupy.

This seems like a kind of odd corner case that nobody should need to
do in the future anyway. On the other hand, I already wrote the code
and it works, so I might as well send it and see what opinions (if
any) it generates.
---
 src/qemu/qemu_domain_address.c | 42 +++++++++++++++++++++++++++++++++++++-----
 1 file changed, 37 insertions(+), 5 deletions(-)

diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index 38b7775..e81d212 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -919,6 +919,9 @@ qemuDomainPCIAddressSetCreate(virDomainDefPtr def,
     virDomainPCIAddressSetPtr addrs;
     size_t i;
     bool hasPCIeRoot = false;
+    unsigned int lowestDMIToPCIBridge = nbuses;
+    unsigned int lowestUnaddressedPCIBridge = nbuses;
+    unsigned int lowestAddressedPCIBridge = nbuses;
     virDomainControllerModelPCI defaultModel;
 
     if ((addrs = virDomainPCIAddressSetAlloc(nbuses)) == NULL)
@@ -943,8 +946,24 @@ qemuDomainPCIAddressSetCreate(virDomainDefPtr def,
         if (virDomainPCIAddressBusSetModel(&addrs->buses[idx], cont->model) < 0)
             goto error;
 
-        if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT)
+        /* we'll use all this info later to determine if we need
+         * to add a dmi-to-pci-bridge due to unaddressed pci-bridge controllers
+         */
+        if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) {
             hasPCIeRoot = true;
+        } else if (cont->model ==
+                   VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE) {
+            if (lowestDMIToPCIBridge > idx)
+                lowestDMIToPCIBridge = idx;
+        } else if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE) {
+            if (virDeviceInfoPCIAddressWanted(&cont->info)) {
+                if (lowestUnaddressedPCIBridge > idx)
+                    lowestUnaddressedPCIBridge = idx;
+            } else {
+                if (lowestAddressedPCIBridge > idx)
+                    lowestAddressedPCIBridge = idx;
+            }
+        }
     }
 
     if (nbuses > 0 && !addrs->buses[0].model) {
@@ -960,13 +979,21 @@ qemuDomainPCIAddressSetCreate(virDomainDefPtr def,
 
     /* 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.
+     * config.  In the rare (and ... strange, but still allowed) case
+     * that a domain has 1) a pcie-root at index 0, 2) *no*
+     * dmi-to-pci-bridge (or pci-bridge that was manually addressed to
+     * sit directly on pcie-root), and 3) does have an unaddressed
+     * pci-bridge at an index > 1, then we need to add a
+     * dmi-to-pci-bridge.
      */
 
-    if (hasPCIeRoot)
-        defaultModel = VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT;
-    else
+    if (!hasPCIeRoot)
         defaultModel = VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE;
+    else if (lowestUnaddressedPCIBridge < MIN(lowestAddressedPCIBridge,
+                                              lowestDMIToPCIBridge))
+        defaultModel = VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE;
+    else
+        defaultModel = VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT;
 
     for (i = 1; i < addrs->nbuses; i++) {
 
@@ -978,6 +1005,11 @@ qemuDomainPCIAddressSetCreate(virDomainDefPtr def,
 
         VIR_DEBUG("Auto-adding <controller type='pci' model='%s' index='%zu'/>",
                   virDomainControllerModelPCITypeToString(defaultModel), i);
+        /* only add a single dmi-to-pci-bridge, then add pcie-root-port
+         * for any other unspecified controller indexes.
+         */
+        if (hasPCIeRoot)
+            defaultModel = VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT;
     }
 
     if (virDomainDeviceInfoIterate(def, qemuDomainCollectPCIAddress, addrs) < 0)
-- 
2.7.4