C1E auto-promotion suspend/resume

C1E auto-promotion suspend/resume

[Andy Lutomirski]

Hi-

By BIOS (1.2.3 on a Dell XPS 13 9350) seems to want to enable C1E
auto-promotion (ugh!), which results in this difference across
suspend/resume according to turbostat:

-cpu3: MSR_IA32_POWER_CTL: 0x0024005d (C1E auto-promotion: DISabled)
+cpu3: MSR_IA32_POWER_CTL: 0x0024005f (C1E auto-promotion: ENabled)

Should intel_idle learn to re-disable idle promotion on resume?

--Andy

RE: C1E auto-promotion suspend/resume

[Brown, Len]

> By BIOS (1.2.3 on a Dell XPS 13 9350) seems to want to enable C1E
> auto-promotion (ugh!), which results in this difference across
> suspend/resume according to turbostat:
> 
> -cpu3: MSR_IA32_POWER_CTL: 0x0024005d (C1E auto-promotion: DISabled)
> +cpu3: MSR_IA32_POWER_CTL: 0x0024005f (C1E auto-promotion: ENabled)
> 
> Should intel_idle learn to re-disable idle promotion on resume?

Yes, it seems that way.

Go ahead and send a patch, or file a bug at bugzilla.kernel.org
and we'll get to it.

thanks,
-len

[PATCH 0/4] intel_idle: Improve MSR fixup resume handling

[Andy Lutomirski]

I can't usefully test the fourth patch.  I'm reasonably confident
that the other three work, though, and I tested them on a laptop
that doesn't preserve the C1E auto-promotion flag across
suspend/resume.

Andy Lutomirski (4):
  intel_idle: Consolidate auto-promotion/auto-demotion fixups
  intel_idle: Remove a broadcast MSR fixup at boot
  intel_idle: Fix MSRs after resume
  intel_idle: Move BYT/CHT auto-demotion fixup into fix_this_cpu

 drivers/idle/intel_idle.c | 56 ++++++++++++++++++++++++-----------------------
 1 file changed, 29 insertions(+), 27 deletions(-)

-- 
2.5.0

[PATCH 1/4] intel_idle: Consolidate auto-promotion/auto-demotion fixups

[Andy Lutomirski]

This eliminates some duplicate code and will make it easier
add fixup calls in places where they're currently missing.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 drivers/idle/intel_idle.c | 34 +++++++++++++---------------------
 1 file changed, 13 insertions(+), 21 deletions(-)

diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
index cd4510a63375..32b3e6049994 100644
--- a/drivers/idle/intel_idle.c
+++ b/drivers/idle/intel_idle.c
@@ -810,21 +810,21 @@ static struct notifier_block cpu_hotplug_notifier = {
 	.notifier_call = cpu_hotplug_notify,
 };
 
-static void auto_demotion_disable(void *dummy)
+static void fix_this_cpu(void *dummy)
 {
 	unsigned long long msr_bits;
 
-	rdmsrl(MSR_NHM_SNB_PKG_CST_CFG_CTL, msr_bits);
-	msr_bits &= ~(icpu->auto_demotion_disable_flags);
-	wrmsrl(MSR_NHM_SNB_PKG_CST_CFG_CTL, msr_bits);
-}
-static void c1e_promotion_disable(void *dummy)
-{
-	unsigned long long msr_bits;
+	if (icpu->auto_demotion_disable_flags) {
+		rdmsrl(MSR_NHM_SNB_PKG_CST_CFG_CTL, msr_bits);
+		msr_bits &= ~(icpu->auto_demotion_disable_flags);
+		wrmsrl(MSR_NHM_SNB_PKG_CST_CFG_CTL, msr_bits);
+	}
 
-	rdmsrl(MSR_IA32_POWER_CTL, msr_bits);
-	msr_bits &= ~0x2;
-	wrmsrl(MSR_IA32_POWER_CTL, msr_bits);
+	if (icpu->disable_promotion_to_c1e) {
+		rdmsrl(MSR_IA32_POWER_CTL, msr_bits);
+		msr_bits &= ~0x2;
+		wrmsrl(MSR_IA32_POWER_CTL, msr_bits);
+	}
 }
 
 static const struct idle_cpu idle_cpu_nehalem = {
@@ -1074,16 +1074,12 @@ static int __init intel_idle_cpuidle_driver_init(void)
 		drv->state_count += 1;
 	}
 
-	if (icpu->auto_demotion_disable_flags)
-		on_each_cpu(auto_demotion_disable, NULL, 1);
-
 	if (icpu->byt_auto_demotion_disable_flag) {
 		wrmsrl(MSR_CC6_DEMOTION_POLICY_CONFIG, 0);
 		wrmsrl(MSR_MC6_DEMOTION_POLICY_CONFIG, 0);
 	}
 
-	if (icpu->disable_promotion_to_c1e)	/* each-cpu is redundant */
-		on_each_cpu(c1e_promotion_disable, NULL, 1);
+	on_each_cpu(fix_this_cpu, NULL, 1);
 
 	return 0;
 }
@@ -1108,11 +1104,7 @@ static int intel_idle_cpu_init(int cpu)
 		return -EIO;
 	}
 
-	if (icpu->auto_demotion_disable_flags)
-		smp_call_function_single(cpu, auto_demotion_disable, NULL, 1);
-
-	if (icpu->disable_promotion_to_c1e)
-		smp_call_function_single(cpu, c1e_promotion_disable, NULL, 1);
+	smp_call_function_single(cpu, fix_this_cpu, NULL, 1);
 
 	return 0;
 }
-- 
2.5.0

[PATCH 2/4] intel_idle: Remove a broadcast MSR fixup at boot

[Andy Lutomirski]

intel_idle already fixes MSRs on each CPU when it registers for that
CPU, so it doesn't need to separately broadcast to all CPUs on
startup.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 drivers/idle/intel_idle.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
index 32b3e6049994..338df09ad60b 100644
--- a/drivers/idle/intel_idle.c
+++ b/drivers/idle/intel_idle.c
@@ -1079,8 +1079,6 @@ static int __init intel_idle_cpuidle_driver_init(void)
 		wrmsrl(MSR_MC6_DEMOTION_POLICY_CONFIG, 0);
 	}
 
-	on_each_cpu(fix_this_cpu, NULL, 1);
-
 	return 0;
 }
 
-- 
2.5.0

[PATCH 3/4] intel_idle: Fix MSRs after resume

[Andy Lutomirski]

Firmware that enables auto-promotion / auto-demotion flags we don't
like will probably re-enable them after suspend/resume.  Disable
them again after resume so they stay fixed.

I've seen this on my Dell XPS 13 9350.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 drivers/idle/intel_idle.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
index 338df09ad60b..e3d7d8bbc843 100644
--- a/drivers/idle/intel_idle.c
+++ b/drivers/idle/intel_idle.c
@@ -61,6 +61,7 @@
 #include <linux/notifier.h>
 #include <linux/cpu.h>
 #include <linux/module.h>
+#include <linux/syscore_ops.h>
 #include <asm/cpu_device_id.h>
 #include <asm/mwait.h>
 #include <asm/msr.h>
@@ -1026,6 +1027,15 @@ void intel_idle_state_table_update(void)
 	return;
 }
 
+static void intel_idle_resume(void)
+{
+	on_each_cpu(fix_this_cpu, NULL, 1);
+}
+
+static struct syscore_ops intel_idle_syscore_ops = {
+	.resume = intel_idle_resume,
+};
+
 /*
  * intel_idle_cpuidle_driver_init()
  * allocate, initialize cpuidle_states
@@ -1119,6 +1129,7 @@ static int __init intel_idle_init(void)
 	if (retval)
 		return retval;
 
+	register_syscore_ops(&intel_idle_syscore_ops);
 	intel_idle_cpuidle_driver_init();
 	retval = cpuidle_register_driver(&intel_idle_driver);
 	if (retval) {
@@ -1153,6 +1164,7 @@ static void __exit intel_idle_exit(void)
 {
 	intel_idle_cpuidle_devices_uninit();
 	cpuidle_unregister_driver(&intel_idle_driver);
+	unregister_syscore_ops(&intel_idle_syscore_ops);
 
 	cpu_notifier_register_begin();
 
-- 
2.5.0

[PATCH 4/4] intel_idle: Move BYT/CHT auto-demotion fixup into fix_this_cpu

[Andy Lutomirski]

The demotion policy config registers are per-package and these CPUs
only exist in single-package variants AFAIK, so it's not stricly
necessary to fix these MSRs on each core, but fixing them on resume
is still likely to be helpful, so move them into fix_this_cpu.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 drivers/idle/intel_idle.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
index e3d7d8bbc843..cbb02d28dc48 100644
--- a/drivers/idle/intel_idle.c
+++ b/drivers/idle/intel_idle.c
@@ -826,6 +826,11 @@ static void fix_this_cpu(void *dummy)
 		msr_bits &= ~0x2;
 		wrmsrl(MSR_IA32_POWER_CTL, msr_bits);
 	}
+
+	if (icpu->byt_auto_demotion_disable_flag) {
+		wrmsrl(MSR_CC6_DEMOTION_POLICY_CONFIG, 0);
+		wrmsrl(MSR_MC6_DEMOTION_POLICY_CONFIG, 0);
+	}
 }
 
 static const struct idle_cpu idle_cpu_nehalem = {
@@ -1084,11 +1089,6 @@ static int __init intel_idle_cpuidle_driver_init(void)
 		drv->state_count += 1;
 	}
 
-	if (icpu->byt_auto_demotion_disable_flag) {
-		wrmsrl(MSR_CC6_DEMOTION_POLICY_CONFIG, 0);
-		wrmsrl(MSR_MC6_DEMOTION_POLICY_CONFIG, 0);
-	}
-
 	return 0;
 }
 
-- 
2.5.0

Re: C1E auto-promotion suspend/resume

[Andy Lutomirski]

On Sun, Mar 13, 2016 at 9:31 PM, Brown, Len <len.brown@intel.com> wrote:
>> By BIOS (1.2.3 on a Dell XPS 13 9350) seems to want to enable C1E
>> auto-promotion (ugh!), which results in this difference across
>> suspend/resume according to turbostat:
>>
>> -cpu3: MSR_IA32_POWER_CTL: 0x0024005d (C1E auto-promotion: DISabled)
>> +cpu3: MSR_IA32_POWER_CTL: 0x0024005f (C1E auto-promotion: ENabled)
>>
>> Should intel_idle learn to re-disable idle promotion on resume?
>
> Yes, it seems that way.
>
> Go ahead and send a patch, or file a bug at bugzilla.kernel.org
> and we'll get to it.

Sent.  The only other differences I see across suspend/resume in
turbostat --debug (with taskset -c 0 to suppress spurious junk) are:

--- pre-susp.txt    2016-03-13 22:21:39.889337697 -0700
+++ post-susp.txt    2016-03-13 21:38:20.782503438 -0700
@@ -24,8 +24,8 @@
 cpu0: MSR_NHM_SNB_PKG_CST_CFG_CTL: 0x1e008006 (UNdemote-C3,
UNdemote-C1, demote-C3, demote-C1, locked: pkg-cstate-limit=6: pc8)
 cpu0: MSR_PM_ENABLE: 0x00000001 (HWP)
 cpu0: MSR_HWP_CAPABILITIES: 0x0108171c (high 0x1c guar 0x17 eff 0x8 low 0x1)
-cpu0: MSR_HWP_REQUEST: 0x80001c04 (min 0x4 max 0x1c des 0x0 epp 0x80
window 0x0 pkg 0x0)
-cpu0: MSR_HWP_INTERRUPT: 0x00000001 (EN_Guaranteed_Perf_Change,
Dis_Excursion_Min)
+cpu0: MSR_HWP_REQUEST: 0x8000ff01 (min 0x1 max 0xff des 0x0 epp 0x80
window 0x0 pkg 0x0)
+cpu0: MSR_HWP_INTERRUPT: 0x00000000 (Dis_Guaranteed_Perf_Change,
Dis_Excursion_Min)
 cpu0: MSR_HWP_STATUS: 0x00000000 (No-Guaranteed_Perf_Change, No-Excursion_Min)
 cpu0: MSR_IA32_ENERGY_PERF_BIAS: 0x00000006 (balanced)
 cpu0: MSR_RAPL_POWER_UNIT: 0x000a0e03 (0.125000 Watts, 0.000061
Joules, 0.000977 sec.)
@@ -36,6 +36,6 @@
 cpu0: MSR_DRAM_POWER_LIMIT: 0x5400de00000000 (UNlocked)
 cpu0: DRAM Limit: DISabled (0.000000 Watts, 0.000977 sec, clamp DISabled)
 cpu0: MSR_IA32_TEMPERATURE_TARGET: 0x00640000 (100 C)
-cpu0: MSR_IA32_PACKAGE_THERM_STATUS: 0x88360000 (46 C)
-cpu0: MSR_IA32_THERM_STATUS: 0x88360000 (46 C +/- 1)
-cpu1: MSR_IA32_THERM_STATUS: 0x88360000 (46 C +/- 1)
+cpu0: MSR_IA32_PACKAGE_THERM_STATUS: 0x883c0000 (40 C)
+cpu0: MSR_IA32_THERM_STATUS: 0x883c0000 (40 C +/- 1)
+cpu1: MSR_IA32_THERM_STATUS: 0x88400000 (36 C +/- 1)


Are either of those potentially interesting?

Re: [PATCH 3/4] intel_idle: Fix MSRs after resume

[Andy Lutomirski]

On Sun, Mar 13, 2016 at 9:48 PM, Andy Lutomirski <luto@kernel.org> wrote:
> Firmware that enables auto-promotion / auto-demotion flags we don't
> like will probably re-enable them after suspend/resume.  Disable
> them again after resume so they stay fixed.
>
> I've seen this on my Dell XPS 13 9350.

This isn't right -- syscore's resume hook is called too early for
smp_call_function_many.  I think the right way is through a device's
dev_pm_ops (SET_SYSTEM_SLEEP_PM_OPS), but there don't seem to be any
devices associated with intel_idle.

--Andy

>
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>  drivers/idle/intel_idle.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
> index 338df09ad60b..e3d7d8bbc843 100644
> --- a/drivers/idle/intel_idle.c
> +++ b/drivers/idle/intel_idle.c
> @@ -61,6 +61,7 @@
>  #include <linux/notifier.h>
>  #include <linux/cpu.h>
>  #include <linux/module.h>
> +#include <linux/syscore_ops.h>
>  #include <asm/cpu_device_id.h>
>  #include <asm/mwait.h>
>  #include <asm/msr.h>
> @@ -1026,6 +1027,15 @@ void intel_idle_state_table_update(void)
>         return;
>  }
>
> +static void intel_idle_resume(void)
> +{
> +       on_each_cpu(fix_this_cpu, NULL, 1);
> +}
> +
> +static struct syscore_ops intel_idle_syscore_ops = {
> +       .resume = intel_idle_resume,
> +};
> +
>  /*
>   * intel_idle_cpuidle_driver_init()
>   * allocate, initialize cpuidle_states
> @@ -1119,6 +1129,7 @@ static int __init intel_idle_init(void)
>         if (retval)
>                 return retval;
>
> +       register_syscore_ops(&intel_idle_syscore_ops);
>         intel_idle_cpuidle_driver_init();
>         retval = cpuidle_register_driver(&intel_idle_driver);
>         if (retval) {
> @@ -1153,6 +1164,7 @@ static void __exit intel_idle_exit(void)
>  {
>         intel_idle_cpuidle_devices_uninit();
>         cpuidle_unregister_driver(&intel_idle_driver);
> +       unregister_syscore_ops(&intel_idle_syscore_ops);
>
>         cpu_notifier_register_begin();
>
> --
> 2.5.0
>



-- 
Andy Lutomirski
AMA Capital Management, LLC