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

Tue Sep 28 12:44:12 UTC 2021

On Tue, Sep 28, 2021 at 17:49 Daniel P. Berrangé <berrange at 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' />

>
>
> 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 :|
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20210928/f4a2074b/attachment-0001.htm>