From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id BB40CC74A5B for ; Sat, 11 Mar 2023 12:54:56 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id EBC7A6B0071; Sat, 11 Mar 2023 07:54:55 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id E6CC28E0002; Sat, 11 Mar 2023 07:54:55 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id D347E8E0001; Sat, 11 Mar 2023 07:54:55 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id C39EE6B0071 for ; Sat, 11 Mar 2023 07:54:55 -0500 (EST) Received: from smtpin17.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id 8C7D380A2F for ; Sat, 11 Mar 2023 12:54:55 +0000 (UTC) X-FDA: 80556612150.17.E97EB04 Received: from mail.skyhub.de (mail.skyhub.de [5.9.137.197]) by imf01.hostedemail.com (Postfix) with ESMTP id D48E340002 for ; Sat, 11 Mar 2023 12:54:49 +0000 (UTC) Authentication-Results: imf01.hostedemail.com; dkim=temperror ("DNS error when getting key") header.d=alien8.de header.s=dkim header.b=BtmLwAFp; dmarc=pass (policy=none) header.from=alien8.de; spf=pass (imf01.hostedemail.com: domain of bp@alien8.de designates 5.9.137.197 as permitted sender) smtp.mailfrom=bp@alien8.de ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1678539294; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=zybHWV8PE9KVm+sFWMv0UYUKNm9Tpjr7UOQFLCyy5YE=; b=0nqAJPKqMOlIvhMr0RJmNzpnBQfgy0Zz2M0hMxTOOFdvP/OIxhjRVKzpFLOuySuHELLIHU MAXeNIztzFeiV/kA6GJnJkThqCkmdER9ieKApVmmJa2LKNdF080lPiaxaYD32Fx1iQ9AlA zN4AyAfAZOG/7P5RIz9BwX9Kvj1jhWA= ARC-Authentication-Results: i=1; imf01.hostedemail.com; dkim=temperror ("DNS error when getting key") header.d=alien8.de header.s=dkim header.b=BtmLwAFp; dmarc=pass (policy=none) header.from=alien8.de; spf=pass (imf01.hostedemail.com: domain of bp@alien8.de designates 5.9.137.197 as permitted sender) smtp.mailfrom=bp@alien8.de ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1678539294; a=rsa-sha256; cv=none; b=2VbQJdu9NSVeqQZvnAmXA7JFKGnFRi5H8IsvoB2ibWlEWIMyg++Hm0gLXV2RizhLcKgqLn Ho2rYAgobTbqwry+oS0qnwYoawse1kNpKqH38ikgf0IxNXqBgJHEMrjQ8IZH8R4uLtuhZS dVJ0g0HyKUP89o4qOA1tayAd5muLQpE= Received: from zn.tnic (p5de8e9fe.dip0.t-ipconnect.de [93.232.233.254]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.skyhub.de (SuperMail on ZX Spectrum 128k) with ESMTPSA id 838581EC0674; Sat, 11 Mar 2023 13:54:47 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=alien8.de; s=dkim; t=1678539287; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:in-reply-to:in-reply-to: references:references; bh=zybHWV8PE9KVm+sFWMv0UYUKNm9Tpjr7UOQFLCyy5YE=; b=BtmLwAFpkPLa/WArXaNCrHBKIkVBveX2pmmKS+AAxuWYpoSOD0oMespV4auQrNfdSSyBYh dfawy8RSb52e5PTjaba6jbvZistFL2o+WvYrpJeDz+ABZmRsfslwvbdXu5BgDduz7rQylT sC06QXcajrk8qGgnHUsUWsiGXZDuWHg= Date: Sat, 11 Mar 2023 13:54:42 +0100 From: Borislav Petkov To: Rick Edgecombe Cc: x86@kernel.org, "H . Peter Anvin" , Thomas Gleixner , Ingo Molnar , 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 , Andy Lutomirski , Balbir Singh , Cyrill Gorcunov , Dave Hansen , Eugene Syromiatnikov , Florian Weimer , "H . J . Lu" , Jann Horn , Jonathan Corbet , Kees Cook , Mike Kravetz , Nadav Amit , Oleg Nesterov , Pavel Machek , Peter Zijlstra , Randy Dunlap , Weijiang Yang , "Kirill A . Shutemov" , John Allen , kcc@google.com, eranian@google.com, rppt@kernel.org, jamorris@linux.microsoft.com, dethoma@microsoft.com, akpm@linux-foundation.org, Andrew.Cooper3@citrix.com, christina.schimpe@intel.com, david@redhat.com, debug@rivosinc.com Subject: Re: [PATCH v7 38/41] x86/fpu: Add helper for initing features Message-ID: References: <20230227222957.24501-1-rick.p.edgecombe@intel.com> <20230227222957.24501-39-rick.p.edgecombe@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20230227222957.24501-39-rick.p.edgecombe@intel.com> X-Rspam-User: X-Rspamd-Server: rspam02 X-Rspamd-Queue-Id: D48E340002 X-Stat-Signature: bdfcfmxg3ukwx455ha9bya61ck6ig67d X-HE-Tag: 1678539289-55033 X-HE-Meta: U2FsdGVkX18jj2AdmIyDUlx3hBAymCnIMfPsM2HKf6sekG/5CX3xOSbv0kWyz+nC1+bQcOlZJNwLnaN0asAQhqPuREgWCyHczh0M81P/W4LAb5xzSRjvm9o8LUzNDPqODWFlyL7HJSuhq5Fz4AudbFn7PPuOK5EplHun1nkiBKAI8p4D/QwTAwV21WG3DwYYpLRiQARXX4FUJzXvnKcJVZB3d8ELKB/d7kpEvJDfVkCojAqS1sS4QdQjle5r/2p8ha+WsS+J1uJGfyMKvYGZ3U4CbY+cAka4w2zxduP/hNeG2pJ4Z8WvUJGiTCxd469HulJnW/Jid4XZvoKtBFhYx3ReYcSTNoiXgEmY8jvlvvwmrYT2EYuzIO4gTTfV7tnh29M3aOLayhVZje5LwEILnXGwm/oxt7ZXby2JyNGqqHme1K7WtWc8EvN9h+AcIvwkfHQLuY9sIcwWmXFXudx25yy+a/JaoPTUwilc3FSYN48h4mInRRL7FesHYKHULA+OPdth79mtctrDrNW49JMKWjkASBpQ0OdCS3AbfdrId1c3AzZIa5CW6R4pocEhp/4rXAv7Ps/gcl26Cqqsvm6RibMP5DCdanB0EM0KPxZKVjMh1uc50+M6UrUAfZ005dfaSakoeoHn0QSBBbTnI/MrHapEKQaRuoFWlnKZ5t3IQMUw3I8li+YnyafyIw2REWlngbs2jSrG7zHl1OeCo9riClXvIOvlEu5ktLnjv7wmficZJqVnoBqbulwC1aMuPTDn11akp+30tNfRDydvd5y1VaYmpF6+Dxgf8tlcS48oITbTxmIynBMWkYCCCgIDq6kIcwJ58hDy6S6Z3TGVjTnT7pDz8hD1jBY6F1lQWs45RRjEqDvlkXKjP1O74hVG/PmRKH+RAVAMPYLitJ5eJ/XdV3nNdksdn7q7bdR2PM1TrmIXqLGFOTq7e5wrdgaOlqqwnccSyQYiaw5ExLhyuhl wireg0dv c2ieDs/2agHcjmBTlAN36J0YR3faTepjSSdEndAdcRV5XGi2iBdwGDpSfFK10Mi+PQ30o633Vbb9li7AWrZZG9pidw0HPfnRNszo+SEEMgJdib8jGUfAHYIIg+OnDDLCAeihGlYvjBiipB1r0IH+AeCKyvrRlqMNiy/PLGFxGmZccJVTnKeruFc01Zx8pzbtBKxSHdEpdsrCXc2AVVtbI+/EU2GUdPIC5P46L81QMCux3HvJ6g/5wR5+FYrxMygNrBRibgzaLiwgpWyJbnb/7I4Ah4h2Sj6rkOMCLWflO+3Xsy4tRdtq0g/ONCUV1B8YgGwvbzYq+RzK8G2zF2M1emzIoRLUL+hPxP5KvZGRmZTWqINTWs581EkZw59CFyyH0FGg8WtWMfKEoQxI= X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Mon, Feb 27, 2023 at 02:29:54PM -0800, Rick Edgecombe wrote: > Subject: Re: [PATCH v7 38/41] x86/fpu: Add helper for initing features "initializing" > 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 "its" > 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. > > Tested-by: Pengfei Xu > Tested-by: John Allen > Tested-by: Kees Cook > Acked-by: Mike Rapoport (IBM) > Reviewed-by: Kees Cook > Signed-off-by: Rick Edgecombe > > --- > 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 13a80521dd51..3ff80be0a441 100644 > --- a/arch/x86/kernel/fpu/xstate.c > +++ b/arch/x86/kernel/fpu/xstate.c > @@ -934,6 +934,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) Function name needs a verb. > +{ > + /* > + * Do we even *have* xsave state? > + */ That comment is superfluous. > + if (!boot_cpu_has(X86_FEATURE_XSAVE)) check_for_deprecated_apis: WARNING: arch/x86/kernel/fpu/xstate.c:942: Do not use boot_cpu_has() - use cpu_feature_enabled() instead. > + return 1; > + > + /* > + * We should not ever be requesting features that we Please use passive voice in your commit message: no "we" or "I", etc, and describe your changes in imperative mood. > + * 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. > @@ -954,17 +972,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; > > /* > @@ -984,6 +992,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. s/this function initializes/initialize/ > + * > + * 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(). So users of get_xsave_addr() would have to know that they would need to call init_xfeature()? I think the better approach would be: void *get_xsave_addr(struct xregs_state *xsave, int xfeature_nr, bool init) and then that @init controls whether get_xsave_addr() should init the buffer. And then you don't have to have a bunch of small functions here and there and know when to call what but get_xsave_addr() would simply DTRT. Thx. -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette