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

login
register
mail settings
Submitter Liran Alon
Date April 14, 2019, 8:48 p.m.
Message ID <20190414204831.93705-1-liran.alon@oracle.com>
Download mbox | patch
Permalink /patch/772733/
State New
Headers show

Comments

Liran Alon - April 14, 2019, 8:48 p.m.
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")

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 drivers/cpufreq/intel_pstate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Srinivas Pandruvada - April 15, 2019, 2 a.m.
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




> 
> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Signed-off-by: Liran Alon <liran.alon@oracle.com>
> ---
>  drivers/cpufreq/intel_pstate.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/cpufreq/intel_pstate.c
> b/drivers/cpufreq/intel_pstate.c
> index dd66decf2087..3ce39c332c7b 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -1200,7 +1200,7 @@ static void intel_pstate_hwp_enable(struct
> cpudata *cpudata)corrrect
>  		cpudata->epp_default = intel_pstate_get_epp(cpudata,
> 0);
>  }
>  
> -#define MSR_IA32_POWER_CTL_BIT_EE	19
> +#define MSR_IA32_POWER_CTL_BIT_EE	20
>  
>  /* Disable energy efficiency optimization */
>  static void intel_pstate_disable_ee(int cpu)
Liran Alon - April 15, 2019, 8:32 a.m.
> 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?

Thanks,
-Liran
Srinivas Pandruvada - April 15, 2019, 6:03 p.m.
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

> Thanks,
> -Liran
>
Liran Alon - April 15, 2019, 6:13 p.m.
> 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
>> 
>
Srinivas Pandruvada - April 15, 2019, 6:22 p.m.
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
Liran Alon - April 15, 2019, 6:27 p.m.
> 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
Srinivas Pandruvada - April 15, 2019, 6:30 p.m.
On Mon, 2019-04-15 at 21:27 +0300, Liran Alon wrote:
> > 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?
Sure.

Thanks,
Srinivas

> 
> -Liran
> 
>

Patch

diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
index dd66decf2087..3ce39c332c7b 100644
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -1200,7 +1200,7 @@  static void intel_pstate_hwp_enable(struct cpudata *cpudata)
 		cpudata->epp_default = intel_pstate_get_epp(cpudata, 0);
 }
 
-#define MSR_IA32_POWER_CTL_BIT_EE	19
+#define MSR_IA32_POWER_CTL_BIT_EE	20
 
 /* Disable energy efficiency optimization */
 static void intel_pstate_disable_ee(int cpu)