[PATCH RFC 0/6] x86/msr: Rename MSR access functions

[PATCH RFC 0/6] x86/msr: Rename MSR access functions

[Juergen Gross]

This is a RFC for renaming the main MSR access functions. The main
motivations for doing that are:

- Prepare for wide spread use cases of WRMSRNS by making it easily
  visible whether a MSR write is serializing or not. This has the
  advantage that new use cases of MSR writes would need to decide
  whether a serializing MSR write is needed or not. Using today's
  MSR write interfaces would probably result in most new use cases
  to use the serializing form, having a negative performance impact.

- Use functions instead of macros for accessing MSRs, which will drop
  modifying variables passed as a parameter.

- Eliminate multiple accessors doing exactly the same thing (e.g.
  rdmsrl() and rdmsrq()).

- Instead of having function names based on the underlying instruction
  mnemonics, have functions of a common name space (msr_*()).

This series is containing only a needed prerequisite patch removing a
name collision with the new access functions, 3 patches introducing
the new access functions, and 2 example patches adapting users to use
the new functions instead the old ones.

I have selected the 2 example patches based on the needed code changes:

Patch 5 is very simple, because all the changes are really trivial.
While using a mixture of serializing and non-serializing MSR writes,
the non-serializing form is used only in case another MSR write is
following in the same function, resulting in each function doing MSR
writes having still serializing semantics (it might be possible to
change that later, but I wanted to be conservative just in case).

Patch 6 is a little bit less simple, as there are a couple of cases
where the new functions are no direct replacements of the old
interfaces. The new functions only work on 64-bit values, so in cases
where 2 32-bit values are needed, "struct msr" is used as a conversion
layer. I thought about adding macros for the same purpose, but in the
end this seemed to be a more simple and readable solution.

In case the general idea is accepted, I'd do the conversion of the rest
of the users (this will be probably a rather large, but mostly trivial
series).

As this series is RFC, I have done only basic compile testing.

Juergen Gross (6):
  x86/msr: Rename msr_read() and msr_write()
  x86/msr: Create a new minimal set of local MSR access functions
  x86/msr: Create a new minimal set of inter-CPU MSR access functions
  x86/msr: Rename the *_safe_regs[_on_cpu]() MSR functions
  x86/events: Switch core parts to use new MSR access functions
  x86/cpu/mce: Switch code to use new MSR access functions

 arch/x86/events/core.c            |  42 ++++++-------
 arch/x86/events/msr.c             |   2 +-
 arch/x86/events/perf_event.h      |  26 ++++----
 arch/x86/events/probe.c           |   2 +-
 arch/x86/events/rapl.c            |   8 +--
 arch/x86/include/asm/msr.h        |  87 ++++++++++++++++++++++---
 arch/x86/kernel/cpu/amd.c         |   4 +-
 arch/x86/kernel/cpu/mce/amd.c     | 101 +++++++++++++++---------------
 arch/x86/kernel/cpu/mce/core.c    |  18 +++---
 arch/x86/kernel/cpu/mce/inject.c  |  40 ++++++------
 arch/x86/kernel/cpu/mce/intel.c   |  32 +++++-----
 arch/x86/kernel/cpu/mce/p5.c      |  16 ++---
 arch/x86/kernel/cpu/mce/winchip.c |  10 +--
 arch/x86/kernel/msr.c             |  16 ++---
 arch/x86/lib/msr-reg-export.c     |   4 +-
 arch/x86/lib/msr-reg.S            |  16 ++---
 arch/x86/lib/msr-smp.c            |  20 +++---
 arch/x86/lib/msr.c                |  12 ++--
 18 files changed, 263 insertions(+), 193 deletions(-)

-- 
2.53.0

[PATCH RFC 5/6] x86/events: Switch core parts to use new MSR access functions

[Juergen Gross]

Switch the core parts of the x86 events subsystem to use the new
msr_*() functions instead of the rdmsr*()/wrmsr*() ones.

Use msr_write_noser() in case there is another MSR write later in the
same function and msr_write_ser() for the last MSR write in a function.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 arch/x86/events/core.c       | 42 ++++++++++++++++++------------------
 arch/x86/events/msr.c        |  2 +-
 arch/x86/events/perf_event.h | 26 +++++++++++-----------
 arch/x86/events/probe.c      |  2 +-
 arch/x86/events/rapl.c       |  8 +++----
 5 files changed, 40 insertions(+), 40 deletions(-)

diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
index 810ab21ffd99..c15e0d1a6658 100644
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -279,7 +279,7 @@ bool check_hw_exists(struct pmu *pmu, unsigned long *cntr_mask,
 	 */
 	for_each_set_bit(i, cntr_mask, X86_PMC_IDX_MAX) {
 		reg = x86_pmu_config_addr(i);
-		ret = rdmsrq_safe(reg, &val);
+		ret = msr_read_safe(reg, &val);
 		if (ret)
 			goto msr_fail;
 		if (val & ARCH_PERFMON_EVENTSEL_ENABLE) {
@@ -293,7 +293,7 @@ bool check_hw_exists(struct pmu *pmu, unsigned long *cntr_mask,
 
 	if (*(u64 *)fixed_cntr_mask) {
 		reg = MSR_ARCH_PERFMON_FIXED_CTR_CTRL;
-		ret = rdmsrq_safe(reg, &val);
+		ret = msr_read_safe(reg, &val);
 		if (ret)
 			goto msr_fail;
 		for_each_set_bit(i, fixed_cntr_mask, X86_PMC_IDX_MAX) {
@@ -324,11 +324,11 @@ bool check_hw_exists(struct pmu *pmu, unsigned long *cntr_mask,
 	 * (qemu/kvm) that don't trap on the MSR access and always return 0s.
 	 */
 	reg = x86_pmu_event_addr(reg_safe);
-	if (rdmsrq_safe(reg, &val))
+	if (msr_read_safe(reg, &val))
 		goto msr_fail;
 	val ^= 0xffffUL;
-	ret = wrmsrq_safe(reg, val);
-	ret |= rdmsrq_safe(reg, &val_new);
+	ret = msr_write_safe_noser(reg, val);
+	ret |= msr_read_safe(reg, &val_new);
 	if (ret || val != val_new)
 		goto msr_fail;
 
@@ -713,13 +713,13 @@ void x86_pmu_disable_all(void)
 
 		if (!test_bit(idx, cpuc->active_mask))
 			continue;
-		rdmsrq(x86_pmu_config_addr(idx), val);
+		val = msr_read(x86_pmu_config_addr(idx));
 		if (!(val & ARCH_PERFMON_EVENTSEL_ENABLE))
 			continue;
 		val &= ~ARCH_PERFMON_EVENTSEL_ENABLE;
-		wrmsrq(x86_pmu_config_addr(idx), val);
+		msr_write_noser(x86_pmu_config_addr(idx), val);
 		if (is_counter_pair(hwc))
-			wrmsrq(x86_pmu_config_addr(idx + 1), 0);
+			msr_write_noser(x86_pmu_config_addr(idx + 1), 0);
 	}
 }
 
@@ -1446,14 +1446,14 @@ int x86_perf_event_set_period(struct perf_event *event)
 	 */
 	local64_set(&hwc->prev_count, (u64)-left);
 
-	wrmsrq(hwc->event_base, (u64)(-left) & x86_pmu.cntval_mask);
+	msr_write_noser(hwc->event_base, (u64)(-left) & x86_pmu.cntval_mask);
 
 	/*
 	 * Sign extend the Merge event counter's upper 16 bits since
 	 * we currently declare a 48-bit counter width
 	 */
 	if (is_counter_pair(hwc))
-		wrmsrq(x86_pmu_event_addr(idx + 1), 0xffff);
+		msr_write_noser(x86_pmu_event_addr(idx + 1), 0xffff);
 
 	perf_event_update_userpage(event);
 
@@ -1575,10 +1575,10 @@ void perf_event_print_debug(void)
 		return;
 
 	if (x86_pmu.version >= 2) {
-		rdmsrq(MSR_CORE_PERF_GLOBAL_CTRL, ctrl);
-		rdmsrq(MSR_CORE_PERF_GLOBAL_STATUS, status);
-		rdmsrq(MSR_CORE_PERF_GLOBAL_OVF_CTRL, overflow);
-		rdmsrq(MSR_ARCH_PERFMON_FIXED_CTR_CTRL, fixed);
+		ctrl = msr_read(MSR_CORE_PERF_GLOBAL_CTRL);
+		status = msr_read(MSR_CORE_PERF_GLOBAL_STATUS);
+		overflow = msr_read(MSR_CORE_PERF_GLOBAL_OVF_CTRL);
+		fixed = msr_read(MSR_ARCH_PERFMON_FIXED_CTR_CTRL);
 
 		pr_info("\n");
 		pr_info("CPU#%d: ctrl:       %016llx\n", cpu, ctrl);
@@ -1586,19 +1586,19 @@ void perf_event_print_debug(void)
 		pr_info("CPU#%d: overflow:   %016llx\n", cpu, overflow);
 		pr_info("CPU#%d: fixed:      %016llx\n", cpu, fixed);
 		if (pebs_constraints) {
-			rdmsrq(MSR_IA32_PEBS_ENABLE, pebs);
+			pebs = msr_read(MSR_IA32_PEBS_ENABLE);
 			pr_info("CPU#%d: pebs:       %016llx\n", cpu, pebs);
 		}
 		if (x86_pmu.lbr_nr) {
-			rdmsrq(MSR_IA32_DEBUGCTLMSR, debugctl);
+			debugctl = msr_read(MSR_IA32_DEBUGCTLMSR);
 			pr_info("CPU#%d: debugctl:   %016llx\n", cpu, debugctl);
 		}
 	}
 	pr_info("CPU#%d: active:     %016llx\n", cpu, *(u64 *)cpuc->active_mask);
 
 	for_each_set_bit(idx, cntr_mask, X86_PMC_IDX_MAX) {
-		rdmsrq(x86_pmu_config_addr(idx), pmc_ctrl);
-		rdmsrq(x86_pmu_event_addr(idx), pmc_count);
+		pmc_ctrl = msr_read(x86_pmu_config_addr(idx));
+		pmc_count = msr_read(x86_pmu_event_addr(idx));
 
 		prev_left = per_cpu(pmc_prev_left[idx], cpu);
 
@@ -1612,7 +1612,7 @@ void perf_event_print_debug(void)
 	for_each_set_bit(idx, fixed_cntr_mask, X86_PMC_IDX_MAX) {
 		if (fixed_counter_disabled(idx, cpuc->pmu))
 			continue;
-		rdmsrq(x86_pmu_fixed_ctr_addr(idx), pmc_count);
+		pmc_count = msr_read(x86_pmu_fixed_ctr_addr(idx));
 
 		pr_info("CPU#%d: fixed-PMC%d count: %016llx\n",
 			cpu, idx, pmc_count);
@@ -2560,9 +2560,9 @@ void perf_clear_dirty_counters(void)
 			if (!test_bit(i - INTEL_PMC_IDX_FIXED, hybrid(cpuc->pmu, fixed_cntr_mask)))
 				continue;
 
-			wrmsrq(x86_pmu_fixed_ctr_addr(i - INTEL_PMC_IDX_FIXED), 0);
+			msr_write_noser(x86_pmu_fixed_ctr_addr(i - INTEL_PMC_IDX_FIXED), 0);
 		} else {
-			wrmsrq(x86_pmu_event_addr(i), 0);
+			msr_write_noser(x86_pmu_event_addr(i), 0);
 		}
 	}
 
diff --git a/arch/x86/events/msr.c b/arch/x86/events/msr.c
index 76d6418c5055..09d5b2808727 100644
--- a/arch/x86/events/msr.c
+++ b/arch/x86/events/msr.c
@@ -158,7 +158,7 @@ static inline u64 msr_read_counter(struct perf_event *event)
 	u64 now;
 
 	if (event->hw.event_base)
-		rdmsrq(event->hw.event_base, now);
+		now = msr_read(event->hw.event_base);
 	else
 		now = rdtsc_ordered();
 
diff --git a/arch/x86/events/perf_event.h b/arch/x86/events/perf_event.h
index fad87d3c8b2c..cce2e7b67c01 100644
--- a/arch/x86/events/perf_event.h
+++ b/arch/x86/events/perf_event.h
@@ -1271,16 +1271,16 @@ static inline void __x86_pmu_enable_event(struct hw_perf_event *hwc,
 	u64 disable_mask = __this_cpu_read(cpu_hw_events.perf_ctr_virt_mask);
 
 	if (hwc->extra_reg.reg)
-		wrmsrq(hwc->extra_reg.reg, hwc->extra_reg.config);
+		msr_write_noser(hwc->extra_reg.reg, hwc->extra_reg.config);
 
 	/*
 	 * Add enabled Merge event on next counter
 	 * if large increment event being enabled on this counter
 	 */
 	if (is_counter_pair(hwc))
-		wrmsrq(x86_pmu_config_addr(hwc->idx + 1), x86_pmu.perf_ctr_pair_en);
+		msr_write_noser(x86_pmu_config_addr(hwc->idx + 1), x86_pmu.perf_ctr_pair_en);
 
-	wrmsrq(hwc->config_base, (hwc->config | enable_mask) & ~disable_mask);
+	msr_write_ser(hwc->config_base, (hwc->config | enable_mask) & ~disable_mask);
 }
 
 void x86_pmu_enable_all(int added);
@@ -1296,10 +1296,10 @@ static inline void x86_pmu_disable_event(struct perf_event *event)
 	u64 disable_mask = __this_cpu_read(cpu_hw_events.perf_ctr_virt_mask);
 	struct hw_perf_event *hwc = &event->hw;
 
-	wrmsrq(hwc->config_base, hwc->config & ~disable_mask);
+	msr_write_ser(hwc->config_base, hwc->config & ~disable_mask);
 
 	if (is_counter_pair(hwc))
-		wrmsrq(x86_pmu_config_addr(hwc->idx + 1), 0);
+		msr_write_ser(x86_pmu_config_addr(hwc->idx + 1), 0);
 }
 
 void x86_pmu_enable_event(struct perf_event *event);
@@ -1473,12 +1473,12 @@ static __always_inline void __amd_pmu_lbr_disable(void)
 {
 	u64 dbg_ctl, dbg_extn_cfg;
 
-	rdmsrq(MSR_AMD_DBG_EXTN_CFG, dbg_extn_cfg);
-	wrmsrq(MSR_AMD_DBG_EXTN_CFG, dbg_extn_cfg & ~DBG_EXTN_CFG_LBRV2EN);
+	dbg_extn_cfg = msr_read(MSR_AMD_DBG_EXTN_CFG);
+	msr_write_ser(MSR_AMD_DBG_EXTN_CFG, dbg_extn_cfg & ~DBG_EXTN_CFG_LBRV2EN);
 
 	if (cpu_feature_enabled(X86_FEATURE_AMD_LBR_PMC_FREEZE)) {
-		rdmsrq(MSR_IA32_DEBUGCTLMSR, dbg_ctl);
-		wrmsrq(MSR_IA32_DEBUGCTLMSR, dbg_ctl & ~DEBUGCTLMSR_FREEZE_LBRS_ON_PMI);
+		dbg_ctl = msr_read(MSR_IA32_DEBUGCTLMSR);
+		msr_write_ser(MSR_IA32_DEBUGCTLMSR, dbg_ctl & ~DEBUGCTLMSR_FREEZE_LBRS_ON_PMI);
 	}
 }
 
@@ -1619,21 +1619,21 @@ static inline bool intel_pmu_has_bts(struct perf_event *event)
 
 static __always_inline void __intel_pmu_pebs_disable_all(void)
 {
-	wrmsrq(MSR_IA32_PEBS_ENABLE, 0);
+	msr_write_ser(MSR_IA32_PEBS_ENABLE, 0);
 }
 
 static __always_inline void __intel_pmu_arch_lbr_disable(void)
 {
-	wrmsrq(MSR_ARCH_LBR_CTL, 0);
+	msr_write_ser(MSR_ARCH_LBR_CTL, 0);
 }
 
 static __always_inline void __intel_pmu_lbr_disable(void)
 {
 	u64 debugctl;
 
-	rdmsrq(MSR_IA32_DEBUGCTLMSR, debugctl);
+	debugctl = msr_read(MSR_IA32_DEBUGCTLMSR);
 	debugctl &= ~(DEBUGCTLMSR_LBR | DEBUGCTLMSR_FREEZE_LBRS_ON_PMI);
-	wrmsrq(MSR_IA32_DEBUGCTLMSR, debugctl);
+	msr_write_ser(MSR_IA32_DEBUGCTLMSR, debugctl);
 }
 
 int intel_pmu_save_and_restart(struct perf_event *event);
diff --git a/arch/x86/events/probe.c b/arch/x86/events/probe.c
index bb719d0d3f0b..85d591fab26c 100644
--- a/arch/x86/events/probe.c
+++ b/arch/x86/events/probe.c
@@ -45,7 +45,7 @@ perf_msr_probe(struct perf_msr *msr, int cnt, bool zero, void *data)
 			if (msr[bit].test && !msr[bit].test(bit, data))
 				continue;
 			/* Virt sucks; you cannot tell if a R/O MSR is present :/ */
-			if (rdmsrq_safe(msr[bit].msr, &val))
+			if (msr_read_safe(msr[bit].msr, &val))
 				continue;
 
 			mask = msr[bit].mask;
diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c
index 8ed03c32f560..bb9ecf78fd90 100644
--- a/arch/x86/events/rapl.c
+++ b/arch/x86/events/rapl.c
@@ -193,7 +193,7 @@ static inline unsigned int get_rapl_pmu_idx(int cpu, int scope)
 static inline u64 rapl_read_counter(struct perf_event *event)
 {
 	u64 raw;
-	rdmsrq(event->hw.event_base, raw);
+	raw = msr_read(event->hw.event_base);
 	return raw;
 }
 
@@ -222,7 +222,7 @@ static u64 rapl_event_update(struct perf_event *event)
 
 	prev_raw_count = local64_read(&hwc->prev_count);
 	do {
-		rdmsrq(event->hw.event_base, new_raw_count);
+		new_raw_count = msr_read(event->hw.event_base);
 	} while (!local64_try_cmpxchg(&hwc->prev_count,
 				      &prev_raw_count, new_raw_count));
 
@@ -611,8 +611,8 @@ static int rapl_check_hw_unit(void)
 	u64 msr_rapl_power_unit_bits;
 	int i;
 
-	/* protect rdmsrq() to handle virtualization */
-	if (rdmsrq_safe(rapl_model->msr_power_unit, &msr_rapl_power_unit_bits))
+	/* protect msr_read() to handle virtualization */
+	if (msr_read_safe(rapl_model->msr_power_unit, &msr_rapl_power_unit_bits))
 		return -1;
 	for (i = 0; i < NR_RAPL_PKG_DOMAINS; i++)
 		rapl_pkg_hw_unit[i] = (msr_rapl_power_unit_bits >> 8) & 0x1FULL;
-- 
2.53.0

Re: [PATCH RFC 0/6] x86/msr: Rename MSR access functions

[Peter Zijlstra]

On Mon, Apr 20, 2026 at 11:16:28AM +0200, Juergen Gross wrote:

> - Use functions instead of macros for accessing MSRs, which will drop
>   modifying variables passed as a parameter.
> 
> - Eliminate multiple accessors doing exactly the same thing (e.g.
>   rdmsrl() and rdmsrq()).

So far so sane.

> - Instead of having function names based on the underlying instruction
>   mnemonics, have functions of a common name space (msr_*()).

Not sure on this one. The whole msr_{read,write}_{safe,noser}() thing is
a royal pain. Also 'noser' reads to me as the noun that goes with 'to
nose' [he that noses (around), like baker: he that bakes].

I would much rather we just stick to the mnemonics here. All of this
really is about wrapping single instructions, no need to make it an
unreadable mess.

Re: [PATCH RFC 0/6] x86/msr: Rename MSR access functions

[Peter Zijlstra]

On Mon, Apr 20, 2026 at 01:35:12PM +0200, Peter Zijlstra wrote:
> On Mon, Apr 20, 2026 at 11:16:28AM +0200, Juergen Gross wrote:
> 
> > - Use functions instead of macros for accessing MSRs, which will drop
> >   modifying variables passed as a parameter.
> > 
> > - Eliminate multiple accessors doing exactly the same thing (e.g.
> >   rdmsrl() and rdmsrq()).
> 
> So far so sane.
> 
> > - Instead of having function names based on the underlying instruction
> >   mnemonics, have functions of a common name space (msr_*()).
> 
> Not sure on this one. The whole msr_{read,write}_{safe,noser}() thing is
> a royal pain. Also 'noser' reads to me as the noun that goes with 'to
> nose' [he that noses (around), like baker: he that bakes].
> 
> I would much rather we just stick to the mnemonics here. All of this
> really is about wrapping single instructions, no need to make it an
> unreadable mess.

Also, the _safe suffix should just go away. All MSR accessors should be
'safe'.

Re: [PATCH RFC 0/6] x86/msr: Rename MSR access functions

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 1434 bytes --]

On 20.04.26 13:35, Peter Zijlstra wrote:
> On Mon, Apr 20, 2026 at 11:16:28AM +0200, Juergen Gross wrote:
> 
>> - Use functions instead of macros for accessing MSRs, which will drop
>>    modifying variables passed as a parameter.
>>
>> - Eliminate multiple accessors doing exactly the same thing (e.g.
>>    rdmsrl() and rdmsrq()).
> 
> So far so sane.
> 
>> - Instead of having function names based on the underlying instruction
>>    mnemonics, have functions of a common name space (msr_*()).
> 
> Not sure on this one. The whole msr_{read,write}_{safe,noser}() thing is
> a royal pain. Also 'noser' reads to me as the noun that goes with 'to
> nose' [he that noses (around), like baker: he that bakes].

Naming is hard. :-)

What about s/ser/sync/ then?

> I would much rather we just stick to the mnemonics here. All of this
> really is about wrapping single instructions, no need to make it an
> unreadable mess.

I'm pretty sure most of the wrmsr*() use cases could switch to the non
serializing variants. The problem not making the serializing aspect visible
in the function name will probably result in most new instances still using
the serializing variant instead of the probably possible non serializing one.

Many of those use cases will even suffer more, as they won't use the
immediate form of WRMSRNS then, which would waste the additional benefits of
that instruction.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH RFC 0/6] x86/msr: Rename MSR access functions

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 1178 bytes --]

