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

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

[Liran Alon]

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(-)

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)
-- 
2.20.1

[PATCH 2/2] x86: intel: Define MSR_POWER_CTL bits with symbolic constants

[Liran Alon]

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 arch/x86/include/asm/msr-index.h      | 7 +++++++
 drivers/cpufreq/intel_pstate.c        | 6 ++----
 drivers/idle/intel_idle.c             | 2 +-
 tools/power/x86/turbostat/turbostat.c | 2 +-
 4 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 8e40c2446fd1..436f3c5aa358 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -209,6 +209,13 @@
 
 #define MSR_IA32_POWER_CTL		0x000001fc
 
+/* POWERCTLMSR bits: */
+#define POWERCTLMSR_BI_DIR_PROCHOT	BIT(0)	/* Bi-directional PROCHOT */
+#define POWERCTLMSR_C1E_ENABLE		BIT(1)  /* C1E Enable */
+#define POWERCTLMSR_EN_ENERGY_PERF_BIAS	BIT(18) /* Enable MSR_IA32_ENERGY_PERF_BIAS */
+#define POWERCTLMSR_DISABLE_RACE_TO_HLT	BIT(19)	/* Disable Race to Halt Optimization */
+#define POWERCTLMSR_DISABLE_EE		BIT(20) /* Disable Energy Efficiency Optimization */
+
 #define MSR_IA32_MC0_CTL		0x00000400
 #define MSR_IA32_MC0_STATUS		0x00000401
 #define MSR_IA32_MC0_ADDR		0x00000402
diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
index 3ce39c332c7b..b42ba4456f66 100644
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -1200,8 +1200,6 @@ 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	20
-
 /* Disable energy efficiency optimization */
 static void intel_pstate_disable_ee(int cpu)
 {
@@ -1212,9 +1210,9 @@ static void intel_pstate_disable_ee(int cpu)
 	if (ret)
 		return;
 
-	if (!(power_ctl & BIT(MSR_IA32_POWER_CTL_BIT_EE))) {
+	if (!(power_ctl & POWERCTLMSR_DISABLE_EE)) {
 		pr_info("Disabling energy efficiency optimization\n");
-		power_ctl |= BIT(MSR_IA32_POWER_CTL_BIT_EE);
+		power_ctl |= POWERCTLMSR_DISABLE_EE;
 		wrmsrl_on_cpu(cpu, MSR_IA32_POWER_CTL, power_ctl);
 	}
 }
diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
index 8b5d85c91e9d..3654575e6697 100644
--- a/drivers/idle/intel_idle.c
+++ b/drivers/idle/intel_idle.c
@@ -977,7 +977,7 @@ static void c1e_promotion_disable(void)
 	unsigned long long msr_bits;
 
 	rdmsrl(MSR_IA32_POWER_CTL, msr_bits);
-	msr_bits &= ~0x2;
+	msr_bits &= ~POWERCTLMSR_C1E_ENABLE;
 	wrmsrl(MSR_IA32_POWER_CTL, msr_bits);
 }
 
diff --git a/tools/power/x86/turbostat/turbostat.c b/tools/power/x86/turbostat/turbostat.c
index 9327c0ddc3a5..0455aa7e9c6f 100644
--- a/tools/power/x86/turbostat/turbostat.c
+++ b/tools/power/x86/turbostat/turbostat.c
@@ -2019,7 +2019,7 @@ dump_nhm_platform_info(void)
 
 	get_msr(base_cpu, MSR_IA32_POWER_CTL, &msr);
 	fprintf(outf, "cpu%d: MSR_IA32_POWER_CTL: 0x%08llx (C1E auto-promotion: %sabled)\n",
-		base_cpu, msr, msr & 0x2 ? "EN" : "DIS");
+		base_cpu, msr, msr & POWERCTLMSR_C1E_ENABLE ? "EN" : "DIS");
 
 	return;
 }
-- 
2.20.1

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

[Srinivas Pandruvada]

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)

Re: [PATCH 2/2] x86: intel: Define MSR_POWER_CTL bits with symbolic constants

[Srinivas Pandruvada]

On Sun, 2019-04-14 at 23:48 +0300, Liran Alon wrote:
> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Signed-off-by: Liran Alon <liran.alon@oracle.com>
> ---
>  arch/x86/include/asm/msr-index.h      | 7 +++++++
>  drivers/cpufreq/intel_pstate.c        | 6 ++----
>  drivers/idle/intel_idle.c             | 2 +-
>  tools/power/x86/turbostat/turbostat.c | 2 +-
>  4 files changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/include/asm/msr-index.h
> b/arch/x86/include/asm/msr-index.h
> index 8e40c2446fd1..436f3c5aa358 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -209,6 +209,13 @@
>  
>  #define MSR_IA32_POWER_CTL		0x000001fc
>  
> +/* POWERCTLMSR bits: */
> +#define POWERCTLMSR_BI_DIR_PROCHOT	BIT(0)	/* Bi-directional
> PROCHOT */
> +#define POWERCTLMSR_C1E_ENABLE		BIT(1)  /* C1E Enable
> */
> +#define POWERCTLMSR_EN_ENERGY_PERF_BIAS	BIT(18) /* Enable
> MSR_IA32_ENERGY_PERF_BIAS */
> +#define POWERCTLMSR_DISABLE_RACE_TO_HLT	BIT(19)	/* Disable
> Race to Halt Optimization */
> +#define POWERCTLMSR_DISABLE_EE		BIT(20) /* Disable
> Energy Efficiency Optimization */
> +
>  #define MSR_IA32_MC0_CTL		0x00000400
>  #define MSR_IA32_MC0_STATUS		0x00000401
>  #define MSR_IA32_MC0_ADDR		0x00000402
> diff --git a/drivers/cpufreq/intel_pstate.c
> b/drivers/cpufreq/intel_pstate.c
> index 3ce39c332c7b..b42ba4456f66 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -1200,8 +1200,6 @@ 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	20
> -
>  /* Disable energy efficiency optimization */
>  static void intel_pstate_disable_ee(int cpu)
>  {
> @@ -1212,9 +1210,9 @@ static void intel_pstate_disable_ee(int cpu)
>  	if (ret)
>  		return;
>  
> -	if (!(power_ctl & BIT(MSR_IA32_POWER_CTL_BIT_EE))) {
> +	if (!(power_ctl & POWERCTLMSR_DISABLE_EE)) {
>  		pr_info("Disabling energy efficiency optimization\n");
> -		power_ctl |= BIT(MSR_IA32_POWER_CTL_BIT_EE);
> +		power_ctl |= POWERCTLMSR_DISABLE_EE;
To match SDM defintion
power_ctl |= POWERCTLMSR_DISABLE_RACE_TO_HLT;
To set BIT 20, we need some data why this is necessary. If you really
need performance set eneregy_perf_preference to performance.

Thanks,
Srinivas

>  		wrmsrl_on_cpu(cpu, MSR_IA32_POWER_CTL, power_ctl);
>  	}
>  }
> diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
> index 8b5d85c91e9d..3654575e6697 100644
> --- a/drivers/idle/intel_idle.c
> +++ b/drivers/idle/intel_idle.c
> @@ -977,7 +977,7 @@ static void c1e_promotion_disable(void)
>  	unsigned long long msr_bits;
>  
>  	rdmsrl(MSR_IA32_POWER_CTL, msr_bits);
> -	msr_bits &= ~0x2;
> +	msr_bits &= ~POWERCTLMSR_C1E_ENABLE;
>  	wrmsrl(MSR_IA32_POWER_CTL, msr_bits);
>  }
>  
> diff --git a/tools/power/x86/turbostat/turbostat.c
> b/tools/power/x86/turbostat/turbostat.c
> index 9327c0ddc3a5..0455aa7e9c6f 100644
> --- a/tools/power/x86/turbostat/turbostat.c
> +++ b/tools/power/x86/turbostat/turbostat.c
> @@ -2019,7 +2019,7 @@ dump_nhm_platform_info(void)
>  
>  	get_msr(base_cpu, MSR_IA32_POWER_CTL, &msr);
>  	fprintf(outf, "cpu%d: MSR_IA32_POWER_CTL: 0x%08llx (C1E auto-
> promotion: %sabled)\n",
> -		base_cpu, msr, msr & 0x2 ? "EN" : "DIS");
> +		base_cpu, msr, msr & POWERCTLMSR_C1E_ENABLE ? "EN" :
> "DIS");
>  
>  	return;
>  }

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

[Liran Alon]

> 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

Re: [PATCH 2/2] x86: intel: Define MSR_POWER_CTL bits with symbolic constants

[Liran Alon]

