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

[Laine Stump]

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.