Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8;
Message-ID: <888fc0db-a8de-4d42-bcd5-84479c3a8f5e@intel.com>
Date:   Wed, 6 Dec 2023 11:00:20 +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>
From:   "Yang, Weijiang" <weijiang.yang@intel.com>
In-Reply-To: <5a8c870875acc5c7e72eb3e885c12d6362f45243.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?VTRxdm9jbTlqSGZrdk9pTTVCUkdUUVRNenMrcW10d3ppTzRySDJIQk9Ja2Vr?=
 =?utf-8?B?eUxJY0psSkRGbzUxaXZvUjFRTk42S0JYeDlRWmhscXFLMzBqMGFzNkZFYlpY?=
 =?utf-8?B?UnhxZ29GWUtuanRRNFRKZjN1RDFZQXRrMDAzUmU3S29PNjA1cHZWYWVMVXhv?=
 =?utf-8?B?Rk1zQUhlQ2JCdFYwdHIzSHg5TW5RL2ExMDNEY1VOWEtUWGtlN1BvM213L1I4?=
 =?utf-8?B?eHJkK0tmOU9SdWJIaWFQREFGRC96bkFWeHVCbUt2OSt4ckJCOG44UlFSWTJJ?=
 =?utf-8?B?bGpvYmVXcGtHeWtHUHJGR29DVnNrUWtVeE1aemJzeVZrQkpCbFdCQnZPV3JP?=
 =?utf-8?B?UmtlbStpT2FGVVh1WHRXb2ZZQkw5NXUvV2Vhb0tCZHcxYTVTM1RPdkFqYnhp?=
 =?utf-8?B?OVRQWmV6ak1LRjJBRUtmejNYR3ExZjNzcXdPaS9NT0tiMFhuU0xPUUlZb21n?=
 =?utf-8?B?clNwUU9kVWJtK2ZoT2R6MGZUWW5NMkhxNHU5TjY5Z2pjdWZ1UHhmSkNJc3hO?=
 =?utf-8?B?WmxhNWxNY1krM0hDVm9OUG1jVFJ5S1R2L1g2VnpLbXRiQVNUZi95cmUzamJw?=
 =?utf-8?B?TG16aHNib3lJUlNxUEhoVThVbk5EUkVQU0lWZ1Jud3NsekVuTGxSNFlZVi9Q?=
 =?utf-8?B?SmhJQm90TzRXZ0ViZFB1MXpjMHV6YzRjSEFNR09zWXUrNnlud0lnbmlrYjJ3?=
 =?utf-8?B?ZTVDYWlRYmtNbXRPcGJpMEtzbGpuSEFyK1gwejU3T2U4RHFoRnZxL21UUUg5?=
 =?utf-8?B?MHNFbzMzSytsODZ4NXFzdmpVWDY3WUtrNWpESEpobUR0bDVQOEU2dDhaUm9N?=
 =?utf-8?B?TElrVDkyZS8yMDB2WmgrQTUwck12Qm10cS9yM0pqWU04SVJWcUtnZ0hiSjVV?=
 =?utf-8?B?SUwxTm96bC9WejRtcThmWldxeXkvUVB0enZLaEUvL0QvdHpqTXQvNDRvOVlx?=
 =?utf-8?B?UnMzMFFLWFk0T1VMbUtxMUJreXdGaWJlVnRhaUhRbWs5ZDVGZ0N4TXlvRHRZ?=
 =?utf-8?B?bXNYL09WcHhzdUNFZURZWHBxeEtxc1FRSnR3eEdyUnpiOWp0VW9CZE5FaEFp?=
 =?utf-8?B?cWlVclFTQVJHSWozSm1XQ01uMCtUanpQMDhhWDBJRGNwMmlQMWs2RWhkOGx6?=
 =?utf-8?B?WnB6UXk3TldxMmpiUElEeWVURitYUTBNeGFtYllLWS9PUkIxQzJ4YW1hZDBQ?=
 =?utf-8?B?Q3RqNmtsYjRKZDJNdU1JdXkvUnZMVjZLZE5weElvRjV2b2NyZGd3NnFyNTlL?=
 =?utf-8?B?UzY2dU9mTXFSTUFZSk9zWUowdHY3M0JlZkdYY2VuTSszTUNEM1hBOFpWZ1or?=
 =?utf-8?B?LzRFbVNYb3pwYVl2eFhUVDFWSVdGbGlTT0NqZXJzWk83SkRldkJrdlFVQVZj?=
 =?utf-8?B?WDhwWmlnMUNVM3MyUTM3MmFUZ2VRN3oxZGUxY2dFYVlvQWRhR2dWcFpZK2xR?=
 =?utf-8?B?bFNIR2FSUXJMbmtkSHd2LzFSN0ZSNmlpWjNCZFIrM2ZjODRvSGRDaEhGd2Nl?=
 =?utf-8?B?SFRYYW5EYnVEMGl2YjNFdmVoSzh3VUtIcXZGZEpia0pnbG4xS2JLVU1Ca3Mx?=
 =?utf-8?B?WmpxU0Y3bmJQdXZGSk81V3JIYmJuL1pqcHl3RWY1Tnh3THYzbFlpc0NSU3Q2?=
 =?utf-8?B?L28wcWhRdFJrVWJ4ZlJJdjhpeE5QUVVsRlhoZGlsU3Bnbjd6TmVuSW5icHFY?=
 =?utf-8?B?T2NKRzV4cXF1RTZyTWtoWktKOFJYYVRBNGtmQkJLYm5NRVNWN3AzK1RJQWNG?=
 =?utf-8?B?Q3l6M0YwMm1oYzJPZUZZNTJ4RnF0NTdkL0lPOGx0WjFHdHFLVjJ5Q3JXcERu?=
 =?utf-8?B?TDMzK3ArcnY3Z0tUQk4vanVFSG1TNy9SRWhIUG5aTkdzRHBrZGZFdWt0blVN?=
 =?utf-8?B?ZWZkbGx3cTRXa3REeE9yMDVlS2xzRmZ4Lzl1cjlKb2ZpdlBNU2VESUx6QVVD?=
 =?utf-8?B?WkIrQ21SRjhtelF1V1hqYU9zTmtTZTJYL1hOOFpPQjk1UG5oZWM1eFQ2cU1q?=
 =?utf-8?B?aXJwRXBOeFVjdFRKMFIzTy9sTW5COUZrNlI5NHh0M29sOXVEeGE2ZkZpWloy?=
 =?utf-8?B?Z0JHelc4VTB2Y3RXMjVMUUo5QUdSVG4veVdMZDFMUmNYNjA1R2FMSDZSd3V6?=
 =?utf-8?B?eGM2UlMrNzlLeG1ZYUo0MHZQem0zTzNQK04rcmltYU1XbnFWaUZuRm9NSkw3?=
 =?utf-8?B?WUE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 893af4b8-2879-4637-8e94-08dbf607820a
X-MS-Exchange-CrossTenant-AuthSource: PH0PR11MB4965.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Dec 2023 03:00:31.6201
 (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: aNyCfNxUdm58Ngn5dU46iEE/46JPVKW++OSGjqZO0XOyZVVCp/TGoMgAja5o3Hi21K4mA8HI2454n7VA+u3juA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA2PR11MB5131
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 fry.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 (fry.vger.email [0.0.0.0]); Tue, 05 Dec 2023 19:01:05 -0800 (PST)

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:

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
>>>
>>>
>>>
>>>
>
>
>