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

[Liran Alon <liran.alon@oracle.com>]

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