Received-SPF: pass (google.com: domain of linux-kernel+bounces-165857-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Wed, 1 May 2024 16:43:43 -0700
In-Reply-To: <20240226213244.18441-7-john.allen@amd.com>
Precedence: bulk
Mime-Version: 1.0
References: <20240226213244.18441-1-john.allen@amd.com> <20240226213244.18441-7-john.allen@amd.com>
Message-ID: <ZjLTr0n0nwBrZW36@google.com>
Subject: Re: [PATCH v2 6/9] KVM: SVM: Add MSR_IA32_XSS to the GHCB for
 hypervisor kernel
From: Sean Christopherson <seanjc@google.com>
To: John Allen <john.allen@amd.com>
Cc: kvm@vger.kernel.org, weijiang.yang@intel.com, rick.p.edgecombe@intel.com, 
	thomas.lendacky@amd.com, bp@alien8.de, pbonzini@redhat.com, 
	mlevitsk@redhat.com, linux-kernel@vger.kernel.org, x86@kernel.org
Content-Type: text/plain; charset="us-ascii"

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@amd.com>
> ---
> 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?
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.