[PATCH 1/2] cpufreq: intel_pstate: Allow enable/disable energy efficiency

[PATCH 1/2] cpufreq: intel_pstate: Allow enable/disable energy efficiency

[Srinivas Pandruvada]

By default intel_pstate driver disables energy efficiency by setting
MSR_IA32_POWER_CTL bit 19 for Kaby Lake desktop CPU model in HWP mode.
This CPU model is also shared by Coffee Lake desktop CPUs. This allows
these systems to reach maximum possible frequency. But this adds power
penalty, which some customers don't want. They want some way to enable/
disable dynamically.

So, add an additional attribute "energy_efficiency_enable" under
/sys/devices/system/cpu/intel_pstate/ for these CPU models. This allows
to read and write bit 19 ("Disable Energy Efficiency Optimization") in
the MSR IA32_POWER_CTL.

This attribute is present in both HWP and non-HWP mode as this has an
effect in both modes. Refer to Intel Software Developer's manual for
details. The scope of this bit is package wide.

Suggested-by: Len Brown <lenb@kernel.org>
Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 Documentation/admin-guide/pm/intel_pstate.rst |  7 +++
 drivers/cpufreq/intel_pstate.c                | 49 ++++++++++++++++++-
 2 files changed, 54 insertions(+), 2 deletions(-)

diff --git a/Documentation/admin-guide/pm/intel_pstate.rst b/Documentation/admin-guide/pm/intel_pstate.rst
index 39d80bc29ccd..939bfdc53f4f 100644
--- a/Documentation/admin-guide/pm/intel_pstate.rst
+++ b/Documentation/admin-guide/pm/intel_pstate.rst
@@ -431,6 +431,13 @@ argument is passed to the kernel in the command line.
 	supported in the current configuration, writes to this attribute will
 	fail with an appropriate error.
 
+``energy_efficiency_enable``
+	This attribute is only present on platforms, which has CPUs matching
+	Kaby Lake desktop CPU model. By default "energy_efficiency" is disabled
+	on these CPU models in HWP mode by this driver. Enabling energy
+	efficiency may limit maximum operating frequency in both HWP and non
+	HWP mode.
+
 Interpretation of Policy Attributes
 -----------------------------------
 
diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
index 8e23a698ce04..1cf6d06f2314 100644
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -1218,6 +1218,44 @@ static ssize_t store_hwp_dynamic_boost(struct kobject *a,
 	return count;
 }
 
+#define MSR_IA32_POWER_CTL_BIT_EE	19
+
+static ssize_t show_energy_efficiency_enable(struct kobject *kobj,
+					     struct kobj_attribute *attr,
+					     char *buf)
+{
+	u64 power_ctl;
+	int enable;
+
+	rdmsrl(MSR_IA32_POWER_CTL, power_ctl);
+	enable = (power_ctl & BIT(MSR_IA32_POWER_CTL_BIT_EE)) >> MSR_IA32_POWER_CTL_BIT_EE;
+	return sprintf(buf, "%d\n", !enable);
+}
+
+static ssize_t store_energy_efficiency_enable(struct kobject *a,
+					      struct kobj_attribute *b,
+					      const char *buf, size_t count)
+{
+	u64 power_ctl;
+	u32 input;
+	int ret;
+
+	ret = kstrtouint(buf, 10, &input);
+	if (ret)
+		return ret;
+
+	mutex_lock(&intel_pstate_driver_lock);
+	rdmsrl(MSR_IA32_POWER_CTL, power_ctl);
+	if (input)
+		power_ctl &= ~BIT(MSR_IA32_POWER_CTL_BIT_EE);
+	else
+		power_ctl |= BIT(MSR_IA32_POWER_CTL_BIT_EE);
+	wrmsrl(MSR_IA32_POWER_CTL, power_ctl);
+	mutex_unlock(&intel_pstate_driver_lock);
+
+	return count;
+}
+
 show_one(max_perf_pct, max_perf_pct);
 show_one(min_perf_pct, min_perf_pct);
 
@@ -1228,6 +1266,7 @@ define_one_global_rw(min_perf_pct);
 define_one_global_ro(turbo_pct);
 define_one_global_ro(num_pstates);
 define_one_global_rw(hwp_dynamic_boost);
+define_one_global_rw(energy_efficiency_enable);
 
 static struct attribute *intel_pstate_attributes[] = {
 	&status.attr,
@@ -1241,6 +1280,8 @@ static const struct attribute_group intel_pstate_attr_group = {
 	.attrs = intel_pstate_attributes,
 };
 
+static const struct x86_cpu_id intel_pstate_cpu_ee_disable_ids[];
+
 static void __init intel_pstate_sysfs_expose_params(void)
 {
 	struct kobject *intel_pstate_kobject;
@@ -1273,6 +1314,12 @@ static void __init intel_pstate_sysfs_expose_params(void)
 				       &hwp_dynamic_boost.attr);
 		WARN_ON(rc);
 	}
