* [RFC PATCH 0/1] x86: msr: add new 'msr_pkg_cst_config_control.h' header @ 2025-02-20 15:43 Artem Bityutskiy 2025-02-20 15:43 ` [RFC PATCH 1/1] " Artem Bityutskiy 0 siblings, 1 reply; 4+ messages in thread From: Artem Bityutskiy @ 2025-02-20 15:43 UTC (permalink / raw) To: Rafael J. Wysocki; +Cc: Linux PM Mailing List 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* [RFC PATCH 1/1] x86: msr: add new 'msr_pkg_cst_config_control.h' header 2025-02-20 15:43 [RFC PATCH 0/1] x86: msr: add new 'msr_pkg_cst_config_control.h' header Artem Bityutskiy @ 2025-02-20 15:43 ` Artem Bityutskiy 2025-02-27 20:01 ` Rafael J. Wysocki 0 siblings, 1 reply; 4+ messages in thread From: Artem Bityutskiy @ 2025-02-20 15:43 UTC (permalink / raw) To: Rafael J. Wysocki; +Cc: Linux PM Mailing List 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [RFC PATCH 1/1] x86: msr: add new 'msr_pkg_cst_config_control.h' header 2025-02-20 15:43 ` [RFC PATCH 1/1] " Artem Bityutskiy @ 2025-02-27 20:01 ` Rafael J. Wysocki 2025-03-17 12:40 ` Artem Bityutskiy 0 siblings, 1 reply; 4+ messages in thread From: Rafael J. Wysocki @ 2025-02-27 20:01 UTC (permalink / raw) To: Artem Bityutskiy; +Cc: Rafael J. Wysocki, Linux PM Mailing List 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) > -- ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC PATCH 1/1] x86: msr: add new 'msr_pkg_cst_config_control.h' header 2025-02-27 20:01 ` Rafael J. Wysocki @ 2025-03-17 12:40 ` Artem Bityutskiy 0 siblings, 0 replies; 4+ messages in thread From: Artem Bityutskiy @ 2025-03-17 12:40 UTC (permalink / raw) To: Rafael J. Wysocki; +Cc: Linux PM Mailing List 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. ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-03-17 12:40 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-02-20 15:43 [RFC PATCH 0/1] x86: msr: add new 'msr_pkg_cst_config_control.h' header Artem Bityutskiy 2025-02-20 15:43 ` [RFC PATCH 1/1] " Artem Bityutskiy 2025-02-27 20:01 ` Rafael J. Wysocki 2025-03-17 12:40 ` Artem Bityutskiy
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).