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

[Pierre Morel]

On 5/8/23 23:47, Nina Schoetterl-Glausch wrote:
> 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.


you are right.


> > +#      "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.

OK, that look good.

I guess I have to modify the patch 8 on handle_ptf ,





> >               setcc(cpu, 0);
> >           }
> >           break;