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

Paolo Bonzini
2023-08-04T21:33:13Z

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