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@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")
> 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));
