Re: [PATCH v15 06/11] s390x/cpu topology: interception of PTF instruction

[Pierre Morel]

On 2/6/23 19:34, Nina Schoetterl-Glausch wrote:
> On Wed, 2023-02-01 at 14:20 +0100, Pierre Morel wrote:
> > When the host supports the CPU topology facility, the PTF
> > instruction with function code 2 is interpreted by the SIE,
> > provided that the userland hypervizor activates the interpretation
> > by using the KVM_CAP_S390_CPU_TOPOLOGY KVM extension.
> >
> > The PTF instructions with function code 0 and 1 are intercepted
> > and must be emulated by the userland hypervizor.
> >
> > During RESET all CPU of the configuration are placed in
> > horizontal polarity.
> >
> > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> > ---
> >   include/hw/s390x/s390-virtio-ccw.h |   6 ++
> >   target/s390x/cpu.h                 |   1 +
> >   hw/s390x/cpu-topology.c            | 103 +++++++++++++++++++++++++++++
> >   target/s390x/cpu-sysemu.c          |  14 ++++
> >   target/s390x/kvm/kvm.c             |  11 +++
> >   5 files changed, 135 insertions(+)
> [...]
>
> > diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
> > index cf63f3dd01..1028bf4476 100644
> > --- a/hw/s390x/cpu-topology.c
> > +++ b/hw/s390x/cpu-topology.c
> > @@ -85,16 +85,104 @@ static void s390_topology_init(MachineState *ms)
> >       QTAILQ_INSERT_HEAD(&s390_topology.list, entry, next);
> >   }
> >   
> > +/**
> > + * s390_topology_set_cpus_polarity:
> > + * @polarity: polarity requested by the caller
> > + *
> > + * Set all CPU entitlement according to polarity and
> > + * dedication.
> > + * Default vertical entitlement is POLARITY_VERTICAL_MEDIUM as
> > + * it does not require host modification of the CPU provisioning
> > + * until the host decide to modify individual CPU provisioning
> > + * using QAPI interface.
> > + * However a dedicated vCPU will have a POLARITY_VERTICAL_HIGH
> > + * entitlement.
> > + */
> > +static void s390_topology_set_cpus_polarity(int polarity)
>
> Since you set the entitlement field I'd prefer _set_cpus_entitlement or similar.

OK if you prefer.

> > +{
> > +    CPUState *cs;
> > +
> > +    CPU_FOREACH(cs) {
> > +        if (polarity == POLARITY_HORIZONTAL) {
> > +            S390_CPU(cs)->env.entitlement = 0;
> > +        } else if (S390_CPU(cs)->env.dedicated) {
> > +            S390_CPU(cs)->env.entitlement = POLARITY_VERTICAL_HIGH;
> > +        } else {
> > +            S390_CPU(cs)->env.entitlement = POLARITY_VERTICAL_MEDIUM;
> > +        }
> > +    }
> > +}
> > +
> [...]
> >   
> >   /**
> > @@ -137,6 +225,21 @@ static void s390_topology_cpu_default(S390CPU *cpu, Error 
> > **errp)
> >                             (smp->books * smp->sockets * smp->cores)) %
> >                            smp->drawers;
> >       }
>
> Why are the changes below in this patch?

Because before thos patch we have only horizontal polarization.

> > +
> > +    /*
> > +     * Machine polarity is set inside the global s390_topology structure.
> > +     * In the case the polarity is set as horizontal set the entitlement

Sorry here an error in the comment should be :
"In the case the polarity is NOT set as horizontal..."

> > +     * to POLARITY_VERTICAL_MEDIUM which is the better equivalent when
> > +     * machine polarity is set to vertical or POLARITY_VERTICAL_HIGH if
> > +     * the vCPU is dedicated.
> > +     */
> > +    if (s390_topology.polarity && !env->entitlement) {
>
> It'd be more readable if you compared against enum values by name.

Right, I will change this to

    if (s390_topology.polarity != S390_POLARITY_HORIZONTAL &&
        env->entitlement == S390_ENTITLEMENT_UNSET) {

> I don't see why you check s390_topology.polarity. If it is horizontal
> then the value of the entitlement doesn't matter at all, so you can set it
> to whatever.

Right, that is why it is done only for vertical polarization (sorry for 
the wrong comment)

> All you want to do is enforce dedicated -> VERTICAL_HIGH, right?
> So why don't you just add
>
> +    if (cpu->env.dedicated && cpu->env.entitlement != POLARITY_VERTICAL_HIGH) {
> +        error_setg(errp, "A dedicated cpu implies high entitlement");
> +        return;
> +    } >
> to s390_topology_check?

Here it is to set the default in the case the values are not provided.

But where you are right is that I should add a verification to the check 
function.

> > +        if (env->dedicated) {
> > +            env->entitlement = POLARITY_VERTICAL_HIGH;
> > +        } else {
> > +            env->entitlement = POLARITY_VERTICAL_MEDIUM;
> > +        }
>
> If it is horizontal, then setting the entitlement is pointless as it will be
> reset to medium on PTF.

That is why the polarity is tested (sorry for the bad comment)

> So the current polarization is vertical and a cpu is being hotplugged,
> but setting the entitlement of the cpu being added is also pointless, because
> it's determined by the dedication. That seems weird.

No it is not determined by the dedication, if there is no dedication the 
3 vertical values are possible.


Regards,
Pierre

> > +    }
> >   }
> >   
>
> [...]

--
Pierre Morel
IBM Lab Boeblingen