msr.h | 130 ++++++++++++++++++++++++++++++++----------------------------- paravirt.h | 44 -------------------- 2 files changed, 71 insertions(+), 103 deletions(-)
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.
- paravirt: Always calls the paravirt code, but might end up
ultimately calling a native implementation version
through paravirt ops.
1. https://lore.kernel.org/all/20260319152210.210854-1-aldocontelk@gmail.com/
msr.h | 130 ++++++++++++++++++++++++++++++++-----------------------------
paravirt.h | 44 --------------------
2 files changed, 71 insertions(+), 103 deletions(-)
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.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 3/20/26 16:33, Borislav Petkov wrote: > 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() How about we just stick with the paravirt naming? We're completely used to having stubs that do nothing for paravirt. We're also used to paravirt_foo() ending up *eventually* doing native_foo(). So, why not just short-circuit that at the compiler and do: /* Short-circuit the paravirt infrastructure when it is disabled: */ #define paravirt_read_msr native_read_msr #define paravirt_read_msr_safe native_read_msr_safe #define paravirt_write_msr native_write_msr #define paravirt_write_msr_safe native_write_msr_safe I'll hack something together and see if I hate it.
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
On 3/20/26 23:23, Jürgen Groß wrote: > 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. I'm not opposed to something more thorough than these 8 patches. But, these do fix real problems. Would they make some future effort harder?
On 31.03.26 23:41, Dave Hansen wrote: > On 3/20/26 23:23, Jürgen Groß wrote: >> 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. > I'm not opposed to something more thorough than these 8 patches. But, > these do fix real problems. Would they make some future effort harder? I would need to rebase my paravirt MSR series, but this isn't a blocker. Maybe I will need to undo some of your changes and fix the underlying problem differently, but this is fine for me. In the end I believe your series and the one I was proposing above wouldn't interfere a lot. It is the layer between both (my paravirt MSR work) which would be affected most, but this series might need more work anyway. Juergen
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
On 3/20/26 16:33, Borislav Petkov wrote: ... > 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... Yup, totally agree. I was having a heck of a time finding a good name. Lemme stew on it for a weekend and see if anything pops out.
© 2016 - 2026 Red Hat, Inc.