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 ? > 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 :|
