[RFC PATCH 0/1] x86: msr: add new 'msr_pkg_cst_config_control.h' header

[RFC PATCH 0/1] x86: msr: add new 'msr_pkg_cst_config_control.h' header

[Artem Bityutskiy]

From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>

In this patch I am trying to implement the suggestion from Rafael Wysocki
that can be found here:
https://patches.linaro.org/project/linux-pm/patch/20250212084232.2349984-1-dedekind1@gmail.com/

The suggestion is basically to have common wrappers for accessing
MSR_PKG_CST_CONFIG_CONTROL in order to make it clear there are multiple
users of the MSR.

I was not sure how to implement this, and here is what came to mind. Just
an RFC to demonstate the approach, please comment/criticize.

Just to linux-pm at this point, also avoid splitting on multiple patches for now.

This patch applies on top of the following patch-set that I sent to linux-pm
a bit earlier today:
"[PATCH v2 0/3] intel_idle: Add C1 demotion on/off sysfs knob"
https://lore.kernel.org/linux-pm/20250220151702.2153579-1-dedekind1@gmail.com/T/#t


Artem Bityutskiy (1):
  x86: msr: add new 'msr_pkg_cst_config_control.h' header

 .../include/asm/msr_pkg_cst_config_control.h  | 28 +++++++++++++++++++
 drivers/idle/intel_idle.c                     | 17 +++++------
 drivers/platform/x86/intel/pmc/cnp.c          | 13 +++++----
 3 files changed, 44 insertions(+), 14 deletions(-)
 create mode 100644 arch/x86/include/asm/msr_pkg_cst_config_control.h

-- 
2.47.1

[RFC PATCH 1/1] x86: msr: add new 'msr_pkg_cst_config_control.h' header

[Artem Bityutskiy]

From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>

There are now two users that modify the MSR_PKG_CST_CONFIG_CONTROL register:
1. The Intel PMC framework (cnp.c).
2. The intel_idle.c driver.

They do not interfere with each other because the former modifies it only
during suspend/resume.

Introduce common accessor functions for the MSR to make it more clear that
there is already more than one user. There is no other purpose at this point.
But if more users are introduced, the header file may be replaced with a small
MSR_PKG_CST_CONFIG_CONTROL subsystem, possibly implementing locking.

Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
---
 .../include/asm/msr_pkg_cst_config_control.h  | 28 +++++++++++++++++++
 drivers/idle/intel_idle.c                     | 17 +++++------
 drivers/platform/x86/intel/pmc/cnp.c          | 13 +++++----
 3 files changed, 44 insertions(+), 14 deletions(-)
 create mode 100644 arch/x86/include/asm/msr_pkg_cst_config_control.h

diff --git a/arch/x86/include/asm/msr_pkg_cst_config_control.h b/arch/x86/include/asm/msr_pkg_cst_config_control.h
new file mode 100644
index 000000000000..0d9dab4c20ef
--- /dev/null
+++ b/arch/x86/include/asm/msr_pkg_cst_config_control.h
@@ -0,0 +1,28 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Accessors from MSR_PKG_CST_CONFIG_CONTROL (0xe2) MSR found on Intel CPUs.
+ *
+ * At this point provide only trival read/write functions. But this header file
+ * can be turned into a small library if there are more MSR users in the future.
+ */
+
+#ifndef _MSR_PKG_CST_CONFIG_CONTROL_H
+#define _MSR_PKG_CST_CONFIG_CONTROL_H
+
+#include <asm/msr-index.h>
+#include <asm/msr.h>
+
+static inline unsigned long long rdmsrl_pkg_cst_config_control(void)
+{
+	unsigned long long val;
+
+	rdmsrl(MSR_PKG_CST_CONFIG_CONTROL, val);
+	return val;
+}
+
+static inline void wrmsrl_pkg_cst_config_control(unsigned long long val)
+{
+	wrmsrl(MSR_PKG_CST_CONFIG_CONTROL, val);
+}
+
+#endif /* _MSR_PKG_CST_CONFIG_CONTROL_H */
diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
index 8d2095078469..e5415e20e0e3 100644
--- a/drivers/idle/intel_idle.c
+++ b/drivers/idle/intel_idle.c
@@ -59,6 +59,7 @@
 #include <asm/mwait.h>
 #include <asm/spec-ctrl.h>
 #include <asm/fpu/api.h>
+#include <asm/msr_pkg_cst_config_control.h>
 
 #define INTEL_IDLE_VERSION "0.5.1"
 
