Re: [PATCH v2 2/4] qemu: assign virtio devices to PCIe slot when appropriate

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

 



On Mon, 2016-08-15 at 01:50 -0400, Laine Stump wrote:
> libvirt previously assigned nearly all devices to a hotpluggable
> legacy PCI slot even on machines with a PCIe root complex. Doing this
> means that the domain will need a dmi-to-pci-bridge (to convert from
> PCIe to legacy PCI) and a pci-bridge (to provide hotpluggable legacy
> PCI slots.
> 
> To help reduce the need for these legacy controllers, this patch
> checks for the QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY capability (if that
> capability is present, then all virtio devices will automatically
> present as PCIe when attached to a PCIe controller, or PCI when
> attached to a legacy PCI controller), and assigns virtio devices to a
> hotpluggable PCIe slot instead.
> 
> NB: since the slot must be hotpluggable, and pcie-root (the PCIe root
> complex) does *not* support hotplug, this means that suitable
> controllers must also be in the config (i.e. either pcie-root-port, or
> pcie-downstream-port). For now, libvirt doesn't add those
> automatically, so if you put virtio devices in a config for a qemu
> that has PCIe-capable virtio devices, you'll need to add extra
> pcie-root-ports yourself. That requirement will be eliminated in a
> future patch, but for now, it's simple to do this:
> 
>    <controller type='pci' model='pcie-root-port'/>
>    <controller type='pci' model='pcie-root-port'/>
>    <controller type='pci' model='pcie-root-port'/>
>    ...
> 
> Partially Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1330024
> ---

[...]

> @@ -1021,17 +1028,22 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def,
>          }
>      }
>  
> -    /* 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)
> +    pciFlags  = VIR_PCI_CONNECT_HOTPLUGGABLE | VIR_PCI_CONNECT_TYPE_PCI_DEVICE;
> +    pcieFlags = VIR_PCI_CONNECT_HOTPLUGGABLE | VIR_PCI_CONNECT_TYPE_PCIE_DEVICE;

Blank line here.

> +    /* if qemu has the disable-legacy option for
> +     * virtio-net, then its virtio devices will present
> +     * themselves as PCIe devices when plugged into a PCIe
> +     * slot, so we can safely assign them to a PCIe slot.
>       */
> -    flags = VIR_PCI_CONNECT_HOTPLUGGABLE | VIR_PCI_CONNECT_TYPE_PCI_DEVICE;
> +    virtioFlags = havePCIeRoot &&
> +        virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY) ?
> +        pcieFlags : pciFlags;

How about unpacking that? I feel like

    if (havePCIeRoot &&
        virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY)) {
        virtioFlags = pcieFlags;
    } else {
        virtioFlags = pciFlags;
    }

would be more readable and maintainable.

