[PATCH v3 0/5] Add support for two i386 pm options which control acpi hotplug
Hi all: This patchset introduces libvirt xml support for the following two pm conf options: <pm> <acpi-hotplug-bridge enabled='no'/> <acpi-root-hotplug enabled='yes'/> </pm> The above two options are only available for qemu driver and that too for x86 guests only. Both of them are global options. ``acpi-hotplug-bridge`` option enables or disables ACPI hotplug support for cold plugged bridges. Examples of cold plugged bridges include PCI-PCI bridge (pci-bridge controller) for pc machines and pcie-root-port controller for q35 machines. The corresponding commandline options to qemu for x86 guests are: (pc machines): -global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=<off/on> (q35 machines): -global ICH9-LPC.acpi-pci-hotplug-with-bridge-support=<off/on> Being global options, no other bridge specific options for pci-bridge controller or pcie-root-port controllers are required. For pc machine type in x86, this option is available in qemu for a long time, from version 2.1. Please see the changes in qemu.git: 9e047b982452c6 ("piix4: add acpi pci hotplug support") 133a2da488062e ("pc: acpi: generate AML only for PCI0 devices if PCI bridge hotplug is disabled") For q35 machine type, this was introduced in qemu 6.1 with the following changes in qemu.git: (a) c0e427d6eb5fef ("hw/acpi/ich9: Enable ACPI PCI hot-plug") (b) 17858a16950860 ("hw/acpi/ich9: Set ACPI PCI hot-plug as default on Q35") The reasons for enabling ACPI based hotplug for PCIe (q35) based machines (as opposed to native hotplug) for bridges are outlined in (b). It is possible that some users might still want to use native hotplug on PCIe [1]. Therefore, this conf option enables users to choose either ACPI based hotplug or native hotplug for cold plugged bridges (for example for pcie root port controller in q35 machines). ``acpi-root-hotplug`` option enables or disables ACPI based hotplug for PCI root bus (pci-root controller). This option is only available for pc machine type. The corresponding commandline option to qemu for x86 guests is: -global PIIX4_PM.acpi-root-pci-hotplug=<off/on> This additional option enables users to disable hotplug for all devices in the system without adding an additional PCI-PCI bridge, putting the devices behind the bridge and using the existing ``acpi-hotplug-bridge`` option to disable hotplug on that bridge. This feature was introduced from qemu version 5.2 with the following change in qemu.git: 3d7e78aa7777f ("Introduce a new flag for i440fx to disable PCI hotplug on the root bus") The above qemu commit describes some compelling reasons why users might to disable hotplug on PCI root buses [2]. A brief summary of the patches:
[PATCH v3 1/5] qemu: capablities: detect presence of [PATCH v3 2/5] qemu: capablities: detect presence of Patches 1 and 2 implement support for qemu capability checks for the above config options.
[PATCH v3 3/5] conf: introduce acpi-hotplug-bridge and Patch 3 actually adds the config option to the schema and adds related unit tests.
[PATCH v3 4/5] qemu: command: add support for qemu options that Patch 4 adds the backend qemu commandline support for the options. It also adds relevant unit tests for the same.
[PATCH v3 5/5] NEWS: add new acpi pci hotplug options in the release Patch 5 adds the release notes for the next libvirt release.
Changelog: v1: initial implementation. Had some bugs and missed some unit tests. v2: fixed bugs and added additional missing unit tests. 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. Notes: [1] One concrete example of why one might still want to use native hotplug with pcie-root-port controller is the fact that we are still discovering issues with acpi hotplug on PCIE. One such issue is: https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg02146.html Another reason is that users have been using native hotplug on pcie root ports up until now. They have built and tested their systems based on native hotplug. They may not want to suddenly move to acpi based hotplug just because it is now the default in qemu. Supporting the option to chose one or the other through libvirt makes things simpler for end users. [2] 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. [3] Finally, I implemented support for ``acpi-root-hotplug`` option in Qemu. Since adding the support for this option, I have not run away :-) I am still around, fixing other issues in the same subsystem in qemu and also now I have added myself as a reviewer of patches in this area. I will also be trying to support/maintain this new xml conf option in libvirt to the extent I can in future with the help of other experienced maintainers. Obviously this is all freelance work at this moment and is highly dependent on available free time.
qemu added support for i440fx specific global boolean flag PIIX4_PM.acpi-pci-hotplug-with-bridge-support around version 2.1. This flag is enabled by default. When disabled, it turns off acpi pci hotplug for cold plugged pci bridges in i440fx machine types. Very recently, in qemu version 6.1, the same global option was also added for q35 machine types as well. ICH9-LPC.acpi-pci-hotplug-with-bridge-support This option turns on or off acpi based hotplug for cold plugged pcie bridges like pcie root ports. This flag is also enabled by default. Please refer to the following qemu changes: c0e427d6eb5fef ("hw/acpi/ich9: Enable ACPI PCI hot-plug") 17858a16950860 ("hw/acpi/ich9: Set ACPI PCI hot-plug as default on Q35") This patch adds the corresponding qemu capabilities in libvirt. For i440fx, the capability is detected as QEMU_CAPS_PIIX_ACPI_HOTPLUG_BRIDGE. For q35, the capability is detected as QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE. 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> --- src/qemu/qemu_capabilities.c | 6 ++++++ src/qemu/qemu_capabilities.h | 4 ++++ tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_3.1.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml | 1 + 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 | 2 ++ 14 files changed, 23 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index f27a621f8c..c954e42382 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -638,6 +638,10 @@ VIR_ENUM_IMPL(virQEMUCaps, "query-display-options", /* QEMU_CAPS_QUERY_DISPLAY_OPTIONS */ "s390-pv-guest", /* QEMU_CAPS_S390_PV_GUEST */ "set-action", /* QEMU_CAPS_SET_ACTION */ + "piix4-acpi-hotplug-bridge", /* QEMU_CAPS_PIIX_ACPI_HOTPLUG_BRIDGE */ + + /* 410 */ + "ich9-acpi-hotplug-bridge", /* QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE */ ); @@ -1465,6 +1469,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-pci-hotplug-with-bridge-support", QEMU_CAPS_PIIX_ACPI_HOTPLUG_BRIDGE, NULL }, }; static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsUSBRedir[] = { @@ -1517,6 +1522,7 @@ static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsVirtioGpu[] = { static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsICH9[] = { { "disable_s3", QEMU_CAPS_ICH9_DISABLE_S3, NULL }, { "disable_s4", QEMU_CAPS_ICH9_DISABLE_S4, NULL }, + { "acpi-pci-hotplug-with-bridge-support", QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE, NULL }, }; static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsUSBNECXHCI[] = { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index f3379f556c..f53009b6e9 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -618,6 +618,10 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ QEMU_CAPS_QUERY_DISPLAY_OPTIONS, /* 'query-display-options' qmp command present */ QEMU_CAPS_S390_PV_GUEST, /* -object s390-pv-guest,... */ QEMU_CAPS_SET_ACTION, /* 'set-action' QMP command */ + QEMU_CAPS_PIIX_ACPI_HOTPLUG_BRIDGE, /* -M pc PIIX4_PM.acpi-pci-hotplug-with-bridge-support */ + + /* 410 */ + QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE, /* -M q35 ICH9-LPC.acpi-pci-hotplug-with-bridge-support */ QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml index 631f644144..e557e9395d 100644 --- a/tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml @@ -189,6 +189,7 @@ <flag name='cpu-max'/> <flag name='vnc-opts'/> <flag name='input-linux'/> + <flag name='piix4-acpi-hotplug-bridge'/> <version>2011000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100288</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml index d74dc5ebd5..c70e81ac0b 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml @@ -200,6 +200,7 @@ <flag name='cpu-max'/> <flag name='vnc-opts'/> <flag name='input-linux'/> + <flag name='piix4-acpi-hotplug-bridge'/> <version>2011090</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100289</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml index 2cc3c11820..20f58b120f 100644 --- a/tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml @@ -206,6 +206,7 @@ <flag name='cpu-max'/> <flag name='vnc-opts'/> <flag name='input-linux'/> + <flag name='piix4-acpi-hotplug-bridge'/> <version>3000000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100239</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_3.1.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_3.1.0.x86_64.xml index bcc4c44d28..275415b1a7 100644 --- a/tests/qemucapabilitiesdata/caps_3.1.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_3.1.0.x86_64.xml @@ -210,6 +210,7 @@ <flag name='vnc-opts'/> <flag name='input-linux'/> <flag name='query-display-options'/> + <flag name='piix4-acpi-hotplug-bridge'/> <version>3000092</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100240</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml index e999d7574c..3356602fa7 100644 --- a/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml @@ -218,6 +218,7 @@ <flag name='rotation-rate'/> <flag name='input-linux'/> <flag name='query-display-options'/> + <flag name='piix4-acpi-hotplug-bridge'/> <version>4000000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100240</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml index 80c3e3cbed..4ee761c4cb 100644 --- a/tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml @@ -225,6 +225,7 @@ <flag name='rotation-rate'/> <flag name='input-linux'/> <flag name='query-display-options'/> + <flag name='piix4-acpi-hotplug-bridge'/> <version>4001000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100241</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml index 99f9375c04..cc59ffb331 100644 --- a/tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml @@ -236,6 +236,7 @@ <flag name='rotation-rate'/> <flag name='input-linux'/> <flag name='query-display-options'/> + <flag name='piix4-acpi-hotplug-bridge'/> <version>4002000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100242</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml index 03fc7d4106..40b2a61aaa 100644 --- a/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml @@ -243,6 +243,7 @@ <flag name='rotation-rate'/> <flag name='input-linux'/> <flag name='query-display-options'/> + <flag name='piix4-acpi-hotplug-bridge'/> <version>5000000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100241</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml index fc0b502ef9..28c7b0e020 100644 --- a/tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml @@ -245,6 +245,7 @@ <flag name='rotation-rate'/> <flag name='input-linux'/> <flag name='query-display-options'/> + <flag name='piix4-acpi-hotplug-bridge'/> <version>5001000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100242</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml index cb1226fc04..126bae2401 100644 --- a/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml @@ -246,6 +246,7 @@ <flag name='rotation-rate'/> <flag name='input-linux'/> <flag name='query-display-options'/> + <flag name='piix4-acpi-hotplug-bridge'/> <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 d03b8aa726..b01db2dfbe 100644 --- a/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml @@ -254,6 +254,7 @@ <flag name='confidential-guest-support'/> <flag name='query-display-options'/> <flag name='set-action'/> + <flag name='piix4-acpi-hotplug-bridge'/> <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 8239f4266a..b136853aba 100644 --- a/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml @@ -256,6 +256,8 @@ <flag name='confidential-guest-support'/> <flag name='query-display-options'/> <flag name='set-action'/> + <flag name='piix4-acpi-hotplug-bridge'/> + <flag name='ich9-acpi-hotplug-bridge'/> <version>6001000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100243</microcodeVersion> -- 2.25.1
On 9/11/21 11:26 PM, Ani Sinha wrote:
qemu added support for i440fx specific global boolean flag
PIIX4_PM.acpi-pci-hotplug-with-bridge-support
around version 2.1. This flag is enabled by default. When disabled, it turns off acpi pci hotplug for cold plugged pci bridges in i440fx machine types.
Very recently, in qemu version 6.1, the same global option was also added for q35 machine types as well.
ICH9-LPC.acpi-pci-hotplug-with-bridge-support
This option turns on or off acpi based hotplug for cold plugged pcie bridges like pcie root ports. This flag is also enabled by default. Please refer to the following qemu changes:
c0e427d6eb5fef ("hw/acpi/ich9: Enable ACPI PCI hot-plug") 17858a16950860 ("hw/acpi/ich9: Set ACPI PCI hot-plug as default on Q35")
This patch adds the corresponding qemu capabilities in libvirt. For i440fx, the capability is detected as QEMU_CAPS_PIIX_ACPI_HOTPLUG_BRIDGE. For q35, the capability is detected as QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE.
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> except that it needs rebasing since upstream has added another capability in the meantime.
--- src/qemu/qemu_capabilities.c | 6 ++++++ src/qemu/qemu_capabilities.h | 4 ++++ tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_3.1.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml | 1 + 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 | 2 ++ 14 files changed, 23 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index f27a621f8c..c954e42382 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -638,6 +638,10 @@ VIR_ENUM_IMPL(virQEMUCaps, "query-display-options", /* QEMU_CAPS_QUERY_DISPLAY_OPTIONS */ "s390-pv-guest", /* QEMU_CAPS_S390_PV_GUEST */ "set-action", /* QEMU_CAPS_SET_ACTION */ + "piix4-acpi-hotplug-bridge", /* QEMU_CAPS_PIIX_ACPI_HOTPLUG_BRIDGE */ + + /* 410 */ + "ich9-acpi-hotplug-bridge", /* QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE */ );
@@ -1465,6 +1469,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-pci-hotplug-with-bridge-support", QEMU_CAPS_PIIX_ACPI_HOTPLUG_BRIDGE, NULL }, };
static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsUSBRedir[] = { @@ -1517,6 +1522,7 @@ static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsVirtioGpu[] = { static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsICH9[] = { { "disable_s3", QEMU_CAPS_ICH9_DISABLE_S3, NULL }, { "disable_s4", QEMU_CAPS_ICH9_DISABLE_S4, NULL }, + { "acpi-pci-hotplug-with-bridge-support", QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE, NULL }, };
static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsUSBNECXHCI[] = { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index f3379f556c..f53009b6e9 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -618,6 +618,10 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ QEMU_CAPS_QUERY_DISPLAY_OPTIONS, /* 'query-display-options' qmp command present */ QEMU_CAPS_S390_PV_GUEST, /* -object s390-pv-guest,... */ QEMU_CAPS_SET_ACTION, /* 'set-action' QMP command */ + QEMU_CAPS_PIIX_ACPI_HOTPLUG_BRIDGE, /* -M pc PIIX4_PM.acpi-pci-hotplug-with-bridge-support */ + + /* 410 */ + QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE, /* -M q35 ICH9-LPC.acpi-pci-hotplug-with-bridge-support */
QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml index 631f644144..e557e9395d 100644 --- a/tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.11.0.x86_64.xml @@ -189,6 +189,7 @@ <flag name='cpu-max'/> <flag name='vnc-opts'/> <flag name='input-linux'/> + <flag name='piix4-acpi-hotplug-bridge'/> <version>2011000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100288</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml index d74dc5ebd5..c70e81ac0b 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml @@ -200,6 +200,7 @@ <flag name='cpu-max'/> <flag name='vnc-opts'/> <flag name='input-linux'/> + <flag name='piix4-acpi-hotplug-bridge'/> <version>2011090</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100289</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml index 2cc3c11820..20f58b120f 100644 --- a/tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml @@ -206,6 +206,7 @@ <flag name='cpu-max'/> <flag name='vnc-opts'/> <flag name='input-linux'/> + <flag name='piix4-acpi-hotplug-bridge'/> <version>3000000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100239</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_3.1.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_3.1.0.x86_64.xml index bcc4c44d28..275415b1a7 100644 --- a/tests/qemucapabilitiesdata/caps_3.1.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_3.1.0.x86_64.xml @@ -210,6 +210,7 @@ <flag name='vnc-opts'/> <flag name='input-linux'/> <flag name='query-display-options'/> + <flag name='piix4-acpi-hotplug-bridge'/> <version>3000092</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100240</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml index e999d7574c..3356602fa7 100644 --- a/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml @@ -218,6 +218,7 @@ <flag name='rotation-rate'/> <flag name='input-linux'/> <flag name='query-display-options'/> + <flag name='piix4-acpi-hotplug-bridge'/> <version>4000000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100240</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml index 80c3e3cbed..4ee761c4cb 100644 --- a/tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml @@ -225,6 +225,7 @@ <flag name='rotation-rate'/> <flag name='input-linux'/> <flag name='query-display-options'/> + <flag name='piix4-acpi-hotplug-bridge'/> <version>4001000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100241</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml index 99f9375c04..cc59ffb331 100644 --- a/tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml @@ -236,6 +236,7 @@ <flag name='rotation-rate'/> <flag name='input-linux'/> <flag name='query-display-options'/> + <flag name='piix4-acpi-hotplug-bridge'/> <version>4002000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100242</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml index 03fc7d4106..40b2a61aaa 100644 --- a/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml @@ -243,6 +243,7 @@ <flag name='rotation-rate'/> <flag name='input-linux'/> <flag name='query-display-options'/> + <flag name='piix4-acpi-hotplug-bridge'/> <version>5000000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100241</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml index fc0b502ef9..28c7b0e020 100644 --- a/tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml @@ -245,6 +245,7 @@ <flag name='rotation-rate'/> <flag name='input-linux'/> <flag name='query-display-options'/> + <flag name='piix4-acpi-hotplug-bridge'/> <version>5001000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100242</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml index cb1226fc04..126bae2401 100644 --- a/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml @@ -246,6 +246,7 @@ <flag name='rotation-rate'/> <flag name='input-linux'/> <flag name='query-display-options'/> + <flag name='piix4-acpi-hotplug-bridge'/> <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 d03b8aa726..b01db2dfbe 100644 --- a/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml @@ -254,6 +254,7 @@ <flag name='confidential-guest-support'/> <flag name='query-display-options'/> <flag name='set-action'/> + <flag name='piix4-acpi-hotplug-bridge'/> <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 8239f4266a..b136853aba 100644 --- a/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml @@ -256,6 +256,8 @@ <flag name='confidential-guest-support'/> <flag name='query-display-options'/> <flag name='set-action'/> + <flag name='piix4-acpi-hotplug-bridge'/> + <flag name='ich9-acpi-hotplug-bridge'/> <version>6001000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100243</microcodeVersion>
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> --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + 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, 6 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index c954e42382..706169a5cf 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -642,6 +642,7 @@ VIR_ENUM_IMPL(virQEMUCaps, /* 410 */ "ich9-acpi-hotplug-bridge", /* QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE */ + "piix4-acpi-root-hotplug-en", /* QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG */ ); @@ -1470,6 +1471,7 @@ static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsPiix4PM[] = { { "disable_s3", QEMU_CAPS_PIIX_DISABLE_S3, NULL }, { "disable_s4", QEMU_CAPS_PIIX_DISABLE_S4, NULL }, { "acpi-pci-hotplug-with-bridge-support", QEMU_CAPS_PIIX_ACPI_HOTPLUG_BRIDGE, 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 f53009b6e9..b0f9559d95 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -622,6 +622,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ /* 410 */ QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE, /* -M q35 ICH9-LPC.acpi-pci-hotplug-with-bridge-support */ + 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 126bae2401..da52eda200 100644 --- a/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml @@ -247,6 +247,7 @@ <flag name='input-linux'/> <flag name='query-display-options'/> <flag name='piix4-acpi-hotplug-bridge'/> + <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 b01db2dfbe..d75760f1f8 100644 --- a/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml @@ -255,6 +255,7 @@ <flag name='query-display-options'/> <flag name='set-action'/> <flag name='piix4-acpi-hotplug-bridge'/> + <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 b136853aba..8fd3c51f4e 100644 --- a/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml @@ -258,6 +258,7 @@ <flag name='set-action'/> <flag name='piix4-acpi-hotplug-bridge'/> <flag name='ich9-acpi-hotplug-bridge'/> + <flag name='piix4-acpi-root-hotplug-en'/> <version>6001000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100243</microcodeVersion> -- 2.25.1
On 9/11/21 11:26 PM, Ani Sinha wrote:
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> (also needs rebasing due to the other capability added)
--- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + 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, 6 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index c954e42382..706169a5cf 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -642,6 +642,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
/* 410 */ "ich9-acpi-hotplug-bridge", /* QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE */ + "piix4-acpi-root-hotplug-en", /* QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG */ );
@@ -1470,6 +1471,7 @@ static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsPiix4PM[] = { { "disable_s3", QEMU_CAPS_PIIX_DISABLE_S3, NULL }, { "disable_s4", QEMU_CAPS_PIIX_DISABLE_S4, NULL }, { "acpi-pci-hotplug-with-bridge-support", QEMU_CAPS_PIIX_ACPI_HOTPLUG_BRIDGE, 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 f53009b6e9..b0f9559d95 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -622,6 +622,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */
/* 410 */ QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE, /* -M q35 ICH9-LPC.acpi-pci-hotplug-with-bridge-support */ + 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 126bae2401..da52eda200 100644 --- a/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml @@ -247,6 +247,7 @@ <flag name='input-linux'/> <flag name='query-display-options'/> <flag name='piix4-acpi-hotplug-bridge'/> + <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 b01db2dfbe..d75760f1f8 100644 --- a/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml @@ -255,6 +255,7 @@ <flag name='query-display-options'/> <flag name='set-action'/> <flag name='piix4-acpi-hotplug-bridge'/> + <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 b136853aba..8fd3c51f4e 100644 --- a/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml @@ -258,6 +258,7 @@ <flag name='set-action'/> <flag name='piix4-acpi-hotplug-bridge'/> <flag name='ich9-acpi-hotplug-bridge'/> + <flag name='piix4-acpi-root-hotplug-en'/> <version>6001000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>43100243</microcodeVersion>
This change introduces libvirt xml support for the following two pm options: <pm> <acpi-hotplug-bridge enabled='no'/> <acpi-root-hotplug enabled='yes'/> </pm> The above two options are only available for qemu driver and that too for x86 guests only. Both of them are global options. 'acpi-hotplug-bridge' option enables or disables ACPI hotplug support for cold plugged bridges. Examples of cold plugged bridges include PCI-PCI bridge (pci-bridge controller) for pc machines and pcie-root-port controller for q35 machines. Being global options, no other bridge specific options for pci-bridge controller or pcie-root-port controllers are required. For pc machine type in x86, this option is available in qemu for a long time, from version 2.1. Please see the changes in qemu.git: 9e047b982452c6 ("piix4: add acpi pci hotplug support") 133a2da488062e ("pc: acpi: generate AML only for PCI0 devices if PCI bridge hotplug is disabled") For q35 machine type, this was introduced in qemu 6.1 with the following changes in qemu.git: (a) c0e427d6eb5fef ("hw/acpi/ich9: Enable ACPI PCI hot-plug") (b) 17858a16950860 ("hw/acpi/ich9: Set ACPI PCI hot-plug as default on Q35") The reasons for enabling ACPI based hotplug for PCIe (q35) based machines (as opposed to native hotplug) are outlined in (b). It is possible that some users might still want to use native hotplug on PCIe. Therefore, this config option enables users to choose either ACPI based hotplug or native hotplug for cold plugged bridges (for example for pcie root port controller in q35 machines). 'acpi-root-hotplug' option enables or disables ACPI based hotplug for PCI root bus (pci-root controller). This option is only available for pc machine type. This additional option enables users to disable hotplug for all devices in the system without adding an additional PCI-PCI bridge, putting the devices behind the bridge and using the existing "acpi-hotplug-bridge" option to disable hotplug on that bridge. This feature was introduced from qemu version 5.2 with the following change in qemu.git: 3d7e78aa7777f ("Introduce a new flag for i440fx to disable PCI hotplug on the root bus") The above qemu commit describes some compelling reasons why users might to disable hotplug on PCI root buses. This change also adds related unit tests to exercise the new conf options. Signed-off-by: Ani Sinha <ani@anisinha.ca> --- docs/formatdomain.rst | 36 ++++++++++++--- docs/schemas/domaincommon.rng | 17 +++++++ src/conf/domain_conf.c | 21 ++++++++- src/conf/domain_conf.h | 2 + .../pc-i440fx-acpi-hotplug-bridge-disable.xml | 17 +++++++ .../pc-i440fx-acpi-root-hotplug-disable.xml | 17 +++++++ .../q35-acpi-hotplug-bridge-disable.xml | 17 +++++++ .../pc-i440fx-acpi-hotplug-bridge-disable.xml | 31 +++++++++++++ .../pc-i440fx-acpi-root-hotplug-disable.xml | 31 +++++++++++++ .../q35-acpi-hotplug-bridge-disable.xml | 45 +++++++++++++++++++ tests/qemuxml2xmltest.c | 9 ++++ 11 files changed, 236 insertions(+), 7 deletions(-) create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.xml create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.xml create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.xml create mode 100644 tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-disable.xml create mode 100644 tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-disable.xml create mode 100644 tests/qemuxml2xmloutdata/q35-acpi-hotplug-bridge-disable.xml diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index ad3b4ea92c..c8b04c625c 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -1793,16 +1793,40 @@ advertisements to the guest OS. (NB: Only qemu driver support) <pm> <suspend-to-disk enabled='no'/> <suspend-to-mem enabled='yes'/> + <acpi-hotplug-bridge enabled='no'/> + <acpi-root-hotplug enabled='yes'/> </pm> ... ``pm`` - These elements enable ('yes') or disable ('no') BIOS support for S3 - (suspend-to-mem) and S4 (suspend-to-disk) ACPI sleep states. If nothing is - specified, then the hypervisor will be left with its default value. - Note: This setting cannot prevent the guest OS from performing a suspend as - the guest OS itself can choose to circumvent the unavailability of the sleep - states (e.g. S4 by turning off completely). + These elements enable ('yes') or disable ('no') certain BIOS advertisements. + If nothing is specified, then the hypervisor will be left with its default value. + The following BIOS options are available: + +``suspend-to-mem`` + support for S3 (suspend-to-mem) ACPI sleep states. +``suspend-to-disk`` + support for S4 (suspend-to-disk) ACPI sleep states. + + Note that for the above two options, the setting cannot prevent the guest OS + from performing a suspend as the guest OS itself can choose to circumvent the + unavailability of the sleep states (e.g. S4 by turning off completely). + +``acpi-hotplug-bridge`` + :since:`Since 7.8.0` This option enables or disables BIOS ACPI based hotplug support + for cold plugged bridges. It is available only for x86 guests, both for q35 and pc + machine types. For pc machines, the support is available from `QEMU 2.12`. For q35 + machines, the support is available from `QEMU 6.1`. Examples of cold plugged bridges + include PCI-PCI bridges for pc machine types (pci-bridge controller). For q35 machines, + it includes PCIE root ports (pcie-root-port controller). This is a global option that + affects all bridges. No other bridge specific option is required to be specified. + +``acpi-root-hotplug`` + :since:`Since 7.8.0 (QEMU 5.2)` This option enables or disables BIOS ACPI based + hotplug support for PCI root bus (pci-root controller). This is available only + for x86 guests, only for pc machine type. For q35 machines, PCIE root complex, + the pcie-root controller, does not support hotplug capability. This is also a global + option. :anchor:`<a id="elementsFeatures"/>` diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 11fa24f398..a51efb933d 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -4333,6 +4333,16 @@ <ref name="suspendChoices"/> </element> </optional> + <optional> + <element name="acpi-hotplug-bridge"> + <ref name="acpiHotplugChoices"/> + </element> + </optional> + <optional> + <element name="acpi-root-hotplug"> + <ref name="acpiHotplugChoices"/> + </element> + </optional> </interleave> <empty/> </element> @@ -4344,6 +4354,13 @@ </attribute> </optional> </define> + <define name="acpiHotplugChoices"> + <optional> + <attribute name="enabled"> + <ref name="virYesNo"/> + </attribute> + </optional> + </define> <!-- Specific setup for a qemu emulated character device. Note: this definition doesn't fully specify the constraints on this node. diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index cb9e7218ff..e48661534f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -19339,6 +19339,16 @@ virDomainDefLifecycleParse(virDomainDef *def, &def->pm.s4) < 0) goto error; + if (virDomainPMStateParseXML(ctxt, + "string(./pm/acpi-hotplug-bridge/@enabled)", + &def->pm.acpi_hp_bridge) < 0) + goto error; + + if (virDomainPMStateParseXML(ctxt, + "string(./pm/acpi-root-hotplug/@enabled)", + &def->pm.acpi_root_hp) < 0) + goto error; + return 0; error: @@ -28151,7 +28161,8 @@ virDomainDefFormatInternalSetRootName(virDomainDef *def, virDomainLockFailureTypeToString) < 0) return -1; - if (def->pm.s3 || def->pm.s4) { + if (def->pm.s3 || def->pm.s4 || def->pm.acpi_hp_bridge || + def->pm.acpi_root_hp) { virBufferAddLit(buf, "<pm>\n"); virBufferAdjustIndent(buf, 2); if (def->pm.s3) { @@ -28162,6 +28173,14 @@ virDomainDefFormatInternalSetRootName(virDomainDef *def, virBufferAsprintf(buf, "<suspend-to-disk enabled='%s'/>\n", virTristateBoolTypeToString(def->pm.s4)); } + if (def->pm.acpi_hp_bridge) { + virBufferAsprintf(buf, "<acpi-hotplug-bridge enabled='%s'/>\n", + virTristateBoolTypeToString(def->pm.acpi_hp_bridge)); + } + if (def->pm.acpi_root_hp) { + virBufferAsprintf(buf, "<acpi-root-hotplug enabled='%s'/>\n", + virTristateBoolTypeToString(def->pm.acpi_root_hp)); + } virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</pm>\n"); } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index c7e6df7981..863bdf54f8 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2644,6 +2644,8 @@ struct _virDomainPowerManagement { /* These options are of type enum virTristateBool */ int s3; int s4; + int acpi_hp_bridge; + int acpi_root_hp; }; struct _virDomainPerfDef { diff --git a/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.xml b/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.xml new file mode 100644 index 0000000000..b96caa9b2d --- /dev/null +++ b/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-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> + <pm> + <acpi-hotplug-bridge enabled='no'/> + </pm> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + </devices> +</domain> 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..ef563200bf --- /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> + <pm> + <acpi-root-hotplug enabled='no'/> + </pm> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.xml b/tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.xml new file mode 100644 index 0000000000..bb9e8d4ee2 --- /dev/null +++ b/tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.xml @@ -0,0 +1,17 @@ +<domain type='qemu'> + <name>q35</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-q35-2.5'>hvm</type> + <boot dev='network'/> + </os> + <pm> + <acpi-hotplug-bridge enabled='no'/> + </pm> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-disable.xml b/tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-disable.xml new file mode 100644 index 0000000000..2534dc7e35 --- /dev/null +++ b/tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-disable.xml @@ -0,0 +1,31 @@ +<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> + <pm> + <acpi-hotplug-bridge enabled='no'/> + </pm> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <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 100644 index 0000000000..0ee404ee46 --- /dev/null +++ b/tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-disable.xml @@ -0,0 +1,31 @@ +<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> + <pm> + <acpi-root-hotplug enabled='no'/> + </pm> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <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/q35-acpi-hotplug-bridge-disable.xml b/tests/qemuxml2xmloutdata/q35-acpi-hotplug-bridge-disable.xml new file mode 100644 index 0000000000..e8c6f82145 --- /dev/null +++ b/tests/qemuxml2xmloutdata/q35-acpi-hotplug-bridge-disable.xml @@ -0,0 +1,45 @@ +<domain type='qemu'> + <name>q35</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-q35-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> + <pm> + <acpi-hotplug-bridge enabled='no'/> + </pm> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='sata' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pcie-root'/> + <controller type='pci' index='1' model='dmi-to-pci-bridge'> + <model name='i82801b11-bridge'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1e' function='0x0'/> + </controller> + <controller type='pci' index='2' model='pci-bridge'> + <model name='pci-bridge'/> + <target chassisNr='2'/> + <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/> + </controller> + <controller type='pci' index='3' model='pcie-root-port'> + <model name='ioh3420'/> + <target chassis='3' port='0x8'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> + </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='0x02' slot='0x01' function='0x0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 6d3526f91f..cbf2306e05 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -425,6 +425,15 @@ 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-hotplug-bridge-disable", + QEMU_CAPS_PIIX_ACPI_HOTPLUG_BRIDGE); + DO_TEST("q35-acpi-hotplug-bridge-disable", + QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_IOH3420, + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, + QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE); + 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, Sep 12, 2021 at 08:56:29AM +0530, Ani Sinha wrote:
This change introduces libvirt xml support for the following two pm options:
<pm> <acpi-hotplug-bridge enabled='no'/> <acpi-root-hotplug enabled='yes'/> </pm>
+``acpi-hotplug-bridge`` + :since:`Since 7.8.0` This option enables or disables BIOS ACPI based hotplug support + for cold plugged bridges. It is available only for x86 guests, both for q35 and pc + machine types. For pc machines, the support is available from `QEMU 2.12`. For q35 + machines, the support is available from `QEMU 6.1`. Examples of cold plugged bridges + include PCI-PCI bridges for pc machine types (pci-bridge controller). For q35 machines, + it includes PCIE root ports (pcie-root-port controller). This is a global option that + affects all bridges. No other bridge specific option is required to be specified.
Can you confirm my understanding of the situation.. - i440fx / PCI topology - hotplug always uses ACPI - q35 / PCIe topology - hotplug historically used native PCIe hotplug, but in 6.1 switched to ACPI Given, the name "acpi-hotplug-bridge", am I right that this option has *no* effect, if the q35 machine is using native PCIe hotplug approach ? IOW, is it a no-op until 6.1 based machine types for q35 ? Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Tue, 28 Sep 2021, Daniel P. Berrangé wrote:
On Sun, Sep 12, 2021 at 08:56:29AM +0530, Ani Sinha wrote:
This change introduces libvirt xml support for the following two pm options:
<pm> <acpi-hotplug-bridge enabled='no'/> <acpi-root-hotplug enabled='yes'/> </pm>
+``acpi-hotplug-bridge`` + :since:`Since 7.8.0` This option enables or disables BIOS ACPI based hotplug support + for cold plugged bridges. It is available only for x86 guests, both for q35 and pc + machine types. For pc machines, the support is available from `QEMU 2.12`. For q35 + machines, the support is available from `QEMU 6.1`. Examples of cold plugged bridges + include PCI-PCI bridges for pc machine types (pci-bridge controller). For q35 machines, + it includes PCIE root ports (pcie-root-port controller). This is a global option that + affects all bridges. No other bridge specific option is required to be specified.
Can you confirm my understanding of the situation..
- i440fx / PCI topology - hotplug always uses ACPI
ACPI is the primary means of enabling hotplug. shpc might also have a role here but I think it is disabled. Igor (cc'd) might throw some lights on how shpc comes to play.
- q35 / PCIe topology - hotplug historically used native PCIe hotplug, but in 6.1 switched to ACPI
Correct.
Given, the name "acpi-hotplug-bridge", am I right that this option has *no* effect, if the q35 machine is using native PCIe hotplug approach ?
Its complicated. With "acpi-hotplug-bridge" ON, native hotplug is disabled in qemu. With "acpi-hotplug-bridge" OFF, native hotplug is enabled in qemu. Libvirt does not allow it, but by directly using qemu commandline it is possible to enable ACPI hotplug for pcie-root-ports by turning "acpi-hotplug-bridge" ON _and_ enable native hotplug at the same time. In that case, as Julia has mentioned in some other thread, the guest OS always choses native over ACPI based hotplug. The <target hotplug="on/off"> switch in libvirt for pcie-root-ports currently does not care whether native or acpi hotplug is used. It simply turns on the hotplug for that particular port. Whether ACPI or native is used is controlled by this global flag that Julia has introduced in 6.1.
IOW, is it a no-op until 6.1 based machine types for q35 ?
Correct. For q35 this command line did not exist until 6.1.
On Tue, Sep 28, 2021 at 02:35:47PM +0530, Ani Sinha wrote:
On Tue, 28 Sep 2021, Daniel P. Berrangé wrote:
On Sun, Sep 12, 2021 at 08:56:29AM +0530, Ani Sinha wrote:
This change introduces libvirt xml support for the following two pm options:
<pm> <acpi-hotplug-bridge enabled='no'/> <acpi-root-hotplug enabled='yes'/> </pm>
+``acpi-hotplug-bridge`` + :since:`Since 7.8.0` This option enables or disables BIOS ACPI based hotplug support + for cold plugged bridges. It is available only for x86 guests, both for q35 and pc + machine types. For pc machines, the support is available from `QEMU 2.12`. For q35 + machines, the support is available from `QEMU 6.1`. Examples of cold plugged bridges + include PCI-PCI bridges for pc machine types (pci-bridge controller). For q35 machines, + it includes PCIE root ports (pcie-root-port controller). This is a global option that + affects all bridges. No other bridge specific option is required to be specified.
Can you confirm my understanding of the situation..
- i440fx / PCI topology - hotplug always uses ACPI
ACPI is the primary means of enabling hotplug. shpc might also have a role here but I think it is disabled. Igor (cc'd) might throw some lights on how shpc comes to play.
Yes, I think it will be important to understand if 'shpc' becomes relevant when ACPI hotplug is disabled for PCI
- q35 / PCIe topology - hotplug historically used native PCIe hotplug, but in 6.1 switched to ACPI
Correct.
Given, the name "acpi-hotplug-bridge", am I right that this option has *no* effect, if the q35 machine is using native PCIe hotplug approach ?
Its complicated. With "acpi-hotplug-bridge" ON, native hotplug is disabled in qemu. With "acpi-hotplug-bridge" OFF, native hotplug is enabled in qemu.
Oh, I mis-read and didn't realize this was controlling the QEMU "acpi-pci-hotplug-with-bridge-support" configuration. With this in mind I think the naming is somewhat misleading. Setting it to off would give users the impression that hotplug is disabled, which is not the case for Q35 at least. It is just switching to a different hotplug implementation. At least from Q35 pov, I think it would be better to call it hotplug-mode="acpi|pcie" so it is clear that no matter what value it is set to, hotplug is still available. If we also consider PIIX, then depending on the answer wrt shpc above, we might want one of hotplug-mode="acpi|pcie|none" hotplug-mode="acpi|pcie|shpc"
Libvirt does not allow it, but by directly using qemu commandline it is possible to enable ACPI hotplug for pcie-root-ports by turning "acpi-hotplug-bridge" ON _and_ enable native hotplug at the same time. In that case, as Julia has mentioned in some other thread, the guest OS always choses native over ACPI based hotplug.
I see that is refering to pie-root-port gaining native_hotplug=bool in 6.1 You say the guest OS always chooses native over ACPI - is that reqiired in the spec, or is that just how some common guest OS have choosen to impl it today ?
The <target hotplug="on/off"> switch in libvirt for pcie-root-ports currently does not care whether native or acpi hotplug is used. It simply turns on the hotplug for that particular port. Whether ACPI or native is used is controlled by this global flag that Julia has introduced in 6.1.
Right so we have * PIIX4 - acpi-root-pci-hotplug=bool Whether hotplug is enabled for the root bridge or not <target hotplug="on/off"> for pci-root controller - acpi-pci-hotplug-with-bridge-support=bool Toggles support for ACPI based hotplug across all bridges. If disabled will there will be no hotplug at all for PIIX4 ? Or does 'shpc' come into play in that scenario ? PIIX combinations (1) acpi-root-pci-hotplug=yes acpi-pci-hotplug-with-bridge-support=yes - All bridges have hotplug (2) acpi-root-pci-hotplug=yes acpi-pci-hotplug-with-bridge-support=no - No bridges have hotplug (3) acpi-root-pci-hotplug=no acpi-pci-hotplug-with-bridge-support=yes - All bridges except root have hotplug (4) acpi-root-pci-hotplug=no acpi-pci-hotplug-with-bridge-support=no - No bridges have hotplug. Essentially identical to (2) * Q35 - acpi-pci-hotplug-with-bridge-support=bool Toggles support for ACPI based hotplug. If disabled native PCIe hotplug is activated instead * pcie-root-port - hotplug=bool Toggles whether hotplug is enabled on the port. Affects either native and ACPI based hotplug, depending on what acpi-pci-hotplug-with-bridge-support=bool is set to ? <target hotplug="on/off"> for pcie-root-port controller - native_hotplug=bool Can be used to also enable native hotplug, even when ACPI based hotplug is globally enabled. IOW only really relevant when acpi-pci-hotplug-with-bridge-support=true Q35 combinations (1) acpi-pci-hotplug-with-bridge-support=yes pcie-root-port.hotplug=yes pcie-root-port.native_hotplug=yes ports have both ACPI + native hotplug, guest prefers native (2) acpi-pci-hotplug-with-bridge-support=yes pcie-root-port.hotplug=no pcie-root-port.native_hotplug=yes ports have no hotplug, despite native_hotplug=yes IIUC (3) acpi-pci-hotplug-with-bridge-support=yes pcie-root-port.hotplug=yes pcie-root-port.native_hotplug=no ports have ACPI hotplug only (4) acpi-pci-hotplug-with-bridge-support=yes pcie-root-port.hotplug=no pcie-root-port.native_hotplug=no ports have no hotplug (5) acpi-pci-hotplug-with-bridge-support=no pcie-root-port.hotplug=yes pcie-root-port.native_hotplug=yes ports have native hotplug (6) acpi-pci-hotplug-with-bridge-support=no pcie-root-port.hotplug=no pcie-root-port.native_hotplug=yes ports have no hotplug, despite native_hotplug=yes IIUC (7) acpi-pci-hotplug-with-bridge-support=no pcie-root-port.hotplug=yes pcie-root-port.native_hotplug=no ports have no hotplug, despite hotplug=yes IIUC (8) acpi-pci-hotplug-with-bridge-support=no pcie-root-port.hotplug=no pcie-root-port.native_hotplug=no ports have no hotplug Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Tue, 28 Sep 2021, Daniel P. Berrangé wrote:
On Tue, Sep 28, 2021 at 02:35:47PM +0530, Ani Sinha wrote:
On Tue, 28 Sep 2021, Daniel P. Berrangé wrote:
On Sun, Sep 12, 2021 at 08:56:29AM +0530, Ani Sinha wrote:
This change introduces libvirt xml support for the following two pm options:
<pm> <acpi-hotplug-bridge enabled='no'/> <acpi-root-hotplug enabled='yes'/> </pm>
+``acpi-hotplug-bridge`` + :since:`Since 7.8.0` This option enables or disables BIOS ACPI based hotplug support + for cold plugged bridges. It is available only for x86 guests, both for q35 and pc + machine types. For pc machines, the support is available from `QEMU 2.12`. For q35 + machines, the support is available from `QEMU 6.1`. Examples of cold plugged bridges + include PCI-PCI bridges for pc machine types (pci-bridge controller). For q35 machines, + it includes PCIE root ports (pcie-root-port controller). This is a global option that + affects all bridges. No other bridge specific option is required to be specified.
Can you confirm my understanding of the situation..
- i440fx / PCI topology - hotplug always uses ACPI
ACPI is the primary means of enabling hotplug. shpc might also have a role here but I think it is disabled. Igor (cc'd) might throw some lights on how shpc comes to play.
Yes, I think it will be important to understand if 'shpc' becomes relevant when ACPI hotplug is disabled for PCI
- q35 / PCIe topology - hotplug historically used native PCIe hotplug, but in 6.1 switched to ACPI
Correct.
Given, the name "acpi-hotplug-bridge", am I right that this option has *no* effect, if the q35 machine is using native PCIe hotplug approach ?
Its complicated. With "acpi-hotplug-bridge" ON, native hotplug is disabled in qemu. With "acpi-hotplug-bridge" OFF, native hotplug is enabled in qemu.
Oh, I mis-read and didn't realize this was controlling the QEMU "acpi-pci-hotplug-with-bridge-support" configuration.
With this in mind I think the naming is somewhat misleading. Setting it to off would give users the impression that hotplug is disabled, which is not the case for Q35 at least. It is just switching to a different hotplug implementation.
At least from Q35 pov, I think it would be better to call it
hotplug-mode="acpi|pcie"
so it is clear that no matter what value it is set to, hotplug is still available.
If we also consider PIIX, then depending on the answer wrt shpc above, we might want one of
hotplug-mode="acpi|pcie|none" hotplug-mode="acpi|pcie|shpc"
If libvirt does not deal with shpc today I think we should not bother with shpc at all. We should simply have a boolean mode appropriately named that choses between acpi hotplug vs native.
Libvirt does not allow it, but by directly using qemu commandline it is possible to enable ACPI hotplug for pcie-root-ports by turning "acpi-hotplug-bridge" ON _and_ enable native hotplug at the same time. In that case, as Julia has mentioned in some other thread, the guest OS always choses native over ACPI based hotplug.
I see that is refering to pie-root-port gaining native_hotplug=bool in 6.1
You say the guest OS always chooses native over ACPI - is that reqiired in the spec, or is that just how some common guest OS have choosen to impl it today ?
I am not sure but I *think* this is guest OS dependent too? Julia to rescue here :-)
The <target hotplug="on/off"> switch in libvirt for pcie-root-ports currently does not care whether native or acpi hotplug is used. It simply turns on the hotplug for that particular port. Whether ACPI or native is used is controlled by this global flag that Julia has introduced in 6.1.
Right so we have
* PIIX4
- acpi-root-pci-hotplug=bool
Whether hotplug is enabled for the root bridge or not
<target hotplug="on/off"> for pci-root controller
- acpi-pci-hotplug-with-bridge-support=bool
Toggles support for ACPI based hotplug across all bridges. If disabled will there will be no hotplug at all for PIIX4 ? Or does 'shpc' come into play in that scenario ?
PIIX combinations
(1) acpi-root-pci-hotplug=yes acpi-pci-hotplug-with-bridge-support=yes
- All bridges have hotplug
(2) acpi-root-pci-hotplug=yes acpi-pci-hotplug-with-bridge-support=no
- No bridges have hotplug
(3) acpi-root-pci-hotplug=no acpi-pci-hotplug-with-bridge-support=yes
- All bridges except root have hotplug
(4) acpi-root-pci-hotplug=no acpi-pci-hotplug-with-bridge-support=no
- No bridges have hotplug. Essentially identical to (2)
no (4) is not identical to (2). In (4) no hotplug is enabled. In (2) pci root bus still has hotplug enabled.
* Q35
- acpi-pci-hotplug-with-bridge-support=bool
Toggles support for ACPI based hotplug. If disabled native PCIe hotplug is activated instead
* pcie-root-port
- hotplug=bool
Toggles whether hotplug is enabled on the port. Affects either native and ACPI based hotplug, depending on what acpi-pci-hotplug-with-bridge-support=bool is set to ?
<target hotplug="on/off"> for pcie-root-port controller
- native_hotplug=bool
Can be used to also enable native hotplug, even when ACPI based hotplug is globally enabled. IOW only really relevant when acpi-pci-hotplug-with-bridge-support=true
Q35 combinations
(1) acpi-pci-hotplug-with-bridge-support=yes pcie-root-port.hotplug=yes pcie-root-port.native_hotplug=yes
ports have both ACPI + native hotplug, guest prefers native
(2) acpi-pci-hotplug-with-bridge-support=yes pcie-root-port.hotplug=no pcie-root-port.native_hotplug=yes
ports have no hotplug, despite native_hotplug=yes IIUC
(3) acpi-pci-hotplug-with-bridge-support=yes pcie-root-port.hotplug=yes pcie-root-port.native_hotplug=no
ports have ACPI hotplug only
(4) acpi-pci-hotplug-with-bridge-support=yes pcie-root-port.hotplug=no pcie-root-port.native_hotplug=no
ports have no hotplug
(5) acpi-pci-hotplug-with-bridge-support=no pcie-root-port.hotplug=yes pcie-root-port.native_hotplug=yes
ports have native hotplug
(6) acpi-pci-hotplug-with-bridge-support=no pcie-root-port.hotplug=no pcie-root-port.native_hotplug=yes
ports have no hotplug, despite native_hotplug=yes IIUC
(7) acpi-pci-hotplug-with-bridge-support=no pcie-root-port.hotplug=yes pcie-root-port.native_hotplug=no
ports have no hotplug, despite hotplug=yes IIUC
(8) acpi-pci-hotplug-with-bridge-support=no pcie-root-port.hotplug=no pcie-root-port.native_hotplug=no
ports have no hotplug
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Tue, Sep 28, 2021 at 3:28 PM Ani Sinha <ani@anisinha.ca> wrote:
On Tue, 28 Sep 2021, Daniel P. Berrangé wrote:
On Tue, Sep 28, 2021 at 02:35:47PM +0530, Ani Sinha wrote:
On Tue, 28 Sep 2021, Daniel P. Berrangé wrote:
On Sun, Sep 12, 2021 at 08:56:29AM +0530, Ani Sinha wrote:
This change introduces libvirt xml support for the following two pm options:
<pm> <acpi-hotplug-bridge enabled='no'/> <acpi-root-hotplug enabled='yes'/> </pm>
+``acpi-hotplug-bridge`` + :since:`Since 7.8.0` This option enables or disables BIOS ACPI based hotplug support + for cold plugged bridges. It is available only for x86 guests, both for q35 and pc + machine types. For pc machines, the support is available from `QEMU 2.12`. For q35 + machines, the support is available from `QEMU 6.1`. Examples of cold plugged bridges + include PCI-PCI bridges for pc machine types (pci-bridge controller). For q35 machines, + it includes PCIE root ports (pcie-root-port controller). This is a global option that + affects all bridges. No other bridge specific option is required to be specified.
Can you confirm my understanding of the situation..
- i440fx / PCI topology - hotplug always uses ACPI
ACPI is the primary means of enabling hotplug. shpc might also have a role here but I think it is disabled. Igor (cc'd) might throw some lights on how shpc comes to play.
Yes, I think it will be important to understand if 'shpc' becomes relevant when ACPI hotplug is disabled for PCI
https://patchwork.kernel.org/project/qemu-devel/patch/1478099802-14188-1-git... has some discussions around shpc. Also "PCI devices can be hot-plugged into PCI Express to PCI and PCI-PCI Bridges. The PCI hot-plug into PCI-PCI bridge is ACPI based, whereas hot-plug into PCI Express to PCI bridges is SHPC-based. They both can work side by side with the PCI Express native hot-plug." https://android.googlesource.com/platform/external/qemu/+/emu-master-dev/doc... My own experiments on i440fx has shown that Windows does not seem to care about shpc. Disabling acpi hotplug on bridges disables hotplug.
- q35 / PCIe topology - hotplug historically used native PCIe hotplug, but in 6.1 switched to ACPI
Correct.
Given, the name "acpi-hotplug-bridge", am I right that this option has *no* effect, if the q35 machine is using native PCIe hotplug approach ?
Its complicated. With "acpi-hotplug-bridge" ON, native hotplug is disabled in qemu. With "acpi-hotplug-bridge" OFF, native hotplug is enabled in qemu.
Oh, I mis-read and didn't realize this was controlling the QEMU "acpi-pci-hotplug-with-bridge-support" configuration.
With this in mind I think the naming is somewhat misleading. Setting it to off would give users the impression that hotplug is disabled, which is not the case for Q35 at least. It is just switching to a different hotplug implementation.
At least from Q35 pov, I think it would be better to call it
hotplug-mode="acpi|pcie"
so it is clear that no matter what value it is set to, hotplug is still available.
If we also consider PIIX, then depending on the answer wrt shpc above, we might want one of
hotplug-mode="acpi|pcie|none" hotplug-mode="acpi|pcie|shpc"
If libvirt does not deal with shpc today I think we should not bother with shpc at all. We should simply have a boolean mode appropriately named that choses between acpi hotplug vs native.
Libvirt does not allow it, but by directly using qemu commandline it is possible to enable ACPI hotplug for pcie-root-ports by turning "acpi-hotplug-bridge" ON _and_ enable native hotplug at the same time. In that case, as Julia has mentioned in some other thread, the guest OS always choses native over ACPI based hotplug.
I see that is refering to pie-root-port gaining native_hotplug=bool in 6.1
You say the guest OS always chooses native over ACPI - is that reqiired in the spec, or is that just how some common guest OS have choosen to impl it today ?
I am not sure but I *think* this is guest OS dependent too? Julia to rescue here :-)
The <target hotplug="on/off"> switch in libvirt for pcie-root-ports currently does not care whether native or acpi hotplug is used. It simply turns on the hotplug for that particular port. Whether ACPI or native is used is controlled by this global flag that Julia has introduced in 6.1.
Right so we have
* PIIX4
- acpi-root-pci-hotplug=bool
Whether hotplug is enabled for the root bridge or not
<target hotplug="on/off"> for pci-root controller
- acpi-pci-hotplug-with-bridge-support=bool
Toggles support for ACPI based hotplug across all bridges. If disabled will there will be no hotplug at all for PIIX4 ? Or does 'shpc' come into play in that scenario ?
PIIX combinations
(1) acpi-root-pci-hotplug=yes acpi-pci-hotplug-with-bridge-support=yes
- All bridges have hotplug
(2) acpi-root-pci-hotplug=yes acpi-pci-hotplug-with-bridge-support=no
- No bridges have hotplug
(3) acpi-root-pci-hotplug=no acpi-pci-hotplug-with-bridge-support=yes
- All bridges except root have hotplug
(4) acpi-root-pci-hotplug=no acpi-pci-hotplug-with-bridge-support=no
- No bridges have hotplug. Essentially identical to (2)
no (4) is not identical to (2). In (4) no hotplug is enabled. In (2) pci root bus still has hotplug enabled.
* Q35
- acpi-pci-hotplug-with-bridge-support=bool
Toggles support for ACPI based hotplug. If disabled native PCIe hotplug is activated instead
* pcie-root-port
- hotplug=bool
Toggles whether hotplug is enabled on the port. Affects either native and ACPI based hotplug, depending on what acpi-pci-hotplug-with-bridge-support=bool is set to ?
<target hotplug="on/off"> for pcie-root-port controller
- native_hotplug=bool
Can be used to also enable native hotplug, even when ACPI based hotplug is globally enabled. IOW only really relevant when acpi-pci-hotplug-with-bridge-support=true
Q35 combinations
(1) acpi-pci-hotplug-with-bridge-support=yes pcie-root-port.hotplug=yes pcie-root-port.native_hotplug=yes
ports have both ACPI + native hotplug, guest prefers native
(2) acpi-pci-hotplug-with-bridge-support=yes pcie-root-port.hotplug=no pcie-root-port.native_hotplug=yes
ports have no hotplug, despite native_hotplug=yes IIUC
(3) acpi-pci-hotplug-with-bridge-support=yes pcie-root-port.hotplug=yes pcie-root-port.native_hotplug=no
ports have ACPI hotplug only
(4) acpi-pci-hotplug-with-bridge-support=yes pcie-root-port.hotplug=no pcie-root-port.native_hotplug=no
ports have no hotplug
(5) acpi-pci-hotplug-with-bridge-support=no pcie-root-port.hotplug=yes pcie-root-port.native_hotplug=yes
ports have native hotplug
(6) acpi-pci-hotplug-with-bridge-support=no pcie-root-port.hotplug=no pcie-root-port.native_hotplug=yes
ports have no hotplug, despite native_hotplug=yes IIUC
(7) acpi-pci-hotplug-with-bridge-support=no pcie-root-port.hotplug=yes pcie-root-port.native_hotplug=no
ports have no hotplug, despite hotplug=yes IIUC
(8) acpi-pci-hotplug-with-bridge-support=no pcie-root-port.hotplug=no pcie-root-port.native_hotplug=no
ports have no hotplug
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Tue, Sep 28, 2021 at 03:28:04PM +0530, Ani Sinha wrote:
On Tue, 28 Sep 2021, Daniel P. Berrangé wrote:
On Tue, Sep 28, 2021 at 02:35:47PM +0530, Ani Sinha wrote:
On Tue, 28 Sep 2021, Daniel P. Berrangé wrote:
On Sun, Sep 12, 2021 at 08:56:29AM +0530, Ani Sinha wrote:
This change introduces libvirt xml support for the following two pm options:
<pm> <acpi-hotplug-bridge enabled='no'/> <acpi-root-hotplug enabled='yes'/> </pm>
+``acpi-hotplug-bridge`` + :since:`Since 7.8.0` This option enables or disables BIOS ACPI based hotplug support + for cold plugged bridges. It is available only for x86 guests, both for q35 and pc + machine types. For pc machines, the support is available from `QEMU 2.12`. For q35 + machines, the support is available from `QEMU 6.1`. Examples of cold plugged bridges + include PCI-PCI bridges for pc machine types (pci-bridge controller). For q35 machines, + it includes PCIE root ports (pcie-root-port controller). This is a global option that + affects all bridges. No other bridge specific option is required to be specified.
Can you confirm my understanding of the situation..
- i440fx / PCI topology - hotplug always uses ACPI
ACPI is the primary means of enabling hotplug. shpc might also have a role here but I think it is disabled. Igor (cc'd) might throw some lights on how shpc comes to play.
Yes, I think it will be important to understand if 'shpc' becomes relevant when ACPI hotplug is disabled for PCI
- q35 / PCIe topology - hotplug historically used native PCIe hotplug, but in 6.1 switched to ACPI
Correct.
Given, the name "acpi-hotplug-bridge", am I right that this option has *no* effect, if the q35 machine is using native PCIe hotplug approach ?
Its complicated. With "acpi-hotplug-bridge" ON, native hotplug is disabled in qemu. With "acpi-hotplug-bridge" OFF, native hotplug is enabled in qemu.
Oh, I mis-read and didn't realize this was controlling the QEMU "acpi-pci-hotplug-with-bridge-support" configuration.
With this in mind I think the naming is somewhat misleading. Setting it to off would give users the impression that hotplug is disabled, which is not the case for Q35 at least. It is just switching to a different hotplug implementation.
At least from Q35 pov, I think it would be better to call it
hotplug-mode="acpi|pcie"
so it is clear that no matter what value it is set to, hotplug is still available.
If we also consider PIIX, then depending on the answer wrt shpc above, we might want one of
hotplug-mode="acpi|pcie|none" hotplug-mode="acpi|pcie|shpc"
If libvirt does not deal with shpc today I think we should not bother with shpc at all. We should simply have a boolean mode appropriately named that choses between acpi hotplug vs native.
I want to understand what's possible at the qemu hardware level, so we don't paint ourselves into a corner. IIUC, with shpc we only have a toggle on "pci-bridge" devices, and those currently have shpc=true by default. There's no shpc setting on the pci-root, and theres no global setting. Seems to imply that if we have acpi-hotplug disabled for PIIX, then there would be no hotplug on the pci-root, but shpc hotplug would still be available on any pci-bridge devices ?
Libvirt does not allow it, but by directly using qemu commandline it is possible to enable ACPI hotplug for pcie-root-ports by turning "acpi-hotplug-bridge" ON _and_ enable native hotplug at the same time. In that case, as Julia has mentioned in some other thread, the guest OS always choses native over ACPI based hotplug.
I see that is refering to pie-root-port gaining native_hotplug=bool in 6.1
You say the guest OS always chooses native over ACPI - is that reqiired in the spec, or is that just how some common guest OS have choosen to impl it today ?
I am not sure but I *think* this is guest OS dependent too? Julia to rescue here :-)
The <target hotplug="on/off"> switch in libvirt for pcie-root-ports currently does not care whether native or acpi hotplug is used. It simply turns on the hotplug for that particular port. Whether ACPI or native is used is controlled by this global flag that Julia has introduced in 6.1.
Right so we have
* PIIX4
- acpi-root-pci-hotplug=bool
Whether hotplug is enabled for the root bridge or not
<target hotplug="on/off"> for pci-root controller
- acpi-pci-hotplug-with-bridge-support=bool
Toggles support for ACPI based hotplug across all bridges. If disabled will there will be no hotplug at all for PIIX4 ? Or does 'shpc' come into play in that scenario ?
PIIX combinations
(1) acpi-root-pci-hotplug=yes acpi-pci-hotplug-with-bridge-support=yes
- All bridges have hotplug
(2) acpi-root-pci-hotplug=yes acpi-pci-hotplug-with-bridge-support=no
- No bridges have hotplug
(3) acpi-root-pci-hotplug=no acpi-pci-hotplug-with-bridge-support=yes
- All bridges except root have hotplug
(4) acpi-root-pci-hotplug=no acpi-pci-hotplug-with-bridge-support=no
- No bridges have hotplug. Essentially identical to (2)
no (4) is not identical to (2). In (4) no hotplug is enabled. In (2) pci root bus still has hotplug enabled.
So you're saying that acpi-root-pci-hotplug=yes overrides the global request acpi-pci-hotplug-with-bridge-support=no and turns ACPI hotplug back on for the pcie-root
* Q35
- acpi-pci-hotplug-with-bridge-support=bool
Toggles support for ACPI based hotplug. If disabled native PCIe hotplug is activated instead
* pcie-root-port
- hotplug=bool
Toggles whether hotplug is enabled on the port. Affects either native and ACPI based hotplug, depending on what acpi-pci-hotplug-with-bridge-support=bool is set to ?
<target hotplug="on/off"> for pcie-root-port controller
- native_hotplug=bool
Can be used to also enable native hotplug, even when ACPI based hotplug is globally enabled. IOW only really relevant when acpi-pci-hotplug-with-bridge-support=true
Q35 combinations
(1) acpi-pci-hotplug-with-bridge-support=yes pcie-root-port.hotplug=yes pcie-root-port.native_hotplug=yes
ports have both ACPI + native hotplug, guest prefers native
(2) acpi-pci-hotplug-with-bridge-support=yes pcie-root-port.hotplug=no pcie-root-port.native_hotplug=yes
ports have no hotplug, despite native_hotplug=yes IIUC
(3) acpi-pci-hotplug-with-bridge-support=yes pcie-root-port.hotplug=yes pcie-root-port.native_hotplug=no
ports have ACPI hotplug only
(4) acpi-pci-hotplug-with-bridge-support=yes pcie-root-port.hotplug=no pcie-root-port.native_hotplug=no
ports have no hotplug
(5) acpi-pci-hotplug-with-bridge-support=no pcie-root-port.hotplug=yes pcie-root-port.native_hotplug=yes
ports have native hotplug
(6) acpi-pci-hotplug-with-bridge-support=no pcie-root-port.hotplug=no pcie-root-port.native_hotplug=yes
ports have no hotplug, despite native_hotplug=yes IIUC
(7) acpi-pci-hotplug-with-bridge-support=no pcie-root-port.hotplug=yes pcie-root-port.native_hotplug=no
ports have no hotplug, despite hotplug=yes IIUC
(8) acpi-pci-hotplug-with-bridge-support=no pcie-root-port.hotplug=no pcie-root-port.native_hotplug=no
ports have no hotplug
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Tue, Sep 28, 2021 at 11:47:26AM +0100, Daniel P. Berrangé wrote:
On Tue, Sep 28, 2021 at 03:28:04PM +0530, Ani Sinha wrote:
On Tue, 28 Sep 2021, Daniel P. Berrangé wrote:
On Tue, Sep 28, 2021 at 02:35:47PM +0530, Ani Sinha wrote:
On Tue, 28 Sep 2021, Daniel P. Berrangé wrote:
On Sun, Sep 12, 2021 at 08:56:29AM +0530, Ani Sinha wrote:
This change introduces libvirt xml support for the following two pm options:
<pm> <acpi-hotplug-bridge enabled='no'/> <acpi-root-hotplug enabled='yes'/> </pm>
+``acpi-hotplug-bridge`` + :since:`Since 7.8.0` This option enables or disables BIOS ACPI based hotplug support + for cold plugged bridges. It is available only for x86 guests, both for q35 and pc + machine types. For pc machines, the support is available from `QEMU 2.12`. For q35 + machines, the support is available from `QEMU 6.1`. Examples of cold plugged bridges + include PCI-PCI bridges for pc machine types (pci-bridge controller). For q35 machines, + it includes PCIE root ports (pcie-root-port controller). This is a global option that + affects all bridges. No other bridge specific option is required to be specified.
Can you confirm my understanding of the situation..
- i440fx / PCI topology - hotplug always uses ACPI
ACPI is the primary means of enabling hotplug. shpc might also have a role here but I think it is disabled. Igor (cc'd) might throw some lights on how shpc comes to play.
Yes, I think it will be important to understand if 'shpc' becomes relevant when ACPI hotplug is disabled for PCI
- q35 / PCIe topology - hotplug historically used native PCIe hotplug, but in 6.1 switched to ACPI
Correct.
Given, the name "acpi-hotplug-bridge", am I right that this option has *no* effect, if the q35 machine is using native PCIe hotplug approach ?
Its complicated. With "acpi-hotplug-bridge" ON, native hotplug is disabled in qemu. With "acpi-hotplug-bridge" OFF, native hotplug is enabled in qemu.
Oh, I mis-read and didn't realize this was controlling the QEMU "acpi-pci-hotplug-with-bridge-support" configuration.
With this in mind I think the naming is somewhat misleading. Setting it to off would give users the impression that hotplug is disabled, which is not the case for Q35 at least. It is just switching to a different hotplug implementation.
At least from Q35 pov, I think it would be better to call it
hotplug-mode="acpi|pcie"
so it is clear that no matter what value it is set to, hotplug is still available.
If we also consider PIIX, then depending on the answer wrt shpc above, we might want one of
hotplug-mode="acpi|pcie|none" hotplug-mode="acpi|pcie|shpc"
If libvirt does not deal with shpc today I think we should not bother with shpc at all. We should simply have a boolean mode appropriately named that choses between acpi hotplug vs native.
I want to understand what's possible at the qemu hardware level, so we don't paint ourselves into a corner.
IIUC, with shpc we only have a toggle on "pci-bridge" devices, and those currently have shpc=true by default. There's no shpc setting on the pci-root, and theres no global setting.
Opps, I was mislead. They have shpc=false by default due to machine types >= 2.9 overriding it to false
Seems to imply that if we have acpi-hotplug disabled for PIIX, then there would be no hotplug on the pci-root, but shpc hotplug would still be available on any pci-bridge devices ?
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Tue, 28 Sep 2021 11:59:42 +0100 Daniel P. Berrangé <berrange@redhat.com> wrote:
On Tue, Sep 28, 2021 at 11:47:26AM +0100, Daniel P. Berrangé wrote:
On Tue, Sep 28, 2021 at 03:28:04PM +0530, Ani Sinha wrote:
On Tue, 28 Sep 2021, Daniel P. Berrangé wrote:
On Tue, Sep 28, 2021 at 02:35:47PM +0530, Ani Sinha wrote:
On Tue, 28 Sep 2021, Daniel P. Berrangé wrote:
On Sun, Sep 12, 2021 at 08:56:29AM +0530, Ani Sinha wrote: > This change introduces libvirt xml support for the following two pm options: > > <pm> > <acpi-hotplug-bridge enabled='no'/> > <acpi-root-hotplug enabled='yes'/> > </pm>
> +``acpi-hotplug-bridge`` > + :since:`Since 7.8.0` This option enables or disables BIOS ACPI based hotplug support > + for cold plugged bridges. It is available only for x86 guests, both for q35 and pc > + machine types. For pc machines, the support is available from `QEMU 2.12`. For q35 > + machines, the support is available from `QEMU 6.1`. Examples of cold plugged bridges > + include PCI-PCI bridges for pc machine types (pci-bridge controller). For q35 machines, > + it includes PCIE root ports (pcie-root-port controller). This is a global option that > + affects all bridges. No other bridge specific option is required to be specified.
Can you confirm my understanding of the situation..
- i440fx / PCI topology - hotplug always uses ACPI
ACPI is the primary means of enabling hotplug. shpc might also have a role here but I think it is disabled. Igor (cc'd) might throw some lights on how shpc comes to play.
Yes, I think it will be important to understand if 'shpc' becomes relevant when ACPI hotplug is disabled for PCI
- q35 / PCIe topology - hotplug historically used native PCIe hotplug, but in 6.1 switched to ACPI
Correct.
Given, the name "acpi-hotplug-bridge", am I right that this option has *no* effect, if the q35 machine is using native PCIe hotplug approach ?
Its complicated. With "acpi-hotplug-bridge" ON, native hotplug is disabled in qemu. With "acpi-hotplug-bridge" OFF, native hotplug is enabled in qemu.
Oh, I mis-read and didn't realize this was controlling the QEMU "acpi-pci-hotplug-with-bridge-support" configuration.
With this in mind I think the naming is somewhat misleading. Setting it to off would give users the impression that hotplug is disabled, which is not the case for Q35 at least. It is just switching to a different hotplug implementation.
At least from Q35 pov, I think it would be better to call it
hotplug-mode="acpi|pcie"
so it is clear that no matter what value it is set to, hotplug is still available.
If we also consider PIIX, then depending on the answer wrt shpc above, we might want one of
hotplug-mode="acpi|pcie|none" hotplug-mode="acpi|pcie|shpc"
If libvirt does not deal with shpc today I think we should not bother with shpc at all. We should simply have a boolean mode appropriately named that choses between acpi hotplug vs native.
I want to understand what's possible at the qemu hardware level, so we don't paint ourselves into a corner.
IIUC, with shpc we only have a toggle on "pci-bridge" devices, and those currently have shpc=true by default. There's no shpc setting on the pci-root, and theres no global setting.
Opps, I was mislead. They have shpc=false by default due to machine types >= 2.9 overriding it to false
If I read it correctly, shcp is on by default (modulo 2.9 see 2fa356629ed2)
Seems to imply that if we have acpi-hotplug disabled for PIIX, then there would be no hotplug on the pci-root, but shpc hotplug would still be available on any pci-bridge devices ?
Regards, Daniel
On Wed, Sep 29, 2021 at 02:49:32PM +0200, Igor Mammedov wrote:
On Tue, 28 Sep 2021 11:59:42 +0100 Daniel P. Berrangé <berrange@redhat.com> wrote:
On Tue, Sep 28, 2021 at 11:47:26AM +0100, Daniel P. Berrangé wrote:
On Tue, Sep 28, 2021 at 03:28:04PM +0530, Ani Sinha wrote:
On Tue, 28 Sep 2021, Daniel P. Berrangé wrote:
On Tue, Sep 28, 2021 at 02:35:47PM +0530, Ani Sinha wrote:
On Tue, 28 Sep 2021, Daniel P. Berrangé wrote:
> On Sun, Sep 12, 2021 at 08:56:29AM +0530, Ani Sinha wrote: > > This change introduces libvirt xml support for the following two pm options: > > > > <pm> > > <acpi-hotplug-bridge enabled='no'/> > > <acpi-root-hotplug enabled='yes'/> > > </pm> > > > > +``acpi-hotplug-bridge`` > > + :since:`Since 7.8.0` This option enables or disables BIOS ACPI based hotplug support > > + for cold plugged bridges. It is available only for x86 guests, both for q35 and pc > > + machine types. For pc machines, the support is available from `QEMU 2.12`. For q35 > > + machines, the support is available from `QEMU 6.1`. Examples of cold plugged bridges > > + include PCI-PCI bridges for pc machine types (pci-bridge controller). For q35 machines, > > + it includes PCIE root ports (pcie-root-port controller). This is a global option that > > + affects all bridges. No other bridge specific option is required to be specified. > > Can you confirm my understanding of the situation.. > > - i440fx / PCI topology - hotplug always uses ACPI >
ACPI is the primary means of enabling hotplug. shpc might also have a role here but I think it is disabled. Igor (cc'd) might throw some lights on how shpc comes to play.
Yes, I think it will be important to understand if 'shpc' becomes relevant when ACPI hotplug is disabled for PCI
> - q35 / PCIe topology - hotplug historically used native PCIe hotplug, > but in 6.1 switched to ACPI >
Correct.
> Given, the name "acpi-hotplug-bridge", am I right that this option > has *no* effect, if the q35 machine is using native PCIe hotplug > approach ?
Its complicated. With "acpi-hotplug-bridge" ON, native hotplug is disabled in qemu. With "acpi-hotplug-bridge" OFF, native hotplug is enabled in qemu.
Oh, I mis-read and didn't realize this was controlling the QEMU "acpi-pci-hotplug-with-bridge-support" configuration.
With this in mind I think the naming is somewhat misleading. Setting it to off would give users the impression that hotplug is disabled, which is not the case for Q35 at least. It is just switching to a different hotplug implementation.
At least from Q35 pov, I think it would be better to call it
hotplug-mode="acpi|pcie"
so it is clear that no matter what value it is set to, hotplug is still available.
If we also consider PIIX, then depending on the answer wrt shpc above, we might want one of
hotplug-mode="acpi|pcie|none" hotplug-mode="acpi|pcie|shpc"
If libvirt does not deal with shpc today I think we should not bother with shpc at all. We should simply have a boolean mode appropriately named that choses between acpi hotplug vs native.
I want to understand what's possible at the qemu hardware level, so we don't paint ourselves into a corner.
IIUC, with shpc we only have a toggle on "pci-bridge" devices, and those currently have shpc=true by default. There's no shpc setting on the pci-root, and theres no global setting.
Opps, I was mislead. They have shpc=false by default due to machine types >= 2.9 overriding it to false
If I read it correctly, shcp is on by default (modulo 2.9 see 2fa356629ed2)
Sigh, so I was mislead twice ! I should have just tested it for real, which I have now done below which confirms what you say: $ echo -e "info qtree\r\nquit\r\n" | qemu-system-x86_64 -monitor stdio -device pci-bridge,chassis_nr=4 | grep shpc shpc = true $ echo -e "info qtree\r\nquit\r\n" | qemu-system-x86_64 -monitor stdio -device pci-bridge,chassis_nr=4 -M pc-i440fx-2.8 | grep shpc shpc = true $ echo -e "info qtree\r\nquit\r\n" | qemu-system-x86_64 -monitor stdio -device pci-bridge,chassis_nr=4 -M pc-i440fx-2.9 | grep shpc shpc = false $ echo -e "info qtree\r\nquit\r\n" | qemu-system-x86_64 -monitor stdio -device pci-bridge,chassis_nr=4 -M pc-i440fx-2.10 | grep shpc shpc = true Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Tue, 28 Sep 2021, Daniel P. Berrangé wrote:
On Tue, Sep 28, 2021 at 03:28:04PM +0530, Ani Sinha wrote:
On Tue, 28 Sep 2021, Daniel P. Berrangé wrote:
On Tue, Sep 28, 2021 at 02:35:47PM +0530, Ani Sinha wrote:
On Tue, 28 Sep 2021, Daniel P. Berrangé wrote:
On Sun, Sep 12, 2021 at 08:56:29AM +0530, Ani Sinha wrote:
This change introduces libvirt xml support for the following two pm options:
<pm> <acpi-hotplug-bridge enabled='no'/> <acpi-root-hotplug enabled='yes'/> </pm>
+``acpi-hotplug-bridge`` + :since:`Since 7.8.0` This option enables or disables BIOS ACPI based hotplug support + for cold plugged bridges. It is available only for x86 guests, both for q35 and pc + machine types. For pc machines, the support is available from `QEMU 2.12`. For q35 + machines, the support is available from `QEMU 6.1`. Examples of cold plugged bridges + include PCI-PCI bridges for pc machine types (pci-bridge controller). For q35 machines, + it includes PCIE root ports (pcie-root-port controller). This is a global option that + affects all bridges. No other bridge specific option is required to be specified.
Can you confirm my understanding of the situation..
- i440fx / PCI topology - hotplug always uses ACPI
ACPI is the primary means of enabling hotplug. shpc might also have a role here but I think it is disabled. Igor (cc'd) might throw some lights on how shpc comes to play.
Yes, I think it will be important to understand if 'shpc' becomes relevant when ACPI hotplug is disabled for PCI
- q35 / PCIe topology - hotplug historically used native PCIe hotplug, but in 6.1 switched to ACPI
Correct.
Given, the name "acpi-hotplug-bridge", am I right that this option has *no* effect, if the q35 machine is using native PCIe hotplug approach ?
Its complicated. With "acpi-hotplug-bridge" ON, native hotplug is disabled in qemu. With "acpi-hotplug-bridge" OFF, native hotplug is enabled in qemu.
Oh, I mis-read and didn't realize this was controlling the QEMU "acpi-pci-hotplug-with-bridge-support" configuration.
With this in mind I think the naming is somewhat misleading. Setting it to off would give users the impression that hotplug is disabled, which is not the case for Q35 at least. It is just switching to a different hotplug implementation.
At least from Q35 pov, I think it would be better to call it
hotplug-mode="acpi|pcie"
so it is clear that no matter what value it is set to, hotplug is still available.
If we also consider PIIX, then depending on the answer wrt shpc above, we might want one of
hotplug-mode="acpi|pcie|none" hotplug-mode="acpi|pcie|shpc"
If libvirt does not deal with shpc today I think we should not bother with shpc at all. We should simply have a boolean mode appropriately named that choses between acpi hotplug vs native.
I want to understand what's possible at the qemu hardware level, so we don't paint ourselves into a corner.
IIUC, with shpc we only have a toggle on "pci-bridge" devices, and those currently have shpc=true by default. There's no shpc setting on the pci-root, and theres no global setting.
Seems to imply that if we have acpi-hotplug disabled for PIIX, then there would be no hotplug on the pci-root, but shpc hotplug would still be available on any pci-bridge devices ?
Libvirt does not allow it, but by directly using qemu commandline it is possible to enable ACPI hotplug for pcie-root-ports by turning "acpi-hotplug-bridge" ON _and_ enable native hotplug at the same time. In that case, as Julia has mentioned in some other thread, the guest OS always choses native over ACPI based hotplug.
I see that is refering to pie-root-port gaining native_hotplug=bool in 6.1
You say the guest OS always chooses native over ACPI - is that reqiired in the spec, or is that just how some common guest OS have choosen to impl it today ?
I am not sure but I *think* this is guest OS dependent too? Julia to rescue here :-)
The <target hotplug="on/off"> switch in libvirt for pcie-root-ports currently does not care whether native or acpi hotplug is used. It simply turns on the hotplug for that particular port. Whether ACPI or native is used is controlled by this global flag that Julia has introduced in 6.1.
Right so we have
* PIIX4
- acpi-root-pci-hotplug=bool
Whether hotplug is enabled for the root bridge or not
<target hotplug="on/off"> for pci-root controller
- acpi-pci-hotplug-with-bridge-support=bool
Toggles support for ACPI based hotplug across all bridges. If disabled will there will be no hotplug at all for PIIX4 ? Or does 'shpc' come into play in that scenario ?
PIIX combinations
(1) acpi-root-pci-hotplug=yes acpi-pci-hotplug-with-bridge-support=yes
- All bridges have hotplug
(2) acpi-root-pci-hotplug=yes acpi-pci-hotplug-with-bridge-support=no
- No bridges have hotplug
(3) acpi-root-pci-hotplug=no acpi-pci-hotplug-with-bridge-support=yes
- All bridges except root have hotplug
(4) acpi-root-pci-hotplug=no acpi-pci-hotplug-with-bridge-support=no
- No bridges have hotplug. Essentially identical to (2)
no (4) is not identical to (2). In (4) no hotplug is enabled. In (2) pci root bus still has hotplug enabled.
So you're saying that acpi-root-pci-hotplug=yes overrides the global request acpi-pci-hotplug-with-bridge-support=no and turns ACPI hotplug back on for the pcie-root
No. acpi-pci-hotplug-with-bridge-support only affects the bridges. It has no effect on pci root bus. pci root bus is controlled by acpi-root-pci-hotplug option. So acpi-pci-hotplug-with-bridge-support is not "global" wrt hotplug for ports on the root bus and bridges. Its "global" as in its a global option for all bridges. Makes sense? I know this is all convoluted.
* Q35
- acpi-pci-hotplug-with-bridge-support=bool
Toggles support for ACPI based hotplug. If disabled native PCIe hotplug is activated instead
* pcie-root-port
- hotplug=bool
Toggles whether hotplug is enabled on the port. Affects either native and ACPI based hotplug, depending on what acpi-pci-hotplug-with-bridge-support=bool is set to ?
<target hotplug="on/off"> for pcie-root-port controller
- native_hotplug=bool
Can be used to also enable native hotplug, even when ACPI based hotplug is globally enabled. IOW only really relevant when acpi-pci-hotplug-with-bridge-support=true
Q35 combinations
(1) acpi-pci-hotplug-with-bridge-support=yes pcie-root-port.hotplug=yes pcie-root-port.native_hotplug=yes
ports have both ACPI + native hotplug, guest prefers native
(2) acpi-pci-hotplug-with-bridge-support=yes pcie-root-port.hotplug=no pcie-root-port.native_hotplug=yes
ports have no hotplug, despite native_hotplug=yes IIUC
(3) acpi-pci-hotplug-with-bridge-support=yes pcie-root-port.hotplug=yes pcie-root-port.native_hotplug=no
ports have ACPI hotplug only
(4) acpi-pci-hotplug-with-bridge-support=yes pcie-root-port.hotplug=no pcie-root-port.native_hotplug=no
ports have no hotplug
(5) acpi-pci-hotplug-with-bridge-support=no pcie-root-port.hotplug=yes pcie-root-port.native_hotplug=yes
ports have native hotplug
(6) acpi-pci-hotplug-with-bridge-support=no pcie-root-port.hotplug=no pcie-root-port.native_hotplug=yes
ports have no hotplug, despite native_hotplug=yes IIUC
(7) acpi-pci-hotplug-with-bridge-support=no pcie-root-port.hotplug=yes pcie-root-port.native_hotplug=no
ports have no hotplug, despite hotplug=yes IIUC
(8) acpi-pci-hotplug-with-bridge-support=no pcie-root-port.hotplug=no pcie-root-port.native_hotplug=no
ports have no hotplug
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Tue, 28 Sep 2021 11:47:26 +0100 Daniel P. Berrangé <berrange@redhat.com> wrote:
On Tue, Sep 28, 2021 at 03:28:04PM +0530, Ani Sinha wrote:
On Tue, 28 Sep 2021, Daniel P. Berrangé wrote:
On Tue, Sep 28, 2021 at 02:35:47PM +0530, Ani Sinha wrote:
On Tue, 28 Sep 2021, Daniel P. Berrangé wrote:
On Sun, Sep 12, 2021 at 08:56:29AM +0530, Ani Sinha wrote:
This change introduces libvirt xml support for the following two pm options:
[...]
The <target hotplug="on/off"> switch in libvirt for pcie-root-ports currently does not care whether native or acpi hotplug is used. It simply turns on the hotplug for that particular port. Whether ACPI or native is used is controlled by this global flag that Julia has introduced in 6.1.
Right so we have
*1*) following applies to piix4/q35: * ACPI hotplug when enabled, affects _only_ cold-plugged 'bridges' since it requires 'slots' being described in DSDT table which in current impl. is static table built at reset time. (i.e. built-in or 'bridges' specified on command line, where 'bridges' could be PCI-PCI or PCIe-PCI or root/downstream-ports') for anything else ('bridges' added with device_add) native hotplug is in use (whether it's SHPC or PCI-E native). ACPI hotplug wiring is done by calling qbus_set_hotplug_handler() * for root bus piix4_pm_realize()/ich9_pm_init() * for anything else acpi_pcihp_device_plug_cb()
* PIIX4
- acpi-root-pci-hotplug=bool
Whether hotplug is enabled for the root bridge or not
<target hotplug="on/off"> for pci-root controller
- acpi-pci-hotplug-with-bridge-support=bool
Toggles support for ACPI based hotplug across all bridges. If disabled will there will be no hotplug at all for PIIX4 ? Or does 'shpc' come into play in that scenario ?
'SHPC' hotplug kicks in if it's enabled. (defaults to 'on' except 2.9 machine type) on q35/APCI side of things we always advertise -all_ hotplug methods available build_q35_osc_method() /* * Always allow native PME, AER (no dependencies) * Allow SHPC (PCI bridges can have SHPC controller) */ aml_append(if_ctx, aml_and(a_ctrl, aml_int(0x1F), a_ctrl)); bits 0, 1 are Native PCI-E hotplug and SHPC respectively for PIIX4 we don't have _OSC so it's up to guest OS to make up supported methods. In order of preference: * Windows supports ACPI hotplug then Native PCI-E (SHPC never worked there) * Linux supports ACPI hotplug, SHPC, Native PCI-E (SHPC worked poorly due to need to reserve IO for bridges io reservation hinting (impl. later by Marcel))
PIIX combinations
(1) acpi-root-pci-hotplug=yes acpi-pci-hotplug-with-bridge-support=yes
- All bridges have hotplug
(2) acpi-root-pci-hotplug=yes acpi-pci-hotplug-with-bridge-support=no
- No bridges have hotplug
(3) acpi-root-pci-hotplug=no acpi-pci-hotplug-with-bridge-support=yes
- All bridges except root have hotplug
requested by Promox guys, to battle against Windows 'feature' that lets any user to unplug sole NIC using an icon on taskbar. (Laine mentioned we have similar per port control for PCI-E ('hotplug' property) that was requested by other users probably for the same reason) so acpi-root-pci-hotplug is similar to pcie-root-port.hotplug with a difference that the former applies to whole root bus on PIIX4, where the later could be controlled per root port.
(4) acpi-root-pci-hotplug=no acpi-pci-hotplug-with-bridge-support=no
- No bridges have hotplug. Essentially identical to (2)
no (4) is not identical to (2). In (4) no hotplug is enabled. In (2) pci root bus still has hotplug enabled.
So you're saying that acpi-root-pci-hotplug=yes overrides the global request acpi-pci-hotplug-with-bridge-support=no and turns ACPI hotplug back on for the pcie-root
historically ACPI hotplug on root bus was always supported without any option, i.e. acpi-root-pci-hotplug=yes by default. acpi-pci-hotplug-with-bridge-support does what its name claims - i.e. adds hotplug for bridges (at least on PIIX4).
* Q35
clarification [*1*] still applies
- acpi-pci-hotplug-with-bridge-support=bool
Toggles support for ACPI based hotplug. If disabled native PCIe hotplug is activated instead
* pcie-root-port
- hotplug=bool
Toggles whether hotplug is enabled on the port. Affects either native and ACPI based hotplug, depending on what acpi-pci-hotplug-with-bridge-support=bool is set to ?
<target hotplug="on/off"> for pcie-root-port controller
- native_hotplug=bool
Can be used to also enable native hotplug, even when ACPI based hotplug is globally enabled. IOW only really relevant when acpi-pci-hotplug-with-bridge-support=true
Q35 combinations
(1) acpi-pci-hotplug-with-bridge-support=yes
name is a bit confusing depending on the point of view, but if one thinks about root/downstream-ports as bridges it fits just fine.
pcie-root-port.hotplug=yes pcie-root-port.native_hotplug=yes
it probably needs to say here what defaults are in place currently. maybe say it once above (1) for all Q35 section
ports have both ACPI + native hotplug, guest prefers native
are you sure about 'prefers'?
(2) acpi-pci-hotplug-with-bridge-support=yes
pcie-root-port.hotplug=no pcie-root-port.native_hotplug=yes
seems as invalid config, we should error out
ports have no hotplug, despite native_hotplug=yes IIUC
(3) acpi-pci-hotplug-with-bridge-support=yes pcie-root-port.hotplug=yes pcie-root-port.native_hotplug=no
ports have ACPI hotplug only
(4) acpi-pci-hotplug-with-bridge-support=yes pcie-root-port.hotplug=no pcie-root-port.native_hotplug=no
ports have no hotplug
(5) acpi-pci-hotplug-with-bridge-support=no pcie-root-port.hotplug=yes pcie-root-port.native_hotplug=yes
ports have native hotplug
(6) acpi-pci-hotplug-with-bridge-support=no
pcie-root-port.hotplug=no pcie-root-port.native_hotplug=yes
ditto
ports have no hotplug, despite native_hotplug=yes IIUC
(7) acpi-pci-hotplug-with-bridge-support=no pcie-root-port.hotplug=yes pcie-root-port.native_hotplug=no
ports have no hotplug, despite hotplug=yes IIUC
looks wrong too, but harder to check without picking into global acpi-pci-hotplug-with-bridge-support from 'random' 'port' device. not sure what to do here
(8) acpi-pci-hotplug-with-bridge-support=no pcie-root-port.hotplug=no pcie-root-port.native_hotplug=no
ports have no hotplug
Ani, it would be nice to have a similar doc in QEMU, lets say docs/pic_hoptplug.rst and maybe move there relevant sections from pcie.txt/pcie_pci_bridge.txt to have everything PCI hotplug related in one place.
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Regards, Daniel
On Wed, Sep 29, 2021 at 7:58 PM Igor Mammedov <imammedo@redhat.com> wrote:
On Tue, 28 Sep 2021 11:47:26 +0100 Daniel P. Berrangé <berrange@redhat.com> wrote:
On Tue, Sep 28, 2021 at 03:28:04PM +0530, Ani Sinha wrote:
On Tue, 28 Sep 2021, Daniel P. Berrangé wrote:
On Tue, Sep 28, 2021 at 02:35:47PM +0530, Ani Sinha wrote:
On Tue, 28 Sep 2021, Daniel P. Berrangé wrote:
On Sun, Sep 12, 2021 at 08:56:29AM +0530, Ani Sinha wrote: > This change introduces libvirt xml support for the following two pm options:
[...]
The <target hotplug="on/off"> switch in libvirt for pcie-root-ports currently does not care whether native or acpi hotplug is used. It simply turns on the hotplug for that particular port. Whether ACPI or native is used is controlled by this global flag that Julia has introduced in 6.1.
Right so we have
*1*) following applies to piix4/q35: * ACPI hotplug when enabled, affects _only_ cold-plugged 'bridges' since it requires 'slots' being described in DSDT table which in current impl. is static table built at reset time.
(i.e. built-in or 'bridges' specified on command line, where 'bridges' could be PCI-PCI or PCIe-PCI or root/downstream-ports') for anything else ('bridges' added with device_add) native hotplug is in use (whether it's SHPC or PCI-E native).
ACPI hotplug wiring is done by calling qbus_set_hotplug_handler() * for root bus piix4_pm_realize()/ich9_pm_init() * for anything else acpi_pcihp_device_plug_cb()
* PIIX4
- acpi-root-pci-hotplug=bool
Whether hotplug is enabled for the root bridge or not
<target hotplug="on/off"> for pci-root controller
- acpi-pci-hotplug-with-bridge-support=bool
Toggles support for ACPI based hotplug across all bridges. If disabled will there will be no hotplug at all for PIIX4 ? Or does 'shpc' come into play in that scenario ?
'SHPC' hotplug kicks in if it's enabled. (defaults to 'on' except 2.9 machine type)
on q35/APCI side of things we always advertise -all_ hotplug methods available
build_q35_osc_method() /* * Always allow native PME, AER (no dependencies) * Allow SHPC (PCI bridges can have SHPC controller) */ aml_append(if_ctx, aml_and(a_ctrl, aml_int(0x1F), a_ctrl));
bits 0, 1 are Native PCI-E hotplug and SHPC respectively
for PIIX4 we don't have _OSC so it's up to guest OS to make up supported methods.
In order of preference: * Windows supports ACPI hotplug then Native PCI-E (SHPC never worked there)
Hmm. from https://www.mail-archive.com/qemu-devel@nongnu.org/msg746673.html : Me:
Stupid question. If both native and acpi is enabled which one does OS chose? Does this choice vary between OSes and different flavours of the same OS like Windows?
Julia: It will always choose native. re: SHPC this is the reason I thought SHPC was disabled. In my experiments, Windows did not seem to care about SHPC.
* Linux supports ACPI hotplug, SHPC, Native PCI-E (SHPC worked poorly due to need to reserve IO for bridges io reservation hinting (impl. later by Marcel))
PIIX combinations
(1) acpi-root-pci-hotplug=yes acpi-pci-hotplug-with-bridge-support=yes
- All bridges have hotplug
(2) acpi-root-pci-hotplug=yes acpi-pci-hotplug-with-bridge-support=no
- No bridges have hotplug
(3) acpi-root-pci-hotplug=no acpi-pci-hotplug-with-bridge-support=yes
- All bridges except root have hotplug
requested by Promox guys, to battle against Windows 'feature' that lets any user to unplug sole NIC using an icon on taskbar.
I implemented that on qemu side :-)
(Laine mentioned we have similar per port control for PCI-E ('hotplug' property) that was requested by other users probably for the same reason)
so acpi-root-pci-hotplug is similar to pcie-root-port.hotplug with a difference that the former applies to whole root bus on PIIX4, where the later could be controlled per root port.
(4) acpi-root-pci-hotplug=no acpi-pci-hotplug-with-bridge-support=no
- No bridges have hotplug. Essentially identical to (2)
no (4) is not identical to (2). In (4) no hotplug is enabled. In (2) pci root bus still has hotplug enabled.
So you're saying that acpi-root-pci-hotplug=yes overrides the global request acpi-pci-hotplug-with-bridge-support=no and turns ACPI hotplug back on for the pcie-root
historically ACPI hotplug on root bus was always supported without any option, i.e. acpi-root-pci-hotplug=yes by default. acpi-pci-hotplug-with-bridge-support does what its name claims - i.e. adds hotplug for bridges (at least on PIIX4).
* Q35
clarification [*1*] still applies
- acpi-pci-hotplug-with-bridge-support=bool
Toggles support for ACPI based hotplug. If disabled native PCIe hotplug is activated instead
* pcie-root-port
- hotplug=bool
Toggles whether hotplug is enabled on the port. Affects either native and ACPI based hotplug, depending on what acpi-pci-hotplug-with-bridge-support=bool is set to ?
<target hotplug="on/off"> for pcie-root-port controller
- native_hotplug=bool
Can be used to also enable native hotplug, even when ACPI based hotplug is globally enabled. IOW only really relevant when acpi-pci-hotplug-with-bridge-support=true
Q35 combinations
(1) acpi-pci-hotplug-with-bridge-support=yes
name is a bit confusing depending on the point of view, but if one thinks about root/downstream-ports as bridges it fits just fine.
pcie-root-port.hotplug=yes pcie-root-port.native_hotplug=yes
it probably needs to say here what defaults are in place currently. maybe say it once above (1) for all Q35 section
ports have both ACPI + native hotplug, guest prefers native
are you sure about 'prefers'?
(2) acpi-pci-hotplug-with-bridge-support=yes
pcie-root-port.hotplug=no pcie-root-port.native_hotplug=yes
seems as invalid config, we should error out
ports have no hotplug, despite native_hotplug=yes IIUC
(3) acpi-pci-hotplug-with-bridge-support=yes pcie-root-port.hotplug=yes pcie-root-port.native_hotplug=no
ports have ACPI hotplug only
(4) acpi-pci-hotplug-with-bridge-support=yes pcie-root-port.hotplug=no pcie-root-port.native_hotplug=no
ports have no hotplug
(5) acpi-pci-hotplug-with-bridge-support=no pcie-root-port.hotplug=yes pcie-root-port.native_hotplug=yes
ports have native hotplug
(6) acpi-pci-hotplug-with-bridge-support=no
pcie-root-port.hotplug=no pcie-root-port.native_hotplug=yes
ditto
ports have no hotplug, despite native_hotplug=yes IIUC
(7) acpi-pci-hotplug-with-bridge-support=no pcie-root-port.hotplug=yes pcie-root-port.native_hotplug=no
ports have no hotplug, despite hotplug=yes IIUC
looks wrong too, but harder to check without picking into global acpi-pci-hotplug-with-bridge-support from 'random' 'port' device. not sure what to do here
(8) acpi-pci-hotplug-with-bridge-support=no pcie-root-port.hotplug=no pcie-root-port.native_hotplug=no
ports have no hotplug
Ani,
it would be nice to have a similar doc in QEMU, lets say docs/pic_hoptplug.rst and maybe move there relevant sections from pcie.txt/pcie_pci_bridge.txt to have everything PCI hotplug related in one place.
Yeah as I was discussing this here and on IRC with Laine, I realized this is all very convoluted. We should definitely document this. Will work on it after I am done with this patchset.
On 9/28/21 4:44 AM, Daniel P. Berrangé wrote:
On Sun, Sep 12, 2021 at 08:56:29AM +0530, Ani Sinha wrote:
This change introduces libvirt xml support for the following two pm options:
<pm> <acpi-hotplug-bridge enabled='no'/> <acpi-root-hotplug enabled='yes'/> </pm>
+``acpi-hotplug-bridge`` + :since:`Since 7.8.0` This option enables or disables BIOS ACPI based hotplug support + for cold plugged bridges. It is available only for x86 guests, both for q35 and pc + machine types. For pc machines, the support is available from `QEMU 2.12`. For q35 + machines, the support is available from `QEMU 6.1`. Examples of cold plugged bridges + include PCI-PCI bridges for pc machine types (pci-bridge controller). For q35 machines, + it includes PCIE root ports (pcie-root-port controller). This is a global option that + affects all bridges. No other bridge specific option is required to be specified.
Can you confirm my understanding of the situation..
- i440fx / PCI topology - hotplug always uses ACPI
- q35 / PCIe topology - hotplug historically used native PCIe hotplug, but in 6.1 switched to ACPI
Given, the name "acpi-hotplug-bridge", am I right that this option has *no* effect, if the q35 machine is using native PCIe hotplug approach ? IOW, is it a no-op until 6.1 based machine types for q35 ?
I *think* that in machinetypes where the default is native-pcie hotplug, setting acpi-hotplug-bridge=on will simultaneously enable ACPI hotplug and disable native-pcie hotplug for all pcie-root-ports and pcie-downstream-ports. Similarly on 6.1-based machinetypes, setting acpi-hotplug-bridge=off will disable ACPI hotplug and enable native-pcie hotplug. On 440fx, where the default has always been ACPI (and where SHPC hotplug has been disabled), acpi-hotplug-bridge=on will be a NOP, and acpi-hotplug-bridge=off will completely disable hotplug on any pci-bridge (but *not* on pci-root). As for the acpi-hotplug-root option, that is only valid for 440fx, is "on" by default, and when acpi-hotplug-root=off it will completely disable hotplug to any slot on pci-root. (for completeness - when a pcie-root-port or pcie-downstream-port has <target hotplug='off'/>, that will disable whatever hotplug mode would have been enabled for the controller - no hotplug at all will be possible on that controller. QEMU also has a "native_hotplug" option (not supported in libvirt) for pcie-root-ports and pcie-downstream-ports which can be used to enable native-pcie hotplug on a specific controller when it had been disabled (in favor of ACPI) with the global acpi-hotplug-bridge ).
On Tue, Sep 28, 2021 at 10:17 PM Laine Stump <laine@redhat.com> wrote:
On Sun, Sep 12, 2021 at 08:56:29AM +0530, Ani Sinha wrote:
This change introduces libvirt xml support for the following two pm
<pm> <acpi-hotplug-bridge enabled='no'/> <acpi-root-hotplug enabled='yes'/> </pm>
+``acpi-hotplug-bridge`` + :since:`Since 7.8.0` This option enables or disables BIOS ACPI
On 9/28/21 4:44 AM, Daniel P. Berrangé wrote: options: based hotplug support
+ for cold plugged bridges. It is available only for x86 guests, both for q35 and pc + machine types. For pc machines, the support is available from `QEMU 2.12`. For q35 + machines, the support is available from `QEMU 6.1`. Examples of
cold plugged bridges >> + include PCI-PCI bridges for pc machine types (pci-bridge controller). For q35 machines, >> + it includes PCIE root ports (pcie-root-port controller). This is a global option that >> + affects all bridges. No other bridge specific option is required to be specified. > > Can you confirm my understanding of the situation.. > > - i440fx / PCI topology - hotplug always uses ACPI > > - q35 / PCIe topology - hotplug historically used native PCIe hotplug, > but in 6.1 switched to ACPI > > Given, the name "acpi-hotplug-bridge", am I right that this option > has *no* effect, if the q35 machine is using native PCIe hotplug > approach ? IOW, is it a no-op until 6.1 based machine types for q35 ?
I *think* that in machinetypes where the default is native-pcie hotplug, setting acpi-hotplug-bridge=on
6.1 not only introduced this option/command line in Qemu but also flipped the switch to make ACPI hotplug default for pcie root ports. So there are no officially released version of Qemu where this command line exists and the default is native pcie hotplug. will simultaneously enable ACPI hotplug
and disable native-pcie hotplug for all pcie-root-ports and pcie-downstream-ports.
This is for 6.1 based machines as well. Similarly on 6.1-based machinetypes, setting
acpi-hotplug-bridge=off will disable ACPI hotplug and enable native-pcie hotplug.
On 440fx, where the default has always been ACPI (and where SHPC hotplug has been disabled), acpi-hotplug-bridge=on will be a NOP, and acpi-hotplug-bridge=off will completely disable hotplug on any pci-bridge (but *not* on pci-root).
As for the acpi-hotplug-root option, that is only valid for 440fx, is "on" by default, and when acpi-hotplug-root=off it will completely disable hotplug to any slot on pci-root.
(for completeness - when a pcie-root-port or pcie-downstream-port has <target hotplug='off'/>, that will disable whatever hotplug mode would have been enabled for the controller - no hotplug at all will be possible on that controller. QEMU also has a "native_hotplug" option (not supported in libvirt) for pcie-root-ports and pcie-downstream-ports which can be used to enable native-pcie hotplug on a specific controller when it had been disabled (in favor of ACPI) with the global acpi-hotplug-bridge ).
On 9/28/21 12:54 PM, Ani Sinha wrote:
On Tue, Sep 28, 2021 at 10:17 PM Laine Stump <laine@redhat.com <mailto:laine@redhat.com>> wrote:
On 9/28/21 4:44 AM, Daniel P. Berrangé wrote: > On Sun, Sep 12, 2021 at 08:56:29AM +0530, Ani Sinha wrote: >> This change introduces libvirt xml support for the following two pm options: >> >> <pm> >> <acpi-hotplug-bridge enabled='no'/> >> <acpi-root-hotplug enabled='yes'/> >> </pm> > > >> +``acpi-hotplug-bridge`` >> + :since:`Since 7.8.0` This option enables or disables BIOS ACPI based hotplug support >> + for cold plugged bridges. It is available only for x86 guests, both for q35 and pc >> + machine types. For pc machines, the support is available from `QEMU 2.12`. For q35 >> + machines, the support is available from `QEMU 6.1`. Examples of cold plugged bridges >> + include PCI-PCI bridges for pc machine types (pci-bridge controller). For q35 machines, >> + it includes PCIE root ports (pcie-root-port controller). This is a global option that >> + affects all bridges. No other bridge specific option is required to be specified. > > Can you confirm my understanding of the situation.. > > - i440fx / PCI topology - hotplug always uses ACPI > > - q35 / PCIe topology - hotplug historically used native PCIe hotplug, > but in 6.1 switched to ACPI > > Given, the name "acpi-hotplug-bridge", am I right that this option > has *no* effect, if the q35 machine is using native PCIe hotplug > approach ? IOW, is it a no-op until 6.1 based machine types for q35 ?
I *think* that in machinetypes where the default is native-pcie hotplug, setting acpi-hotplug-bridge=on
6.1 not only introduced this option/command line in Qemu but also flipped the switch to make ACPI hotplug default for pcie root ports. So there are no officially released version of Qemu where this command line exists and the default is native pcie hotplug.
You're assuming that everyone will use the canonical "q35" machinetype rather than a specific versioned machinetype (e.g. "pc-q35-6.0"). For pre-6.1 machinetypes, the default will still be native-pcie hotplug, even when running qemu-6.1+.
will simultaneously enable ACPI hotplug and disable native-pcie hotplug for all pcie-root-ports and pcie-downstream-ports.
This is for 6.1 based machines as well.
Similarly on 6.1-based machinetypes, setting acpi-hotplug-bridge=off will disable ACPI hotplug and enable native-pcie hotplug.
On 440fx, where the default has always been ACPI (and where SHPC hotplug has been disabled), acpi-hotplug-bridge=on will be a NOP, and acpi-hotplug-bridge=off will completely disable hotplug on any pci-bridge (but *not* on pci-root).
As for the acpi-hotplug-root option, that is only valid for 440fx, is "on" by default, and when acpi-hotplug-root=off it will completely disable hotplug to any slot on pci-root.
(for completeness - when a pcie-root-port or pcie-downstream-port has <target hotplug='off'/>, that will disable whatever hotplug mode would have been enabled for the controller - no hotplug at all will be possible on that controller. QEMU also has a "native_hotplug" option (not supported in libvirt) for pcie-root-ports and pcie-downstream-ports which can be used to enable native-pcie hotplug on a specific controller when it had been disabled (in favor of ACPI) with the global acpi-hotplug-bridge ).
On Tue, 28 Sep 2021, Laine Stump wrote:
On 9/28/21 12:54 PM, Ani Sinha wrote:
On Tue, Sep 28, 2021 at 10:17 PM Laine Stump <laine@redhat.com <mailto:laine@redhat.com>> wrote:
On 9/28/21 4:44 AM, Daniel P. Berrangé wrote: > On Sun, Sep 12, 2021 at 08:56:29AM +0530, Ani Sinha wrote: >> This change introduces libvirt xml support for the following two pm options: >> >> <pm> >> <acpi-hotplug-bridge enabled='no'/> >> <acpi-root-hotplug enabled='yes'/> >> </pm> > > >> +``acpi-hotplug-bridge`` >> + :since:`Since 7.8.0` This option enables or disables BIOS ACPI based hotplug support >> + for cold plugged bridges. It is available only for x86 guests, both for q35 and pc >> + machine types. For pc machines, the support is available from `QEMU 2.12`. For q35 >> + machines, the support is available from `QEMU 6.1`. Examples of cold plugged bridges >> + include PCI-PCI bridges for pc machine types (pci-bridge controller). For q35 machines, >> + it includes PCIE root ports (pcie-root-port controller). This is a global option that >> + affects all bridges. No other bridge specific option is required to be specified. > > Can you confirm my understanding of the situation.. > > - i440fx / PCI topology - hotplug always uses ACPI > > - q35 / PCIe topology - hotplug historically used native PCIe hotplug, > but in 6.1 switched to ACPI > > Given, the name "acpi-hotplug-bridge", am I right that this option > has *no* effect, if the q35 machine is using native PCIe hotplug > approach ? IOW, is it a no-op until 6.1 based machine types for q35 ?
I *think* that in machinetypes where the default is native-pcie hotplug, setting acpi-hotplug-bridge=on
6.1 not only introduced this option/command line in Qemu but also flipped the switch to make ACPI hotplug default for pcie root ports. So there are no officially released version of Qemu where this command line exists and the default is native pcie hotplug.
You're assuming that everyone will use the canonical "q35" machinetype rather than a specific versioned machinetype (e.g. "pc-q35-6.0"). For pre-6.1 machinetypes, the default will still be native-pcie hotplug, even when running qemu-6.1+.
Yes you are right. If one uses the 6.1 binary but specifies a compat pre 6.1 machine type then yes, native hotplug would be the default and acpi-hotplug-bridge will be off. If someone forces it on (not sure why they would though because they could simply use the 6.1 machine), the acpi hotplug would be enabled and native hotplug would get disabled on q35.
will simultaneously enable ACPI hotplug and disable native-pcie hotplug for all pcie-root-ports and pcie-downstream-ports.
This is for 6.1 based machines as well.
Similarly on 6.1-based machinetypes, setting acpi-hotplug-bridge=off will disable ACPI hotplug and enable native-pcie hotplug.
On 440fx, where the default has always been ACPI (and where SHPC hotplug has been disabled), acpi-hotplug-bridge=on will be a NOP, and acpi-hotplug-bridge=off will completely disable hotplug on any pci-bridge (but *not* on pci-root).
As for the acpi-hotplug-root option, that is only valid for 440fx, is "on" by default, and when acpi-hotplug-root=off it will completely disable hotplug to any slot on pci-root.
(for completeness - when a pcie-root-port or pcie-downstream-port has <target hotplug='off'/>, that will disable whatever hotplug mode would have been enabled for the controller - no hotplug at all will be possible on that controller. QEMU also has a "native_hotplug" option (not supported in libvirt) for pcie-root-ports and pcie-downstream-ports which can be used to enable native-pcie hotplug on a specific controller when it had been disabled (in favor of ACPI) with the global acpi-hotplug-bridge ).
This change adds backend qemu commandline support for two new libvirt global conf options: <pm> <acpi-hotplug-bridge enabled='no'/> <acpi-root-hotplug enabled='yes'/> </pm> Additionally it adds validations to make sure that qemu has the capabilities to support the above two options. The details of the above two options are provided in the commit log for the following commit: 69e01d2d3a50f ("conf: introduce acpi-hotplug-bridge and acpi-root-hotplug pm options") 'acpi-hotplug-bridge' turns on the following commandline option to qemu for x86 guests: (pc machines): -global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=<off/on> (q35 machines): -global ICH9-LPC.acpi-pci-hotplug-with-bridge-support=<off/on> 'acpi-root-hotplug' is only valid for pc machines and turns on the following commandline option 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. Also unit tests have been added to test qemu capability validation checks. Signed-off-by: Ani Sinha <ani@anisinha.ca> --- src/qemu/qemu_command.c | 28 +++++++++++++++++ src/qemu/qemu_validate.c | 21 +++++++++++++ ...pc-i440fx-acpi-hotplug-bridge-disable.args | 29 +++++++++++++++++ .../pc-i440fx-acpi-hotplug-bridge-enable.err | 1 + .../pc-i440fx-acpi-root-hotplug-disable.args | 29 +++++++++++++++++ .../pc-i440fx-acpi-root-hotplug-enable.err | 1 + .../q35-acpi-hotplug-bridge-disable.args | 31 +++++++++++++++++++ .../q35-acpi-hotplug-bridge-enable.err | 1 + tests/qemuxml2argvtest.c | 18 +++++++++++ 9 files changed, 159 insertions(+) create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.args create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-enable.err create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.args create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-enable.err create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.args create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-enable.err diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 4d29313f45..f23b9a7b1d 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6197,6 +6197,34 @@ qemuBuildPMCommandLine(virCommand *cmd, pm_object, def->pm.s4 == VIR_TRISTATE_BOOL_NO); } + if (def->pm.acpi_hp_bridge) { + const char *pm_object = "PIIX4_PM"; + const char *switch_str = "on"; + + if (def->pm.acpi_hp_bridge == VIR_TRISTATE_BOOL_NO) + switch_str = "off"; + + if (qemuDomainIsQ35(def) && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE)) + pm_object = "ICH9-LPC"; + + virCommandAddArg(cmd, "-global"); + virCommandAddArgFormat(cmd, "%s.acpi-pci-hotplug-with-bridge-support=%s", + pm_object, switch_str); + } + + if (def->pm.acpi_root_hp) { + const char *pm_object = "PIIX4_PM"; + const char *switch_str = "on"; + + if (def->pm.acpi_root_hp == VIR_TRISTATE_BOOL_NO) + switch_str = "off"; + + virCommandAddArg(cmd, "-global"); + virCommandAddArgFormat(cmd, "%s.acpi-root-pci-hotplug=%s", + pm_object, switch_str); + } + return 0; } diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 9d93f373ab..9bde0e1f0a 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -561,6 +561,27 @@ qemuValidateDomainDefPM(const virDomainDef *def, } } + if (def->pm.acpi_hp_bridge) { + bool q35ICH9_pcihpbr = q35Dom && + virQEMUCapsGet(qemuCaps, + QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE); + + if (!q35ICH9_pcihpbr && !virQEMUCapsGet(qemuCaps, + QEMU_CAPS_PIIX_ACPI_HOTPLUG_BRIDGE)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + "%s", _("setting ACPI hotplug bridge not supported")); + return -1; + } + } + + if (def->pm.acpi_root_hp) { + if (!q35Dom && !virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + "%s", _("setting ACPI root pci hotplug not supported")); + return -1; + } + } + return 0; } diff --git a/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.args b/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.args new file mode 100644 index 0000000000..1eed691975 --- /dev/null +++ b/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.args @@ -0,0 +1,29 @@ +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 \ +-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 \ +-global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=off \ +-usb \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x2 \ +-msg timestamp=on diff --git a/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-enable.err b/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-enable.err new file mode 100644 index 0000000000..98211b726f --- /dev/null +++ b/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-enable.err @@ -0,0 +1 @@ +unsupported configuration: setting ACPI hotplug bridge not supported 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..6f1e2814bf --- /dev/null +++ b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.args @@ -0,0 +1,29 @@ +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 \ +-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 \ +-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-enable.err b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-enable.err new file mode 100644 index 0000000000..c5c9de8389 --- /dev/null +++ b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-enable.err @@ -0,0 +1 @@ +unsupported configuration: setting ACPI root pci hotplug not supported diff --git a/tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.args b/tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.args new file mode 100644 index 0000000000..b88dd251b0 --- /dev/null +++ b/tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.args @@ -0,0 +1,31 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/tmp/lib/domain--1-q35 \ +USER=test \ +LOGNAME=test \ +XDG_DATA_HOME=/tmp/lib/domain--1-q35/.local/share \ +XDG_CACHE_HOME=/tmp/lib/domain--1-q35/.cache \ +XDG_CONFIG_HOME=/tmp/lib/domain--1-q35/.config \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-x86_64 \ +-name guest=q35,debug-threads=on \ +-S \ +-machine pc-q35-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-q35/monitor.sock,server=on,wait=off \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-no-acpi \ +-global ICH9-LPC.acpi-pci-hotplug-with-bridge-support=off \ +-device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x1e \ +-device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x0 \ +-device ioh3420,port=0x8,chassis=3,id=pci.3,bus=pcie.0,addr=0x1 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.2,addr=0x1 \ +-msg timestamp=on diff --git a/tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-enable.err b/tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-enable.err new file mode 100644 index 0000000000..98211b726f --- /dev/null +++ b/tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-enable.err @@ -0,0 +1 @@ +unsupported configuration: setting ACPI hotplug bridge not supported diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 74af93b08f..6e80b9cedf 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -2629,6 +2629,24 @@ 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("q35-acpi-hotplug-bridge-disable", + QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_IOH3420, + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, QEMU_CAPS_ICH9_AHCI, + QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE); + DO_TEST("pc-i440fx-acpi-hotplug-bridge-disable", + QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_IOH3420, + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, + QEMU_CAPS_PIIX_ACPI_HOTPLUG_BRIDGE); + DO_TEST("pc-i440fx-acpi-root-hotplug-disable", + QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_IOH3420, + QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, + QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG); + DO_TEST_PARSE_ERROR_NOCAPS("q35-acpi-hotplug-bridge-enable"); + DO_TEST_PARSE_ERROR_NOCAPS("pc-i440fx-acpi-hotplug-bridge-enable"); + DO_TEST_PARSE_ERROR_NOCAPS("pc-i440fx-acpi-root-hotplug-enable"); DO_TEST("q35-usb2", QEMU_CAPS_DEVICE_PCI_BRIDGE, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, -- 2.25.1
Added the following new libvirt conf options to the release note to indicate their availability with the next release: <pm> <acpi-hotplug-bridge enabled='no'/> <acpi-root-hotplug enabled='yes'/> </pm> The details of these two options are available in the following commit logs: e27d02694497a1 ("qemu: command: add support for qemu options that enables/disables acpi hotplug") c15daf3cd03efc ("conf: introduce acpi-hotplug-bridge and acpi-root-hotplug pm options") 0a09d090065735 ("qemu: capablities: detect presence of acpi-root-pci-hotplug for i440fx machines") c898e0d5bdf144 ("qemu: capablities: detect presence of acpi-pci-hotplug-with-bridge-support") Signed-off-by: Ani Sinha <ani@anisinha.ca> --- NEWS.rst | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/NEWS.rst b/NEWS.rst index 4521499db7..3f7f2c4d4d 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -17,6 +17,15 @@ v7.8.0 (unreleased) * **New features** + * Add support for two new global ACPI BIOS options controlling PCI hotplug + + Two new ``<pm>`` options have been added to allow users control BIOS ACPI + based PCI hotplug for x86 guests using qemu driver. + + ``acpi-hotplug-bridge`` enables or disables ACPI based hotplug for cold plugged + bridges. ``acpi-root-hotplug`` enables or disables ACPI based hotplug for pci + root bus. + * **Improvements** * **Bug fixes** -- 2.25.1
On Sun, 12 Sep 2021, Ani Sinha wrote:
Hi all:
This patchset introduces libvirt xml support for the following two pm conf options:
<pm> <acpi-hotplug-bridge enabled='no'/> <acpi-root-hotplug enabled='yes'/> </pm>
Another option is to create a new xml tag and add these under that tag. Something like: + <acpi-hotplug> + <acpi-hotplug-bridge enabled='no'/> + <acpi-root-hotplug enabled='yes'/> + </acpi-hotplug> These are not exactly power management options. So putting them under <pm> may not be correct. If this is a better approach I will resend the patchset with the changes along with addressing other review comments.
The above two options are only available for qemu driver and that too for x86 guests only. Both of them are global options.
``acpi-hotplug-bridge`` option enables or disables ACPI hotplug support for cold plugged bridges. Examples of cold plugged bridges include PCI-PCI bridge (pci-bridge controller) for pc machines and pcie-root-port controller for q35 machines. The corresponding commandline options to qemu for x86 guests are:
(pc machines): -global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=<off/on> (q35 machines): -global ICH9-LPC.acpi-pci-hotplug-with-bridge-support=<off/on>
Being global options, no other bridge specific options for pci-bridge controller or pcie-root-port controllers are required. For pc machine type in x86, this option is available in qemu for a long time, from version 2.1. Please see the changes in qemu.git:
9e047b982452c6 ("piix4: add acpi pci hotplug support") 133a2da488062e ("pc: acpi: generate AML only for PCI0 devices if PCI bridge hotplug is disabled")
For q35 machine type, this was introduced in qemu 6.1 with the following changes in qemu.git:
(a) c0e427d6eb5fef ("hw/acpi/ich9: Enable ACPI PCI hot-plug") (b) 17858a16950860 ("hw/acpi/ich9: Set ACPI PCI hot-plug as default on Q35")
The reasons for enabling ACPI based hotplug for PCIe (q35) based machines (as opposed to native hotplug) for bridges are outlined in (b). It is possible that some users might still want to use native hotplug on PCIe [1]. Therefore, this conf option enables users to choose either ACPI based hotplug or native hotplug for cold plugged bridges (for example for pcie root port controller in q35 machines).
``acpi-root-hotplug`` option enables or disables ACPI based hotplug for PCI root bus (pci-root controller). This option is only available for pc machine type. The corresponding commandline option to qemu for x86 guests is:
-global PIIX4_PM.acpi-root-pci-hotplug=<off/on>
This additional option enables users to disable hotplug for all devices in the system without adding an additional PCI-PCI bridge, putting the devices behind the bridge and using the existing ``acpi-hotplug-bridge`` option to disable hotplug on that bridge. This feature was introduced from qemu version 5.2 with the following change in qemu.git:
3d7e78aa7777f ("Introduce a new flag for i440fx to disable PCI hotplug on the root bus")
The above qemu commit describes some compelling reasons why users might to disable hotplug on PCI root buses [2].
A brief summary of the patches:
[PATCH v3 1/5] qemu: capablities: detect presence of [PATCH v3 2/5] qemu: capablities: detect presence of Patches 1 and 2 implement support for qemu capability checks for the above config options.
[PATCH v3 3/5] conf: introduce acpi-hotplug-bridge and Patch 3 actually adds the config option to the schema and adds related unit tests.
[PATCH v3 4/5] qemu: command: add support for qemu options that Patch 4 adds the backend qemu commandline support for the options. It also adds relevant unit tests for the same.
[PATCH v3 5/5] NEWS: add new acpi pci hotplug options in the release Patch 5 adds the release notes for the next libvirt release.
Changelog: v1: initial implementation. Had some bugs and missed some unit tests. v2: fixed bugs and added additional missing unit tests. 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.
Notes:
[1] One concrete example of why one might still want to use native hotplug with pcie-root-port controller is the fact that we are still discovering issues with acpi hotplug on PCIE. One such issue is: https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg02146.html Another reason is that users have been using native hotplug on pcie root ports up until now. They have built and tested their systems based on native hotplug. They may not want to suddenly move to acpi based hotplug just because it is now the default in qemu. Supporting the option to chose one or the other through libvirt makes things simpler for end users.
[2] 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.
[3] Finally, I implemented support for ``acpi-root-hotplug`` option in Qemu. Since adding the support for this option, I have not run away :-) I am still around, fixing other issues in the same subsystem in qemu and also now I have added myself as a reviewer of patches in this area. I will also be trying to support/maintain this new xml conf option in libvirt to the extent I can in future with the help of other experienced maintainers. Obviously this is all freelance work at this moment and is highly dependent on available free time.
Ping On Mon, Sep 13, 2021 at 1:39 PM Ani Sinha <ani@anisinha.ca> wrote:
On Sun, 12 Sep 2021, Ani Sinha wrote:
Hi all:
This patchset introduces libvirt xml support for the following two pm conf options:
<pm> <acpi-hotplug-bridge enabled='no'/> <acpi-root-hotplug enabled='yes'/> </pm>
Another option is to create a new xml tag and add these under that tag. Something like:
+ <acpi-hotplug> + <acpi-hotplug-bridge enabled='no'/> + <acpi-root-hotplug enabled='yes'/> + </acpi-hotplug>
These are not exactly power management options. So putting them under <pm> may not be correct. If this is a better approach I will resend the patchset with the changes along with addressing other review comments.
The above two options are only available for qemu driver and that too for x86 guests only. Both of them are global options.
``acpi-hotplug-bridge`` option enables or disables ACPI hotplug support for cold plugged bridges. Examples of cold plugged bridges include PCI-PCI bridge (pci-bridge controller) for pc machines and pcie-root-port controller for q35 machines. The corresponding commandline options to qemu for x86 guests are:
(pc machines): -global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=<off/on> (q35 machines): -global ICH9-LPC.acpi-pci-hotplug-with-bridge-support=<off/on>
Being global options, no other bridge specific options for pci-bridge controller or pcie-root-port controllers are required. For pc machine type in x86, this option is available in qemu for a long time, from version 2.1. Please see the changes in qemu.git:
9e047b982452c6 ("piix4: add acpi pci hotplug support") 133a2da488062e ("pc: acpi: generate AML only for PCI0 devices if PCI bridge hotplug is disabled")
For q35 machine type, this was introduced in qemu 6.1 with the following changes in qemu.git:
(a) c0e427d6eb5fef ("hw/acpi/ich9: Enable ACPI PCI hot-plug") (b) 17858a16950860 ("hw/acpi/ich9: Set ACPI PCI hot-plug as default on Q35")
The reasons for enabling ACPI based hotplug for PCIe (q35) based machines (as opposed to native hotplug) for bridges are outlined in (b). It is possible that some users might still want to use native hotplug on PCIe [1]. Therefore, this conf option enables users to choose either ACPI based hotplug or native hotplug for cold plugged bridges (for example for pcie root port controller in q35 machines).
``acpi-root-hotplug`` option enables or disables ACPI based hotplug for PCI root bus (pci-root controller). This option is only available for pc machine type. The corresponding commandline option to qemu for x86 guests is:
-global PIIX4_PM.acpi-root-pci-hotplug=<off/on>
This additional option enables users to disable hotplug for all devices in the system without adding an additional PCI-PCI bridge, putting the devices behind the bridge and using the existing ``acpi-hotplug-bridge`` option to disable hotplug on that bridge. This feature was introduced from qemu version 5.2 with the following change in qemu.git:
3d7e78aa7777f ("Introduce a new flag for i440fx to disable PCI hotplug on the root bus")
The above qemu commit describes some compelling reasons why users might to disable hotplug on PCI root buses [2].
A brief summary of the patches:
[PATCH v3 1/5] qemu: capablities: detect presence of [PATCH v3 2/5] qemu: capablities: detect presence of Patches 1 and 2 implement support for qemu capability checks for the above config options.
[PATCH v3 3/5] conf: introduce acpi-hotplug-bridge and Patch 3 actually adds the config option to the schema and adds related unit tests.
[PATCH v3 4/5] qemu: command: add support for qemu options that Patch 4 adds the backend qemu commandline support for the options. It also adds relevant unit tests for the same.
[PATCH v3 5/5] NEWS: add new acpi pci hotplug options in the release Patch 5 adds the release notes for the next libvirt release.
Changelog: v1: initial implementation. Had some bugs and missed some unit tests. v2: fixed bugs and added additional missing unit tests. 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.
Notes:
[1] One concrete example of why one might still want to use native hotplug with pcie-root-port controller is the fact that we are still discovering issues with acpi hotplug on PCIE. One such issue is: https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg02146.html Another reason is that users have been using native hotplug on pcie root ports up until now. They have built and tested their systems based on native hotplug. They may not want to suddenly move to acpi based hotplug just because it is now the default in qemu. Supporting the option to chose one or the other through libvirt makes things simpler for end users.
[2] The use case scenario described by Laine in
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
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
https://listman.redhat.com/archives/libvir-list/2020-February/msg00110.html this option
using libvirt passthrough feature as has been documented in
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.
[3] Finally, I implemented support for ``acpi-root-hotplug`` option in Qemu. Since adding the support for this option, I have not run away :-) I am still around, fixing other issues in the same subsystem in qemu and also now I have added myself as a reviewer of patches in this area. I will also be
http://blog.vmsplice.net/2011/04/how-to-pass-qemu-command-line-options.html trying to
support/maintain this new xml conf option in libvirt to the extent I can in future with the help of other experienced maintainers. Obviously this is all freelance work at this moment and is highly dependent on available free time.
Ping again On Mon, Sep 20, 2021 at 21:25 Ani Sinha <ani@anisinha.ca> wrote:
Ping
On Mon, Sep 13, 2021 at 1:39 PM Ani Sinha <ani@anisinha.ca> wrote:
On Sun, 12 Sep 2021, Ani Sinha wrote:
Hi all:
This patchset introduces libvirt xml support for the following two pm conf options:
<pm> <acpi-hotplug-bridge enabled='no'/> <acpi-root-hotplug enabled='yes'/> </pm>
Another option is to create a new xml tag and add these under that tag. Something like:
+ <acpi-hotplug> + <acpi-hotplug-bridge enabled='no'/> + <acpi-root-hotplug enabled='yes'/> + </acpi-hotplug>
These are not exactly power management options. So putting them under <pm> may not be correct. If this is a better approach I will resend the patchset with the changes along with addressing other review comments.
The above two options are only available for qemu driver and that too for x86 guests only. Both of them are global options.
``acpi-hotplug-bridge`` option enables or disables ACPI hotplug support for cold plugged bridges. Examples of cold plugged bridges include PCI-PCI bridge (pci-bridge controller) for pc machines and pcie-root-port controller for q35 machines. The corresponding commandline options to qemu for x86 guests are:
(pc machines): -global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=<off/on> (q35 machines): -global ICH9-LPC.acpi-pci-hotplug-with-bridge-support=<off/on>
Being global options, no other bridge specific options for pci-bridge controller or pcie-root-port controllers are required. For pc machine type in x86, this option is available in qemu for a long time, from version 2.1. Please see the changes in qemu.git:
9e047b982452c6 ("piix4: add acpi pci hotplug support") 133a2da488062e ("pc: acpi: generate AML only for PCI0 devices if PCI bridge hotplug is disabled")
For q35 machine type, this was introduced in qemu 6.1 with the following changes in qemu.git:
(a) c0e427d6eb5fef ("hw/acpi/ich9: Enable ACPI PCI hot-plug") (b) 17858a16950860 ("hw/acpi/ich9: Set ACPI PCI hot-plug as default on Q35")
The reasons for enabling ACPI based hotplug for PCIe (q35) based machines (as opposed to native hotplug) for bridges are outlined in (b). It is possible that some users might still want to use native hotplug on PCIe [1]. Therefore, this conf option enables users to choose either ACPI based hotplug or native hotplug for cold plugged bridges (for example for pcie root port controller in q35 machines).
``acpi-root-hotplug`` option enables or disables ACPI based hotplug for PCI root bus (pci-root controller). This option is only available for pc machine type. The corresponding commandline option to qemu for x86 guests is:
-global PIIX4_PM.acpi-root-pci-hotplug=<off/on>
This additional option enables users to disable hotplug for all devices in the system without adding an additional PCI-PCI bridge, putting the devices behind the bridge and using the existing ``acpi-hotplug-bridge`` option to disable hotplug on that bridge. This feature was introduced from qemu version 5.2 with the following change in qemu.git:
3d7e78aa7777f ("Introduce a new flag for i440fx to disable PCI hotplug on the root bus")
The above qemu commit describes some compelling reasons why users might to disable hotplug on PCI root buses [2].
A brief summary of the patches:
[PATCH v3 1/5] qemu: capablities: detect presence of [PATCH v3 2/5] qemu: capablities: detect presence of Patches 1 and 2 implement support for qemu capability checks for the above config options.
[PATCH v3 3/5] conf: introduce acpi-hotplug-bridge and Patch 3 actually adds the config option to the schema and adds related unit tests.
[PATCH v3 4/5] qemu: command: add support for qemu options that Patch 4 adds the backend qemu commandline support for the options. It also adds relevant unit tests for the same.
[PATCH v3 5/5] NEWS: add new acpi pci hotplug options in the release Patch 5 adds the release notes for the next libvirt release.
Changelog: v1: initial implementation. Had some bugs and missed some unit tests. v2: fixed bugs and added additional missing unit tests. 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.
Notes:
[1] One concrete example of why one might still want to use native hotplug with pcie-root-port controller is the fact that we are still discovering issues with acpi hotplug on PCIE. One such issue is: https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg02146.html Another reason is that users have been using native hotplug on pcie root ports up until now. They have built and tested their systems based on native hotplug. They may not want to suddenly move to acpi based hotplug just because it is now the default in qemu. Supporting the option to chose one or the other through libvirt makes things simpler for end users.
[2] The use case scenario described by Laine in
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
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
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
https://listman.redhat.com/archives/libvir-list/2020-February/msg00110.html this limitations. option
using libvirt passthrough feature as has been documented in
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.
[3] Finally, I implemented support for ``acpi-root-hotplug`` option in Qemu. Since adding the support for this option, I have not run away :-) I am still around, fixing other issues in the same subsystem in qemu and also now I have added myself as a reviewer of patches in this area. I will also be
http://blog.vmsplice.net/2011/04/how-to-pass-qemu-command-line-options.html trying to
support/maintain this new xml conf option in libvirt to the extent I can in future with the help of other experienced maintainers. Obviously this is all freelance work at this moment and is highly dependent on available free time.
On 9/11/21 11:26 PM, Ani Sinha wrote:
Hi all:
This patchset introduces libvirt xml support for the following two pm conf options:
<pm> <acpi-hotplug-bridge enabled='no'/> <acpi-root-hotplug enabled='yes'/> </pm>
(before I get into a more radical discussion about different options - since we aren't exactly duplicating the QEMU option name anyway, what if we made these names more consistent, e.g. "acpi-hotplug-bridge" and "acpi-hotplug-root"?) I've thought quite a bit about whether to put these attributes here, or somewhere else, and I'm still undecided. My initial reaction to this was "PM == Power Management, and power management is all about suspend mode support. Hotplug isn't power management." But then you look at the name of the QEMU option and PM is right there in the name, and I guess it's *kind of related* (effectively suspending/resuming a single device), so maybe I'm thinking too narrowly. So are there alternate places that might fit the purpose of these new options better, rather than directly mimicking the QEMU option placement (for better or worse)? A couple alternative possibilities: 1) **** One possibility would be to include these new flags within the existing <acpi> subelement of <features>, which is already used to control whether the guest exposes ACPI to the guest *at all* (via adding "-no-acpi" to the QEMU commandline when <acpi> is missing - NB: this feature flag is currently supported only on x86 and aarch64 QEMU platforms, and ignored for all other hypervisors). Possibly the new flags could be put in something like this: <features> <acpi> <hotplug-bridge enabled='no'/> <hotplug-root enabled='yes'/> </acpi> ... </features> But: * currently there are no subelements to <acpi>. So this isn't "extending according to an existing pattern". * even though the <features> element uses presence of a subelement to indicate "enabled" and absence of the subelement to indicate "disabled". But in the case of these new acpi bridge options we would need to explicitly have the "enabled='yes/no'" rather than just using presence of the option to mean "enabled" and absence to mean "disabled" because the default for "root-hotplug" up until now has been *enabled*, and the default for hotplug-bridge is different depending on machinetype. We need to continue working properly (and identically) with old/existing XML, but if we didn't have an "enabled" attribute for these new flags, there would be no way to tell the difference between "not specified" and "disabled", and so no way to disable the feature for a QEMU where the default was "enabled". (Why does this matter? Because I don't like the inconsistency that would arise from some feature flags using absense to mean "disabled" and some using it to mean "use the default".) * Having something in <features> in the domain XML kind of implies that the associated capability flags should be represented in the <features> section of the domain capabilities. For example, <acpi/> is listed under <features> in the output of virsh capabilities, separately from the flag indicating presence of the -no-acpi option. I'm not sure if we would need to add something there for these options if we moved them into <features> (seems a bit redundant to me to have it in both places, but I'm sure there are $reasons). 2) ***** Alternately, there is an <acpi> subelement of <os>, which is currently used to add a SLIC table (some sort of software license table, which I'd never heard of before) using QEMU's -acpitable commandline option. It is also used somehow by the Xen driver. <os> <acpi> <table type='slic'>/path/to/slic.dat</table> <hotplug-bridge enabled='no'/> <hotplug-root enabled='yes'/> </acpi> ... </os> My problem with adding these new PCI controller acpi options to os/acpi is simply that it's in the <os> subelement, which is claimed elsewhere to be intended for OS boot options, and is used for things like specifying the path to a kernel / initrd to boot from. 3) **** A third option, suggested somewhere by Ani, would be to make a completely new top-level element, called something like <acpiHotplug> that would have separate attributes for the two flags, e.g.: <acpiHotplug bridge='yes' root='yes'/> I dislike new toplevel options because they just seem so adhoc, as if the XML namespace is a cluttered, disorganized room. That reminds me too much of my own workspace, which is just... depressing. **** Since I always seem to spend *way too much time* worrying about naming, only to have it come out wrong in the end anyway, I'm looking for some other opinions. Counting the version that is in Ani's patch currently as option "0", which option do you all think is the best? Or is it completely unimportant?
The above two options are only available for qemu driver and that too for x86 guests only. Both of them are global options.
``acpi-hotplug-bridge`` option enables or disables ACPI hotplug support for cold plugged bridges. Examples of cold plugged bridges include PCI-PCI bridge (pci-bridge controller) for pc machines and pcie-root-port controller for q35 machines. The corresponding commandline options to qemu for x86 guests are:
The "cold plugged bridges" term here throws me for a loop - it implies that hotplugging bridges is something that's supported, and I think it still isn't. Of course this is just the cover letter, so it won't go into git anywhere, but I think it should be enough to say "enables ACPI hotplug into non-root bus PCI bridges/ports".
(pc machines): -global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=<off/on> (q35 machines): -global ICH9-LPC.acpi-pci-hotplug-with-bridge-support=<off/on>
So I'm curious - if the QEMU commandline also included "-no-acpi" along with these, what would happen? Would it be silently ignored? Generate an error? Or does -no-acpi only control the suspend support, and acpi hotplug is still available?
Being global options, no other bridge specific options for pci-bridge controller or pcie-root-port controllers are required. For pc machine type in x86, this option is available in qemu for a long time, from version 2.1. Please see the changes in qemu.git:
9e047b982452c6 ("piix4: add acpi pci hotplug support")
Interesting. So how was hotplug handled before this? With SHPC? I know there must be *some* kind of hotplug support in older QEMU, because RHEL6 QEMU supported hotplug, and it was based on qemu 0.12 or something ancient like that...
133a2da488062e ("pc: acpi: generate AML only for PCI0 devices if PCI bridge hotplug is disabled")
For q35 machine type, this was introduced in qemu 6.1 with the following changes in qemu.git:
(a) c0e427d6eb5fef ("hw/acpi/ich9: Enable ACPI PCI hot-plug") (b) 17858a16950860 ("hw/acpi/ich9: Set ACPI PCI hot-plug as default on Q35")
The reasons for enabling ACPI based hotplug for PCIe (q35) based machines (as opposed to native hotplug) for bridges are outlined in (b). It is possible that some users might still want to use native hotplug on PCIe [1]. Therefore, this conf option enables users to choose either ACPI based hotplug or native hotplug for cold plugged bridges (for example for pcie root port controller in q35 machines).
``acpi-root-hotplug`` option enables or disables ACPI based hotplug for PCI root bus (pci-root controller). This option is only available for pc machine type. The corresponding commandline option to qemu for x86 guests is:
-global PIIX4_PM.acpi-root-pci-hotplug=<off/on>
This additional option enables users to disable hotplug for all devices in the system without adding an additional PCI-PCI bridge, putting the devices behind the bridge and using the existing ``acpi-hotplug-bridge`` option to disable hotplug on that bridge. This feature was introduced from qemu version 5.2 with the following change in qemu.git:
3d7e78aa7777f ("Introduce a new flag for i440fx to disable PCI hotplug on the root bus")
The above qemu commit describes some compelling reasons why users might to disable hotplug on PCI root buses [2].
A brief summary of the patches:
[PATCH v3 1/5] qemu: capablities: detect presence of [PATCH v3 2/5] qemu: capablities: detect presence of Patches 1 and 2 implement support for qemu capability checks for the above config options.
[PATCH v3 3/5] conf: introduce acpi-hotplug-bridge and Patch 3 actually adds the config option to the schema and adds related unit tests.
[PATCH v3 4/5] qemu: command: add support for qemu options that Patch 4 adds the backend qemu commandline support for the options. It also adds relevant unit tests for the same.
[PATCH v3 5/5] NEWS: add new acpi pci hotplug options in the release Patch 5 adds the release notes for the next libvirt release.
Changelog: v1: initial implementation. Had some bugs and missed some unit tests. v2: fixed bugs and added additional missing unit tests. 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.
Notes:
[1] One concrete example of why one might still want to use native hotplug with pcie-root-port controller is the fact that we are still discovering issues with acpi hotplug on PCIE.
Yes, sigh. I recall someone saying something like "if we switch to ACPI hotplug then all these bugs just go away and everything works" or something like that. Reality never matches the ideal picture we put in our brains. At least ACPI hotplug is only the default on new machinetypes (doesn't help much for management platforms that always just use "q35" every time they start a guest). And it can also cause problems with distro-specific machinetypes in downstream distros when they are rebased: https://bugzilla.redhat.com/2006409
One such issue is: https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg02146.html Another reason is that users have been using native hotplug on pcie root ports up until now. They have built and tested their systems based on native hotplug. They may not want to suddenly move to acpi based hotplug just because it is now the default in qemu. Supporting the option to chose one or the other through libvirt makes things simpler for end users.
[2] 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.
To be fair, aside from "support for Win2000/WinXP", none of the items on the "limitations" page of that slide deck is something that's impossible to do with a Q35 machinetype; it's just that accomplishing some things may be more complicated. But I understand your point. Mainly I brought it up because I wanted to be sure that we're adding these to fulfill an actual need, rather than just adding bulk for the sake of completeness, or to satisfy curiosity.
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.
[3] Finally, I implemented support for ``acpi-root-hotplug`` option in Qemu. Since adding the support for this option, I have not run away :-) I am still around, fixing other issues in the same subsystem in qemu and also now I have added myself as a reviewer of patches in this area. I will also be trying to support/maintain this new xml conf option in libvirt to the extent I can in future with the help of other experienced maintainers. Obviously this is all freelance work at this moment and is highly dependent on available free time.
Since I don't follow qemu-devel closely, I didn't have prior knowledge of exactly what the options did, and it was unclear in the earlier versions of your patches that what <acpi-hotplug-bridge enabled='no'/> did was to disable ACPI hotplug for the entire guest (which on Q35 means that native PCIe hotplug will be found/used, and on 440fx means that hotplug won't be possible (unless SHPC hotplugged is enabled)). Your exaplanation and documentation in this spin of the patches makes that all clear though, so I'm beyond the "what does this do and do we need it?" stage to the "are there any problems with the code?" stage, and that's what I'll try to address in my review of the patches.
+Igor +Michael On Thu, 23 Sep 2021, Laine Stump wrote:
On 9/11/21 11:26 PM, Ani Sinha wrote:
Hi all:
This patchset introduces libvirt xml support for the following two pm conf options:
<pm> <acpi-hotplug-bridge enabled='no'/> <acpi-root-hotplug enabled='yes'/> </pm>
(before I get into a more radical discussion about different options - since we aren't exactly duplicating the QEMU option name anyway, what if we made these names more consistent, e.g. "acpi-hotplug-bridge" and "acpi-hotplug-root"?)
yes this is fine. I can swap the two words.
I've thought quite a bit about whether to put these attributes here, or somewhere else, and I'm still undecided.
My initial reaction to this was "PM == Power Management, and power management is all about suspend mode support. Hotplug isn't power management." But then you look at the name of the QEMU option and PM is right there in the name, and I guess it's *kind of related* (effectively suspending/resuming a single device), so maybe I'm thinking too narrowly.
So are there alternate places that might fit the purpose of these new options better, rather than directly mimicking the QEMU option placement (for better or worse)? A couple alternative possibilities:
1) ****
One possibility would be to include these new flags within the existing <acpi> subelement of <features>, which is already used to control whether the guest exposes ACPI to the guest *at all* (via adding "-no-acpi" to the QEMU commandline when <acpi> is missing - NB: this feature flag is currently supported only on x86 and aarch64 QEMU platforms, and ignored for all other hypervisors).
Possibly the new flags could be put in something like this:
<features> <acpi> <hotplug-bridge enabled='no'/> <hotplug-root enabled='yes'/> </acpi> ... </features>
But:
* currently there are no subelements to <acpi>. So this isn't "extending according to an existing pattern".
* even though the <features> element uses presence of a subelement to indicate "enabled" and absence of the subelement to indicate "disabled". But in the case of these new acpi bridge options we would need to explicitly have the "enabled='yes/no'" rather than just using presence of the option to mean "enabled" and absence to mean "disabled" because the default for "root-hotplug" up until now has been *enabled*, and the default for hotplug-bridge is different depending on machinetype. We need to continue working properly (and identically) with old/existing XML, but if we didn't have an "enabled" attribute for these new flags, there would be no way to tell the difference between "not specified" and "disabled", and so no way to disable the feature for a QEMU where the default was "enabled". (Why does this matter? Because I don't like the inconsistency that would arise from some feature flags using absense to mean "disabled" and some using it to mean "use the default".)
* Having something in <features> in the domain XML kind of implies that the associated capability flags should be represented in the <features> section of the domain capabilities. For example, <acpi/> is listed under <features> in the output of virsh capabilities, separately from the flag indicating presence of the -no-acpi option. I'm not sure if we would need to add something there for these options if we moved them into <features> (seems a bit redundant to me to have it in both places, but I'm sure there are $reasons).
2) *****
Alternately, there is an <acpi> subelement of <os>, which is currently used to add a SLIC table (some sort of software license table, which I'd never heard of before) using QEMU's -acpitable commandline option. It is also used somehow by the Xen driver.
<os> <acpi> <table type='slic'>/path/to/slic.dat</table> <hotplug-bridge enabled='no'/> <hotplug-root enabled='yes'/> </acpi> ... </os>
My problem with adding these new PCI controller acpi options to os/acpi is simply that it's in the <os> subelement, which is claimed elsewhere to be intended for OS boot options, and is used for things like specifying the path to a kernel / initrd to boot from.
3) ****
A third option, suggested somewhere by Ani, would be to make a completely new top-level element, called something like <acpiHotplug> that would have separate attributes for the two flags, e.g.:
<acpiHotplug bridge='yes' root='yes'/>
I dislike new toplevel options because they just seem so adhoc, as if the XML namespace is a cluttered, disorganized room. That reminds me too much of my own workspace, which is just... depressing.
****
Since I always seem to spend *way too much time* worrying about naming, only to have it come out wrong in the end anyway, I'm looking for some other opinions. Counting the version that is in Ani's patch currently as option "0", which option do you all think is the best? Or is it completely unimportant?
My preference is obviously option #0 and #3. However, community opinion/perspective is certainly required here.
The above two options are only available for qemu driver and that too for x86 guests only. Both of them are global options.
``acpi-hotplug-bridge`` option enables or disables ACPI hotplug support for cold plugged bridges. Examples of cold plugged bridges include PCI-PCI bridge (pci-bridge controller) for pc machines and pcie-root-port controller for q35 machines. The corresponding commandline options to qemu for x86 guests are:
The "cold plugged bridges" term here throws me for a loop - it implies that hotplugging bridges is something that's supported, and I think it still isn't. Of course this is just the cover letter, so it won't go into git anywhere, but I think it should be enough to say "enables ACPI hotplug into non-root bus PCI bridges/ports".
(pc machines): -global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=<off/on> (q35 machines): -global ICH9-LPC.acpi-pci-hotplug-with-bridge-support=<off/on>
So I'm curious - if the QEMU commandline also included "-no-acpi" along with these, what would happen? Would it be silently ignored? Generate an error? Or does -no-acpi only control the suspend support, and acpi hotplug is still available?
-no-acpi disables acpi completely from i386 machines. Please see acpi_setup() where we bail out of x86_machine_is_acpi_enabled() is false. So no support for any acpi based hotplug will be available. Those other options will be ignored.
Being global options, no other bridge specific options for pci-bridge controller or pcie-root-port controllers are required. For pc machine type in x86, this option is available in qemu for a long time, from version 2.1. Please see the changes in qemu.git:
9e047b982452c6 ("piix4: add acpi pci hotplug support")
Interesting. So how was hotplug handled before this? With SHPC? I know there must be *some* kind of hotplug support in older QEMU, because RHEL6 QEMU supported hotplug, and it was based on qemu 0.12 or something ancient like that...
good question. I do not know. may be imammeodo and mst (cc'd) can help here.
133a2da488062e ("pc: acpi: generate AML only for PCI0 devices if PCI bridge hotplug is disabled")
For q35 machine type, this was introduced in qemu 6.1 with the following changes in qemu.git:
(a) c0e427d6eb5fef ("hw/acpi/ich9: Enable ACPI PCI hot-plug") (b) 17858a16950860 ("hw/acpi/ich9: Set ACPI PCI hot-plug as default on Q35")
The reasons for enabling ACPI based hotplug for PCIe (q35) based machines (as opposed to native hotplug) for bridges are outlined in (b). It is possible that some users might still want to use native hotplug on PCIe [1]. Therefore, this conf option enables users to choose either ACPI based hotplug or native hotplug for cold plugged bridges (for example for pcie root port controller in q35 machines).
``acpi-root-hotplug`` option enables or disables ACPI based hotplug for PCI root bus (pci-root controller). This option is only available for pc machine type. The corresponding commandline option to qemu for x86 guests is:
-global PIIX4_PM.acpi-root-pci-hotplug=<off/on>
This additional option enables users to disable hotplug for all devices in the system without adding an additional PCI-PCI bridge, putting the devices behind the bridge and using the existing ``acpi-hotplug-bridge`` option to disable hotplug on that bridge. This feature was introduced from qemu version 5.2 with the following change in qemu.git:
3d7e78aa7777f ("Introduce a new flag for i440fx to disable PCI hotplug on the root bus")
The above qemu commit describes some compelling reasons why users might to disable hotplug on PCI root buses [2].
A brief summary of the patches:
[PATCH v3 1/5] qemu: capablities: detect presence of [PATCH v3 2/5] qemu: capablities: detect presence of Patches 1 and 2 implement support for qemu capability checks for the above config options.
[PATCH v3 3/5] conf: introduce acpi-hotplug-bridge and Patch 3 actually adds the config option to the schema and adds related unit tests.
[PATCH v3 4/5] qemu: command: add support for qemu options that Patch 4 adds the backend qemu commandline support for the options. It also adds relevant unit tests for the same.
[PATCH v3 5/5] NEWS: add new acpi pci hotplug options in the release Patch 5 adds the release notes for the next libvirt release.
Changelog: v1: initial implementation. Had some bugs and missed some unit tests. v2: fixed bugs and added additional missing unit tests. 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.
Notes:
[1] One concrete example of why one might still want to use native hotplug with pcie-root-port controller is the fact that we are still discovering issues with acpi hotplug on PCIE.
Yes, sigh. I recall someone saying something like "if we switch to ACPI hotplug then all these bugs just go away and everything works" or something like that. Reality never matches the ideal picture we put in our brains.
At least ACPI hotplug is only the default on new machinetypes (doesn't help much for management platforms that always just use "q35" every time they start a guest). And it can also cause problems with distro-specific machinetypes in downstream distros when they are rebased: https://bugzilla.redhat.com/2006409
Oh wow, what a tangled web! Yes, during the transition we might see some more issues until things get stable.
One such issue is: https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg02146.html Another reason is that users have been using native hotplug on pcie root ports up until now. They have built and tested their systems based on native hotplug. They may not want to suddenly move to acpi based hotplug just because it is now the default in qemu. Supporting the option to chose one or the other through libvirt makes things simpler for end users.
[2] 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.
To be fair, aside from "support for Win2000/WinXP", none of the items on the "limitations" page of that slide deck is something that's impossible to do with a Q35 machinetype; it's just that accomplishing some things may be more complicated. But I understand your point. Mainly I brought it up because I wanted to be sure that we're adding these to fulfill an actual need, rather than just adding bulk for the sake of completeness, or to satisfy curiosity.
Makes sense.
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.
[3] Finally, I implemented support for ``acpi-root-hotplug`` option in Qemu. Since adding the support for this option, I have not run away :-) I am still around, fixing other issues in the same subsystem in qemu and also now I have added myself as a reviewer of patches in this area. I will also be trying to support/maintain this new xml conf option in libvirt to the extent I can in future with the help of other experienced maintainers. Obviously this is all freelance work at this moment and is highly dependent on available free time.
Since I don't follow qemu-devel closely, I didn't have prior knowledge of exactly what the options did, and it was unclear in the earlier versions of your patches that what <acpi-hotplug-bridge enabled='no'/> did was to disable ACPI hotplug for the entire guest (which on Q35 means that native PCIe hotplug will be found/used, and on 440fx means that hotplug won't be possible (unless SHPC hotplugged is enabled)). Your exaplanation and documentation in this spin of the patches makes that all clear though, so I'm beyond the "what does this do and do we need it?" stage to the "are there any problems with the code?" stage, and that's what I'll try to address in my review of the patches.
Sounds good.
DanPB, Could you please reiterate the suggestion regarding the placement of the pci root hotplug xml you made in the irc channel? On Fri, Sep 24, 2021 at 02:16 Laine Stump <laine@redhat.com> wrote:
On 9/11/21 11:26 PM, Ani Sinha wrote:
Hi all:
This patchset introduces libvirt xml support for the following two pm conf options:
<pm> <acpi-hotplug-bridge enabled='no'/> <acpi-root-hotplug enabled='yes'/> </pm>
(before I get into a more radical discussion about different options - since we aren't exactly duplicating the QEMU option name anyway, what if we made these names more consistent, e.g. "acpi-hotplug-bridge" and "acpi-hotplug-root"?)
I've thought quite a bit about whether to put these attributes here, or somewhere else, and I'm still undecided.
My initial reaction to this was "PM == Power Management, and power management is all about suspend mode support. Hotplug isn't power management." But then you look at the name of the QEMU option and PM is right there in the name, and I guess it's *kind of related* (effectively suspending/resuming a single device), so maybe I'm thinking too narrowly.
So are there alternate places that might fit the purpose of these new options better, rather than directly mimicking the QEMU option placement (for better or worse)? A couple alternative possibilities:
1) ****
One possibility would be to include these new flags within the existing <acpi> subelement of <features>, which is already used to control whether the guest exposes ACPI to the guest *at all* (via adding "-no-acpi" to the QEMU commandline when <acpi> is missing - NB: this feature flag is currently supported only on x86 and aarch64 QEMU platforms, and ignored for all other hypervisors).
Possibly the new flags could be put in something like this:
<features> <acpi> <hotplug-bridge enabled='no'/> <hotplug-root enabled='yes'/> </acpi> ... </features>
But:
* currently there are no subelements to <acpi>. So this isn't "extending according to an existing pattern".
* even though the <features> element uses presence of a subelement to indicate "enabled" and absence of the subelement to indicate "disabled". But in the case of these new acpi bridge options we would need to explicitly have the "enabled='yes/no'" rather than just using presence of the option to mean "enabled" and absence to mean "disabled" because the default for "root-hotplug" up until now has been *enabled*, and the default for hotplug-bridge is different depending on machinetype. We need to continue working properly (and identically) with old/existing XML, but if we didn't have an "enabled" attribute for these new flags, there would be no way to tell the difference between "not specified" and "disabled", and so no way to disable the feature for a QEMU where the default was "enabled". (Why does this matter? Because I don't like the inconsistency that would arise from some feature flags using absense to mean "disabled" and some using it to mean "use the default".)
* Having something in <features> in the domain XML kind of implies that the associated capability flags should be represented in the <features> section of the domain capabilities. For example, <acpi/> is listed under <features> in the output of virsh capabilities, separately from the flag indicating presence of the -no-acpi option. I'm not sure if we would need to add something there for these options if we moved them into <features> (seems a bit redundant to me to have it in both places, but I'm sure there are $reasons).
2) *****
Alternately, there is an <acpi> subelement of <os>, which is currently used to add a SLIC table (some sort of software license table, which I'd never heard of before) using QEMU's -acpitable commandline option. It is also used somehow by the Xen driver.
<os> <acpi> <table type='slic'>/path/to/slic.dat</table> <hotplug-bridge enabled='no'/> <hotplug-root enabled='yes'/> </acpi> ... </os>
My problem with adding these new PCI controller acpi options to os/acpi is simply that it's in the <os> subelement, which is claimed elsewhere to be intended for OS boot options, and is used for things like specifying the path to a kernel / initrd to boot from.
3) ****
A third option, suggested somewhere by Ani, would be to make a completely new top-level element, called something like <acpiHotplug> that would have separate attributes for the two flags, e.g.:
<acpiHotplug bridge='yes' root='yes'/>
I dislike new toplevel options because they just seem so adhoc, as if the XML namespace is a cluttered, disorganized room. That reminds me too much of my own workspace, which is just... depressing.
****
Since I always seem to spend *way too much time* worrying about naming, only to have it come out wrong in the end anyway, I'm looking for some other opinions. Counting the version that is in Ani's patch currently as option "0", which option do you all think is the best? Or is it completely unimportant?
The above two options are only available for qemu driver and that too for x86 guests only. Both of them are global options.
``acpi-hotplug-bridge`` option enables or disables ACPI hotplug support for cold plugged bridges. Examples of cold plugged bridges include PCI-PCI bridge (pci-bridge controller) for pc machines and pcie-root-port controller for q35 machines. The corresponding commandline options to qemu for x86 guests are:
The "cold plugged bridges" term here throws me for a loop - it implies that hotplugging bridges is something that's supported, and I think it still isn't. Of course this is just the cover letter, so it won't go into git anywhere, but I think it should be enough to say "enables ACPI hotplug into non-root bus PCI bridges/ports".
(pc machines): -global
PIIX4_PM.acpi-pci-hotplug-with-bridge-support=<off/on>
(q35 machines): -global ICH9-LPC.acpi-pci-hotplug-with-bridge-support=<off/on>
So I'm curious - if the QEMU commandline also included "-no-acpi" along with these, what would happen? Would it be silently ignored? Generate an error? Or does -no-acpi only control the suspend support, and acpi hotplug is still available?
Being global options, no other bridge specific options for pci-bridge controller or pcie-root-port controllers are required. For pc machine
type in
x86, this option is available in qemu for a long time, from version 2.1. Please see the changes in qemu.git:
9e047b982452c6 ("piix4: add acpi pci hotplug support")
Interesting. So how was hotplug handled before this? With SHPC? I know there must be *some* kind of hotplug support in older QEMU, because RHEL6 QEMU supported hotplug, and it was based on qemu 0.12 or something ancient like that...
133a2da488062e ("pc: acpi: generate AML only for PCI0 devices if PCI bridge hotplug is disabled")
For q35 machine type, this was introduced in qemu 6.1 with the following changes in qemu.git:
(a) c0e427d6eb5fef ("hw/acpi/ich9: Enable ACPI PCI hot-plug") (b) 17858a16950860 ("hw/acpi/ich9: Set ACPI PCI hot-plug as default on Q35")
The reasons for enabling ACPI based hotplug for PCIe (q35) based machines (as opposed to native hotplug) for bridges are outlined in (b). It is possible that some users might still want to use native hotplug on PCIe [1]. Therefore, this conf option enables users to choose either ACPI based hotplug or native hotplug for cold plugged bridges (for example for pcie root port controller in q35 machines).
``acpi-root-hotplug`` option enables or disables ACPI based hotplug for PCI root bus (pci-root controller). This option is only available for pc machine type. The corresponding commandline option to qemu for x86 guests is:
-global PIIX4_PM.acpi-root-pci-hotplug=<off/on>
This additional option enables users to disable hotplug for all devices in the system without adding an additional PCI-PCI bridge, putting the devices behind the bridge and using the existing ``acpi-hotplug-bridge`` option to disable hotplug on that bridge. This feature was introduced from qemu version 5.2 with the following change in qemu.git:
3d7e78aa7777f ("Introduce a new flag for i440fx to disable PCI hotplug on the root bus")
The above qemu commit describes some compelling reasons why users might to disable hotplug on PCI root buses [2].
A brief summary of the patches:
[PATCH v3 1/5] qemu: capablities: detect presence of [PATCH v3 2/5] qemu: capablities: detect presence of Patches 1 and 2 implement support for qemu capability checks for the above config options.
[PATCH v3 3/5] conf: introduce acpi-hotplug-bridge and Patch 3 actually adds the config option to the schema and adds related unit tests.
[PATCH v3 4/5] qemu: command: add support for qemu options that Patch 4 adds the backend qemu commandline support for the options. It also adds relevant unit tests for the same.
[PATCH v3 5/5] NEWS: add new acpi pci hotplug options in the release Patch 5 adds the release notes for the next libvirt release.
Changelog: v1: initial implementation. Had some bugs and missed some unit tests. v2: fixed bugs and added additional missing unit tests. 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.
Notes:
[1] One concrete example of why one might still want to use native hotplug with pcie-root-port controller is the fact that we are still discovering issues with acpi hotplug on PCIE.
Yes, sigh. I recall someone saying something like "if we switch to ACPI hotplug then all these bugs just go away and everything works" or something like that. Reality never matches the ideal picture we put in our brains.
At least ACPI hotplug is only the default on new machinetypes (doesn't help much for management platforms that always just use "q35" every time they start a guest). And it can also cause problems with distro-specific machinetypes in downstream distros when they are rebased: https://bugzilla.redhat.com/2006409
One such issue is: https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg02146.html Another reason is that users have been using native hotplug on pcie root ports up until now. They have built and tested their systems based on native hotplug. They may not want to suddenly move to acpi based hotplug just because it is now the default in qemu. Supporting the option to chose one or the other through libvirt makes things simpler for end users.
[2] The use case scenario described by Laine in
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
https://listman.redhat.com/archives/libvir-list/2020-February/msg00110.html 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.
To be fair, aside from "support for Win2000/WinXP", none of the items on the "limitations" page of that slide deck is something that's impossible to do with a Q35 machinetype; it's just that accomplishing some things may be more complicated. But I understand your point. Mainly I brought it up because I wanted to be sure that we're adding these to fulfill an actual need, rather than just adding bulk for the sake of completeness, or to satisfy curiosity.
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
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.
[3] Finally, I implemented support for ``acpi-root-hotplug`` option in Qemu. Since adding the support for this option, I have not run away :-) I am still around, fixing other issues in the same subsystem in qemu and also now I have added myself as a reviewer of patches in this area. I will also be
http://blog.vmsplice.net/2011/04/how-to-pass-qemu-command-line-options.html trying to
support/maintain this new xml conf option in libvirt to the extent I can in future with the help of other experienced maintainers. Obviously this is all freelance work at this moment and is highly dependent on available free time.
Since I don't follow qemu-devel closely, I didn't have prior knowledge of exactly what the options did, and it was unclear in the earlier versions of your patches that what <acpi-hotplug-bridge enabled='no'/> did was to disable ACPI hotplug for the entire guest (which on Q35 means that native PCIe hotplug will be found/used, and on 440fx means that hotplug won't be possible (unless SHPC hotplugged is enabled)). Your exaplanation and documentation in this spin of the patches makes that all clear though, so I'm beyond the "what does this do and do we need it?" stage to the "are there any problems with the code?" stage, and that's what I'll try to address in my review of the patches.
On 9/23/21 4:46 PM, Laine Stump wrote:
On 9/11/21 11:26 PM, Ani Sinha wrote:
Hi all:
This patchset introduces libvirt xml support for the following two pm conf options:
<pm> <acpi-hotplug-bridge enabled='no'/> <acpi-root-hotplug enabled='yes'/> </pm>
(before I get into a more radical discussion about different options - since we aren't exactly duplicating the QEMU option name anyway, what if we made these names more consistent, e.g. "acpi-hotplug-bridge" and "acpi-hotplug-root"?)
I've thought quite a bit about whether to put these attributes here, or somewhere else, and I'm still undecided.
My initial reaction to this was "PM == Power Management, and power management is all about suspend mode support. Hotplug isn't power management." But then you look at the name of the QEMU option and PM is right there in the name, and I guess it's *kind of related* (effectively suspending/resuming a single device), so maybe I'm thinking too narrowly.
So are there alternate places that might fit the purpose of these new options better, rather than directly mimicking the QEMU option placement (for better or worse)? A couple alternative possibilities:
1) ****
One possibility would be to include these new flags within the existing <acpi> subelement of <features>, which is already used to control whether the guest exposes ACPI to the guest *at all* (via adding "-no-acpi" to the QEMU commandline when <acpi> is missing - NB: this feature flag is currently supported only on x86 and aarch64 QEMU platforms, and ignored for all other hypervisors).
Possibly the new flags could be put in something like this:
<features> <acpi> <hotplug-bridge enabled='no'/> <hotplug-root enabled='yes'/> </acpi> ... </features>
But:
* currently there are no subelements to <acpi>. So this isn't "extending according to an existing pattern".
* even though the <features> element uses presence of a subelement to indicate "enabled" and absence of the subelement to indicate "disabled". But in the case of these new acpi bridge options we would need to explicitly have the "enabled='yes/no'" rather than just using presence of the option to mean "enabled" and absence to mean "disabled" because the default for "root-hotplug" up until now has been *enabled*, and the default for hotplug-bridge is different depending on machinetype. We need to continue working properly (and identically) with old/existing XML, but if we didn't have an "enabled" attribute for these new flags, there would be no way to tell the difference between "not specified" and "disabled", and so no way to disable the feature for a QEMU where the default was "enabled". (Why does this matter? Because I don't like the inconsistency that would arise from some feature flags using absense to mean "disabled" and some using it to mean "use the default".)
* Having something in <features> in the domain XML kind of implies that the associated capability flags should be represented in the <features> section of the domain capabilities. For example, <acpi/> is listed under <features> in the output of virsh capabilities, separately from the flag indicating presence of the -no-acpi option. I'm not sure if we would need to add something there for these options if we moved them into <features> (seems a bit redundant to me to have it in both places, but I'm sure there are $reasons).
2) *****
Alternately, there is an <acpi> subelement of <os>, which is currently used to add a SLIC table (some sort of software license table, which I'd never heard of before) using QEMU's -acpitable commandline option. It is also used somehow by the Xen driver.
<os> <acpi> <table type='slic'>/path/to/slic.dat</table> <hotplug-bridge enabled='no'/> <hotplug-root enabled='yes'/> </acpi> ... </os>
My problem with adding these new PCI controller acpi options to os/acpi is simply that it's in the <os> subelement, which is claimed elsewhere to be intended for OS boot options, and is used for things like specifying the path to a kernel / initrd to boot from.
3) ****
A third option, suggested somewhere by Ani, would be to make a completely new top-level element, called something like <acpiHotplug> that would have separate attributes for the two flags, e.g.:
<acpiHotplug bridge='yes' root='yes'/>
I dislike new toplevel options because they just seem so adhoc, as if the XML namespace is a cluttered, disorganized room. That reminds me too much of my own workspace, which is just... depressing.
****
Since I always seem to spend *way too much time* worrying about naming, only to have it come out wrong in the end anyway, I'm looking for some other opinions. Counting the version that is in Ani's patch currently as option "0", which option do you all think is the best? Or is it completely unimportant?
In an IRC discussion, danpb suggested what I'll label as option (4): 4) **** <danpb1> laine: i didn't have time to reply,but was thinking why it isn't a property of the pci(e)-root <controller> That makes perfect sense for acpi-hotplug-root: <controller type='pci' index='0' model='pci-root'> <target hotplug='off'/> </controller> This is the same attribute used to disable all hotplug for pcie-root-ports (and downstream ports, I guess - I never use those and recall a bug being filed about failures to hotplug devices on a downstream port; but I digress...). So it fits logically. (I will point out though that normally the options within a device's XML element are added to the qemu commandline as a part of that devices "-device", not as a separate global option as happens with this (for that matter there is no -device added to the commandline for pci-root or pcie-root at all - they're just implicit in the base machine).) But the "acpi-hotplug-bridge" option doesn't really fit as an attribute of pci-root/pcie-root - imagine for a moment that you had this option in a pcie-root controller, it would look something like this: <controller type='pci' index='0' model='pcie-root'> <target acpiHotplugBridge='on'/> </controller> Note that in this case, the option turns on *ACPI* hotplug support *for other PCI controllers*, **NOT** this controller (and as a side effect, also turns *off* PCIe native hotplug for all controllers, which can then be turned on/off on a per controller basis using QEMU's native_hotplug commandline option, which isn't supported by libvirt). Another issue - with this - putting this option into the XML of the pcie-root controller and saying that it applies to "the other controllers in the PCI hierarchy" implies that if there was a separate pcie-root (for example a pxb-pcie controller) then the option wouldn't apply to bridges that were a part of that separate hierarchy, and I believe that is *not* the case. So while I like controlling acpi-hotplug-root with <target hotplug='on|off'/> inside the pci-root's element, I don't really like putting acpi-hotplug-bridge in the pci(e)-root controller's element.
Ok then let me do this. I will split up this patch set and send out a separate patch set just for acpi-hotplug-root with the conf change as suggested by danpb. It makes sense for me too to go down the path of his suggestion. Meanwhile let's figure out what we wanted to do for acpi-hotplug-bridge. That could be posted as a separate patch set. Sounds good? On Sat, Sep 25, 2021 at 8:29 PM Laine Stump <laine@redhat.com> wrote:
On 9/23/21 4:46 PM, Laine Stump wrote:
On 9/11/21 11:26 PM, Ani Sinha wrote:
Hi all:
This patchset introduces libvirt xml support for the following two pm conf options:
<pm> <acpi-hotplug-bridge enabled='no'/> <acpi-root-hotplug enabled='yes'/> </pm>
(before I get into a more radical discussion about different options - since we aren't exactly duplicating the QEMU option name anyway, what if we made these names more consistent, e.g. "acpi-hotplug-bridge" and "acpi-hotplug-root"?)
I've thought quite a bit about whether to put these attributes here, or somewhere else, and I'm still undecided.
My initial reaction to this was "PM == Power Management, and power management is all about suspend mode support. Hotplug isn't power management." But then you look at the name of the QEMU option and PM is right there in the name, and I guess it's *kind of related* (effectively suspending/resuming a single device), so maybe I'm thinking too narrowly.
So are there alternate places that might fit the purpose of these new options better, rather than directly mimicking the QEMU option placement (for better or worse)? A couple alternative possibilities:
1) ****
One possibility would be to include these new flags within the existing <acpi> subelement of <features>, which is already used to control whether the guest exposes ACPI to the guest *at all* (via adding "-no-acpi" to the QEMU commandline when <acpi> is missing - NB: this feature flag is currently supported only on x86 and aarch64 QEMU platforms, and ignored for all other hypervisors).
Possibly the new flags could be put in something like this:
<features> <acpi> <hotplug-bridge enabled='no'/> <hotplug-root enabled='yes'/> </acpi> ... </features>
But:
* currently there are no subelements to <acpi>. So this isn't "extending according to an existing pattern".
* even though the <features> element uses presence of a subelement to indicate "enabled" and absence of the subelement to indicate "disabled". But in the case of these new acpi bridge options we would need to explicitly have the "enabled='yes/no'" rather than just using presence of the option to mean "enabled" and absence to mean "disabled" because the default for "root-hotplug" up until now has been *enabled*, and the default for hotplug-bridge is different depending on machinetype. We need to continue working properly (and identically) with old/existing XML, but if we didn't have an "enabled" attribute for these new flags, there would be no way to tell the difference between "not specified" and "disabled", and so no way to disable the feature for a QEMU where the default was "enabled". (Why does this matter? Because I don't like the inconsistency that would arise from some feature flags using absense to mean "disabled" and some using it to mean "use the default".)
* Having something in <features> in the domain XML kind of implies that the associated capability flags should be represented in the <features> section of the domain capabilities. For example, <acpi/> is listed under <features> in the output of virsh capabilities, separately from the flag indicating presence of the -no-acpi option. I'm not sure if we would need to add something there for these options if we moved them into <features> (seems a bit redundant to me to have it in both places, but I'm sure there are $reasons).
2) *****
Alternately, there is an <acpi> subelement of <os>, which is currently used to add a SLIC table (some sort of software license table, which I'd never heard of before) using QEMU's -acpitable commandline option. It is also used somehow by the Xen driver.
<os> <acpi> <table type='slic'>/path/to/slic.dat</table> <hotplug-bridge enabled='no'/> <hotplug-root enabled='yes'/> </acpi> ... </os>
My problem with adding these new PCI controller acpi options to os/acpi is simply that it's in the <os> subelement, which is claimed elsewhere to be intended for OS boot options, and is used for things like specifying the path to a kernel / initrd to boot from.
3) ****
A third option, suggested somewhere by Ani, would be to make a completely new top-level element, called something like <acpiHotplug> that would have separate attributes for the two flags, e.g.:
<acpiHotplug bridge='yes' root='yes'/>
I dislike new toplevel options because they just seem so adhoc, as if the XML namespace is a cluttered, disorganized room. That reminds me too much of my own workspace, which is just... depressing.
****
Since I always seem to spend *way too much time* worrying about naming, only to have it come out wrong in the end anyway, I'm looking for some other opinions. Counting the version that is in Ani's patch currently as option "0", which option do you all think is the best? Or is it completely unimportant?
In an IRC discussion, danpb suggested what I'll label as option (4):
4) ****
<danpb1> laine: i didn't have time to reply,but was thinking why it isn't a property of the pci(e)-root <controller>
That makes perfect sense for acpi-hotplug-root:
<controller type='pci' index='0' model='pci-root'> <target hotplug='off'/> </controller>
This is the same attribute used to disable all hotplug for pcie-root-ports (and downstream ports, I guess - I never use those and recall a bug being filed about failures to hotplug devices on a downstream port; but I digress...). So it fits logically.
(I will point out though that normally the options within a device's XML element are added to the qemu commandline as a part of that devices "-device", not as a separate global option as happens with this (for that matter there is no -device added to the commandline for pci-root or pcie-root at all - they're just implicit in the base machine).)
But the "acpi-hotplug-bridge" option doesn't really fit as an attribute of pci-root/pcie-root - imagine for a moment that you had this option in a pcie-root controller, it would look something like this:
<controller type='pci' index='0' model='pcie-root'> <target acpiHotplugBridge='on'/> </controller>
Note that in this case, the option turns on *ACPI* hotplug support *for other PCI controllers*, **NOT** this controller (and as a side effect, also turns *off* PCIe native hotplug for all controllers, which can then be turned on/off on a per controller basis using QEMU's native_hotplug commandline option, which isn't supported by libvirt).
Another issue - with this - putting this option into the XML of the pcie-root controller and saying that it applies to "the other controllers in the PCI hierarchy" implies that if there was a separate pcie-root (for example a pxb-pcie controller) then the option wouldn't apply to bridges that were a part of that separate hierarchy, and I believe that is *not* the case.
So while I like controlling acpi-hotplug-root with <target hotplug='on|off'/> inside the pci-root's element, I don't really like putting acpi-hotplug-bridge in the pci(e)-root controller's element.
On Sat, Sep 25, 2021 at 8:39 PM Ani Sinha <ani@anisinha.ca> wrote:
Ok then let me do this. I will split up this patch set and send out a separate patch set just for acpi-hotplug-root with the conf change as suggested by danpb. It makes sense for me too to go down the path of his suggestion.
Meanwhile let's figure out what we wanted to do for acpi-hotplug-bridge. That could be posted as a separate patch set.
Sounds good?
OK, while we have split this up and are making progress with the pci-root-hotplug patchset, have we decided where to put the conf for pci-hotplug-bridge? Should we leave it as it is under <pm>?
On Sat, Sep 25, 2021 at 8:29 PM Laine Stump <laine@redhat.com> wrote:
On 9/23/21 4:46 PM, Laine Stump wrote:
On 9/11/21 11:26 PM, Ani Sinha wrote:
Hi all:
This patchset introduces libvirt xml support for the following two pm conf options:
<pm> <acpi-hotplug-bridge enabled='no'/> <acpi-root-hotplug enabled='yes'/> </pm>
(before I get into a more radical discussion about different options - since we aren't exactly duplicating the QEMU option name anyway, what if we made these names more consistent, e.g. "acpi-hotplug-bridge" and "acpi-hotplug-root"?)
I've thought quite a bit about whether to put these attributes here, or somewhere else, and I'm still undecided.
My initial reaction to this was "PM == Power Management, and power management is all about suspend mode support. Hotplug isn't power management." But then you look at the name of the QEMU option and PM is right there in the name, and I guess it's *kind of related* (effectively suspending/resuming a single device), so maybe I'm thinking too narrowly.
So are there alternate places that might fit the purpose of these new options better, rather than directly mimicking the QEMU option placement (for better or worse)? A couple alternative possibilities:
1) ****
One possibility would be to include these new flags within the existing <acpi> subelement of <features>, which is already used to control whether the guest exposes ACPI to the guest *at all* (via adding "-no-acpi" to the QEMU commandline when <acpi> is missing - NB: this feature flag is currently supported only on x86 and aarch64 QEMU platforms, and ignored for all other hypervisors).
Possibly the new flags could be put in something like this:
<features> <acpi> <hotplug-bridge enabled='no'/> <hotplug-root enabled='yes'/> </acpi> ... </features>
But:
* currently there are no subelements to <acpi>. So this isn't "extending according to an existing pattern".
* even though the <features> element uses presence of a subelement to indicate "enabled" and absence of the subelement to indicate "disabled". But in the case of these new acpi bridge options we would need to explicitly have the "enabled='yes/no'" rather than just using presence of the option to mean "enabled" and absence to mean "disabled" because the default for "root-hotplug" up until now has been *enabled*, and the default for hotplug-bridge is different depending on machinetype. We need to continue working properly (and identically) with old/existing XML, but if we didn't have an "enabled" attribute for these new flags, there would be no way to tell the difference between "not specified" and "disabled", and so no way to disable the feature for a QEMU where the default was "enabled". (Why does this matter? Because I don't like the inconsistency that would arise from some feature flags using absense to mean "disabled" and some using it to mean "use the default".)
* Having something in <features> in the domain XML kind of implies that the associated capability flags should be represented in the <features> section of the domain capabilities. For example, <acpi/> is listed under <features> in the output of virsh capabilities, separately from the flag indicating presence of the -no-acpi option. I'm not sure if we would need to add something there for these options if we moved them into <features> (seems a bit redundant to me to have it in both places, but I'm sure there are $reasons).
2) *****
Alternately, there is an <acpi> subelement of <os>, which is currently used to add a SLIC table (some sort of software license table, which I'd never heard of before) using QEMU's -acpitable commandline option. It is also used somehow by the Xen driver.
<os> <acpi> <table type='slic'>/path/to/slic.dat</table> <hotplug-bridge enabled='no'/> <hotplug-root enabled='yes'/> </acpi> ... </os>
My problem with adding these new PCI controller acpi options to os/acpi is simply that it's in the <os> subelement, which is claimed elsewhere to be intended for OS boot options, and is used for things like specifying the path to a kernel / initrd to boot from.
3) ****
A third option, suggested somewhere by Ani, would be to make a completely new top-level element, called something like <acpiHotplug> that would have separate attributes for the two flags, e.g.:
<acpiHotplug bridge='yes' root='yes'/>
I dislike new toplevel options because they just seem so adhoc, as if the XML namespace is a cluttered, disorganized room. That reminds me too much of my own workspace, which is just... depressing.
****
Since I always seem to spend *way too much time* worrying about naming, only to have it come out wrong in the end anyway, I'm looking for some other opinions. Counting the version that is in Ani's patch currently as option "0", which option do you all think is the best? Or is it completely unimportant?
In an IRC discussion, danpb suggested what I'll label as option (4):
4) ****
<danpb1> laine: i didn't have time to reply,but was thinking why it isn't a property of the pci(e)-root <controller>
That makes perfect sense for acpi-hotplug-root:
<controller type='pci' index='0' model='pci-root'> <target hotplug='off'/> </controller>
This is the same attribute used to disable all hotplug for pcie-root-ports (and downstream ports, I guess - I never use those and recall a bug being filed about failures to hotplug devices on a downstream port; but I digress...). So it fits logically.
(I will point out though that normally the options within a device's XML element are added to the qemu commandline as a part of that devices "-device", not as a separate global option as happens with this (for that matter there is no -device added to the commandline for pci-root or pcie-root at all - they're just implicit in the base machine).)
But the "acpi-hotplug-bridge" option doesn't really fit as an attribute of pci-root/pcie-root - imagine for a moment that you had this option in a pcie-root controller, it would look something like this:
<controller type='pci' index='0' model='pcie-root'> <target acpiHotplugBridge='on'/> </controller>
Note that in this case, the option turns on *ACPI* hotplug support *for other PCI controllers*, **NOT** this controller (and as a side effect, also turns *off* PCIe native hotplug for all controllers, which can then be turned on/off on a per controller basis using QEMU's native_hotplug commandline option, which isn't supported by libvirt).
Another issue - with this - putting this option into the XML of the pcie-root controller and saying that it applies to "the other controllers in the PCI hierarchy" implies that if there was a separate pcie-root (for example a pxb-pcie controller) then the option wouldn't apply to bridges that were a part of that separate hierarchy, and I believe that is *not* the case.
So while I like controlling acpi-hotplug-root with <target hotplug='on|off'/> inside the pci-root's element, I don't really like putting acpi-hotplug-bridge in the pci(e)-root controller's element.
On Thu, Sep 23, 2021 at 04:46:38PM -0400, Laine Stump wrote:
On 9/11/21 11:26 PM, Ani Sinha wrote:
Hi all:
This patchset introduces libvirt xml support for the following two pm conf options:
<pm> <acpi-hotplug-bridge enabled='no'/> <acpi-root-hotplug enabled='yes'/> </pm>
(before I get into a more radical discussion about different options - since we aren't exactly duplicating the QEMU option name anyway, what if we made these names more consistent, e.g. "acpi-hotplug-bridge" and "acpi-hotplug-root"?)
I've thought quite a bit about whether to put these attributes here, or somewhere else, and I'm still undecided.
My initial reaction to this was "PM == Power Management, and power management is all about suspend mode support. Hotplug isn't power management." But then you look at the name of the QEMU option and PM is right there in the name, and I guess it's *kind of related* (effectively suspending/resuming a single device), so maybe I'm thinking too narrowly.
I had the same reaction. Even if QEMU hangs it off a "_PM" device, I feel it is a pretty wierd location from libvirt POV to put this.
So are there alternate places that might fit the purpose of these new options better, rather than directly mimicking the QEMU option placement (for better or worse)? A couple alternative possibilities:
1) ****
One possibility would be to include these new flags within the existing <acpi> subelement of <features>, which is already used to control whether the guest exposes ACPI to the guest *at all* (via adding "-no-acpi" to the QEMU commandline when <acpi> is missing - NB: this feature flag is currently supported only on x86 and aarch64 QEMU platforms, and ignored for all other hypervisors).
Possibly the new flags could be put in something like this:
<features> <acpi> <hotplug-bridge enabled='no'/> <hotplug-root enabled='yes'/> </acpi> ... </features>
But:
* currently there are no subelements to <acpi>. So this isn't "extending according to an existing pattern".
* even though the <features> element uses presence of a subelement to indicate "enabled" and absence of the subelement to indicate "disabled". But in the case of these new acpi bridge options we would need to explicitly have the "enabled='yes/no'" rather than just using presence of the option to mean "enabled" and absence to mean "disabled" because the default for "root-hotplug" up until now has been *enabled*, and the default for hotplug-bridge is different depending on machinetype. We need to continue working properly (and identically) with old/existing XML, but if we didn't have an "enabled" attribute for these new flags, there would be no way to tell the difference between "not specified" and "disabled", and so no way to disable the feature for a QEMU where the default was "enabled". (Why does this matter? Because I don't like the inconsistency that would arise from some feature flags using absense to mean "disabled" and some using it to mean "use the default".)
* Having something in <features> in the domain XML kind of implies that the associated capability flags should be represented in the <features> section of the domain capabilities. For example, <acpi/> is listed under <features> in the output of virsh capabilities, separately from the flag indicating presence of the -no-acpi option. I'm not sure if we would need to add something there for these options if we moved them into <features> (seems a bit redundant to me to have it in both places, but I'm sure there are $reasons).
Essentially <features> has become a dumping ground for adhoc global properties. So in that sense it probably is the best fit for this. If we don't want to touch th existing <acpi> element for fear of back compat issues, we could have <pci-hotplug acpi="yes|no"/> for the acpi-pci-hotplug-with-bridge-support setting ?
2) *****
Alternately, there is an <acpi> subelement of <os>, which is currently used to add a SLIC table (some sort of software license table, which I'd never heard of before) using QEMU's -acpitable commandline option. It is also used somehow by the Xen driver.
<os> <acpi> <table type='slic'>/path/to/slic.dat</table> <hotplug-bridge enabled='no'/> <hotplug-root enabled='yes'/> </acpi> ... </os>
My problem with adding these new PCI controller acpi options to os/acpi is simply that it's in the <os> subelement, which is claimed elsewhere to be intended for OS boot options, and is used for things like specifying the path to a kernel / initrd to boot from.
Yeah, we've kind of abused <os> a little with adding <acpi> under that. I can see why we did it, as its another blob kinda like the loader blob, but it was probabl a mistake.
3) ****
A third option, suggested somewhere by Ani, would be to make a completely new top-level element, called something like <acpiHotplug> that would have separate attributes for the two flags, e.g.:
<acpiHotplug bridge='yes' root='yes'/>
I dislike new toplevel options because they just seem so adhoc, as if the XML namespace is a cluttered, disorganized room. That reminds me too much of my own workspace, which is just... depressing.
Agreed, lets not add more top level pieces. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Tue, 28 Sep 2021, Daniel P. Berrangé wrote:
On Thu, Sep 23, 2021 at 04:46:38PM -0400, Laine Stump wrote:
On 9/11/21 11:26 PM, Ani Sinha wrote:
Hi all:
This patchset introduces libvirt xml support for the following two pm conf options:
<pm> <acpi-hotplug-bridge enabled='no'/> <acpi-root-hotplug enabled='yes'/> </pm>
(before I get into a more radical discussion about different options - since we aren't exactly duplicating the QEMU option name anyway, what if we made these names more consistent, e.g. "acpi-hotplug-bridge" and "acpi-hotplug-root"?)
I've thought quite a bit about whether to put these attributes here, or somewhere else, and I'm still undecided.
My initial reaction to this was "PM == Power Management, and power management is all about suspend mode support. Hotplug isn't power management." But then you look at the name of the QEMU option and PM is right there in the name, and I guess it's *kind of related* (effectively suspending/resuming a single device), so maybe I'm thinking too narrowly.
I had the same reaction. Even if QEMU hangs it off a "_PM" device, I feel it is a pretty wierd location from libvirt POV to put this.
So are there alternate places that might fit the purpose of these new options better, rather than directly mimicking the QEMU option placement (for better or worse)? A couple alternative possibilities:
1) ****
One possibility would be to include these new flags within the existing <acpi> subelement of <features>, which is already used to control whether the guest exposes ACPI to the guest *at all* (via adding "-no-acpi" to the QEMU commandline when <acpi> is missing - NB: this feature flag is currently supported only on x86 and aarch64 QEMU platforms, and ignored for all other hypervisors).
Possibly the new flags could be put in something like this:
<features> <acpi> <hotplug-bridge enabled='no'/> <hotplug-root enabled='yes'/> </acpi> ... </features>
But:
* currently there are no subelements to <acpi>. So this isn't "extending according to an existing pattern".
* even though the <features> element uses presence of a subelement to indicate "enabled" and absence of the subelement to indicate "disabled". But in the case of these new acpi bridge options we would need to explicitly have the "enabled='yes/no'" rather than just using presence of the option to mean "enabled" and absence to mean "disabled" because the default for "root-hotplug" up until now has been *enabled*, and the default for hotplug-bridge is different depending on machinetype. We need to continue working properly (and identically) with old/existing XML, but if we didn't have an "enabled" attribute for these new flags, there would be no way to tell the difference between "not specified" and "disabled", and so no way to disable the feature for a QEMU where the default was "enabled". (Why does this matter? Because I don't like the inconsistency that would arise from some feature flags using absense to mean "disabled" and some using it to mean "use the default".)
* Having something in <features> in the domain XML kind of implies that the associated capability flags should be represented in the <features> section of the domain capabilities. For example, <acpi/> is listed under <features> in the output of virsh capabilities, separately from the flag indicating presence of the -no-acpi option. I'm not sure if we would need to add something there for these options if we moved them into <features> (seems a bit redundant to me to have it in both places, but I'm sure there are $reasons).
Essentially <features> has become a dumping ground for adhoc global properties. So in that sense it probably is the best fit for this.
If we don't want to touch th existing <acpi> element for fear of back compat issues, we could have
<pci-hotplug acpi="yes|no"/>
for the acpi-pci-hotplug-with-bridge-support setting ?
Since this is pci bridge related setting, maybe we should have: <pci-hotplug-bridge acpi="yes|no"/> Although in that case, the user should be aware that pcie-root-ports are like bridges. But if we do not have -bridge, then it does not convey the fact that this setting does not apply to pci-root bus on i440fx. :-\
2) *****
Alternately, there is an <acpi> subelement of <os>, which is currently used to add a SLIC table (some sort of software license table, which I'd never heard of before) using QEMU's -acpitable commandline option. It is also used somehow by the Xen driver.
<os> <acpi> <table type='slic'>/path/to/slic.dat</table> <hotplug-bridge enabled='no'/> <hotplug-root enabled='yes'/> </acpi> ... </os>
My problem with adding these new PCI controller acpi options to os/acpi is simply that it's in the <os> subelement, which is claimed elsewhere to be intended for OS boot options, and is used for things like specifying the path to a kernel / initrd to boot from.
Yeah, we've kind of abused <os> a little with adding <acpi> under that. I can see why we did it, as its another blob kinda like the loader blob, but it was probabl a mistake.
3) ****
A third option, suggested somewhere by Ani, would be to make a completely new top-level element, called something like <acpiHotplug> that would have separate attributes for the two flags, e.g.:
<acpiHotplug bridge='yes' root='yes'/>
I dislike new toplevel options because they just seem so adhoc, as if the XML namespace is a cluttered, disorganized room. That reminds me too much of my own workspace, which is just... depressing.
Agreed, lets not add more top level pieces.
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Tue, Sep 28, 2021 at 05:31:43PM +0530, Ani Sinha wrote:
On Tue, 28 Sep 2021, Daniel P. Berrangé wrote:
On Thu, Sep 23, 2021 at 04:46:38PM -0400, Laine Stump wrote:
On 9/11/21 11:26 PM, Ani Sinha wrote:
Hi all:
This patchset introduces libvirt xml support for the following two pm conf options:
<pm> <acpi-hotplug-bridge enabled='no'/> <acpi-root-hotplug enabled='yes'/> </pm>
(before I get into a more radical discussion about different options - since we aren't exactly duplicating the QEMU option name anyway, what if we made these names more consistent, e.g. "acpi-hotplug-bridge" and "acpi-hotplug-root"?)
I've thought quite a bit about whether to put these attributes here, or somewhere else, and I'm still undecided.
My initial reaction to this was "PM == Power Management, and power management is all about suspend mode support. Hotplug isn't power management." But then you look at the name of the QEMU option and PM is right there in the name, and I guess it's *kind of related* (effectively suspending/resuming a single device), so maybe I'm thinking too narrowly.
I had the same reaction. Even if QEMU hangs it off a "_PM" device, I feel it is a pretty wierd location from libvirt POV to put this.
So are there alternate places that might fit the purpose of these new options better, rather than directly mimicking the QEMU option placement (for better or worse)? A couple alternative possibilities:
1) ****
One possibility would be to include these new flags within the existing <acpi> subelement of <features>, which is already used to control whether the guest exposes ACPI to the guest *at all* (via adding "-no-acpi" to the QEMU commandline when <acpi> is missing - NB: this feature flag is currently supported only on x86 and aarch64 QEMU platforms, and ignored for all other hypervisors).
Possibly the new flags could be put in something like this:
<features> <acpi> <hotplug-bridge enabled='no'/> <hotplug-root enabled='yes'/> </acpi> ... </features>
But:
* currently there are no subelements to <acpi>. So this isn't "extending according to an existing pattern".
* even though the <features> element uses presence of a subelement to indicate "enabled" and absence of the subelement to indicate "disabled". But in the case of these new acpi bridge options we would need to explicitly have the "enabled='yes/no'" rather than just using presence of the option to mean "enabled" and absence to mean "disabled" because the default for "root-hotplug" up until now has been *enabled*, and the default for hotplug-bridge is different depending on machinetype. We need to continue working properly (and identically) with old/existing XML, but if we didn't have an "enabled" attribute for these new flags, there would be no way to tell the difference between "not specified" and "disabled", and so no way to disable the feature for a QEMU where the default was "enabled". (Why does this matter? Because I don't like the inconsistency that would arise from some feature flags using absense to mean "disabled" and some using it to mean "use the default".)
* Having something in <features> in the domain XML kind of implies that the associated capability flags should be represented in the <features> section of the domain capabilities. For example, <acpi/> is listed under <features> in the output of virsh capabilities, separately from the flag indicating presence of the -no-acpi option. I'm not sure if we would need to add something there for these options if we moved them into <features> (seems a bit redundant to me to have it in both places, but I'm sure there are $reasons).
Essentially <features> has become a dumping ground for adhoc global properties. So in that sense it probably is the best fit for this.
If we don't want to touch th existing <acpi> element for fear of back compat issues, we could have
<pci-hotplug acpi="yes|no"/>
for the acpi-pci-hotplug-with-bridge-support setting ?
Since this is pci bridge related setting, maybe we should have:
<pci-hotplug-bridge acpi="yes|no"/>
Although in that case, the user should be aware that pcie-root-ports are like bridges. But if we do not have -bridge, then it does not convey the fact that this setting does not apply to pci-root bus on i440fx. :-\
I thought without -bridge is better, because we might want to hang more PCI hotplug options off it later. The docs can clarify the semantics Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Tue, Sep 28, 2021 at 17:49 Daniel P. Berrangé <berrange@redhat.com> wrote: > On Tue, Sep 28, 2021 at 05:31:43PM +0530, Ani Sinha wrote: > > > > > > On Tue, 28 Sep 2021, Daniel P. Berrangé wrote: > > > > > On Thu, Sep 23, 2021 at 04:46:38PM -0400, Laine Stump wrote: > > > > On 9/11/21 11:26 PM, Ani Sinha wrote: > > > > > Hi all: > > > > > > > > > > This patchset introduces libvirt xml support for the following two > pm conf > > > > > options: > > > > > > > > > > <pm> > > > > > <acpi-hotplug-bridge enabled='no'/> > > > > > <acpi-root-hotplug enabled='yes'/> > > > > > </pm> > > > > > > > > (before I get into a more radical discussion about different options > - since > > > > we aren't exactly duplicating the QEMU option name anyway, what if > we made > > > > these names more consistent, e.g. "acpi-hotplug-bridge" and > > > > "acpi-hotplug-root"?) > > > > > > > > I've thought quite a bit about whether to put these attributes here, > or > > > > somewhere else, and I'm still undecided. > > > > > > > > My initial reaction to this was "PM == Power Management, and power > > > > management is all about suspend mode support. Hotplug isn't power > > > > management." But then you look at the name of the QEMU option and PM > is > > > > right there in the name, and I guess it's *kind of related* > (effectively > > > > suspending/resuming a single device), so maybe I'm thinking too > narrowly. > > > > > > I had the same reaction. Even if QEMU hangs it off a "_PM" device, > > > I feel it is a pretty wierd location from libvirt POV to put this. > > > > > > > So are there alternate places that might fit the purpose of these new > > > > options better, rather than directly mimicking the QEMU option > placement > > > > (for better or worse)? A couple alternative possibilities: > > > > > > > > 1) **** > > > > > > > > One possibility would be to include these new flags within the > existing > > > > <acpi> subelement of <features>, which is already used to control > whether > > > > the guest exposes ACPI to the guest *at all* (via adding "-no-acpi" > to the > > > > QEMU commandline when <acpi> is missing - NB: this feature flag is > currently > > > > supported only on x86 and aarch64 QEMU platforms, and ignored for > all other > > > > hypervisors). > > > > > > > > Possibly the new flags could be put in something like this: > > > > > > > > <features> > > > > <acpi> > > > > <hotplug-bridge enabled='no'/> > > > > <hotplug-root enabled='yes'/> > > > > </acpi> > > > > ... > > > > </features> > > > > > > > > But: > > > > > > > > * currently there are no subelements to <acpi>. So this isn't > "extending > > > > according to an existing pattern". > > > > > > > > * even though the <features> element uses presence of a subelement to > > > > indicate "enabled" and absence of the subelement to indicate > "disabled". But > > > > in the case of these new acpi bridge options we would need to > explicitly > > > > have the "enabled='yes/no'" rather than just using presence of the > option to > > > > mean "enabled" and absence to mean "disabled" because the default for > > > > "root-hotplug" up until now has been *enabled*, and the default for > > > > hotplug-bridge is different depending on machinetype. We need to > continue > > > > working properly (and identically) with old/existing XML, but if we > didn't > > > > have an "enabled" attribute for these new flags, there would be no > way to > > > > tell the difference between "not specified" and "disabled", and so > no way to > > > > disable the feature for a QEMU where the default was "enabled". (Why > does > > > > this matter? Because I don't like the inconsistency that would arise > from > > > > some feature flags using absense to mean "disabled" and some using > it to > > > > mean "use the default".) > > > > > > > > * Having something in <features> in the domain XML kind of implies > that the > > > > associated capability flags should be represented in the <features> > section > > > > of the domain capabilities. For example, <acpi/> is listed under > <features> > > > > in the output of virsh capabilities, separately from the flag > indicating > > > > presence of the -no-acpi option. I'm not sure if we would need to add > > > > something there for these options if we moved them into <features> > (seems a > > > > bit redundant to me to have it in both places, but I'm sure there are > > > > $reasons). > > > > > > Essentially <features> has become a dumping ground for adhoc global > > > properties. So in that sense it probably is the best fit for this. > > > > > > If we don't want to touch th existing <acpi> element for fear of > > > back compat issues, we could have > > > > > > <pci-hotplug acpi="yes|no"/> > > > > > > for the acpi-pci-hotplug-with-bridge-support setting ? > > > > > > > Since this is pci bridge related setting, maybe we should have: > > > > <pci-hotplug-bridge acpi="yes|no"/> > > > > Although in that case, the user should be aware that pcie-root-ports are > > like bridges. But if we do not have -bridge, then it does not convey the > > fact that this setting does not apply to pci-root bus on i440fx. :-\ > > I thought without -bridge is better, because we might want to hang > more PCI hotplug options off it later. The docs can clarify the > semantics How about <pci-hotplug bridge-acpi='yes/no' /> > > > Regards, > Daniel > -- > |: https://berrange.com -o- > https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- > https://fstop138.berrange.com :| > |: https://entangle-photo.org -o- > https://www.instagram.com/dberrange :| > >
On Tue, Sep 28, 2021 at 06:14:12PM +0530, Ani Sinha wrote: > On Tue, Sep 28, 2021 at 17:49 Daniel P. Berrangé <berrange@redhat.com> > wrote: > > > On Tue, Sep 28, 2021 at 05:31:43PM +0530, Ani Sinha wrote: > > > > > > > > > On Tue, 28 Sep 2021, Daniel P. Berrangé wrote: > > > > > > > On Thu, Sep 23, 2021 at 04:46:38PM -0400, Laine Stump wrote: > > > > > On 9/11/21 11:26 PM, Ani Sinha wrote: > > > > > > Hi all: > > > > > > > > > > > > This patchset introduces libvirt xml support for the following two > > pm conf > > > > > > options: > > > > > > > > > > > > <pm> > > > > > > <acpi-hotplug-bridge enabled='no'/> > > > > > > <acpi-root-hotplug enabled='yes'/> > > > > > > </pm> > > > > > > > > > > (before I get into a more radical discussion about different options > > - since > > > > > we aren't exactly duplicating the QEMU option name anyway, what if > > we made > > > > > these names more consistent, e.g. "acpi-hotplug-bridge" and > > > > > "acpi-hotplug-root"?) > > > > > > > > > > I've thought quite a bit about whether to put these attributes here, > > or > > > > > somewhere else, and I'm still undecided. > > > > > > > > > > My initial reaction to this was "PM == Power Management, and power > > > > > management is all about suspend mode support. Hotplug isn't power > > > > > management." But then you look at the name of the QEMU option and PM > > is > > > > > right there in the name, and I guess it's *kind of related* > > (effectively > > > > > suspending/resuming a single device), so maybe I'm thinking too > > narrowly. > > > > > > > > I had the same reaction. Even if QEMU hangs it off a "_PM" device, > > > > I feel it is a pretty wierd location from libvirt POV to put this. > > > > > > > > > So are there alternate places that might fit the purpose of these new > > > > > options better, rather than directly mimicking the QEMU option > > placement > > > > > (for better or worse)? A couple alternative possibilities: > > > > > > > > > > 1) **** > > > > > > > > > > One possibility would be to include these new flags within the > > existing > > > > > <acpi> subelement of <features>, which is already used to control > > whether > > > > > the guest exposes ACPI to the guest *at all* (via adding "-no-acpi" > > to the > > > > > QEMU commandline when <acpi> is missing - NB: this feature flag is > > currently > > > > > supported only on x86 and aarch64 QEMU platforms, and ignored for > > all other > > > > > hypervisors). > > > > > > > > > > Possibly the new flags could be put in something like this: > > > > > > > > > > <features> > > > > > <acpi> > > > > > <hotplug-bridge enabled='no'/> > > > > > <hotplug-root enabled='yes'/> > > > > > </acpi> > > > > > ... > > > > > </features> > > > > > > > > > > But: > > > > > > > > > > * currently there are no subelements to <acpi>. So this isn't > > "extending > > > > > according to an existing pattern". > > > > > > > > > > * even though the <features> element uses presence of a subelement to > > > > > indicate "enabled" and absence of the subelement to indicate > > "disabled". But > > > > > in the case of these new acpi bridge options we would need to > > explicitly > > > > > have the "enabled='yes/no'" rather than just using presence of the > > option to > > > > > mean "enabled" and absence to mean "disabled" because the default for > > > > > "root-hotplug" up until now has been *enabled*, and the default for > > > > > hotplug-bridge is different depending on machinetype. We need to > > continue > > > > > working properly (and identically) with old/existing XML, but if we > > didn't > > > > > have an "enabled" attribute for these new flags, there would be no > > way to > > > > > tell the difference between "not specified" and "disabled", and so > > no way to > > > > > disable the feature for a QEMU where the default was "enabled". (Why > > does > > > > > this matter? Because I don't like the inconsistency that would arise > > from > > > > > some feature flags using absense to mean "disabled" and some using > > it to > > > > > mean "use the default".) > > > > > > > > > > * Having something in <features> in the domain XML kind of implies > > that the > > > > > associated capability flags should be represented in the <features> > > section > > > > > of the domain capabilities. For example, <acpi/> is listed under > > <features> > > > > > in the output of virsh capabilities, separately from the flag > > indicating > > > > > presence of the -no-acpi option. I'm not sure if we would need to add > > > > > something there for these options if we moved them into <features> > > (seems a > > > > > bit redundant to me to have it in both places, but I'm sure there are > > > > > $reasons). > > > > > > > > Essentially <features> has become a dumping ground for adhoc global > > > > properties. So in that sense it probably is the best fit for this. > > > > > > > > If we don't want to touch th existing <acpi> element for fear of > > > > back compat issues, we could have > > > > > > > > <pci-hotplug acpi="yes|no"/> > > > > > > > > for the acpi-pci-hotplug-with-bridge-support setting ? > > > > > > > > > > Since this is pci bridge related setting, maybe we should have: > > > > > > <pci-hotplug-bridge acpi="yes|no"/> > > > > > > Although in that case, the user should be aware that pcie-root-ports are > > > like bridges. But if we do not have -bridge, then it does not convey the > > > fact that this setting does not apply to pci-root bus on i440fx. :-\ > > > > I thought without -bridge is better, because we might want to hang > > more PCI hotplug options off it later. The docs can clarify the > > semantics > > > How about <pci-hotplug bridge-acpi='yes/no' /> Lets actally do <pci acpi-bridge-hotplug="yes|no"/> so we can put any PCI related global bits here later Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Tue, Sep 28, 2021 at 10:21 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
On Tue, Sep 28, 2021 at 17:49 Daniel P. Berrangé <berrange@redhat.com> wrote:
On Tue, Sep 28, 2021 at 05:31:43PM +0530, Ani Sinha wrote:
On Tue, 28 Sep 2021, Daniel P. Berrangé wrote:
On Thu, Sep 23, 2021 at 04:46:38PM -0400, Laine Stump wrote:
On 9/11/21 11:26 PM, Ani Sinha wrote: > Hi all: > > This patchset introduces libvirt xml support for the following
two pm conf
> options: > > <pm> > <acpi-hotplug-bridge enabled='no'/> > <acpi-root-hotplug enabled='yes'/> > </pm>
(before I get into a more radical discussion about different
- since
we aren't exactly duplicating the QEMU option name anyway, what if we made these names more consistent, e.g. "acpi-hotplug-bridge" and "acpi-hotplug-root"?)
I've thought quite a bit about whether to put these attributes here, or somewhere else, and I'm still undecided.
My initial reaction to this was "PM == Power Management, and
management is all about suspend mode support. Hotplug isn't power management." But then you look at the name of the QEMU option and PM is right there in the name, and I guess it's *kind of related* (effectively suspending/resuming a single device), so maybe I'm thinking too narrowly.
I had the same reaction. Even if QEMU hangs it off a "_PM" device, I feel it is a pretty wierd location from libvirt POV to put this.
So are there alternate places that might fit the purpose of
options better, rather than directly mimicking the QEMU option placement (for better or worse)? A couple alternative possibilities:
1) ****
One possibility would be to include these new flags within the existing <acpi> subelement of <features>, which is already used to control whether the guest exposes ACPI to the guest *at all* (via adding "-no-acpi" to the QEMU commandline when <acpi> is missing - NB: this feature flag is currently supported only on x86 and aarch64 QEMU platforms, and ignored for all other hypervisors).
Possibly the new flags could be put in something like this:
<features> <acpi> <hotplug-bridge enabled='no'/> <hotplug-root enabled='yes'/> </acpi> ... </features>
But:
* currently there are no subelements to <acpi>. So this isn't "extending according to an existing pattern".
* even though the <features> element uses presence of a subelement to indicate "enabled" and absence of the subelement to indicate "disabled". But in the case of these new acpi bridge options we would need to explicitly have the "enabled='yes/no'" rather than just using presence of
mean "enabled" and absence to mean "disabled" because the default for "root-hotplug" up until now has been *enabled*, and the default for hotplug-bridge is different depending on machinetype. We need to continue working properly (and identically) with old/existing XML, but if we didn't have an "enabled" attribute for these new flags, there would be no way to tell the difference between "not specified" and "disabled", and so no way to disable the feature for a QEMU where the default was "enabled". (Why does this matter? Because I don't like the inconsistency that would arise from some feature flags using absense to mean "disabled" and some using it to mean "use the default".)
* Having something in <features> in the domain XML kind of implies
associated capability flags should be represented in the <features>
option to that the section
of the domain capabilities. For example, <acpi/> is listed under <features> in the output of virsh capabilities, separately from the flag indicating presence of the -no-acpi option. I'm not sure if we would need to add something there for these options if we moved them into <features> (seems a bit redundant to me to have it in both places, but I'm sure
$reasons).
Essentially <features> has become a dumping ground for adhoc global properties. So in that sense it probably is the best fit for this.
If we don't want to touch th existing <acpi> element for fear of back compat issues, we could have
<pci-hotplug acpi="yes|no"/>
for the acpi-pci-hotplug-with-bridge-support setting ?
Since this is pci bridge related setting, maybe we should have:
<pci-hotplug-bridge acpi="yes|no"/>
Although in that case, the user should be aware that pcie-root-ports are like bridges. But if we do not have -bridge, then it does not convey
On Tue, Sep 28, 2021 at 06:14:12PM +0530, Ani Sinha wrote: options power these new the there are the
fact that this setting does not apply to pci-root bus on i440fx. :-\
I thought without -bridge is better, because we might want to hang more PCI hotplug options off it later. The docs can clarify the semantics
How about <pci-hotplug bridge-acpi='yes/no' />
Lets actally do
<pci acpi-bridge-hotplug="yes|no"/>
so we can put any PCI related global bits here later
Yes this sounds good.
On Tue, 28 Sep 2021, Daniel P. Berrangé wrote: > On Tue, Sep 28, 2021 at 06:14:12PM +0530, Ani Sinha wrote: > > On Tue, Sep 28, 2021 at 17:49 Daniel P. Berrangé <berrange@redhat.com> > > wrote: > > > > > On Tue, Sep 28, 2021 at 05:31:43PM +0530, Ani Sinha wrote: > > > > > > > > > > > > On Tue, 28 Sep 2021, Daniel P. Berrangé wrote: > > > > > > > > > On Thu, Sep 23, 2021 at 04:46:38PM -0400, Laine Stump wrote: > > > > > > On 9/11/21 11:26 PM, Ani Sinha wrote: > > > > > > > Hi all: > > > > > > > > > > > > > > This patchset introduces libvirt xml support for the following two > > > pm conf > > > > > > > options: > > > > > > > > > > > > > > <pm> > > > > > > > <acpi-hotplug-bridge enabled='no'/> > > > > > > > <acpi-root-hotplug enabled='yes'/> > > > > > > > </pm> > > > > > > > > > > > > (before I get into a more radical discussion about different options > > > - since > > > > > > we aren't exactly duplicating the QEMU option name anyway, what if > > > we made > > > > > > these names more consistent, e.g. "acpi-hotplug-bridge" and > > > > > > "acpi-hotplug-root"?) > > > > > > > > > > > > I've thought quite a bit about whether to put these attributes here, > > > or > > > > > > somewhere else, and I'm still undecided. > > > > > > > > > > > > My initial reaction to this was "PM == Power Management, and power > > > > > > management is all about suspend mode support. Hotplug isn't power > > > > > > management." But then you look at the name of the QEMU option and PM > > > is > > > > > > right there in the name, and I guess it's *kind of related* > > > (effectively > > > > > > suspending/resuming a single device), so maybe I'm thinking too > > > narrowly. > > > > > > > > > > I had the same reaction. Even if QEMU hangs it off a "_PM" device, > > > > > I feel it is a pretty wierd location from libvirt POV to put this. > > > > > > > > > > > So are there alternate places that might fit the purpose of these new > > > > > > options better, rather than directly mimicking the QEMU option > > > placement > > > > > > (for better or worse)? A couple alternative possibilities: > > > > > > > > > > > > 1) **** > > > > > > > > > > > > One possibility would be to include these new flags within the > > > existing > > > > > > <acpi> subelement of <features>, which is already used to control > > > whether > > > > > > the guest exposes ACPI to the guest *at all* (via adding "-no-acpi" > > > to the > > > > > > QEMU commandline when <acpi> is missing - NB: this feature flag is > > > currently > > > > > > supported only on x86 and aarch64 QEMU platforms, and ignored for > > > all other > > > > > > hypervisors). > > > > > > > > > > > > Possibly the new flags could be put in something like this: > > > > > > > > > > > > <features> > > > > > > <acpi> > > > > > > <hotplug-bridge enabled='no'/> > > > > > > <hotplug-root enabled='yes'/> > > > > > > </acpi> > > > > > > ... > > > > > > </features> > > > > > > > > > > > > But: > > > > > > > > > > > > * currently there are no subelements to <acpi>. So this isn't > > > "extending > > > > > > according to an existing pattern". > > > > > > > > > > > > * even though the <features> element uses presence of a subelement to > > > > > > indicate "enabled" and absence of the subelement to indicate > > > "disabled". But > > > > > > in the case of these new acpi bridge options we would need to > > > explicitly > > > > > > have the "enabled='yes/no'" rather than just using presence of the > > > option to > > > > > > mean "enabled" and absence to mean "disabled" because the default for > > > > > > "root-hotplug" up until now has been *enabled*, and the default for > > > > > > hotplug-bridge is different depending on machinetype. We need to > > > continue > > > > > > working properly (and identically) with old/existing XML, but if we > > > didn't > > > > > > have an "enabled" attribute for these new flags, there would be no > > > way to > > > > > > tell the difference between "not specified" and "disabled", and so > > > no way to > > > > > > disable the feature for a QEMU where the default was "enabled". (Why > > > does > > > > > > this matter? Because I don't like the inconsistency that would arise > > > from > > > > > > some feature flags using absense to mean "disabled" and some using > > > it to > > > > > > mean "use the default".) > > > > > > > > > > > > * Having something in <features> in the domain XML kind of implies > > > that the > > > > > > associated capability flags should be represented in the <features> > > > section > > > > > > of the domain capabilities. For example, <acpi/> is listed under > > > <features> > > > > > > in the output of virsh capabilities, separately from the flag > > > indicating > > > > > > presence of the -no-acpi option. I'm not sure if we would need to add > > > > > > something there for these options if we moved them into <features> > > > (seems a > > > > > > bit redundant to me to have it in both places, but I'm sure there are > > > > > > $reasons). > > > > > > > > > > Essentially <features> has become a dumping ground for adhoc global > > > > > properties. So in that sense it probably is the best fit for this. > > > > > > > > > > If we don't want to touch th existing <acpi> element for fear of > > > > > back compat issues, we could have > > > > > > > > > > <pci-hotplug acpi="yes|no"/> > > > > > > > > > > for the acpi-pci-hotplug-with-bridge-support setting ? > > > > > > > > > > > > > Since this is pci bridge related setting, maybe we should have: > > > > > > > > <pci-hotplug-bridge acpi="yes|no"/> > > > > > > > > Although in that case, the user should be aware that pcie-root-ports are > > > > like bridges. But if we do not have -bridge, then it does not convey the > > > > fact that this setting does not apply to pci-root bus on i440fx. :-\ > > > > > > I thought without -bridge is better, because we might want to hang > > > more PCI hotplug options off it later. The docs can clarify the > > > semantics > > > > > > How about <pci-hotplug bridge-acpi='yes/no' /> > > Lets actally do > > <pci acpi-bridge-hotplug="yes|no"/> > > so we can put any PCI related global bits here later > How about this? <pci> <acpi-bridge-hotplug state="on|off" /> </pci>
On Fri, Sep 24, 2021 at 2:16 AM Laine Stump <laine@redhat.com> wrote:
On 9/11/21 11:26 PM, Ani Sinha wrote:
Hi all:
This patchset introduces libvirt xml support for the following two pm conf options:
<pm> <acpi-hotplug-bridge enabled='no'/> <acpi-root-hotplug enabled='yes'/> </pm>
(before I get into a more radical discussion about different options - since we aren't exactly duplicating the QEMU option name anyway, what if we made these names more consistent, e.g. "acpi-hotplug-bridge" and "acpi-hotplug-root"?)
I've thought quite a bit about whether to put these attributes here, or somewhere else, and I'm still undecided.
My initial reaction to this was "PM == Power Management, and power management is all about suspend mode support. Hotplug isn't power management." But then you look at the name of the QEMU option and PM is right there in the name, and I guess it's *kind of related* (effectively suspending/resuming a single device), so maybe I'm thinking too narrowly.
So are there alternate places that might fit the purpose of these new options better, rather than directly mimicking the QEMU option placement (for better or worse)? A couple alternative possibilities:
1) ****
One possibility would be to include these new flags within the existing <acpi> subelement of <features>, which is already used to control whether the guest exposes ACPI to the guest *at all* (via adding "-no-acpi" to the QEMU commandline when <acpi> is missing - NB: this feature flag is currently supported only on x86 and aarch64 QEMU platforms, and ignored for all other hypervisors).
Possibly the new flags could be put in something like this:
<features> <acpi> <hotplug-bridge enabled='no'/> <hotplug-root enabled='yes'/> </acpi> ... </features>
But:
* currently there are no subelements to <acpi>. So this isn't "extending according to an existing pattern".
* even though the <features> element uses presence of a subelement to indicate "enabled" and absence of the subelement to indicate "disabled". But in the case of these new acpi bridge options we would need to explicitly have the "enabled='yes/no'" rather than just using presence of the option to mean "enabled" and absence to mean "disabled" because the default for "root-hotplug" up until now has been *enabled*, and the default for hotplug-bridge is different depending on machinetype. We need to continue working properly (and identically) with old/existing XML, but if we didn't have an "enabled" attribute for these new flags, there would be no way to tell the difference between "not specified" and "disabled", and so no way to disable the feature for a QEMU where the default was "enabled". (Why does this matter? Because I don't like the inconsistency that would arise from some feature flags using absense to mean "disabled" and some using it to mean "use the default".)
* Having something in <features> in the domain XML kind of implies that the associated capability flags should be represented in the <features> section of the domain capabilities. For example, <acpi/> is listed under <features> in the output of virsh capabilities, separately from the flag indicating presence of the -no-acpi option. I'm not sure if we would need to add something there for these options if we moved them into <features> (seems a bit redundant to me to have it in both places, but I'm sure there are $reasons).
2) *****
Alternately, there is an <acpi> subelement of <os>, which is currently used to add a SLIC table (some sort of software license table, which I'd never heard of before) using QEMU's -acpitable commandline option. It is also used somehow by the Xen driver.
<os> <acpi> <table type='slic'>/path/to/slic.dat</table> <hotplug-bridge enabled='no'/> <hotplug-root enabled='yes'/> </acpi> ... </os>
My problem with adding these new PCI controller acpi options to os/acpi is simply that it's in the <os> subelement, which is claimed elsewhere to be intended for OS boot options, and is used for things like specifying the path to a kernel / initrd to boot from.
3) ****
A third option, suggested somewhere by Ani, would be to make a completely new top-level element, called something like <acpiHotplug> that would have separate attributes for the two flags, e.g.:
<acpiHotplug bridge='yes' root='yes'/>
I dislike new toplevel options because they just seem so adhoc, as if the XML namespace is a cluttered, disorganized room. That reminds me too much of my own workspace, which is just... depressing.
****
Since I always seem to spend *way too much time* worrying about naming, only to have it come out wrong in the end anyway, I'm looking for some other opinions. Counting the version that is in Ani's patch currently as option "0", which option do you all think is the best? Or is it completely unimportant?
The above two options are only available for qemu driver and that too for x86 guests only. Both of them are global options.
``acpi-hotplug-bridge`` option enables or disables ACPI hotplug support for cold plugged bridges. Examples of cold plugged bridges include PCI-PCI bridge (pci-bridge controller) for pc machines and pcie-root-port controller for q35 machines. The corresponding commandline options to qemu for x86 guests are:
The "cold plugged bridges" term here throws me for a loop - it implies that hotplugging bridges is something that's supported, and I think it still isn't. Of course this is just the cover letter, so it won't go into git anywhere, but I think it should be enough to say "enables ACPI hotplug into non-root bus PCI bridges/ports".
I think emphasizing cold plugged bridges is important as Igor (CC'd) has clarified in the other email on patch #3 of this series.
(pc machines): -global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=<off/on> (q35 machines): -global ICH9-LPC.acpi-pci-hotplug-with-bridge-support=<off/on>
So I'm curious - if the QEMU commandline also included "-no-acpi" along with these, what would happen? Would it be silently ignored? Generate an error? Or does -no-acpi only control the suspend support, and acpi hotplug is still available?
Being global options, no other bridge specific options for pci-bridge controller or pcie-root-port controllers are required. For pc machine type in x86, this option is available in qemu for a long time, from version 2.1. Please see the changes in qemu.git:
9e047b982452c6 ("piix4: add acpi pci hotplug support")
Interesting. So how was hotplug handled before this? With SHPC? I know there must be *some* kind of hotplug support in older QEMU, because RHEL6 QEMU supported hotplug, and it was based on qemu 0.12 or something ancient like that...
133a2da488062e ("pc: acpi: generate AML only for PCI0 devices if PCI bridge hotplug is disabled")
For q35 machine type, this was introduced in qemu 6.1 with the following changes in qemu.git:
(a) c0e427d6eb5fef ("hw/acpi/ich9: Enable ACPI PCI hot-plug") (b) 17858a16950860 ("hw/acpi/ich9: Set ACPI PCI hot-plug as default on Q35")
The reasons for enabling ACPI based hotplug for PCIe (q35) based machines (as opposed to native hotplug) for bridges are outlined in (b). It is possible that some users might still want to use native hotplug on PCIe [1]. Therefore, this conf option enables users to choose either ACPI based hotplug or native hotplug for cold plugged bridges (for example for pcie root port controller in q35 machines).
``acpi-root-hotplug`` option enables or disables ACPI based hotplug for PCI root bus (pci-root controller). This option is only available for pc machine type. The corresponding commandline option to qemu for x86 guests is:
-global PIIX4_PM.acpi-root-pci-hotplug=<off/on>
This additional option enables users to disable hotplug for all devices in the system without adding an additional PCI-PCI bridge, putting the devices behind the bridge and using the existing ``acpi-hotplug-bridge`` option to disable hotplug on that bridge. This feature was introduced from qemu version 5.2 with the following change in qemu.git:
3d7e78aa7777f ("Introduce a new flag for i440fx to disable PCI hotplug on the root bus")
The above qemu commit describes some compelling reasons why users might to disable hotplug on PCI root buses [2].
A brief summary of the patches:
[PATCH v3 1/5] qemu: capablities: detect presence of [PATCH v3 2/5] qemu: capablities: detect presence of Patches 1 and 2 implement support for qemu capability checks for the above config options.
[PATCH v3 3/5] conf: introduce acpi-hotplug-bridge and Patch 3 actually adds the config option to the schema and adds related unit tests.
[PATCH v3 4/5] qemu: command: add support for qemu options that Patch 4 adds the backend qemu commandline support for the options. It also adds relevant unit tests for the same.
[PATCH v3 5/5] NEWS: add new acpi pci hotplug options in the release Patch 5 adds the release notes for the next libvirt release.
Changelog: v1: initial implementation. Had some bugs and missed some unit tests. v2: fixed bugs and added additional missing unit tests. 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.
Notes:
[1] One concrete example of why one might still want to use native hotplug with pcie-root-port controller is the fact that we are still discovering issues with acpi hotplug on PCIE.
Yes, sigh. I recall someone saying something like "if we switch to ACPI hotplug then all these bugs just go away and everything works" or something like that. Reality never matches the ideal picture we put in our brains.
At least ACPI hotplug is only the default on new machinetypes (doesn't help much for management platforms that always just use "q35" every time they start a guest). And it can also cause problems with distro-specific machinetypes in downstream distros when they are rebased: https://bugzilla.redhat.com/2006409
One such issue is: https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg02146.html Another reason is that users have been using native hotplug on pcie root ports up until now. They have built and tested their systems based on native hotplug. They may not want to suddenly move to acpi based hotplug just because it is now the default in qemu. Supporting the option to chose one or the other through libvirt makes things simpler for end users.
[2] 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.
To be fair, aside from "support for Win2000/WinXP", none of the items on the "limitations" page of that slide deck is something that's impossible to do with a Q35 machinetype; it's just that accomplishing some things may be more complicated. But I understand your point. Mainly I brought it up because I wanted to be sure that we're adding these to fulfill an actual need, rather than just adding bulk for the sake of completeness, or to satisfy curiosity.
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.
[3] Finally, I implemented support for ``acpi-root-hotplug`` option in Qemu. Since adding the support for this option, I have not run away :-) I am still around, fixing other issues in the same subsystem in qemu and also now I have added myself as a reviewer of patches in this area. I will also be trying to support/maintain this new xml conf option in libvirt to the extent I can in future with the help of other experienced maintainers. Obviously this is all freelance work at this moment and is highly dependent on available free time.
Since I don't follow qemu-devel closely, I didn't have prior knowledge of exactly what the options did, and it was unclear in the earlier versions of your patches that what <acpi-hotplug-bridge enabled='no'/> did was to disable ACPI hotplug for the entire guest (which on Q35 means that native PCIe hotplug will be found/used, and on 440fx means that hotplug won't be possible (unless SHPC hotplugged is enabled)). Your exaplanation and documentation in this spin of the patches makes that all clear though, so I'm beyond the "what does this do and do we need it?" stage to the "are there any problems with the code?" stage, and that's what I'll try to address in my review of the patches.
On 9/30/21 2:16 AM, Ani Sinha wrote:
On Fri, Sep 24, 2021 at 2:16 AM Laine Stump <laine@redhat.com> wrote:
On 9/11/21 11:26 PM, Ani Sinha wrote:
The above two options are only available for qemu driver and that too for x86 guests only. Both of them are global options.
``acpi-hotplug-bridge`` option enables or disables ACPI hotplug support for cold plugged bridges. Examples of cold plugged bridges include PCI-PCI bridge (pci-bridge controller) for pc machines and pcie-root-port controller for q35 machines. The corresponding commandline options to qemu for x86 guests are:
The "cold plugged bridges" term here throws me for a loop - it implies that hotplugging bridges is something that's supported, and I think it still isn't. Of course this is just the cover letter, so it won't go into git anywhere, but I think it should be enough to say "enables ACPI hotplug into non-root bus PCI bridges/ports".
I think emphasizing cold plugged bridges is important as Igor (CC'd) has clarified in the other email on patch #3 of this series.
Okay, so the implication in Igor's email is that a) it is possible to hotplug a pcie controller, but b) any controller that is hotplugged will not have ACPI enabled. Note though that libvirt doesn't allow hotplugging *any* PCI controller, since we were told long ago that no OS will actually rescan the PCI topology when this is done, and so the new controller wouldn't be usable anyway. (that information may well be outdated). I think if you're going to mention that it is specifically for "cold-plugged bridges" then you should also 1) define what "cold-plugged" means, i.e. "(PCI controllers that were present in the domain definition when the guest was first started"), and 2) note that "ACPI is not enabled for bridges that are hot-plugged (but currently libvirt doesn't support hotplugging a pci controller anyway)" or something like that.
On Thu, Sep 30, 2021 at 18:52 Laine Stump <laine@redhat.com> wrote:
On 9/30/21 2:16 AM, Ani Sinha wrote:
On Fri, Sep 24, 2021 at 2:16 AM Laine Stump <laine@redhat.com> wrote:
On 9/11/21 11:26 PM, Ani Sinha wrote:
The above two options are only available for qemu driver and that too
for x86
guests only. Both of them are global options.
``acpi-hotplug-bridge`` option enables or disables ACPI hotplug support for cold plugged bridges. Examples of cold plugged bridges include PCI-PCI bridge (pci-bridge controller) for pc machines and pcie-root-port controller for q35 machines. The corresponding commandline options to qemu for x86 guests are:
The "cold plugged bridges" term here throws me for a loop - it implies that hotplugging bridges is something that's supported, and I think it still isn't. Of course this is just the cover letter, so it won't go into git anywhere, but I think it should be enough to say "enables ACPI hotplug into non-root bus PCI bridges/ports".
I think emphasizing cold plugged bridges is important as Igor (CC'd) has clarified in the other email on patch #3 of this series.
Okay, so the implication in Igor's email is that a) it is possible to hotplug a pcie controller, but b) any controller that is hotplugged will not have ACPI enabled. Note though that libvirt doesn't allow hotplugging *any* PCI controller, since we were told long ago that no OS will actually rescan the PCI topology when this is done, and so the new controller wouldn't be usable anyway. (that information may well be outdated).
From i440fx side all empty ports in the pci root controller are described as hotplug capable from ACPI. So I do not see why we cannot hotplug a pci bridge in one of the pci root ports and OS should be able to detect it without reboot. I have not tried it though.
I think if you're going to mention that it is specifically for "cold-plugged bridges" then you should also 1) define what "cold-plugged" means, i.e. "(PCI controllers that were present in the domain definition when the guest was first started"), and 2) note that "ACPI is not enabled for bridges that are hot-plugged (but currently libvirt doesn't support hotplugging a pci controller anyway)" or something like that.
participants (4)
-
Ani Sinha -
Daniel P. Berrangé -
Igor Mammedov -
Laine Stump