@@ -1975,7 +1976,7 @@ static void __init sklh_idle_state_table_update(void)
 	if ((mwait_substates & (0xF << 28)) == 0)
 		return;
 
-	rdmsrl(MSR_PKG_CST_CONFIG_CONTROL, msr);
+	msr = rdmsrl_pkg_cst_config_control();
 
 	/* PC10 is not enabled in PKG C-state limit */
 	if ((msr & 0xF) != 8)
@@ -2006,7 +2007,7 @@ static void __init skx_idle_state_table_update(void)
 {
 	unsigned long long msr;
 
-	rdmsrl(MSR_PKG_CST_CONFIG_CONTROL, msr);
+	msr = rdmsrl_pkg_cst_config_control();
 
 	/*
 	 * 000b: C0/C1 (no package C-state support)
@@ -2059,7 +2060,7 @@ static void __init spr_idle_state_table_update(void)
 	 * C6. However, if PC6 is disabled, we update the numbers to match
 	 * core C6.
 	 */
-	rdmsrl(MSR_PKG_CST_CONFIG_CONTROL, msr);
+	msr = rdmsrl_pkg_cst_config_control();
 
 	/* Limit value 2 and above allow for PC6. */
 	if ((msr & 0x7) < 2) {
@@ -2221,9 +2222,9 @@ static void auto_demotion_disable(void)
 {
 	unsigned long long msr_bits;
 
-	rdmsrl(MSR_PKG_CST_CONFIG_CONTROL, msr_bits);
+	msr_bits = rdmsrl_pkg_cst_config_control();
 	msr_bits &= ~auto_demotion_disable_flags;
-	wrmsrl(MSR_PKG_CST_CONFIG_CONTROL, msr_bits);
+	wrmsrl_pkg_cst_config_control(msr_bits);
 }
 
 static void c1e_promotion_enable(void)
@@ -2309,7 +2310,7 @@ static void intel_c1_demotion_toggle(void *info)
 	unsigned long long msr_val;
 	bool enable = *(bool *)info;
 
-	rdmsrl(MSR_PKG_CST_CONFIG_CONTROL, msr_val);
+	msr_val = rdmsrl_pkg_cst_config_control();
 	/*
 	 * Enable/disable C1 undemotion along with C1 demotion, as this is the
 	 * most sensible configuration in general.
@@ -2318,7 +2319,7 @@ static void intel_c1_demotion_toggle(void *info)
 		msr_val |= NHM_C1_AUTO_DEMOTE | SNB_C1_AUTO_UNDEMOTE;
 	else
 		msr_val &= ~(NHM_C1_AUTO_DEMOTE | SNB_C1_AUTO_UNDEMOTE);
-	wrmsrl(MSR_PKG_CST_CONFIG_CONTROL, msr_val);
+	wrmsrl_pkg_cst_config_control(msr_val);
 }
 
 static ssize_t intel_c1_demotion_store(struct device *dev,
@@ -2349,7 +2350,7 @@ static ssize_t intel_c1_demotion_show(struct device *dev,
 	 * Read the MSR value for a CPU and assume it is the same for all CPUs. Any other
 	 * configureation would be a BIOS bug.
 	 */
-	rdmsrl(MSR_PKG_CST_CONFIG_CONTROL, msr_val);
+	msr_val = rdmsrl_pkg_cst_config_control();
 	return sysfs_emit(buf, "%d\n", !!(msr_val & NHM_C1_AUTO_DEMOTE));
 }
 static DEVICE_ATTR_RW(intel_c1_demotion);
diff --git a/drivers/platform/x86/intel/pmc/cnp.c b/drivers/platform/x86/intel/pmc/cnp.c
index fc5193fdf8a8..4ef8dfe07664 100644
--- a/drivers/platform/x86/intel/pmc/cnp.c
+++ b/drivers/platform/x86/intel/pmc/cnp.c
@@ -10,6 +10,7 @@
 
 #include <linux/smp.h>
 #include <linux/suspend.h>
+#include <asm/msr_pkg_cst_config_control.h>
 #include "core.h"
 
 /* Cannon Lake: PGD PFET Enable Ack Status Register(s) bitmap */
@@ -225,12 +226,12 @@ static DEFINE_PER_CPU(u64, pkg_cst_config);
 static void disable_c1_auto_demote(void *unused)
 {
 	int cpunum = smp_processor_id();
-	u64 val;
+	unsigned long long val;
 
-	rdmsrl(MSR_PKG_CST_CONFIG_CONTROL, val);
+	val = rdmsrl_pkg_cst_config_control();
 	per_cpu(pkg_cst_config, cpunum) = val;
 	val &= ~NHM_C1_AUTO_DEMOTE;
-	wrmsrl(MSR_PKG_CST_CONFIG_CONTROL, val);
+	wrmsrl_pkg_cst_config_control(val);
 
 	pr_debug("%s: cpu:%d cst %llx\n", __func__, cpunum, val);
 }
@@ -238,11 +239,11 @@ static void disable_c1_auto_demote(void *unused)
 static void restore_c1_auto_demote(void *unused)
 {
 	int cpunum = smp_processor_id();
+	unsigned long long val = per_cpu(pkg_cst_config, cpunum);
 
-	wrmsrl(MSR_PKG_CST_CONFIG_CONTROL, per_cpu(pkg_cst_config, cpunum));
+	wrmsrl_pkg_cst_config_control(val);
 
-	pr_debug("%s: cpu:%d cst %llx\n", __func__, cpunum,
-		 per_cpu(pkg_cst_config, cpunum));
+	pr_debug("%s: cpu:%d cst %llx\n", __func__, cpunum, val);
 }
 
 static void s2idle_cpu_quirk(smp_call_func_t func)
-- 
2.47.1

Re: [RFC PATCH 1/1] x86: msr: add new 'msr_pkg_cst_config_control.h' header

[Rafael J. Wysocki]

On Thu, Feb 20, 2025 at 4:43 PM Artem Bityutskiy <dedekind1@gmail.com> wrote:
>
> From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
>
> There are now two users that modify the MSR_PKG_CST_CONFIG_CONTROL register:
> 1. The Intel PMC framework (cnp.c).
> 2. The intel_idle.c driver.
>
> They do not interfere with each other because the former modifies it only
> during suspend/resume.
>
> Introduce common accessor functions for the MSR to make it more clear that
> there is already more than one user. There is no other purpose at this point.
> But if more users are introduced, the header file may be replaced with a small
> MSR_PKG_CST_CONFIG_CONTROL subsystem, possibly implementing locking.
>
> Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> ---
>  .../include/asm/msr_pkg_cst_config_control.h  | 28 +++++++++++++++++++
>  drivers/idle/intel_idle.c                     | 17 +++++------
>  drivers/platform/x86/intel/pmc/cnp.c          | 13 +++++----
>  3 files changed, 44 insertions(+), 14 deletions(-)
>  create mode 100644 arch/x86/include/asm/msr_pkg_cst_config_control.h
>
> diff --git a/arch/x86/include/asm/msr_pkg_cst_config_control.h b/arch/x86/include/asm/msr_pkg_cst_config_control.h
> new file mode 100644
> index 000000000000..0d9dab4c20ef
> --- /dev/null
> +++ b/arch/x86/include/asm/msr_pkg_cst_config_control.h
> @@ -0,0 +1,28 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Accessors from MSR_PKG_CST_CONFIG_CONTROL (0xe2) MSR found on Intel CPUs.
> + *
> + * At this point provide only trival read/write functions. But this header file
> + * can be turned into a small library if there are more MSR users in the future.
> + */
> +
> +#ifndef _MSR_PKG_CST_CONFIG_CONTROL_H
> +#define _MSR_PKG_CST_CONFIG_CONTROL_H
> +
> +#include <asm/msr-index.h>
> +#include <asm/msr.h>
> +
> +static inline unsigned long long rdmsrl_pkg_cst_config_control(void)
> +{
> +       unsigned long long val;
> +
> +       rdmsrl(MSR_PKG_CST_CONFIG_CONTROL, val);
> +       return val;
> +}
> +
> +static inline void wrmsrl_pkg_cst_config_control(unsigned long long val)
> +{
> +       wrmsrl(MSR_PKG_CST_CONFIG_CONTROL, val);
> +}

I didn't actually mean this.

My comment was based on the observation that both
disable_c1_auto_demote() and the new code in intel_idle would
implement a very similar code pattern, that is

(1) read MSR_PKG_CST_CONFIG_CONTROL
(2) either set or clear NHM_C1_AUTO_DEMOTE (possibly along with some
additional bits) in the value read from it
(3) write the updated value to MSR_PKG_CST_CONFIG_CONTROL

and it would be good to have a wrapper for it.  So something like

static inline unsigned long long
msr_pkg_cst_config_c1_auto_demote(bool set, unsigned long long
other_bits)
{
        unsigned long long val, newval;

        rdmsrl(MSR_PKG_CST_CONFIG_CONTROL, val);

        if (set)
                newval |= NHM_C1_AUTO_DEMOTE | other_bits;
        else
                newval &= ~(NHM_C1_AUTO_DEMOTE | other_bits);

        wrmsrl(MSR_PKG_CST_CONFIG_CONTROL, newval);

        return val;
}

> +
> +#endif /* _MSR_PKG_CST_CONFIG_CONTROL_H */
> diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
> index 8d2095078469..e5415e20e0e3 100644
> --- a/drivers/idle/intel_idle.c
> +++ b/drivers/idle/intel_idle.c
> @@ -59,6 +59,7 @@
>  #include <asm/mwait.h>
>  #include <asm/spec-ctrl.h>
>  #include <asm/fpu/api.h>
> +#include <asm/msr_pkg_cst_config_control.h>
>
>  #define INTEL_IDLE_VERSION "0.5.1"
>
> @@ -1975,7 +1976,7 @@ static void __init sklh_idle_state_table_update(void)
>         if ((mwait_substates & (0xF << 28)) == 0)
>                 return;
>
> -       rdmsrl(MSR_PKG_CST_CONFIG_CONTROL, msr);
> +       msr = rdmsrl_pkg_cst_config_control();
>
>         /* PC10 is not enabled in PKG C-state limit */
>         if ((msr & 0xF) != 8)
> @@ -2006,7 +2007,7 @@ static void __init skx_idle_state_table_update(void)
>  {
>         unsigned long long msr;
>
> -       rdmsrl(MSR_PKG_CST_CONFIG_CONTROL, msr);
> +       msr = rdmsrl_pkg_cst_config_control();
>
>         /*
>          * 000b: C0/C1 (no package C-state support)
> @@ -2059,7 +2060,7 @@ static void __init spr_idle_state_table_update(void)
>          * C6. However, if PC6 is disabled, we update the numbers to match
>          * core C6.
>          */
> -       rdmsrl(MSR_PKG_CST_CONFIG_CONTROL, msr);
> +       msr = rdmsrl_pkg_cst_config_control();
>
>         /* Limit value 2 and above allow for PC6. */
>         if ((msr & 0x7) < 2) {
> @@ -2221,9 +2222,9 @@ static void auto_demotion_disable(void)
>  {
>         unsigned long long msr_bits;
>
> -       rdmsrl(MSR_PKG_CST_CONFIG_CONTROL, msr_bits);
> +       msr_bits = rdmsrl_pkg_cst_config_control();
>         msr_bits &= ~auto_demotion_disable_flags;
> -       wrmsrl(MSR_PKG_CST_CONFIG_CONTROL, msr_bits);
> +       wrmsrl_pkg_cst_config_control(msr_bits);
>  }
>
>  static void c1e_promotion_enable(void)
> @@ -2309,7 +2310,7 @@ static void intel_c1_demotion_toggle(void *info)
>         unsigned long long msr_val;
>         bool enable = *(bool *)info;
>
> -       rdmsrl(MSR_PKG_CST_CONFIG_CONTROL, msr_val);
> +       msr_val = rdmsrl_pkg_cst_config_control();
>         /*
>          * Enable/disable C1 undemotion along with C1 demotion, as this is the
>          * most sensible configuration in general.
> @@ -2318,7 +2319,7 @@ static void intel_c1_demotion_toggle(void *info)
>                 msr_val |= NHM_C1_AUTO_DEMOTE | SNB_C1_AUTO_UNDEMOTE;
>         else
>                 msr_val &= ~(NHM_C1_AUTO_DEMOTE | SNB_C1_AUTO_UNDEMOTE);
> -       wrmsrl(MSR_PKG_CST_CONFIG_CONTROL, msr_val);
> +       wrmsrl_pkg_cst_config_control(msr_val);

And now you can do

static void intel_c1_demotion_toggle(void *info)
{
        msr_pkg_cst_config_c1_auto_demote(*(bool *)info, SNB_C1_AUTO_UNDEMOTE);
}

here and similarly below.

>  }
>
>  static ssize_t intel_c1_demotion_store(struct device *dev,
> @@ -2349,7 +2350,7 @@ static ssize_t intel_c1_demotion_show(struct device *dev,
>          * Read the MSR value for a CPU and assume it is the same for all CPUs. Any other
>          * configureation would be a BIOS bug.
>          */
> -       rdmsrl(MSR_PKG_CST_CONFIG_CONTROL, msr_val);
> +       msr_val = rdmsrl_pkg_cst_config_control();
>         return sysfs_emit(buf, "%d\n", !!(msr_val & NHM_C1_AUTO_DEMOTE));
>  }
>  static DEVICE_ATTR_RW(intel_c1_demotion);
> diff --git a/drivers/platform/x86/intel/pmc/cnp.c b/drivers/platform/x86/intel/pmc/cnp.c
> index fc5193fdf8a8..4ef8dfe07664 100644
> --- a/drivers/platform/x86/intel/pmc/cnp.c
> +++ b/drivers/platform/x86/intel/pmc/cnp.c
> @@ -10,6 +10,7 @@
>
>  #include <linux/smp.h>
>  #include <linux/suspend.h>
> +#include <asm/msr_pkg_cst_config_control.h>
>  #include "core.h"
>
>  /* Cannon Lake: PGD PFET Enable Ack Status Register(s) bitmap */
> @@ -225,12 +226,12 @@ static DEFINE_PER_CPU(u64, pkg_cst_config);
>  static void disable_c1_auto_demote(void *unused)
>  {
>         int cpunum = smp_processor_id();
> -       u64 val;
> +       unsigned long long val;
>
> -       rdmsrl(MSR_PKG_CST_CONFIG_CONTROL, val);
> +       val = rdmsrl_pkg_cst_config_control();
>         per_cpu(pkg_cst_config, cpunum) = val;
>         val &= ~NHM_C1_AUTO_DEMOTE;
> -       wrmsrl(MSR_PKG_CST_CONFIG_CONTROL, val);
> +       wrmsrl_pkg_cst_config_control(val);

That is

per_cpu(pkg_cst_config, cpunum) = msr_pkg_cst_config_c1_auto_demote(false, 0);

>         pr_debug("%s: cpu:%d cst %llx\n", __func__, cpunum, val);
>  }
> @@ -238,11 +239,11 @@ static void disable_c1_auto_demote(void *unused)
>  static void restore_c1_auto_demote(void *unused)
>  {
>         int cpunum = smp_processor_id();
> +       unsigned long long val = per_cpu(pkg_cst_config, cpunum);
>
> -       wrmsrl(MSR_PKG_CST_CONFIG_CONTROL, per_cpu(pkg_cst_config, cpunum));
> +       wrmsrl_pkg_cst_config_control(val);

And I would leave this code as is (in this patch, but generally, as a
matter of cleanup, adding a local variable var to it would make sense
IMV).

>
> -       pr_debug("%s: cpu:%d cst %llx\n", __func__, cpunum,
> -                per_cpu(pkg_cst_config, cpunum));
> +       pr_debug("%s: cpu:%d cst %llx\n", __func__, cpunum, val);
>  }
>
>  static void s2idle_cpu_quirk(smp_call_func_t func)
> --

Re: [RFC PATCH 1/1] x86: msr: add new 'msr_pkg_cst_config_control.h' header

[Artem Bityutskiy]

Hi Rafael,

sorry for so long delay.

On Thu, 2025-02-27 at 21:01 +0100, Rafael J. Wysocki wrote:
> static inline unsigned long long
> msr_pkg_cst_config_c1_auto_demote(bool set, unsigned long long
> other_bits)
> {
>         unsigned long long val, newval;
> 
>         rdmsrl(MSR_PKG_CST_CONFIG_CONTROL, val);
> 
>         if (set)
>                 newval |= NHM_C1_AUTO_DEMOTE | other_bits;
>         else
>                 newval &= ~(NHM_C1_AUTO_DEMOTE | other_bits);
> 
>         wrmsrl(MSR_PKG_CST_CONFIG_CONTROL, newval);
> 
>         return val;
> }

Sure I can do this, sure, thanks. 

One reason why I am still not sure it is worth it is because the 
"lincroft" platform the C1 demotion bit is

#define ATM_LNC_C6_AUTO_DEMOTE		(1UL << 25)

so this function does not seem to be universal enough to cover this code in
intel_idle.c:

static void auto_demotion_disable(void)
{
	unsigned long long msr_bits;

	wrmsrl_pkg_cst_config_control(false, auto_demotion_disable_flags);
	msr_bits = rdmsrl_pkg_cst_config_control();
	msr_bits &= ~auto_demotion_disable_flags;
	wrmsrl_pkg_cst_config_control(msr_bits);
}

Artem.