| Message ID | 20250220154306.2166129-2-dedekind1@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | x86: msr: add new 'msr_pkg_cst_config_control.h' header | expand |
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) > --
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.
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)