On Fri, Mar 28, 2025, Xin Li (Intel) wrote:
> From: Xin Li <xin3.li@xxxxxxxxx>
>
> Save guest FRED RSP0 in vmx_prepare_switch_to_host() and restore it
> in vmx_prepare_switch_to_guest() because MSR_IA32_FRED_RSP0 is passed
> through to the guest, thus is volatile/unknown.
>
> Note, host FRED RSP0 is restored in arch_exit_to_user_mode_prepare(),
> regardless of whether it is modified in KVM.
>
> Signed-off-by: Xin Li <xin3.li@xxxxxxxxx>
> Signed-off-by: Xin Li (Intel) <xin@xxxxxxxxx>
> Tested-by: Shan Kang <shan.kang@xxxxxxxxx>
> ---
>
> Changes in v3:
> * KVM only needs to save/restore guest FRED RSP0 now as host FRED RSP0
> is restored in arch_exit_to_user_mode_prepare() (Sean Christopherson).
>
> Changes in v2:
> * Don't use guest_cpuid_has() in vmx_prepare_switch_to_{host,guest}(),
> which are called from IRQ-disabled context (Chao Gao).
> * Reset msr_guest_fred_rsp0 in __vmx_vcpu_reset() (Chao Gao).
> ---
> arch/x86/kvm/vmx/vmx.c | 9 +++++++++
> arch/x86/kvm/vmx/vmx.h | 1 +
> 2 files changed, 10 insertions(+)
>
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 236fe5428a74..1fd32aa255f9 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -1349,6 +1349,10 @@ void vmx_prepare_switch_to_guest(struct kvm_vcpu *vcpu)
> }
>
> wrmsrl(MSR_KERNEL_GS_BASE, vmx->msr_guest_kernel_gs_base);
> +
> + if (cpu_feature_enabled(X86_FEATURE_FRED) && guest_cpu_cap_has(vcpu, X86_FEATURE_FRED))
For these paths, I'm leaning towards omitting the cpu_feature_enabled() check.
The guest_cpu_cap_has() check should suffice, this isn't a super hot path, and
the cost of the runtime check will likely be a single, well-predicted uop when
FRED is unsupported (e.g. a fused BT+Jcc).
Unlike the MSR interception toggling, the "extra" work is negligible (and it's
something confusing to check cpu_feature_enabled() instead of kvm_cpu_cap_has()).
> + wrmsrns(MSR_IA32_FRED_RSP0, vmx->msr_guest_fred_rsp0);
> +
> #else
> savesegment(fs, fs_sel);
> savesegment(gs, gs_sel);
> @@ -1393,6 +1397,11 @@ static void vmx_prepare_switch_to_host(struct vcpu_vmx *vmx)
> invalidate_tss_limit();
> #ifdef CONFIG_X86_64
> wrmsrl(MSR_KERNEL_GS_BASE, vmx->msr_host_kernel_gs_base);
> +
> + if (cpu_feature_enabled(X86_FEATURE_FRED) && guest_cpu_cap_has(&vmx->vcpu, X86_FEATURE_FRED)) {
> + vmx->msr_guest_fred_rsp0 = read_msr(MSR_IA32_FRED_RSP0);
> + fred_sync_rsp0(vmx->msr_guest_fred_rsp0);
Can you add a comment here? Passing the guest value to fred_sync_rsp0() surprised
me a bit. The code and naming makes sense after looking at everything, but it's
quite different than the surrounding code, e.g. the MSR_KERNEL_GS_BASE, handling.
Something like this?
/*
* Synchronize the current value in hardware to the kernel's
* local cache. The desired host RSP0 will be set if/when the
* CPU exits to userspace (RSP0 is a per-task value).
*/
> + }
> #endif
> load_fixmap_gdt(raw_smp_processor_id());
> vmx->guest_state_loaded = false;
> diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> index f48791cf6aa6..8e27b7cc700d 100644
> --- a/arch/x86/kvm/vmx/vmx.h
> +++ b/arch/x86/kvm/vmx/vmx.h
> @@ -276,6 +276,7 @@ struct vcpu_vmx {
> #ifdef CONFIG_X86_64
> u64 msr_host_kernel_gs_base;
> u64 msr_guest_kernel_gs_base;
> + u64 msr_guest_fred_rsp0;
> #endif
>
> u64 spec_ctrl;
> --
> 2.48.1
>
