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

On Tue, Sep 28, 2021 at 06:14:12PM +0530, Ani Sinha wrote: > On Tue, Sep 28, 2021 at 17:49 Daniel P. Berrangé <berrange(a)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 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. :-\ > > > 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