This change introduces libvirt xml support to enable/disable hotplug on the pci-root controller. It adds a 'target' subelement for the pci-root controller with a 'hotplug' property. This property can be used to enable or disable hotplug for the pci-root controller. For example, in order to disable hotplug on the pci-root controller, one has to use set '<target hotplug='off'>' as shown below: <controller type='pci' model='pci-root'> <target hotplug='off'/> </controller> '<target hotplug='on'>' option would enable hotplug for pci-root controller. This is also the default value. This option is only available for pc machine types and is applicable for qemu only/kvm accelerator onlt.This feature was introduced from qemu version 5.2 with the following change in qemu repository: 3d7e78aa7777f ("Introduce a new flag for i440fx to disable PCI hotplug on the root bus") The above qemu commit describes some reasons why users might to disable hotplug on PCI root buses. Related unit tests to exercise the new conf option has also been added. Signed-off-by: Ani Sinha <ani(a)anisinha.ca&gt; --- docs/formatdomain.rst | 12 ++++---- docs/schemas/domaincommon.rng | 10 +++++++ src/qemu/qemu_validate.c | 9 +++++- .../pc-i440fx-acpi-root-hotplug-disable.xml | 17 +++++++++++ .../pc-i440fx-acpi-root-hotplug-enable.xml | 17 +++++++++++ .../pc-i440fx-acpi-root-hotplug-disable.xml | 30 +++++++++++++++++++ tests/qemuxml2xmltest.c | 2 ++ 7 files changed, 91 insertions(+), 6 deletions(-) create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.xml create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-enable.xml create mode 100644 tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-disable.xml diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index 0f5d833521..e2a1768ba7 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -3776,11 +3776,13 @@ generated by libvirt. :since:`Since 1.2.19 (QEMU only).` controller's "port" configuration value, which is visible to the virtual machine. If set, port must be between 0 and 255. ``hotplug`` - pcie-root-port and pcie-switch-downstream-port controllers can also have a - ``hotplug`` attribute in the ``<target>`` subelement, which is used to - disable hotplug/unplug of devices on a particular controller. The default - setting of ``hotplug`` is ``on``; it should be set to ``off`` to disable - hotplug/unplug of devices on a particular controller. :since:`Since 6.3.0` + pci-root (:since:`Since 7.8.0`), pcie-root-port (:since:`Since 6.3.0`) and + pcie-switch-downstream-port controllers (:since:`Since 6.3.0`) can + also have a ``hotplug`` attribute in the ``<target>`` subelement, which is + used to disable hotplug/unplug of devices on a particular controller. The + default setting of ``hotplug`` is ``on``; it should be set to ``off`` to + disable hotplug/unplug of devices on a particular controller. + ``busNr`` pci-expander-bus and pcie-expander-bus controllers can have an optional ``busNr`` attribute (1-254). This will be the bus number of the new bus; All diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index fdc04f90aa..cce50c9c89 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2668,6 +2668,16 @@ <ref name="scaledInteger"/> </element> </optional> + <!-- pci-root controller has an optional element "target" --> + <optional> + <element name="target"> + <optional> + <attribute name="hotplug"> + <ref name="virOnOff"/> + </attribute> + </optional> + </element> + </optional> </group> <group> <attribute name="model"> diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 13fbfd01b2..510e766cfd 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -3879,6 +3879,14 @@ qemuValidateDomainDeviceDefControllerPCI(const virDomainControllerDef *cont, /* hotplug */ if (pciopts->hotplug != VIR_TRISTATE_SWITCH_ABSENT) { switch ((virDomainControllerModelPCI) cont->model) { + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("setting the %s property on a pci-root device is not supported by this QEMU binary"), + "hotplug"); + return -1; + } + break; case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT: case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT: if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCIE_ROOT_PORT_HOTPLUG)) { @@ -3889,7 +3897,6 @@ qemuValidateDomainDeviceDefControllerPCI(const virDomainControllerDef *cont, } break; - case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: diff --git a/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.xml b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.xml new file mode 100644 index 0000000000..f6a4c4b16a --- /dev/null +++ b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.xml @@ -0,0 +1,17 @@ +<domain type='qemu'> + <name>i440fx</name> + <uuid>56f5055c-1b8d-490c-844a-ad646a1caaaa</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc-i440fx-2.5'>hvm</type> + <boot dev='network'/> + </os> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='pci' model='pci-root'> + <target hotplug='off'/> + </controller> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-enable.xml b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-enable.xml new file mode 100644 index 0000000000..90a568cd9b --- /dev/null +++ b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-enable.xml @@ -0,0 +1,17 @@ +<domain type='qemu'> + <name>i440fx</name> + <uuid>56f5055c-1b8d-490c-844a-ad646a1caaaa</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc-i440fx-2.5'>hvm</type> + <boot dev='network'/> + </os> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='pci' model='pci-root'> + <target hotplug='on'/> + </controller> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-disable.xml b/tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-disable.xml new file mode 100644 index 0000000000..93f2779f68 --- /dev/null +++ b/tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-disable.xml @@ -0,0 +1,30 @@ +<domain type='qemu'> + <name>i440fx</name> + <uuid>56f5055c-1b8d-490c-844a-ad646a1caaaa</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc-i440fx-2.5'>hvm</type> + <boot dev='network'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='pci' index='0' model='pci-root'> + <target hotplug='off'/> + </controller> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <audio id='1' type='none'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 49b291fadb..ef45e4d9f1 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -424,6 +424,8 @@ mymain(void) DO_TEST_NOCAPS("input-usbtablet"); DO_TEST_NOCAPS("misc-acpi"); DO_TEST("misc-disable-s3", QEMU_CAPS_PIIX_DISABLE_S3); + DO_TEST("pc-i440fx-acpi-root-hotplug-disable", + QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG); DO_TEST("misc-disable-suspends", QEMU_CAPS_PIIX_DISABLE_S3, QEMU_CAPS_PIIX_DISABLE_S4); -- 2.25.1

