Messages in this thread | | | Date | Fri, 4 Aug 2023 23:32:00 +0200 | From | Paolo Bonzini <> | Subject | Re: [PATCH v5 09/19] KVM:x86: Make guest supervisor states as non-XSAVE managed |
| |
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
| |