Re: [PATCH v3 09/18] qemu: set/use info->pciConnectFlags during qemuDomainAssignDevicePCISlots

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 10/04/2016 10:11 AM, Andrea Bolognani wrote:
On Tue, 2016-09-20 at 15:14 -0400, Laine Stump wrote:
Set pciConnectFlags in each device's DeviceInfo prior to assigning PCI
addresses, and then use those flags later when actually assigning the
addresses with qemuDomainPCIAddressReserveNextAddr() (rather than
scattering the logic about which devices need which type of slot all
over the place).
---
   src/qemu/qemu_domain_address.c | 234 ++++++++++++++++++-----------------------
   1 file changed, 104 insertions(+), 130 deletions(-)
[...]
@@ -686,9 +685,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);
This hunk you're removing seems to support my comment in
patch 4, the one arguing against adding HOTPLUGGABLE at the
end of the function.

       if (!virDeviceInfoPCIAddressPresent(info) ||
           ((device->type == VIR_DOMAIN_DEVICE_HOSTDEV) &&
@@ -700,69 +696,25 @@ qemuDomainCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED,
           return 0;
       }
- /* Change flags according to differing requirements of different
-     * devices.
+    /* If we get to here, the device has a PCI address assigned in the
+     * config and we should mark it as in-use. But if the
+     * pciConnectFlags are 0, then this device shouldn't have a PCI
+     * address associated with it. *BUT* since there are cases in the
+     * past where we've apparently allowed that, we need to pretend
+     * for now that it's okay, otherwise an existing domain could
+     * "disappear" from the list of domains due to a parse failure. We
+     * can fix this by just forcing the pciConnectFlags to be
+     * PCI_DEVICE (and then relying on validation functions to report
+     * inappropriate address types.
        */
-    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);
The new code doesn't seem to have any special handling of
CONTROLLER_TYPE_SATA, which means it will end up as
PCI_DEVICE|HOTPLUGGABLE instead of PCI_DEVICE|PCIE_DEVICE.

All of the flag settings in this function are essentially pointless, and *not* what is used when actually assigning an address to a device (it took me a while to realize this). There is a fuller explanation below, but in short, when we are examining device addresses that were manually set (or auto-assigned at some earlier time), any combination of the following flags is considered equivalent:

      VIR_PCI_CONNECT_TYPE_PCI_DEVICE
      VIR_PCI_CONNECT_TYPE_PCIE_DEVICE
      VIR_PCI_CONNECT_HOTPLUGGABLE

so all of the flags settings in this function are equivalent to PCI_DEVICE | HOTPLUGGABLE.


-            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;
All these will be HOTPLUGGABLE.

^^ e.g. this


-                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);
This will be PCI_DEVICE|HOTPLUGGABLE instead of
PCI_DEVICE|PCIE_DEVICE.

^^ and this. etc.


-                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;
All these will be HOTPLUGGABLE.

-            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);
These will be hotpluggable and PCI only.

All these differences between the code you're removing and
the code that's supposed to replace it make me wonder whether
I'm missing something. Feel free to point out my mistake
right about... Now! :)

I believed you were onto something, so I modified the new function (qemuDomainDeviceCalculatePCIConnectFlags()) to exactly reflect the flags as they're set in this function (qemuDomainCollectPCIAddress()). But that caused some unit tests to fail. That's when I realized that the flag settings in qemuDomainCollectPCIAddress()) are only used when checking manually set addresses ("fromConfig") as we're setting in-use bits for all the addresses already used, and in this case the setting of HOTPLUGGABLE, or PCI vs. PCIE doesn't is ignored. What is actually important is how the flags are set in qemuDomainAssignDevicePCISlots(), which uses "PCI_DEVICE | HOTPLUGGABLE" for *everything* except PCI controllers.

In the case of qemuDomainCollectPCIAddress(), since the addresses are coming "fromConfig", validation will succeed regardless of any mismatch in hotpluggable flag, or PCI vs PCIE (the address validation function allows you to break those rules when manually addressing, because qemu allows it).

So the proper thing for the new qemuDomainDeviceCalculatePCIConnectFlags() function (I've renamed it, but haven't yet reposted) is to set PCI_DEVICE | HOTPLUGGABLE for everything (until the patches that make exceptions for specific device/type/model combinations).

Note that one very nice side effect of this patch is that there is now only a single place where the PCI connect flags are calculated, rather than 2, so such confusion won't happen in the future.


On a related note, reviewing this would be *much* easier if
you could start off qemuDomainDeviceConnectFlagsInternal()
as (mostly) a straight copy of this chunk of code, and then
stray from it after switching over, instead of starting with
a completely different implementation.

Actually, not a copy of this chunk of code, because this chunk of code has settings that would break the other use of qemuDomainDeviceCalculatePCIConnectFlags() (see above).

I have rewritten (and renamed) the function, but its initial version now always sets the flags to PCI_DEVICE | HOTPLUGGABLE.


I also believe the original code is easier to follow, because
it looks more like a direct mapping from model to flags.

In all honesty, the original code is just plain pointless. It *tried* to be right, but got some things wrong, and then it ended up not mattering anyway because the hotpluggable flag, and PCI vs. PCIE, is ignored when validating already-assigned addresses anyway.

Ideally we would just list all devices and models, grouped by
purpose, and explicitly assign flags to them, in a similar
way we get flags for PCI controllers.

That's what I've done now. You'll see the results in a day or two.


And shouldn't the compiler give us at least a warning when
we're not considering all the possible values of an enum as
part of a switch()? That would be super-neat!

The jury is out.

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]