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@xxxxxxxx> 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?