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

[Pierre Morel <pmorel@linux.ibm.com>]

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