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

From: "Yang, Weijiang" <weijiang.yang@intel.com>
To: Paolo Bonzini <pbonzini@redhat.com>,
	Sean Christopherson <seanjc@google.com>
Cc: Chao Gao <chao.gao@intel.com>, <peterz@infradead.org>,
	<john.allen@amd.com>, <kvm@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, <rick.p.edgecombe@intel.com>,
	<binbin.wu@linux.intel.com>
Subject: Re: [PATCH v5 09/19] KVM:x86: Make guest supervisor states as non-XSAVE managed
Date: Wed, 9 Aug 2023 10:51:01 +0800	[thread overview]
Message-ID: <5cba5a47-8863-2ac5-de44-94f4365bbca5@intel.com> (raw)
In-Reply-To: <cf97cfba-941a-5a77-6591-fa84ef6fe8d1@redhat.com>

On 8/5/2023 5:32 AM, Paolo Bonzini wrote:
> 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.
This is my original assumption that compiler can help do some level of optimization with the modifier. Thanks!
>>  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.
Yes, this is another way to optimize the kernel-managed solution, I'll investigate it, thanks!
>> 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
>