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
> 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.
>
>
>