On 20.04.26 13:41, Peter Zijlstra wrote:
> On Mon, Apr 20, 2026 at 01:35:12PM +0200, Peter Zijlstra wrote:
>> On Mon, Apr 20, 2026 at 11:16:28AM +0200, Juergen Gross wrote:
>>
>>> - Use functions instead of macros for accessing MSRs, which will drop
>>>    modifying variables passed as a parameter.
>>>
>>> - Eliminate multiple accessors doing exactly the same thing (e.g.
>>>    rdmsrl() and rdmsrq()).
>>
>> So far so sane.
>>
>>> - Instead of having function names based on the underlying instruction
>>>    mnemonics, have functions of a common name space (msr_*()).
>>
>> Not sure on this one. The whole msr_{read,write}_{safe,noser}() thing is
>> a royal pain. Also 'noser' reads to me as the noun that goes with 'to
>> nose' [he that noses (around), like baker: he that bakes].
>>
>> I would much rather we just stick to the mnemonics here. All of this
>> really is about wrapping single instructions, no need to make it an
>> unreadable mess.
> 
> Also, the _safe suffix should just go away. All MSR accessors should be
> 'safe'.

That would be fine by me, but I'd like to have some confirmation this is
really the route to go.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH RFC 0/6] x86/msr: Rename MSR access functions

[Peter Zijlstra]

[-- Attachment #1: Type: text/plain, Size: 2447 bytes --]

On Mon, Apr 20, 2026 at 01:49:02PM +0200, Jürgen Groß wrote:
> On 20.04.26 13:35, Peter Zijlstra wrote:
> > On Mon, Apr 20, 2026 at 11:16:28AM +0200, Juergen Gross wrote:
> > 
> > > - Use functions instead of macros for accessing MSRs, which will drop
> > >    modifying variables passed as a parameter.
> > > 
> > > - Eliminate multiple accessors doing exactly the same thing (e.g.
> > >    rdmsrl() and rdmsrq()).
> > 
> > So far so sane.
> > 
> > > - Instead of having function names based on the underlying instruction
> > >    mnemonics, have functions of a common name space (msr_*()).
> > 
> > Not sure on this one. The whole msr_{read,write}_{safe,noser}() thing is
> > a royal pain. Also 'noser' reads to me as the noun that goes with 'to
> > nose' [he that noses (around), like baker: he that bakes].
> 
> Naming is hard. :-)
> 
> What about s/ser/sync/ then?
> 
> > I would much rather we just stick to the mnemonics here. All of this
> > really is about wrapping single instructions, no need to make it an
> > unreadable mess.
> 
> I'm pretty sure most of the wrmsr*() use cases could switch to the non
> serializing variants. The problem not making the serializing aspect visible
> in the function name will probably result in most new instances still using
> the serializing variant instead of the probably possible non serializing one.
> 
> Many of those use cases will even suffer more, as they won't use the
> immediate form of WRMSRNS then, which would waste the additional benefits of
> that instruction.

I'm confused, if we have a wrmsrns() function, that could see if the msr
argument was a constant and use the immediate form, no?

That is, we have the following instructions: RDMSR, WRMSR, WRMSRNS
And we should have the exact same functions:

	val = rdmsr(msr);
	wrmsr(msr, val);
	wrmsrns(msr, val);

The only interesting question is what to do with the 'safe' aspect. The
instruction takes a fault, we do the extable, but rdmsr() above already
has a return value, so that can't be used.

One option is to, like uaccess and the proposed overflow, is to use
labels like:

	val = rdmsr(msr, label);

And then, even though the wrmsr*() functions have the return available,
do we want to be consistent and do:

	wrmsr(msr, val, label);
	wrmsrns(msr, val, label);

rather than be inconsistent and have them have a boolean return for
success.

What am I missing?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH RFC 0/6] x86/msr: Rename MSR access functions

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 3310 bytes --]

On 20.04.26 14:33, Peter Zijlstra wrote:
> On Mon, Apr 20, 2026 at 01:49:02PM +0200, Jürgen Groß wrote:
>> On 20.04.26 13:35, Peter Zijlstra wrote:
>>> On Mon, Apr 20, 2026 at 11:16:28AM +0200, Juergen Gross wrote:
>>>
>>>> - Use functions instead of macros for accessing MSRs, which will drop
>>>>     modifying variables passed as a parameter.
>>>>
>>>> - Eliminate multiple accessors doing exactly the same thing (e.g.
>>>>     rdmsrl() and rdmsrq()).
>>>
>>> So far so sane.
>>>
>>>> - Instead of having function names based on the underlying instruction
>>>>     mnemonics, have functions of a common name space (msr_*()).
>>>
>>> Not sure on this one. The whole msr_{read,write}_{safe,noser}() thing is
>>> a royal pain. Also 'noser' reads to me as the noun that goes with 'to
>>> nose' [he that noses (around), like baker: he that bakes].
>>
>> Naming is hard. :-)
>>
>> What about s/ser/sync/ then?
>>
>>> I would much rather we just stick to the mnemonics here. All of this
>>> really is about wrapping single instructions, no need to make it an
>>> unreadable mess.
>>
>> I'm pretty sure most of the wrmsr*() use cases could switch to the non
>> serializing variants. The problem not making the serializing aspect visible
>> in the function name will probably result in most new instances still using
>> the serializing variant instead of the probably possible non serializing one.
>>
>> Many of those use cases will even suffer more, as they won't use the
>> immediate form of WRMSRNS then, which would waste the additional benefits of
>> that instruction.
> 
> I'm confused, if we have a wrmsrns() function, that could see if the msr
> argument was a constant and use the immediate form, no?
> 
> That is, we have the following instructions: RDMSR, WRMSR, WRMSRNS
> And we should have the exact same functions:
> 
> 	val = rdmsr(msr);
> 	wrmsr(msr, val);
> 	wrmsrns(msr, val);

People tend to copy similar code, maybe using older kernels as the source.

So even if wrmsrns() would be fine (and, resulting from that, better), they
will more likely end up using wrmsr() instead.

Using new function names implying the exact semantics (serializing vs.
non-serializing) will make it more likely the correct one is being used.

> The only interesting question is what to do with the 'safe' aspect. The
> instruction takes a fault, we do the extable, but rdmsr() above already
> has a return value, so that can't be used.
> 
> One option is to, like uaccess and the proposed overflow, is to use
> labels like:
> 
> 	val = rdmsr(msr, label);
> 
> And then, even though the wrmsr*() functions have the return available,
> do we want to be consistent and do:
> 
> 	wrmsr(msr, val, label);
> 	wrmsrns(msr, val, label);
> 
> rather than be inconsistent and have them have a boolean return for
> success.
> 
> What am I missing?

I like the idea to use a label, but this would result in the need to use
macros instead of functions. So this is trading one aspect against another.
I'm not sure which is the better one here.

An alternative might be to switch rdmsr() to the interface used by rdmsr_safe(),
i.e. let all the accessors return a bool for success/failure and use a pointer
for the MSR value in rdmsr().


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH RFC 0/6] x86/msr: Rename MSR access functions

[Peter Zijlstra]

[-- Attachment #1: Type: text/plain, Size: 2205 bytes --]

On Mon, Apr 20, 2026 at 03:01:31PM +0200, Jürgen Groß wrote:
> On 20.04.26 14:33, Peter Zijlstra wrote:

> > That is, we have the following instructions: RDMSR, WRMSR, WRMSRNS
> > And we should have the exact same functions:
> > 
> > 	val = rdmsr(msr);
> > 	wrmsr(msr, val);
> > 	wrmsrns(msr, val);
> 
> People tend to copy similar code, maybe using older kernels as the source.
> 
> So even if wrmsrns() would be fine (and, resulting from that, better), they
> will more likely end up using wrmsr() instead.
> 
> Using new function names implying the exact semantics (serializing vs.
> non-serializing) will make it more likely the correct one is being used.

You cannot fix stupid. If you want friction, the label thing will
ensure 'old' code doesn't compile and will need fixing.

Also, if wrmsrns() really is faster, the performance folks will finger
'incorrect' wrmsr() usage sooner or later.

> > The only interesting question is what to do with the 'safe' aspect. The
> > instruction takes a fault, we do the extable, but rdmsr() above already
> > has a return value, so that can't be used.
> > 
> > One option is to, like uaccess and the proposed overflow, is to use
> > labels like:
> > 
> > 	val = rdmsr(msr, label);
> > 
> > And then, even though the wrmsr*() functions have the return available,
> > do we want to be consistent and do:
> > 
> > 	wrmsr(msr, val, label);
> > 	wrmsrns(msr, val, label);
> > 
> > rather than be inconsistent and have them have a boolean return for
> > success.
> > 
> > What am I missing?
> 
> I like the idea to use a label, but this would result in the need to use
> macros instead of functions. So this is trading one aspect against another.
> I'm not sure which is the better one here.
> 
> An alternative might be to switch rdmsr() to the interface used by rdmsr_safe(),
> i.e. let all the accessors return a bool for success/failure and use a pointer
> for the MSR value in rdmsr().

Yes, either way around works. Perhaps that is 'better' because mostly we
don't care about the faults since we've checked the 'feature' earlier.

Its just inconvenient to have return in argument crud, but whatever ;-)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH RFC 0/6] x86/msr: Rename MSR access functions

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 2808 bytes --]

On 20.04.26 15:10, Peter Zijlstra wrote:
> On Mon, Apr 20, 2026 at 03:01:31PM +0200, Jürgen Groß wrote:
>> On 20.04.26 14:33, Peter Zijlstra wrote:
> 
>>> That is, we have the following instructions: RDMSR, WRMSR, WRMSRNS
>>> And we should have the exact same functions:
>>>
>>> 	val = rdmsr(msr);
>>> 	wrmsr(msr, val);
>>> 	wrmsrns(msr, val);
>>
>> People tend to copy similar code, maybe using older kernels as the source.
>>
>> So even if wrmsrns() would be fine (and, resulting from that, better), they
>> will more likely end up using wrmsr() instead.
>>
>> Using new function names implying the exact semantics (serializing vs.
>> non-serializing) will make it more likely the correct one is being used.
> 
> You cannot fix stupid. If you want friction, the label thing will
> ensure 'old' code doesn't compile and will need fixing.
> 
> Also, if wrmsrns() really is faster, the performance folks will finger
> 'incorrect' wrmsr() usage sooner or later.

Fine with me. :-)

> 
>>> The only interesting question is what to do with the 'safe' aspect. The
>>> instruction takes a fault, we do the extable, but rdmsr() above already
>>> has a return value, so that can't be used.
>>>
>>> One option is to, like uaccess and the proposed overflow, is to use
>>> labels like:
>>>
>>> 	val = rdmsr(msr, label);
>>>
>>> And then, even though the wrmsr*() functions have the return available,
>>> do we want to be consistent and do:
>>>
>>> 	wrmsr(msr, val, label);
>>> 	wrmsrns(msr, val, label);
>>>
>>> rather than be inconsistent and have them have a boolean return for
>>> success.
>>>
>>> What am I missing?
>>
>> I like the idea to use a label, but this would result in the need to use
>> macros instead of functions. So this is trading one aspect against another.
>> I'm not sure which is the better one here.
>>
>> An alternative might be to switch rdmsr() to the interface used by rdmsr_safe(),
>> i.e. let all the accessors return a bool for success/failure and use a pointer
>> for the MSR value in rdmsr().
> 
> Yes, either way around works. Perhaps that is 'better' because mostly we
> don't care about the faults since we've checked the 'feature' earlier.
> 
> Its just inconvenient to have return in argument crud, but whatever ;-)

Okay, so would you be fine with the following plan?

- drop *_safe() variants, switch the "normal" ones to "safe" semantics.
- make all interfaces return a bool (true == success, false == failure)
- switch all interfaces to use 64-bit values, drop the 32-bit low/high
   split variants
- use always_inline functions for all local MSR accessors

This will result in rdmsr(), wrmsr() and wrmsrns() as the only available
MSR access functions (plus the related *_on_cpu[s]() ones).


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH RFC 0/6] x86/msr: Rename MSR access functions

[Sean Christopherson]

On Mon, Apr 20, 2026, Peter Zijlstra wrote:
> On Mon, Apr 20, 2026 at 03:01:31PM +0200, Jürgen Groß wrote:
> > On 20.04.26 14:33, Peter Zijlstra wrote:
> > > The only interesting question is what to do with the 'safe' aspect. The
> > > instruction takes a fault, we do the extable, but rdmsr() above already
> > > has a return value, so that can't be used.
> > > 
> > > One option is to, like uaccess and the proposed overflow, is to use
> > > labels like:
> > > 
> > > 	val = rdmsr(msr, label);
> > > 
> > > And then, even though the wrmsr*() functions have the return available,
> > > do we want to be consistent and do:
> > > 
> > > 	wrmsr(msr, val, label);
> > > 	wrmsrns(msr, val, label);
> > > 
> > > rather than be inconsistent and have them have a boolean return for
> > > success.
> > > 
> > > What am I missing?
> > 
> > I like the idea to use a label, but this would result in the need to use
> > macros instead of functions. So this is trading one aspect against another.
> > I'm not sure which is the better one here.
> > 
> > An alternative might be to switch rdmsr() to the interface used by rdmsr_safe(),
> > i.e. let all the accessors return a bool for success/failure and use a pointer
> > for the MSR value in rdmsr().
> 
> Yes, either way around works. Perhaps that is 'better' because mostly we
> don't care about the faults since we've checked the 'feature' earlier.
> 
> Its just inconvenient to have return in argument crud, but whatever ;-)

Why not do both?  There are definitely flows where one is obviously more readable
than the the other.  E.g. if the RDMSR is being fed right back into a WRMSR, the
out-param version requires a local variable.  And it can be visually jarring if
the surrounding code is a bunch of "val = xyz" expressions.

On the other hand, the outparam with a 0/-errno return can be very useful too,
e.g. when wrapping the RDMSR in a multi-expression if-statement:

	if (rdmsrq_safe(MSR_IA32_CR_PAT, &host_pat) ||
	    (host_pat & GENMASK(2, 0)) != 6) {

As it avoids having to assign with '=' in the if-statement, and avoids having to
define a label.

It would be trivial to add a wrapper around the label version, the hard part is
just the naming :-)

Re: [PATCH RFC 5/6] x86/events: Switch core parts to use new MSR access functions

[sashiko-bot]

Sashiko AI review found 1 potential issue(s):
- [High] Inconsistent application of `msr_write_noser()` and `msr_write_ser()` APIs, violating the commit's design rules and risking hardware-level race conditions.
--

commit 3fc3352a68324488e42156725645cf2d490b75c7
Author: Juergen Gross <jgross@suse.com>

x86/events: Switch core parts to use new MSR access functions

This commit switches the core x86 events subsystem to use the new msr_*() 
access functions. The commit message establishes a rule to use 
msr_write_noser() when another MSR write follows in the same function, and 
msr_write_ser() for the final MSR write.

> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> index 810ab21ffd991..c15e0d1a66584 100644
> --- a/arch/x86/events/core.c
> +++ b/arch/x86/events/core.c
[ ... ]
> @@ -713,13 +713,13 @@ void x86_pmu_disable_all(void)
>  
>  		if (!test_bit(idx, cpuc->active_mask))
>  			continue;
> -		rdmsrq(x86_pmu_config_addr(idx), val);
> +		val = msr_read(x86_pmu_config_addr(idx));
>  		if (!(val & ARCH_PERFMON_EVENTSEL_ENABLE))
>  			continue;
>  		val &= ~ARCH_PERFMON_EVENTSEL_ENABLE;
> -		wrmsrq(x86_pmu_config_addr(idx), val);
> +		msr_write_noser(x86_pmu_config_addr(idx), val);
>  		if (is_counter_pair(hwc))
> -			wrmsrq(x86_pmu_config_addr(idx + 1), 0);
> +			msr_write_noser(x86_pmu_config_addr(idx + 1), 0);
>  	}
>  }

