Re: [PATCH v15 29/41] KVM: SEV: Synchronize MSR_IA32_XSS from the GHCB when it's valid

[Sean Christopherson <seanjc@google.com>]

On Thu, Sep 18, 2025, John Allen wrote:
> On Tue, Sep 16, 2025 at 05:55:33PM -0500, John Allen wrote:
> > On Tue, Sep 16, 2025 at 02:38:52PM -0700, Sean Christopherson wrote:
> > > On Tue, Sep 16, 2025, John Allen wrote:
> > > > On Tue, Sep 16, 2025 at 12:53:58PM -0700, Sean Christopherson wrote:
> > > > > On Tue, Sep 16, 2025, John Allen wrote:
> > > > > > On Fri, Sep 12, 2025 at 04:23:07PM -0700, Sean Christopherson wrote:
> > > > > > > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> > > > > > > index 0cd77a87dd84..0cd32df7b9b6 100644
> > > > > > > --- a/arch/x86/kvm/svm/sev.c
> > > > > > > +++ b/arch/x86/kvm/svm/sev.c
> > > > > > > @@ -3306,6 +3306,9 @@ static void sev_es_sync_from_ghcb(struct vcpu_svm *svm)
> > > > > > >  	if (kvm_ghcb_xcr0_is_valid(svm))
> > > > > > >  		__kvm_set_xcr(vcpu, 0, kvm_ghcb_get_xcr0(ghcb));
> > > > > > >  
> > > > > > > +	if (kvm_ghcb_xss_is_valid(svm))
> > > > > > > +		__kvm_emulate_msr_write(vcpu, MSR_IA32_XSS, kvm_ghcb_get_xss(ghcb));
> > > > > > > +
> > > > > > 
> > > > > > It looks like this is the change that caused the selftest regression
> > > > > > with sev-es. It's not yet clear to me what the problem is though.
> > > > > 
> > > > > Do you see any WARNs in the guest kernel log?
> > > > > 
> > > > > The most obvious potential bug is that KVM is missing a CPUID update, e.g. due
> > > > > to dropping an XSS write, consuming stale data, not setting cpuid_dynamic_bits_dirty,
> > > > > etc.  But AFAICT, CPUID.0xD.1.EBX (only thing that consumes the current XSS) is
> > > > > only used by init_xstate_size(), and I would expect the guest kernel's sanity
> > > > > checks in paranoid_xstate_size_valid() to yell if KVM botches CPUID emulation.
> > > > 
> > > > Yes, actually that looks to be the case:
> > > > 
> > > > [    0.463504] ------------[ cut here ]------------
> > > > [    0.464443] XSAVE consistency problem: size 880 != kernel_size 840
> > > > [    0.465445] WARNING: CPU: 0 PID: 0 at arch/x86/kernel/fpu/xstate.c:638 paranoid_xstate_size_valid+0x101/0x140
> > > 
> > > Can you run with the below printk tracing in the host (and optionally tracing in
> > > the guest for its updates)?  Compile tested only.
> > 
> > Interesting, I see "Guest CPUID doesn't have XSAVES" times the number of
> > cpus followed by "XSS already set to val = 0, eliding updates" times the
> > number of cpus. This is with host tracing only. I can try with guest
> > tracing too in the morning.
> 
> Ok, I think I see the problem. The cases above where we were seeing the
> added print statements from kvm_set_msr_common were not situations where
> we were going through the __kvm_emulate_msr_write via
> sev_es_sync_from_ghcb. When we call __kvm_emulate_msr_write from this
> context, we never end up getting to kvm_set_msr_common because we hit
> the following statement at the top of svm_set_msr:
> 
> if (sev_es_prevent_msr_access(vcpu, msr))
> 	return vcpu->kvm->arch.has_protected_state ? -EINVAL : 0;

Gah, I was looking for something like that but couldn't find it, obviously.

> So I'm not sure if this would force using the original method of
> directly setting arch.ia32_xss or if there's some additional handling
> here that we need in this scenario to allow the msr access.

Does this fix things?  If so, I'll slot in a patch to extract setting XSS to
the helper, and then this patch can use that API.  I like the symmetry between
__kvm_set_xcr() and __kvm_set_xss(), and I especially like not doing a generic
end-around on svm_set_msr() by calling kvm_set_msr_common() directly.

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 945f7da60107..ace9f321d2c9 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -2213,6 +2213,7 @@ unsigned long kvm_get_cr8(struct kvm_vcpu *vcpu);
 void kvm_lmsw(struct kvm_vcpu *vcpu, unsigned long msw);
 int __kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr);
 int kvm_emulate_xsetbv(struct kvm_vcpu *vcpu);
+int __kvm_set_xss(struct kvm_vcpu *vcpu, u64 xss);
 
 int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr);
 int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr);
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 94d9acc94c9a..462aebc54135 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -3355,7 +3355,7 @@ static void sev_es_sync_from_ghcb(struct vcpu_svm *svm)
                __kvm_set_xcr(vcpu, 0, kvm_ghcb_get_xcr0(svm));
 
        if (kvm_ghcb_xss_is_valid(svm))
-               __kvm_emulate_msr_write(vcpu, MSR_IA32_XSS, kvm_ghcb_get_xss(svm));
+               __kvm_set_xss(vcpu, kvm_ghcb_get_xss(svm));
 
        /* Copy the GHCB exit information into the VMCB fields */
        exit_code = kvm_ghcb_get_sw_exit_code(svm);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 5bbc187ab428..9b81e92a8de5 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1313,6 +1313,22 @@ int kvm_emulate_xsetbv(struct kvm_vcpu *vcpu)
 }
 EXPORT_SYMBOL_FOR_KVM_INTERNAL(kvm_emulate_xsetbv);
 
+int __kvm_set_xss(struct kvm_vcpu *vcpu, u64 xss)
+{
+       if (!guest_cpuid_has(vcpu, X86_FEATURE_XSAVES))
+               return KVM_MSR_RET_UNSUPPORTED;
+
+       if (xss & ~vcpu->arch.guest_supported_xss)
+               return 1;
+       if (vcpu->arch.ia32_xss == xss)
+               return 0;
+
+       vcpu->arch.ia32_xss = xss;
+       vcpu->arch.cpuid_dynamic_bits_dirty = true;
+       return 0;
+}
+EXPORT_SYMBOL_GPL(__kvm_set_xss);
+
 static bool kvm_is_valid_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)
 {
        return __kvm_is_valid_cr4(vcpu, cr4) &&
@@ -4119,16 +4135,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
                }
                break;
        case MSR_IA32_XSS:
-               if (!guest_cpuid_has(vcpu, X86_FEATURE_XSAVES))
-                       return KVM_MSR_RET_UNSUPPORTED;
-
-               if (data & ~vcpu->arch.guest_supported_xss)
-                       return 1;
-               if (vcpu->arch.ia32_xss == data)
-                       break;
-               vcpu->arch.ia32_xss = data;
-               vcpu->arch.cpuid_dynamic_bits_dirty = true;
-               break;
+               return __kvm_set_xss(vcpu, data);
        case MSR_SMI_COUNT:
                if (!msr_info->host_initiated)
                        return 1;