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

[Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>]

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