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

[Jürgen Groß]

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: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature