Re: [PATCH v4 07/19] KVM: VMX: Save/restore guest FRED RSP0

[Sean Christopherson]

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
>