Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <8c1d2951-1f95-774f-cc8a-35b7d69c521e@intel.com>
Date:   Mon, 3 Oct 2022 12:07:16 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.13.1
Subject: Re: [OPTIONAL/RFC v2 36/39] x86/fpu: Add helper for initing features
Content-Language: en-CA
To:     Rick Edgecombe <rick.p.edgecombe@intel.com>, <x86@kernel.org>,
        "H . Peter Anvin" <hpa@zytor.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, <linux-kernel@vger.kernel.org>,
        <linux-doc@vger.kernel.org>, <linux-mm@kvack.org>,
        <linux-arch@vger.kernel.org>, <linux-api@vger.kernel.org>,
        Arnd Bergmann <arnd@arndb.de>,
        Andy Lutomirski <luto@kernel.org>,
        Balbir Singh <bsingharora@gmail.com>,
        Borislav Petkov <bp@alien8.de>,
        Cyrill Gorcunov <gorcunov@gmail.com>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        "Eugene Syromiatnikov" <esyr@redhat.com>,
        Florian Weimer <fweimer@redhat.com>,
        "H . J . Lu" <hjl.tools@gmail.com>, Jann Horn <jannh@google.com>,
        Jonathan Corbet <corbet@lwn.net>,
        Kees Cook <keescook@chromium.org>,
        Mike Kravetz <mike.kravetz@oracle.com>,
        Nadav Amit <nadav.amit@gmail.com>,
        Oleg Nesterov <oleg@redhat.com>, Pavel Machek <pavel@ucw.cz>,
        Peter Zijlstra <peterz@infradead.org>,
        Randy Dunlap <rdunlap@infradead.org>,
        "Ravi V . Shankar" <ravi.v.shankar@intel.com>,
        Weijiang Yang <weijiang.yang@intel.com>,
        "Kirill A . Shutemov" <kirill.shutemov@linux.intel.com>,
        <joao.moreira@intel.com>, John Allen <john.allen@amd.com>,
        <kcc@google.com>, <eranian@google.com>, <rppt@kernel.org>,
        <jamorris@linux.microsoft.com>, <dethoma@microsoft.com>
References: <20220929222936.14584-1-rick.p.edgecombe@intel.com>
 <20220929222936.14584-37-rick.p.edgecombe@intel.com>