>      for (i = 0; i < def->nfss; i++) {
>          if (!virDeviceInfoPCIAddressWanted(&def->fss[i]->info))
>              continue;
>  
> +        flags = virtioFlags;

Blank line here.

>          /* Only support VirtIO-9p-pci so far. If that changes,
>           * we might need to skip devices here */
>          if (virDomainPCIAddressReserveNextSlot(addrs, &def->fss[i]->info,
> @@ -1045,12 +1057,18 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def,
>           * in hostdevs list anyway, so handle them with other hostdevs
>           * instead of here.
>           */
> -        if ((def->nets[i]->type == VIR_DOMAIN_NET_TYPE_HOSTDEV) ||
> -            !virDeviceInfoPCIAddressWanted(&def->nets[i]->info)) {
> +        virDomainNetDefPtr net = def->nets[i];
> +
> +        if ((net->type == VIR_DOMAIN_NET_TYPE_HOSTDEV) ||
> +            !virDeviceInfoPCIAddressWanted(&net->info)) {
>              continue;
>          }
> -        if (virDomainPCIAddressReserveNextSlot(addrs, &def->nets[i]->info,
> -                                               flags) < 0)

Blank line here.

> +        if (STREQ(net->model, "virtio"))
> +            flags = virtioFlags;
> +        else
> +            flags = pciFlags;

It's kind of insane that we don't have a virDomainNetModel
enum for this sort of check, isn't it?

> +
> +        if (virDomainPCIAddressReserveNextSlot(addrs, &net->info, flags) < 0)
>              goto error;
>      }
>  
> @@ -1064,6 +1082,7 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def,
>              def->sounds[i]->model == VIR_DOMAIN_SOUND_MODEL_USB)
>              continue;
>  
> +        flags = pciFlags;
>          if (virDomainPCIAddressReserveNextSlot(addrs, &def->sounds[i]->info,
>                                                 flags) < 0)
>              goto error;
> @@ -1094,6 +1113,7 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def,
>          if (!virDeviceInfoPCIAddressWanted(&def->controllers[i]->info))
>              continue;
>  
> +        flags = pciFlags;

Blank line here.

>          /* USB2 needs special handling to put all companions in the same slot */
>          if (IS_USB2_CONTROLLER(def->controllers[i])) {
>              virPCIDeviceAddress addr = { 0, 0, 0, 0, false };
> @@ -1150,6 +1170,12 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def,
>              def->controllers[i]->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
>              def->controllers[i]->info.addr.pci = addr;
>          } else {
> +            if ((def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI &&
> +                 def->controllers[i]->model ==
> +                 VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI) ||
> +                (def->controllers[i]->type ==
> +                 VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL))
> +                flags = virtioFlags;

This is one of those times I really wish our coding style
guidelines allowed us to go past 80 columns :)

[...]

> @@ -1284,6 +1323,7 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def,
>              !virDeviceInfoPCIAddressWanted(&chr->info))
>              continue;
>  
> +        flags = pciFlags;
>          if (virDomainPCIAddressReserveNextSlot(addrs, &chr->info, flags) < 0)
>              goto error;
>      }

Having gotten this far, I wonder if the pattern used here
couldn't be semplified a bit... Try squashing in the attached
patch, see if you like it.

> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35-virtio-pci.xml b/tests/qemuxml2argvdata/qemuxml2argv-q35-virtio-pci.xml
> new file mode 100644
> index 0000000..7bed08c
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-q35-virtio-pci.xml

As you mention in the test programs below,
qemuxml2argv-q35-virtio-pci.xml and qemuxml2argv-q35-pcie.xml
have the exact same contents.

Please make one of them a symbolic link to the other one, to
save space and ensure they will remain in sync.

> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35-virtio-pcie.args b/tests/qemuxml2argvdata/qemuxml2argv-q35-virtio-pcie.args
> new file mode 100644
> index 0000000..c43c537
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-q35-virtio-pcie.args

This looks like a leftover from a previous attempt, and in
fact 'make check' still passes after deleting it.

> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index ad0693f..46b602f 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -607,7 +607,6 @@ mymain(void)
>      unsetenv("QEMU_AUDIO_DRV");
>      unsetenv("SDL_AUDIODRIVER");
>  
> -    DO_TEST("minimal", NONE);

No reason to delete this AFAICT.

>      DO_TEST_PARSE_ERROR("minimal-no-memory", NONE);
>      DO_TEST("minimal-msg-timestamp", QEMU_CAPS_MSG_TIMESTAMP);
>      DO_TEST("machine-aliases1", NONE);
> @@ -1670,6 +1669,48 @@ mymain(void)
>              QEMU_CAPS_PCI_MULTIFUNCTION, QEMU_CAPS_ICH9_USB_EHCI1,
>              QEMU_CAPS_DEVICE_VIDEO_PRIMARY,
>              QEMU_CAPS_VGA_QXL, QEMU_CAPS_DEVICE_QXL);
> +    DO_TEST("q35-pcie",
> +            QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY,
> +            QEMU_CAPS_DEVICE_VIRTIO_RNG,
> +            QEMU_CAPS_OBJECT_RNG_RANDOM,
> +            QEMU_CAPS_NETDEV,
> +            QEMU_CAPS_DEVICE_VIRTIO_NET,
> +            QEMU_CAPS_DEVICE_VIRTIO_GPU,
> +            QEMU_CAPS_DEVICE_VIRTIO_GPU_VIRGL,
> +            QEMU_CAPS_VIRTIO_KEYBOARD,
> +            QEMU_CAPS_VIRTIO_MOUSE,
> +            QEMU_CAPS_VIRTIO_TABLET,
> +            QEMU_CAPS_VIRTIO_INPUT_HOST,
> +            QEMU_CAPS_VIRTIO_SCSI,
> +            QEMU_CAPS_FSDEV,
> +            QEMU_CAPS_FSDEV_WRITEOUT,
> +            QEMU_CAPS_DEVICE_PCI_BRIDGE,
> +            QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
> +            QEMU_CAPS_DEVICE_IOH3420,
> +            QEMU_CAPS_ICH9_AHCI,
> +            QEMU_CAPS_PCI_MULTIFUNCTION, QEMU_CAPS_ICH9_USB_EHCI1,
> +            QEMU_CAPS_DEVICE_VIDEO_PRIMARY);
> +    /* same XML as q35-pcie, but don't set QEMU_CAPS_VIRTIO_PCI_LEGACY */

This is really nice. We have a bunch of tests where the name
is not that helpful in conveying exactly what they're supposed
to be testing... Actually, adding a few words for q35-pcie
would be even nicer! :)

> +    DO_TEST("q35-virtio-pci",
> +            QEMU_CAPS_DEVICE_VIRTIO_RNG,
> +            QEMU_CAPS_OBJECT_RNG_RANDOM,
> +            QEMU_CAPS_NETDEV,
> +            QEMU_CAPS_DEVICE_VIRTIO_NET,
> +            QEMU_CAPS_DEVICE_VIRTIO_GPU,
> +            QEMU_CAPS_DEVICE_VIRTIO_GPU_VIRGL,
> +            QEMU_CAPS_VIRTIO_KEYBOARD,
> +            QEMU_CAPS_VIRTIO_MOUSE,
> +            QEMU_CAPS_VIRTIO_TABLET,
> +            QEMU_CAPS_VIRTIO_INPUT_HOST,
> +            QEMU_CAPS_VIRTIO_SCSI,
> +            QEMU_CAPS_FSDEV,
> +            QEMU_CAPS_FSDEV_WRITEOUT,
> +            QEMU_CAPS_DEVICE_PCI_BRIDGE,
> +            QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
> +            QEMU_CAPS_DEVICE_IOH3420,
> +            QEMU_CAPS_ICH9_AHCI,
> +            QEMU_CAPS_PCI_MULTIFUNCTION, QEMU_CAPS_ICH9_USB_EHCI1,
> +            QEMU_CAPS_DEVICE_VIDEO_PRIMARY);
>      DO_TEST("pcie-root-port",
>              QEMU_CAPS_DEVICE_PCI_BRIDGE,
>              QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,

I didn't go through the test suite additions in detail,
because when I tried I got cross-eyed very quickly. Plus,
I assume you already verified the output of the tests
make sense. I'll give it a closer look in the respin.

-- 
Andrea Bolognani / Red Hat / Virtualization
From cf9ca9693fa72e444815b6226b942489da2dd9dd Mon Sep 17 00:00:00 2001
From: Andrea Bolognani <abologna@xxxxxxxxxx>
Date: Tue, 16 Aug 2016 18:18:03 +0200
Subject: [PATCH] qemu: Simplify virDomainPCIConnectFlags handling

---
 src/qemu/qemu_domain_address.c | 62 ++++++++++++++++++++++--------------------
 1 file changed, 32 insertions(+), 30 deletions(-)

diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index 49181d1..108c311 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -996,7 +996,6 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def,
                                virDomainPCIAddressSetPtr addrs)
 {
     size_t i, j;
-    virDomainPCIConnectFlags flags = 0; /* initialize to quiet gcc warning */
     virDomainPCIConnectFlags virtioFlags;
     virDomainPCIConnectFlags pciFlags;
     virDomainPCIConnectFlags pcieFlags;
@@ -1007,6 +1006,9 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def,
     for (i = 0; i < def->ncontrollers; i++) {
         if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) {
             virDomainControllerModelPCI model = def->controllers[i]->model;
+            virDomainPCIConnectFlags flags;
+
+            flags = virDomainPCIControllerModelToConnectType(model);
 
             if (model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) {
                 havePCIeRoot = true;
@@ -1020,7 +1022,6 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def,
              * flag to use when searching for the proper
              * controller/bus to connect it to on the upstream side.
              */
-            flags = virDomainPCIControllerModelToConnectType(model);
             if (virDomainPCIAddressReserveNextSlot(addrs,
                                                    &def->controllers[i]->info,
                                                    flags) < 0)
@@ -1043,11 +1044,10 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def,
         if (!virDeviceInfoPCIAddressWanted(&def->fss[i]->info))
             continue;
 
-        flags = virtioFlags;
         /* Only support VirtIO-9p-pci so far. If that changes,
          * we might need to skip devices here */
         if (virDomainPCIAddressReserveNextSlot(addrs, &def->fss[i]->info,
-                                               flags) < 0)
+                                               virtioFlags) < 0)
             goto error;
     }
 
@@ -1058,6 +1058,7 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def,
          * instead of here.
          */
         virDomainNetDefPtr net = def->nets[i];
+        virDomainPCIConnectFlags flags;
 
         if ((net->type == VIR_DOMAIN_NET_TYPE_HOSTDEV) ||
             !virDeviceInfoPCIAddressWanted(&net->info)) {
@@ -1082,9 +1083,8 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def,
             def->sounds[i]->model == VIR_DOMAIN_SOUND_MODEL_USB)
             continue;
 
-        flags = pciFlags;
         if (virDomainPCIAddressReserveNextSlot(addrs, &def->sounds[i]->info,
-                                               flags) < 0)
+                                               pciFlags) < 0)
             goto error;
     }
 
@@ -1113,7 +1113,6 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def,
         if (!virDeviceInfoPCIAddressWanted(&def->controllers[i]->info))
             continue;
 
-        flags = pciFlags;
         /* USB2 needs special handling to put all companions in the same slot */
         if (IS_USB2_CONTROLLER(def->controllers[i])) {
             virPCIDeviceAddress addr = { 0, 0, 0, 0, false };
@@ -1152,7 +1151,8 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def,
             if (!foundAddr) {
                 /* This is the first part of the controller, so need
                  * to find a free slot & then reserve a function */
-                if (virDomainPCIAddressGetNextSlot(addrs, &tmp_addr, flags) < 0)
+                if (virDomainPCIAddressGetNextSlot(addrs, &tmp_addr,
+                                                   pciFlags) < 0)
                     goto error;
 
                 addr.bus = tmp_addr.bus;
@@ -1163,19 +1163,24 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def,
                 addrs->lastaddr.multi = VIR_TRISTATE_SWITCH_ABSENT;
             }
             /* Finally we can reserve the slot+function */
-            if (virDomainPCIAddressReserveAddr(addrs, &addr, flags,
+            if (virDomainPCIAddressReserveAddr(addrs, &addr, pciFlags,
                                                false, foundAddr) < 0)
                 goto error;
 
             def->controllers[i]->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
             def->controllers[i]->info.addr.pci = addr;
         } else {
+            virDomainPCIConnectFlags flags;
+
             if ((def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI &&
                  def->controllers[i]->model ==
                  VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI) ||
                 (def->controllers[i]->type ==
                  VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL))
                 flags = virtioFlags;
+            else
+                flags = pciFlags;
+
             if (virDomainPCIAddressReserveNextSlot(addrs,
                                                    &def->controllers[i]->info,
                                                    flags) < 0)
@@ -1210,9 +1215,8 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def,
             goto error;
         }
 
-        flags = virtioFlags;
         if (virDomainPCIAddressReserveNextSlot(addrs, &def->disks[i]->info,
-                                               flags) < 0)
+                                               virtioFlags) < 0)
             goto error;
     }
 
@@ -1224,10 +1228,9 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def,
             def->hostdevs[i]->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI)
             continue;
 
