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

[Xin Li <xin@xxxxxxxxx>]

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