[PATCH v2 0/4] Add support for two i386 pm options which control acpi hotplug

[Laine Stump]

On 8/19/21 6:50 AM, Ani Sinha wrote:
> Hi:
> 
> I added some negative xml2argv tests as well as new xml2xml tests. In the process,
> I also fixed a bug where I had not added appropriate code to generate the output
> xml correctly.
> The patch series is sent again as v2. Kindly, please provide inputs and review them.
> 
> [PATCH v2 1/4] pm/i386: add support for two options that controls
> [PATCH v2 2/4] tests: add positive xml2argv tests to exercize pm acpi
> [PATCH v2 3/4] tests: add negative xml2argv tests to exercize pm acpi
> [PATCH v2 4/4] tests: add xml2xml tests to exercize pm acpi hotplug


Hi,

Sorry for not sending email about this earlier. We went over a few 
things in an IRC chat a couple weeks ago. I'll reiterate the questions I 
asked there, and fill in more info on what we are looking for in patches.


* Usually when a new feature like this is supported, there will be 
several patches, divided something like this:


1) A patch that just adds the new QEMU capabilities flag(s) that will be 
used later to detect whether or not the selected QEMU binary supports 
the feature. (This might require updates to the sample QEMU capabilities 
test files, if the flag/option didn't happen to coincidentally already 
be in them. Instructions for regenerating the capabilities .replies 
files are in the comments of tests/qemucapabilitiestest.c.)


2) A patch adding the new XML to the RNG and to the XML parser in conf. 
This patch will also contain

  2b) an addition to XML documentation in docs/formatdomain.rst that 
names & describes the purpose of the new elements/attributes and 
provides at least one example of their use (the commit log message 
should include an example too, to make searching for the commit easier), and

  2c) parser/formatter tests added to qemuxml2xmltest.c, with the 
original XML in tests/qemuxml2argvdata, and output XML in qemuxml2xmlout 
(if the input and output files are identical, then the file in 
qemuxml2xmlout should be a symlink to the file in qemuxml2argvdata)


3) A patch that implements the backend of this new feature in the qemu 
driver by checking for its availability using the capabilities in patch 
1, and using the data in the domain object now being parsed by patch 2 
to add something to the qemu commandline. This patch should also include

   3b) all additional test cases for qemuxml2argvtest. By careful 
planning, these new test cases will use the same source XML that was 
added in (2c).

Note that *all* tests for new code are in the same patch as the new code 
itself. I like doing it this way because it ensures that the tests won't 
be forgotten/omitted on purpose when backporting to a different branch.

4) Oh, and just for abologna, a separate patch to add an entry to the 
NEWS.rst file for the next release :-)


0) (backing up a bit) In addition, the cover-letter (patch 0/n) should 
contain a *thorough* description of what each new XML element/attribute 
does, why it is desirable, and a link to the QEMU documentation (and/or 
patches as pushed into qemu) describing what they do in QEMU terms. I 
*think* that the 440fx-only option you added in these patches 
disables/enables hotplug of devices with a single systemwide flag 
(right?); I'm still uncertain what the other option does - apparently 
something about enabling the hotplug of a conventional PCI bridge? Or 
does it enable/disable hotplug of endpoint devices *into* conventional 
PCI bridges?).

This information could be included directly in the cover letter, or the 
cover letter can point to the commit log message for patch 2 or 3 which 
would then have all the information.


=========

About the "Why?" question above - many years ago someone decided that 
every feature added to QEMU *must* be supported directly in libvirt. 
This led to a large bloat of XML to support QEMU features that "seemed 
like a good idea at the time", but nobody ever uses (partly because it's 
unclear exactly how/when they should be used). In the more recent past, 
we've started asking "Is the maintenance burden of supporting this 
feature in libvirt really worthwhile? Is it usable? Who will actually 
use it, and for what?" (for a few years both QEMU and libvirt have been 
trying to get away from conventional PCI + 440fx, and concentrate our 
efforts on PCIe + Q35, so adding new functionality for conventional PCI 
+ 440fx feels kind of like adding a new option to the IDE disk 
controller :-)

I had questions on this topic for these new options, but realized that 
they all depended on my proper understanding of their function, and 
since I don't think I properly understand them yet, all my questions are 
potentially pointless, so I've removed them for now. Maybe it will all 
be clear once I've been properly informed.

====

In the meantime, although not officially "supported" (I believe its use 
will taint the libvirt config) it's possible to add any random option 
not supported by libvirt to the QEMU commandline with libvirt's 
<qemu:commandline> element, documented here:

 
http://blog.vmsplice.net/2011/04/how-to-pass-qemu-command-line-options.html

If your reason for posting these patches was just so that you could try 
out these QEMU options, <qemu:commandline> is a much simpler/quicker way 
to do it than going through all the trouble of adding specific support 
to libvirt.

====

If you're beyond the experimenting stage with these new options, and 
really do have a real-world use case that requires specific support in 
libvirt, then I'd be happy to review (hopefully in a more timely 
fashion!) a resubmission of the patches organized as I laid out above, 
with the additional requested documentation added to 1) the cover 
letter, 2) the patch that adds the new XML to the parser/formatter, and 
3) formatdomain.rst.