2013-10-13 14:20:50

by Jiang Liu

Subject: [RFT PATCH v2 1/4] arm64: fix possible invalid FPSIMD initialization state

From: Jiang Liu <[email protected]>

If context switching happens during executing fpsimd_flush_thread(),
stale value in FPSIMD registers will be saved into current thread's
fpsimd_state by fpsimd_thread_switch(). That may cause invalid
initialization state for the new process, so disable preemption
when executing fpsimd_flush_thread().

Signed-off-by: Jiang Liu <[email protected]>
Cc: Jiang Liu <[email protected]>
---
arch/arm64/kernel/fpsimd.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index 1f2e4d5..bb785d2 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -80,8 +80,10 @@ void fpsimd_thread_switch(struct task_struct *next)

void fpsimd_flush_thread(void)
{
+ preempt_disable();
memset(&current->thread.fpsimd_state, 0, sizeof(struct fpsimd_state));
fpsimd_load_state(&current->thread.fpsimd_state);
+ preempt_enable();
}

#ifdef CONFIG_KERNEL_MODE_NEON
--
1.8.1.2

2013-10-13 14:21:34

by Jiang Liu

Subject: [RFT PATCH v2 2/4] arm64: restore FPSIMD to default state for kernel and signal contexts

From: Jiang Liu <[email protected]>

Restore FPSIMD control and status registers to default values
when creating new FPSIMD contexts for kernel context and reset
FPSIMD status register when creating FPSIMD context for signal
handling, otherwise the stale value in FPSIMD control and status
registers may affect the new kernal or signal handling contexts.

Signed-off-by: Jiang Liu <[email protected]>
Cc: Jiang Liu <[email protected]>
---
arch/arm64/include/asm/fpsimd.h | 16 ++++++++++++++++
arch/arm64/kernel/fpsimd.c | 11 +++++++++--
arch/arm64/kernel/signal.c | 1 +
arch/arm64/kernel/signal32.c | 1 +
4 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h
index c43b4ac..b2dc30f 100644
--- a/arch/arm64/include/asm/fpsimd.h
+++ b/arch/arm64/include/asm/fpsimd.h
@@ -50,8 +50,24 @@ struct fpsimd_state {
#define VFP_STATE_SIZE ((32 * 8) + 4)
#endif

+#define AARCH64_FPCR_DEFAULT_VAL 0
+
struct task_struct;

+static inline void fpsimd_init_hw_state(void)
+{
+ int val = AARCH64_FPCR_DEFAULT_VAL;
+
+ asm ("msr fpcr, %x0\n"
+ "msr fpsr, xzr\n"
+ : : "r"(val));
+}
+
+static inline void fpsimd_clear_fpsr(void)
+{
+ asm ("msr fpsr, xzr\n");
+}
+
extern void fpsimd_save_state(struct fpsimd_state *state);
extern void fpsimd_load_state(struct fpsimd_state *state);

diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index bb785d2..12a25e5 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -80,9 +80,14 @@ void fpsimd_thread_switch(struct task_struct *next)

void fpsimd_flush_thread(void)
{
+ struct fpsimd_state *state = &current->thread.fpsimd_state;
+
preempt_disable();
- memset(&current->thread.fpsimd_state, 0, sizeof(struct fpsimd_state));
- fpsimd_load_state(&current->thread.fpsimd_state);
+ memset(state, 0, sizeof(struct fpsimd_state));
+#if (AARCH64_FPCR_DEFAULT_VAL != 0)
+ state->fpcr = AARCH64_FPCR_DEFAULT_VAL;
+#endif
+ fpsimd_load_state(state);
preempt_enable();
}

@@ -99,6 +104,8 @@ void kernel_neon_begin(void)

if (current->mm)
fpsimd_save_state(&current->thread.fpsimd_state);
+
+ fpsimd_init_hw_state();
}
EXPORT_SYMBOL(kernel_neon_begin);

diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
index 890a591..4ee231e 100644
--- a/arch/arm64/kernel/signal.c
+++ b/arch/arm64/kernel/signal.c
@@ -52,6 +52,7 @@ static int preserve_fpsimd_context(struct fpsimd_context __user *ctx)

/* dump the hardware registers to the fpsimd_state structure */
fpsimd_save_state(fpsimd);
+ fpsimd_clear_fpsr();

