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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aFrHy09b4x2C95nv@google.com>
Date: Tue, 24 Jun 2025 08:44:11 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: "Xin Li (Intel)" <xin@...or.com>
Cc: pbonzini@...hat.com, kvm@...r.kernel.org, linux-doc@...r.kernel.org, 
	linux-kernel@...r.kernel.org, corbet@....net, tglx@...utronix.de, 
	mingo@...hat.com, bp@...en8.de, dave.hansen@...ux.intel.com, x86@...nel.org, 
	hpa@...or.com, andrew.cooper3@...rix.com, luto@...nel.org, 
	peterz@...radead.org, chao.gao@...el.com, xin3.li@...el.com
Subject: Re: [PATCH v4 07/19] KVM: VMX: Save/restore guest FRED RSP0

On Fri, Mar 28, 2025, Xin Li (Intel) wrote:
> From: Xin Li <xin3.li@...el.com>
> 
> 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@...el.com>
> Signed-off-by: Xin Li (Intel) <xin@...or.com>
> Tested-by: Shan Kang <shan.kang@...el.com>
> ---
> 
> 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
>