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

[Pierre Morel]

On 2/7/23 12:27, Nina Schoetterl-Glausch wrote:
> 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:

...

> > > [...]
> > > >    
> > > >    /**
> > > > @@ -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.

OK right

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

Right, I think I added this when working on modifying the attributes in 
QAPI and rebased it in the wrong patch.
Here it has no sense.

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

on PTF yes but not on hotplug, on hotplug all 3 values are possible.

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

Yes, right.

Regards,
Pierre

--
Pierre Morel
IBM Lab Boeblingen