The commit message mentions using msr_write_ser() for the last MSR write in a
function. Since msr_write_noser() relies on a non-serializing instruction,
could leaving the final write non-serialized here allow the CPU to proceed 
into critical sections before the hardware PMU disable actually completes? 

If the hardware is still actively counting, this might risk triggering 
unexpected PMIs.

[ ... ]
> @@ -1446,14 +1446,14 @@ int x86_perf_event_set_period(struct perf_event *event)
>  	 */
>  	local64_set(&hwc->prev_count, (u64)-left);
>  
> -	wrmsrq(hwc->event_base, (u64)(-left) & x86_pmu.cntval_mask);
> +	msr_write_noser(hwc->event_base, (u64)(-left) & x86_pmu.cntval_mask);
>  
>  	/*
>  	 * Sign extend the Merge event counter's upper 16 bits since
>  	 * we currently declare a 48-bit counter width
>  	 */
>  	if (is_counter_pair(hwc))
> -		wrmsrq(x86_pmu_event_addr(idx + 1), 0xffff);
> +		msr_write_noser(x86_pmu_event_addr(idx + 1), 0xffff);
>  
>  	perf_event_update_userpage(event);
>  
>  	return ret;
>  }

Similar to the above, this function finishes without a serializing MSR write
or a barrier. Should this final write use msr_write_ser() to match the design
rules outlined in the commit description?

[ ... ]
> @@ -2560,9 +2560,9 @@ void perf_clear_dirty_counters(void)
>  			if (!test_bit(i - INTEL_PMC_IDX_FIXED, hybrid(cpuc->pmu, fixed_cntr_mask)))
>  				continue;
>  
> -			wrmsrq(x86_pmu_fixed_ctr_addr(i - INTEL_PMC_IDX_FIXED), 0);
> +			msr_write_noser(x86_pmu_fixed_ctr_addr(i - INTEL_PMC_IDX_FIXED), 0);
>  		} else {
> -			wrmsrq(x86_pmu_event_addr(i), 0);
> +			msr_write_noser(x86_pmu_event_addr(i), 0);
>  		}
>  	}
>  

The final MSR write in this loop also exclusively uses msr_write_noser().
Is it intended to leave the PMU clearing un-serialized upon returning from 
this function?

> diff --git a/arch/x86/events/perf_event.h b/arch/x86/events/perf_event.h
> index fad87d3c8b2ca..cce2e7b67c01f 100644
> --- a/arch/x86/events/perf_event.h
> +++ b/arch/x86/events/perf_event.h
[ ... ]
> @@ -1296,10 +1296,10 @@ static inline void x86_pmu_disable_event(struct perf_event *event)
>  	u64 disable_mask = __this_cpu_read(cpu_hw_events.perf_ctr_virt_mask);
>  	struct hw_perf_event *hwc = &event->hw;
>  
> -	wrmsrq(hwc->config_base, hwc->config & ~disable_mask);
> +	msr_write_ser(hwc->config_base, hwc->config & ~disable_mask);
>  
>  	if (is_counter_pair(hwc))
> -		wrmsrq(x86_pmu_config_addr(hwc->idx + 1), 0);
> +		msr_write_ser(x86_pmu_config_addr(hwc->idx + 1), 0);
>  }

Here the first write uses msr_write_ser() even when a second write might follow 
(when is_counter_pair(hwc) is true). 

Should the first write use msr_write_noser() instead to take advantage of the 
optimization mentioned in the commit message?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260420091634.128787-1-jgross@suse.com?part=5

Re: [PATCH RFC 0/6] x86/msr: Rename MSR access functions

[Sean Christopherson]

On Mon, Apr 20, 2026, Jürgen Groß wrote:
> On 20.04.26 13:41, Peter Zijlstra wrote:
> > On Mon, Apr 20, 2026 at 01:35:12PM +0200, Peter Zijlstra wrote:
> > > On Mon, Apr 20, 2026 at 11:16:28AM +0200, Juergen Gross wrote:
> > > 
> > > > - Use functions instead of macros for accessing MSRs, which will drop
> > > >    modifying variables passed as a parameter.
> > > > 
> > > > - Eliminate multiple accessors doing exactly the same thing (e.g.
> > > >    rdmsrl() and rdmsrq()).
> > > 
> > > So far so sane.
> > > 
> > > > - Instead of having function names based on the underlying instruction
> > > >    mnemonics, have functions of a common name space (msr_*()).
> > > 
> > > Not sure on this one. The whole msr_{read,write}_{safe,noser}() thing is
> > > a royal pain. Also 'noser' reads to me as the noun that goes with 'to
> > > nose' [he that noses (around), like baker: he that bakes].
> > > 
> > > I would much rather we just stick to the mnemonics here. All of this
> > > really is about wrapping single instructions, no need to make it an
> > > unreadable mess.
> > 
> > Also, the _safe suffix should just go away. All MSR accessors should be
> > 'safe'.
> 
> That would be fine by me, but I'd like to have some confirmation this is
> really the route to go.

I don't care what the suffix is called, or if there is even a suffix, but there
needs to be a way for the caller to communicate that it wants to handle faults,
so that the "caller isn't going to handle a fault" case generates a WARN if the
access does #GP.

If we make rdmsr() return a u64, then we can vacate __rdmsr() and use that for
the version with a label.

E.g. something like this if we provide both the out-param and label variants:

static __always_inline u64 rdmsr(u32 msr)
{
	<this version WARNs on fault>
}

#define __rdmsr(msr, label)
({
	u64 __val;

	<this version jumps to @label on fault>
	__val;
})

static __always_inline int rdmsr_p(u32 msr, u64 *val)
{
	<this version zeros *val on fault and returns 0/-errno>
}

Re: [PATCH RFC 0/6] x86/msr: Rename MSR access functions

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 2557 bytes --]

On 20.04.26 15:36, Sean Christopherson wrote:
> On Mon, Apr 20, 2026, Peter Zijlstra wrote:
>> On Mon, Apr 20, 2026 at 03:01:31PM +0200, Jürgen Groß wrote:
>>> On 20.04.26 14:33, Peter Zijlstra wrote:
>>>> The only interesting question is what to do with the 'safe' aspect. The
>>>> instruction takes a fault, we do the extable, but rdmsr() above already
>>>> has a return value, so that can't be used.
>>>>
>>>> One option is to, like uaccess and the proposed overflow, is to use
>>>> labels like:
>>>>
>>>> 	val = rdmsr(msr, label);
>>>>
>>>> And then, even though the wrmsr*() functions have the return available,
>>>> do we want to be consistent and do:
>>>>
>>>> 	wrmsr(msr, val, label);
>>>> 	wrmsrns(msr, val, label);
>>>>
>>>> rather than be inconsistent and have them have a boolean return for
>>>> success.
>>>>
>>>> What am I missing?
>>>
>>> I like the idea to use a label, but this would result in the need to use
>>> macros instead of functions. So this is trading one aspect against another.
>>> I'm not sure which is the better one here.
>>>
>>> An alternative might be to switch rdmsr() to the interface used by rdmsr_safe(),
>>> i.e. let all the accessors return a bool for success/failure and use a pointer
>>> for the MSR value in rdmsr().
>>
>> Yes, either way around works. Perhaps that is 'better' because mostly we
>> don't care about the faults since we've checked the 'feature' earlier.
>>
>> Its just inconvenient to have return in argument crud, but whatever ;-)
> 
> Why not do both?  There are definitely flows where one is obviously more readable
> than the the other.  E.g. if the RDMSR is being fed right back into a WRMSR, the
> out-param version requires a local variable.  And it can be visually jarring if
> the surrounding code is a bunch of "val = xyz" expressions.

