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

["Jürgen Groß" <jgross@suse.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 3310 bytes --]

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

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]