Re: [PATCH resend 02/15] arm64: add abstractions for FPSIMD state manipulation

2014-05-06 Thread Ard Biesheuvel 
On 6 May 2014 16:43, Catalin Marinas catalin.mari...@arm.com wrote:
 On Thu, May 01, 2014 at 04:49:34PM +0100, Ard Biesheuvel wrote:
 diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
 index 4aef42a04bdc..86ac6a9bc86a 100644
 --- a/arch/arm64/kernel/fpsimd.c
 +++ b/arch/arm64/kernel/fpsimd.c
 @@ -87,6 +87,39 @@ void fpsimd_flush_thread(void)
   preempt_enable();
  }

 +/*
 + * Save the userland FPSIMD state of 'current' to memory
 + */
 +void fpsimd_preserve_current_state(void)
 +{
 + fpsimd_save_state(current-thread.fpsimd_state);
 +}
 +
 +/*
 + * Load the userland FPSIMD state of 'current' from memory
 + */
 +void fpsimd_restore_current_state(void)
 +{
 + fpsimd_load_state(current-thread.fpsimd_state);
 +}
 +
 +/*
 + * Load an updated userland FPSIMD state for 'current' from memory
 + */
 +void fpsimd_update_current_state(struct fpsimd_state *state)
 +{
 + preempt_disable();
 + fpsimd_load_state(state);
 + preempt_enable();
 +}

 Minor - please update the comment above the functions to state that
 preemption needs to be disabled by the caller.


Do you mean in all three cases? And, by implication, that the
preempt_disable()/enable() pair should be moved to the call site for
fpsimd_update_current_state() ?


 +/*
 + * Invalidate live CPU copies of task t's FPSIMD state
 + */
 +void fpsimd_flush_task_state(struct task_struct *t)
 +{
 +}

 I guess this will be added in a subsequent patch. You could either move
 it there or add a comment in the commit log that it is a dummy function
 for now (I prefer the former).


OK

-- 
Ard.
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH resend 02/15] arm64: add abstractions for FPSIMD state manipulation

2014-05-06 Thread Catalin Marinas 
On Tue, May 06, 2014 at 03:48:08PM +0100, Ard Biesheuvel wrote:
 On 6 May 2014 16:43, Catalin Marinas catalin.mari...@arm.com wrote:
  On Thu, May 01, 2014 at 04:49:34PM +0100, Ard Biesheuvel wrote:
  diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
  index 4aef42a04bdc..86ac6a9bc86a 100644
  --- a/arch/arm64/kernel/fpsimd.c
  +++ b/arch/arm64/kernel/fpsimd.c
  @@ -87,6 +87,39 @@ void fpsimd_flush_thread(void)
preempt_enable();
   }
 
  +/*
  + * Save the userland FPSIMD state of 'current' to memory
  + */
  +void fpsimd_preserve_current_state(void)
  +{
  + fpsimd_save_state(current-thread.fpsimd_state);
  +}
  +
  +/*
  + * Load the userland FPSIMD state of 'current' from memory
  + */
  +void fpsimd_restore_current_state(void)
  +{
  + fpsimd_load_state(current-thread.fpsimd_state);
  +}
  +
  +/*
  + * Load an updated userland FPSIMD state for 'current' from memory
  + */
  +void fpsimd_update_current_state(struct fpsimd_state *state)
  +{
  + preempt_disable();
  + fpsimd_load_state(state);
  + preempt_enable();
  +}
 
  Minor - please update the comment above the functions to state that
  preemption needs to be disabled by the caller.
 
 
 Do you mean in all three cases? And, by implication, that the
 preempt_disable()/enable() pair should be moved to the call site for
 fpsimd_update_current_state() ?

No, just the comment for the first two functions updated.

-- 
Catalin
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH resend 02/15] arm64: add abstractions for FPSIMD state manipulation

2014-05-06 Thread Catalin Marinas 
On Tue, May 06, 2014 at 04:12:55PM +0100, Catalin Marinas wrote:
 On Tue, May 06, 2014 at 03:48:08PM +0100, Ard Biesheuvel wrote:
  On 6 May 2014 16:43, Catalin Marinas catalin.mari...@arm.com wrote:
   On Thu, May 01, 2014 at 04:49:34PM +0100, Ard Biesheuvel wrote:
   diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
   index 4aef42a04bdc..86ac6a9bc86a 100644
   --- a/arch/arm64/kernel/fpsimd.c
   +++ b/arch/arm64/kernel/fpsimd.c
   @@ -87,6 +87,39 @@ void fpsimd_flush_thread(void)
 preempt_enable();
}
  
   +/*
   + * Save the userland FPSIMD state of 'current' to memory
   + */
   +void fpsimd_preserve_current_state(void)
   +{
   + fpsimd_save_state(current-thread.fpsimd_state);
   +}
   +
   +/*
   + * Load the userland FPSIMD state of 'current' from memory
   + */
   +void fpsimd_restore_current_state(void)
   +{
   + fpsimd_load_state(current-thread.fpsimd_state);
   +}
   +
   +/*
   + * Load an updated userland FPSIMD state for 'current' from memory
   + */
   +void fpsimd_update_current_state(struct fpsimd_state *state)
   +{
   + preempt_disable();
   + fpsimd_load_state(state);
   + preempt_enable();
   +}
  
   Minor - please update the comment above the functions to state that
   preemption needs to be disabled by the caller.
  
  
  Do you mean in all three cases? And, by implication, that the
  preempt_disable()/enable() pair should be moved to the call site for
  fpsimd_update_current_state() ?
 
 No, just the comment for the first two functions updated.

