diff mbox series

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

Message ID 20190414204831.93705-2-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

Commit Message

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

Comments

srinivas pandruvada April 15, 2019, 2:10 a.m. UTC | #1 
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;
>  }
Liran Alon April 15, 2019, 8:35 a.m. UTC | #2 
> 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;
>> }
>
diff mbox series

Patch

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