> On 15 Apr 2019, at 5:10, Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> wrote:
> 
> On Sun, 2019-04-14 at 23:48 +0300, Liran Alon wrote:
>> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> Signed-off-by: Liran Alon <liran.alon@oracle.com>
>> ---
>> arch/x86/include/asm/msr-index.h      | 7 +++++++
>> drivers/cpufreq/intel_pstate.c        | 6 ++----
>> drivers/idle/intel_idle.c             | 2 +-
>> tools/power/x86/turbostat/turbostat.c | 2 +-
>> 4 files changed, 11 insertions(+), 6 deletions(-)
>> 
>> diff --git a/arch/x86/include/asm/msr-index.h
>> b/arch/x86/include/asm/msr-index.h
>> index 8e40c2446fd1..436f3c5aa358 100644
>> --- a/arch/x86/include/asm/msr-index.h
>> +++ b/arch/x86/include/asm/msr-index.h
>> @@ -209,6 +209,13 @@
>> 
>> #define MSR_IA32_POWER_CTL		0x000001fc
>> 
>> +/* POWERCTLMSR bits: */
>> +#define POWERCTLMSR_BI_DIR_PROCHOT	BIT(0)	/* Bi-directional
>> PROCHOT */
>> +#define POWERCTLMSR_C1E_ENABLE		BIT(1)  /* C1E Enable
>> */
>> +#define POWERCTLMSR_EN_ENERGY_PERF_BIAS	BIT(18) /* Enable
>> MSR_IA32_ENERGY_PERF_BIAS */
>> +#define POWERCTLMSR_DISABLE_RACE_TO_HLT	BIT(19)	/* Disable
>> Race to Halt Optimization */
>> +#define POWERCTLMSR_DISABLE_EE		BIT(20) /* Disable
>> Energy Efficiency Optimization */
>> +
>> #define MSR_IA32_MC0_CTL		0x00000400
>> #define MSR_IA32_MC0_STATUS		0x00000401
>> #define MSR_IA32_MC0_ADDR		0x00000402
>> diff --git a/drivers/cpufreq/intel_pstate.c
>> b/drivers/cpufreq/intel_pstate.c
>> index 3ce39c332c7b..b42ba4456f66 100644
>> --- a/drivers/cpufreq/intel_pstate.c
>> +++ b/drivers/cpufreq/intel_pstate.c
>> @@ -1200,8 +1200,6 @@ 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	20
>> -
>> /* Disable energy efficiency optimization */
>> static void intel_pstate_disable_ee(int cpu)
>> {
>> @@ -1212,9 +1210,9 @@ static void intel_pstate_disable_ee(int cpu)
>> 	if (ret)
>> 		return;
>> 
>> -	if (!(power_ctl & BIT(MSR_IA32_POWER_CTL_BIT_EE))) {
>> +	if (!(power_ctl & POWERCTLMSR_DISABLE_EE)) {
>> 		pr_info("Disabling energy efficiency optimization\n");
>> -		power_ctl |= BIT(MSR_IA32_POWER_CTL_BIT_EE);
>> +		power_ctl |= POWERCTLMSR_DISABLE_EE;
> To match SDM defintion
> power_ctl |= POWERCTLMSR_DISABLE_RACE_TO_HLT;
> To set BIT 20, we need some data why this is necessary. If you really
> need performance set eneregy_perf_preference to performance.
> 
> Thanks,
> Srinivas

This patch is solely a refactoring patch. It doesn’t intend to change semantics.
Based on our discussion on previous patch, it seems we are just misaligned on the meaning of the various MSR_POWER_CTL bits.

-Liran

> 
>> 		wrmsrl_on_cpu(cpu, MSR_IA32_POWER_CTL, power_ctl);
>> 	}
>> }
>> diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
>> index 8b5d85c91e9d..3654575e6697 100644
>> --- a/drivers/idle/intel_idle.c
>> +++ b/drivers/idle/intel_idle.c
>> @@ -977,7 +977,7 @@ static void c1e_promotion_disable(void)
>> 	unsigned long long msr_bits;
>> 
>> 	rdmsrl(MSR_IA32_POWER_CTL, msr_bits);
>> -	msr_bits &= ~0x2;
>> +	msr_bits &= ~POWERCTLMSR_C1E_ENABLE;
>> 	wrmsrl(MSR_IA32_POWER_CTL, msr_bits);
>> }
>> 
>> diff --git a/tools/power/x86/turbostat/turbostat.c
>> b/tools/power/x86/turbostat/turbostat.c
>> index 9327c0ddc3a5..0455aa7e9c6f 100644
>> --- a/tools/power/x86/turbostat/turbostat.c
>> +++ b/tools/power/x86/turbostat/turbostat.c
>> @@ -2019,7 +2019,7 @@ dump_nhm_platform_info(void)
>> 
>> 	get_msr(base_cpu, MSR_IA32_POWER_CTL, &msr);
>> 	fprintf(outf, "cpu%d: MSR_IA32_POWER_CTL: 0x%08llx (C1E auto-
>> promotion: %sabled)\n",
>> -		base_cpu, msr, msr & 0x2 ? "EN" : "DIS");
>> +		base_cpu, msr, msr & POWERCTLMSR_C1E_ENABLE ? "EN" :
>> "DIS");
>> 
>> 	return;
>> }
> 

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

[Srinivas Pandruvada]

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
> 

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

[Liran Alon]

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

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

[Srinivas Pandruvada]

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

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

[Liran Alon]

> 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

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

[Srinivas Pandruvada]

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