Re: [PATCH v13 05/21] KVM: x86: Load guest FPU state when access XSAVE-managed MSRs

[Chao Gao <chao.gao@xxxxxxxxx>]

On Mon, Aug 25, 2025 at 10:55:20AM +0800, Chao Gao wrote:
>On Sun, Aug 24, 2025 at 06:52:55PM -0700, Xin Li wrote:
>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>> index 6b01c6e9330e..799ac76679c9 100644
>>> --- a/arch/x86/kvm/x86.c
>>> +++ b/arch/x86/kvm/x86.c
>>> @@ -4566,6 +4569,21 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>>>   }
>>>   EXPORT_SYMBOL_GPL(kvm_get_msr_common);
>>> +/*
>>> + *  Returns true if the MSR in question is managed via XSTATE, i.e. is context
>>> + *  switched with the rest of guest FPU state.
>>> + */
>>> +static bool is_xstate_managed_msr(u32 index)
>>> +{
>>> +	switch (index) {
>>> +	case MSR_IA32_U_CET:
>>
>>
>>Why MSR_IA32_S_CET is not included here?
>
>Emm. I didn't think about this.
>
>MSR_IA32_S_CET is read from or written to a dedicated VMCS/B field, so KVM
>doesn't need to load the guest FPU to access MSR_IA32_S_CET. This pairs with
>the kvm_{get,set}_xstate_msr() in kvm_{get,set}_msr_common().
>
>That said, userspace writes can indeed cause an inconsistency between the guest
>FPU and VMCS fields regarding MSR_IA32_S_CET. If migration occurs right after a
>userspace write (without a VM-entry, which would bring them in sync) and
>userspace just restores MSR_IA32_S_CET from the guest FPU, the write before
>migration could be lost.
>
>If that migration issue is a practical problem, I think MSR_IA32_S_CET should
>be included here, and we need to perform a kvm_set_xstate_msr() after writing
>to the VMCS/B.

I prefer to make guest FPU and VMCS always consistent regarding MSR_IA32_S_CET.
The cost is to load guest FPU before userspace accesses the MSR and save guest
FPU after that. It isn't a problem as MSR_IA32_S_CET accesses from userspace
shouldn't be on any hot-path. So, I will add:

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 989008f5307e..92daf63c9487 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -2435,6 +2435,7 @@ int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
		break;
	case MSR_IA32_S_CET:
		vmcs_writel(GUEST_S_CET, data);
+		kvm_set_xstate_msr(vcpu, msr_info);
		break;
	case MSR_KVM_INTERNAL_GUEST_SSP:
		vmcs_writel(GUEST_SSP, data);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index a3770e3d5154..6f64a3355274 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4647,6 +4647,7 @@ EXPORT_SYMBOL_GPL(kvm_get_msr_common);
 static bool is_xstate_managed_msr(u32 index)
 {
	switch (index) {
+	case MSR_IA32_S_CET:
	case MSR_IA32_U_CET:
	case MSR_IA32_PL0_SSP ... MSR_IA32_PL3_SSP:
		return true;