| 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: <f23b11bbf8bf01194404aefd53bdf67fef3bcba8.camel@linux.intel.com>
Date: Thu, 25 Jun 2020 09:06:39 -0700
From: srinivas pandruvada <srinivas.pandruvada@...ux.intel.com>
To: Doug Smythies <dsmythies@...us.net>
Cc: linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org,
rjw@...ysocki.net, viresh.kumar@...aro.org, lenb@...nel.org
Subject: Re: [PATCH 1/2] cpufreq: intel_pstate: Allow enable/disable energy
efficiency
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@...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?
>
> 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
>
>
Powered by blists - more mailing lists