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()

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