From:   "Chang S. Bae" <chang.seok.bae@intel.com>
In-Reply-To: <20220929222936.14584-37-rick.p.edgecombe@intel.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?aGJ2NGlpMVpERU1yNnBEVHpLSGxVa250blcyMC9kelY3dG1xOHJnRnE1SDdS?=
 =?utf-8?B?WnlqckZ2U3I4MXRPd2YwL3FBVHFxYXo0VlBONm1RTldZSFcwemRKUzJUd3kx?=
 =?utf-8?B?dWIyTmlpOWpYUzZJWjc4YWFoclExTG4zaXhiZEdOZ0dKT2xlbzVzTGszdzY5?=
 =?utf-8?B?clZUZ1Z5cm9raXRyM0hvcmk1TUFIWGJabUFPd2plRTdGUDM0dG45Z2VSZXhx?=
 =?utf-8?B?YkRjUWhsV044NWpnU0dQcHI0bFFncmdoQUUwaXJvZGJSNlllM0tyU0luRzFC?=
 =?utf-8?B?dDJXNVJqaFgyaFd2SmZtNUNjT3ZZMjhkSlhvSllsQTh0MTNMOFlhYm5nTng4?=
 =?utf-8?B?OWRSdXQvSGZTV051U1cxcC9vWW40NmN3Wko4UHhTWTd4UzhkYnA1bWtZWmhV?=
 =?utf-8?B?enBIbXRrcGw3U2thSzZTTjREcUZ2bitXdGtHSWZHckVUV2hpNTRqc2JQb0k2?=
 =?utf-8?B?M29vRmtvK2J2NlRDM1VDRnlCWjZHZmZuRTk5ZE5IRTBEMEpwY0tGNTJTNmQx?=
 =?utf-8?B?UGhFdDNVS01pcFpOYnQ1emU2SmxWdVA0ejYwMndwSHcrZlFZWXVsN1JmdVFx?=
 =?utf-8?B?Snd6a3A2ajFKaGpRV0ZCdXNKcGk5UTVJU201cWNsdmx3dkhiQ1A4YmxXZExx?=
 =?utf-8?B?Zm9VMVd6L1R5NDhPdER4Y21QVHBnZitOWUU3cWNLeGpkelN3NEFKZTdweGxq?=
 =?utf-8?B?aWdTcUcxZlR3dGlDaEtzaURUcDdMNjJHRGphcGVVaDZwQkt6RXhUdDN5WmFB?=
 =?utf-8?B?OW40amJ3OExWUnNobXVDcUdJM2krWFdkMERPY0NLM0VhenhoRUdHSFFwdE0w?=
 =?utf-8?B?Qjh5NEk1ZHFZR2J1cnF0VlpsMTdKYlJhTGVuNHN6OUdNM0ZhWGVOSmdwcE9y?=
 =?utf-8?B?QVBLTWc0eXJTV2tGaHpQMDZLQktoZ3ZzTUJVcjUxRmQ2czBCQjdibXRxcjhy?=
 =?utf-8?B?TDROWTlGbU1rbVZxQVRpQXRwa2JmeUpiTkYzN2F1Y1NSTVo1N1gyZ3hoUFJP?=
 =?utf-8?B?UTdWNEFJZXdId2J3QVZISVZsaE1BaFExLzRUU3dTNit1dDV2QlIwMkh0T214?=
 =?utf-8?B?UWpiOUxlWFlkSFg2UWhObUs5SzZnL2RJRGFWK1hMcTRVNmxaZnhMQTdZSjVu?=
 =?utf-8?B?WkpkV3I2dWdmakV0UFlUQnBVMXB5NEw2REo0bWZxRkVtQzZsaElrSVUxcjh0?=
 =?utf-8?B?NGRHaVZEa2hHWFVEa0haZGFYSDRQdVRGK2ErRW5QODhNR24xTU5IUjdaQVRT?=
 =?utf-8?B?aE0xbnhDVE1reWlOMTllRGhQSWVIOU1iNFRreENodUI3aTJjcmpzd0xVTDE5?=
 =?utf-8?B?S2E2U2ozRVB3dEJyV2dkS3hqV05RQlFvL3FkcjJIYVB1MkR4Rm0zbG9GY2xL?=
 =?utf-8?B?Y1REakY5TmZGT2NmT3ZRSHE4ZjNXeUY4eDJFVXFZam43SzU4c1NYYkxHaUNp?=
 =?utf-8?B?eTFUTVJkODZ3ZlhFWS8yZUhhRExOY2JqUzNMVFo1V0o0T2ZZTTdqZkpVemFp?=
 =?utf-8?B?WGZleDlZQ3VObWxZQW40T0JBbWEvSzZBeC9iQm5VYUYwd2ZsaUc0TUl3S3lw?=
 =?utf-8?B?SWFYUkczYXM2b2JGeDN6eGVRNy9ZWWMrTDRvMFR2ckF4RmVuUW9kc3lWK3cx?=
 =?utf-8?B?ZkpYUjFLWDEwNzkxRlF6ZHp6UCt0cTQ1QUtUN3BRU0xiN2xVUEZYSnQwYzJJ?=
 =?utf-8?B?TEJWU0RSMVd0TTNYcXRhbmtwb0JJT1ZMWmdoc2FzZHNraERudzZSWVJkTmY2?=
 =?utf-8?B?a3YrTVA0UnRVcXRrNHhsSFB4aTNRRlFOWkg3SGFxMlpDTHRnRGhwOC8vQ3l6?=
 =?utf-8?B?eExYbzFsTlY5bFdlc2hmcFFZYkQvbjlBcG9JQ0MybzZpSTBCN2hndGV6NCtO?=
 =?utf-8?B?dEJUU1N5TzR1bjRybDFJSVluOVh3RDNVUjJIRjliR1dZSmRnR0FSbFc2ZnlW?=
 =?utf-8?B?ZzhNL1ZlbzNJY3pRdDZOckdBTjV2dXBYaHhvd1ltU3d5aEZ3cGVTUW1VOHB1?=
 =?utf-8?B?QjdkUVo1RnFtbDNjZkpQV3c1WmxEWmV2RGFjT3NXVnkyZ2NHSG1BLzhYU0pn?=
 =?utf-8?B?T0ViaWJ0NUNSK2tHS21ZY2tabXpaWUJPWmE3akZwNlRaTDZ6QnB1a0QvUVhO?=
 =?utf-8?B?djRBc1Bmd0JuSSthOXR6UExNeVE4QWp5S3N0bDhGbG5IdnBpUUxYV1d0Sjlu?=
 =?utf-8?B?amc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 4810845f-174f-4dbe-e2b0-08daa5727fef
