diff mbox series

[17/23] x86/fpu: Eager switch PKRU state

Message ID 20181107194858.9380-18-bigeasy@linutronix.de (mailing list archive)
State New, archived
Headers show
Series [01/23] x86/fpu: Use ULL for shift in xfeature_uncompacted_offset() | expand

Commit Message

Sebastian Andrzej Siewior Nov. 7, 2018, 7:48 p.m. UTC 
From: Rik van Riel <riel@surriel.com>

While most of a task's FPU state is only needed in user space, the
protection keys need to be in place immediately after a context switch.

The reason is that any access to userspace memory while running in
kernel mode also need to abide by the memory permissions specified in
the protection keys.

The "eager switch" is a preparation for loading the FPU state on return
to userland. Instead of decoupling PKRU state from xstate I update PKRU
within xstate on write operations by the kernel.

The read/write_pkru() is moved to another header file so it can easily
accessed from pgtable.h and fpu/internal.h.

Signed-off-by: Rik van Riel <riel@surriel.com>
[bigeasy: save pkru to xstate, no cache]
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 arch/x86/include/asm/fpu/internal.h | 13 +++++++++++--
 arch/x86/include/asm/fpu/xstate.h   |  2 ++
 arch/x86/kernel/fpu/xstate.c        |  2 +-
 3 files changed, 14 insertions(+), 3 deletions(-)

Comments

Paolo Bonzini Nov. 8, 2018, 11:12 a.m. UTC | #1 
On 07/11/2018 20:48, Sebastian Andrzej Siewior wrote:
> index 375226055a413..5b33985d9f475 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -811,7 +811,7 @@ void fpu__resume_cpu(void)
>   *
>   * Note: does not work for compacted buffers.
>   */

The comment is wrong, which was already the case before but it becomes a
bit more important if the function is used outside its module.

However, why not use get_xsave_addr?  I don't see why it is important to
skip the checks, and if it is it probably deserves a comment. "Raw" and
double underscores in the function name is scary...

Paolo

> -static void *__raw_xsave_addr(struct xregs_state *xsave, int xfeature_nr)
> +void *__raw_xsave_addr(struct xregs_state *xsave, int xfeature_nr)
>  {
>  	if (!xfeature_enabled(xfeature_nr)) {
>  		WARN_ON_FPU(1);
Sebastian Andrzej Siewior Nov. 19, 2018, 6:17 p.m. UTC | #2 
On 2018-11-08 12:12:52 [+0100], Paolo Bonzini wrote:
> On 07/11/2018 20:48, Sebastian Andrzej Siewior wrote:
> > index 375226055a413..5b33985d9f475 100644
> > --- a/arch/x86/kernel/fpu/xstate.c
> > +++ b/arch/x86/kernel/fpu/xstate.c
> > @@ -811,7 +811,7 @@ void fpu__resume_cpu(void)
> >   *
> >   * Note: does not work for compacted buffers.
> >   */
> 
> The comment is wrong, which was already the case before but it becomes a
> bit more important if the function is used outside its module.

let me fix it.

> However, why not use get_xsave_addr?  I don't see why it is important to
> skip the checks, and if it is it probably deserves a comment. "Raw" and
> double underscores in the function name is scary...

yeah, it is scary and only those that can face baba jaga may use it.
I though it is a fast path and it would be okay to skip those checks.
However. Let me fix the comment and use the normal function like
everyone else. If it is too slow then we can still short circuit it
later.

> Paolo

Sebastian
diff mbox series

Patch

diff --git a/arch/x86/include/asm/fpu/internal.h b/arch/x86/include/asm/fpu/internal.h
index 260cd4f4ba2bb..ed65e0642a1e1 100644
--- a/arch/x86/include/asm/fpu/internal.h
+++ b/arch/x86/include/asm/fpu/internal.h
@@ -561,8 +561,17 @@  switch_fpu_prepare(struct fpu *old_fpu, int cpu)
  */
 static inline void switch_fpu_finish(struct fpu *new_fpu, int cpu)
 {
-	if (static_cpu_has(X86_FEATURE_FPU))
-		__fpregs_load_activate(new_fpu, cpu);
+	if (!static_cpu_has(X86_FEATURE_FPU))
+		return;
+
+	__fpregs_load_activate(new_fpu, cpu);
+
+	if (cpu_feature_enabled(X86_FEATURE_OSPKE)) {
+		struct pkru_state *pk;
+
+		pk = __raw_xsave_addr(&new_fpu->state.xsave, XFEATURE_PKRU);
+		__write_pkru(pk->pkru);
+	}
 }
 
 /*
diff --git a/arch/x86/include/asm/fpu/xstate.h b/arch/x86/include/asm/fpu/xstate.h
index fbe41f808e5d8..dd138f5eb5c66 100644
--- a/arch/x86/include/asm/fpu/xstate.h
+++ b/arch/x86/include/asm/fpu/xstate.h
@@ -5,6 +5,7 @@ 
 #include <linux/types.h>
 #include <asm/processor.h>
 #include <linux/uaccess.h>
+#include <asm/user.h>
 
 /* Bit 63 of XCR0 is reserved for future expansion */
 #define XFEATURE_MASK_EXTEND	(~(XFEATURE_MASK_FPSSE | (1ULL << 63)))
@@ -47,6 +48,7 @@  extern void __init update_regset_xstate_info(unsigned int size,
 
 void fpu__xstate_clear_all_cpu_caps(void);
 void *get_xsave_addr(struct xregs_state *xsave, int xfeature_nr);
+void *__raw_xsave_addr(struct xregs_state *xsave, int feature_nr);
 const void *get_xsave_field_ptr(int xfeature_nr);
 int using_compacted_format(void);
 int copy_xstate_to_kernel(void *kbuf, struct xregs_state *xsave, unsigned int offset, unsigned int size);
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 375226055a413..5b33985d9f475 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -811,7 +811,7 @@  void fpu__resume_cpu(void)
  *
  * Note: does not work for compacted buffers.
  */
-static void *__raw_xsave_addr(struct xregs_state *xsave, int xfeature_nr)
+void *__raw_xsave_addr(struct xregs_state *xsave, int xfeature_nr)
 {
 	if (!xfeature_enabled(xfeature_nr)) {
 		WARN_ON_FPU(1);