-        flags = pciFlags;
         if (virDomainPCIAddressReserveNextSlot(addrs,
                                                def->hostdevs[i]->info,
-                                               flags) < 0)
+                                               pciFlags) < 0)
             goto error;
     }
 
@@ -1235,10 +1238,10 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def,
     if (def->memballoon &&
         def->memballoon->model == VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO &&
         virDeviceInfoPCIAddressWanted(&def->memballoon->info)) {
-        flags = virtioFlags;
+
         if (virDomainPCIAddressReserveNextSlot(addrs,
                                                &def->memballoon->info,
-                                               flags) < 0)
+                                               virtioFlags) < 0)
             goto error;
     }
 
@@ -1248,9 +1251,8 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def,
             !virDeviceInfoPCIAddressWanted(&def->rngs[i]->info))
             continue;
 
-        flags = virtioFlags;
-        if (virDomainPCIAddressReserveNextSlot(addrs,
-                                               &def->rngs[i]->info, flags) < 0)
+        if (virDomainPCIAddressReserveNextSlot(addrs, &def->rngs[i]->info,
+                                               virtioFlags) < 0)
             goto error;
     }
 
@@ -1258,9 +1260,9 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def,
     if (def->watchdog &&
         def->watchdog->model == VIR_DOMAIN_WATCHDOG_MODEL_I6300ESB &&
         virDeviceInfoPCIAddressWanted(&def->watchdog->info)) {
-        flags = pciFlags;
+
         if (virDomainPCIAddressReserveNextSlot(addrs, &def->watchdog->info,
-                                               flags) < 0)
+                                               pciFlags) < 0)
             goto error;
     }
 
@@ -1268,10 +1270,13 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def,
      * assigned address. */
     if (def->nvideos > 0 &&
         virDeviceInfoPCIAddressWanted(&def->videos[0]->info)) {
+        virDomainPCIConnectFlags flags;
+
         if (def->videos[0]->type == VIR_DOMAIN_VIDEO_TYPE_VIRTIO)
             flags = virtioFlags;
         else
             flags = pciFlags;
+
         if (virDomainPCIAddressReserveNextSlot(addrs, &def->videos[0]->info,
                                                flags) < 0)
             goto error;
@@ -1287,9 +1292,8 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def,
         if (!virDeviceInfoPCIAddressWanted(&def->videos[i]->info))
             continue;
 
-        flags = pciFlags;
         if (virDomainPCIAddressReserveNextSlot(addrs, &def->videos[i]->info,
-                                               flags) < 0)
+                                               pciFlags) < 0)
             goto error;
     }
 
@@ -1298,9 +1302,8 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def,
         if (!virDeviceInfoPCIAddressWanted(&def->shmems[i]->info))
             continue;
 
-        flags = pciFlags;
-        if (virDomainPCIAddressReserveNextSlot(addrs,
-                                               &def->shmems[i]->info, flags) < 0)
+        if (virDomainPCIAddressReserveNextSlot(addrs, &def->shmems[i]->info,
+                                               pciFlags) < 0)
             goto error;
     }
     for (i = 0; i < def->ninputs; i++) {
@@ -1308,9 +1311,8 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def,
             !virDeviceInfoPCIAddressWanted(&def->inputs[i]->info))
             continue;
 
-        flags = virtioFlags;
-        if (virDomainPCIAddressReserveNextSlot(addrs,
-                                               &def->inputs[i]->info, flags) < 0)
+        if (virDomainPCIAddressReserveNextSlot(addrs, &def->inputs[i]->info,
+                                               virtioFlags) < 0)
             goto error;
     }
     for (i = 0; i < def->nparallels; i++) {
@@ -1323,8 +1325,8 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def,
             !virDeviceInfoPCIAddressWanted(&chr->info))
             continue;
 
-        flags = pciFlags;
-        if (virDomainPCIAddressReserveNextSlot(addrs, &chr->info, flags) < 0)
+        if (virDomainPCIAddressReserveNextSlot(addrs, &chr->info,
+                                               pciFlags) < 0)
             goto error;
     }
     for (i = 0; i < def->nchannels; i++) {
-- 
2.7.4

--
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]