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

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.

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

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?

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

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 ;-)

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 :-)

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

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

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'.

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

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>
}

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

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.

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

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

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?