[PATCH v7 0/4] Add support to enable/disable hotplug on pci-root controller
changelog: v7: rebased the patches post libvirt release 7.8. Adjusted NEWS.rst to reflect 7.9 (unreleased) version. Changed version info for new config option in formatdomain.rst as well. Added reviewed-by tags. Also in formatdomain.rst added information on what type of hotplug (ACPI/native etc) are affected by the setting. v6: incorporated Jan's suggestions. v5: incorporated suggestions from Laine for patches #2 and #3. The qemu command line is now added in a new function and called from qemuBuildControllersByTypeCommandLine(). Output xmls are now symlinked to input xmls for unit tests. v4: split the original patchset into a pci-root controller specific patch series. also the libvirt conf is now a sub-element of the pci-root controller as was suggested by Dan Berrange. Please see discussion here: https://listman.redhat.com/archives/libvir-list/2021-September/msg00839.html v3: reorganized the patches as per Laine's suggestion. Added more details in commit messages. Added conf description in formatdomain.rst. Added changelog for next release. v2: fixed bugs and added additional missing unit tests. v1: initial implementation. Had some bugs and missed some unit tests This patchset 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 [1]. The corresponding commandline option to qemu for x86 guests is: -global PIIX4_PM.acpi-root-pci-hotplug=<off/on> Notes: 1. The use case scenario described by Laine in https://listman.redhat.com/archives/libvir-list/2020-February/msg00110.html intentionally does not discuss i440fx and focusses solely on q35. I do realize that redhat has moved on from i440fx and currently efforts for new features are concentrated on q35 machines only. We have had some hard debates on this on the qemu mailing list before. The fact of the matter is that i440fx is not at 1-1 parity with q35. There are many users who are currenly using i440fx and are simply not ready to move to q35 without sacrificing some existing features they support today. For example https://wiki.qemu.org/images/4/4e/Q35.pdf lists some of q35 limitations. https://www.linux-kvm.org/images/0/06/2012-forum-Q35.pdf provides more information on the differences. Hence we need to solve the issue Laine has described in the above email for i440fx without adding additional bridges. Further, in Daniel Berrange's words from : https://lists.gnu.org/archive/html/qemu-devel/2020-04/msg03012.html "From the upstream POV, there's been no decision / agreement to phase out PIIX, this is purely a RHEL downstream decision & plan. If other distros / users have a different POV, and find the feature useful, we should accept the patch if it meets the normal QEMU patch requirements. " Also to be noted that I have already experimented this qemu commandline option using libvirt passthrough feature as has been documented in http://blog.vmsplice.net/2011/04/how-to-pass-qemu-command-line-options.html This was only meant to be a short term solution until libvirt started supporting this natively. Supporting this option through libvirt would simplify their use case as well as add capability validations and graceful failure scenarios in case qemu did not support the option. Ani Sinha (4): qemu: capablities: detect presence of acpi-root-pci-hotplug for i440fx machines conf: introduce option to enable/disable pci hotplug on pci-root controller qemu: command: add support to enable/disable hotplug on pci-root controller NEWS: release note the new hotplug enable/disable option on pci-root controller NEWS.rst | 6 ++++ docs/formatdomain.rst | 14 ++++++--- src/qemu/qemu_capabilities.c | 4 +++ src/qemu/qemu_capabilities.h | 3 ++ src/qemu/qemu_command.c | 17 ++++++++++ src/qemu/qemu_validate.c | 9 +++++- .../caps_5.2.0.x86_64.xml | 1 + .../caps_6.0.0.x86_64.xml | 1 + .../caps_6.1.0.x86_64.xml | 1 + .../pc-i440fx-acpi-root-hotplug-disable.args | 31 +++++++++++++++++++ .../pc-i440fx-acpi-root-hotplug-disable.err | 1 + .../pc-i440fx-acpi-root-hotplug-disable.xml | 30 ++++++++++++++++++ .../pc-i440fx-acpi-root-hotplug-enable.xml | 30 ++++++++++++++++++ tests/qemuxml2argvtest.c | 3 ++ .../pc-i440fx-acpi-root-hotplug-disable.xml | 1 + .../pc-i440fx-acpi-root-hotplug-enable.xml | 1 + tests/qemuxml2xmltest.c | 4 +++ 17 files changed, 151 insertions(+), 6 deletions(-) create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.args create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.err 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 120000 tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-disable.xml create mode 120000 tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-enable.xml -- 2.25.1
The following change in qemu added support for a global boolean flag specific to i440fx machines that would turn off or on acpi based hotplug for pci root bus: 3d7e78aa7777f ("Introduce a new flag for i440fx to disable PCI hotplug on the root bus") The option is passed as "-global PIIX4_PM.acpi-root-pci-hotplug=on" etc in qemu commandline. It is enabled by default. This patch adds the corresponding qemu capabilities in libvirt as QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG. Please note that the test specific qemu capabilities .replies files has already been updated as a part of regular refreshing them when a new qemu version is released. Hence, no updates to those files are required. Signed-off-by: Ani Sinha <ani@anisinha.ca> Reviewed-by: Laine Stump <laine@redhat.com> --- src/qemu/qemu_capabilities.c | 4 ++++ src/qemu/qemu_capabilities.h | 3 +++ tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml | 1 + 5 files changed, 10 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index a1be0cb74e..9d0c96a20c 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -639,6 +639,9 @@ VIR_ENUM_IMPL(virQEMUCaps, "s390-pv-guest", /* QEMU_CAPS_S390_PV_GUEST */ "set-action", /* QEMU_CAPS_SET_ACTION */ "virtio-blk.queue-size", /* QEMU_CAPS_VIRTIO_BLK_QUEUE_SIZE */ + + /* 410 */ + "piix4-acpi-root-hotplug-en", /* QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG */ ); @@ -1465,6 +1468,7 @@ static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsIDEDrive[] = { static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsPiix4PM[] = { { "disable_s3", QEMU_CAPS_PIIX_DISABLE_S3, NULL }, { "disable_s4", QEMU_CAPS_PIIX_DISABLE_S4, NULL }, + { "acpi-root-pci-hotplug", QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG, NULL }, }; static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsUSBRedir[] = { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index b0fa1eec35..3460daac00 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -620,6 +620,9 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ QEMU_CAPS_SET_ACTION, /* 'set-action' QMP command */ QEMU_CAPS_VIRTIO_BLK_QUEUE_SIZE, /* virtio-blk-*.queue-size */ + /* 410 */ + QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG, /* -M pc PIIX4_PM.acpi-root-pci-hotplug */ + QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml index 515a970f28..1e5833a9f0 100644 --- a/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml @@ -230,6 +230,7 @@ <flag name='input-linux'/> <flag name='query-display-options'/> <flag name='virtio-blk.queue-size'/> + <flag name='piix4-acpi-root-hotplug-en'/> <version>5002000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100243</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml index 02eb9a15bb..b54dd8a22e 100644 --- a/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml @@ -238,6 +238,7 @@ <flag name='query-display-options'/> <flag name='set-action'/> <flag name='virtio-blk.queue-size'/> + <flag name='piix4-acpi-root-hotplug-en'/> <version>6000000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100242</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml index 678b440d92..0ad493191d 100644 --- a/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml @@ -240,6 +240,7 @@ <flag name='query-display-options'/> <flag name='set-action'/> <flag name='virtio-blk.queue-size'/> + <flag name='piix4-acpi-root-hotplug-en'/> <version>6001000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100243</microcodeVersion> -- 2.25.1
On Fri, Oct 01, 2021 at 02:59:45PM +0530, Ani Sinha wrote:
+++ b/src/qemu/qemu_capabilities.c + /* 410 */ + "piix4-acpi-root-hotplug-en", /* QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG */
Sorry if this is a silly question, but can you please explain where the "-en" suffix comes from? -- Andrea Bolognani / Red Hat / Virtualization
On Mon, 4 Oct 2021, Andrea Bolognani wrote:
On Fri, Oct 01, 2021 at 02:59:45PM +0530, Ani Sinha wrote:
+++ b/src/qemu/qemu_capabilities.c + /* 410 */ + "piix4-acpi-root-hotplug-en", /* QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG */
Sorry if this is a silly question, but can you please explain where the "-en" suffix comes from?
"en" stands for enable.
On Mon, Oct 04, 2021 at 10:19:17PM +0530, Ani Sinha wrote:
On Mon, 4 Oct 2021, Andrea Bolognani wrote:
On Fri, Oct 01, 2021 at 02:59:45PM +0530, Ani Sinha wrote:
+++ b/src/qemu/qemu_capabilities.c + /* 410 */ + "piix4-acpi-root-hotplug-en", /* QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG */
Sorry if this is a silly question, but can you please explain where the "-en" suffix comes from?
"en" stands for enable.
AFAICT there are no other capabilities that use this convention. Moreover, the option can be used both to enable *and* disable hotplug, so the name doesn't seem accurate either... The string is not exposed to users, so ultimately none of this really matters, but I would still suggest changing it to piix4.acpi-root-pci-hotplug I'd be happy to either review a patch of yours that does that, or preparing such a patch myself. -- Andrea Bolognani / Red Hat / Virtualization
On Mon, 4 Oct 2021, Andrea Bolognani wrote:
On Mon, Oct 04, 2021 at 10:19:17PM +0530, Ani Sinha wrote:
On Mon, 4 Oct 2021, Andrea Bolognani wrote:
On Fri, Oct 01, 2021 at 02:59:45PM +0530, Ani Sinha wrote:
+++ b/src/qemu/qemu_capabilities.c + /* 410 */ + "piix4-acpi-root-hotplug-en", /* QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG */
Sorry if this is a silly question, but can you please explain where the "-en" suffix comes from?
"en" stands for enable.
AFAICT there are no other capabilities that use this convention. Moreover, the option can be used both to enable *and* disable hotplug, so the name doesn't seem accurate either...
The string is not exposed to users, so ultimately none of this really matters, but I would still suggest changing it to
piix4.acpi-root-pci-hotplug
I'd be happy to either review a patch of yours that does that, or preparing such a patch myself.
Sent a patch to rename it.
On 10/4/21 1:05 PM, Andrea Bolognani wrote:
On Mon, Oct 04, 2021 at 10:19:17PM +0530, Ani Sinha wrote:
On Mon, 4 Oct 2021, Andrea Bolognani wrote:
On Fri, Oct 01, 2021 at 02:59:45PM +0530, Ani Sinha wrote:
+++ b/src/qemu/qemu_capabilities.c + /* 410 */ + "piix4-acpi-root-hotplug-en", /* QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG */
Sorry if this is a silly question, but can you please explain where the "-en" suffix comes from?
"en" stands for enable.
AFAICT there are no other capabilities that use this convention. Moreover, the option can be used both to enable *and* disable hotplug, so the name doesn't seem accurate either...
The string is not exposed to users, so ultimately none of this really matters, but I would still suggest changing it to
piix4.acpi-root-pci-hotplug
I'd be happy to either review a patch of yours that does that, or preparing such a patch myself.
BAH!!! Someone else mentioned that in an earlier iteration of the patches (and may have even suggested the "." after piix4 instead of "-"), and I had meant to check/ask about it in the respin, and then forgot. :-/ (somehow my brain skipped right over those 3 characters) I agree with Andrea's suggested change, and apologize for not catching it in review (fortunately it was pushed just *after* a release instead of before, so we can still make such a change). I'll gladly review and/or push any such patch that arrives.
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/kvm accelerator only.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@anisinha.ca> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> --- docs/formatdomain.rst | 14 +++++---- src/qemu/qemu_validate.c | 9 +++++- .../pc-i440fx-acpi-root-hotplug-disable.xml | 30 +++++++++++++++++++ .../pc-i440fx-acpi-root-hotplug-enable.xml | 30 +++++++++++++++++++ .../pc-i440fx-acpi-root-hotplug-disable.xml | 1 + .../pc-i440fx-acpi-root-hotplug-enable.xml | 1 + tests/qemuxml2xmltest.c | 4 +++ 7 files changed, 83 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 120000 tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-disable.xml create mode 120000 tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-enable.xml diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index 0f5d833521..467213cc29 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -3776,11 +3776,15 @@ 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.9.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. For + the pci-root controller, the setting affects the ACPI based hotplug. For the + rest, the setting affects both ACPI based hotplug as well as PCIE native + hotplug. 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/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 294cc24b36..d7e306e29d 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -3872,6 +3872,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 '%s' device is not supported by this QEMU binary"), + "hotplug", "pci-root"); + 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)) { @@ -3882,7 +3890,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..93f2779f68 --- /dev/null +++ b/tests/qemuxml2argvdata/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/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-enable.xml b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-enable.xml new file mode 100644 index 0000000000..7c9241958c --- /dev/null +++ b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-enable.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='on'/> + </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/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-disable.xml b/tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-disable.xml new file mode 120000 index 0000000000..0570e40273 --- /dev/null +++ b/tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-disable.xml @@ -0,0 +1 @@ +../qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.xml \ No newline at end of file diff --git a/tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-enable.xml b/tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-enable.xml new file mode 120000 index 0000000000..2b0e42925f --- /dev/null +++ b/tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-enable.xml @@ -0,0 +1 @@ +../qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-enable.xml \ No newline at end of file diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 49b291fadb..eb638c112d 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -424,6 +424,10 @@ 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("pc-i440fx-acpi-root-hotplug-enable", + 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
This change adds qemu backend command line support for enabling or disabling hotplug on the pci-root controller using the 'target' sub-element of the pci-root controller as shown below: <controller type='pci' model='pci-root'> <target hotplug='off'/> </controller> '<target hotplug='off/on'/>' is only valid for pc (x86) machines and turns on the following command line option that is passed to qemu for x86 guests: -global PIIX4_PM.acpi-root-pci-hotplug=<off/on> This change also adds the required qemuxml2argv unit tests in order to test correct qemu arguments. Unit tests have also been added to test qemu capability validation checks. Signed-off-by: Ani Sinha <ani@anisinha.ca> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> --- src/qemu/qemu_command.c | 17 ++++++++++ .../pc-i440fx-acpi-root-hotplug-disable.args | 31 +++++++++++++++++++ .../pc-i440fx-acpi-root-hotplug-disable.err | 1 + tests/qemuxml2argvtest.c | 3 ++ 4 files changed, 52 insertions(+) create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.args create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.err diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 48df8818a6..9368f85639 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2645,6 +2645,20 @@ qemuBuildSkipController(const virDomainControllerDef *controller, return false; } +static int +qemuBuildPMPCIRootHotplugCommandLine(virCommand *cmd, + const virDomainControllerDef *controller) +{ + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI && + controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT && + controller->idx == 0 && + controller->opts.pciopts.hotplug != VIR_TRISTATE_SWITCH_ABSENT) { + virCommandAddArg(cmd, "-global"); + virCommandAddArgFormat(cmd, "PIIX4_PM.acpi-root-pci-hotplug=%s", + virTristateSwitchTypeToString(controller->opts.pciopts.hotplug)); + } + return 0; +} static int qemuBuildControllersByTypeCommandLine(virCommand *cmd, @@ -2661,6 +2675,9 @@ qemuBuildControllersByTypeCommandLine(virCommand *cmd, if (cont->type != type) continue; + if (qemuBuildPMPCIRootHotplugCommandLine(cmd, cont)) + continue; + if (qemuBuildSkipController(cont, def)) continue; diff --git a/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.args b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.args new file mode 100644 index 0000000000..dd8ea503fc --- /dev/null +++ b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.args @@ -0,0 +1,31 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/tmp/lib/domain--1-i440fx \ +USER=test \ +LOGNAME=test \ +XDG_DATA_HOME=/tmp/lib/domain--1-i440fx/.local/share \ +XDG_CACHE_HOME=/tmp/lib/domain--1-i440fx/.cache \ +XDG_CONFIG_HOME=/tmp/lib/domain--1-i440fx/.config \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-x86_64 \ +-name guest=i440fx,debug-threads=on \ +-S \ +-object secret,id=masterKey0,format=raw,file=/tmp/lib/domain--1-i440fx/master-key.aes \ +-machine pc-i440fx-2.5,accel=tcg,usb=off,dump-guest-core=off \ +-m 1024 \ +-realtime mlock=off \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid 56f5055c-1b8d-490c-844a-ad646a1caaaa \ +-display none \ +-no-user-config \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-i440fx/monitor.sock,server=on,wait=off \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-no-acpi \ +-boot strict=on \ +-global PIIX4_PM.acpi-root-pci-hotplug=off \ +-usb \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x2 \ +-msg timestamp=on diff --git a/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.err b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.err new file mode 100644 index 0000000000..b507f1f8bc --- /dev/null +++ b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.err @@ -0,0 +1 @@ +unsupported configuration: setting the hotplug property on a 'pci-root' device is not supported by this QEMU binary diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 13e387df3f..d5ddd60182 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -2571,6 +2571,9 @@ mymain(void) QEMU_CAPS_DEVICE_IOH3420, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, QEMU_CAPS_ICH9_AHCI, QEMU_CAPS_PIIX_DISABLE_S3, QEMU_CAPS_PIIX_DISABLE_S4); + DO_TEST("pc-i440fx-acpi-root-hotplug-disable", + QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG); + DO_TEST_PARSE_ERROR_NOCAPS("pc-i440fx-acpi-root-hotplug-disable"); DO_TEST("q35-usb2", QEMU_CAPS_DEVICE_PCI_BRIDGE, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, -- 2.25.1
A new 'target' subelement of the pci-root controller has been introduced having a 'hotplug' property. This proprty can be used to turn off or turn on hotplug capability of the devices plugged into the pci-root ports. This change release notes this feature for the next release. The new element can be used like this: <controller type='pci' model='pci-root'> <target hotplug='off'/> </controller> This will turn off hotplug capability on the pci-root ports. To turn the capability on, we set hotplug='on' above (which is also the default). Signed-off-by: Ani Sinha <ani@anisinha.ca> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> --- NEWS.rst | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/NEWS.rst b/NEWS.rst index 848f4bd89c..bcd67a893d 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -17,6 +17,12 @@ v7.9.0 (unreleased) * **New features** + * qemu: support disabling hotplug of devices on the pci-root controller + + libvirt can now set the "hotplug" option for pci-root controller which can + be used to disable hotplug/unplug of devices from the pci root port. The + default behavior is to keep hotplug on pci-root ports enabled. + * **Improvements** * **Bug fixes** -- 2.25.1
On 10/1/21 5:29 AM, Ani Sinha wrote:
changelog:
v7: rebased the patches post libvirt release 7.8. Adjusted NEWS.rst to reflect 7.9 (unreleased) version. Changed version info for new config option in formatdomain.rst as well. Added reviewed-by tags. Also in formatdomain.rst added information on what type of hotplug (ACPI/native etc) are affected by the setting. v6: incorporated Jan's suggestions. v5: incorporated suggestions from Laine for patches #2 and #3. The qemu command line is now added in a new function and called from qemuBuildControllersByTypeCommandLine(). Output xmls are now symlinked to input xmls for unit tests. v4: split the original patchset into a pci-root controller specific patch series. also the libvirt conf is now a sub-element of the pci-root controller as was suggested by Dan Berrange. Please see discussion here: https://listman.redhat.com/archives/libvir-list/2021-September/msg00839.html v3: reorganized the patches as per Laine's suggestion. Added more details in commit messages. Added conf description in formatdomain.rst. Added changelog for next release. v2: fixed bugs and added additional missing unit tests. v1: initial implementation. Had some bugs and missed some unit tests
This patchset 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 [1].
The corresponding commandline option to qemu for x86 guests is:
-global PIIX4_PM.acpi-root-pci-hotplug=<off/on>
Notes:
1. The use case scenario described by Laine in https://listman.redhat.com/archives/libvir-list/2020-February/msg00110.html intentionally does not discuss i440fx and focusses solely on q35. I do realize that redhat has moved on from i440fx and currently efforts for new features are concentrated on q35 machines only. We have had some hard debates on this on the qemu mailing list before. The fact of the matter is that i440fx is not at 1-1 parity with q35. There are many users who are currenly using i440fx and are simply not ready to move to q35 without sacrificing some existing features they support today. For example https://wiki.qemu.org/images/4/4e/Q35.pdf lists some of q35 limitations. https://www.linux-kvm.org/images/0/06/2012-forum-Q35.pdf provides more information on the differences. Hence we need to solve the issue Laine has described in the above email for i440fx without adding additional bridges.
Further, in Daniel Berrange's words from : https://lists.gnu.org/archive/html/qemu-devel/2020-04/msg03012.html
"From the upstream POV, there's been no decision / agreement to phase out PIIX, this is purely a RHEL downstream decision & plan. If other distros / users have a different POV, and find the feature useful, we should accept the patch if it meets the normal QEMU patch requirements. "
Also to be noted that I have already experimented this qemu commandline option using libvirt passthrough feature as has been documented in http://blog.vmsplice.net/2011/04/how-to-pass-qemu-command-line-options.html This was only meant to be a short term solution until libvirt started supporting this natively. Supporting this option through libvirt would simplify their use case as well as add capability validations and graceful failure scenarios in case qemu did not support the option.
Ani Sinha (4): qemu: capablities: detect presence of acpi-root-pci-hotplug for i440fx machines conf: introduce option to enable/disable pci hotplug on pci-root controller qemu: command: add support to enable/disable hotplug on pci-root controller NEWS: release note the new hotplug enable/disable option on pci-root controller
NEWS.rst | 6 ++++ docs/formatdomain.rst | 14 ++++++--- src/qemu/qemu_capabilities.c | 4 +++ src/qemu/qemu_capabilities.h | 3 ++ src/qemu/qemu_command.c | 17 ++++++++++ src/qemu/qemu_validate.c | 9 +++++- .../caps_5.2.0.x86_64.xml | 1 + .../caps_6.0.0.x86_64.xml | 1 + .../caps_6.1.0.x86_64.xml | 1 + .../pc-i440fx-acpi-root-hotplug-disable.args | 31 +++++++++++++++++++ .../pc-i440fx-acpi-root-hotplug-disable.err | 1 + .../pc-i440fx-acpi-root-hotplug-disable.xml | 30 ++++++++++++++++++ .../pc-i440fx-acpi-root-hotplug-enable.xml | 30 ++++++++++++++++++ tests/qemuxml2argvtest.c | 3 ++ .../pc-i440fx-acpi-root-hotplug-disable.xml | 1 + .../pc-i440fx-acpi-root-hotplug-enable.xml | 1 + tests/qemuxml2xmltest.c | 4 +++ 17 files changed, 151 insertions(+), 6 deletions(-) create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.args create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.err 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 120000 tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-disable.xml create mode 120000 tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-enable.xml
Okay, I made slight changes to the wording in a couple of the commit messages and documentation to emphasize that it only applies to i440fx machinetypes, and pushed the series. Thanks for the contribution!
On Fri, 1 Oct 2021, Laine Stump wrote:
On 10/1/21 5:29 AM, Ani Sinha wrote:
changelog:
v7: rebased the patches post libvirt release 7.8. Adjusted NEWS.rst to reflect 7.9 (unreleased) version. Changed version info for new config option in formatdomain.rst as well. Added reviewed-by tags. Also in formatdomain.rst added information on what type of hotplug (ACPI/native etc) are affected by the setting. v6: incorporated Jan's suggestions. v5: incorporated suggestions from Laine for patches #2 and #3. The qemu command line is now added in a new function and called from qemuBuildControllersByTypeCommandLine(). Output xmls are now symlinked to input xmls for unit tests. v4: split the original patchset into a pci-root controller specific patch series. also the libvirt conf is now a sub-element of the pci-root controller as was suggested by Dan Berrange. Please see discussion here: https://listman.redhat.com/archives/libvir-list/2021-September/msg00839.html v3: reorganized the patches as per Laine's suggestion. Added more details in commit messages. Added conf description in formatdomain.rst. Added changelog for next release. v2: fixed bugs and added additional missing unit tests. v1: initial implementation. Had some bugs and missed some unit tests
This patchset 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 [1].
The corresponding commandline option to qemu for x86 guests is:
-global PIIX4_PM.acpi-root-pci-hotplug=<off/on>
Notes:
1. The use case scenario described by Laine in https://listman.redhat.com/archives/libvir-list/2020-February/msg00110.html intentionally does not discuss i440fx and focusses solely on q35. I do realize that redhat has moved on from i440fx and currently efforts for new features are concentrated on q35 machines only. We have had some hard debates on this on the qemu mailing list before. The fact of the matter is that i440fx is not at 1-1 parity with q35. There are many users who are currenly using i440fx and are simply not ready to move to q35 without sacrificing some existing features they support today. For example https://wiki.qemu.org/images/4/4e/Q35.pdf lists some of q35 limitations. https://www.linux-kvm.org/images/0/06/2012-forum-Q35.pdf provides more information on the differences. Hence we need to solve the issue Laine has described in the above email for i440fx without adding additional bridges.
Further, in Daniel Berrange's words from : https://lists.gnu.org/archive/html/qemu-devel/2020-04/msg03012.html
"From the upstream POV, there's been no decision / agreement to phase out PIIX, this is purely a RHEL downstream decision & plan. If other distros / users have a different POV, and find the feature useful, we should accept the patch if it meets the normal QEMU patch requirements. "
Also to be noted that I have already experimented this qemu commandline option using libvirt passthrough feature as has been documented in http://blog.vmsplice.net/2011/04/how-to-pass-qemu-command-line-options.html This was only meant to be a short term solution until libvirt started supporting this natively. Supporting this option through libvirt would simplify their use case as well as add capability validations and graceful failure scenarios in case qemu did not support the option.
Ani Sinha (4): qemu: capablities: detect presence of acpi-root-pci-hotplug for i440fx machines conf: introduce option to enable/disable pci hotplug on pci-root controller qemu: command: add support to enable/disable hotplug on pci-root controller NEWS: release note the new hotplug enable/disable option on pci-root controller
NEWS.rst | 6 ++++ docs/formatdomain.rst | 14 ++++++--- src/qemu/qemu_capabilities.c | 4 +++ src/qemu/qemu_capabilities.h | 3 ++ src/qemu/qemu_command.c | 17 ++++++++++ src/qemu/qemu_validate.c | 9 +++++- .../caps_5.2.0.x86_64.xml | 1 + .../caps_6.0.0.x86_64.xml | 1 + .../caps_6.1.0.x86_64.xml | 1 + .../pc-i440fx-acpi-root-hotplug-disable.args | 31 +++++++++++++++++++ .../pc-i440fx-acpi-root-hotplug-disable.err | 1 + .../pc-i440fx-acpi-root-hotplug-disable.xml | 30 ++++++++++++++++++ .../pc-i440fx-acpi-root-hotplug-enable.xml | 30 ++++++++++++++++++ tests/qemuxml2argvtest.c | 3 ++ .../pc-i440fx-acpi-root-hotplug-disable.xml | 1 + .../pc-i440fx-acpi-root-hotplug-enable.xml | 1 + tests/qemuxml2xmltest.c | 4 +++ 17 files changed, 151 insertions(+), 6 deletions(-) create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.args create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.err 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 120000 tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-disable.xml create mode 120000 tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-enable.xml
Okay, I made slight changes to the wording in a couple of the commit messages and documentation to emphasize that it only applies to i440fx machinetypes, and pushed the series.
Thanks for the contribution!
Thanks Laine! Much appretiated! One down, one more to go :-)
participants (3)
-
Andrea Bolognani -
Ani Sinha -
Laine Stump