Re: [PATCH v15 10/11] qapi/s390x/cpu topology: CPU_POLARITY_CHANGE qapi event

[Daniel P . Berrangé]

On Wed, Feb 08, 2023 at 06:35:39PM +0100, Nina Schoetterl-Glausch wrote:
> On Wed, 2023-02-01 at 14:20 +0100, Pierre Morel wrote:
> > When the guest asks to change the polarity this change
> > is forwarded to the admin using QAPI.
> > The admin is supposed to take according decisions concerning
> > CPU provisioning.
> > 
> > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> > ---
> >  qapi/machine-target.json | 30 ++++++++++++++++++++++++++++++
> >  hw/s390x/cpu-topology.c  |  2 ++
> >  2 files changed, 32 insertions(+)
> > 
> > diff --git a/qapi/machine-target.json b/qapi/machine-target.json
> > index 58df0f5061..5883c3b020 100644
> > --- a/qapi/machine-target.json
> > +++ b/qapi/machine-target.json
> > @@ -371,3 +371,33 @@
> >    },
> >    'if': { 'all': [ 'TARGET_S390X', 'CONFIG_KVM' ] }
> >  }
> > +
> > +##
> > +# @CPU_POLARITY_CHANGE:
> > +#
> > +# Emitted when the guest asks to change the polarity.
> > +#
> > +# @polarity: polarity specified by the guest
> > +#
> > +# The guest can tell the host (via the PTF instruction) whether the
> > +# CPUs should be provisioned using horizontal or vertical polarity.
> > +#
> > +# On horizontal polarity the host is expected to provision all vCPUs
> > +# equally.
> > +# On vertical polarity the host can provision each vCPU differently.
> > +# The guest will get information on the details of the provisioning
> > +# the next time it uses the STSI(15) instruction.
> > +#
> > +# Since: 8.0
> > +#
> > +# Example:
> > +#
> > +# <- { "event": "CPU_POLARITY_CHANGE",
> > +#      "data": { "polarity": 0 },
> > +#      "timestamp": { "seconds": 1401385907, "microseconds": 422329 } }
> > +#
> > +##
> > +{ 'event': 'CPU_POLARITY_CHANGE',
> > +  'data': { 'polarity': 'int' },
> > +   'if': { 'all': [ 'TARGET_S390X', 'CONFIG_KVM'] }
> 
> I wonder if you should depend on CONFIG_KVM or not. If tcg gets topology
> support it will use the same event and right now it would just never be 
> emitted.
> On the other hand it's more conservative this way.
> 
> I also wonder if you should add 'feature' : [ 'unstable' ].
> On the upside, it would mark the event as unstable, but I don't know what the
> consequences are exactly.

The intention of this flag is to allow mgmt apps to make a usage policy
decision.

Libvirt's policy is that we'll never use features marked unstable.

IOW, the consequence of marking it unstable is that it'll likely
go unused until the unstable marker gets removed.

Using 'unstable' is useful if you want to get complex code merged
before you're quite happy with the design, and then iterate on the
impl in-tree. This is OK if there's no urgent need for apps to
consume the feature. If you want the feature to be used for real
though, the unstable flag is not desirable and you need to finalize
the design.

> Also I guess one can remove qemu events without breaking backwards 
> compatibility,
> since they just won't be emitted? Unless I guess you specify that a event must
> occur under certain situations and the client waits on it?

As Markus says, that's not a safe assumption. If a mgmt app is expecting
to receive an event, ceasing to emit it would likely be considered a
regression.


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