On Sun, 26 Sep 2021, Laine Stump wrote: There is value in hacing input and output xmls as different. It also tests if libvirt is properly populating the other devices in output xmls that were not in input xmls. In any case, in v5 I have added the symlinks as you have suggested. All done in v5.

On 9/26/21 10:35 AM, Ani Sinha wrote: It would make more sense to include this in the commandline generator for PCI controllers, since that's where all other XML for PCI controllers is converted into a qemu commandline argument. That's a bit convoluted though, unfortunately. The loop going through all the controllers is in qemuBuildControllersByTypeCommandLine() which then calls qemuBuildControllerDevStr() and *that* is the function that builds the argument to the -device for each controller. For two reasons, we can't add the code for this new option alongside the other PCI controller commandline generation code in qemuBuildControllerDevStr() though: 1) the output of qemuBuildControllerDevStr() is assumed to be following a "-device" argument on the commandline, so we would end up with: "-device -global PIIX4_PM.acpi-root-pci-hotplug=off" which is *not* what we want. 2) the top of the loop in qemuBuildControllersByTypeCommandLine() calls qemuBuildSkipController(), which tells the loop to skip generating any commandline for a pci-root controller (unless it's a P-series guest and the index is non-0). So the only way to make this work is to add a special case to qemuBuildControllersByTypeCommandLine() *before* the call to qemuBuildSkipController() - if the type is pci, model is pci-root, and index is 0 then conditionally add "-global", "PIIX4...." and continue (skip the rest of the body of the loop). As with what you've already done here, this is also not 100% clean and consistent with the rest of the code, but since neither method is perfect I think putting it in the function that generates all the rest of the commandline args associated with PCI controllers is more logical and easier to follow. (If anyone disagrees and thinks that the commandline for this should be generated in the place this patch already does it, please speak up!) Are these three caps ^^^ actually needed? I hope not, because that would indicate something seriously wrong with the code (those caps are related to PCIe controllers, and this option only applies to machinetypes that use conventional pci-root). Ah, now I understand the origin of pc-i440fx-acpi-root-hotplug-enable.xml in the previous patch - you had created it to be test case for this negative test. Actually I think the negative test should be done using the ...-disable test case with no caps; in the case that someone has "hotplug='on'" and the option is missing, I think we should just ignore it, since "hotplug='on'" is what they're going to get anyway (that could be added into the validation that was added in the previous patch). You still you add the -enable test case to qemuxml2xmltest.c as I suggested in the previous patch.