Re: [PATCH v5 09/19] KVM:x86: Make guest supervisor states as non-XSAVE managed

[Paolo Bonzini <pbonzini@redhat.com>]

On 8/4/23 22:45, Sean Christopherson wrote:
>>>> +void save_cet_supervisor_ssp(struct kvm_vcpu *vcpu)
>>>> +{
>>>> +	if (unlikely(guest_can_use(vcpu, X86_FEATURE_SHSTK))) {
> Drop the unlikely, KVM should not speculate on the guest configuration or underlying
> hardware.

In general unlikely() can still be a good idea if you have a fast path 
vs. a slow path; the extra cost of a branch will be much more visible on 
the fast path.  That said the compiler should already be doing that.

>  the Pros:
>   - Super easy to implement for KVM.
>   - Automatically avoids saving and restoring this data when the vmexit
>     is handled within KVM.
>
>  the Cons:
>   - Unnecessarily restores XFEATURE_CET_KERNEL when switching to
>     non-KVM task's userspace.
>   - Forces allocating space for this state on all tasks, whether or not
>     they use KVM, and with likely zero users today and the near future.
>   - Complicates the FPU optimization thinking by including things that
>     can have no affect on userspace in the FPU

I'm not sure if Linux will ever use XFEATURE_CET_KERNEL.  Linux does not 
use MSR_IA32_PL{1,2}_SSP; MSR_IA32_PL0_SSP probably would be per-CPU but 
it is not used while in ring 0 (except for SETSSBSY) and the restore can 
be delayed until return to userspace.  It is not unlike the SYSCALL MSRs.

So I would treat the bit similar to the dynamic features even if it's 
not guarded by XFD, for example

#define XFEATURE_MASK_USER_DYNAMIC XFEATURE_MASK_XTILE_DATA
#define XFEATURE_MASK_USER_OPTIONAL \
	(XFEATURE_MASK_DYNAMIC | XFEATURE_MASK_CET_KERNEL)

where XFEATURE_MASK_USER_DYNAMIC is used for xfd-related tasks but 
everything else uses XFEATURE_MASK_USER_OPTIONAL.

Then you'd enable the feature by hand when allocating the guest fpstate.

> Especially because another big negative is that not utilizing XSTATE bleeds into
> KVM's ABI.  Userspace has to be told to manually save+restore MSRs instead of just
> letting KVM_{G,S}ET_XSAVE handle the state.  And that will create a bit of a
> snafu if Linux does gain support for SSS.

I don't think this matters, we don't have any MSRs in KVM_GET/SET_XSAVE 
and in fact we can't even add them since the uABI uses the non-compacted 
format.  MSRs should be retrieved and set via KVM_GET/SET_MSR and 
userspace will learn about the index automatically via 
KVM_GET_MSR_INDEX_LIST.

Paolo