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: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?

I have also haven’t found any documentation that describes bit 19 have different meaning when system is in HWP mode.

-Liran

> 
>> Thanks,
>> -Liran
>> 
>