[PATCH v4 2/4] conf: introduce option to enable/disable pci hotplug on pci-root controller
Laine Stump
laine at redhat.com
Sun Sep 26 18:32:40 UTC 2021
On 9/26/21 10:35 AM, Ani Sinha wrote:
> 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 at anisinha.ca>
> ---
> 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">
This chunk ^^^^ is unnecessary, since the schema already allows for a
target/hotplug attribute for *all* PCI controller models (on line 2645),
so this new bit in the scheme has no effect.
(qemuValidateDomainDeviceDefControllerPCI is doing a more specific check
to assure nobody tries to specify the hotplug attribute for controller
models that don't support it, and you've added the proper check there)
> 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
It makes for less maintenance (and a *miniscule* reduction in disk space
used :-)) if you populate the source xml in qemuxml2argvdata with all
the extra stuff that's added by the parser (ie, just copy the file from
qemuxml2xmloutdata to qemuxml2argvdata) and then make the file in
qemuxml2argvdata a symlink to the original file. That way if something
changes in the source XML, the output XML is automatically updated.
> 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);
You created a qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.xml
testfile, but forgot to add it to qemuxml2xmltest.c. (again, once you do
that and create the
> DO_TEST("misc-disable-suspends",
> QEMU_CAPS_PIIX_DISABLE_S3,
> QEMU_CAPS_PIIX_DISABLE_S4);
>
So 3 items:
1) get rid of unnecessary new chunk in domaincommon.rng
2) add pc-i440fx-acpi-root-hotplug-enable test to qemuxml2xmltest.c
3) make the 2 new xml files in qemuxml2xmloutdata into symlinks to their
(fully populated) mates in qemuxml2argvdata.
More information about the libvir-list
mailing list