Re: [PATCH 1/2] x86: intel_pstate: Fix wrong definition of Disable Energy Efficiency Optimization bit

[Liran Alon <liran.alon@oracle.com>]

> On 15 Apr 2019, at 21:22, Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> wrote:
> 
> On Mon, 2019-04-15 at 21:13 +0300, Liran Alon wrote:
>>> On 15 Apr 2019, at 21:03, Srinivas Pandruvada <
>>> srinivas.pandruvada@linux.intel.com> wrote:
>>> 
>>> On Mon, 2019-04-15 at 11:32 +0300, Liran Alon wrote:
>>>>> On 15 Apr 2019, at 5:00, Srinivas Pandruvada <
>>>>> srinivas.pandruvada@linux.intel.com> wrote:
>>>>> 
>>>>> On Sun, 2019-04-14 at 23:48 +0300, Liran Alon wrote:
>>>>>> Bit definition can be found in Intel SDM Section 2.16 MSRS IN
>>>>>> THE
>>>>>> 6TH
>>>>>> GENERATION, 7TH GENERATION AND 8TH GENERATION
>>>>>> INTEL® CORE™ PROCESSORS, INTEL® XEON® PROCESSOR SCALABLE
>>>>>> FAMILY, AND FUTURE INTEL® CORE™ PROCESSORS.
>>>>>> 
>>>>>> Definition of all Skylake MSR_POWER_CTL bits can also be
>>>>>> found at
>>>>>> EDK2
>>>>>> source at UefiCpuPkg/Include/Register/Msr/SkylakeMsr.h union
>>>>>> MSR_SKYLAKE_POWER_CTL_REGISTER.
>>>>>> 
>>>>>> Fixes: 6e978b22efa1 ("cpufreq: intel_pstate: Disable energy
>>>>>> efficiency optimization")
>>>>> 
>>>>> What are you trying to address? This bit 19 has a special
>>>>> meaning
>>>>> when
>>>>> system is in HWP mode. So this is correct.
>>>>> 
>>>>> Bit 20 has a different meaning depending on legacy or in HWP
>>>>> mode.
>>>>> 
>>>>> Thanks,
>>>>> Srinivas
>>>>> 
>>>> 
>>>> Maybe I’m misinterpreting Intel SDM, but it seems to me that bit
>>>> 19
>>>> in MSR_POWER_CTL is always "Disable Race to Halt Optimization
>>>> (R/W)”
>>>> while bit 20 is the "Disable Energy Efficiency Optimization
>>>> (R/W)”.
>>>> 
>>>> I didn’t find a place in Intel SDM where it is discussed that bit
>>>> 19
>>>> have a special meaning when system is in HWP mode.
>>>> Can you point me to relevant place in Intel SDM?
>>>> 
>>> 
>>> SDM doesn't describe the algorithms. This is a feature of Intel
>>> Speed
>>> Shift Technology aka HWP. Both bits target disabling some energy
>>> efficiency features of the processor. I wish there are some better
>>> names of these bits. Ideas is to pick the best for a platform based
>>> on
>>> the performance needs. Here based on the experiments, setting bit
>>> 19
>>> gave the required performance on Kaby Lake desktops.
>>> 
>>> So unless you found some performance/power issue with setting of
>>> bit 19
>>> vs bit 20, on Kaby Lake based platforms, we shouldn't change (may
>>> be
>>> rename as per SDM definition).
>>> 
>>> Thanks,
>>> Srinivas
>> 
>> I haven’t found any performance/power issue.
>> 
>> The name of the bit, the function names, prints and comments just
>> seems to refer to bit 20 and not bit 19.
>> If the code intention is to manipulate "Disable Race to Halt
>> Optimization” bit instead of "Disable Energy Efficiency Optimization”
>> bit,
>> code should be renamed appropriately.
>> Is this code intention?
> The code change was done before SDM documented this feature, so it is
> not matching.
> 
> Thanks,
> Srinivas
> 

So should I submit a v2 patch that renames these appropriately by updated SDM?

-Liran