I noticed in a subsequent patch that you add preempt_disable/enable
already in the first two functions. You could do it here as well to
avoid confusion (and no need to update the comment).

-- 
Catalin
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH resend 02/15] arm64: add abstractions for FPSIMD state manipulation

2014-05-01 Thread Ard Biesheuvel 
There are two tacit assumptions in the FPSIMD handling code that will no longer
hold after the next patch that optimizes away some FPSIMD state restores:
. the FPSIMD registers of this CPU contain the userland FPSIMD state of
  task 'current';
. when switching to a task, its FPSIMD state will always be restored from
  memory.

This patch adds the following functions to abstract away from straight FPSIMD
register file saves and restores:
- fpsimd_preserve_current_state - ensure current's FPSIMD state is saved
- fpsimd_restore_current_state - ensure current's FPSIMD state is loaded
- fpsimd_update_current_state - replace current's FPSIMD state
- fpsimd_flush_task_state - invalidate live copies of a task's FPSIMD state

Where necessary, the ptrace, signal handling and fork code are updated to use
the above wrappers instead of poking into the FPSIMD registers directly.

Signed-off-by: Ard Biesheuvel ard.biesheu...@linaro.org
---
 arch/arm64/include/asm/fpsimd.h |  6 ++
 arch/arm64/kernel/fpsimd.c  | 33 +
 arch/arm64/kernel/process.c |  2 +-
 arch/arm64/kernel/ptrace.c  |  2 ++
 arch/arm64/kernel/signal.c  |  9 +++--
 arch/arm64/kernel/signal32.c|  9 +++--
 6 files changed, 48 insertions(+), 13 deletions(-)

diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h
index c43b4ac13008..9f9b8e438546 100644
--- a/arch/arm64/include/asm/fpsimd.h
+++ b/arch/arm64/include/asm/fpsimd.h
@@ -58,6 +58,12 @@ extern void fpsimd_load_state(struct fpsimd_state *state);
 extern void fpsimd_thread_switch(struct task_struct *next);
 extern void fpsimd_flush_thread(void);
 
+extern void fpsimd_preserve_current_state(void);
+extern void fpsimd_restore_current_state(void);
+extern void fpsimd_update_current_state(struct fpsimd_state *state);
+
+extern void fpsimd_flush_task_state(struct task_struct *target);
+
 #endif
 
 #endif
diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index 4aef42a04bdc..86ac6a9bc86a 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -87,6 +87,39 @@ void fpsimd_flush_thread(void)
preempt_enable();
 }
 
+/*
+ * Save the userland FPSIMD state of 'current' to memory
+ */
+void fpsimd_preserve_current_state(void)
+{
+   fpsimd_save_state(current-thread.fpsimd_state);
+}
+
+/*
+ * Load the userland FPSIMD state of 'current' from memory
+ */
+void fpsimd_restore_current_state(void)
+{
+   fpsimd_load_state(current-thread.fpsimd_state);
+}
+
+/*
+ * Load an updated userland FPSIMD state for 'current' from memory
+ */
+void fpsimd_update_current_state(struct fpsimd_state *state)
+{
+   preempt_disable();
+   fpsimd_load_state(state);
+   preempt_enable();
+}
+
+/*
+ * Invalidate live CPU copies of task t's FPSIMD state
+ */
+void fpsimd_flush_task_state(struct task_struct *t)
+{
+}
+
 #ifdef CONFIG_KERNEL_MODE_NEON
 
 /*
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 6391485f342d..c5693163408c 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -205,7 +205,7 @@ void release_thread(struct task_struct *dead_task)
 
 int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src)
 {
-   fpsimd_save_state(current-thread.fpsimd_state);
+   fpsimd_preserve_current_state();
*dst = *src;
return 0;
 }
diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
index 6a8928bba03c..f8700eca24e7 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -517,6 +517,7 @@ static int fpr_set(struct task_struct *target, const struct 
user_regset *regset,
return ret;
 
target-thread.fpsimd_state.user_fpsimd = newstate;
+   fpsimd_flush_task_state(target);
return ret;
 }
 
@@ -764,6 +765,7 @@ static int compat_vfp_set(struct task_struct *target,
uregs-fpcr = fpscr  VFP_FPSCR_CTRL_MASK;
}
 
+   fpsimd_flush_task_state(target);
return ret;
 }
 
diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
index 890a591f75dd..06448a77ff53 100644
--- a/arch/arm64/kernel/signal.c
+++ b/arch/arm64/kernel/signal.c
@@ -51,7 +51,7 @@ static int preserve_fpsimd_context(struct fpsimd_context 
__user *ctx)
int err;
 
/* dump the hardware registers to the fpsimd_state structure */
-   fpsimd_save_state(fpsimd);
+   fpsimd_preserve_current_state();
 
/* copy the FP and status/control registers */
err = __copy_to_user(ctx-vregs, fpsimd-vregs, sizeof(fpsimd-vregs));
@@ -86,11 +86,8 @@ static int restore_fpsimd_context(struct fpsimd_context 
__user *ctx)
__get_user_error(fpsimd.fpcr, ctx-fpcr, err);
 
/* load the hardware registers from the fpsimd_state structure */
-   if (!err) {
-   preempt_disable();
-   fpsimd_load_state(fpsimd);
-   preempt_enable();
-   }
+   if (!err)
+