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

[Jürgen Groß]

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

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature