qemu: set/use info->pciConnectFlags when validating/assigning PCI addresses

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 2) the new flag settings
done by qemuDomainDeviceCalculatePCIConnectFlags() (which are
currently exactly that - HOTPLUGGABLE | PCI_DEVICE for everything
except PCI controllers).

src/qemu/qemu_domain_address.c

@@ -722,7 +722,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)
 {
@@ -747,7 +747,7 @@ qemuDomainFillAllPCIConnectFlags(virDomainDefPtr def,
  *
  * No return value.
  */
-static void ATTRIBUTE_UNUSED
+static void
 qemuDomainFillDevicePCIConnectFlags(virDomainDefPtr def,
                                     virDomainDeviceDefPtr dev,
                                     virQEMUCapsPtr qemuCaps)
@@ -778,21 +778,20 @@ qemuDomainFillDevicePCIConnectFlags(virDomainDefPtr def,
 static int
 qemuDomainPCIAddressReserveNextAddr(virDomainPCIAddressSetPtr addrs,
                                     virDomainDeviceInfoPtr dev,
-                                    virDomainPCIConnectFlags flags,
                                     unsigned int function,
                                     bool reserveEntireSlot)
 {
-    return virDomainPCIAddressReserveNextAddr(addrs, dev, flags,
+    return virDomainPCIAddressReserveNextAddr(addrs, dev,
+                                              dev->pciConnectFlags,
                                               function, reserveEntireSlot);
 }
 
 
 static int
 qemuDomainPCIAddressReserveNextSlot(virDomainPCIAddressSetPtr addrs,
-                                    virDomainDeviceInfoPtr dev,
-                                    virDomainPCIConnectFlags flags)
+                                    virDomainDeviceInfoPtr dev)
 {
-    return qemuDomainPCIAddressReserveNextAddr(addrs, dev, flags, 0, true);
+    return qemuDomainPCIAddressReserveNextAddr(addrs, dev, 0, true);
 }
 
 
@@ -806,9 +805,6 @@ qemuDomainCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED,
     int ret = -1;
     virPCIDeviceAddressPtr addr = &info->addr.pci;
     bool entireSlot;
-    /* flags may be changed from default below */
-    virDomainPCIConnectFlags flags = (VIR_PCI_CONNECT_HOTPLUGGABLE |
-                                      VIR_PCI_CONNECT_TYPE_PCI_DEVICE);
 
     if (!virDeviceInfoPCIAddressPresent(info) ||
         ((device->type == VIR_DOMAIN_DEVICE_HOSTDEV) &&
@@ -820,71 +816,6 @@ qemuDomainCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED,
         return 0;
     }
 
-    /* Change flags according to differing requirements of different
-     * devices.
-     */
-    switch (device->type) {
-    case VIR_DOMAIN_DEVICE_CONTROLLER:
-        switch (device->data.controller->type) {
-        case  VIR_DOMAIN_CONTROLLER_TYPE_PCI:
-           flags = virDomainPCIControllerModelToConnectType(device->data.controller->model);
-            break;
-
-        case VIR_DOMAIN_CONTROLLER_TYPE_SATA:
-            /* SATA controllers aren't hot-plugged, and can be put in
-             * either a PCI or PCIe slot
-             */
-            flags = (VIR_PCI_CONNECT_TYPE_PCI_DEVICE
-                     | VIR_PCI_CONNECT_TYPE_PCIE_DEVICE);
-            break;
-
-        case VIR_DOMAIN_CONTROLLER_TYPE_USB:
-            /* allow UHCI and EHCI controllers to be manually placed on
-             * the PCIe bus (but don't put them there automatically)
-             */
-            switch (device->data.controller->model) {
-            case VIR_DOMAIN_CONTROLLER_MODEL_USB_EHCI:
-            case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_EHCI1:
-            case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI1:
-            case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI2:
-            case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI3:
-            case VIR_DOMAIN_CONTROLLER_MODEL_USB_VT82C686B_UHCI:
-                flags = VIR_PCI_CONNECT_TYPE_PCI_DEVICE;
-                break;
-            case VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI:
-                /* should this be PCIE-only? Or do we need to allow PCI
-                 * for backward compatibility?
-                 */
-                flags = (VIR_PCI_CONNECT_TYPE_PCI_DEVICE
-                         | VIR_PCI_CONNECT_TYPE_PCIE_DEVICE);
-                break;
-            case VIR_DOMAIN_CONTROLLER_MODEL_USB_PCI_OHCI:
-            case VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI:
-            case VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX4_UHCI:
-                /* Allow these for PCI only */
-                break;
-            }
-        }
-        break;
-
-    case VIR_DOMAIN_DEVICE_SOUND:
-        switch (device->data.sound->model) {
-        case VIR_DOMAIN_SOUND_MODEL_ICH6:
-        case VIR_DOMAIN_SOUND_MODEL_ICH9:
-            flags = VIR_PCI_CONNECT_TYPE_PCI_DEVICE;
-            break;
-        }
-        break;
-
-    case VIR_DOMAIN_DEVICE_VIDEO:
-        /* video cards aren't hot-plugged, and can be put in either a
-         * PCI or PCIe slot
-         */
-       flags = (VIR_PCI_CONNECT_TYPE_PCI_DEVICE
-                | VIR_PCI_CONNECT_TYPE_PCIE_DEVICE);
-        break;
-    }
-
     /* Ignore implicit controllers on slot 0:0:1.0:
      * implicit IDE controller on 0:0:1.1 (no qemu command line)
      * implicit USB controller on 0:0:1.2 (-usb)
@@ -927,7 +858,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)
         goto cleanup;
 
@@ -1083,8 +1014,7 @@ qemuDomainValidateDevicePCISlotsPIIX3(virDomainDefPtr def,
             if (virDomainPCIAddressSlotInUse(addrs, &tmp_addr)) {
                 if (qemuDeviceVideoUsable) {
                     if (qemuDomainPCIAddressReserveNextSlot(addrs,
-                                                            &primaryVideo->info,
-                                                            flags) < 0) {
+                                                            &primaryVideo->info) < 0) {
                         goto cleanup;
                     }
                 } else {
@@ -1273,8 +1203,7 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def,
             if (virDomainPCIAddressSlotInUse(addrs, &tmp_addr)) {
                 if (qemuDeviceVideoUsable) {
                     if (qemuDomainPCIAddressReserveNextSlot(addrs,
-                                                           &primaryVideo->info,
-                                                           flags) < 0)
+                                                            &primaryVideo->info) < 0)
                         goto cleanup;
                 } else {
                     virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
@@ -1395,7 +1324,6 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def,
                                virDomainPCIAddressSetPtr addrs)
 {
     size_t i, j;
-    virDomainPCIConnectFlags flags = 0; /* initialize to quiet gcc warning */
 
     /* PCI controllers */
     for (i = 0; i < def->ncontrollers; i++) {
@@ -1409,33 +1337,18 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def,
                 !virDeviceInfoPCIAddressWanted(&cont->info))
                 continue;
 
-            /* convert the type of controller into a "CONNECT_TYPE"
-             * flag to use when searching for the proper
-             * controller/bus to connect it to on the upstream side.
-             */
-            flags = virDomainPCIControllerModelToConnectType(model);
-            if (qemuDomainPCIAddressReserveNextSlot(addrs,
-                                                    &cont->info, flags) < 0) {
+            if (qemuDomainPCIAddressReserveNextSlot(addrs, &cont->info) < 0)
                 goto error;
-            }
         }
     }
 
-    /* all other devices that plug into a PCI slot are treated as a
-     * PCI endpoint devices that require a hotplug-capable slot
-     * (except for some special cases which have specific handling
-     * below)
-     */
-    flags = VIR_PCI_CONNECT_HOTPLUGGABLE | VIR_PCI_CONNECT_TYPE_PCI_DEVICE;
-
     for (i = 0; i < def->nfss; i++) {
         if (!virDeviceInfoPCIAddressWanted(&def->fss[i]->info))
             continue;
 
         /* Only support VirtIO-9p-pci so far. If that changes,
          * we might need to skip devices here */
-        if (qemuDomainPCIAddressReserveNextSlot(addrs, &def->fss[i]->info,
-                                                flags) < 0)
+        if (qemuDomainPCIAddressReserveNextSlot(addrs, &def->fss[i]->info) < 0)
             goto error;
     }
 
@@ -1451,7 +1364,8 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def,
             !virDeviceInfoPCIAddressWanted(&net->info)) {
             continue;
         }
-        if (qemuDomainPCIAddressReserveNextSlot(addrs, &net->info, flags) < 0)
+
+        if (qemuDomainPCIAddressReserveNextSlot(addrs, &net->info) < 0)
             goto error;
     }
 
@@ -1469,7 +1383,7 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def,
             continue;
         }
 
-        if (qemuDomainPCIAddressReserveNextSlot(addrs, &sound->info, flags) < 0)
+        if (qemuDomainPCIAddressReserveNextSlot(addrs, &sound->info) < 0)
             goto error;
     }
 
@@ -1536,7 +1450,8 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def,
 
             if (foundAddr) {
                 /* Reserve this function on the slot we found */
-                if (virDomainPCIAddressReserveAddr(addrs, &addr, flags,
+                if (virDomainPCIAddressReserveAddr(addrs, &addr,
+                                                   cont->info.pciConnectFlags,
                                                    false, true) < 0)
                     goto error;
 
@@ -1545,8 +1460,7 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def,
             } else {
                 /* This is the first part of the controller, so need
                  * to find a free slot & then reserve this function */
-                if (qemuDomainPCIAddressReserveNextAddr(addrs,
-                                                        &cont->info, flags,
+                if (qemuDomainPCIAddressReserveNextAddr(addrs, &cont->info,
                                                         addr.function,
                                                         false) < 0) {
                     goto error;
@@ -1555,10 +1469,8 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def,
                 cont->info.addr.pci.multi = addr.multi;
             }
         } else {
-            if (qemuDomainPCIAddressReserveNextSlot(addrs,
-                                                    &cont->info, flags) < 0) {
-                goto error;
-            }
+            if (qemuDomainPCIAddressReserveNextSlot(addrs, &cont->info) < 0)
+                 goto error;
         }
     }
 
@@ -1589,8 +1501,7 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def,
             goto error;
         }
 
-        if (qemuDomainPCIAddressReserveNextSlot(addrs, &def->disks[i]->info,
-                                                flags) < 0)
+        if (qemuDomainPCIAddressReserveNextSlot(addrs, &def->disks[i]->info) < 0)
             goto error;
     }
 
@@ -1602,8 +1513,8 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def,
             def->hostdevs[i]->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI)
             continue;
 
