On Tue, Sep 28, 2021 at 10:21 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
On Tue, Sep 28, 2021 at 17:49 Daniel P. Berrangé <berrange@redhat.com> wrote:
On Tue, Sep 28, 2021 at 05:31:43PM +0530, Ani Sinha wrote:
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
- 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
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
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
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
associated capability flags should be represented in the <features>
option to that the 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
$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
On Tue, Sep 28, 2021 at 06:14:12PM +0530, Ani Sinha wrote: options power these new the there are the
fact that this setting does not apply to pci-root bus on i440fx. :-\
I thought without -bridge is better, because we might want to hang more PCI hotplug options off it later. The docs can clarify the semantics
How about <pci-hotplug bridge-acpi='yes/no' />
Lets actally do
<pci acpi-bridge-hotplug="yes|no"/>
so we can put any PCI related global bits here later
Yes this sounds good.