/* copy the FP and status/control registers */
err = __copy_to_user(ctx->vregs, fpsimd->vregs, sizeof(fpsimd->vregs));
diff --git a/arch/arm64/kernel/signal32.c b/arch/arm64/kernel/signal32.c
index e393174..cf85c36 100644
--- a/arch/arm64/kernel/signal32.c
+++ b/arch/arm64/kernel/signal32.c
@@ -248,6 +248,7 @@ static int compat_preserve_vfp_context(struct compat_vfp_sigframe __user *frame)
* in AArch32.
*/
fpsimd_save_state(fpsimd);
+ fpsimd_clear_fpsr();

/* Place structure header on the stack */
__put_user_error(magic, &frame->magic, err);
--
1.8.1.2

2013-10-13 14:22:13

by Jiang Liu

Subject: [RFT PATCH v2 3/4] arm64: reduce duplicated code when saving/restoring FPSIMD for signal handling

From: Jiang Liu <[email protected]>

Reduce duplicated code when saving/restoring FPSIMD for signal
handling, it also helps to concentrate all FPSIMD hardware related
code into fpsimd.c.

Signed-off-by: Jiang Liu <[email protected]>
Cc: Jiang Liu <[email protected]>
---
arch/arm64/include/asm/fpsimd.h | 18 ++++--------------
arch/arm64/kernel/fpsimd.c | 35 +++++++++++++++++++++++++++++++++++
arch/arm64/kernel/process.c | 3 +--
arch/arm64/kernel/signal.c | 12 +++---------
arch/arm64/kernel/signal32.c | 10 +++-------
5 files changed, 46 insertions(+), 32 deletions(-)

diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h
index b2dc30f..6f034082f 100644
--- a/arch/arm64/include/asm/fpsimd.h
+++ b/arch/arm64/include/asm/fpsimd.h
@@ -54,25 +54,15 @@ struct fpsimd_state {

struct task_struct;

-static inline void fpsimd_init_hw_state(void)
-{
- int val = AARCH64_FPCR_DEFAULT_VAL;
-
- asm ("msr fpcr, %x0\n"
- "msr fpsr, xzr\n"
- : : "r"(val));
-}
-
-static inline void fpsimd_clear_fpsr(void)
-{
- asm ("msr fpsr, xzr\n");
-}
-
extern void fpsimd_save_state(struct fpsimd_state *state);
extern void fpsimd_load_state(struct fpsimd_state *state);

+extern void fpsimd_dup_task_struct(struct task_struct *dst,
+ struct task_struct *src);
extern void fpsimd_thread_switch(struct task_struct *next);
extern void fpsimd_flush_thread(void);
+extern void fpsimd_prepare_sigctx(struct fpsimd_state *state);
+extern void fpsimd_restore_sigctx(struct fpsimd_state *state);

#endif

diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index 12a25e5..604fe9f 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -33,6 +33,20 @@
#define FPEXC_IXF (1 << 4)
#define FPEXC_IDF (1 << 7)

+static inline void fpsimd_init_hw_state(void)
+{
+ int val = AARCH64_FPCR_DEFAULT_VAL;
+
+ asm ("msr fpcr, %x0\n"
+ "msr fpsr, xzr\n"
+ : : "r"(val));
+}
+
+static inline void fpsimd_clear_fpsr(void)
+{
+ asm ("msr fpsr, xzr\n");
+}
+
/*
* Trapped FP/ASIMD access.
*/
@@ -69,6 +83,12 @@ void do_fpsimd_exc(unsigned int esr, struct pt_regs *regs)
send_sig_info(SIGFPE, &info, current);
}

+void fpsimd_dup_task_struct(struct task_struct *dst, struct task_struct *src)
+{
+ fpsimd_save_state(&src->thread.fpsimd_state);
+ *dst = *src;
+}
+
void fpsimd_thread_switch(struct task_struct *next)
{
/* check if not kernel threads */
@@ -91,6 +111,21 @@ void fpsimd_flush_thread(void)
preempt_enable();
}

+void fpsimd_prepare_sigctx(struct fpsimd_state *state)
+{
+ /* dump the hardware registers to the fpsimd_state structure */
+ fpsimd_save_state(state);
+ fpsimd_clear_fpsr();
+}
+
+void fpsimd_restore_sigctx(struct fpsimd_state *state)
+{
+ /* load the hardware registers from the fpsimd_state structure */
+ preempt_disable();
+ fpsimd_load_state(state);
+ preempt_enable();
+}
+
#ifdef CONFIG_KERNEL_MODE_NEON

/*
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 7ae8a1f..6796080 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -195,8 +195,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);
- *dst = *src;
+ fpsimd_dup_task_struct(dst, src);
return 0;
}

diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
index 4ee231e..1597a33 100644
--- a/arch/arm64/kernel/signal.c
+++ b/arch/arm64/kernel/signal.c
@@ -50,9 +50,7 @@ static int preserve_fpsimd_context(struct fpsimd_context __user *ctx)
struct fpsimd_state *fpsimd = &current->thread.fpsimd_state;
int err;

- /* dump the hardware registers to the fpsimd_state structure */
- fpsimd_save_state(fpsimd);
- fpsimd_clear_fpsr();
+ fpsimd_prepare_sigctx(fpsimd);

/* copy the FP and status/control registers */
err = __copy_to_user(ctx->vregs, fpsimd->vregs, sizeof(fpsimd->vregs));
@@ -86,12 +84,8 @@ static int restore_fpsimd_context(struct fpsimd_context __user *ctx)
__get_user_error(fpsimd.fpsr, &ctx->fpsr, err);
__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)
+ fpsimd_restore_sigctx(&fpsimd);

return err ? -EFAULT : 0;
}
diff --git a/arch/arm64/kernel/signal32.c b/arch/arm64/kernel/signal32.c
index cf85c36..f910d3b 100644
--- a/arch/arm64/kernel/signal32.c
+++ b/arch/arm64/kernel/signal32.c
@@ -247,8 +247,7 @@ static int compat_preserve_vfp_context(struct compat_vfp_sigframe __user *frame)
* Note that this also saves V16-31, which aren't visible
* in AArch32.
*/
- fpsimd_save_state(fpsimd);
- fpsimd_clear_fpsr();
+ fpsimd_prepare_sigctx(fpsimd);

/* Place structure header on the stack */
__put_user_error(magic, &frame->magic, err);
@@ -311,11 +310,8 @@ static int compat_restore_vfp_context(struct compat_vfp_sigframe __user *frame)
* We don't need to touch the exception register, so
* reload the hardware state.
*/
- if (!err) {
- preempt_disable();
- fpsimd_load_state(&fpsimd);
- preempt_enable();
- }
+ if (!err)
+ fpsimd_restore_sigctx(&fpsimd);

return err ? -EFAULT : 0;
}
--
1.8.1.2

2013-10-13 14:22:49

by Jiang Liu

Subject: [RFT PATCH v2 4/4] arm64: reuse FPSIMD hardware context if possible

From: Jiang Liu <[email protected]>

Reuse FPSIMD hardware context if it hasn't been touched by other thread
yet, so we can get rid of unnecessary FPSIMD context restores. This is
especially useful when switching between kernel thread and user thread
because kernel thread usaually doesn't touch FPSIMD registers.

Signed-off-by: Jiang Liu <[email protected]>
Cc: Jiang Liu <[email protected]>
---
arch/arm64/include/asm/fpsimd.h | 2 ++
arch/arm64/kernel/fpsimd.c | 35 +++++++++++++++++++++++++++++++++--
arch/arm64/kernel/smp.c | 1 +
3 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h
index 6f034082f..ab7bf61 100644
--- a/arch/arm64/include/asm/fpsimd.h
+++ b/arch/arm64/include/asm/fpsimd.h
@@ -35,6 +35,7 @@ struct fpsimd_state {
__uint128_t vregs[32];
u32 fpsr;
u32 fpcr;
+ int last_cpu;
};
};
};
@@ -56,6 +57,7 @@ struct task_struct;

extern void fpsimd_save_state(struct fpsimd_state *state);
extern void fpsimd_load_state(struct fpsimd_state *state);
+extern void fpsimd_reset_lazy_restore(void);

extern void fpsimd_dup_task_struct(struct task_struct *dst,
struct task_struct *src);
diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index 604fe9f..b1b3b0e 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -22,6 +22,7 @@
#include <linux/sched.h>
#include <linux/signal.h>
#include <linux/hardirq.h>
+#include <linux/percpu.h>

#include <asm/fpsimd.h>
#include <asm/cputype.h>
@@ -33,6 +34,13 @@
#define FPEXC_IXF (1 << 4)
#define FPEXC_IDF (1 << 7)

+static DEFINE_PER_CPU(struct fpsimd_state *, fpsimd_owner);
+
+static inline void fpsimd_set_last_cpu(struct fpsimd_state *state, int cpu)
+{
+ state->last_cpu = cpu;
+}
+
static inline void fpsimd_init_hw_state(void)
{
int val = AARCH64_FPCR_DEFAULT_VAL;
@@ -83,19 +91,41 @@ void do_fpsimd_exc(unsigned int esr, struct pt_regs *regs)
send_sig_info(SIGFPE, &info, current);
}

