Re: [PATCH v6 11/25] KVM: x86: Report XSS as to-be-saved if there are supported features

[Maxim Levitsky <mlevitsk@redhat.com>]

On Thu, 2023-09-14 at 02:33 -0400, Yang Weijiang wrote:
> From: Sean Christopherson <seanjc@google.com>
> 
> Add MSR_IA32_XSS to list of MSRs reported to userspace if supported_xss
> is non-zero, i.e. KVM supports at least one XSS based feature.


I can't believe that CET is the first supervisor feature that KVM supports...

Ah, now I understand why:

1. XSAVES on AMD can't really be intercepted (other than clearing CR4.OSXSAVE bit, which isn't an option if you want to support AVX for example)
   On VMX however you can intercept XSAVES and even intercept it only when it touches specific bits of state that you don't want the guest to read/write
   freely.

2. Even if it was possible to intercept it, guests use XSAVES on every context switch if available and emulating it might be costly.

3. Emulating XSAVES is also not that easy to do correctly.

However XSAVES touches various MSRs, thus letting the guest use it unintercepted means giving access to host MSRs,
which might be wrong security wise in some cases.

Thus I see that KVM hardcodes the IA32_XSS to 0, and that makes the XSAVES work exactly like XSAVE.

And for some features which would benefit from XSAVES state components,
KVM likely won't even be able to do so due to this limitation.
(this is allowed thankfully by the CPUID), forcing the guests to use rdmsr/wrmsr instead.


However it is possible to enable IA32_XSS bits in case the msrs XSAVES reads/writes can't do harm to the host, and then KVM
can context switch these MSRs when the guest exits and that is what is done here with CET.

If you think that a short summary of the above can help the future reader to understand why IA32_XSS support is added only now,
it might be a good idea to add a few lines to the changelog of this patch.

> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
> ---
>  arch/x86/kvm/x86.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index e0b55c043dab..1258d1d6dd52 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1464,6 +1464,7 @@ static const u32 msrs_to_save_base[] = {
>  	MSR_IA32_UMWAIT_CONTROL,
>  
>  	MSR_IA32_XFD, MSR_IA32_XFD_ERR,
> +	MSR_IA32_XSS,
>  };
>  
>  static const u32 msrs_to_save_pmu[] = {
> @@ -7195,6 +7196,10 @@ static void kvm_probe_msr_to_save(u32 msr_index)
>  		if (!(kvm_get_arch_capabilities() & ARCH_CAP_TSX_CTRL_MSR))
>  			return;
>  		break;
> +	case MSR_IA32_XSS:
> +		if (!kvm_caps.supported_xss)
> +			return;
> +		break;
>  	default:
>  		break;
>  	}


Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Best regards,
	Maxim Levitsky