Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 as permitted sender) client-ip=23.128.96.31;
Message-ID: <9a7052ca-9c67-45b5-ba23-dbd23e69722c@intel.com>
Date:   Fri, 1 Dec 2023 15:49:36 +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>
From:   "Yang, Weijiang" <weijiang.yang@intel.com>
In-Reply-To: <3c16bb90532fbd2ec95b5a3d42a93bbbf77c4d37.camel@redhat.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?bnp6RzFJcm1hL1BoMitiQWZ3bVVtemgwVDNxK0k1bkdyMnF3aDZNaHpzMktE?=
 =?utf-8?B?K2czSFMvLy9JSXNDU0VPRFRDUmhqQzN4aVhWcmd2dEhSUXowdGJ0U1BpNE1q?=
 =?utf-8?B?MVl0MkJYY0xnZDVxUEJQNUVWejNBRUZvN0RRZlhzTndkSVdxZEgwNGVsZDRs?=
 =?utf-8?B?T1lLcGdKcDh4TFlCR0lkT2NoeVlzeWRRZzZiUndiZDZPWFRxeFBLQW1BL2VI?=
 =?utf-8?B?NERkcEVlU1dwZUc4bkhVYlFXK2FvRWp2TzRXZGpXMVk3aWljNEFIMkJrUGcy?=
 =?utf-8?B?Z3RjQXpsZ0ZNMHR0Z2FERVpwOURKRVJVV0xQMnNXR2sweG1BRHExSnZZZk9I?=
 =?utf-8?B?d09wMWlVclhKaTF6Q0tHMldwRTluLzl2cW5DaEc2NXl3NkFZZUF4aHFWU2ZZ?=
 =?utf-8?B?dVFBRzhpZmVOSndHS1hGbTUzNXZqOC8yOTZpYitaT3l5NkRQemNWRkluOENT?=
 =?utf-8?B?SmluRnJZaDloNnZyRHpQTDAyV3hWaUd4cjVoU3plaVZnUTdBNGl1bHB1Mytt?=
 =?utf-8?B?MSsrTGhZb2grcHVkY2dDQzNyUE9qZlJ3WGJvQ2JaTjMvUUF3M1pRaFZoc2xE?=
 =?utf-8?B?emZaZHVKdnc3UGJkN29jeGNwejd2UzZ0SlFGQmRuUTdMd3RjNEFPaHNReWJz?=
 =?utf-8?B?bzB1eG8yRVBWWGVOUWE4NXlQLzlkS2JNZVg4S0hqNFpKR1lJdGdxMlJPNUhD?=
 =?utf-8?B?K2R2ZVlpdUlIcmNDVGJmY3lqSUZYZ09sVmRRREczOUc4Y0NzZlFtWmk4dktp?=
 =?utf-8?B?elpQT1FJTXFHMkw4U2hsSFUwWWoxUUNnbTdoQzdnMDhwNHNjaklMdUpmRzBz?=
 =?utf-8?B?OC9FZ0MwdWRZVStQRnZmbmVKbzk2cFQ1SFd6UXhtSC9QYnRPNXV4UC9hTE1X?=
 =?utf-8?B?M1V3YjBHbms0V0JpTS9paWxHRHl2OHoxWk5FdEcyZlc5NW9NcFBCWWZPYUh3?=
 =?utf-8?B?UTZIdkttMkw1Znk5OUVQWURUaVhnMk5lR2orVU9zQnNGa05KYzJWQmpXamhO?=
 =?utf-8?B?VU9OVUdVZyszekM4NGFuM0pDNkRiN0krNFlRRllYclZJNmdqZk5zK1luUnJz?=
 =?utf-8?B?WmRIUElSQkRJdzZZSVFkbzR2NHhTQVNUYm5QbGdCVnAxcXZzNkJIaHdrUWxs?=
 =?utf-8?B?ekJFWjBzYnd1WHEvdlBNNzVTUzJON0QxQXBrcW91MSt0cGRJSVZjUnhSY2JG?=
 =?utf-8?B?UzZ4YVFkcVJQaTFmelFvcEZQVjVKZjNGU2ZVVDEzcDBLamdsajFrUWlXTmdk?=
 =?utf-8?B?Wmc0QTFaNGlKUGptMnNDTFdyY05Cb0RNZnBCZjZ6YzRZTUM2MVVVZjN4ano3?=
 =?utf-8?B?NVNWczFNK3ltYUxUK3JUcGZsZUl6bnZVNXVDT2pEb05YU0ZLSUE2Vlh3eDNr?=
 =?utf-8?B?RzZ1VWdyN0xuemVMQlk3d1RleGFxQnRFNmU3djMyR0VVK2dOdlZFL2ZQaTlS?=
 =?utf-8?B?ZnE3Nm84WEh3QVZKUmp5anhCaWFEanYyUmxrVWM3VWZRNmViVGZ6Y25aSjZv?=
 =?utf-8?B?NU9TRWlwZ3VidHkvL1I2NXJyTmRmNG5EeGU5dU1hZHNLcFhXdTFGL2hQQk5i?=
 =?utf-8?B?cnpWY0d5UFhtUS9uMDFnek8zR3dBZWhKRmFRNU9JNm9SQzBjWm5zUTNrTEFh?=
 =?utf-8?B?djhuKzJWRDJUZVpRbmc0Z0Nrb2ozdEZMZDc4S0k5MmFpU0lxcHpnU05tM29z?=
 =?utf-8?B?Q3FxOHNOSnRYcVIzZ3VybWdMSW5YblNCeE5BMnM3cnhVK1hYOFlSbXIzelJQ?=
 =?utf-8?B?K2dkZzRBcnVsNTFnWGNtK1h6TkF3bXAya05tTWFzb2poblZHcmJGR0J5WDFp?=
 =?utf-8?B?WXhVK2JuNnZKbUlPRW5QbExQSG13Y1cxT1M3VGZOSE04KytIUXd2YzdDYTZq?=
 =?utf-8?B?R09YTFJ5VWNoam93QXJXS2ltU1AyR1RnbWhCRWtoS2g2QW1sQlZjVXdLS2RW?=
 =?utf-8?B?b3pWU3liQlJKdWdBOTljTWJLbld6eGZOYmxEQ3lYd1c1TExFbHJXT1FzUUV2?=
 =?utf-8?B?MTA0R0NXWGttWjlnNStibU5lRHZkSHExMVdObW1KdnVwYk1vZ2RKalRib0dK?=
 =?utf-8?B?MGUvTnBycEZpb01MeithUzNBK0FtQ2hhQWFtWXQ4VWpZTHFwZURKRWZEVkxx?=
 =?utf-8?B?NFFQai9pV0IvWHhSaDNXNU5aS1VoeHNrYzBhc0RGRURaMk5MNFFTYUppc2Np?=
 =?utf-8?B?cnc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 163e8412-282f-4bf4-9bf3-08dbf2421581
X-MS-Exchange-CrossTenant-AuthSource: PH0PR11MB4965.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Dec 2023 07:49:45.7435
 (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: s5FkJgZvmLXSJh65QCsBbds3516UwAD8YWVHSyRQv4P+ChtSIyEQZzP8G1JiKlZgnq9FfvWXSTQTNu7mWguNpg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR11MB7999
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 morse.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 (morse.vger.email [0.0.0.0]); Thu, 30 Nov 2023 23:50:19 -0800 (PST)

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.

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