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

[Nina Schoetterl-Glausch]

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;