+static void fpsimd_load_state_lazy(struct fpsimd_state *state)
+{
+ /* Could we reuse the hardware context? */
+ if (state->last_cpu == smp_processor_id() &&
+ __this_cpu_read(fpsimd_owner) == state)
+ return;
+ fpsimd_load_state(state);
+}
+
+static void fpsimd_save_state_lazy(struct fpsimd_state *state)
+{
+ fpsimd_save_state(state);
+ fpsimd_set_last_cpu(state, smp_processor_id());
+ __this_cpu_write(fpsimd_owner, state);
+}
+
+void fpsimd_reset_lazy_restore(void)
+{
+ this_cpu_write(fpsimd_owner, NULL);
+}
+
void fpsimd_dup_task_struct(struct task_struct *dst, struct task_struct *src)
{
fpsimd_save_state(&src->thread.fpsimd_state);
*dst = *src;
+ fpsimd_set_last_cpu(&dst->thread.fpsimd_state, -1);
}

void fpsimd_thread_switch(struct task_struct *next)
{
/* check if not kernel threads */
if (current->mm)
- fpsimd_save_state(&current->thread.fpsimd_state);
+ fpsimd_save_state_lazy(&current->thread.fpsimd_state);
if (next->mm)
- fpsimd_load_state(&next->thread.fpsimd_state);
+ fpsimd_load_state_lazy(&next->thread.fpsimd_state);
}

void fpsimd_flush_thread(void)
@@ -107,6 +137,7 @@ void fpsimd_flush_thread(void)
#if (AARCH64_FPCR_DEFAULT_VAL != 0)
state->fpcr = AARCH64_FPCR_DEFAULT_VAL;
#endif
+ fpsimd_set_last_cpu(state, -1);
fpsimd_load_state(state);
preempt_enable();
}
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index 78db90d..aae15c4 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -183,6 +183,7 @@ asmlinkage void secondary_start_kernel(void)
*/
cpu_set_reserved_ttbr0();
flush_tlb_all();
+ fpsimd_reset_lazy_restore();

preempt_disable();
trace_hardirqs_off();
--
1.8.1.2

2013-10-14 13:55:58

by Catalin Marinas

Subject: Re: [RFT PATCH v2 1/4] arm64: fix possible invalid FPSIMD initialization state

On Sun, Oct 13, 2013 at 03:20:17PM +0100, Jiang Liu wrote:
> From: Jiang Liu <[email protected]>
>
> If context switching happens during executing fpsimd_flush_thread(),
> stale value in FPSIMD registers will be saved into current thread's
> fpsimd_state by fpsimd_thread_switch(). That may cause invalid
> initialization state for the new process, so disable preemption
> when executing fpsimd_flush_thread().
>
> Signed-off-by: Jiang Liu <[email protected]>
> Cc: Jiang Liu <[email protected]>

That's already in mainline (3.12-rc4).

--
Catalin

2013-10-14 13:58:50

by Jiang Liu

Subject: Re: [RFT PATCH v2 1/4] arm64: fix possible invalid FPSIMD initialization state

On 10/14/2013 09:54 PM, Catalin Marinas wrote:
> On Sun, Oct 13, 2013 at 03:20:17PM +0100, Jiang Liu wrote:
>> From: Jiang Liu <[email protected]>
>>
>> If context switching happens during executing fpsimd_flush_thread(),
>> stale value in FPSIMD registers will be saved into current thread's
>> fpsimd_state by fpsimd_thread_switch(). That may cause invalid
>> initialization state for the new process, so disable preemption
>> when executing fpsimd_flush_thread().
>>
>> Signed-off-by: Jiang Liu <[email protected]>
>> Cc: Jiang Liu <[email protected]>
>
> That's already in mainline (3.12-rc4).
>

Hi Catalin,
I haven't noticed that yet, sorry for the noise:)
Thanks!
Gerry

2013-10-14 15:17:14

by Will Deacon

Subject: Re: [RFT PATCH v2 2/4] arm64: restore FPSIMD to default state for kernel and signal contexts

