Re: [PATCH 1/2] PM / arch: x86: Rework the MSR_IA32_ENERGY_PERF_BIAS handling
On Thu, Mar 21, 2019 at 11:18:01PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki
>
> The current handling of MSR_IA32_ENERGY_PERF_BIAS in the kernel is
> problematic, because it may cause changes made by user space to that
> MSR (with the help of the x86_energy_perf_policy tool, for example)
> to be lost every time a CPU goes offline and then back online as well
> as during system-wide power management transitions into sleep states
> and back into the working state.
>
> The first problem is that if the current EPB value for a CPU going
> online is 0 ('performance'), the kernel will change it to 6 ('normal')
> regardless of whether or not this is the first bring-up of that CPU.
> That also happens during system-wide resume from sleep states
> (including, but not limited to, hibernation). However, the EPB may
> have been adjusted by user space this way and the kernel should not
> blindly override that setting.
>
> The second problem is that if the platform firmware resets the EPB
> values for any CPUs during system-wide resume from a sleep state,
> the kernel will not restore their previous EPB values that may
> have been set by user space before the preceding system-wide
> suspend transition. Again, that behavior may at least be confusing
> from the user space perspective.
>
> In order to address these issues, rework the handling of
> MSR_IA32_ENERGY_PERF_BIAS so that the EPB value is saved on CPU
> offline and restored on CPU online as well as (for the boot CPU)
> during the syscore stages of system-wide suspend and resume
> transitions, respectively.
>
> However, retain the policy by which the EPB is set to 6 ('normal')
> on the first bring-up of each CPU if its initial value is 0, based
> on the observation that 0 may mean 'not initialized' just as well as
> 'performance' in that case.
>
> While at it, move the MSR_IA32_ENERGY_PERF_BIAS handling code into
> a separate file and document it in Documentation/admin-guide.
>
> Fixes: abe48b108247 (x86, intel, power: Initialize MSR_IA32_ENERGY_PERF_BIAS)
> Fixes: b51ef52df71c (x86/cpu: Restore MSR_IA32_ENERGY_PERF_BIAS after resume)
> Reported-by: Thomas Renninger
> Signed-off-by: Rafael J. Wysocki
Acked-by: Borislav Petkov
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
Re: [PATCH 1/2] PM / arch: x86: Rework the MSR_IA32_ENERGY_PERF_BIAS handling
On Fri, Mar 22, 2019 at 3:28 PM Borislav Petkov wrote:
>
> On Thu, Mar 21, 2019 at 11:18:01PM +0100, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki
> >
> > The current handling of MSR_IA32_ENERGY_PERF_BIAS in the kernel is
> > problematic, because it may cause changes made by user space to that
> > MSR (with the help of the x86_energy_perf_policy tool, for example)
>
> One more reason to control MSR accesses from userspace. I'm working on
> a series to even completely forbid accesses to some MSRs over /dev/msr
> so I think accessing MSR_IA32_ENERGY_PERF_BIAS solely over the new
> interface in patch 2 would be much better.
>
> So, you're carrying those and you'd like to have an ACK from me?
>
> Btw, a couple of nitpicks below.
>
> > Index: linux-pm/arch/x86/kernel/cpu/intel_epb.c
> > ===
> > --- /dev/null
> > +++ linux-pm/arch/x86/kernel/cpu/intel_epb.c
> > @@ -0,0 +1,131 @@
> > +// SPDX-License-Identifier: GPL-2.0
>
> ...
>
> > +static DEFINE_PER_CPU(u8, saved_epb);
> > +
> > +#define EPB_MASK 0x0fULL
> > +#define EPB_SAVED0x10ULL
> > +
> > +static int intel_epb_save(void)
>
> I'd drop that "intel_epb_" prefix from those static functions, but your
> call...
They help indexing tools (elixir.bootlin.com and similar) a bit, so
I'd rather retain them.
> > +{
> > + u64 epb;
> > +
> > + rdmsrl(MSR_IA32_ENERGY_PERF_BIAS, epb);
> > + /*
> > + * Ensure that saved_epb will always be nonzero after this write even
> > if
> > + * the EPB value read from the MSR is 0.
> > + */
> > + this_cpu_write(saved_epb, (epb & EPB_MASK) | EPB_SAVED);
> > +
> > + return 0;
> > +}
>
> ...
>
> > Index: linux-pm/Documentation/admin-guide/pm/intel_epb.rst
> > ===
> > --- /dev/null
> > +++ linux-pm/Documentation/admin-guide/pm/intel_epb.rst
>
> WARNING: Missing or malformed SPDX-License-Identifier tag in line 1
> #345: FILE: Documentation/admin-guide/pm/intel_epb.rst:1:
Well, this is documentation, so ...
Re: [PATCH 1/2] PM / arch: x86: Rework the MSR_IA32_ENERGY_PERF_BIAS handling
On Fri, 2019-03-22 at 17:12 +0100, Thomas Gleixner wrote:
> On Fri, 22 Mar 2019, Borislav Petkov wrote:
>
> > On Fri, Mar 22, 2019 at 03:31:54PM +0100, Thomas Gleixner wrote:
> > > We have no proper decision/recommendation about documentation
> > > licensing. That's being worked on.
> >
> > Ok, I'm ignoring this checkpatch warning from now on.
>
> Only for Documentation/* files please.
Perhaps
---
scripts/checkpatch.pl | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index d0001fd1112d..00c1457fe79b 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3058,7 +3058,8 @@ sub process {
"Improper SPDX comment style for
'$realfile', please use '$comment' instead\n" . $herecurr);
}
- if ($comment !~ /^$/ &&
+ if ($realfile !~ m@^Documentation/@ &&
+ $comment !~ /^$/ &&
$rawline !~ m@^\+\Q$comment\E
SPDX-License-Identifier: @) {
WARN("SPDX_LICENSE_TAG",
"Missing or malformed
SPDX-License-Identifier tag in line $checklicenseline\n" . $herecurr);
Re: [PATCH 1/2] PM / arch: x86: Rework the MSR_IA32_ENERGY_PERF_BIAS handling
On Fri, Mar 22, 2019 at 05:27:43PM +0100, Thomas Renninger wrote: > Thanks Rafael for your quick look at and all the time you spend for this! > > A /sys userspace knob will certainly not be enough for us. > You'll need a tool installed fixing this. We won't pick up the second patch converting to the sysfs interface of course - only the first one. The second patch is beginning the slow phasing out of the direct /dev/msr access. -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply.
Re: [PATCH 1/2] PM / arch: x86: Rework the MSR_IA32_ENERGY_PERF_BIAS handling
Thanks Rafael for your quick look at and all the time you spend for this!
A /sys userspace knob will certainly not be enough for us.
You'll need a tool installed fixing this.
powertop on laptops or tuned on servers or a well hidden bootup quirk or
whatsoever.
The patch I sent with this part:
+ if (acpi_gbl_FADT.preferred_profile == PM_PERFORMANCE_SERVER ||
+ acpi_gbl_FADT.preferred_profile == PM_ENTERPRISE_SERVER)
+ return;
and not touching the EBP value then should at least match most of
our users and OEMs who want a "performance" setting out of the box and
set this on purpose.
Even nicer would be compile option to not touch this at all.
On Thursday, March 21, 2019 11:18:01 PM CET Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki
...
> + *
> + * Second, on many systems the initial EPB value coming from the platform
> + * firmware is 0 ('performance') and at least on some of them that is
> because + * the platform firmware does not initialize EPB
Why does the CPU not initialize this value to 6?
That would allow OEMs/BIOS to also suggest an init value for the system.
We should try to get microcode people or whoever is in charge to initialize
this value "properly" if Intel thinks 6 is the correct init value.
> at all with the
> assumption that + * the OS will do that anyway. That sometimes is
> problematic, as it may cause + * the system battery to drain too fast, for
> example, so it is better to adjust + * it on CPU bring-up and if the
> initial EPB value for a given CPU is 0, the + * kernel changes it to 6
> ('normal').
I have an idea to let the kernel more decide about such policies.
It's a nice example that it makes sense to let the kernel set default values.
But not unconditionally, according to what the system is intended to do.
I wanted to do this for quite some time.., I hopefully find the time and
motivation now.
Thanks Rafael.
Sorry for the somewhat rude sounding previous mail, that was not on purpose.
You helped me quite a lot in the past and you obviously still do.
Thomas
Re: [PATCH 1/2] PM / arch: x86: Rework the MSR_IA32_ENERGY_PERF_BIAS handling
On Fri, 22 Mar 2019, Borislav Petkov wrote: > On Fri, Mar 22, 2019 at 03:31:54PM +0100, Thomas Gleixner wrote: > > We have no proper decision/recommendation about documentation > > licensing. That's being worked on. > > Ok, I'm ignoring this checkpatch warning from now on. Only for Documentation/* files please.
Re: [PATCH 1/2] PM / arch: x86: Rework the MSR_IA32_ENERGY_PERF_BIAS handling
On Fri, Mar 22, 2019 at 03:31:54PM +0100, Thomas Gleixner wrote: > We have no proper decision/recommendation about documentation > licensing. That's being worked on. Ok, I'm ignoring this checkpatch warning from now on. Thx. -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply.
Re: [PATCH 1/2] PM / arch: x86: Rework the MSR_IA32_ENERGY_PERF_BIAS handling
On Fri, 22 Mar 2019, Borislav Petkov wrote: > On Thu, Mar 21, 2019 at 11:18:01PM +0100, Rafael J. Wysocki wrote: > > Index: linux-pm/Documentation/admin-guide/pm/intel_epb.rst > > === > > --- /dev/null > > +++ linux-pm/Documentation/admin-guide/pm/intel_epb.rst > > WARNING: Missing or malformed SPDX-License-Identifier tag in line 1 > #345: FILE: Documentation/admin-guide/pm/intel_epb.rst:1: > +== We have no proper decision/recommendation about documentation licensing. That's being worked on. Thanks, tglx
Re: [PATCH 1/2] PM / arch: x86: Rework the MSR_IA32_ENERGY_PERF_BIAS handling
On Thu, Mar 21, 2019 at 11:18:01PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki
>
> The current handling of MSR_IA32_ENERGY_PERF_BIAS in the kernel is
> problematic, because it may cause changes made by user space to that
> MSR (with the help of the x86_energy_perf_policy tool, for example)
One more reason to control MSR accesses from userspace. I'm working on
a series to even completely forbid accesses to some MSRs over /dev/msr
so I think accessing MSR_IA32_ENERGY_PERF_BIAS solely over the new
interface in patch 2 would be much better.
So, you're carrying those and you'd like to have an ACK from me?
Btw, a couple of nitpicks below.
> Index: linux-pm/arch/x86/kernel/cpu/intel_epb.c
> ===
> --- /dev/null
> +++ linux-pm/arch/x86/kernel/cpu/intel_epb.c
> @@ -0,0 +1,131 @@
> +// SPDX-License-Identifier: GPL-2.0
...
> +static DEFINE_PER_CPU(u8, saved_epb);
> +
> +#define EPB_MASK 0x0fULL
> +#define EPB_SAVED0x10ULL
> +
> +static int intel_epb_save(void)
I'd drop that "intel_epb_" prefix from those static functions, but your
call...
> +{
> + u64 epb;
> +
> + rdmsrl(MSR_IA32_ENERGY_PERF_BIAS, epb);
> + /*
> + * Ensure that saved_epb will always be nonzero after this write even if
> + * the EPB value read from the MSR is 0.
> + */
> + this_cpu_write(saved_epb, (epb & EPB_MASK) | EPB_SAVED);
> +
> + return 0;
> +}
...
> Index: linux-pm/Documentation/admin-guide/pm/intel_epb.rst
> ===
> --- /dev/null
> +++ linux-pm/Documentation/admin-guide/pm/intel_epb.rst
WARNING: Missing or malformed SPDX-License-Identifier tag in line 1
#345: FILE: Documentation/admin-guide/pm/intel_epb.rst:1:
+==
> @@ -0,0 +1,6 @@
> +==
> +Intel Performance and Energy Bias Hint
> +==
> +
> +.. kernel-doc:: arch/x86/kernel/cpu/intel_epb.c
> + :doc: overview
> Index: linux-pm/Documentation/admin-guide/pm/working-state.rst
> ===
> --- linux-pm.orig/Documentation/admin-guide/pm/working-state.rst
> +++ linux-pm/Documentation/admin-guide/pm/working-state.rst
> @@ -8,3 +8,4 @@ Working-State Power Management
> cpuidle
> cpufreq
> intel_pstate
> + intel_epb
>
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
Re: [PATCH 1/2] PM / arch: x86: Rework the MSR_IA32_ENERGY_PERF_BIAS handling
On 3/21/19 11:18 PM, Rafael J. Wysocki wrote:
From: Rafael J. Wysocki
The current handling of MSR_IA32_ENERGY_PERF_BIAS in the kernel is
problematic, because it may cause changes made by user space to that
MSR (with the help of the x86_energy_perf_policy tool, for example)
to be lost every time a CPU goes offline and then back online as well
as during system-wide power management transitions into sleep states
and back into the working state.
The first problem is that if the current EPB value for a CPU going
online is 0 ('performance'), the kernel will change it to 6 ('normal')
regardless of whether or not this is the first bring-up of that CPU.
That also happens during system-wide resume from sleep states
(including, but not limited to, hibernation). However, the EPB may
have been adjusted by user space this way and the kernel should not
blindly override that setting.
The second problem is that if the platform firmware resets the EPB
values for any CPUs during system-wide resume from a sleep state,
the kernel will not restore their previous EPB values that may
have been set by user space before the preceding system-wide
suspend transition. Again, that behavior may at least be confusing
from the user space perspective.
In order to address these issues, rework the handling of
MSR_IA32_ENERGY_PERF_BIAS so that the EPB value is saved on CPU
offline and restored on CPU online as well as (for the boot CPU)
during the syscore stages of system-wide suspend and resume
transitions, respectively.
However, retain the policy by which the EPB is set to 6 ('normal')
on the first bring-up of each CPU if its initial value is 0, based
on the observation that 0 may mean 'not initialized' just as well as
'performance' in that case.
While at it, move the MSR_IA32_ENERGY_PERF_BIAS handling code into
a separate file and document it in Documentation/admin-guide.
Fixes: abe48b108247 (x86, intel, power: Initialize MSR_IA32_ENERGY_PERF_BIAS)
Fixes: b51ef52df71c (x86/cpu: Restore MSR_IA32_ENERGY_PERF_BIAS after resume)
Reported-by: Thomas Renninger
Signed-off-by: Rafael J. Wysocki
---
Reviewed-by: Hannes Reinecke
Cheers,
Hannes
--
Dr. Hannes ReineckeTeamlead Storage & Networking
h...@suse.de +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)
[PATCH 1/2] PM / arch: x86: Rework the MSR_IA32_ENERGY_PERF_BIAS handling
From: Rafael J. Wysocki
The current handling of MSR_IA32_ENERGY_PERF_BIAS in the kernel is
problematic, because it may cause changes made by user space to that
MSR (with the help of the x86_energy_perf_policy tool, for example)
to be lost every time a CPU goes offline and then back online as well
as during system-wide power management transitions into sleep states
and back into the working state.
The first problem is that if the current EPB value for a CPU going
online is 0 ('performance'), the kernel will change it to 6 ('normal')
regardless of whether or not this is the first bring-up of that CPU.
That also happens during system-wide resume from sleep states
(including, but not limited to, hibernation). However, the EPB may
have been adjusted by user space this way and the kernel should not
blindly override that setting.
The second problem is that if the platform firmware resets the EPB
values for any CPUs during system-wide resume from a sleep state,
the kernel will not restore their previous EPB values that may
have been set by user space before the preceding system-wide
suspend transition. Again, that behavior may at least be confusing
from the user space perspective.
In order to address these issues, rework the handling of
MSR_IA32_ENERGY_PERF_BIAS so that the EPB value is saved on CPU
offline and restored on CPU online as well as (for the boot CPU)
during the syscore stages of system-wide suspend and resume
transitions, respectively.
However, retain the policy by which the EPB is set to 6 ('normal')
on the first bring-up of each CPU if its initial value is 0, based
on the observation that 0 may mean 'not initialized' just as well as
'performance' in that case.
While at it, move the MSR_IA32_ENERGY_PERF_BIAS handling code into
a separate file and document it in Documentation/admin-guide.
Fixes: abe48b108247 (x86, intel, power: Initialize MSR_IA32_ENERGY_PERF_BIAS)
Fixes: b51ef52df71c (x86/cpu: Restore MSR_IA32_ENERGY_PERF_BIAS after resume)
Reported-by: Thomas Renninger
Signed-off-by: Rafael J. Wysocki
---
This basically is a resend of https://patchwork.kernel.org/patch/10862699/,
except that EPB_MASK and EPB_SAVED are ULL values now and the year in the
copyright notice for the new file is 2019.
---
Documentation/admin-guide/pm/intel_epb.rst |6 +
Documentation/admin-guide/pm/working-state.rst |1
arch/x86/kernel/cpu/Makefile |2
arch/x86/kernel/cpu/common.c | 17 ---
arch/x86/kernel/cpu/cpu.h |1
arch/x86/kernel/cpu/intel.c| 34 --
arch/x86/kernel/cpu/intel_epb.c| 131 +
include/linux/cpuhotplug.h |1
8 files changed, 140 insertions(+), 53 deletions(-)
Index: linux-pm/include/linux/cpuhotplug.h
===
--- linux-pm.orig/include/linux/cpuhotplug.h
+++ linux-pm/include/linux/cpuhotplug.h
@@ -147,6 +147,7 @@ enum cpuhp_state {
CPUHP_AP_X86_VDSO_VMA_ONLINE,
CPUHP_AP_IRQ_AFFINITY_ONLINE,
CPUHP_AP_ARM_MVEBU_SYNC_CLOCKS,
+ CPUHP_AP_X86_INTEL_EPB_ONLINE,
CPUHP_AP_PERF_ONLINE,
CPUHP_AP_PERF_X86_ONLINE,
CPUHP_AP_PERF_X86_UNCORE_ONLINE,
Index: linux-pm/arch/x86/kernel/cpu/intel.c
===
--- linux-pm.orig/arch/x86/kernel/cpu/intel.c
+++ linux-pm/arch/x86/kernel/cpu/intel.c
@@ -596,36 +596,6 @@ detect_keyid_bits:
c->x86_phys_bits -= keyid_bits;
}
-static void init_intel_energy_perf(struct cpuinfo_x86 *c)
-{
- u64 epb;
-
- /*
-* Initialize MSR_IA32_ENERGY_PERF_BIAS if not already initialized.
-* (x86_energy_perf_policy(8) is available to change it at run-time.)
-*/
- if (!cpu_has(c, X86_FEATURE_EPB))
- return;
-
- rdmsrl(MSR_IA32_ENERGY_PERF_BIAS, epb);
- if ((epb & 0xF) != ENERGY_PERF_BIAS_PERFORMANCE)
- return;
-
- pr_warn_once("ENERGY_PERF_BIAS: Set to 'normal', was 'performance'\n");
- pr_warn_once("ENERGY_PERF_BIAS: View and update with
x86_energy_perf_policy(8)\n");
- epb = (epb & ~0xF) | ENERGY_PERF_BIAS_NORMAL;
- wrmsrl(MSR_IA32_ENERGY_PERF_BIAS, epb);
-}
-
-static void intel_bsp_resume(struct cpuinfo_x86 *c)
-{
- /*
-* MSR_IA32_ENERGY_PERF_BIAS is lost across suspend/resume,
-* so reinitialize it properly like during bootup:
-*/
- init_intel_energy_perf(c);
-}
-
static void init_cpuid_fault(struct cpuinfo_x86 *c)
{
u64 msr;
@@ -763,8 +733,6 @@ static void init_intel(struct cpuinfo_x8
if (cpu_has(c, X86_FEATURE_TME))
detect_tme(c);
- init_intel_energy_perf(c);
-
init_intel_misc_features(c);
}
@@ -1023,9 +991,7 @@ static const struct cpu_dev intel_cpu_de
.c_detect_tlb = intel_detect_tlb,
.c_early_init = early_init_intel,
