Re: [PATCH 0/8] x86/msr: Consolidate native/paravirt MSR functions

[Juergen Gross <jgross@suse.com>]

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

On 21.03.26 07:23, Jürgen Groß wrote:
> On 21.03.26 00:33, Borislav Petkov wrote:
>> On Fri, Mar 20, 2026 at 12:03:30PM -0700, Dave Hansen wrote:
>>> This is old cruft, but it appears that having two copies of these
>>> MSR functions is enabling warnings to creep in[1].
>>>
>>> I know there's also been some work to pare down the XXL code, but
>>> it's obviously not merged yet and this is a good baby step.
>>>
>>> Create helpers that both paravirt and native can use in common code
>>> and remove the paravirt implementations of the helpers. This reduces
>>> the amount of logic that is duplicated in the paravirt code.
>>>
>>> The other thing I really like about this is that it puts the
>>> raw=>{native,paravirt} switch in one compact place in the code.
>>>
>>> Conceptually:
>>>   -   native: The bare-metal implementation. Might not be usable under
>>>          paravirt XXL.
>>>   -      raw: The lowest-level function that is always usable. Might
>>>          be native or paravirt under the hood.
>>
>> I went through the patchset twice and I kinda get what you're trying to do but
>> the "raw" thing is confusing as hell.
>>
>> To me "raw" means, the lowest level of the functionality - something like
>> __<function_name> with the two underscores. Or three, depending on the
>> indirection levels.
>>
>> And those do *only* *raw* instructions - no more indirections.
>>
>> But then how can "raw" be the lowest level and then still have something else
>> underneath - native_ and paravirt_?
>>
>> I *think* this is only a naming issue and with "raw_" you probably wanna say
>> "native_or_paravirt_" depending on the ifdeffery... but shorter...
>>
>> If so, I wouldn't call it "raw". I'd say
>>
>> xx_read_msr()
>> xx_write_msr()
>>
>> to denote that the "xx" resolves to either of the two types. But a better
>> name. I can't think of a good one now but I know that "raw" isn't it...
>>
>> Hmmm.
>>
> 
> I'd like to suggest to do a major cleanup of the MSR interfaces. We have too
> many of them doing similar things. Some are capable to do tracing, some aren't.
> Some are paravirt capable, some aren't. And the names of those functions don't
> reflect that at all. We even have multiple functions or macros doing exactly
> the same thing, but having different names.
> 
> And in future it will be even more complicated due to the write MSR interfaces
> needing serializing and non-serializing variants.
> 
> My idea would be to have something like:
> 
> msr_read()
> msr_read_notrace()
> msr_write_sync()
> msr_write_nosync()
> msr_write_sync_notrace()
> msr_write_nosync_notrace()

Oh, I missed the "safe" variants.

At the same time maybe the "notrace" variants aren't even needed at this
level?

> 
> All of those should be paravirt capable and they should be the only "official"
> interfaces. They will depend on low-level primitives, but those should be used
> only by the official access functions and maybe in some very special places.
> 
> I think this should be the first step towards a MSR access consolidation, as
> it allows any internal optimizations and changes without further bothering most
> of the users.
> 
> 
> 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 --]