X-MS-Exchange-CrossTenant-AuthSource: PH0PR11MB4855.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Oct 2022 19:07:21.9503
 (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: SV+5mUgSM2F5B4WEgcBuwYk4LeL5rwZVlcERxzKetfr45cOm+rgLujh8Nf4JhidM255LZgxfRd3mUEwJx2k8Iw5Kk1rml3ux+x/cwCWZSpA=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR11MB6991
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-5.9 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_MED,SPF_HELO_PASS,SPF_NONE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 9/29/2022 3:29 PM, Rick Edgecombe wrote:
> If an xfeature is saved in a buffer, the xfeature's bit will be set in
> xsave->header.xfeatures. The CPU may opt to not save the xfeature if it
> is in it's init state. In this case the xfeature buffer address cannot
> be retrieved with get_xsave_addr().
> 
> Future patches will need to handle the case of writing to an xfeature
> that may not be saved. So provide helpers to init an xfeature in an
> xsave buffer.
> 
> This could of course be done directly by reaching into the xsave buffer,
> however this would not be robust against future changes to optimize the
> xsave buffer by compacting it. In that case the xsave buffer would need
> to be re-arranged as well. So the logic properly belongs encapsulated
> in a helper where the logic can be unified.
> 
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> 
> ---
> 
> v2:
>   - New patch
> 
>   arch/x86/kernel/fpu/xstate.c | 58 +++++++++++++++++++++++++++++-------
>   arch/x86/kernel/fpu/xstate.h |  6 ++++
>   2 files changed, 53 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index 9258fc1169cc..82cee1f2f0c8 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -942,6 +942,24 @@ static void *__raw_xsave_addr(struct xregs_state *xsave, int xfeature_nr)
>   	return (void *)xsave + xfeature_get_offset(xcomp_bv, xfeature_nr);
>   }
>   
> +static int xsave_buffer_access_checks(int xfeature_nr)
> +{
> +	/*
> +	 * Do we even *have* xsave state?
> +	 */
> +	if (!boot_cpu_has(X86_FEATURE_XSAVE))
> +		return 1;
> +
> +	/*
> +	 * We should not ever be requesting features that we
> +	 * have not enabled.
> +	 */
> +	if (WARN_ON_ONCE(!xfeature_enabled(xfeature_nr)))
> +		return 1;
> +
> +	return 0;
> +}
> +
>   /*
>    * Given the xsave area and a state inside, this function returns the
>    * address of the state.
> @@ -962,17 +980,7 @@ static void *__raw_xsave_addr(struct xregs_state *xsave, int xfeature_nr)
>    */
>   void *get_xsave_addr(struct xregs_state *xsave, int xfeature_nr)
>   {
> -	/*
> -	 * Do we even *have* xsave state?
> -	 */
> -	if (!boot_cpu_has(X86_FEATURE_XSAVE))
> -		return NULL;
> -
> -	/*
> -	 * We should not ever be requesting features that we
> -	 * have not enabled.
> -	 */
> -	if (WARN_ON_ONCE(!xfeature_enabled(xfeature_nr)))
> +	if (xsave_buffer_access_checks(xfeature_nr))
>   		return NULL;
>   
>   	/*
> @@ -992,6 +1000,34 @@ void *get_xsave_addr(struct xregs_state *xsave, int xfeature_nr)
>   	return __raw_xsave_addr(xsave, xfeature_nr);
>   }
>   
> +/*
> + * Given the xsave area and a state inside, this function
> + * initializes an xfeature in the buffer.

But, this function sets XSTATE_BV bits in the buffer. That does not 
*initialize* the state, right?

> + *
> + * get_xsave_addr() will return NULL if the feature bit is
> + * not present in the header. This function will make it so
> + * the xfeature buffer address is ready to be retrieved by
> + * get_xsave_addr().

Looks like this is used in the next patch to help ptracer().

We have the state copy function -- copy_uabi_to_xstate() that retrieves 
the address using __raw_xsave_addr() instead of get_xsave_addr(), copies 
the state, and then updates XSTATE_BV.

__raw_xsave_addr() also ensures whether the state is in the compacted 
format or not. I think you can use it.

Also, I'm curious about the reason why you want to update XSTATE_BV 
first with this new helper.

Overall, I'm not sure these new helpers are necessary.

Thanks,
Chang