7:01 a.m.
On Tue, 28 Sep 2021, Daniel P. Berrangé wrote:
On Thu, Sep 23, 2021 at 04:46:38PM -0400, 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.
I had the same reaction. Even if QEMU hangs it off a "_PM" device,
I feel it is a pretty wierd location from libvirt POV to put this.
> 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).
Essentially <features> has become a dumping ground for adhoc global
properties. So in that sense it probably is the best fit for this.
If we don't want to touch th existing <acpi> element for fear of
back compat issues, we could have
<pci-hotplug acpi="yes|no"/>
for the acpi-pci-hotplug-with-bridge-support setting ?
Since this is pci bridge related setting, maybe we should have:
<pci-hotplug-bridge acpi="yes|no"/>
Although in that case, the user should be aware that pcie-root-ports are
like bridges. But if we do not have -bridge, then it does not convey the
fact that this setting does not apply to pci-root bus on i440fx. :-\
> 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.
Yeah, we've kind of abused <os> a little with adding <acpi> under
that. I can see why we did it, as its another blob kinda like the
loader blob, but it was probabl a mistake.
>
> 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.
Agreed, lets not add more top level pieces.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|