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

[Jürgen Groß]

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

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature