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