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.
