On Tue, Sep 28, 2021 at 10:21 PM Daniel P. Berrangé <berrange(a)redhat.com
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