Thread (154 messages) 154 messages, 12 authors, 2023-03-20

Re: [PATCH v7 38/41] x86/fpu: Add helper for initing features

From: Borislav Petkov <bp@alien8.de>
Date: 2023-03-11 12:54:59
Also in: linux-arch, linux-doc, linux-mm, lkml

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"
quoted hunk ↗ jump to hunk
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 <redacted>
Tested-by: John Allen <john.allen@amd.com>
Tested-by: Kees Cook <redacted>
Acked-by: Mike Rapoport (IBM) <rppt@kernel.org>
Reviewed-by: Kees Cook <redacted>
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 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.
quoted hunk ↗ jump to hunk
+	 * 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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help