[PATCH 0/3] idle, Honor Hardware Disabled States

[PATCH 0/3] idle, Honor Hardware Disabled States

[Prarit Bhargava]

Some Intel processors have cstates limited by MSR_PKG_CST_CONFIG_CONTROL
(0xE2), which specifies the lowest processor-specific C-state code name
(consuming the least power).  The intel_idle driver does not query this MSR to
see if states are limited by hardware.  In the case of the i7-6700HQ the value
of bits 3:0 on the MSR are 0x6, meaning that C9-SKL (0x7) and C10-SKL (0x8)
should have been disabled.

This patch adds a limit field to the cpuidle_state struct to hold the
cstate encodings for each processor, and queries the values the values
to confirm support.

After applying this patchset the system now boots and reports that
some states have been disabled:

[root@dhcp40-125 ~]# dmesg | grep intel_idle
[    1.163367] intel_idle: MWAIT substates: 0x11142120
[    1.163368] intel_idle: v0.4 model 0x5E
[    1.163369] intel_idle: lapic_timer_reliable_states 0xffffffff
[    1.163370] intel_idle: state C9-SKL (0x7) is disabled.  Max Package limit is 0x6.
[    1.163371] intel_idle: state C10-SKL (0x8) is disabled.  Max Package limit is 0x6.

Cc: x86@kernel.org
Cc: Len Brown <lenb@kernel.org>
Cc: "Peter Zijlstra (Intel)" <peterz@infradead.org>
Cc: Borislav Petkov <bp@suse.de>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Dasaratharaman Chandramouli <dasaratharaman.chandramouli@intel.com>
Cc: linux-pm@vger.kernel.org
Signed-off-by: Prarit Bhargava <prarit@redhat.com>

Prarit Bhargava (3):
  idle, rename MSR_NHM_SNB_PKG_CST_CFG_CTL to
    MSR_PKG_CST_CONFIG_CONTROL
  intel_idle, Introduce cstate limits and fix hardware cstate disable
    value
  cpuidle, Prevent users from enabling cstates that are disabled in
    Hardware

 arch/x86/include/asm/msr-index.h      |    2 +-
 drivers/cpuidle/governors/ladder.c    |    3 ++
 drivers/cpuidle/governors/menu.c      |    4 ++
 drivers/cpuidle/sysfs.c               |   34 +++++++++++-
 drivers/idle/intel_idle.c             |   93 ++++++++++++++++++++++++++++++++-
 include/linux/cpuidle.h               |   11 +++-
 tools/power/x86/turbostat/turbostat.8 |    2 +-
 tools/power/x86/turbostat/turbostat.c |   10 ++--
 8 files changed, 146 insertions(+), 13 deletions(-)

-- 
1.7.9.3

[PATCH 1/3] idle, rename MSR_NHM_SNB_PKG_CST_CFG_CTL to MSR_PKG_CST_CONFIG_CONTROL

[Prarit Bhargava]

MSR 0xE2 is not a Nehalem specific register and is used by newer
processors.  This patch renames the processor to the name given in the
Intel SDM [1]

[1] http://www.intel.com/content/www/us/en/processors/architectures-software-developer-manuals.html

Cc: x86@kernel.org
Cc: Len Brown <lenb@kernel.org>
Cc: "Peter Zijlstra (Intel)" <peterz@infradead.org>
Cc: Borislav Petkov <bp@suse.de>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Dasaratharaman Chandramouli <dasaratharaman.chandramouli@intel.com>
Cc: linux-pm@vger.kernel.org
Signed-off-by: Prarit Bhargava <prarit@redhat.com>
---
 arch/x86/include/asm/msr-index.h      |    2 +-
 drivers/idle/intel_idle.c             |    4 ++--
 tools/power/x86/turbostat/turbostat.8 |    2 +-
 tools/power/x86/turbostat/turbostat.c |   10 +++++-----
 4 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 984ab75..63909ba 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -42,7 +42,7 @@
 #define MSR_FSB_FREQ			0x000000cd
 #define MSR_PLATFORM_INFO		0x000000ce
 
-#define MSR_NHM_SNB_PKG_CST_CFG_CTL	0x000000e2
+#define MSR_PKG_CST_CONFIG_CONTROL	0x000000e2
 #define NHM_C3_AUTO_DEMOTE		(1UL << 25)
 #define NHM_C1_AUTO_DEMOTE		(1UL << 26)
 #define ATM_LNC_C6_AUTO_DEMOTE		(1UL << 25)
diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
index cd4510a..2609dff 100644
--- a/drivers/idle/intel_idle.c
+++ b/drivers/idle/intel_idle.c
@@ -814,9 +814,9 @@ static void auto_demotion_disable(void *dummy)
 {
 	unsigned long long msr_bits;
 
-	rdmsrl(MSR_NHM_SNB_PKG_CST_CFG_CTL, msr_bits);
+	rdmsrl(MSR_PKG_CST_CONFIG_CONTROL, msr_bits);
 	msr_bits &= ~(icpu->auto_demotion_disable_flags);
-	wrmsrl(MSR_NHM_SNB_PKG_CST_CFG_CTL, msr_bits);
+	wrmsrl(MSR_PKG_CST_CONFIG_CONTROL, msr_bits);
 }
 static void c1e_promotion_disable(void *dummy)
 {
diff --git a/tools/power/x86/turbostat/turbostat.8 b/tools/power/x86/turbostat/turbostat.8
index 622db68..7973d2a 100644
--- a/tools/power/x86/turbostat/turbostat.8
+++ b/tools/power/x86/turbostat/turbostat.8
@@ -118,7 +118,7 @@ cpu0: MSR_NHM_PLATFORM_INFO: 0x80838f3012300
 8 * 100 = 800 MHz max efficiency
 35 * 100 = 3500 MHz TSC frequency
 cpu0: MSR_IA32_POWER_CTL: 0x0004005d (C1E auto-promotion: DISabled)
-cpu0: MSR_NHM_SNB_PKG_CST_CFG_CTL: 0x1e000400 (UNdemote-C3, UNdemote-C1, demote-C3, demote-C1, UNlocked: pkg-cstate-limit=0: pc0)
+cpu0: MSR_PKG_CST_CONFIG_CONTROL: 0x1e000400 (UNdemote-C3, UNdemote-C1, demote-C3, demote-C1, UNlocked: pkg-cstate-limit=0: pc0)
 cpu0: MSR_NHM_TURBO_RATIO_LIMIT: 0x25262727
 37 * 100 = 3700 MHz max turbo 4 active cores
 38 * 100 = 3800 MHz max turbo 3 active cores
diff --git a/tools/power/x86/turbostat/turbostat.c b/tools/power/x86/turbostat/turbostat.c
index 3fa94e2..289ff9f 100644
--- a/tools/power/x86/turbostat/turbostat.c
+++ b/tools/power/x86/turbostat/turbostat.c
@@ -1384,12 +1384,12 @@ dump_nhm_cst_cfg(void)
 {
 	unsigned long long msr;
 
-	get_msr(base_cpu, MSR_NHM_SNB_PKG_CST_CFG_CTL, &msr);
+	get_msr(base_cpu, MSR_PKG_CST_CONFIG_CONTROL, &msr);
 
 #define SNB_C1_AUTO_UNDEMOTE              (1UL << 27)
 #define SNB_C3_AUTO_UNDEMOTE              (1UL << 28)
 
-	fprintf(stderr, "cpu%d: MSR_NHM_SNB_PKG_CST_CFG_CTL: 0x%08llx", base_cpu, msr);
+	fprintf(stderr, "cpu%d: MSR_PKG_CST_CONFIG_CONTROL: 0x%08llx", base_cpu, msr);
 
 	fprintf(stderr, " (%s%s%s%s%slocked: pkg-cstate-limit=%d: %s)\n",
 		(msr & SNB_C3_AUTO_UNDEMOTE) ? "UNdemote-C3, " : "",
@@ -1808,7 +1808,7 @@ void check_permissions()
  * MSR_SMI_COUNT                   0x00000034
  *
  * MSR_PLATFORM_INFO               0x000000ce
- * MSR_NHM_SNB_PKG_CST_CFG_CTL     0x000000e2
+ * MSR_PKG_CST_CONFIG_CONTROL     0x000000e2
  *
  * MSR_PKG_C3_RESIDENCY            0x000003f8
  * MSR_PKG_C6_RESIDENCY            0x000003f9
@@ -1816,7 +1816,7 @@ void check_permissions()
  * MSR_CORE_C6_RESIDENCY           0x000003fd
  *
  * Side effect:
- * sets global pkg_cstate_limit to decode MSR_NHM_SNB_PKG_CST_CFG_CTL
+ * sets global pkg_cstate_limit to decode MSR_PKG_CST_CONFIG_CONTROL
  */
 int probe_nhm_msrs(unsigned int family, unsigned int model)
 {
@@ -1873,7 +1873,7 @@ int probe_nhm_msrs(unsigned int family, unsigned int model)
 	default:
 		return 0;
 	}
-	get_msr(base_cpu, MSR_NHM_SNB_PKG_CST_CFG_CTL, &msr);
+	get_msr(base_cpu, MSR_PKG_CST_CONFIG_CONTROL, &msr);
 	pkg_cstate_limit = pkg_cstate_limits[msr & 0xF];
 
 	get_msr(base_cpu, MSR_PLATFORM_INFO, &msr);
-- 
1.7.9.3

Re: [PATCH 1/3] idle, rename MSR_NHM_SNB_PKG_CST_CFG_CTL to MSR_PKG_CST_CONFIG_CONTROL

[Len Brown]

I agree with this intent, but re-names are a headache.

The last time we re-named an MSR, we had to add the new name,
try to get patches in flight to cut over to it, and then a few
releases later, remove the old name.

Right now, we have the same situation as patches in flight use this register,
so I'll not apply this patch right now.

thanks,
-Len




On Mon, Mar 21, 2016 at 8:49 AM, Prarit Bhargava <prarit@redhat.com> wrote:
> MSR 0xE2 is not a Nehalem specific register and is used by newer
> processors.  This patch renames the processor to the name given in the
> Intel SDM [1]
>
> [1] http://www.intel.com/content/www/us/en/processors/architectures-software-developer-manuals.html
>
> Cc: x86@kernel.org
> Cc: Len Brown <lenb@kernel.org>
> Cc: "Peter Zijlstra (Intel)" <peterz@infradead.org>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Andi Kleen <ak@linux.intel.com>
> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Dasaratharaman Chandramouli <dasaratharaman.chandramouli@intel.com>
> Cc: linux-pm@vger.kernel.org
> Signed-off-by: Prarit Bhargava <prarit@redhat.com>
> ---
>  arch/x86/include/asm/msr-index.h      |    2 +-
>  drivers/idle/intel_idle.c             |    4 ++--
>  tools/power/x86/turbostat/turbostat.8 |    2 +-
>  tools/power/x86/turbostat/turbostat.c |   10 +++++-----
>  4 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> index 984ab75..63909ba 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -42,7 +42,7 @@
>  #define MSR_FSB_FREQ                   0x000000cd
>  #define MSR_PLATFORM_INFO              0x000000ce
>
> -#define MSR_NHM_SNB_PKG_CST_CFG_CTL    0x000000e2
> +#define MSR_PKG_CST_CONFIG_CONTROL     0x000000e2
>  #define NHM_C3_AUTO_DEMOTE             (1UL << 25)
>  #define NHM_C1_AUTO_DEMOTE             (1UL << 26)
>  #define ATM_LNC_C6_AUTO_DEMOTE         (1UL << 25)
> diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
> index cd4510a..2609dff 100644
> --- a/drivers/idle/intel_idle.c
> +++ b/drivers/idle/intel_idle.c
> @@ -814,9 +814,9 @@ static void auto_demotion_disable(void *dummy)
>  {
>         unsigned long long msr_bits;
>
> -       rdmsrl(MSR_NHM_SNB_PKG_CST_CFG_CTL, msr_bits);
> +       rdmsrl(MSR_PKG_CST_CONFIG_CONTROL, msr_bits);
>         msr_bits &= ~(icpu->auto_demotion_disable_flags);
> -       wrmsrl(MSR_NHM_SNB_PKG_CST_CFG_CTL, msr_bits);
> +       wrmsrl(MSR_PKG_CST_CONFIG_CONTROL, msr_bits);
>  }
>  static void c1e_promotion_disable(void *dummy)
>  {
> diff --git a/tools/power/x86/turbostat/turbostat.8 b/tools/power/x86/turbostat/turbostat.8
> index 622db68..7973d2a 100644
> --- a/tools/power/x86/turbostat/turbostat.8
> +++ b/tools/power/x86/turbostat/turbostat.8
> @@ -118,7 +118,7 @@ cpu0: MSR_NHM_PLATFORM_INFO: 0x80838f3012300
>  8 * 100 = 800 MHz max efficiency
>  35 * 100 = 3500 MHz TSC frequency
>  cpu0: MSR_IA32_POWER_CTL: 0x0004005d (C1E auto-promotion: DISabled)
> -cpu0: MSR_NHM_SNB_PKG_CST_CFG_CTL: 0x1e000400 (UNdemote-C3, UNdemote-C1, demote-C3, demote-C1, UNlocked: pkg-cstate-limit=0: pc0)
> +cpu0: MSR_PKG_CST_CONFIG_CONTROL: 0x1e000400 (UNdemote-C3, UNdemote-C1, demote-C3, demote-C1, UNlocked: pkg-cstate-limit=0: pc0)
>  cpu0: MSR_NHM_TURBO_RATIO_LIMIT: 0x25262727
>  37 * 100 = 3700 MHz max turbo 4 active cores
>  38 * 100 = 3800 MHz max turbo 3 active cores
> diff --git a/tools/power/x86/turbostat/turbostat.c b/tools/power/x86/turbostat/turbostat.c
> index 3fa94e2..289ff9f 100644
> --- a/tools/power/x86/turbostat/turbostat.c
> +++ b/tools/power/x86/turbostat/turbostat.c
> @@ -1384,12 +1384,12 @@ dump_nhm_cst_cfg(void)
>  {
>         unsigned long long msr;
>
> -       get_msr(base_cpu, MSR_NHM_SNB_PKG_CST_CFG_CTL, &msr);
> +       get_msr(base_cpu, MSR_PKG_CST_CONFIG_CONTROL, &msr);
>
>  #define SNB_C1_AUTO_UNDEMOTE              (1UL << 27)
>  #define SNB_C3_AUTO_UNDEMOTE              (1UL << 28)
>
> -       fprintf(stderr, "cpu%d: MSR_NHM_SNB_PKG_CST_CFG_CTL: 0x%08llx", base_cpu, msr);
> +       fprintf(stderr, "cpu%d: MSR_PKG_CST_CONFIG_CONTROL: 0x%08llx", base_cpu, msr);
>
>         fprintf(stderr, " (%s%s%s%s%slocked: pkg-cstate-limit=%d: %s)\n",
>                 (msr & SNB_C3_AUTO_UNDEMOTE) ? "UNdemote-C3, " : "",
> @@ -1808,7 +1808,7 @@ void check_permissions()
>   * MSR_SMI_COUNT                   0x00000034
>   *
>   * MSR_PLATFORM_INFO               0x000000ce
> - * MSR_NHM_SNB_PKG_CST_CFG_CTL     0x000000e2
> + * MSR_PKG_CST_CONFIG_CONTROL     0x000000e2
>   *
>   * MSR_PKG_C3_RESIDENCY            0x000003f8
>   * MSR_PKG_C6_RESIDENCY            0x000003f9
> @@ -1816,7 +1816,7 @@ void check_permissions()
>   * MSR_CORE_C6_RESIDENCY           0x000003fd
>   *
>   * Side effect:
> - * sets global pkg_cstate_limit to decode MSR_NHM_SNB_PKG_CST_CFG_CTL
> + * sets global pkg_cstate_limit to decode MSR_PKG_CST_CONFIG_CONTROL
>   */
>  int probe_nhm_msrs(unsigned int family, unsigned int model)
>  {
> @@ -1873,7 +1873,7 @@ int probe_nhm_msrs(unsigned int family, unsigned int model)
>         default:
>                 return 0;
>         }
> -       get_msr(base_cpu, MSR_NHM_SNB_PKG_CST_CFG_CTL, &msr);
> +       get_msr(base_cpu, MSR_PKG_CST_CONFIG_CONTROL, &msr);
>         pkg_cstate_limit = pkg_cstate_limits[msr & 0xF];
>
>         get_msr(base_cpu, MSR_PLATFORM_INFO, &msr);
> --
> 1.7.9.3
>



-- 
Len Brown, Intel Open Source Technology Center

[PATCH 2/3] intel_idle, Introduce cstate limits and fix hardware cstate disable value

[Prarit Bhargava]

Some Skylake Intel i7-6700HQ processors have cstates limited by
MSR_PKG_CST_CONFIG_CONTROL (0xE2), which specifies the lowest
processor-specific C-state code name (consuming the least power).  The
intel_idle driver does not query this MSR to see if states are limited by
hardware.  In the case of the i7-6700HQ the value of bits 3:0 on the MSR
are 0x6, meaning that C9-SKL (0x7) and C10-SKL (0x8) should have been
disabled.

This patch adds a limit field to the cpuidle_state struct to hold the
cstate encodings for each processor, and queries the values the values
to confirm support.

After applying this patch the Skylake-H system now boots and reports that
some states have been disabled:

[root@dhcp40-125 ~]# dmesg | grep intel_idle
[    1.163367] intel_idle: MWAIT substates: 0x11142120
[    1.163368] intel_idle: v0.4 model 0x5E
[    1.163369] intel_idle: lapic_timer_reliable_states 0xffffffff
[    1.163370] intel_idle: state C9-SKL (0x7) is disabled.  Max Package limit is 0x6.
[    1.163371] intel_idle: state C10-SKL (0x8) is disabled.  Max Package limit is 0x6.

[root@dhcp40-125 ~]# egrep ^ /sys/devices/system/cpu/cpu0/cpuidle/state?/disable
/sys/devices/system/cpu/cpu0/cpuidle/state0/disable:1:0
/sys/devices/system/cpu/cpu0/cpuidle/state1/disable:1:0
/sys/devices/system/cpu/cpu0/cpuidle/state2/disable:1:0
/sys/devices/system/cpu/cpu0/cpuidle/state3/disable:1:0
/sys/devices/system/cpu/cpu0/cpuidle/state4/disable:1:0
/sys/devices/system/cpu/cpu0/cpuidle/state5/disable:1:0
/sys/devices/system/cpu/cpu0/cpuidle/state6/disable:1:0
/sys/devices/system/cpu/cpu0/cpuidle/state7/disable:1:1
/sys/devices/system/cpu/cpu0/cpuidle/state8/disable:1:1

The cstate encodings were determined from the Intel SDM [1].

[1] http://www.intel.com/content/www/us/en/processors/architectures-software-developer-manuals.html

Cc: x86@kernel.org
Cc: Len Brown <lenb@kernel.org>
Cc: "Peter Zijlstra (Intel)" <peterz@infradead.org>
Cc: Borislav Petkov <bp@suse.de>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Dasaratharaman Chandramouli <dasaratharaman.chandramouli@intel.com>
Cc: linux-pm@vger.kernel.org
Signed-off-by: Prarit Bhargava <prarit@redhat.com>
---
 drivers/idle/intel_idle.c |   87 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/cpuidle.h   |    1 +
 2 files changed, 88 insertions(+)

diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
index 2609dff..58bc913 100644
--- a/drivers/idle/intel_idle.c
+++ b/drivers/idle/intel_idle.c
@@ -130,6 +130,7 @@ static struct cpuidle_state nehalem_cstates[] = {
 	{
 		.name = "C1-NHM",
 		.desc = "MWAIT 0x00",
+		.limit = 0x1,
 		.flags = MWAIT2flg(0x00),
 		.exit_latency = 3,
 		.target_residency = 6,
@@ -138,6 +139,7 @@ static struct cpuidle_state nehalem_cstates[] = {
 	{
 		.name = "C1E-NHM",
 		.desc = "MWAIT 0x01",
+		.limit = 0x1,
 		.flags = MWAIT2flg(0x01),
 		.exit_latency = 10,
 		.target_residency = 20,
@@ -146,6 +148,7 @@ static struct cpuidle_state nehalem_cstates[] = {
 	{
 		.name = "C3-NHM",
 		.desc = "MWAIT 0x10",
+		.limit = 0x2,
 		.flags = MWAIT2flg(0x10) | CPUIDLE_FLAG_TLB_FLUSHED,
 		.exit_latency = 20,
 		.target_residency = 80,
@@ -154,6 +157,7 @@ static struct cpuidle_state nehalem_cstates[] = {
 	{
 		.name = "C6-NHM",
 		.desc = "MWAIT 0x20",
+		.limit = 0x3,
 		.flags = MWAIT2flg(0x20) | CPUIDLE_FLAG_TLB_FLUSHED,
 		.exit_latency = 200,
 		.target_residency = 800,
@@ -167,6 +171,7 @@ static struct cpuidle_state snb_cstates[] = {
 	{
 		.name = "C1-SNB",
 		.desc = "MWAIT 0x00",
+		.limit = 0x0,
 		.flags = MWAIT2flg(0x00),
 		.exit_latency = 2,
 		.target_residency = 2,
@@ -175,6 +180,7 @@ static struct cpuidle_state snb_cstates[] = {
 	{
 		.name = "C1E-SNB",
 		.desc = "MWAIT 0x01",
+		.limit = 0x0,
 		.flags = MWAIT2flg(0x01),
 		.exit_latency = 10,
 		.target_residency = 20,
@@ -183,6 +189,7 @@ static struct cpuidle_state snb_cstates[] = {
 	{
 		.name = "C3-SNB",
 		.desc = "MWAIT 0x10",
+		.limit = 0x7, /* cannot limit to C3 */
 		.flags = MWAIT2flg(0x10) | CPUIDLE_FLAG_TLB_FLUSHED,
 		.exit_latency = 80,
 		.target_residency = 211,
@@ -191,6 +198,7 @@ static struct cpuidle_state snb_cstates[] = {
 	{
 		.name = "C6-SNB",
 		.desc = "MWAIT 0x20",
+		.limit = 0x2,
 		.flags = MWAIT2flg(0x20) | CPUIDLE_FLAG_TLB_FLUSHED,
 		.exit_latency = 104,
 		.target_residency = 345,
@@ -199,6 +207,7 @@ static struct cpuidle_state snb_cstates[] = {
 	{
 		.name = "C7-SNB",
 		.desc = "MWAIT 0x30",
+		.limit = 0x4,
 		.flags = MWAIT2flg(0x30) | CPUIDLE_FLAG_TLB_FLUSHED,
 		.exit_latency = 109,
 		.target_residency = 345,
@@ -212,6 +221,7 @@ static struct cpuidle_state byt_cstates[] = {
 	{
 		.name = "C1-BYT",
 		.desc = "MWAIT 0x00",
+		.limit = 0x1,
 		.flags = MWAIT2flg(0x00),
 		.exit_latency = 1,
 		.target_residency = 1,
@@ -220,6 +230,7 @@ static struct cpuidle_state byt_cstates[] = {
 	{
 		.name = "C6N-BYT",
 		.desc = "MWAIT 0x58",
+		.limit = 0x6,
 		.flags = MWAIT2flg(0x58) | CPUIDLE_FLAG_TLB_FLUSHED,
 		.exit_latency = 300,
 		.target_residency = 275,
@@ -229,6 +240,7 @@ static struct cpuidle_state byt_cstates[] = {
 		.name = "C6S-BYT",
 		.desc = "MWAIT 0x52",
 		.flags = MWAIT2flg(0x52) | CPUIDLE_FLAG_TLB_FLUSHED,
+		.limit = 0x6,
 		.exit_latency = 500,
 		.target_residency = 560,
 		.enter = &intel_idle,
@@ -236,6 +248,7 @@ static struct cpuidle_state byt_cstates[] = {
 	{
 		.name = "C7-BYT",
 		.desc = "MWAIT 0x60",
+		.limit = 0x7,
 		.flags = MWAIT2flg(0x60) | CPUIDLE_FLAG_TLB_FLUSHED,
 		.exit_latency = 1200,
 		.target_residency = 4000,
@@ -244,6 +257,7 @@ static struct cpuidle_state byt_cstates[] = {
 	{
 		.name = "C7S-BYT",
 		.desc = "MWAIT 0x64",
+		.limit = 0x7,
 		.flags = MWAIT2flg(0x64) | CPUIDLE_FLAG_TLB_FLUSHED,
 		.exit_latency = 10000,
 		.target_residency = 20000,
@@ -257,6 +271,7 @@ static struct cpuidle_state cht_cstates[] = {
 	{
 		.name = "C1-CHT",
 		.desc = "MWAIT 0x00",
+		.limit = 0x1,
 		.flags = MWAIT2flg(0x00),
 		.exit_latency = 1,
 		.target_residency = 1,
@@ -265,6 +280,7 @@ static struct cpuidle_state cht_cstates[] = {
 	{
 		.name = "C6N-CHT",
 		.desc = "MWAIT 0x58",
+		.limit = 0x6,
 		.flags = MWAIT2flg(0x58) | CPUIDLE_FLAG_TLB_FLUSHED,
 		.exit_latency = 80,
 		.target_residency = 275,
@@ -273,6 +289,7 @@ static struct cpuidle_state cht_cstates[] = {
 	{
 		.name = "C6S-CHT",
 		.desc = "MWAIT 0x52",
+		.limit = 0x6,
 		.flags = MWAIT2flg(0x52) | CPUIDLE_FLAG_TLB_FLUSHED,
 		.exit_latency = 200,
 		.target_residency = 560,
@@ -281,6 +298,7 @@ static struct cpuidle_state cht_cstates[] = {
 	{
 		.name = "C7-CHT",
 		.desc = "MWAIT 0x60",
+		.limit = 0x7,
 		.flags = MWAIT2flg(0x60) | CPUIDLE_FLAG_TLB_FLUSHED,
 		.exit_latency = 1200,
 		.target_residency = 4000,
@@ -289,6 +307,7 @@ static struct cpuidle_state cht_cstates[] = {
 	{
 		.name = "C7S-CHT",
 		.desc = "MWAIT 0x64",
+		.limit = 0x7,
 		.flags = MWAIT2flg(0x64) | CPUIDLE_FLAG_TLB_FLUSHED,
 		.exit_latency = 10000,
 		.target_residency = 20000,
@@ -302,6 +321,7 @@ static struct cpuidle_state ivb_cstates[] = {
 	{
 		.name = "C1-IVB",
 		.desc = "MWAIT 0x00",
+		.limit = 0x1,
 		.flags = MWAIT2flg(0x00),
 		.exit_latency = 1,
 		.target_residency = 1,
@@ -310,6 +330,7 @@ static struct cpuidle_state ivb_cstates[] = {
 	{
 		.name = "C1E-IVB",
 		.desc = "MWAIT 0x01",
+		.limit = 0x1,
 		.flags = MWAIT2flg(0x01),
 		.exit_latency = 10,
 		.target_residency = 20,
@@ -318,6 +339,7 @@ static struct cpuidle_state ivb_cstates[] = {
 	{
 		.name = "C3-IVB",
 		.desc = "MWAIT 0x10",
+		.limit = 0x2,
 		.flags = MWAIT2flg(0x10) | CPUIDLE_FLAG_TLB_FLUSHED,
 		.exit_latency = 59,
 		.target_residency = 156,
@@ -326,6 +348,7 @@ static struct cpuidle_state ivb_cstates[] = {
 	{
 		.name = "C6-IVB",
 		.desc = "MWAIT 0x20",
+		.limit = 0x3,
 		.flags = MWAIT2flg(0x20) | CPUIDLE_FLAG_TLB_FLUSHED,
 		.exit_latency = 80,
 		.target_residency = 300,
@@ -334,6 +357,7 @@ static struct cpuidle_state ivb_cstates[] = {
 	{
 		.name = "C7-IVB",
 		.desc = "MWAIT 0x30",
+		.limit = 0x4,
 		.flags = MWAIT2flg(0x30) | CPUIDLE_FLAG_TLB_FLUSHED,
 		.exit_latency = 87,
 		.target_residency = 300,
@@ -347,6 +371,7 @@ static struct cpuidle_state ivt_cstates[] = {
 	{
 		.name = "C1-IVT",
 		.desc = "MWAIT 0x00",
+		.limit = 0x0,
 		.flags = MWAIT2flg(0x00),
 		.exit_latency = 1,
 		.target_residency = 1,
@@ -355,6 +380,7 @@ static struct cpuidle_state ivt_cstates[] = {
 	{
 		.name = "C1E-IVT",
 		.desc = "MWAIT 0x01",
+		.limit = 0x0,
 		.flags = MWAIT2flg(0x01),
 		.exit_latency = 10,
 		.target_residency = 80,
@@ -363,6 +389,7 @@ static struct cpuidle_state ivt_cstates[] = {
 	{
 		.name = "C3-IVT",
 		.desc = "MWAIT 0x10",
+		.limit = 0x7, /* cannot limit to C3 */
 		.flags = MWAIT2flg(0x10) | CPUIDLE_FLAG_TLB_FLUSHED,
 		.exit_latency = 59,
 		.target_residency = 156,
@@ -371,6 +398,7 @@ static struct cpuidle_state ivt_cstates[] = {
 	{
 		.name = "C6-IVT",
 		.desc = "MWAIT 0x20",
+		.limit = 0x3,
 		.flags = MWAIT2flg(0x20) | CPUIDLE_FLAG_TLB_FLUSHED,
 		.exit_latency = 82,
 		.target_residency = 300,
@@ -384,6 +412,7 @@ static struct cpuidle_state ivt_cstates_4s[] = {
 	{
 		.name = "C1-IVT-4S",
 		.desc = "MWAIT 0x00",
+		.limit = 0x0,
 		.flags = MWAIT2flg(0x00),
 		.exit_latency = 1,
 		.target_residency = 1,
@@ -392,6 +421,7 @@ static struct cpuidle_state ivt_cstates_4s[] = {
 	{
 		.name = "C1E-IVT-4S",
 		.desc = "MWAIT 0x01",
+		.limit = 0x0,
 		.flags = MWAIT2flg(0x01),
 		.exit_latency = 10,
 		.target_residency = 250,
@@ -400,6 +430,7 @@ static struct cpuidle_state ivt_cstates_4s[] = {
 	{
 		.name = "C3-IVT-4S",
 		.desc = "MWAIT 0x10",
+		.limit = 0x7, /* cannot limit to C3 */
 		.flags = MWAIT2flg(0x10) | CPUIDLE_FLAG_TLB_FLUSHED,
 		.exit_latency = 59,
 		.target_residency = 300,
@@ -408,6 +439,7 @@ static struct cpuidle_state ivt_cstates_4s[] = {
 	{
 		.name = "C6-IVT-4S",
 		.desc = "MWAIT 0x20",
+		.limit = 0x3,
 		.flags = MWAIT2flg(0x20) | CPUIDLE_FLAG_TLB_FLUSHED,
 		.exit_latency = 84,
 		.target_residency = 400,
@@ -421,6 +453,7 @@ static struct cpuidle_state ivt_cstates_8s[] = {
 	{
 		.name = "C1-IVT-8S",
 		.desc = "MWAIT 0x00",
+		.limit = 0x0,
 		.flags = MWAIT2flg(0x00),
 		.exit_latency = 1,
 		.target_residency = 1,
@@ -429,6 +462,7 @@ static struct cpuidle_state ivt_cstates_8s[] = {
 	{
 		.name = "C1E-IVT-8S",
 		.desc = "MWAIT 0x01",
+		.limit = 0x0,
 		.flags = MWAIT2flg(0x01),
 		.exit_latency = 10,
 		.target_residency = 500,
@@ -437,6 +471,7 @@ static struct cpuidle_state ivt_cstates_8s[] = {
 	{
 		.name = "C3-IVT-8S",
 		.desc = "MWAIT 0x10",
+		.limit = 0x7, /* cannot limit to C3 */
 		.flags = MWAIT2flg(0x10) | CPUIDLE_FLAG_TLB_FLUSHED,
 		.exit_latency = 59,
 		.target_residency = 600,
@@ -445,6 +480,7 @@ static struct cpuidle_state ivt_cstates_8s[] = {
 	{
 		.name = "C6-IVT-8S",
 		.desc = "MWAIT 0x20",
+		.limit = 0x3,
 		.flags = MWAIT2flg(0x20) | CPUIDLE_FLAG_TLB_FLUSHED,
 		.exit_latency = 88,
 		.target_residency = 700,
@@ -458,6 +494,7 @@ static struct cpuidle_state hsw_cstates[] = {
 	{
 		.name = "C1-HSW",
 		.desc = "MWAIT 0x00",
+		.limit = 0x1,
 		.flags = MWAIT2flg(0x00),
 		.exit_latency = 2,
 		.target_residency = 2,
@@ -466,6 +503,7 @@ static struct cpuidle_state hsw_cstates[] = {
 	{
 		.name = "C1E-HSW",
 		.desc = "MWAIT 0x01",
+		.limit = 0x1,
 		.flags = MWAIT2flg(0x01),
 		.exit_latency = 10,
 		.target_residency = 20,
@@ -474,6 +512,7 @@ static struct cpuidle_state hsw_cstates[] = {
 	{
 		.name = "C3-HSW",
 		.desc = "MWAIT 0x10",
+		.limit = 0x7, /* cannot limit to C3 */
 		.flags = MWAIT2flg(0x10) | CPUIDLE_FLAG_TLB_FLUSHED,
 		.exit_latency = 33,
 		.target_residency = 100,
@@ -482,6 +521,7 @@ static struct cpuidle_state hsw_cstates[] = {
 	{
 		.name = "C6-HSW",
 		.desc = "MWAIT 0x20",
+		.limit = 0x3,
 		.flags = MWAIT2flg(0x20) | CPUIDLE_FLAG_TLB_FLUSHED,
 		.exit_latency = 133,
 		.target_residency = 400,
@@ -490,6 +530,7 @@ static struct cpuidle_state hsw_cstates[] = {
 	{
 		.name = "C7s-HSW",
 		.desc = "MWAIT 0x32",
+		.limit = 0x4,
 		.flags = MWAIT2flg(0x32) | CPUIDLE_FLAG_TLB_FLUSHED,
 		.exit_latency = 166,
 		.target_residency = 500,
@@ -498,6 +539,7 @@ static struct cpuidle_state hsw_cstates[] = {
 	{
 		.name = "C8-HSW",
 		.desc = "MWAIT 0x40",
+		.limit = 0x6,
 		.flags = MWAIT2flg(0x40) | CPUIDLE_FLAG_TLB_FLUSHED,
 		.exit_latency = 300,
 		.target_residency = 900,
@@ -506,6 +548,7 @@ static struct cpuidle_state hsw_cstates[] = {
 	{
 		.name = "C9-HSW",
 		.desc = "MWAIT 0x50",
+		.limit = 0x7,
 		.flags = MWAIT2flg(0x50) | CPUIDLE_FLAG_TLB_FLUSHED,
 		.exit_latency = 600,
 		.target_residency = 1800,
@@ -514,6 +557,7 @@ static struct cpuidle_state hsw_cstates[] = {
 	{
 		.name = "C10-HSW",
 		.desc = "MWAIT 0x60",
+		.limit = 0x8,
 		.flags = MWAIT2flg(0x60) | CPUIDLE_FLAG_TLB_FLUSHED,
 		.exit_latency = 2600,
 		.target_residency = 7700,
@@ -526,6 +570,7 @@ static struct cpuidle_state bdw_cstates[] = {
 	{
 		.name = "C1-BDW",
 		.desc = "MWAIT 0x00",
+		.limit = 0x0,
 		.flags = MWAIT2flg(0x00),
 		.exit_latency = 2,
 		.target_residency = 2,
@@ -534,6 +579,7 @@ static struct cpuidle_state bdw_cstates[] = {
 	{
 		.name = "C1E-BDW",
 		.desc = "MWAIT 0x01",
+		.limit = 0x0,
 		.flags = MWAIT2flg(0x01),
 		.exit_latency = 10,
 		.target_residency = 20,
@@ -542,6 +588,7 @@ static struct cpuidle_state bdw_cstates[] = {
 	{
 		.name = "C3-BDW",
 		.desc = "MWAIT 0x10",
+		.limit = 0x2,
 		.flags = MWAIT2flg(0x10) | CPUIDLE_FLAG_TLB_FLUSHED,
 		.exit_latency = 40,
 		.target_residency = 100,
@@ -550,6 +597,7 @@ static struct cpuidle_state bdw_cstates[] = {
 	{
 		.name = "C6-BDW",
 		.desc = "MWAIT 0x20",
+		.limit = 0x3,
 		.flags = MWAIT2flg(0x20) | CPUIDLE_FLAG_TLB_FLUSHED,
 		.exit_latency = 133,
 		.target_residency = 400,
@@ -558,6 +606,7 @@ static struct cpuidle_state bdw_cstates[] = {
 	{
 		.name = "C7s-BDW",
 		.desc = "MWAIT 0x32",
+		.limit = 0x5,
 		.flags = MWAIT2flg(0x32) | CPUIDLE_FLAG_TLB_FLUSHED,
 		.exit_latency = 166,
 		.target_residency = 500,
@@ -566,6 +615,7 @@ static struct cpuidle_state bdw_cstates[] = {
 	{
 		.name = "C8-BDW",
 		.desc = "MWAIT 0x40",
+		.limit = 0x6,
 		.flags = MWAIT2flg(0x40) | CPUIDLE_FLAG_TLB_FLUSHED,
 		.exit_latency = 300,
 		.target_residency = 900,
@@ -574,6 +624,7 @@ static struct cpuidle_state bdw_cstates[] = {
 	{
 		.name = "C9-BDW",
 		.desc = "MWAIT 0x50",
+		.limit = 0x7,
 		.flags = MWAIT2flg(0x50) | CPUIDLE_FLAG_TLB_FLUSHED,
 		.exit_latency = 600,
 		.target_residency = 1800,
@@ -582,6 +633,7 @@ static struct cpuidle_state bdw_cstates[] = {
 	{
 		.name = "C10-BDW",
 		.desc = "MWAIT 0x60",
+		.limit = 0x8,
 		.flags = MWAIT2flg(0x60) | CPUIDLE_FLAG_TLB_FLUSHED,
 		.exit_latency = 2600,
 		.target_residency = 7700,
@@ -595,6 +647,7 @@ static struct cpuidle_state skl_cstates[] = {
 	{
 		.name = "C1-SKL",
 		.desc = "MWAIT 0x00",
+		.limit = 0x0,
 		.flags = MWAIT2flg(0x00),
 		.exit_latency = 2,
 		.target_residency = 2,
@@ -603,6 +656,7 @@ static struct cpuidle_state skl_cstates[] = {
 	{
 		.name = "C1E-SKL",
 		.desc = "MWAIT 0x01",
+		.limit = 0x0,
 		.flags = MWAIT2flg(0x01),
 		.exit_latency = 10,
 		.target_residency = 20,
@@ -611,6 +665,7 @@ static struct cpuidle_state skl_cstates[] = {
 	{
 		.name = "C3-SKL",
 		.desc = "MWAIT 0x10",
+		.limit = 0x2,
 		.flags = MWAIT2flg(0x10) | CPUIDLE_FLAG_TLB_FLUSHED,
 		.exit_latency = 70,
 		.target_residency = 100,
@@ -619,6 +674,7 @@ static struct cpuidle_state skl_cstates[] = {
 	{
 		.name = "C6-SKL",
 		.desc = "MWAIT 0x20",
+		.limit = 0x3,
 		.flags = MWAIT2flg(0x20) | CPUIDLE_FLAG_TLB_FLUSHED,
 		.exit_latency = 85,
 		.target_residency = 200,
@@ -627,6 +683,7 @@ static struct cpuidle_state skl_cstates[] = {
 	{
 		.name = "C7s-SKL",
 		.desc = "MWAIT 0x33",
+		.limit = 0x5,
 		.flags = MWAIT2flg(0x33) | CPUIDLE_FLAG_TLB_FLUSHED,
 		.exit_latency = 124,
 		.target_residency = 800,
@@ -635,6 +692,7 @@ static struct cpuidle_state skl_cstates[] = {
 	{
 		.name = "C8-SKL",
 		.desc = "MWAIT 0x40",
+		.limit = 0x6,
 		.flags = MWAIT2flg(0x40) | CPUIDLE_FLAG_TLB_FLUSHED,
 		.exit_latency = 200,
 		.target_residency = 800,
@@ -643,6 +701,7 @@ static struct cpuidle_state skl_cstates[] = {
 	{
 		.name = "C9-SKL",
 		.desc = "MWAIT 0x50",
+		.limit = 0x7,
 		.flags = MWAIT2flg(0x50) | CPUIDLE_FLAG_TLB_FLUSHED,
 		.exit_latency = 480,
 		.target_residency = 5000,
@@ -651,6 +710,7 @@ static struct cpuidle_state skl_cstates[] = {
 	{
 		.name = "C10-SKL",
 		.desc = "MWAIT 0x60",
+		.limit = 0x8,
 		.flags = MWAIT2flg(0x60) | CPUIDLE_FLAG_TLB_FLUSHED,
 		.exit_latency = 890,
 		.target_residency = 5000,
@@ -664,6 +724,7 @@ static struct cpuidle_state atom_cstates[] = {
 	{
 		.name = "C1E-ATM",
 		.desc = "MWAIT 0x00",
+		.limit = 0x1,
 		.flags = MWAIT2flg(0x00),
 		.exit_latency = 10,
 		.target_residency = 20,
@@ -672,6 +733,7 @@ static struct cpuidle_state atom_cstates[] = {
 	{
 		.name = "C2-ATM",
 		.desc = "MWAIT 0x10",
+		.limit = 0x7, /* undefined */
 		.flags = MWAIT2flg(0x10),
 		.exit_latency = 20,
 		.target_residency = 80,
@@ -680,6 +742,7 @@ static struct cpuidle_state atom_cstates[] = {
 	{
 		.name = "C4-ATM",
 		.desc = "MWAIT 0x30",
+		.limit = 0x4,
 		.flags = MWAIT2flg(0x30) | CPUIDLE_FLAG_TLB_FLUSHED,
 		.exit_latency = 100,
 		.target_residency = 400,
@@ -688,6 +751,7 @@ static struct cpuidle_state atom_cstates[] = {
 	{
 		.name = "C6-ATM",
 		.desc = "MWAIT 0x52",
+		.limit = 0x5,
 		.flags = MWAIT2flg(0x52) | CPUIDLE_FLAG_TLB_FLUSHED,
 		.exit_latency = 140,
 		.target_residency = 560,
@@ -700,6 +764,7 @@ static struct cpuidle_state avn_cstates[] = {
 	{
 		.name = "C1-AVN",
 		.desc = "MWAIT 0x00",
+		.limit = 0x1,
 		.flags = MWAIT2flg(0x00),
 		.exit_latency = 2,
 		.target_residency = 2,
@@ -708,6 +773,7 @@ static struct cpuidle_state avn_cstates[] = {
 	{
 		.name = "C6-AVN",
 		.desc = "MWAIT 0x51",
+		.limit = 0x5,
 		.flags = MWAIT2flg(0x51) | CPUIDLE_FLAG_TLB_FLUSHED,
 		.exit_latency = 15,
 		.target_residency = 45,
@@ -1034,11 +1100,22 @@ static int __init intel_idle_cpuidle_driver_init(void)
 {
 	int cstate;
 	struct cpuidle_driver *drv = &intel_idle_driver;
+	u64 msr;
+	unsigned int limit;
 
 	intel_idle_state_table_update();
 
 	drv->state_count = 1;
 
+	rdmsrl_on_cpu(0, MSR_PKG_CST_CONFIG_CONTROL, &msr);
+	/*
+	 * Older processors use bits 2:0 and have bits 9:3 reserved.
+	 * Newer processors use bits 3:0 and have bits 9:4 reserved.
+	 * It looks safe to use bits 3:0 everywhere as a mask, as
+	 * bit 3 on older processors is always zero.
+	 */
+	limit = msr & 0xF;
+
 	for (cstate = 0; cstate < CPUIDLE_STATE_MAX; ++cstate) {
 		int num_substates, mwait_hint, mwait_cstate;
 
@@ -1063,6 +1140,16 @@ static int __init intel_idle_cpuidle_driver_init(void)
 		if (num_substates == 0)
 			continue;
 
+		/* Has this state been disabled in hardware? */
+		if (limit < cpuidle_state_table[cstate].limit) {
+			cpuidle_state_table[cstate].disabled = 1;
+			pr_debug(PREFIX "state %s (0x%x) is disabled.  Max Package limit is 0x%x.\n",
+				 cpuidle_state_table[cstate].name,
+				 cpuidle_state_table[cstate].limit,
+				 limit);
+
+		}
+
 		if (((mwait_cstate + 1) > 2) &&
 			!boot_cpu_has(X86_FEATURE_NONSTOP_TSC))
 			mark_tsc_unstable("TSC halts in idle"
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index 786ad32..8bcfabb 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -39,6 +39,7 @@ struct cpuidle_state {
 	char		name[CPUIDLE_NAME_LEN];
 	char		desc[CPUIDLE_DESC_LEN];
 
+	unsigned int	limit; /* compared to Package C-state Limit */
 	unsigned int	flags;
 	unsigned int	exit_latency; /* in US */
 	int		power_usage; /* in mW */
-- 
1.7.9.3

Re: [PATCH 0/3] idle, Honor Hardware Disabled States

[Len Brown]

This patch assumes that if a package state is disabled,
the corresponding core state must be disabled.
That assumption is false.
Indeed, that is a very popular and useful configuration.

But even if that were not the case, this software is not necessary,
since the hardware handles demotion "c-state clipping" automatically.

Yes, there is a case where a certain version of a certain processor
has broken demotion, but this isn't the right fix for that.
The right fix for that is here:

https://bugzilla.kernel.org/show_bug.cgi?id=109081

and will be upstream shortly.

thanks,
-Len



On Mon, Mar 21, 2016 at 8:49 AM, Prarit Bhargava <prarit@redhat.com> wrote:
> Some Intel processors have cstates limited by MSR_PKG_CST_CONFIG_CONTROL
> (0xE2), which specifies the lowest processor-specific C-state code name
> (consuming the least power).  The intel_idle driver does not query this MSR to
> see if states are limited by hardware.  In the case of the i7-6700HQ the value
> of bits 3:0 on the MSR are 0x6, meaning that C9-SKL (0x7) and C10-SKL (0x8)
> should have been disabled.
>
> This patch adds a limit field to the cpuidle_state struct to hold the
> cstate encodings for each processor, and queries the values the values
> to confirm support.
>
> After applying this patchset the system now boots and reports that
> some states have been disabled:
>
> [root@dhcp40-125 ~]# dmesg | grep intel_idle
> [    1.163367] intel_idle: MWAIT substates: 0x11142120
> [    1.163368] intel_idle: v0.4 model 0x5E
> [    1.163369] intel_idle: lapic_timer_reliable_states 0xffffffff
> [    1.163370] intel_idle: state C9-SKL (0x7) is disabled.  Max Package limit is 0x6.
> [    1.163371] intel_idle: state C10-SKL (0x8) is disabled.  Max Package limit is 0x6.
>
> Cc: x86@kernel.org
> Cc: Len Brown <lenb@kernel.org>
> Cc: "Peter Zijlstra (Intel)" <peterz@infradead.org>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Andi Kleen <ak@linux.intel.com>
> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Dasaratharaman Chandramouli <dasaratharaman.chandramouli@intel.com>
> Cc: linux-pm@vger.kernel.org
> Signed-off-by: Prarit Bhargava <prarit@redhat.com>
>
> Prarit Bhargava (3):
>   idle, rename MSR_NHM_SNB_PKG_CST_CFG_CTL to
>     MSR_PKG_CST_CONFIG_CONTROL
>   intel_idle, Introduce cstate limits and fix hardware cstate disable
>     value
>   cpuidle, Prevent users from enabling cstates that are disabled in
>     Hardware
>
>  arch/x86/include/asm/msr-index.h      |    2 +-
>  drivers/cpuidle/governors/ladder.c    |    3 ++
>  drivers/cpuidle/governors/menu.c      |    4 ++
>  drivers/cpuidle/sysfs.c               |   34 +++++++++++-
>  drivers/idle/intel_idle.c             |   93 ++++++++++++++++++++++++++++++++-
>  include/linux/cpuidle.h               |   11 +++-
>  tools/power/x86/turbostat/turbostat.8 |    2 +-
>  tools/power/x86/turbostat/turbostat.c |   10 ++--
>  8 files changed, 146 insertions(+), 13 deletions(-)
>
> --
> 1.7.9.3
>



-- 
Len Brown, Intel Open Source Technology Center

Re: [PATCH 0/3] idle, Honor Hardware Disabled States

[Prarit Bhargava]

On 03/23/2016 04:05 PM, Len Brown wrote:
> This patch assumes that if a package state is disabled,
> the corresponding core state must be disabled.
> That assumption is false.
> Indeed, that is a very popular and useful configuration.
> 
> But even if that were not the case, this software is not necessary,
> since the hardware handles demotion "c-state clipping" automatically.
> 
> Yes, there is a case where a certain version of a certain processor
> has broken demotion, but this isn't the right fix for that.
> The right fix for that is here:
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=109081

Len, should I rebase on top of this?  Would that work for you?

P.

> 
> and will be upstream shortly.
> 
> thanks,
> -Len
> 
> 
> 
> On Mon, Mar 21, 2016 at 8:49 AM, Prarit Bhargava <prarit@redhat.com> wrote:
>> Some Intel processors have cstates limited by MSR_PKG_CST_CONFIG_CONTROL
>> (0xE2), which specifies the lowest processor-specific C-state code name
>> (consuming the least power).  The intel_idle driver does not query this MSR to
>> see if states are limited by hardware.  In the case of the i7-6700HQ the value
>> of bits 3:0 on the MSR are 0x6, meaning that C9-SKL (0x7) and C10-SKL (0x8)
>> should have been disabled.
>>
>> This patch adds a limit field to the cpuidle_state struct to hold the
>> cstate encodings for each processor, and queries the values the values
>> to confirm support.
>>
>> After applying this patchset the system now boots and reports that
>> some states have been disabled:
>>
>> [root@dhcp40-125 ~]# dmesg | grep intel_idle
>> [    1.163367] intel_idle: MWAIT substates: 0x11142120
>> [    1.163368] intel_idle: v0.4 model 0x5E
>> [    1.163369] intel_idle: lapic_timer_reliable_states 0xffffffff
>> [    1.163370] intel_idle: state C9-SKL (0x7) is disabled.  Max Package limit is 0x6.
>> [    1.163371] intel_idle: state C10-SKL (0x8) is disabled.  Max Package limit is 0x6.
>>
>> Cc: x86@kernel.org
>> Cc: Len Brown <lenb@kernel.org>
>> Cc: "Peter Zijlstra (Intel)" <peterz@infradead.org>
>> Cc: Borislav Petkov <bp@suse.de>
>> Cc: Andi Kleen <ak@linux.intel.com>
>> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
>> Cc: Dasaratharaman Chandramouli <dasaratharaman.chandramouli@intel.com>
>> Cc: linux-pm@vger.kernel.org
>> Signed-off-by: Prarit Bhargava <prarit@redhat.com>
>>
>> Prarit Bhargava (3):
>>   idle, rename MSR_NHM_SNB_PKG_CST_CFG_CTL to
>>     MSR_PKG_CST_CONFIG_CONTROL
>>   intel_idle, Introduce cstate limits and fix hardware cstate disable
>>     value
>>   cpuidle, Prevent users from enabling cstates that are disabled in
>>     Hardware
>>
>>  arch/x86/include/asm/msr-index.h      |    2 +-
>>  drivers/cpuidle/governors/ladder.c    |    3 ++
>>  drivers/cpuidle/governors/menu.c      |    4 ++
>>  drivers/cpuidle/sysfs.c               |   34 +++++++++++-
>>  drivers/idle/intel_idle.c             |   93 ++++++++++++++++++++++++++++++++-
>>  include/linux/cpuidle.h               |   11 +++-
>>  tools/power/x86/turbostat/turbostat.8 |    2 +-
>>  tools/power/x86/turbostat/turbostat.c |   10 ++--
>>  8 files changed, 146 insertions(+), 13 deletions(-)
>>
>> --
>> 1.7.9.3
>>
> 
> 
> 

Re: [PATCH 0/3] idle, Honor Hardware Disabled States

[Len Brown]

On Wed, Mar 23, 2016 at 7:50 PM, Prarit Bhargava <prarit@redhat.com> wrote:
>
>
> On 03/23/2016 04:05 PM, Len Brown wrote:
>> This patch assumes that if a package state is disabled,
>> the corresponding core state must be disabled.
>> That assumption is false.
>> Indeed, that is a very popular and useful configuration.
>>
>> But even if that were not the case, this software is not necessary,
>> since the hardware handles demotion "c-state clipping" automatically.
>>
>> Yes, there is a case where a certain version of a certain processor
>> has broken demotion, but this isn't the right fix for that.
>> The right fix for that is here:
>>
>> https://bugzilla.kernel.org/show_bug.cgi?id=109081
>
> Len, should I rebase on top of this?  Would that work for you?


I guess I wasn't clear.
I don't see the benefit of your patch.
Please explain it to me.

thanks,
-Len

Re: [PATCH 0/3] idle, Honor Hardware Disabled States

[Prarit Bhargava]

On 03/24/2016 05:54 PM, Len Brown wrote:
> On Wed, Mar 23, 2016 at 7:50 PM, Prarit Bhargava <prarit@redhat.com> wrote:
>>
>>
>> On 03/23/2016 04:05 PM, Len Brown wrote:
>>> This patch assumes that if a package state is disabled,
>>> the corresponding core state must be disabled.
>>> That assumption is false.
>>> Indeed, that is a very popular and useful configuration.
>>>
>>> But even if that were not the case, this software is not necessary,
>>> since the hardware handles demotion "c-state clipping" automatically.
>>>
>>> Yes, there is a case where a certain version of a certain processor
>>> has broken demotion, but this isn't the right fix for that.
>>> The right fix for that is here:
>>>
>>> https://bugzilla.kernel.org/show_bug.cgi?id=109081
>>
>> Len, should I rebase on top of this?  Would that work for you?
> 
> 
> I guess I wasn't clear.
> I don't see the benefit of your patch.
> Please explain it to me.
> 

Len,

Your patch does

+	skl_cstates[5].disabled = 1;	/* C8-SKL */
+	skl_cstates[6].disabled = 1;	/* C9-SKL */

and I don't think that is correct for SKY-H.

Your patch does not take into account that the states are explicitly disabled
in MSR_NHM_SNB_PKG_CST_CFG_CTL.  That is the problem here and what you've done
is simply hammered a disable into those states.

Additionally, your patch does not show the user the correct state information:

    [root@dhcp40-125 ~]# egrep ^ /sys/devices/system/cpu/cpu0/cpuidle/state?/disable
    /sys/devices/system/cpu/cpu0/cpuidle/state0/disable:1:0
    /sys/devices/system/cpu/cpu0/cpuidle/state1/disable:1:0
    /sys/devices/system/cpu/cpu0/cpuidle/state2/disable:1:0
    /sys/devices/system/cpu/cpu0/cpuidle/state3/disable:1:0
    /sys/devices/system/cpu/cpu0/cpuidle/state4/disable:1:0
    /sys/devices/system/cpu/cpu0/cpuidle/state5/disable:1:0
    /sys/devices/system/cpu/cpu0/cpuidle/state6/disable:1:0
    /sys/devices/system/cpu/cpu0/cpuidle/state7/disable:1:0 << should be 1
    /sys/devices/system/cpu/cpu0/cpuidle/state8/disable:1:0 << should be 1

The fix is to honour the settings in MSR_NHM_SNB_PKG_CST_CFG_CTL.  I cannot say
for certain that ALL SKY-H are impacted (you are admittedly in better position
to say so or not).  I can say that on the 2 systems tested here the
MSR_NHM_SNB_PKG_CST_CFG_CTL do have the appropriate disable value set.

/me could be missing some important info  -- again, perhaps there are some
SKY-H's out there that do not have states disabled in
MSR_NHM_SNB_PKG_CST_CFG_CTL, and that's why I've proposed rebasing on top of
your change.

P.


> thanks,
> -Len
> 

Re: [PATCH 0/3] idle, Honor Hardware Disabled States

[Len Brown]

> Len,
>
> Your patch does
>
> +       skl_cstates[5].disabled = 1;    /* C8-SKL */
> +       skl_cstates[6].disabled = 1;    /* C9-SKL */
>
> and I don't think that is correct for SKY-H.

For https://bugzilla.kernel.org/show_bug.cgi?id=109081
it is correct.

> Your patch does not take into account that the states are explicitly disabled
> in MSR_NHM_SNB_PKG_CST_CFG_CTL.  That is the problem here and what you've done
> is simply hammered a disable into those states.

ENOPARSE.
Are we talking about the failure in
https://bugzilla.kernel.org/show_bug.cgi?id=109081
or a different problem?

>
> Additionally, your patch does not show the user the correct state information:
>
>     [root@dhcp40-125 ~]# egrep ^ /sys/devices/system/cpu/cpu0/cpuidle/state?/disable
>     /sys/devices/system/cpu/cpu0/cpuidle/state0/disable:1:0
>     /sys/devices/system/cpu/cpu0/cpuidle/state1/disable:1:0
>     /sys/devices/system/cpu/cpu0/cpuidle/state2/disable:1:0
>     /sys/devices/system/cpu/cpu0/cpuidle/state3/disable:1:0
>     /sys/devices/system/cpu/cpu0/cpuidle/state4/disable:1:0
>     /sys/devices/system/cpu/cpu0/cpuidle/state5/disable:1:0
>     /sys/devices/system/cpu/cpu0/cpuidle/state6/disable:1:0
>     /sys/devices/system/cpu/cpu0/cpuidle/state7/disable:1:0 << should be 1
>     /sys/devices/system/cpu/cpu0/cpuidle/state8/disable:1:0 << should be 1

the 'disabled' attribute you see in sysfs is not
struct cpuidle_state.disabled
it is
struct cpuidle_state_usage.disabled

> The fix is to honour the settings in MSR_NHM_SNB_PKG_CST_CFG_CTL.  I cannot say
> for certain that ALL SKY-H are impacted (you are admittedly in better position
> to say so or not).  I can say that on the 2 systems tested here the
> MSR_NHM_SNB_PKG_CST_CFG_CTL do have the appropriate disable value set.
>
> /me could be missing some important info  -- again, perhaps there are some
> SKY-H's out there that do not have states disabled in
> MSR_NHM_SNB_PKG_CST_CFG_CTL, and that's why I've proposed rebasing on top of
> your change.

Do you see this debug message when you run current upstream on this hardware?

                /* if state marked as disabled, skip it */
                if (cpuidle_state_table[cstate].disabled != 0) {
                        pr_debug(PREFIX "state %s is disabled",
                                cpuidle_state_table[cstate].name);
                        continue;
                }


If no, then my patch is not disabling C8/C9 on your system.

Also, if it were, the code above causes the states to not appear
at all in sysfs, because they are not registered.

Re: MSR_NHM_SNB_PKG_CST_CFG_CTL

if PC10 is disabled there, then functionally, it doesn't matter what we do,
which is why my patch does nothing when PC10 is disabled.

In such a scenario, pc10 presence in sysfs (and cpufreq)
is cosmetic.  The hardware knows what to do.

Do you think that cosmetic issue is worth dealing with?
Note that the decoding of that MSR changes with every CPU,
so to get it right (like turbostat does), we'd need a table.
Also, it would be useful only for states which are  PC states only.
ie. we can't disable CC7 just because PC7 is disabled. etc.
So you could remove PC8, PC9, PC10 from sysfs on SKL
when they are disabled, but that is all.

thanks,
Len Brown, Intel Open Source Technology Center

Re: [PATCH 0/3] idle, Honor Hardware Disabled States

[Prarit Bhargava]

On 03/31/2016 12:59 AM, Len Brown wrote:
>> Len,
>>
>> Your patch does
>>
>> +       skl_cstates[5].disabled = 1;    /* C8-SKL */
>> +       skl_cstates[6].disabled = 1;    /* C9-SKL */
>>
>> and I don't think that is correct for SKY-H.
>
> For https://bugzilla.kernel.org/show_bug.cgi?id=109081
> it is correct.
>
>> Your patch does not take into account that the states are explicitly disabled
>> in MSR_NHM_SNB_PKG_CST_CFG_CTL.  That is the problem here and what you've done
>> is simply hammered a disable into those states.
>
> ENOPARSE.
> Are we talking about the failure in
> https://bugzilla.kernel.org/show_bug.cgi?id=109081
> or a different problem?
>

I see now.  There are two bugs.  The first is your case where the deep c-states
should be disabled but are not disabled, and the my case where the states *are*
disabled but not indicated as such in sysfs.

>>
>> Additionally, your patch does not show the user the correct state information:
>>
>>     [root@dhcp40-125 ~]# egrep ^
/sys/devices/system/cpu/cpu0/cpuidle/state?/disable
>>     /sys/devices/system/cpu/cpu0/cpuidle/state0/disable:1:0
>>     /sys/devices/system/cpu/cpu0/cpuidle/state1/disable:1:0
>>     /sys/devices/system/cpu/cpu0/cpuidle/state2/disable:1:0
>>     /sys/devices/system/cpu/cpu0/cpuidle/state3/disable:1:0
>>     /sys/devices/system/cpu/cpu0/cpuidle/state4/disable:1:0
>>     /sys/devices/system/cpu/cpu0/cpuidle/state5/disable:1:0
>>     /sys/devices/system/cpu/cpu0/cpuidle/state6/disable:1:0
>>     /sys/devices/system/cpu/cpu0/cpuidle/state7/disable:1:0 << should be 1
>>     /sys/devices/system/cpu/cpu0/cpuidle/state8/disable:1:0 << should be 1
>
> the 'disabled' attribute you see in sysfs is not
> struct cpuidle_state.disabled
> it is
> struct cpuidle_state_usage.disabled

Yes.  I know that.  But the problem is that your patch should take that
into account.

>
>> The fix is to honour the settings in MSR_NHM_SNB_PKG_CST_CFG_CTL.  I cannot say
>> for certain that ALL SKY-H are impacted (you are admittedly in better position
>> to say so or not).  I can say that on the 2 systems tested here the
>> MSR_NHM_SNB_PKG_CST_CFG_CTL do have the appropriate disable value set.
>>
>> /me could be missing some important info  -- again, perhaps there are some
>> SKY-H's out there that do not have states disabled in
>> MSR_NHM_SNB_PKG_CST_CFG_CTL, and that's why I've proposed rebasing on top of
>> your change.
>
> Do you see this debug message when you run current upstream on this hardware?
>
>                 /* if state marked as disabled, skip it */
>                 if (cpuidle_state_table[cstate].disabled != 0) {
>                         pr_debug(PREFIX "state %s is disabled",
>                                 cpuidle_state_table[cstate].name);
>                         continue;
>                 }
>
>
> If no, then my patch is not disabling C8/C9 on your system.
>
> Also, if it were, the code above causes the states to not appear
> at all in sysfs, because they are not registered.
>
> Re: MSR_NHM_SNB_PKG_CST_CFG_CTL
>
> if PC10 is disabled there, then functionally, it doesn't matter what we do,
> which is why my patch does nothing when PC10 is disabled.
>
> In such a scenario, pc10 presence in sysfs (and cpufreq)
> is cosmetic.  The hardware knows what to do.
>
> Do you think that cosmetic issue is worth dealing with?

The bug reported is that the system will not go into the deep
c-states.  This is because the c-states are disabled by hardware.  This
means that turbostat shows only transitions to C7, while sysfs (and the
monitoring software) indicates deeper c-states are available.

So yes, I do think that it is worth dealing with.

> Note that the decoding of that MSR changes with every CPU,

:( Ugh ... I know :(.  I just decoded it.

> so to get it right (like turbostat does), we'd need a table.
> Also, it would be useful only for states which are  PC states only.
> ie. we can't disable CC7 just because PC7 is disabled. etc.
> So you could remove PC8, PC9, PC10 from sysfs on SKL
> when they are disabled, but that is all.
>

P.

> thanks,
> Len Brown, Intel Open Source Technology Center
>

Re: [PATCH 0/3] idle, Honor Hardware Disabled States

[Len Brown]

On Mon, Apr 11, 2016 at 7:37 AM, Prarit Bhargava <prarit@redhat.com> wrote:
>
>
> On 03/31/2016 12:59 AM, Len Brown wrote:
>>> Len,
>>>
>>> Your patch does
>>>
>>> +       skl_cstates[5].disabled = 1;    /* C8-SKL */
>>> +       skl_cstates[6].disabled = 1;    /* C9-SKL */
>>>
>>> and I don't think that is correct for SKY-H.
>>
>> For https://bugzilla.kernel.org/show_bug.cgi?id=109081
>> it is correct.
>>
>>> Your patch does not take into account that the states are explicitly disabled
>>> in MSR_NHM_SNB_PKG_CST_CFG_CTL.  That is the problem here and what you've done
>>> is simply hammered a disable into those states.
>>
>> ENOPARSE.
>> Are we talking about the failure in
>> https://bugzilla.kernel.org/show_bug.cgi?id=109081
>> or a different problem?
>>
>
> I see now.  There are two bugs.  The first is your case where the deep c-states
> should be disabled but are not disabled, and the my case where the states *are*
> disabled but not indicated as such in sysfs.
>
>>>
>>> Additionally, your patch does not show the user the correct state information:
>>>
>>>     [root@dhcp40-125 ~]# egrep ^
> /sys/devices/system/cpu/cpu0/cpuidle/state?/disable
>>>     /sys/devices/system/cpu/cpu0/cpuidle/state0/disable:1:0
>>>     /sys/devices/system/cpu/cpu0/cpuidle/state1/disable:1:0
>>>     /sys/devices/system/cpu/cpu0/cpuidle/state2/disable:1:0
>>>     /sys/devices/system/cpu/cpu0/cpuidle/state3/disable:1:0
>>>     /sys/devices/system/cpu/cpu0/cpuidle/state4/disable:1:0
>>>     /sys/devices/system/cpu/cpu0/cpuidle/state5/disable:1:0
>>>     /sys/devices/system/cpu/cpu0/cpuidle/state6/disable:1:0
>>>     /sys/devices/system/cpu/cpu0/cpuidle/state7/disable:1:0 << should be 1
>>>     /sys/devices/system/cpu/cpu0/cpuidle/state8/disable:1:0 << should be 1
>>
>> the 'disabled' attribute you see in sysfs is not
>> struct cpuidle_state.disabled
>> it is
>> struct cpuidle_state_usage.disabled
>
> Yes.  I know that.  But the problem is that your patch should take that
> into account.

When the patch to disable C8/C9 takes effect,
cpuidle_state.disabled is at intel_idle initizliation time.
That causes it to skip registering C8/C9 with cpuidle.
So  those states  do not appear in sysfs
and so cpuidle_state_usage.disabled does not exist for them.

In the example above, the patch to disable C8/C9 was not invoked,
or else C8 would not be present in sysfs at all.

>>> The fix is to honour the settings in MSR_NHM_SNB_PKG_CST_CFG_CTL.  I cannot say
>>> for certain that ALL SKY-H are impacted (you are admittedly in better position
>>> to say so or not).  I can say that on the 2 systems tested here the
>>> MSR_NHM_SNB_PKG_CST_CFG_CTL do have the appropriate disable value set.
>>>
>>> /me could be missing some important info  -- again, perhaps there are some
>>> SKY-H's out there that do not have states disabled in
>>> MSR_NHM_SNB_PKG_CST_CFG_CTL, and that's why I've proposed rebasing on top of
>>> your change.
>>
>> Do you see this debug message when you run current upstream on this hardware?
>>
>>                 /* if state marked as disabled, skip it */
>>                 if (cpuidle_state_table[cstate].disabled != 0) {
>>                         pr_debug(PREFIX "state %s is disabled",
>>                                 cpuidle_state_table[cstate].name);
>>                         continue;
>>                 }
>>
>>
>> If no, then my patch is not disabling C8/C9 on your system.
>>
>> Also, if it were, the code above causes the states to not appear
>> at all in sysfs, because they are not registered.
>>
>> Re: MSR_NHM_SNB_PKG_CST_CFG_CTL
>>
>> if PC10 is disabled there, then functionally, it doesn't matter what we do,
>> which is why my patch does nothing when PC10 is disabled.
>>
>> In such a scenario, pc10 presence in sysfs (and cpufreq)
>> is cosmetic.  The hardware knows what to do.
>>
>> Do you think that cosmetic issue is worth dealing with?
>
> The bug reported is that the system will not go into the deep
> c-states.  This is because the c-states are disabled by hardware.  This
> means that turbostat shows only transitions to C7, while sysfs (and the
> monitoring software) indicates deeper c-states are available.
> So yes, I do think that it is worth dealing with.

Please reply with the URL of the bug you are referring to.

Note that somebody who looks in just sysfs will be dumb and happy - as
it will show
what states were requested, and everything will be requested, eventually...

Somebody who looks at turbostat output will sometimes see columns that are 0%,
as it is showing the underlying HW residency counters.  There is also
a line at the top that decodes pkg-cstate-limit, which may or may not
explain why those columns are 0.

So the most useful way to pretty this up would be for turbostat to assume that
users don't understand the pkg-cstate-limit decoding, and to hide
disabled columns.
Sure, I can do that.

Yes, we could delete the redundant states from cpuidle, but on current HW,
we can only do it for package states that have no corresponding core state.
eg. PC8, PC9, PC10.  This will have no effect on the operation of the hardware,
but I agree it would look more tidy to a user who is under the impression
that the counters in sysfs reflect closely what the hardware is doing,
rather than the permission that Linux is giving the HW to do.

But above is all cosmetic.  The real "bug" that users are running into is
that they can't get into deep c-states when they are enabled.
Linux (and Intel) need to do a much better job enabling diagnosis of
that condition.

Len Brown, Intel Open Source Technology Center

Re: [PATCH 0/3] idle, Honor Hardware Disabled States

[Prarit Bhargava]

On 04/29/2016 05:36 AM, Len Brown wrote:

> But above is all cosmetic.  The real "bug" that users are running into is
> that they can't get into deep c-states when they are enabled.
> Linux (and Intel) need to do a much better job enabling diagnosis of
> that condition.
> 
Okay, I geddit.

Acked-by: Prarit Bhargava <prarit@redhat.com>

P.