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

[Nina Schoetterl-Glausch <nsg@linux.ibm.com>]

On Tue, 2023-02-07 at 10:59 +0100, Pierre Morel wrote:
> 
> 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.

Not really since you only enable topology in the next patch.
> 
> > 
> > > +
> > > +    /*
> > > +     * 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)

I'm saying you don't need to check it at all. You adjust the values for
vertical polarization, but you could just always do that since the values
don't matter at all if the polarization is horizontal.
> 
> > 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.

If no values are provided, they default to dedication=false and entitlement=medium,
as defined by patch 1, which are fine and don't need to be adjusted.

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

I said this because I'm fine with setting it pointlessly.

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

You set it to either high or medium based on the dedication. And for horizontal
polarization it obviously doesn't matter.

As far as I understand you don't need this because the default values are fine.
You should add the check that if a dedicated cpu is hotplugged, then the entitlement
must be high, to patch 2 and that's it, no additional changes necessary.
> 
> 
> Regards,
> Pierre
> 
> > 
> > > +    }
> > >   }
> > >   
> > 
> > [...]
>