+
+	if (x86_match_cpu(intel_pstate_cpu_ee_disable_ids)) {
+		rc = sysfs_create_file(intel_pstate_kobject,
+				       &energy_efficiency_enable.attr);
+		WARN_ON(rc);
+	}
 }
 /************************** sysfs end ************************/
 
@@ -1288,8 +1335,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	19
-
 /* Disable energy efficiency optimization */
 static void intel_pstate_disable_ee(int cpu)
 {
-- 
2.25.4

[PATCH 2/2] cpufreq: intel_pstate: Allow raw energy performance preference value

[Srinivas Pandruvada]

Currently using attribute "energy_performance_preference", user space can
write one of the four per-defined preference string. These preference
strings gets mapped to a hard-coded Energy-Performance Preference (EPP) or
Energy-Performance Bias (EPB) knob.

These four values supposed to cover broad spectrum of use cases, but they
are not uniformly distributed in the range. There are number of cases,
where this is not enough. For example:

Suppose user wants more performance when connected to AC. Instead of using
default "balance performance", the "performance" setting can be used. This
changes EPP value from 0x80 to 0x00. But setting EPP to 0, results in
electrical and thermal issues on some platforms. This results in CPU to do
aggressive throttling, which causes drop in performance. But some value
between 0x80 and 0x00 results in better performance. But that value can't
be fixed as the power curve is not linear. In some cases just changing EPP
from 0x80 to 0x75 is enough to get significant performance gain.

Similarly on battery EPP 0x80 can be very aggressive in power consumption.
But picking up the next choice "balance power" results in too much loss
of performance, which cause bad user experience in use case like "Google
Hangout". It was observed that some value between these two EPP is
optimal.

This change allows fine grain EPP tuning for platform like Chromebooks.
Here based on the product and use cases, different EPP values can be set.
This change is similar to the change done for:
/sys/devices/system/cpu/cpu*/power/energy_perf_bias
where user has choice to write a predefined string or raw value.

The change itself is trivial. When user preference doesn't match
predefined string preferences and value is an unsigned integer and in
range, use that value for EPP/EPB.

Suggested-by: Len Brown <lenb@kernel.org>
Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 Documentation/admin-guide/pm/intel_pstate.rst |  4 +-
 drivers/cpufreq/intel_pstate.c                | 63 ++++++++++++++++---
 2 files changed, 56 insertions(+), 11 deletions(-)

diff --git a/Documentation/admin-guide/pm/intel_pstate.rst b/Documentation/admin-guide/pm/intel_pstate.rst
index 939bfdc53f4f..1f4ef187f8a5 100644
--- a/Documentation/admin-guide/pm/intel_pstate.rst
+++ b/Documentation/admin-guide/pm/intel_pstate.rst
@@ -561,7 +561,9 @@ somewhere between the two extremes:
 Strings written to the ``energy_performance_preference`` attribute are
 internally translated to integer values written to the processor's
 Energy-Performance Preference (EPP) knob (if supported) or its
-Energy-Performance Bias (EPB) knob.
+Energy-Performance Bias (EPB) knob. It is also possible to write a positive
+integer value between 0 to 255 for EPP or 0 to 15 for EPB. Writing Invalid
+value results in error.
 
 [Note that tasks may by migrated from one CPU to another by the scheduler's
 load-balancing algorithm and if different energy vs performance hints are
diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
index 1cf6d06f2314..251813b7060b 100644
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -602,11 +602,12 @@ static const unsigned int epp_values[] = {
 	HWP_EPP_POWERSAVE
 };
 
-static int intel_pstate_get_energy_pref_index(struct cpudata *cpu_data)
+static int intel_pstate_get_energy_pref_index(struct cpudata *cpu_data, int *raw_epp)
 {
 	s16 epp;
 	int index = -EINVAL;
 
+	*raw_epp = 0;
 	epp = intel_pstate_get_epp(cpu_data, 0);
 	if (epp < 0)
 		return epp;
@@ -614,12 +615,14 @@ static int intel_pstate_get_energy_pref_index(struct cpudata *cpu_data)
 	if (boot_cpu_has(X86_FEATURE_HWP_EPP)) {
 		if (epp == HWP_EPP_PERFORMANCE)
 			return 1;
-		if (epp <= HWP_EPP_BALANCE_PERFORMANCE)
+		if (epp == HWP_EPP_BALANCE_PERFORMANCE)
 			return 2;
-		if (epp <= HWP_EPP_BALANCE_POWERSAVE)
+		if (epp == HWP_EPP_BALANCE_POWERSAVE)
 			return 3;
-		else
+		if (epp == HWP_EPP_POWERSAVE)
 			return 4;
+		*raw_epp = epp;
+		return 0;
 	} else if (boot_cpu_has(X86_FEATURE_EPB)) {
 		/*
 		 * Range:
@@ -631,6 +634,13 @@ static int intel_pstate_get_energy_pref_index(struct cpudata *cpu_data)
 		 * value which can be set. Here only using top two bits
 		 * effectively.
 		 */
+
+		if (epp & 0x03) {
+			/* Raw value was set in EPB */
+			*raw_epp = epp;
+			return 0;
+		}
+
 		index = (epp >> 2) + 1;
 	}
 
@@ -638,7 +648,8 @@ static int intel_pstate_get_energy_pref_index(struct cpudata *cpu_data)
 }
 
 static int intel_pstate_set_energy_pref_index(struct cpudata *cpu_data,
-					      int pref_index)
+					      int pref_index, bool use_raw,
+					      u32 raw_epp)
 {
 	int epp = -EINVAL;
 	int ret;
@@ -657,12 +668,31 @@ static int intel_pstate_set_energy_pref_index(struct cpudata *cpu_data,
 
 		value &= ~GENMASK_ULL(31, 24);
 
+		if (use_raw) {
+			if (raw_epp > 255) {
+				ret = -EINVAL;
+				goto return_pref;
+			}
+			value |= (u64)raw_epp << 24;
+			ret = wrmsrl_on_cpu(cpu_data->cpu, MSR_HWP_REQUEST, value);
+			goto return_pref;
+		}
+
 		if (epp == -EINVAL)
 			epp = epp_values[pref_index - 1];
 
 		value |= (u64)epp << 24;
 		ret = wrmsrl_on_cpu(cpu_data->cpu, MSR_HWP_REQUEST, value);
 	} else {
+		if (use_raw) {
+			if (raw_epp > 0x0f) {
+				ret = -EINVAL;
+				goto return_pref;
+			}
+			ret = intel_pstate_set_epb(cpu_data->cpu, epp);
+			goto return_pref;
+		}
+
 		if (epp == -EINVAL)
 			epp = (pref_index - 1) << 2;
 		ret = intel_pstate_set_epb(cpu_data->cpu, epp);
@@ -694,6 +724,8 @@ static ssize_t store_energy_performance_preference(
 {
 	struct cpudata *cpu_data = all_cpu_data[policy->cpu];
 	char str_preference[21];
+	bool raw = false;
+	u32 epp;
 	int ret;
 
 	ret = sscanf(buf, "%20s", str_preference);
@@ -701,10 +733,18 @@ static ssize_t store_energy_performance_preference(
 		return -EINVAL;
 
 	ret = match_string(energy_perf_strings, -1, str_preference);
-	if (ret < 0)
+	if (ret < 0) {
+		ret = kstrtouint(buf, 10, &epp);
+		if (ret)
+			return ret;
+
+		raw = true;
+	}
+
+	ret = intel_pstate_set_energy_pref_index(cpu_data, ret, raw, epp);
+	if (ret)
 		return ret;
 
-	intel_pstate_set_energy_pref_index(cpu_data, ret);
 	return count;
 }
 
@@ -712,13 +752,16 @@ static ssize_t show_energy_performance_preference(
 				struct cpufreq_policy *policy, char *buf)
 {
 	struct cpudata *cpu_data = all_cpu_data[policy->cpu];
-	int preference;
+	int preference, raw_epp;
 
-	preference = intel_pstate_get_energy_pref_index(cpu_data);
+	preference = intel_pstate_get_energy_pref_index(cpu_data, &raw_epp);
 	if (preference < 0)
 		return preference;
 
-	return  sprintf(buf, "%s\n", energy_perf_strings[preference]);
+	if (raw_epp)
+		return  sprintf(buf, "%d\n", raw_epp);
+	else
+		return  sprintf(buf, "%s\n", energy_perf_strings[preference]);
 }
 
 cpufreq_freq_attr_rw(energy_performance_preference);
-- 
2.25.4

RE: [PATCH 1/2] cpufreq: intel_pstate: Allow enable/disable energy efficiency

[Doug Smythies]

On 2020.06.22 22:13 Srinivas Pandruvada wrote:

> By default intel_pstate driver disables energy efficiency by setting
> MSR_IA32_POWER_CTL bit 19 for Kaby Lake desktop CPU model in HWP mode.
> This CPU model is also shared by Coffee Lake desktop CPUs. This allows
> these systems to reach maximum possible frequency. But this adds power
> penalty, which some customers don't want. They want some way to enable/
> disable dynamically.
> 
> So, add an additional attribute "energy_efficiency_enable" under
> /sys/devices/system/cpu/intel_pstate/ for these CPU models. This allows
> to read and write bit 19 ("Disable Energy Efficiency Optimization") in
> the MSR IA32_POWER_CTL.
> 
> This attribute is present in both HWP and non-HWP mode as this has an
> effect in both modes. Refer to Intel Software Developer's manual for
> details. The scope of this bit is package wide.

I do and always have. However these manuals are 1000's of pages,
are updated often, and it can be difficult to find the correct page
for the correct processor. So it is great that, in general, the same
mnemonics are used in the code as the manuals.

> 
> Suggested-by: Len Brown <lenb@kernel.org>
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
>  Documentation/admin-guide/pm/intel_pstate.rst |  7 +++
>  drivers/cpufreq/intel_pstate.c                | 49 ++++++++++++++++++-
>  2 files changed, 54 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/admin-guide/pm/intel_pstate.rst b/Documentation/admin-
> guide/pm/intel_pstate.rst
> index 39d80bc29ccd..939bfdc53f4f 100644
> --- a/Documentation/admin-guide/pm/intel_pstate.rst
> +++ b/Documentation/admin-guide/pm/intel_pstate.rst
> @@ -431,6 +431,13 @@ argument is passed to the kernel in the command line.
>  	supported in the current configuration, writes to this attribute will
>  	fail with an appropriate error.
> 
> +``energy_efficiency_enable``
> +	This attribute is only present on platforms, which has CPUs matching
> +	Kaby Lake desktop CPU model. By default "energy_efficiency" is disabled

So, why not mention Coffee Lake also, as you did above?

> +	on these CPU models in HWP mode by this driver. Enabling energy
> +	efficiency may limit maximum operating frequency in both HWP and non
> +	HWP mode.
> +
>  Interpretation of Policy Attributes
>  -----------------------------------
> 
> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> index 8e23a698ce04..1cf6d06f2314 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -1218,6 +1218,44 @@ static ssize_t store_hwp_dynamic_boost(struct kobject *a,
>  	return count;
>  }
> 
> +#define MSR_IA32_POWER_CTL_BIT_EE	19

(same comment as the other day, for another patch) In my opinion and for
consistency, this bit should be defined in

arch/x86/include/asm/msr-index.h

like so (I defined the other bits also):

diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index e8370e64a155..1a6084067f18 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -255,6 +255,12 @@

 #define MSR_IA32_POWER_CTL             0x000001fc

+/* POWER_CTL bits (some are model specific): */
+
+#define POWER_CTL_C1E                  1
+#define POWER_CTL_EEO                  19
+#define POWER_CTL_RHO                  20
+
 #define MSR_IA32_MC0_CTL               0x00000400
 #define MSR_IA32_MC0_STATUS            0x00000401
 #define MSR_IA32_MC0_ADDR              0x00000402

There is another almost identical file at:

tools/arch/x86/include/asm/msr-index.h

and I am not sure which one is the master, but
the former contains stuff that the later does not.

I have defined the bits names in a consistent way
as observed elsewhere in the file. i.e. drop the
"MSR_IA32_" prefix and add the bit.

Now, tying this back to my earlier comment about the
manuals:
The file "tools/arch/x86/include/asm/msr-index.h"
is an excellent gateway reference for those
1000's of pages of software reference manuals.

As a user that uses them all the time, but typically
only digs deep into those manuals once every year or
two, I find tremendous value added via the msr-index.h
file.

> +
> +static ssize_t show_energy_efficiency_enable(struct kobject *kobj,
> +					     struct kobj_attribute *attr,
> +					     char *buf)
> +{
> +	u64 power_ctl;
> +	int enable;
> +
> +	rdmsrl(MSR_IA32_POWER_CTL, power_ctl);
> +	enable = (power_ctl & BIT(MSR_IA32_POWER_CTL_BIT_EE)) >> MSR_IA32_POWER_CTL_BIT_EE;
> +	return sprintf(buf, "%d\n", !enable);
> +}
> +
> +static ssize_t store_energy_efficiency_enable(struct kobject *a,
> +					      struct kobj_attribute *b,
> +					      const char *buf, size_t count)
> +{
> +	u64 power_ctl;
> +	u32 input;
> +	int ret;
> +
> +	ret = kstrtouint(buf, 10, &input);
> +	if (ret)
> +		return ret;
> +
> +	mutex_lock(&intel_pstate_driver_lock);
> +	rdmsrl(MSR_IA32_POWER_CTL, power_ctl);
> +	if (input)
> +		power_ctl &= ~BIT(MSR_IA32_POWER_CTL_BIT_EE);
> +	else
> +		power_ctl |= BIT(MSR_IA32_POWER_CTL_BIT_EE);
> +	wrmsrl(MSR_IA32_POWER_CTL, power_ctl);
> +	mutex_unlock(&intel_pstate_driver_lock);
> +
> +	return count;
> +}
> +
>  show_one(max_perf_pct, max_perf_pct);
>  show_one(min_perf_pct, min_perf_pct);
> 
> @@ -1228,6 +1266,7 @@ define_one_global_rw(min_perf_pct);
>  define_one_global_ro(turbo_pct);
>  define_one_global_ro(num_pstates);
>  define_one_global_rw(hwp_dynamic_boost);
> +define_one_global_rw(energy_efficiency_enable);
> 
>  static struct attribute *intel_pstate_attributes[] = {
>  	&status.attr,
> @@ -1241,6 +1280,8 @@ static const struct attribute_group intel_pstate_attr_group = {
>  	.attrs = intel_pstate_attributes,
>  };
> 
> +static const struct x86_cpu_id intel_pstate_cpu_ee_disable_ids[];
> +
>  static void __init intel_pstate_sysfs_expose_params(void)
>  {
>  	struct kobject *intel_pstate_kobject;
> @@ -1273,6 +1314,12 @@ static void __init intel_pstate_sysfs_expose_params(void)
>  				       &hwp_dynamic_boost.attr);
>  		WARN_ON(rc);
>  	}
> +
> +	if (x86_match_cpu(intel_pstate_cpu_ee_disable_ids)) {
> +		rc = sysfs_create_file(intel_pstate_kobject,
> +				       &energy_efficiency_enable.attr);
> +		WARN_ON(rc);
> +	}
>  }
>  /************************** sysfs end ************************/
> 
> @@ -1288,8 +1335,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	19
> -
>  /* Disable energy efficiency optimization */
>  static void intel_pstate_disable_ee(int cpu)
>  {
> --
> 2.25.4

RE: [PATCH 1/2] cpufreq: intel_pstate: Allow enable/disable energy efficiency

[Doug Smythies]

Hi Srinivas,

I saw your V3.
I do not understand your reluctance to use

arch/x86/include/asm/msr-index.h

as the place to define anything MSR related.
Please explain.

One more comment about 1/3 of the way down below.

... Doug

On 2020.06.23 08:53 Doug Smythies wrote:
> On 2020.06.22 22:13 Srinivas Pandruvada wrote:
> 
> > By default intel_pstate driver disables energy efficiency by setting
> > MSR_IA32_POWER_CTL bit 19 for Kaby Lake desktop CPU model in HWP mode.
> > This CPU model is also shared by Coffee Lake desktop CPUs. This allows
> > these systems to reach maximum possible frequency. But this adds power
> > penalty, which some customers don't want. They want some way to enable/
> > disable dynamically.
> >
> > So, add an additional attribute "energy_efficiency_enable" under
> > /sys/devices/system/cpu/intel_pstate/ for these CPU models. This allows
> > to read and write bit 19 ("Disable Energy Efficiency Optimization") in
> > the MSR IA32_POWER_CTL.
> >
> > This attribute is present in both HWP and non-HWP mode as this has an
> > effect in both modes. Refer to Intel Software Developer's manual for
> > details. The scope of this bit is package wide.
> 
> I do and always have. However these manuals are 1000's of pages,
> are updated often, and it can be difficult to find the correct page
> for the correct processor. So it is great that, in general, the same
> mnemonics are used in the code as the manuals.
> 
> >
> > Suggested-by: Len Brown <lenb@kernel.org>
> > Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> > ---
> >  Documentation/admin-guide/pm/intel_pstate.rst |  7 +++
> >  drivers/cpufreq/intel_pstate.c                | 49 ++++++++++++++++++-
> >  2 files changed, 54 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/admin-guide/pm/intel_pstate.rst b/Documentation/admin-
> > guide/pm/intel_pstate.rst
> > index 39d80bc29ccd..939bfdc53f4f 100644
> > --- a/Documentation/admin-guide/pm/intel_pstate.rst
> > +++ b/Documentation/admin-guide/pm/intel_pstate.rst
> > @@ -431,6 +431,13 @@ argument is passed to the kernel in the command line.
> >  	supported in the current configuration, writes to this attribute will
> >  	fail with an appropriate error.
> >
> > +``energy_efficiency_enable``
> > +	This attribute is only present on platforms, which has CPUs matching
> > +	Kaby Lake desktop CPU model. By default "energy_efficiency" is disabled
> 
> So, why not mention Coffee Lake also, as you did above?

And I still think you need to add "Coffee Lake" here also.

> 
> > +	on these CPU models in HWP mode by this driver. Enabling energy
> > +	efficiency may limit maximum operating frequency in both HWP and non
> > +	HWP mode.
> > +
> >  Interpretation of Policy Attributes
> >  -----------------------------------
> >
> > diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> > index 8e23a698ce04..1cf6d06f2314 100644
> > --- a/drivers/cpufreq/intel_pstate.c
> > +++ b/drivers/cpufreq/intel_pstate.c
> > @@ -1218,6 +1218,44 @@ static ssize_t store_hwp_dynamic_boost(struct kobject *a,
> >  	return count;
> >  }
> >
> > +#define MSR_IA32_POWER_CTL_BIT_EE	19
> 
> (same comment as the other day, for another patch) In my opinion and for
> consistency, this bit should be defined in
> 
> arch/x86/include/asm/msr-index.h
> 
> like so (I defined the other bits also):
> 
> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> index e8370e64a155..1a6084067f18 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -255,6 +255,12 @@
> 
>  #define MSR_IA32_POWER_CTL             0x000001fc
> 
> +/* POWER_CTL bits (some are model specific): */
> +
> +#define POWER_CTL_C1E                  1
> +#define POWER_CTL_EEO                  19
> +#define POWER_CTL_RHO                  20
> +
>  #define MSR_IA32_MC0_CTL               0x00000400
>  #define MSR_IA32_MC0_STATUS            0x00000401
>  #define MSR_IA32_MC0_ADDR              0x00000402
> 
> There is another almost identical file at:
> 
> tools/arch/x86/include/asm/msr-index.h
> 
> and I am not sure which one is the master, but
> the former contains stuff that the later does not.
> 
> I have defined the bits names in a consistent way
> as observed elsewhere in the file. i.e. drop the
> "MSR_IA32_" prefix and add the bit.
> 
> Now, tying this back to my earlier comment about the
> manuals:
> The file "tools/arch/x86/include/asm/msr-index.h"
> is an excellent gateway reference for those
> 1000's of pages of software reference manuals.
> 
> As a user that uses them all the time, but typically
> only digs deep into those manuals once every year or
> two, I find tremendous value added via the msr-index.h
> file.
> 
> > +
> > +static ssize_t show_energy_efficiency_enable(struct kobject *kobj,
> > +					     struct kobj_attribute *attr,
> > +					     char *buf)
> > +{
> > +	u64 power_ctl;
> > +	int enable;
> > +
> > +	rdmsrl(MSR_IA32_POWER_CTL, power_ctl);
> > +	enable = (power_ctl & BIT(MSR_IA32_POWER_CTL_BIT_EE)) >> MSR_IA32_POWER_CTL_BIT_EE;
> > +	return sprintf(buf, "%d\n", !enable);
> > +}
> > +
> > +static ssize_t store_energy_efficiency_enable(struct kobject *a,
> > +					      struct kobj_attribute *b,
> > +					      const char *buf, size_t count)
> > +{
> > +	u64 power_ctl;
> > +	u32 input;
> > +	int ret;
> > +
> > +	ret = kstrtouint(buf, 10, &input);
> > +	if (ret)
> > +		return ret;
> > +
> > +	mutex_lock(&intel_pstate_driver_lock);
> > +	rdmsrl(MSR_IA32_POWER_CTL, power_ctl);
> > +	if (input)
> > +		power_ctl &= ~BIT(MSR_IA32_POWER_CTL_BIT_EE);
> > +	else
> > +		power_ctl |= BIT(MSR_IA32_POWER_CTL_BIT_EE);
> > +	wrmsrl(MSR_IA32_POWER_CTL, power_ctl);
> > +	mutex_unlock(&intel_pstate_driver_lock);
> > +
> > +	return count;
> > +}
> > +
> >  show_one(max_perf_pct, max_perf_pct);
> >  show_one(min_perf_pct, min_perf_pct);
> >
> > @@ -1228,6 +1266,7 @@ define_one_global_rw(min_perf_pct);
> >  define_one_global_ro(turbo_pct);
> >  define_one_global_ro(num_pstates);
> >  define_one_global_rw(hwp_dynamic_boost);
> > +define_one_global_rw(energy_efficiency_enable);
> >
> >  static struct attribute *intel_pstate_attributes[] = {
> >  	&status.attr,
> > @@ -1241,6 +1280,8 @@ static const struct attribute_group intel_pstate_attr_group = {
> >  	.attrs = intel_pstate_attributes,
> >  };
> >
> > +static const struct x86_cpu_id intel_pstate_cpu_ee_disable_ids[];
> > +
> >  static void __init intel_pstate_sysfs_expose_params(void)
> >  {
> >  	struct kobject *intel_pstate_kobject;
> > @@ -1273,6 +1314,12 @@ static void __init intel_pstate_sysfs_expose_params(void)
> >  				       &hwp_dynamic_boost.attr);
> >  		WARN_ON(rc);
> >  	}
> > +
> > +	if (x86_match_cpu(intel_pstate_cpu_ee_disable_ids)) {
> > +		rc = sysfs_create_file(intel_pstate_kobject,
> > +				       &energy_efficiency_enable.attr);
> > +		WARN_ON(rc);
> > +	}
> >  }
> >  /************************** sysfs end ************************/
> >
> > @@ -1288,8 +1335,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	19
> > -
> >  /* Disable energy efficiency optimization */
> >  static void intel_pstate_disable_ee(int cpu)
> >  {
> > --
> > 2.25.4

Re: [PATCH 1/2] cpufreq: intel_pstate: Allow enable/disable energy efficiency

[srinivas pandruvada]

Hi Doug,

On Thu, 2020-06-25 at 07:59 -0700, Doug Smythies wrote:
> Hi Srinivas,
> 
> I saw your V3.
> I do not understand your reluctance to use
> 
> arch/x86/include/asm/msr-index.h

I don't have reluctance. That was the guidance from x86 core
maintainers years back. But may have changed. So checking again.

"
Unless the BIT is used in more than one places, it should stay local by
not adding to msr_indes.h.
"

It can be moved to msr_index.h once turbostat start using this.
Len can comment on it when that will happen.

> 
> 
[..]

> > > So, add an additional attribute "energy_efficiency_enable" under
> > > /sys/devices/system/cpu/intel_pstate/ for these CPU models. This
> > > allows
> > > to read and write bit 19 ("Disable Energy Efficiency
> > > Optimization") in
> > > the MSR IA32_POWER_CTL.
> > > 
> > > This attribute is present in both HWP and non-HWP mode as this
> > > has an
> > > effect in both modes. Refer to Intel Software Developer's manual
> > > for
> > > details. The scope of this bit is package wide.
> > 
> > I do and always have. However these manuals are 1000's of pages,
> > are updated often, and it can be difficult to find the correct page
> > for the correct processor. So it is great that, in general, the
> > same
> > mnemonics are used in the code as the manuals.
There is no mnemonic for bits like EE in SDM. The MSR name matches SDM.


> > 
> > > Suggested-by: Len Brown <lenb@kernel.org>
> > > Signed-off-by: Srinivas Pandruvada <
> > > srinivas.pandruvada@linux.intel.com>
> > > ---
> > >  Documentation/admin-guide/pm/intel_pstate.rst |  7 +++
> > >  drivers/cpufreq/intel_pstate.c                | 49
> > > ++++++++++++++++++-
> > >  2 files changed, 54 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/Documentation/admin-guide/pm/intel_pstate.rst
> > > b/Documentation/admin-
> > > guide/pm/intel_pstate.rst
> > > index 39d80bc29ccd..939bfdc53f4f 100644
> > > --- a/Documentation/admin-guide/pm/intel_pstate.rst
> > > +++ b/Documentation/admin-guide/pm/intel_pstate.rst
> > > @@ -431,6 +431,13 @@ argument is passed to the kernel in the
> > > command line.
> > >  	supported in the current configuration, writes to this
> > > attribute will
> > >  	fail with an appropriate error.
> > > 
> > > +``energy_efficiency_enable``
> > > +	This attribute is only present on platforms, which has CPUs
> > > matching
> > > +	Kaby Lake desktop CPU model. By default "energy_efficiency" is
> > > disabled
> > 
> > So, why not mention Coffee Lake also, as you did above?
> 
> And I still think you need to add "Coffee Lake" here also.
We can add.

Thanks,
Srinivas

> 
> > > +	on these CPU models in HWP mode by this driver. Enabling energy
> > > +	efficiency may limit maximum operating frequency in both HWP
> > > and non
> > > +	HWP mode.
> > > +
> > >  Interpretation of Policy Attributes
> > >  -----------------------------------
> > > 
> > > diff --git a/drivers/cpufreq/intel_pstate.c
> > > b/drivers/cpufreq/intel_pstate.c
> > > index 8e23a698ce04..1cf6d06f2314 100644
> > > --- a/drivers/cpufreq/intel_pstate.c
> > > +++ b/drivers/cpufreq/intel_pstate.c
> > > @@ -1218,6 +1218,44 @@ static ssize_t
> > > store_hwp_dynamic_boost(struct kobject *a,
> > >  	return count;
> > >  }
> > > 
> > > +#define MSR_IA32_POWER_CTL_BIT_EE	19
> > 
> > (same comment as the other day, for another patch) In my opinion
> > and for
> > consistency, this bit should be defined in
> > 
> > arch/x86/include/asm/msr-index.h
> > 
> > like so (I defined the other bits also):
> > 
> > diff --git a/arch/x86/include/asm/msr-index.h
> > b/arch/x86/include/asm/msr-index.h
> > index e8370e64a155..1a6084067f18 100644
> > --- a/arch/x86/include/asm/msr-index.h
> > +++ b/arch/x86/include/asm/msr-index.h
> > @@ -255,6 +255,12 @@
> > 
> >  #define MSR_IA32_POWER_CTL             0x000001fc
> > 
> > +/* POWER_CTL bits (some are model specific): */
> > +
> > +#define POWER_CTL_C1E                  1
> > +#define POWER_CTL_EEO                  19
> > +#define POWER_CTL_RHO                  20
> > +
> >  #define MSR_IA32_MC0_CTL               0x00000400
> >  #define MSR_IA32_MC0_STATUS            0x00000401
> >  #define MSR_IA32_MC0_ADDR              0x00000402
> > 
> > There is another almost identical file at:
> > 
> > tools/arch/x86/include/asm/msr-index.h
> > 
> > and I am not sure which one is the master, but
> > the former contains stuff that the later does not.
> > 
> > I have defined the bits names in a consistent way
> > as observed elsewhere in the file. i.e. drop the
> > "MSR_IA32_" prefix and add the bit.
> > 
> > Now, tying this back to my earlier comment about the
> > manuals:
> > The file "tools/arch/x86/include/asm/msr-index.h"
> > is an excellent gateway reference for those
> > 1000's of pages of software reference manuals.
> > 
> > As a user that uses them all the time, but typically
> > only digs deep into those manuals once every year or
> > two, I find tremendous value added via the msr-index.h
> > file.
> > 
> > > +
> > > +static ssize_t show_energy_efficiency_enable(struct kobject
> > > *kobj,
> > > +					     struct kobj_attribute
> > > *attr,
> > > +					     char *buf)
> > > +{
> > > +	u64 power_ctl;
> > > +	int enable;
> > > +
> > > +	rdmsrl(MSR_IA32_POWER_CTL, power_ctl);
> > > +	enable = (power_ctl & BIT(MSR_IA32_POWER_CTL_BIT_EE)) >>
> > > MSR_IA32_POWER_CTL_BIT_EE;
> > > +	return sprintf(buf, "%d\n", !enable);
> > > +}
> > > +
> > > +static ssize_t store_energy_efficiency_enable(struct kobject *a,
> > > +					      struct kobj_attribute *b,
> > > +					      const char *buf, size_t
> > > count)
> > > +{
> > > +	u64 power_ctl;
> > > +	u32 input;
> > > +	int ret;
> > > +
> > > +	ret = kstrtouint(buf, 10, &input);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	mutex_lock(&intel_pstate_driver_lock);
> > > +	rdmsrl(MSR_IA32_POWER_CTL, power_ctl);
> > > +	if (input)
> > > +		power_ctl &= ~BIT(MSR_IA32_POWER_CTL_BIT_EE);
> > > +	else
> > > +		power_ctl |= BIT(MSR_IA32_POWER_CTL_BIT_EE);
> > > +	wrmsrl(MSR_IA32_POWER_CTL, power_ctl);
> > > +	mutex_unlock(&intel_pstate_driver_lock);
> > > +
> > > +	return count;
> > > +}
> > > +
> > >  show_one(max_perf_pct, max_perf_pct);
> > >  show_one(min_perf_pct, min_perf_pct);
> > > 
> > > @@ -1228,6 +1266,7 @@ define_one_global_rw(min_perf_pct);
> > >  define_one_global_ro(turbo_pct);
> > >  define_one_global_ro(num_pstates);
> > >  define_one_global_rw(hwp_dynamic_boost);
> > > +define_one_global_rw(energy_efficiency_enable);
> > > 
> > >  static struct attribute *intel_pstate_attributes[] = {
> > >  	&status.attr,
> > > @@ -1241,6 +1280,8 @@ static const struct attribute_group
> > > intel_pstate_attr_group = {
> > >  	.attrs = intel_pstate_attributes,
> > >  };
> > > 
> > > +static const struct x86_cpu_id
> > > intel_pstate_cpu_ee_disable_ids[];
> > > +
> > >  static void __init intel_pstate_sysfs_expose_params(void)
> > >  {
> > >  	struct kobject *intel_pstate_kobject;
> > > @@ -1273,6 +1314,12 @@ static void __init
> > > intel_pstate_sysfs_expose_params(void)
> > >  				       &hwp_dynamic_boost.attr);
> > >  		WARN_ON(rc);
> > >  	}
> > > +
> > > +	if (x86_match_cpu(intel_pstate_cpu_ee_disable_ids)) {
> > > +		rc = sysfs_create_file(intel_pstate_kobject,
> > > +				       &energy_efficiency_enable.attr);
> > > +		WARN_ON(rc);
> > > +	}
> > >  }
> > >  /************************** sysfs end ************************/
> > > 
> > > @@ -1288,8 +1335,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	19
> > > -
> > >  /* Disable energy efficiency optimization */
> > >  static void intel_pstate_disable_ee(int cpu)
> > >  {
> > > --
> > > 2.25.4
> 
>