On Sun, Oct 13, 2013 at 03:20:18PM +0100, Jiang Liu wrote:
> From: Jiang Liu <[email protected]>
>
> Restore FPSIMD control and status registers to default values
> when creating new FPSIMD contexts for kernel context and reset
> FPSIMD status register when creating FPSIMD context for signal
> handling, otherwise the stale value in FPSIMD control and status
> registers may affect the new kernal or signal handling contexts.
>
> Signed-off-by: Jiang Liu <[email protected]>
> Cc: Jiang Liu <[email protected]>
> ---
> arch/arm64/include/asm/fpsimd.h | 16 ++++++++++++++++
> arch/arm64/kernel/fpsimd.c | 11 +++++++++--
> arch/arm64/kernel/signal.c | 1 +
> arch/arm64/kernel/signal32.c | 1 +
> 4 files changed, 27 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h
> index c43b4ac..b2dc30f 100644
> --- a/arch/arm64/include/asm/fpsimd.h
> +++ b/arch/arm64/include/asm/fpsimd.h
> @@ -50,8 +50,24 @@ struct fpsimd_state {
> #define VFP_STATE_SIZE ((32 * 8) + 4)
> #endif
>
> +#define AARCH64_FPCR_DEFAULT_VAL 0
> +
> struct task_struct;
>
> +static inline void fpsimd_init_hw_state(void)
> +{
> + int val = AARCH64_FPCR_DEFAULT_VAL;
> +
> + asm ("msr fpcr, %x0\n"
> + "msr fpsr, xzr\n"
> + : : "r"(val));
> +}
> +
> +static inline void fpsimd_clear_fpsr(void)
> +{
> + asm ("msr fpsr, xzr\n");
> +}

You have pretty weak asm constraints here...

> extern void fpsimd_save_state(struct fpsimd_state *state);
> extern void fpsimd_load_state(struct fpsimd_state *state);
>
> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> index bb785d2..12a25e5 100644
> --- a/arch/arm64/kernel/fpsimd.c
> +++ b/arch/arm64/kernel/fpsimd.c
> @@ -80,9 +80,14 @@ void fpsimd_thread_switch(struct task_struct *next)
>
> void fpsimd_flush_thread(void)
> {
> + struct fpsimd_state *state = &current->thread.fpsimd_state;
> +
> preempt_disable();
> - memset(&current->thread.fpsimd_state, 0, sizeof(struct fpsimd_state));
> - fpsimd_load_state(&current->thread.fpsimd_state);
> + memset(state, 0, sizeof(struct fpsimd_state));
> +#if (AARCH64_FPCR_DEFAULT_VAL != 0)
> + state->fpcr = AARCH64_FPCR_DEFAULT_VAL;
> +#endif
> + fpsimd_load_state(state);
> preempt_enable();
> }
>
> @@ -99,6 +104,8 @@ void kernel_neon_begin(void)
>
> if (current->mm)
> fpsimd_save_state(&current->thread.fpsimd_state);
> +
> + fpsimd_init_hw_state();
> }
> EXPORT_SYMBOL(kernel_neon_begin);
>
> diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
> index 890a591..4ee231e 100644
> --- a/arch/arm64/kernel/signal.c
> +++ b/arch/arm64/kernel/signal.c
> @@ -52,6 +52,7 @@ static int preserve_fpsimd_context(struct fpsimd_context __user *ctx)
>
> /* dump the hardware registers to the fpsimd_state structure */
> fpsimd_save_state(fpsimd);
> + fpsimd_clear_fpsr();

... so I reckon GCC could reorder these two calls, resulting in corruption
of the saved state register.

Will

2013-10-14 15:24:45

by Catalin Marinas

Subject: Re: [RFT PATCH v2 2/4] arm64: restore FPSIMD to default state for kernel and signal contexts

