Re: [RFC PATCH v2 10/34] x86/msr: Convert __rdmsr() uses to native_rdmsrq() uses

Xin Li
2025-04-23T09:28:45Z

On 4/22/2025 8:09 AM, Sean Christopherson wrote:
> On Tue, Apr 22, 2025, Xin Li (Intel) wrote:
>> __rdmsr() is the lowest level primitive MSR read API, and its direct
>> use is NOT preferred.
>
> Doesn't mean it's wrong.

I wouldn't go so far as to claim that it's wrong :-)

>> Use its wrapper function native_rdmsrq() instead.

The current code exhibits a somewhat haphazard use of MSR APIs, so I
wanted to clarify which API to employ in specific situations with
verbose function naming.

Here is an example that Boris had to fix the use of MSR APIs:

https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f980f9c31a923e9040dee0bc679a5f5b09e61f40

>
> ...
>
>> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
>> index 1547bfacd40f..e73c1d5ba6c4 100644
>> --- a/arch/x86/kvm/vmx/vmx.c
>> +++ b/arch/x86/kvm/vmx/vmx.c
>> @@ -380,7 +380,7 @@ static __always_inline void vmx_disable_fb_clear(struct vcpu_vmx *vmx)
>> if (!vmx->disable_fb_clear)
>> return;
>>
>> - msr = __rdmsr(MSR_IA32_MCU_OPT_CTRL);
>> + msr = native_rdmsrq(MSR_IA32_MCU_OPT_CTRL);
>> msr |= FB_CLEAR_DIS;
>> native_wrmsrq(MSR_IA32_MCU_OPT_CTRL, msr);
>> /* Cache the MSR value to avoid reading it later */
>> @@ -7307,7 +7307,7 @@ void noinstr vmx_spec_ctrl_restore_host(struct vcpu_vmx *vmx,
>> return;
>>
>> if (flags & VMX_RUN_SAVE_SPEC_CTRL)
>> - vmx->spec_ctrl = __rdmsr(MSR_IA32_SPEC_CTRL);
>> + vmx->spec_ctrl = native_rdmsrq(MSR_IA32_SPEC_CTRL);
>
> And what guarantees that native_rdmsrq() won't have tracing? Ugh, a later patch
> renames native_rdmsrq() => native_rdmsrq_no_trace().
>
> I really don't like this. It makes simple and obvious code:
>
> vmx->spec_ctrl = __rdmsr(MSR_IA32_SPEC_CTRL);
>
> so much harder to read:
>
> vmx->spec_ctrl = native_rdmsrq_no_trace(MSR_IA32_SPEC_CTRL);
>
> and does so in a way that is difficult to review, e.g. I have to peek ahead to
> understand that this is even ok.
>
> I strongly prefer that we find a way to not require such verbose APIs, especially
> if KVM ends up using native variants throughout. Xen PV is supposed to be the
> odd one out, yet native code is what suffers. Blech.

Will try to figure out how to name the APIs.

One reason I chose verbose names is that short names are in use and
renaming needs to touch a lot of files (and not fun at all).

Thanks!
Xin