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

[Ani Sinha]

On Sat, Sep 25, 2021 at 8:39 PM Ani Sinha <ani at 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 at 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.
>>