| Message ID | 20190414204831.93705-1-liran.alon@oracle.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [1/2] x86: intel_pstate: Fix wrong definition of Disable Energy Efficiency Optimization bit | expand |
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)
> 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
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 >
> 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 >> >
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
> 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
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 > >
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)
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(-)