On Sun, Oct 13, 2013 at 03:20:18PM +0100, Jiang Liu wrote:
> From: Jiang Liu <[email protected]>
>
> Restore FPSIMD control and status registers to default values
> when creating new FPSIMD contexts for kernel context and reset
> FPSIMD status register when creating FPSIMD context for signal
> handling, otherwise the stale value in FPSIMD control and status
> registers may affect the new kernal or signal handling contexts.
>
> Signed-off-by: Jiang Liu <[email protected]>
> Cc: Jiang Liu <[email protected]>
> ---
> arch/arm64/include/asm/fpsimd.h | 16 ++++++++++++++++
> arch/arm64/kernel/fpsimd.c | 11 +++++++++--
> arch/arm64/kernel/signal.c | 1 +
> arch/arm64/kernel/signal32.c | 1 +
> 4 files changed, 27 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h
> index c43b4ac..b2dc30f 100644
> --- a/arch/arm64/include/asm/fpsimd.h
> +++ b/arch/arm64/include/asm/fpsimd.h
> @@ -50,8 +50,24 @@ struct fpsimd_state {
> #define VFP_STATE_SIZE ((32 * 8) + 4)
> #endif
>
> +#define AARCH64_FPCR_DEFAULT_VAL 0
> +
> struct task_struct;
>
> +static inline void fpsimd_init_hw_state(void)
> +{
> + int val = AARCH64_FPCR_DEFAULT_VAL;
> +
> + asm ("msr fpcr, %x0\n"
> + "msr fpsr, xzr\n"
> + : : "r"(val));
> +}

These could go in the fpsimd.c file, they are not used outside it.

> +
> +static inline void fpsimd_clear_fpsr(void)
> +{
> + asm ("msr fpsr, xzr\n");
> +}
> +
> extern void fpsimd_save_state(struct fpsimd_state *state);
> extern void fpsimd_load_state(struct fpsimd_state *state);
>
> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> index bb785d2..12a25e5 100644
> --- a/arch/arm64/kernel/fpsimd.c
> +++ b/arch/arm64/kernel/fpsimd.c
> @@ -80,9 +80,14 @@ void fpsimd_thread_switch(struct task_struct *next)
>
> void fpsimd_flush_thread(void)
> {
> + struct fpsimd_state *state = &current->thread.fpsimd_state;
> +
> preempt_disable();
> - memset(&current->thread.fpsimd_state, 0, sizeof(struct fpsimd_state));
> - fpsimd_load_state(&current->thread.fpsimd_state);
> + memset(state, 0, sizeof(struct fpsimd_state));
> +#if (AARCH64_FPCR_DEFAULT_VAL != 0)
> + state->fpcr = AARCH64_FPCR_DEFAULT_VAL;
> +#endif

Better to write as:

if (AARCH64_FPCR_DEFAULT_VAL)
state->fpcr = AARCH64_FPCR_DEFAULT_VAL;

The compiler should optimise it out.

> diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
> index 890a591..4ee231e 100644
> --- a/arch/arm64/kernel/signal.c
> +++ b/arch/arm64/kernel/signal.c
> @@ -52,6 +52,7 @@ static int preserve_fpsimd_context(struct fpsimd_context __user *ctx)
>
> /* dump the hardware registers to the fpsimd_state structure */
> fpsimd_save_state(fpsimd);
> + fpsimd_clear_fpsr();

That's the wrong place for such call. This function is supposed to save
the existing FP context and not modify it. Ideally we should do this as
with the other registers, just restoring them when returning to user.
But I wouldn't add fpsr to pt_regs, so somewhere like handle_signal()
should be OK.

--
Catalin

2013-10-14 15:30:11

by Jiang Liu

Subject: Re: [RFT PATCH v2 2/4] arm64: restore FPSIMD to default state for kernel and signal contexts

On 10/14/2013 11:16 PM, Will Deacon wrote:
> On Sun, Oct 13, 2013 at 03:20:18PM +0100, Jiang Liu wrote:
>> From: Jiang Liu <[email protected]>
>>
>> Restore FPSIMD control and status registers to default values
>> when creating new FPSIMD contexts for kernel context and reset
>> FPSIMD status register when creating FPSIMD context for signal
>> handling, otherwise the stale value in FPSIMD control and status
>> registers may affect the new kernal or signal handling contexts.
>>
>> Signed-off-by: Jiang Liu <[email protected]>
>> Cc: Jiang Liu <[email protected]>
>> ---
>> arch/arm64/include/asm/fpsimd.h | 16 ++++++++++++++++
>> arch/arm64/kernel/fpsimd.c | 11 +++++++++--
>> arch/arm64/kernel/signal.c | 1 +
>> arch/arm64/kernel/signal32.c | 1 +
>> 4 files changed, 27 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h
>> index c43b4ac..b2dc30f 100644
>> --- a/arch/arm64/include/asm/fpsimd.h
>> +++ b/arch/arm64/include/asm/fpsimd.h
>> @@ -50,8 +50,24 @@ struct fpsimd_state {
>> #define VFP_STATE_SIZE ((32 * 8) + 4)
>> #endif
>>
>> +#define AARCH64_FPCR_DEFAULT_VAL 0
>> +
>> struct task_struct;
>>
>> +static inline void fpsimd_init_hw_state(void)
>> +{
>> + int val = AARCH64_FPCR_DEFAULT_VAL;
>> +
>> + asm ("msr fpcr, %x0\n"
>> + "msr fpsr, xzr\n"
>> + : : "r"(val));
>> +}
>> +
>> +static inline void fpsimd_clear_fpsr(void)
>> +{
>> + asm ("msr fpsr, xzr\n");
>> +}
>
> You have pretty weak asm constraints here...
Hi Will,
We will add an explicit "volatile" here. But according to GCC docs, it
should have the same effect:
An asm instruction without any output operands is treated identically to
a volatile asm instruction.
Thanks!
Gerry

>
>> extern void fpsimd_save_state(struct fpsimd_state *state);
>> extern void fpsimd_load_state(struct fpsimd_state *state);
>>
>> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
>> index bb785d2..12a25e5 100644
>> --- a/arch/arm64/kernel/fpsimd.c
>> +++ b/arch/arm64/kernel/fpsimd.c
>> @@ -80,9 +80,14 @@ void fpsimd_thread_switch(struct task_struct *next)
>>
>> void fpsimd_flush_thread(void)
>> {
>> + struct fpsimd_state *state = &current->thread.fpsimd_state;
>> +
>> preempt_disable();
>> - memset(&current->thread.fpsimd_state, 0, sizeof(struct fpsimd_state));
>> - fpsimd_load_state(&current->thread.fpsimd_state);
>> + memset(state, 0, sizeof(struct fpsimd_state));
>> +#if (AARCH64_FPCR_DEFAULT_VAL != 0)
>> + state->fpcr = AARCH64_FPCR_DEFAULT_VAL;
>> +#endif
>> + fpsimd_load_state(state);
>> preempt_enable();
>> }
>>
>> @@ -99,6 +104,8 @@ void kernel_neon_begin(void)
>>
>> if (current->mm)
>> fpsimd_save_state(&current->thread.fpsimd_state);
>> +
>> + fpsimd_init_hw_state();
>> }
>> EXPORT_SYMBOL(kernel_neon_begin);
>>
>> diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
>> index 890a591..4ee231e 100644
>> --- a/arch/arm64/kernel/signal.c
>> +++ b/arch/arm64/kernel/signal.c
>> @@ -52,6 +52,7 @@ static int preserve_fpsimd_context(struct fpsimd_context __user *ctx)
>>
>> /* dump the hardware registers to the fpsimd_state structure */
>> fpsimd_save_state(fpsimd);
>> + fpsimd_clear_fpsr();
>
> ... so I reckon GCC could reorder these two calls, resulting in corruption
> of the saved state register.
>
> Will
>

2013-10-14 15:39:59

by Will Deacon

Subject: Re: [RFT PATCH v2 2/4] arm64: restore FPSIMD to default state for kernel and signal contexts

On Mon, Oct 14, 2013 at 04:30:00PM +0100, Jiang Liu wrote:
> On 10/14/2013 11:16 PM, Will Deacon wrote:
> > On Sun, Oct 13, 2013 at 03:20:18PM +0100, Jiang Liu wrote:
> >> From: Jiang Liu <[email protected]>
> >>
> >> Restore FPSIMD control and status registers to default values
> >> when creating new FPSIMD contexts for kernel context and reset
> >> FPSIMD status register when creating FPSIMD context for signal
> >> handling, otherwise the stale value in FPSIMD control and status
> >> registers may affect the new kernal or signal handling contexts.
> >>
> >> Signed-off-by: Jiang Liu <[email protected]>
> >> Cc: Jiang Liu <[email protected]>
> >> ---
> >> arch/arm64/include/asm/fpsimd.h | 16 ++++++++++++++++
> >> arch/arm64/kernel/fpsimd.c | 11 +++++++++--
> >> arch/arm64/kernel/signal.c | 1 +
> >> arch/arm64/kernel/signal32.c | 1 +
> >> 4 files changed, 27 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h
> >> index c43b4ac..b2dc30f 100644
> >> --- a/arch/arm64/include/asm/fpsimd.h
> >> +++ b/arch/arm64/include/asm/fpsimd.h
> >> @@ -50,8 +50,24 @@ struct fpsimd_state {
> >> #define VFP_STATE_SIZE ((32 * 8) + 4)
> >> #endif
> >>
> >> +#define AARCH64_FPCR_DEFAULT_VAL 0
> >> +
> >> struct task_struct;
> >>
> >> +static inline void fpsimd_init_hw_state(void)
> >> +{
> >> + int val = AARCH64_FPCR_DEFAULT_VAL;
> >> +
> >> + asm ("msr fpcr, %x0\n"
> >> + "msr fpsr, xzr\n"
> >> + : : "r"(val));
> >> +}
> >> +
> >> +static inline void fpsimd_clear_fpsr(void)
> >> +{
> >> + asm ("msr fpsr, xzr\n");
> >> +}
> >
> > You have pretty weak asm constraints here...
> Hi Will,
> We will add an explicit "volatile" here. But according to GCC docs, it
> should have the same effect:
> An asm instruction without any output operands is treated identically to
> a volatile asm instruction.

I don't think volatile is enough to prevent re-ordering across a function
call; it just prevents the block from being optimised away entirely and/or
reordered with respect to other volatile statements.

A "memory" clobber should do the trick in this case.

Will

2013-10-14 15:45:05

by Catalin Marinas

Subject: Re: [RFT PATCH v2 3/4] arm64: reduce duplicated code when saving/restoring FPSIMD for signal handling

On Sun, Oct 13, 2013 at 03:20:19PM +0100, Jiang Liu wrote:
> +void fpsimd_prepare_sigctx(struct fpsimd_state *state)
> +{
> + /* dump the hardware registers to the fpsimd_state structure */
> + fpsimd_save_state(state);
> + fpsimd_clear_fpsr();
> +}

What don't particularly like is that you save the FP context and then
corrupt it. Can we get preempted after this function and before we save
it on the signal stack?

--
Catalin

2013-10-14 15:51:03

by Jiang Liu

Subject: Re: [RFT PATCH v2 2/4] arm64: restore FPSIMD to default state for kernel and signal contexts

On 10/14/2013 11:39 PM, Will Deacon wrote:
> On Mon, Oct 14, 2013 at 04:30:00PM +0100, Jiang Liu wrote:
>> On 10/14/2013 11:16 PM, Will Deacon wrote:
>>> On Sun, Oct 13, 2013 at 03:20:18PM +0100, Jiang Liu wrote:
>>>> From: Jiang Liu <[email protected]>
>>>>
>>>> Restore FPSIMD control and status registers to default values
>>>> when creating new FPSIMD contexts for kernel context and reset
>>>> FPSIMD status register when creating FPSIMD context for signal
>>>> handling, otherwise the stale value in FPSIMD control and status
>>>> registers may affect the new kernal or signal handling contexts.
>>>>
>>>> Signed-off-by: Jiang Liu <[email protected]>
>>>> Cc: Jiang Liu <[email protected]>
>>>> ---
>>>> arch/arm64/include/asm/fpsimd.h | 16 ++++++++++++++++
>>>> arch/arm64/kernel/fpsimd.c | 11 +++++++++--
>>>> arch/arm64/kernel/signal.c | 1 +
>>>> arch/arm64/kernel/signal32.c | 1 +
>>>> 4 files changed, 27 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h
>>>> index c43b4ac..b2dc30f 100644
>>>> --- a/arch/arm64/include/asm/fpsimd.h
>>>> +++ b/arch/arm64/include/asm/fpsimd.h
>>>> @@ -50,8 +50,24 @@ struct fpsimd_state {
>>>> #define VFP_STATE_SIZE ((32 * 8) + 4)
>>>> #endif
>>>>
>>>> +#define AARCH64_FPCR_DEFAULT_VAL 0
>>>> +
>>>> struct task_struct;
>>>>
>>>> +static inline void fpsimd_init_hw_state(void)
>>>> +{
>>>> + int val = AARCH64_FPCR_DEFAULT_VAL;
>>>> +
>>>> + asm ("msr fpcr, %x0\n"
>>>> + "msr fpsr, xzr\n"
>>>> + : : "r"(val));
>>>> +}
>>>> +
>>>> +static inline void fpsimd_clear_fpsr(void)
>>>> +{
>>>> + asm ("msr fpsr, xzr\n");
>>>> +}
>>>
>>> You have pretty weak asm constraints here...
>> Hi Will,
>> We will add an explicit "volatile" here. But according to GCC docs, it
>> should have the same effect:
>> An asm instruction without any output operands is treated identically to
>> a volatile asm instruction.
>
> I don't think volatile is enough to prevent re-ordering across a function
> call; it just prevents the block from being optimised away entirely and/or
> reordered with respect to other volatile statements.
>
> A "memory" clobber should do the trick in this case.
Thanks for education, will fix it in next version.

>
> Will
>

2013-10-14 16:01:28

by Jiang Liu

Subject: Re: [RFT PATCH v2 3/4] arm64: reduce duplicated code when saving/restoring FPSIMD for signal handling

On 10/14/2013 11:44 PM, Catalin Marinas wrote:
> On Sun, Oct 13, 2013 at 03:20:19PM +0100, Jiang Liu wrote:
>> +void fpsimd_prepare_sigctx(struct fpsimd_state *state)
>> +{
>> + /* dump the hardware registers to the fpsimd_state structure */
>> + fpsimd_save_state(state);
>> + fpsimd_clear_fpsr();
>> +}
>
> What don't particularly like is that you save the FP context and then
> corrupt it. Can we get preempted after this function and before we save
> it on the signal stack?
>
Yeah, good point!
There's a race window to get preempted, will fix it.
Thanks!
Gerry