Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
Message-ID: <c1ee3d5b-5c54-4eea-b3d7-7385d39bae45@intel.com>
Date:   Fri, 8 Dec 2023 23:57:00 +0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v7 04/26] x86/fpu/xstate: Introduce
 XFEATURE_MASK_KERNEL_DYNAMIC xfeature set
Content-Language: en-US
To:     Maxim Levitsky <mlevitsk@redhat.com>
CC:     <linux-kernel@vger.kernel.org>, <kvm@vger.kernel.org>,
        <dave.hansen@intel.com>, <pbonzini@redhat.com>,
        <seanjc@google.com>, <peterz@infradead.org>, <chao.gao@intel.com>,
        <rick.p.edgecombe@intel.com>, <john.allen@amd.com>
References: <20231124055330.138870-1-weijiang.yang@intel.com>
 <20231124055330.138870-5-weijiang.yang@intel.com>
 <3c16bb90532fbd2ec95b5a3d42a93bbbf77c4d37.camel@redhat.com>
 <9a7052ca-9c67-45b5-ba23-dbd23e69722c@intel.com>
 <5a8c870875acc5c7e72eb3e885c12d6362f45243.camel@redhat.com>
 <888fc0db-a8de-4d42-bcd5-84479c3a8f5e@intel.com>
 <8e7b64f06fe2a8132a8f9f76d673ac663ecfd854.camel@redhat.com>
