Two bug fixes of MSA registers set handling during context switch.
This fixes are respones to multithreading MSA application crash.
It was traced to incorrect handling of MSA registers set during
thread cloning. See inside.
---
Leonid Yegoshin (2):
MIPS: MSA: bugfix - disable MSA during thread switch correctly
MIPS: MSA: bugfix of keeping MSA live context through clone or fork
arch/mips/include/asm/switch_to.h | 1 -
arch/mips/kernel/process.c | 1 -
arch/mips/kernel/r4k_switch.S | 6 ++++++
3 files changed, 6 insertions(+), 2 deletions(-)
--
Signature
During thread cloning the new (child) thread should have MSA disabled even
at first thread entry. So, the code to disable MSA is moved from macro
'switch_to' to assembler function 'resume' before it switches kernel stack
to 'next' (new) thread. Call of 'disable_msa' after 'resume' in 'switch_to'
macro never called a first time entry into thread.
Signed-off-by: Leonid Yegoshin <[email protected]>
---
arch/mips/include/asm/switch_to.h | 1 -
arch/mips/kernel/r4k_switch.S | 6 ++++++
2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/arch/mips/include/asm/switch_to.h b/arch/mips/include/asm/switch_to.h
index e92d6c4b5ed1..0d0f7f8f8b3a 100644
--- a/arch/mips/include/asm/switch_to.h
+++ b/arch/mips/include/asm/switch_to.h
@@ -104,7 +104,6 @@ do { \
if (test_and_clear_tsk_thread_flag(prev, TIF_USEDMSA)) \
__fpsave = FP_SAVE_VECTOR; \
(last) = resume(prev, next, task_thread_info(next), __fpsave); \
- disable_msa(); \
} while (0)
#define finish_arch_switch(prev) \
diff --git a/arch/mips/kernel/r4k_switch.S b/arch/mips/kernel/r4k_switch.S
index 04cbbde3521b..7dbb64656bfe 100644
--- a/arch/mips/kernel/r4k_switch.S
+++ b/arch/mips/kernel/r4k_switch.S
@@ -25,6 +25,7 @@
/* preprocessor replaces the fp in ".set fp=64" with $30 otherwise */
#undef fp
+#define t4 $12
/*
* Offset to the current process status flags, the first 32 bytes of the
* stack are not used.
@@ -73,6 +74,11 @@
cfc1 t1, fcr31
msa_save_all a0
.set pop /* SET_HARDFLOAT */
+ li t4, MIPS_CONF5_MSAEN
+ mfc0 t3, CP0_CONFIG, 5
+ or t3, t3, t4
+ xor t3, t3, t4
+ mtc0 t3, CP0_CONFIG, 5
sw t1, THREAD_FCR31(a0)
b 2f
It seems the patch 39148e94e3e1f0477ce8ed3fda00123722681f3a
"MIPS: fork: Fix MSA/FPU/DSP context duplication race"
assumes that DSP/FPU and MSA context should be inherited in child at clone/fork
(look into patch description). It was done on Matthew Fortune request from
toolchain team, I guess.
Well, in this case it should prevent clearing TIF_MSA_CTX_LIVE in copy_thread().
Signed-off-by: Leonid Yegoshin <[email protected]>
---
arch/mips/kernel/process.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/arch/mips/kernel/process.c b/arch/mips/kernel/process.c
index f2975d4d1e44..a16e62d40210 100644
--- a/arch/mips/kernel/process.c
+++ b/arch/mips/kernel/process.c
@@ -163,7 +163,6 @@ int copy_thread(unsigned long clone_flags, unsigned long usp,
clear_tsk_thread_flag(p, TIF_USEDFPU);
clear_tsk_thread_flag(p, TIF_USEDMSA);
- clear_tsk_thread_flag(p, TIF_MSA_CTX_LIVE);
#ifdef CONFIG_MIPS_MT_FPAFF
clear_tsk_thread_flag(p, TIF_FPUBOUND);
Cancel this, please.
Reason - MSA registers are not supposed to be preserved through
caller-called interface, including syscall.
In other side, keeping MSA context is expensive.
- Leonid.
On Tue, May 19, 2015 at 02:13:51PM -0700, Leonid Yegoshin wrote:
> During thread cloning the new (child) thread should have MSA disabled even
> at first thread entry. So, the code to disable MSA is moved from macro
> 'switch_to' to assembler function 'resume' before it switches kernel stack
> to 'next' (new) thread. Call of 'disable_msa' after 'resume' in 'switch_to'
> macro never called a first time entry into thread.
Hi Leonid,
I'm not sure I understand what you're trying to say here. Do you have an
example of a program that demonstrates the behaviour you believe to be
broken?
Thanks,
Paul
> Signed-off-by: Leonid Yegoshin <[email protected]>
> ---
> arch/mips/include/asm/switch_to.h | 1 -
> arch/mips/kernel/r4k_switch.S | 6 ++++++
> 2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/arch/mips/include/asm/switch_to.h b/arch/mips/include/asm/switch_to.h
> index e92d6c4b5ed1..0d0f7f8f8b3a 100644
> --- a/arch/mips/include/asm/switch_to.h
> +++ b/arch/mips/include/asm/switch_to.h
> @@ -104,7 +104,6 @@ do { \
> if (test_and_clear_tsk_thread_flag(prev, TIF_USEDMSA)) \
> __fpsave = FP_SAVE_VECTOR; \
> (last) = resume(prev, next, task_thread_info(next), __fpsave); \
> - disable_msa(); \
> } while (0)
>
> #define finish_arch_switch(prev) \
> diff --git a/arch/mips/kernel/r4k_switch.S b/arch/mips/kernel/r4k_switch.S
> index 04cbbde3521b..7dbb64656bfe 100644
> --- a/arch/mips/kernel/r4k_switch.S
> +++ b/arch/mips/kernel/r4k_switch.S
> @@ -25,6 +25,7 @@
> /* preprocessor replaces the fp in ".set fp=64" with $30 otherwise */
> #undef fp
>
> +#define t4 $12
> /*
> * Offset to the current process status flags, the first 32 bytes of the
> * stack are not used.
> @@ -73,6 +74,11 @@
> cfc1 t1, fcr31
> msa_save_all a0
> .set pop /* SET_HARDFLOAT */
> + li t4, MIPS_CONF5_MSAEN
> + mfc0 t3, CP0_CONFIG, 5
> + or t3, t3, t4
> + xor t3, t3, t4
> + mtc0 t3, CP0_CONFIG, 5
>
> sw t1, THREAD_FCR31(a0)
> b 2f
>
Yes, I have a program but it is binary only.
If you want to understand why disable_DMA() after resume() doesn't work,
search for restoring RA register in resume() after changing SP.
- Leonid.
Paul Burton <[email protected]> wrote:
On Tue, May 19, 2015 at 02:13:51PM -0700, Leonid Yegoshin wrote:
> During thread cloning the new (child) thread should have MSA disabled even
> at first thread entry. So, the code to disable MSA is moved from macro
> 'switch_to' to assembler function 'resume' before it switches kernel stack
> to 'next' (new) thread. Call of 'disable_msa' after 'resume' in 'switch_to'
> macro never called a first time entry into thread.
Hi Leonid,
I'm not sure I understand what you're trying to say here. Do you have an
example of a program that demonstrates the behaviour you believe to be
broken?
Thanks,
Paul
> Signed-off-by: Leonid Yegoshin <[email protected]>
> ---
> arch/mips/include/asm/switch_to.h | 1 -
> arch/mips/kernel/r4k_switch.S | 6 ++++++
> 2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/arch/mips/include/asm/switch_to.h b/arch/mips/include/asm/switch_to.h
> index e92d6c4b5ed1..0d0f7f8f8b3a 100644
> --- a/arch/mips/include/asm/switch_to.h
> +++ b/arch/mips/include/asm/switch_to.h
> @@ -104,7 +104,6 @@ do { \
> if (test_and_clear_tsk_thread_flag(prev, TIF_USEDMSA)) \
> __fpsave = FP_SAVE_VECTOR; \
> (last) = resume(prev, next, task_thread_info(next), __fpsave); \
> - disable_msa(); \
> } while (0)
>
> #define finish_arch_switch(prev) \
> diff --git a/arch/mips/kernel/r4k_switch.S b/arch/mips/kernel/r4k_switch.S
> index 04cbbde3521b..7dbb64656bfe 100644
> --- a/arch/mips/kernel/r4k_switch.S
> +++ b/arch/mips/kernel/r4k_switch.S
> @@ -25,6 +25,7 @@
> /* preprocessor replaces the fp in ".set fp=64" with $30 otherwise */
> #undef fp
>
> +#define t4 $12
> /*
> * Offset to the current process status flags, the first 32 bytes of the
> * stack are not used.
> @@ -73,6 +74,11 @@
> cfc1 t1, fcr31
> msa_save_all a0
> .set pop /* SET_HARDFLOAT */
> + li t4, MIPS_CONF5_MSAEN
> + mfc0 t3, CP0_CONFIG, 5
> + or t3, t3, t4
> + xor t3, t3, t4
> + mtc0 t3, CP0_CONFIG, 5
>
> sw t1, THREAD_FCR31(a0)
> b 2f
>
Rather than saving the scalar FP or vector context in the assembly
resume function, simply call lose_fpu(1) from the switch_to macro in
order to save the appropriate context, disabling the FPU & MSA.
Signed-off-by: Paul Burton <[email protected]>
---
How about this (lightly tested for the moment) alternative?
arch/mips/include/asm/switch_to.h | 21 ++++----------------
arch/mips/kernel/r4k_switch.S | 41 +--------------------------------------
2 files changed, 5 insertions(+), 57 deletions(-)
diff --git a/arch/mips/include/asm/switch_to.h b/arch/mips/include/asm/switch_to.h
index e92d6c4b..1a7a316 100644
--- a/arch/mips/include/asm/switch_to.h
+++ b/arch/mips/include/asm/switch_to.h
@@ -16,29 +16,21 @@
#include <asm/watch.h>
#include <asm/dsp.h>
#include <asm/cop2.h>
-#include <asm/msa.h>
+#include <asm/fpu.h>
struct task_struct;
-enum {
- FP_SAVE_NONE = 0,
- FP_SAVE_VECTOR = -1,
- FP_SAVE_SCALAR = 1,
-};
-
/**
* resume - resume execution of a task
* @prev: The task previously executed.
* @next: The task to begin executing.
* @next_ti: task_thread_info(next).
- * @fp_save: Which, if any, FP context to save for prev.
*
* This function is used whilst scheduling to save the context of prev & load
* the context of next. Returns prev.
*/
extern asmlinkage struct task_struct *resume(struct task_struct *prev,
- struct task_struct *next, struct thread_info *next_ti,
- s32 fp_save);
+ struct task_struct *next, struct thread_info *next_ti);
extern unsigned int ll_bit;
extern struct task_struct *ll_task;
@@ -86,8 +78,8 @@ do { if (cpu_has_rw_llb) { \
#define switch_to(prev, next, last) \
do { \
u32 __c0_stat; \
- s32 __fpsave = FP_SAVE_NONE; \
__mips_mt_fpaff_switch_to(prev); \
+ lose_fpu(1); \
if (cpu_has_dsp) \
__save_dsp(prev); \
if (cop2_present && (KSTK_STATUS(prev) & ST0_CU2)) { \
@@ -99,12 +91,7 @@ do { \
write_c0_status(__c0_stat & ~ST0_CU2); \
} \
__clear_software_ll_bit(); \
- if (test_and_clear_tsk_thread_flag(prev, TIF_USEDFPU)) \
- __fpsave = FP_SAVE_SCALAR; \
- if (test_and_clear_tsk_thread_flag(prev, TIF_USEDMSA)) \
- __fpsave = FP_SAVE_VECTOR; \
- (last) = resume(prev, next, task_thread_info(next), __fpsave); \
- disable_msa(); \
+ (last) = resume(prev, next, task_thread_info(next)); \
} while (0)
#define finish_arch_switch(prev) \
diff --git a/arch/mips/kernel/r4k_switch.S b/arch/mips/kernel/r4k_switch.S
index 04cbbde3..92cd051 100644
--- a/arch/mips/kernel/r4k_switch.S
+++ b/arch/mips/kernel/r4k_switch.S
@@ -34,7 +34,7 @@
#ifndef USE_ALTERNATE_RESUME_IMPL
/*
* task_struct *resume(task_struct *prev, task_struct *next,
- * struct thread_info *next_ti, s32 fp_save)
+ * struct thread_info *next_ti)
*/
.align 5
LEAF(resume)
@@ -43,45 +43,6 @@
cpu_save_nonscratch a0
LONG_S ra, THREAD_REG31(a0)
- /*
- * Check whether we need to save any FP context. FP context is saved
- * iff the process has used the context with the scalar FPU or the MSA
- * ASE in the current time slice, as indicated by _TIF_USEDFPU and
- * _TIF_USEDMSA respectively. switch_to will have set fp_save
- * accordingly to an FP_SAVE_ enum value.
- */
- beqz a3, 2f
-
- /*
- * We do. Clear the saved CU1 bit for prev, such that next time it is
- * scheduled it will start in userland with the FPU disabled. If the
- * task uses the FPU then it will be enabled again via the do_cpu trap.
- * This allows us to lazily restore the FP context.
- */
- PTR_L t3, TASK_THREAD_INFO(a0)
- LONG_L t0, ST_OFF(t3)
- li t1, ~ST0_CU1
- and t0, t0, t1
- LONG_S t0, ST_OFF(t3)
-
- /* Check whether we're saving scalar or vector context. */
- bgtz a3, 1f
-
- /* Save 128b MSA vector context + scalar FP control & status. */
- .set push
- SET_HARDFLOAT
- cfc1 t1, fcr31
- msa_save_all a0
- .set pop /* SET_HARDFLOAT */
-
- sw t1, THREAD_FCR31(a0)
- b 2f
-
-1: /* Save 32b/64b scalar FP context. */
- fpu_save_double a0 t0 t1 # c0_status passed in t0
- # clobbers t1
-2:
-
#if defined(CONFIG_CC_STACKPROTECTOR) && !defined(CONFIG_SMP)
PTR_LA t8, __stack_chk_guard
LONG_L t9, TASK_STACK_CANARY(a1)
--
2.4.1
On Tue, May 19, 2015 at 02:13:51PM -0700, Leonid Yegoshin wrote:
> During thread cloning the new (child) thread should have MSA disabled even
> at first thread entry. So, the code to disable MSA is moved from macro
> 'switch_to' to assembler function 'resume' before it switches kernel stack
> to 'next' (new) thread. Call of 'disable_msa' after 'resume' in 'switch_to'
> macro never called a first time entry into thread.
>
> Signed-off-by: Leonid Yegoshin <[email protected]>
> ---
> arch/mips/include/asm/switch_to.h | 1 -
> arch/mips/kernel/r4k_switch.S | 6 ++++++
> 2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/arch/mips/include/asm/switch_to.h b/arch/mips/include/asm/switch_to.h
> index e92d6c4b5ed1..0d0f7f8f8b3a 100644
> --- a/arch/mips/include/asm/switch_to.h
> +++ b/arch/mips/include/asm/switch_to.h
> @@ -104,7 +104,6 @@ do { \
> if (test_and_clear_tsk_thread_flag(prev, TIF_USEDMSA)) \
> __fpsave = FP_SAVE_VECTOR; \
> (last) = resume(prev, next, task_thread_info(next), __fpsave); \
> - disable_msa(); \
> } while (0)
>
> #define finish_arch_switch(prev) \
> diff --git a/arch/mips/kernel/r4k_switch.S b/arch/mips/kernel/r4k_switch.S
> index 04cbbde3521b..7dbb64656bfe 100644
> --- a/arch/mips/kernel/r4k_switch.S
> +++ b/arch/mips/kernel/r4k_switch.S
> @@ -25,6 +25,7 @@
> /* preprocessor replaces the fp in ".set fp=64" with $30 otherwise */
> #undef fp
>
> +#define t4 $12
You're kidding, right?
> /*
> * Offset to the current process status flags, the first 32 bytes of the
> * stack are not used.
> @@ -73,6 +74,11 @@
> cfc1 t1, fcr31
> msa_save_all a0
> .set pop /* SET_HARDFLOAT */
> + li t4, MIPS_CONF5_MSAEN
> + mfc0 t3, CP0_CONFIG, 5
> + or t3, t3, t4
> + xor t3, t3, t4
> + mtc0 t3, CP0_CONFIG, 5
>
> sw t1, THREAD_FCR31(a0)
> b 2f
Just move the call to finish_arch_switch().
Your rewrite also dropped the if (cpu_has_msa) condition from disable_msa()
probably causing havoc on lots of CPUs which will likely not decode the
set bits of the MFC0/MTC0 instructions thus end up accessing Config0.
Ralf
Rather than saving the scalar FP or vector context in the assembly
resume function, reuse the existing C code we have in fpu.h to do
exactly that. This reduces duplication, results in a much easier to read
resume function & should allow the compiler to optimise out more MSA
code due to is_msa_enabled()/cpu_has_msa being known-zero at compile
time for kernels without MSA support.
As a side effect this correctly disables MSA on the first entry to a
task, in which case resume will "return" to ret_from_kernel_thread or
ret_from_fork in order to call schedule_tail. This would lead to the
disable_msa call in switch_to not being executed, as reported by Leonid.
Signed-off-by: Paul Burton <[email protected]>
Reported-by: Leonid Yegoshin <[email protected]>
---
How about this (lightly tested for the moment) alternative to 10082?
Changes in v2:
- Introduce lose_fpu_inatomic to skip the preempt_{en,dis}able calls
and operate on the provided struct task_struct, which should be prev
rather than current.
arch/mips/include/asm/fpu.h | 21 ++++++++++++--------
arch/mips/include/asm/switch_to.h | 21 ++++----------------
arch/mips/kernel/r4k_switch.S | 41 +--------------------------------------
3 files changed, 18 insertions(+), 65 deletions(-)
diff --git a/arch/mips/include/asm/fpu.h b/arch/mips/include/asm/fpu.h
index 084780b..87e8c78 100644
--- a/arch/mips/include/asm/fpu.h
+++ b/arch/mips/include/asm/fpu.h
@@ -164,25 +164,30 @@ static inline int own_fpu(int restore)
return ret;
}
-static inline void lose_fpu(int save)
+static inline void lose_fpu_inatomic(int save, struct task_struct *tsk)
{
- preempt_disable();
if (is_msa_enabled()) {
if (save) {
- save_msa(current);
- current->thread.fpu.fcr31 =
+ save_msa(tsk);
+ tsk->thread.fpu.fcr31 =
read_32bit_cp1_register(CP1_STATUS);
}
disable_msa();
- clear_thread_flag(TIF_USEDMSA);
+ clear_tsk_thread_flag(tsk, TIF_USEDMSA);
__disable_fpu();
} else if (is_fpu_owner()) {
if (save)
- _save_fp(current);
+ _save_fp(tsk);
__disable_fpu();
}
- KSTK_STATUS(current) &= ~ST0_CU1;
- clear_thread_flag(TIF_USEDFPU);
+ KSTK_STATUS(tsk) &= ~ST0_CU1;
+ clear_tsk_thread_flag(tsk, TIF_USEDFPU);
+}
+
+static inline void lose_fpu(int save)
+{
+ preempt_disable();
+ lose_fpu_inatomic(save, current);
preempt_enable();
}
diff --git a/arch/mips/include/asm/switch_to.h b/arch/mips/include/asm/switch_to.h
index e92d6c4b..c429d1a 100644
--- a/arch/mips/include/asm/switch_to.h
+++ b/arch/mips/include/asm/switch_to.h
@@ -16,29 +16,21 @@
#include <asm/watch.h>
#include <asm/dsp.h>
#include <asm/cop2.h>
-#include <asm/msa.h>
+#include <asm/fpu.h>
struct task_struct;
-enum {
- FP_SAVE_NONE = 0,
- FP_SAVE_VECTOR = -1,
- FP_SAVE_SCALAR = 1,
-};
-
/**
* resume - resume execution of a task
* @prev: The task previously executed.
* @next: The task to begin executing.
* @next_ti: task_thread_info(next).
- * @fp_save: Which, if any, FP context to save for prev.
*
* This function is used whilst scheduling to save the context of prev & load
* the context of next. Returns prev.
*/
extern asmlinkage struct task_struct *resume(struct task_struct *prev,
- struct task_struct *next, struct thread_info *next_ti,
- s32 fp_save);
+ struct task_struct *next, struct thread_info *next_ti);
extern unsigned int ll_bit;
extern struct task_struct *ll_task;
@@ -86,8 +78,8 @@ do { if (cpu_has_rw_llb) { \
#define switch_to(prev, next, last) \
do { \
u32 __c0_stat; \
- s32 __fpsave = FP_SAVE_NONE; \
__mips_mt_fpaff_switch_to(prev); \
+ lose_fpu_inatomic(1, prev); \
if (cpu_has_dsp) \
__save_dsp(prev); \
if (cop2_present && (KSTK_STATUS(prev) & ST0_CU2)) { \
@@ -99,12 +91,7 @@ do { \
write_c0_status(__c0_stat & ~ST0_CU2); \
} \
__clear_software_ll_bit(); \
- if (test_and_clear_tsk_thread_flag(prev, TIF_USEDFPU)) \
- __fpsave = FP_SAVE_SCALAR; \
- if (test_and_clear_tsk_thread_flag(prev, TIF_USEDMSA)) \
- __fpsave = FP_SAVE_VECTOR; \
- (last) = resume(prev, next, task_thread_info(next), __fpsave); \
- disable_msa(); \
+ (last) = resume(prev, next, task_thread_info(next)); \
} while (0)
#define finish_arch_switch(prev) \
diff --git a/arch/mips/kernel/r4k_switch.S b/arch/mips/kernel/r4k_switch.S
index 04cbbde3..92cd051 100644
--- a/arch/mips/kernel/r4k_switch.S
+++ b/arch/mips/kernel/r4k_switch.S
@@ -34,7 +34,7 @@
#ifndef USE_ALTERNATE_RESUME_IMPL
/*
* task_struct *resume(task_struct *prev, task_struct *next,
- * struct thread_info *next_ti, s32 fp_save)
+ * struct thread_info *next_ti)
*/
.align 5
LEAF(resume)
@@ -43,45 +43,6 @@
cpu_save_nonscratch a0
LONG_S ra, THREAD_REG31(a0)
- /*
- * Check whether we need to save any FP context. FP context is saved
- * iff the process has used the context with the scalar FPU or the MSA
- * ASE in the current time slice, as indicated by _TIF_USEDFPU and
- * _TIF_USEDMSA respectively. switch_to will have set fp_save
- * accordingly to an FP_SAVE_ enum value.
- */
- beqz a3, 2f
-
- /*
- * We do. Clear the saved CU1 bit for prev, such that next time it is
- * scheduled it will start in userland with the FPU disabled. If the
- * task uses the FPU then it will be enabled again via the do_cpu trap.
- * This allows us to lazily restore the FP context.
- */
- PTR_L t3, TASK_THREAD_INFO(a0)
- LONG_L t0, ST_OFF(t3)
- li t1, ~ST0_CU1
- and t0, t0, t1
- LONG_S t0, ST_OFF(t3)
-
- /* Check whether we're saving scalar or vector context. */
- bgtz a3, 1f
-
- /* Save 128b MSA vector context + scalar FP control & status. */
- .set push
- SET_HARDFLOAT
- cfc1 t1, fcr31
- msa_save_all a0
- .set pop /* SET_HARDFLOAT */
-
- sw t1, THREAD_FCR31(a0)
- b 2f
-
-1: /* Save 32b/64b scalar FP context. */
- fpu_save_double a0 t0 t1 # c0_status passed in t0
- # clobbers t1
-2:
-
#if defined(CONFIG_CC_STACKPROTECTOR) && !defined(CONFIG_SMP)
PTR_LA t8, __stack_chk_guard
LONG_L t9, TASK_STACK_CANARY(a1)
--
2.4.1
On 05/22/2015 02:38 AM, Ralf Baechle wrote:
> Just move the call to finish_arch_switch().
It might be a problem later, then a correct MSA partiton starts working.
It should be tight to saving MSA registers in that case.
> Your rewrite also dropped the if (cpu_has_msa) condition from
> disable_msa() probably causing havoc on lots of CPUs which will likely
> not decode the set bits of the MFC0/MTC0 instructions thus end up
> accessing Config0. Ralf
Right before this chunk of code there is a saving MSA registers. Does it
causing a havoc or else?
May I ask you to look into switch_to macro to figure out how "if
(cpu_has_msa)" check works in this case?
- Leonid.
Ralf,
If there was TIF_USEDMSA in "prev" task then it means that all MSA HW is
in use.
And switch_to() checks this and transfers it to resume() to indicate
that MSA processing should be done.
Macro call "msa_save_all a0" right before disabling MSA in Config5
does a save of MSA registers. If it doesn't cause an exception then it
means that Config5 does exist and Config5.MIPS_CONF5_MSAEN does exist too.
- Leonid.
On Fri, May 22, 2015 at 11:37:34AM -0700, Leonid Yegoshin wrote:
> On 05/22/2015 02:38 AM, Ralf Baechle wrote:
> >Just move the call to finish_arch_switch().
>
> It might be a problem later, then a correct MSA partiton starts working. It
> should be tight to saving MSA registers in that case.
>
> >Your rewrite also dropped the if (cpu_has_msa) condition from
> >disable_msa() probably causing havoc on lots of CPUs which will likely not
> >decode the set bits of the MFC0/MTC0 instructions thus end up accessing
> >Config0. Ralf
>
> Right before this chunk of code there is a saving MSA registers. Does it
> causing a havoc or else?
>
> May I ask you to look into switch_to macro to figure out how "if
> (cpu_has_msa)" check works in this case?
Ah sorry I now see that your added code is not executed for all CPUs but
only those having MSA. So then it's safe.
Still I don't stylistically like defining the register t4 in the middle
of the code.
Below my suggested patch. It's advantage is that for non-MSA platforms
the call to disable_msa() will be removed entirely.
Something like Paul's http://patchwork.linux-mips.org/patch/10111/ (assuming
it's correct and tested) seems like a full cleanup but it's way too
complex for 4.1 or the stable kernels.
Ralf
Signed-off-by: Ralf Baechle <[email protected]>
arch/mips/include/asm/switch_to.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/mips/include/asm/switch_to.h b/arch/mips/include/asm/switch_to.h
index e92d6c4b..7163cd7 100644
--- a/arch/mips/include/asm/switch_to.h
+++ b/arch/mips/include/asm/switch_to.h
@@ -104,7 +104,6 @@ do { \
if (test_and_clear_tsk_thread_flag(prev, TIF_USEDMSA)) \
__fpsave = FP_SAVE_VECTOR; \
(last) = resume(prev, next, task_thread_info(next), __fpsave); \
- disable_msa(); \
} while (0)
#define finish_arch_switch(prev) \
@@ -122,6 +121,7 @@ do { \
if (cpu_has_userlocal) \
write_c0_userlocal(current_thread_info()->tp_value); \
__restore_watch(); \
+ disable_msa(); \
} while (0)
#endif /* _ASM_SWITCH_TO_H */
On 05/22/2015 04:20 PM, Ralf Baechle wrote:
> On Fri, May 22, 2015 at 11:37:34AM -0700, Leonid Yegoshin wrote:
>
>> On 05/22/2015 02:38 AM, Ralf Baechle wrote:
>>> Just move the call to finish_arch_switch().
>> It might be a problem later, then a correct MSA partiton starts working. It
>> should be tight to saving MSA registers in that case.
>>
>>> Your rewrite also dropped the if (cpu_has_msa) condition from
>>> disable_msa() probably causing havoc on lots of CPUs which will likely not
>>> decode the set bits of the MFC0/MTC0 instructions thus end up accessing
>>> Config0. Ralf
>> Right before this chunk of code there is a saving MSA registers. Does it
>> causing a havoc or else?
>>
>> May I ask you to look into switch_to macro to figure out how "if
>> (cpu_has_msa)" check works in this case?
> Ah sorry I now see that your added code is not executed for all CPUs but
> only those having MSA. So then it's safe.
>
> Still I don't stylistically like defining the register t4 in the middle
> of the code.
>
> Below my suggested patch. It's advantage is that for non-MSA platforms
> the call to disable_msa() will be removed entirely.
>
> Something like Paul's http://patchwork.linux-mips.org/patch/10111/ (assuming
> it's correct and tested) seems like a full cleanup but it's way too
> complex for 4.1 or the stable kernels.
>
> Ralf
>
>
All 3 patches seems working (I tested), but if you don't like mine then
I prefer Paul's patch more - it concentrates stuff more closely and
removes some assembly stuff.
Besides that, it introduces lose_fpu_inatomic() which is needed for me :)
- Leonid.