Re: [PATCH v2 6/9] KVM: SVM: Add MSR_IA32_XSS to the GHCB for hypervisor kernel

[Tom Lendacky]

On 5/1/24 18:43, Sean Christopherson wrote:
> On Mon, Feb 26, 2024, John Allen wrote:
> > When a guest issues a cpuid instruction for Fn0000000D_x0B
> > (CetUserOffset), KVM will intercept and need to access the guest
> > MSR_IA32_XSS value. For SEV-ES, this is encrypted and needs to be
> > included in the GHCB to be visible to the hypervisor.
>
> Heh, too many pronouns and implicit subjects.  I read this, several times, as:
>
>    When a guest issues a cpuid instruction for Fn0000000D_x0B
>    (CetUserOffset), KVM will intercept MSR_IA32_XSS and need to access the
>    guest MSR_IA32_XSS value.
>
> I think you mean this?
>
>    When a vCPU executes CPUID.0xD.0xB (CetUserOffset), KVM will intercept
>    and emulate CPUID.  To emulate CPUID, KVM needs access to the vCPU's
>    MSR_IA32_XSS value.  For SEV-ES guests, XSS is encrypted, and so the guest
>    must include its XSS value in the GHCB as part of the CPUID request.
>
> Hmm, I suspect that last sentence is wrong though.  Question on that below.
>
> > Signed-off-by: John Allen <john.allen@xxxxxxx>
> > ---
> > v2:
> >    - Omit passing through XSS as this has already been properly
> >      implemented in a26b7cd22546 ("KVM: SEV: Do not intercept
> >      accesses to MSR_IA32_XSS for SEV-ES guests")
> > ---
> >   arch/x86/include/asm/svm.h | 1 +
> >   arch/x86/kvm/svm/sev.c     | 9 +++++++--
> >   arch/x86/kvm/svm/svm.h     | 1 +
> >   3 files changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h
> > index 728c98175b9c..44cd41e2fb68 100644
> > --- a/arch/x86/include/asm/svm.h
> > +++ b/arch/x86/include/asm/svm.h
> > @@ -673,5 +673,6 @@ DEFINE_GHCB_ACCESSORS(sw_exit_info_1)
> >   DEFINE_GHCB_ACCESSORS(sw_exit_info_2)
> >   DEFINE_GHCB_ACCESSORS(sw_scratch)
> >   DEFINE_GHCB_ACCESSORS(xcr0)
> > +DEFINE_GHCB_ACCESSORS(xss)
> >   
> >   #endif
> > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> > index f06f9e51ad9d..c3060d2068eb 100644
> > --- a/arch/x86/kvm/svm/sev.c
> > +++ b/arch/x86/kvm/svm/sev.c
> > @@ -2458,8 +2458,13 @@ static void sev_es_sync_from_ghcb(struct vcpu_svm *svm)
> >   
> >   	svm->vmcb->save.cpl = kvm_ghcb_get_cpl_if_valid(svm, ghcb);
> >   
> > -	if (kvm_ghcb_xcr0_is_valid(svm)) {
> > -		vcpu->arch.xcr0 = ghcb_get_xcr0(ghcb);
> > +	if (kvm_ghcb_xcr0_is_valid(svm) || kvm_ghcb_xss_is_valid(svm)) {
> > +		if (kvm_ghcb_xcr0_is_valid(svm))
> > +			vcpu->arch.xcr0 = ghcb_get_xcr0(ghcb);
> > +
> > +		if (kvm_ghcb_xss_is_valid(svm))
> > +			vcpu->arch.ia32_xss = ghcb_get_xss(ghcb);
> > +
> >   		kvm_update_cpuid_runtime(vcpu);
>
> Pre-existing code, but isn't updating CPUID runtime on every VMGEXIT super wasteful?
> Or is the guest behavior to mark XCR0 and XSS as valid only when changing XCR0/XSS?

It's not really on every VMGEXIT. It's only if those values have been 
supplied in the GHCB will the CPUID runtime be updated. And the Linux 
guest code supplies XCR0 and XSS only on a CPUID VMGEXIT.

Both sides of that can optimized. The guest can be optimized down to 
just supplying the values on CPUID 0xD or even further to only supplying 
the values if they have changed since the last time they were supplied. 
The hypervisor side could be optimized to compare the value and only 
update the CPUID runtime if those values are different.

Thanks,
Tom

> If so, the last sentence of the changelog should be something like:
>
>    MSR_IA32_XSS value.  For SEV-ES guests, XSS is encrypted, and so the guest
>    must notify the host of XSS changes by performing a ??? VMGEXIT and
>    providing its XSS value in the GHCB.