Re: [PATCH v20 11/21] qapi/s390x/cpu topology: CPU_POLARIZATION_CHANGE qapi event

[Nina Schoetterl-Glausch]

On Tue, 2023-04-25 at 18:14 +0200, Pierre Morel wrote:
> When the guest asks to change the polarization this change
> is forwarded to the upper layer using QAPI.
> The upper layer is supposed to take according decisions concerning
> CPU provisioning.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  qapi/machine-target.json | 33 +++++++++++++++++++++++++++++++++
>  hw/s390x/cpu-topology.c  |  2 ++
>  2 files changed, 35 insertions(+)
> 
> diff --git a/qapi/machine-target.json b/qapi/machine-target.json
> index 3b7a0b77f4..ffde2e9cbd 100644
> --- a/qapi/machine-target.json
> +++ b/qapi/machine-target.json
> @@ -391,3 +391,36 @@
>    'features': [ 'unstable' ],
>    'if': { 'all': [ 'TARGET_S390X' , 'CONFIG_KVM' ] }
>  }
> +
> +##
> +# @CPU_POLARIZATION_CHANGE:
> +#
> +# Emitted when the guest asks to change the polarization.
> +#
> +# @polarization: polarization specified by the guest
> +#
> +# Features:
> +# @unstable: This command may still be modified.
> +#
> +# The guest can tell the host (via the PTF instruction) whether the
> +# CPUs should be provisioned using horizontal or vertical polarization.
> +#
> +# On horizontal polarization the host is expected to provision all vCPUs
> +# equally.
> +# On vertical polarization 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.1
> +#
> +# Example:
> +#
> +# <- { "event": "CPU_POLARIZATION_CHANGE",
> +#      "data": { "polarization": 0 },

I think you'd be getting "horizontal" instead of 0.

> +#      "timestamp": { "seconds": 1401385907, "microseconds": 422329 } }
> +##
> +{ 'event': 'CPU_POLARIZATION_CHANGE',
> +  'data': { 'polarization': 'CpuS390Polarization' },
> +  'features': [ 'unstable' ],
> +  'if': { 'all': [ 'TARGET_S390X', 'CONFIG_KVM' ] }
> +}
> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
> index e5fb976594..e8b140d623 100644
> --- a/hw/s390x/cpu-topology.c
> +++ b/hw/s390x/cpu-topology.c
> @@ -17,6 +17,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"
>  
>  /*
>   * s390_topology is used to keep the topology information.
> @@ -138,6 +139,7 @@ void s390_handle_ptf(S390CPU *cpu, uint8_t r1, uintptr_t 
> ra)
>          } else {
>              s390_topology.vertical_polarization = !!fc;
>              s390_cpu_topology_set_changed(true);
> +            qapi_event_send_cpu_polarization_change(fc);

I'm not sure I like the implicit conversation of the function code to the enum 
value.
How about you do 
qapi_event_send_cpu_polarization_change(s390_topology.polarization);
and rename vertical_polarization and change it's type to the enum.
You can then also do

+    CpuS390Polarization polarization = S390_CPU_POLARIZATION_HORIZONTAL;
+    switch (fc) {
+    case S390_CPU_POLARIZATION_VERTICAL:
+        polarization = S390_CPU_POLARIZATION_VERTICAL;
+        /* fallthrough */
+    case S390_CPU_POLARIZATION_HORIZONTAL:
+        if (s390_topology.polarization == polarization) {

and use the value for the assignment further down, too.
>              setcc(cpu, 0);
>          }
>          break;