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

[Nina Schoetterl-Glausch <nsg@linux.ibm.com>]

On Wed, 2023-02-08 at 20:23 +0100, Markus Armbruster wrote:
> Nina Schoetterl-Glausch <nsg@linux.ibm.com> writes:
> 
> > 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.
> 
> TCG vs. KVM should be as transparent as we can make it.
> 
> If only KVM can get into the state where the event is emitted, say
> because the state is only possible with features only KVM supports, then
> making the event conditional on KVM makes sense.  Of course, when
> another accelerator acquires these features, we need to emit the event
> there as well, which will involve adjusting the condition.

That's the case here, KVM supports the feature, TCG doesn't, although there is no
reason it couldn't in the future.

> 
> > 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.
> 
> docs/devel/qapi-code-gen.rst:
> 
>     Special features
>     ~~~~~~~~~~~~~~~~
> 
>     Feature "deprecated" marks a command, event, enum value, or struct
>     member as deprecated.  It is not supported elsewhere so far.
>     Interfaces so marked may be withdrawn in future releases in accordance
>     with QEMU's deprecation policy.
> 
>     Feature "unstable" marks a command, event, enum value, or struct
>     member as unstable.  It is not supported elsewhere so far.  Interfaces
>     so marked may be withdrawn or changed incompatibly in future releases.

Yeah, I saw that, but wasn't aware of -compat, thanks.

> 
> See also -compat parameters unstable-input, unstable-output, both
> intended for "testing the future".
> 
> > 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?
> 
> Events are part of the interface just like command returns are.  Not
> emitting an event in a situation where it was emitted before can easily
> break things.  Only when the situation is no longer possible, the event
> can be removed safely.

@Pierre, seems it would be a good idea to mark all changes to qmp unstable, not just
set-cpu-topology, can just remove it later after all.

> 
> Questions?
> 
> [...]
>