-        if (qemuDomainPCIAddressReserveNextSlot(addrs, def->hostdevs[i]->info,
-                                                flags) < 0)
+        if (qemuDomainPCIAddressReserveNextSlot(addrs,
+                                                def->hostdevs[i]->info) < 0)
             goto error;
     }
 
@@ -1612,8 +1523,8 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def,
         def->memballoon->model == VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO &&
         virDeviceInfoPCIAddressWanted(&def->memballoon->info)) {
 
-        if (qemuDomainPCIAddressReserveNextSlot(addrs, &def->memballoon->info,
-                                                flags) < 0)
+        if (qemuDomainPCIAddressReserveNextSlot(addrs,
+                                                &def->memballoon->info) < 0)
             goto error;
     }
 
@@ -1623,8 +1534,7 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def,
             !virDeviceInfoPCIAddressWanted(&def->rngs[i]->info))
             continue;
 
-        if (qemuDomainPCIAddressReserveNextSlot(addrs, &def->rngs[i]->info,
-                                                flags) < 0)
+        if (qemuDomainPCIAddressReserveNextSlot(addrs, &def->rngs[i]->info) < 0)
             goto error;
     }
 
@@ -1632,8 +1542,7 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def,
     if (def->watchdog &&
         def->watchdog->model == VIR_DOMAIN_WATCHDOG_MODEL_I6300ESB &&
         virDeviceInfoPCIAddressWanted(&def->watchdog->info)) {
-        if (qemuDomainPCIAddressReserveNextSlot(addrs, &def->watchdog->info,
-                                                flags) < 0)
+        if (qemuDomainPCIAddressReserveNextSlot(addrs, &def->watchdog->info) < 0)
             goto error;
     }
 
@@ -1641,16 +1550,15 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def,
      * assigned address. */
     if (def->nvideos > 0 &&
         virDeviceInfoPCIAddressWanted(&def->videos[0]->info)) {
-        if (qemuDomainPCIAddressReserveNextSlot(addrs, &def->videos[0]->info,
-                                                flags) < 0)
+        if (qemuDomainPCIAddressReserveNextSlot(addrs, &def->videos[0]->info) < 0)
             goto error;
     }
 
     for (i = 1; i < def->nvideos; i++) {
         if (!virDeviceInfoPCIAddressWanted(&def->videos[i]->info))
             continue;
-        if (qemuDomainPCIAddressReserveNextSlot(addrs, &def->videos[i]->info,
-                                                flags) < 0)
+
+        if (qemuDomainPCIAddressReserveNextSlot(addrs, &def->videos[i]->info) < 0)
             goto error;
     }
 
@@ -1659,8 +1567,7 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def,
         if (!virDeviceInfoPCIAddressWanted(&def->shmems[i]->info))
             continue;
 
-        if (qemuDomainPCIAddressReserveNextSlot(addrs, &def->shmems[i]->info,
-                                                flags) < 0)
+        if (qemuDomainPCIAddressReserveNextSlot(addrs, &def->shmems[i]->info) < 0)
             goto error;
     }
     for (i = 0; i < def->ninputs; i++) {
@@ -1668,8 +1575,7 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def,
             !virDeviceInfoPCIAddressWanted(&def->inputs[i]->info))
             continue;
 
-        if (qemuDomainPCIAddressReserveNextSlot(addrs, &def->inputs[i]->info,
-                                                flags) < 0)
+        if (qemuDomainPCIAddressReserveNextSlot(addrs, &def->inputs[i]->info) < 0)
             goto error;
     }
     for (i = 0; i < def->nparallels; i++) {
@@ -1682,7 +1588,7 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def,
             !virDeviceInfoPCIAddressWanted(&chr->info))
             continue;
 
-        if (qemuDomainPCIAddressReserveNextSlot(addrs, &chr->info, flags) < 0)
+        if (qemuDomainPCIAddressReserveNextSlot(addrs, &chr->info) < 0)
             goto error;
     }
     for (i = 0; i < def->nchannels; i++) {
@@ -1839,8 +1745,6 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
     int rv;
     bool buses_reserved = true;
 
-    virDomainPCIConnectFlags flags = VIR_PCI_CONNECT_TYPE_PCI_DEVICE;
-
     for (i = 0; i < def->ncontrollers; i++) {
         virDomainControllerDefPtr cont = def->controllers[i];
 
@@ -1852,10 +1756,23 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
 
     nbuses = max_idx + 1;
 
+    /* set the connect type flags (pci vs. pcie) in the DeviceInfo
+     * of all devices. This will be used to pick an appropriate
+     * bus when assigning addresses.
+     */
+    if (qemuDomainFillAllPCIConnectFlags(def, qemuCaps) < 0)
+        goto cleanup;
+
     if (nbuses > 0 &&
         virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCI_BRIDGE)) {
+        /* This is a dummy info used to reserve a slot for a legacy
+         * PCI device that doesn't exist, but may in the future, e.g.
+         * if another device is hotplugged into the domain.
+         */
         virDomainDeviceInfo info;
 
+        info.pciConnectFlags = VIR_PCI_CONNECT_TYPE_PCI_DEVICE;
+
         /* 1st pass to figure out how many PCI bridges we need */
         if (!(addrs = qemuDomainPCIAddressSetCreate(def, nbuses, true)))
             goto cleanup;
@@ -1879,24 +1796,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 */
+
+            /* 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;