| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <001e01d64976$616de950$2449bbf0$@net>
Date: Tue, 23 Jun 2020 08:53:01 -0700
From: "Doug Smythies" <dsmythies@...us.net>
To: "'Srinivas Pandruvada'" <srinivas.pandruvada@...ux.intel.com>,
<lenb@...nel.org>
Cc: <linux-pm@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<rjw@...ysocki.net>, <viresh.kumar@...aro.org>
Subject: RE: [PATCH 1/2] cpufreq: intel_pstate: Allow enable/disable energy efficiency
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@...nel.org>
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@...ux.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
Powered by blists - more mailing lists