I looked through my search results regarding wrmsrq() and rdmsrq() use cases and
I couldn't find such an instance. But maybe I have overlooked it or you have
some patch pending using this pattern?

> On the other hand, the outparam with a 0/-errno return can be very useful too,
> e.g. when wrapping the RDMSR in a multi-expression if-statement:
> 
> 	if (rdmsrq_safe(MSR_IA32_CR_PAT, &host_pat) ||
> 	    (host_pat & GENMASK(2, 0)) != 6) {
> 
> As it avoids having to assign with '=' in the if-statement, and avoids having to
> define a label.
> 
> It would be trivial to add a wrapper around the label version, the hard part is
> just the naming :-)

Indeed, naming is hard.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH RFC 0/6] x86/msr: Rename MSR access functions

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 2497 bytes --]

On 20.04.26 15:44, Sean Christopherson wrote:
> On Mon, Apr 20, 2026, Jürgen Groß wrote:
>> On 20.04.26 13:41, Peter Zijlstra wrote:
>>> On Mon, Apr 20, 2026 at 01:35:12PM +0200, Peter Zijlstra wrote:
>>>> On Mon, Apr 20, 2026 at 11:16:28AM +0200, Juergen Gross wrote:
>>>>
>>>>> - Use functions instead of macros for accessing MSRs, which will drop
>>>>>     modifying variables passed as a parameter.
>>>>>
>>>>> - Eliminate multiple accessors doing exactly the same thing (e.g.
>>>>>     rdmsrl() and rdmsrq()).
>>>>
>>>> So far so sane.
>>>>
>>>>> - Instead of having function names based on the underlying instruction
>>>>>     mnemonics, have functions of a common name space (msr_*()).
>>>>
>>>> Not sure on this one. The whole msr_{read,write}_{safe,noser}() thing is
>>>> a royal pain. Also 'noser' reads to me as the noun that goes with 'to
>>>> nose' [he that noses (around), like baker: he that bakes].
>>>>
>>>> I would much rather we just stick to the mnemonics here. All of this
>>>> really is about wrapping single instructions, no need to make it an
>>>> unreadable mess.
>>>
>>> Also, the _safe suffix should just go away. All MSR accessors should be
>>> 'safe'.
>>
>> That would be fine by me, but I'd like to have some confirmation this is
>> really the route to go.
> 
> I don't care what the suffix is called, or if there is even a suffix, but there
> needs to be a way for the caller to communicate that it wants to handle faults,
> so that the "caller isn't going to handle a fault" case generates a WARN if the
> access does #GP.

This could be handled by just switching parameters:

Let rdmsr() return a u64 and use a pointer parameter for 0/-errno. If that
parameter is NULL we can do the WARN() on error.

> If we make rdmsr() return a u64, then we can vacate __rdmsr() and use that for
> the version with a label.
> 
> E.g. something like this if we provide both the out-param and label variants:
> 
> static __always_inline u64 rdmsr(u32 msr)
> {
> 	<this version WARNs on fault>
> }
> 
> #define __rdmsr(msr, label)
> ({
> 	u64 __val;
> 
> 	<this version jumps to @label on fault>
> 	__val;
> })
> 
> static __always_inline int rdmsr_p(u32 msr, u64 *val)
> {
> 	<this version zeros *val on fault and returns 0/-errno>
> }

Thinking of paravirt support I'm not really sure the label variant is
something I'd like to do. It is possible, but it would certainly not be
more readable. :-)


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH RFC 0/6] x86/msr: Rename MSR access functions

[H. Peter Anvin]

On April 20, 2026 7:04:16 AM PDT, "Jürgen Groß" <jgross@suse.com> wrote:
>On 20.04.26 15:44, Sean Christopherson wrote:
>> On Mon, Apr 20, 2026, Jürgen Groß wrote:
>>> On 20.04.26 13:41, Peter Zijlstra wrote:
>>>> On Mon, Apr 20, 2026 at 01:35:12PM +0200, Peter Zijlstra wrote:
>>>>> On Mon, Apr 20, 2026 at 11:16:28AM +0200, Juergen Gross wrote:
>>>>> 
>>>>>> - Use functions instead of macros for accessing MSRs, which will drop
>>>>>>     modifying variables passed as a parameter.
>>>>>> 
>>>>>> - Eliminate multiple accessors doing exactly the same thing (e.g.
>>>>>>     rdmsrl() and rdmsrq()).
>>>>> 
>>>>> So far so sane.
>>>>> 
>>>>>> - Instead of having function names based on the underlying instruction
>>>>>>     mnemonics, have functions of a common name space (msr_*()).
>>>>> 
>>>>> Not sure on this one. The whole msr_{read,write}_{safe,noser}() thing is
>>>>> a royal pain. Also 'noser' reads to me as the noun that goes with 'to
>>>>> nose' [he that noses (around), like baker: he that bakes].
>>>>> 
>>>>> I would much rather we just stick to the mnemonics here. All of this
>>>>> really is about wrapping single instructions, no need to make it an
>>>>> unreadable mess.
>>>> 
>>>> Also, the _safe suffix should just go away. All MSR accessors should be
>>>> 'safe'.
>>> 
>>> That would be fine by me, but I'd like to have some confirmation this is
>>> really the route to go.
>> 
>> I don't care what the suffix is called, or if there is even a suffix, but there
>> needs to be a way for the caller to communicate that it wants to handle faults,
>> so that the "caller isn't going to handle a fault" case generates a WARN if the
>> access does #GP.
>
>This could be handled by just switching parameters:
>
>Let rdmsr() return a u64 and use a pointer parameter for 0/-errno. If that
>parameter is NULL we can do the WARN() on error.
>
>> If we make rdmsr() return a u64, then we can vacate __rdmsr() and use that for
>> the version with a label.
>> 
>> E.g. something like this if we provide both the out-param and label variants:
>> 
>> static __always_inline u64 rdmsr(u32 msr)
>> {
>> 	<this version WARNs on fault>
>> }
>> 
>> #define __rdmsr(msr, label)
>> ({
>> 	u64 __val;
>> 
>> 	<this version jumps to @label on fault>
>> 	__val;
>> })
>> 
>> static __always_inline int rdmsr_p(u32 msr, u64 *val)
>> {
>> 	<this version zeros *val on fault and returns 0/-errno>
>> }
>
>Thinking of paravirt support I'm not really sure the label variant is
>something I'd like to do. It is possible, but it would certainly not be
>more readable. :-)
>
>
>Juergen

Can we freaking let PV burn in hell please?

Re: [PATCH RFC 0/6] x86/msr: Rename MSR access functions

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 2326 bytes --]

On 20.04.26 15:44, Sean Christopherson wrote:
> On Mon, Apr 20, 2026, Jürgen Groß wrote:
>> On 20.04.26 13:41, Peter Zijlstra wrote:
>>> On Mon, Apr 20, 2026 at 01:35:12PM +0200, Peter Zijlstra wrote:
>>>> On Mon, Apr 20, 2026 at 11:16:28AM +0200, Juergen Gross wrote:
>>>>
>>>>> - Use functions instead of macros for accessing MSRs, which will drop
>>>>>     modifying variables passed as a parameter.
>>>>>
>>>>> - Eliminate multiple accessors doing exactly the same thing (e.g.
>>>>>     rdmsrl() and rdmsrq()).
>>>>
>>>> So far so sane.
>>>>
>>>>> - Instead of having function names based on the underlying instruction
>>>>>     mnemonics, have functions of a common name space (msr_*()).
>>>>
>>>> Not sure on this one. The whole msr_{read,write}_{safe,noser}() thing is
>>>> a royal pain. Also 'noser' reads to me as the noun that goes with 'to
>>>> nose' [he that noses (around), like baker: he that bakes].
>>>>
>>>> I would much rather we just stick to the mnemonics here. All of this
>>>> really is about wrapping single instructions, no need to make it an
>>>> unreadable mess.
>>>
>>> Also, the _safe suffix should just go away. All MSR accessors should be
>>> 'safe'.
>>
>> That would be fine by me, but I'd like to have some confirmation this is
>> really the route to go.
> 
> I don't care what the suffix is called, or if there is even a suffix, but there
> needs to be a way for the caller to communicate that it wants to handle faults,
> so that the "caller isn't going to handle a fault" case generates a WARN if the
> access does #GP.