From:   "Yang, Weijiang" <weijiang.yang@intel.com>
In-Reply-To: <8e7b64f06fe2a8132a8f9f76d673ac663ecfd854.camel@redhat.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?ZWk0OHc4WWNndk1wUUpocUVMd0dzS1VjRTlTSjZUL1FsVzUxMmV6OThRS0x1?=
 =?utf-8?B?ZkhGQWorVlF5WC8wYlRKSGV1Q1F6MFl2OVNPbzlTdzFKTXNldVIrelMybnhG?=
 =?utf-8?B?NldWU2tQVzM1cTlTcTVaaTBhUW4wdmkwN1pxTUJDcDdrcTFxQm5Wc0Rqczd6?=
 =?utf-8?B?cFVmY1NOZkZBNU9HQmNpQXM4bEdCczhiMUU0QWJwdTNEbHpkbEJLdWcwRUlh?=
 =?utf-8?B?RUR1Q25KU0g3bkgyMHFUSUMyK01yTDZNODJUN1NwcUpaMU0vTVFLS1dOalk2?=
 =?utf-8?B?Q1pqWDFtVjVFWWIrV0JIZ1lnZWM5WU5nVXV3aTF1cnFsbG54WnVsU2F6SVJ2?=
 =?utf-8?B?VDlPeDNtcmZhQnltQnRSQW53NEJYM25EejNBbjRjalQ2b204UFdLR3Nhbnp3?=
 =?utf-8?B?YUJGbnEwN1FKTzdPTW1wbG5XRERmZElBWFc1VHE5enAvcFg2eCtNd0V5Tjln?=
 =?utf-8?B?NmVTKy9odmRZbWpmVExtZ3ZrdVhEM0hJVldhK2s3Ym9Kb3NsY3hhZnVZYUI5?=
 =?utf-8?B?NHQySE5TVmo1NnJ3ejBDaWMrZ1MvR1pYOWtrMEJ2eE1zTmFYcndOZ0Q2Nkd5?=
 =?utf-8?B?eWtZbmZudUIzTWxHU29xMDR3ZGdvS2QwZkZkMmhpZE1QOUVjbk5Ea0xwemVE?=
 =?utf-8?B?TU5xVGcreTdKV25BNVM0WE1yemRHTUk1WVJlWm9uY0lrOTBuR2dGaGNleTlB?=
 =?utf-8?B?UmNhaU5FaGRtWVk5SWYrSjAreTMzbEt0dEhpb1hWTkJhaEgzLzdzYlI5SUV0?=
 =?utf-8?B?YURRQU9DQTJENEpGUnk2M0xraTNtWS80TllNQmJtS2dwUG1rOTVHSk45RHp4?=
 =?utf-8?B?dFFtTmxVaUJSdG1yL093SW1qdFI4YkIxRHNHV0Y3ejRGLy90MmVsNkZJanMy?=
 =?utf-8?B?RlJ1dFgxQ0tVWS9pRzh5QnExMG5JcjdiaVpCSTRkQTZReVd1ZkRsSEorNEJ0?=
 =?utf-8?B?Y2FnYkNuYXBDSkRVbDc4cjZTSnNLUkN0T3dUM1JCQy9sendQYXZaSXZZUWJU?=
 =?utf-8?B?cCt0VElUL0VBaUtrU3lFL3pBVmtCS2JjbmN0R2F5Z1czMG81STFRRGZvQjdz?=
 =?utf-8?B?T1BkVG1WU3IvZFZaTFlrSzR5a0d2TUVacTVPVGlwQXZ5M1ZiTzFiZkJnSjUz?=
 =?utf-8?B?V1JwTUdYUmt3bFZVdkZtK0tRTmtEZ2d1ckdEem82TUpzMHBQU2ZNd1hCamF2?=
 =?utf-8?B?ZGV3dndVaXlYSkt5NS95b0cyRDBwanBETjlwSEVNYzAxRU42NEpENFVVZnlR?=
 =?utf-8?B?V1BvUGF3citPTUR5ejJKeCtqblNXUlc5dmFJTnNxK3pUcmUra0ZLZWdkbFBq?=
 =?utf-8?B?M0Z6aFZKYmFmUTFpZmcrLzhzNjhrR2tlZTY1a0pOZVJNTDhWMUMzMnNlZXdE?=
 =?utf-8?B?QkFtTytjbkVQYmhLdDNEYnZkdThmeGxtV0RkbWdiM2VNd2lubkE2NmpNSlJ5?=
 =?utf-8?B?Z1lsK25MT0Z5U1A3N0pha1ZXZlNpSXJKMGZKV2xmRjZqRXBURVd4NWNmWmtG?=
 =?utf-8?B?Q242SEFmZGVZbnMzcWsyZkRFQjNRUXNDbjhsN0JYdWxqYjFjczNaSW4zeVhz?=
 =?utf-8?B?M3ZkUE5vcm9mUWZZSFRCa21OYVhUcDBsaXdRYVdvTzY1eWlNKzRYUDBMQjQ3?=
 =?utf-8?B?d21oTWEwaEFkckJJby9mRTJId2wyOGxSMTN6a3NCWXJKcDEyUXoyblNOZ044?=
 =?utf-8?B?Wm5MNVNTR3cvc2Z5MEgvUGVZZkJoZEo3T0JLeFVncnpPSHpUeUtNL2xkODd0?=
 =?utf-8?B?WXZhUW4zZm1lbkF3UGRVNk5uOGpZcVdZY3ZIaFhJOXdUK05pS29pTEh3c3Bx?=
 =?utf-8?B?ZVVST1ZwS2NsVERoQ0xEQlZ6SjNhRDhIQzREb3FLTkZKQ1I5OUNWaFgxR0JZ?=
 =?utf-8?B?TlpwdnRubEdONlNJVEIzNmNGekxxWml2VFVMbUVpREF4QmtQNHJGMjhVZG0w?=
 =?utf-8?B?citsekE4Q1ZSdU94a2ZSK0lacEtaSHpuNndoU2x0MDN1RWorRnhUY2h1ZFNo?=
 =?utf-8?B?dVY0cW5sdHI2bUNML3hRZVNaSi9YeldKM0JtZmRQOXplY1pFS3VhZkt2TThk?=
 =?utf-8?B?NnlzWmZ0WkxTNFE0NUJ3NTJYSFpoZHdDSGhaSThyQy9la3NzQk5XelltTTVj?=
 =?utf-8?B?cmd1MzNLdU9va1lLWGtQSGlBQks5bGc2M1AzV2RKTjI4VkZoZFY0d1pGYnZW?=
 =?utf-8?B?eWc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: e92b10c2-32a5-4ffd-a7b9-08dbf8065719
X-MS-Exchange-CrossTenant-AuthSource: PH0PR11MB4965.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Dec 2023 15:57:12.6647
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: lQZ8vq4ZAKcI0G2JpVI11mdc77xvPkPOu90/zKz+OPb0dhhr8ZydE9aEogEZ+Bt8bVKgDuITbRLzTHdyglrtIg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB7643
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on pete.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (pete.vger.email [0.0.0.0]); Fri, 08 Dec 2023 07:57:33 -0800 (PST)

