[PATCH v4 4/5] KVM: SVM: Add MSR_IA32_XSS to the GHCB for hypervisor kernel

John Allen posted 5 patches 5 months ago
[PATCH v4 4/5] KVM: SVM: Add MSR_IA32_XSS to the GHCB for hypervisor kernel
Posted by John Allen 5 months ago
When a guest issues a cpuid instruction for Fn0000000D_x0B_{x00,x01}, KVM will
be intercepting the CPUID instruction and will need to access the guest
MSR_IA32_XSS value. For SEV-ES, the XSS value is encrypted and needs to be
included in the GHCB to be visible to the hypervisor.

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")
v3:
  - Move guest kernel GHCB_ACCESSORS definition to new series.
v4:
  - Change logic structure to be more intuitive.
---
 arch/x86/kvm/svm/sev.c | 5 +++++
 arch/x86/kvm/svm/svm.h | 1 +
 2 files changed, 6 insertions(+)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index f4381878a9e5..33c42dd853b3 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -3310,6 +3310,11 @@ static void sev_es_sync_from_ghcb(struct vcpu_svm *svm)
 		vcpu->arch.cpuid_dynamic_bits_dirty = true;
 	}
 
+	if (kvm_ghcb_xss_is_valid(svm)) {
+		vcpu->arch.ia32_xss = ghcb_get_xss(ghcb);
+		vcpu->arch.cpuid_dynamic_bits_dirty = true;
+	}
+
 	/* Copy the GHCB exit information into the VMCB fields */
 	exit_code = ghcb_get_sw_exit_code(ghcb);
 	control->exit_code = lower_32_bits(exit_code);
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 3c7f208b7935..552c58b050f1 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -935,5 +935,6 @@ DEFINE_KVM_GHCB_ACCESSORS(sw_exit_info_1)
 DEFINE_KVM_GHCB_ACCESSORS(sw_exit_info_2)
 DEFINE_KVM_GHCB_ACCESSORS(sw_scratch)
 DEFINE_KVM_GHCB_ACCESSORS(xcr0)
+DEFINE_KVM_GHCB_ACCESSORS(xss)
 
 #endif
-- 
2.47.3
Re: [PATCH v4 4/5] KVM: SVM: Add MSR_IA32_XSS to the GHCB for hypervisor kernel
Posted by Sean Christopherson 5 months ago
On Mon, Sep 08, 2025, John Allen wrote:
> When a guest issues a cpuid instruction for Fn0000000D_x0B_{x00,x01}, KVM will
> be intercepting the CPUID instruction and will need to access the guest
> MSR_IA32_XSS value. For SEV-ES, the XSS value is encrypted and needs to be
> included in the GHCB to be visible to the hypervisor.
> 
> 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")
> v3:
>   - Move guest kernel GHCB_ACCESSORS definition to new series.

Except that broke _this_ series.

arch/x86/kvm/svm/sev.c: In function ‘sev_es_sync_from_ghcb’:
arch/x86/kvm/svm/sev.c:3293:39: error: implicit declaration of function ‘ghcb_get_xss’;
                                       did you mean ‘ghcb_get_rsi’? [-Wimplicit-function-declaration]
 3293 |                 vcpu->arch.ia32_xss = ghcb_get_xss(ghcb);
      |                                       ^~~~~~~~~~~~
      |                                       ghcb_get_rsi
  AR      drivers/base/built-in.a
  AR      drivers/built-in.a

> v4:
>   - Change logic structure to be more intuitive.
> ---
>  arch/x86/kvm/svm/sev.c | 5 +++++
>  arch/x86/kvm/svm/svm.h | 1 +
>  2 files changed, 6 insertions(+)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index f4381878a9e5..33c42dd853b3 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -3310,6 +3310,11 @@ static void sev_es_sync_from_ghcb(struct vcpu_svm *svm)
>  		vcpu->arch.cpuid_dynamic_bits_dirty = true;
>  	}
>  
> +	if (kvm_ghcb_xss_is_valid(svm)) {
> +		vcpu->arch.ia32_xss = ghcb_get_xss(ghcb);

Honestly, I think the ghcb_get_xxx() helpers do more harm than good.  For set()
and if_valid(), I'm totally on board with a wrapper.  For get(), unless we WARN
on trying to read an invalid field, I just don't see the point.  Ugh, and we
_can't_ WARN, at least not in KVM, because of the whole TOCTOU mess.

Case in point, this and the xcr0 check can elide setting cpuid_dynamic_bits_dirty
if XCR0/XSS isn't actually changing, but then this

	if (kvm_ghcb_xcr0_is_valid(svm) && vcpu->arch.xcr0 != ghcb_get_xcr0(ghcb)) {
		vcpu->arch.xcr0 = ghcb_get_xcr0(ghcb);
		vcpu->arch.cpuid_dynamic_bits_dirty = true;
	}

looks wonky unless the reader knows that ghcb_get_xcr0() is just reading a struct
field, which obviously isn't terribly difficult to figure out, but the macros
make it more than a bit annoying.

Argh, even worse, that check is technically subject to a TOCTOU bug as well.  It
just doesn't matter in practice because the guest can only hose it self, e.g. by
swizzling XCR0/XSS.  But it's still flawed.

And for both XCR0/XSS, KVM lets the guest throw garbage into vcpu->arch.xcr0 and
now vcpu->arch.xss.  Maybe that's not problematic in practice, but I'd rather not
find out the hard way.

Lastly, open coding the write to cpuid_dynamic_bits_dirty and vcpu->arch.xcr0 is
just gross.

So to avoid a rather pointless dependency for CET, which I'm trying my darndest
to land in 6.18, I'm going to put together a separate fixup patch and replace
this patchh to end up with code that does:

	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));
Re: [PATCH v4 4/5] KVM: SVM: Add MSR_IA32_XSS to the GHCB for hypervisor kernel
Posted by John Allen 5 months ago
On Wed, Sep 10, 2025 at 02:24:29PM -0700, Sean Christopherson wrote:
> On Mon, Sep 08, 2025, John Allen wrote:
> > When a guest issues a cpuid instruction for Fn0000000D_x0B_{x00,x01}, KVM will
> > be intercepting the CPUID instruction and will need to access the guest
> > MSR_IA32_XSS value. For SEV-ES, the XSS value is encrypted and needs to be
> > included in the GHCB to be visible to the hypervisor.
> > 
> > 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")
> > v3:
> >   - Move guest kernel GHCB_ACCESSORS definition to new series.
> 
> Except that broke _this_ series.
> 
> arch/x86/kvm/svm/sev.c: In function ‘sev_es_sync_from_ghcb’:
> arch/x86/kvm/svm/sev.c:3293:39: error: implicit declaration of function ‘ghcb_get_xss’;
>                                        did you mean ‘ghcb_get_rsi’? [-Wimplicit-function-declaration]
>  3293 |                 vcpu->arch.ia32_xss = ghcb_get_xss(ghcb);
>       |                                       ^~~~~~~~~~~~
>       |                                       ghcb_get_rsi
>   AR      drivers/base/built-in.a
>   AR      drivers/built-in.a

Apologies, that series should be considered a prerequisite for this
series. I pulled the guest kernel patch into a separate series since it
doesn't depend on the main series and we ideally would want it to be
pulled in ASAP rather than wait on the rest of the series since it
enables linux guests running on non-KVM hypervisors.

Thanks,
John