So this calls for keeping the *_safe() variants (keeping the name or not).

Using the label based error handling for those is a no-go IMHO, as we are
trying to inline all MSR accesses as much as possible in order to avoid
calls and the associated possible retpoline stuff (request by Thomas Gleixner
IIRC). Doing the WARN inline will bloat code more than necessary, especially
as we already have the central WARN today when fixing up the #GP.

In order not having to modify the logic for current use cases it would be
best to keep the main interface of the *_safe() functions (let them return
0/-errno, use a pointer for the value returned by rdmsr_safe()).

We can add label variants on top using macros.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH RFC 0/6] x86/msr: Rename MSR access functions

[Sean Christopherson]

On Wed, Apr 22, 2026, Juergen Gross wrote:
> On 20.04.26 15:44, Sean Christopherson wrote:
> > On Mon, Apr 20, 2026, Jürgen Groß wrote:
> > > On 20.04.26 13:41, Peter Zijlstra wrote:
> > > > On Mon, Apr 20, 2026 at 01:35:12PM +0200, Peter Zijlstra wrote:
> > > > > On Mon, Apr 20, 2026 at 11:16:28AM +0200, Juergen Gross wrote:
> > > > > 
> > > > > > - Use functions instead of macros for accessing MSRs, which will drop
> > > > > >     modifying variables passed as a parameter.
> > > > > > 
> > > > > > - Eliminate multiple accessors doing exactly the same thing (e.g.
> > > > > >     rdmsrl() and rdmsrq()).
> > > > > 
> > > > > So far so sane.
> > > > > 
> > > > > > - Instead of having function names based on the underlying instruction
> > > > > >     mnemonics, have functions of a common name space (msr_*()).
> > > > > 
> > > > > Not sure on this one. The whole msr_{read,write}_{safe,noser}() thing is
> > > > > a royal pain. Also 'noser' reads to me as the noun that goes with 'to
> > > > > nose' [he that noses (around), like baker: he that bakes].
> > > > > 
> > > > > I would much rather we just stick to the mnemonics here. All of this
> > > > > really is about wrapping single instructions, no need to make it an
> > > > > unreadable mess.
> > > > 
> > > > Also, the _safe suffix should just go away. All MSR accessors should be
> > > > 'safe'.
> > > 
> > > That would be fine by me, but I'd like to have some confirmation this is
> > > really the route to go.
> > 
> > I don't care what the suffix is called, or if there is even a suffix, but there
> > needs to be a way for the caller to communicate that it wants to handle faults,
> > so that the "caller isn't going to handle a fault" case generates a WARN if the
> > access does #GP.
> 
> So this calls for keeping the *_safe() variants (keeping the name or not).
> 
> Using the label based error handling for those is a no-go IMHO, as we are
> trying to inline all MSR accesses as much as possible in order to avoid
> calls and the associated possible retpoline stuff (request by Thomas Gleixner
> IIRC). Doing the WARN inline will bloat code more than necessary, especially
> as we already have the central WARN today when fixing up the #GP.

I don't see why those two are mutually exclusive.  Keep the WARN in the exception
fixup code, and let the caller use a label (or not).  Label-based error handling
should allow for the best of both worlds, as the RDMSR/WRMSR can be inlined, with
the (presumably) unlikely error path being shoved into an out-of-line, cold path.

> In order not having to modify the logic for current use cases it would be
> best to keep the main interface of the *_safe() functions (let them return
> 0/-errno, use a pointer for the value returned by rdmsr_safe()).
> 
> We can add label variants on top using macros.

But that's likely going to defeat the value of using labels.  Or at least make
it more difficult to generate optimal code.

Re: [PATCH RFC 0/6] x86/msr: Rename MSR access functions

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 3427 bytes --]

On 22.04.26 21:21, Sean Christopherson wrote:
> On Wed, Apr 22, 2026, Juergen Gross wrote:
>> On 20.04.26 15:44, Sean Christopherson wrote:
>>> On Mon, Apr 20, 2026, Jürgen Groß wrote:
>>>> On 20.04.26 13:41, Peter Zijlstra wrote:
>>>>> On Mon, Apr 20, 2026 at 01:35:12PM +0200, Peter Zijlstra wrote:
>>>>>> On Mon, Apr 20, 2026 at 11:16:28AM +0200, Juergen Gross wrote:
>>>>>>
>>>>>>> - Use functions instead of macros for accessing MSRs, which will drop
>>>>>>>      modifying variables passed as a parameter.
>>>>>>>
>>>>>>> - Eliminate multiple accessors doing exactly the same thing (e.g.
>>>>>>>      rdmsrl() and rdmsrq()).
>>>>>>
>>>>>> So far so sane.
>>>>>>
>>>>>>> - Instead of having function names based on the underlying instruction
>>>>>>>      mnemonics, have functions of a common name space (msr_*()).
>>>>>>
>>>>>> Not sure on this one. The whole msr_{read,write}_{safe,noser}() thing is
>>>>>> a royal pain. Also 'noser' reads to me as the noun that goes with 'to
>>>>>> nose' [he that noses (around), like baker: he that bakes].
>>>>>>
>>>>>> I would much rather we just stick to the mnemonics here. All of this
>>>>>> really is about wrapping single instructions, no need to make it an
>>>>>> unreadable mess.
>>>>>
>>>>> Also, the _safe suffix should just go away. All MSR accessors should be
>>>>> 'safe'.
>>>>
>>>> That would be fine by me, but I'd like to have some confirmation this is
>>>> really the route to go.
>>>
>>> I don't care what the suffix is called, or if there is even a suffix, but there
>>> needs to be a way for the caller to communicate that it wants to handle faults,
>>> so that the "caller isn't going to handle a fault" case generates a WARN if the
>>> access does #GP.
>>
>> So this calls for keeping the *_safe() variants (keeping the name or not).
>>
>> Using the label based error handling for those is a no-go IMHO, as we are
>> trying to inline all MSR accesses as much as possible in order to avoid
>> calls and the associated possible retpoline stuff (request by Thomas Gleixner
>> IIRC). Doing the WARN inline will bloat code more than necessary, especially
>> as we already have the central WARN today when fixing up the #GP.
> 
> I don't see why those two are mutually exclusive.  Keep the WARN in the exception
> fixup code, and let the caller use a label (or not).  Label-based error handling
> should allow for the best of both worlds, as the RDMSR/WRMSR can be inlined, with
> the (presumably) unlikely error path being shoved into an out-of-line, cold path.

Hmm, true.

> 
>> In order not having to modify the logic for current use cases it would be
>> best to keep the main interface of the *_safe() functions (let them return
>> 0/-errno, use a pointer for the value returned by rdmsr_safe()).
>>
>> We can add label variants on top using macros.
> 
> But that's likely going to defeat the value of using labels.  Or at least make
> it more difficult to generate optimal code.

Okay, this would mean to have additional inline functions for the *_safe()
variants building on top of the label based ones with a similar interface as
today. This would be for the use cases where either the error is being ignored,
or the *_safe() call is used as part of a larger if statement.

Would probably work for me, assuming the label based variant with paravirt
doesn't end up too ugly.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]