On 12/7/2023 12:11 AM, Maxim Levitsky wrote:
> On Wed, 2023-12-06 at 11:00 +0800, Yang, Weijiang wrote:
>> On 12/5/2023 5:55 PM, Maxim Levitsky wrote:
>>> On Fri, 2023-12-01 at 15:49 +0800, Yang, Weijiang wrote:
>>>> On 12/1/2023 1:33 AM, Maxim Levitsky wrote:
>>>>> On Fri, 2023-11-24 at 00:53 -0500, Yang Weijiang wrote:
>>>>>> Define new XFEATURE_MASK_KERNEL_DYNAMIC set including the features can be
>>>>> I am not sure though that this name is correct, but I don't know if I can
>>>>> suggest a better name.
>>>> It's a symmetry of XFEATURE_MASK_USER_DYNAMIC ;-)
>>>>>> optionally enabled by kernel components, i.e., the features are required by
>>>>>> specific kernel components. Currently it's used by KVM to configure guest
>>>>>> dedicated fpstate for calculating the xfeature and fpstate storage size etc.
>>>>>>
>>>>>> The kernel dynamic xfeatures now only contain XFEATURE_CET_KERNEL, which is
>>>>>> supported by host as they're enabled in xsaves/xrstors operating xfeature set
>>>>>> (XCR0 | XSS), but the relevant CPU feature, i.e., supervisor shadow stack, is
>>>>>> not enabled in host kernel so it can be omitted for normal fpstate by default.
>>>>>>
>>>>>> Remove the kernel dynamic feature from fpu_kernel_cfg.default_features so that
>>>>>> the bits in xstate_bv and xcomp_bv are cleared and xsaves/xrstors can be
>>>>>> optimized by HW for normal fpstate.
>>>>>>
>>>>>> Suggested-by: Dave Hansen <dave.hansen@intel.com>
>>>>>> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
>>>>>> ---
>>>>>>     arch/x86/include/asm/fpu/xstate.h | 5 ++++-
>>>>>>     arch/x86/kernel/fpu/xstate.c      | 1 +
>>>>>>     2 files changed, 5 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/arch/x86/include/asm/fpu/xstate.h b/arch/x86/include/asm/fpu/xstate.h
>>>>>> index 3b4a038d3c57..a212d3851429 100644
>>>>>> --- a/arch/x86/include/asm/fpu/xstate.h
>>>>>> +++ b/arch/x86/include/asm/fpu/xstate.h
>>>>>> @@ -46,9 +46,12 @@
>>>>>>     #define XFEATURE_MASK_USER_RESTORE	\
>>>>>>     	(XFEATURE_MASK_USER_SUPPORTED & ~XFEATURE_MASK_PKRU)
>>>>>>     
>>>>>> -/* Features which are dynamically enabled for a process on request */
>>>>>> +/* Features which are dynamically enabled per userspace request */
>>>>>>     #define XFEATURE_MASK_USER_DYNAMIC	XFEATURE_MASK_XTILE_DATA
>>>>>>     
>>>>>> +/* Features which are dynamically enabled per kernel side request */
>>>>> I suggest to explain this a bit better. How about something like that:
>>>>>
>>>>> "Kernel features that are not enabled by default for all processes, but can
>>>>> be still used by some processes, for example to support guest virtualization"
>>>> It looks good to me, will apply it in next version, thanks!
>>>>
>>>>> But feel free to keep it as is or propose something else. IMHO this will
>>>>> be confusing this way or another.
>>>>>
>>>>>
>>>>> Another question: kernel already has a notion of 'independent features'
>>>>> which are currently kernel features that are enabled in IA32_XSS but not present in 'fpu_kernel_cfg.max_features'
>>>>>
>>>>> Currently only 'XFEATURE_LBR' is in this set. These features are saved/restored manually
>>>>> from independent buffer (in case of LBRs, perf code cares for this).
>>>>>
>>>>> Does it make sense to add CET_S to there as well instead of having XFEATURE_MASK_KERNEL_DYNAMIC,
>>>> CET_S here refers to PL{0,1,2}_SSP, right?
>>>>
>>>> IMHO, perf relies on dedicated code to switch LBR MSRs for various reason, e.g., overhead, the feature
>>>> owns dozens of MSRs, remove xfeature bit will offload the burden of common FPU/xsave framework.
>>> This is true, but the question that begs to be asked, is what is the true purpose of the 'independent features' is
>>> from the POV of the kernel FPU framework. IMHO these are features that the framework is not aware of, except
>>> that it enables it in IA32_XSS (and in XCR0 in the future).
>> This is the origin intention for introducing independent features(firstly called dynamic feature, renamed later), from the
>> changelog the major concern is overhead:
> Yes, and to some extent the reason why we want to have CET supervisor state not saved on normal thread's FPU state is also overhead,
> because in theory if the kernel did save it, the MSRs will be in INIT state and thus XSAVES shouldn't have any functional impact,
> even if it saves/restores them for nothing.

CET supervisor state in normal thread's FPU state won't always be in INIT state. Per SDM, it's INIT state is defined only if 3 MSRs are 0,
but if guest is using supervisor CET,  then with vCPU migration between pCPUs, more and more MSRs would hold non-zero contents.
This doesn't impact host kernel behavior because host CET_S is still disabled, but it does impact host XSAVES/XRSTORS behavior.

> In other words, as I said, independent features = features that FPU state doesn't manage, and are just optionally enabled,
> so that a custom code can do a custom xsave(s)/xrstor(s), likely from/to a custom area to save/load these features.
>
> It might make sense to rename independent features again to something like 'unmanaged features' or 'manual features' or something
> like that.
>
>
> Another interesting question that arises here, is once KVM supports arch LBRs, it will likely need to expose the XFEATURE_LBR
> to the guest and will need to context switch it similar to CET_S state, which strengthens the argument that CET_S should
> be in the same group as the 'independent features'.
>
> Depending on the performance impact, XFEATURE_LBR might even need to be dynamically allocated.

This is most likely true for fpu_guest_cfg instead of fpu_kernel_cfg, let me think it over, thanks for bring up this brilliant idea :-)

> For the reference this is the patch series that introduced the arch LBRs to KVM:
> https://www.spinics.net/lists/kvm/msg296507.html
>
>
> Best regards,
> 	Maxim Levitsky
>
>> commit f0dccc9da4c0fda049e99326f85db8c242fd781f
>> Author: Kan Liang <kan.liang@linux.intel.com>
>> Date:   Fri Jul 3 05:49:26 2020 -0700
>>
>>       x86/fpu/xstate: Support dynamic supervisor feature for LBR
>>
>> "However, the kernel should not save/restore the LBR state component at
>> each context switch, like other state components, because of the
>> following unique features of LBR:
>> - The LBR state component only contains valuable information when LBR
>>     is enabled in the perf subsystem, but for most of the time, LBR is
>>     disabled.
>> - The size of the LBR state component is huge. For the current
>>     platform, it's 808 bytes.
>> If the kernel saves/restores the LBR state at each context switch, for
>> most of the time, it is just a waste of space and cycles."
>>
>>> For the guest only features, like CET_S, it is also kind of the same thing (xsave but to guest state area only).
>>> I don't insist that we add CET_S to independent features, but I just gave an idea that maybe that is better
>>> from complexity point of view to add CET there. It's up to you to decide.
>>>
>>> Sean what do you think?
>>>
>>> Best regards,
>>> 	Maxim Levitsky
>>>
>>>
>>>> But CET only has 3 supervisor MSRs and they need to be managed together with user mode MSRs.
>>>> Enabling it in common FPU framework would make the switch/swap much easier without additional
>>>> support code.
>>>>
>>>>>     and maybe rename the
>>>>> 'XFEATURE_MASK_INDEPENDENT' to something like 'XFEATURES_THE_KERNEL_DOESNT_CARE_ABOUT'
>>>>> (terrible name, but you might think of a better name)
>>>>>
>>>>>
>>>>>> +#define XFEATURE_MASK_KERNEL_DYNAMIC	XFEATURE_MASK_CET_KERNEL
>>>>>> +
>>>>>>     /* All currently supported supervisor features */
>>>>>>     #define XFEATURE_MASK_SUPERVISOR_SUPPORTED (XFEATURE_MASK_PASID | \
>>>>>>     					    XFEATURE_MASK_CET_USER | \
>>>>>> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
>>>>>> index b57d909facca..ba4172172afd 100644
>>>>>> --- a/arch/x86/kernel/fpu/xstate.c
>>>>>> +++ b/arch/x86/kernel/fpu/xstate.c
>>>>>> @@ -824,6 +824,7 @@ void __init fpu__init_system_xstate(unsigned int legacy_size)
>>>>>>     	/* Clean out dynamic features from default */
>>>>>>     	fpu_kernel_cfg.default_features = fpu_kernel_cfg.max_features;
>>>>>>     	fpu_kernel_cfg.default_features &= ~XFEATURE_MASK_USER_DYNAMIC;
>>>>>> +	fpu_kernel_cfg.default_features &= ~XFEATURE_MASK_KERNEL_DYNAMIC;
>>>>>>     
>>>>>>     	fpu_user_cfg.default_features = fpu_user_cfg.max_features;
>>>>>>     	fpu_user_cfg.default_features &= ~XFEATURE_MASK_USER_DYNAMIC;
>>>>> Best regards,
>>>>> 	Maxim Levitsky
>>>>>
>>>>>
>>>>>
>>>>>
>>>
>