diff mbox series

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

Message ID 20230201132051.126868-11-pmorel@linux.ibm.com (mailing list archive)
State New, archived
Headers show
Series s390x: CPU Topology | expand

Commit Message

Pierre Morel Feb. 1, 2023, 1:20 p.m. UTC
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(+)

Comments

Nina Schoetterl-Glausch Feb. 8, 2023, 5:35 p.m. UTC | #1
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.
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?

Patch looks good.

> +}
> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
> index 6c50050991..2f8e1b60cf 100644
> --- a/hw/s390x/cpu-topology.c
> +++ b/hw/s390x/cpu-topology.c
> @@ -19,6 +19,7 @@
>  #include "hw/s390x/s390-virtio-ccw.h"
>  #include "hw/s390x/cpu-topology.h"
>  #include "qapi/qapi-commands-machine-target.h"
> +#include "qapi/qapi-events-machine-target.h"
>  #include "qapi/qmp/qdict.h"
>  #include "monitor/hmp.h"
>  #include "monitor/monitor.h"
> @@ -163,6 +164,7 @@ void s390_handle_ptf(S390CPU *cpu, uint8_t r1, uintptr_t ra)
>              s390_topology.polarity = fc;
>              s390_cpu_topology_set_modified();
>              s390_topology_set_cpus_polarity(fc);
> +            qapi_event_send_cpu_polarity_change(fc);
>              setcc(cpu, 0);
>          }
>          break;
Markus Armbruster Feb. 8, 2023, 7:23 p.m. UTC | #2
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.

> 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.

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.

Questions?

[...]
Daniel P. Berrangé Feb. 9, 2023, 10:04 a.m. UTC | #3
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
Nina Schoetterl-Glausch Feb. 9, 2023, 12:12 p.m. UTC | #4
On Thu, 2023-02-09 at 10:04 +0000, Daniel P. Berrangé wrote:
> 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.

Does it enforce that, e.g via compat policies?
If so, I assume there is some way to allow use of unstable features in libvirt for development?
If for example you're prototyping a new mgmt feature that uses unstable commands.

> 
> 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
Daniel P. Berrangé Feb. 9, 2023, 12:15 p.m. UTC | #5
On Thu, Feb 09, 2023 at 01:12:17PM +0100, Nina Schoetterl-Glausch wrote:
> On Thu, 2023-02-09 at 10:04 +0000, Daniel P. Berrangé wrote:
> > 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.
> 
> Does it enforce that, e.g via compat policies?

The policy is applied at time of code review, in that we'll not
merge patches that use features marked unstable.

> If so, I assume there is some way to allow use of unstable features in libvirt for development?
> If for example you're prototyping a new mgmt feature that uses unstable commands.

You can prototype usage in libvirt in a fork of course, but we
won't take patches into the libvirt upstream repo.

Alternatively in some cases the the libvirt QMP passthrough can
be used for experiemnts (eg virsh qemu-monitor-command ) in a
non-production envionrment.

With regards,
Daniel
Nina Schoetterl-Glausch Feb. 9, 2023, 12:28 p.m. UTC | #6
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?
> 
> [...]
>
Pierre Morel Feb. 9, 2023, 1 p.m. UTC | #7
On 2/9/23 13:28, Nina Schoetterl-Glausch wrote:
> On Wed, 2023-02-08 at 20:23 +0100, Markus Armbruster wrote:
>> Nina Schoetterl-Glausch <nsg@linux.ibm.com> writes:
>>

...

> 
>>
>>> 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.

OK.

Just curious: how do you think this simple event matching the guest 
request 1 on 1 may evolve?

Regards,
Pierre
Nina Schoetterl-Glausch Feb. 9, 2023, 2:50 p.m. UTC | #8
On Thu, 2023-02-09 at 14:00 +0100, Pierre Morel wrote:
> 
> On 2/9/23 13:28, Nina Schoetterl-Glausch wrote:
> > On Wed, 2023-02-08 at 20:23 +0100, Markus Armbruster wrote:
> > > Nina Schoetterl-Glausch <nsg@linux.ibm.com> writes:
> > > 
> 
> ...
> 
> > 
> > > 
> > > > 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.
> 
> OK.
> 
> Just curious: how do you think this simple event matching the guest 
> request 1 on 1 may evolve?

Well, I don't know really, making it unstable is just more conservative for now.
But this way you can prototype/implement support in libvirt for topology and then
make the interface stable one you've confirmed that everything works as intended.

Here is something to think about: The architecture allows rejection of the PTF
polarization change request, if a request is currently in process. A possible design
to allow the same semantics for qemu/libvirt would be to set a polarization_change_in_progess
bit when a PTF request occurs and refuse subsequent requests until this bit was reset via qmp.
This wouldn't result in the event data structure being changed, but its semantics, since it
isn't fired for every request anymore.

> 
> Regards,
> Pierre
>
diff mbox series

Patch

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'] }
+}
diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
index 6c50050991..2f8e1b60cf 100644
--- a/hw/s390x/cpu-topology.c
+++ b/hw/s390x/cpu-topology.c
@@ -19,6 +19,7 @@ 
 #include "hw/s390x/s390-virtio-ccw.h"
 #include "hw/s390x/cpu-topology.h"
 #include "qapi/qapi-commands-machine-target.h"
+#include "qapi/qapi-events-machine-target.h"
 #include "qapi/qmp/qdict.h"
 #include "monitor/hmp.h"
 #include "monitor/monitor.h"
@@ -163,6 +164,7 @@  void s390_handle_ptf(S390CPU *cpu, uint8_t r1, uintptr_t ra)
             s390_topology.polarity = fc;
             s390_cpu_topology_set_modified();
             s390_topology_set_cpus_polarity(fc);
+            qapi_event_send_cpu_polarity_change(fc);
             setcc(cpu, 0);
         }
         break;