All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] MIPS: Don't BUG_ON(!is_fpu_owner()) in do_ade() when preemptible
@ 2014-07-11  3:06 Huacai Chen
  2014-07-11  3:14 ` [PATCH] Not preempt in CP1 exception handling chenj
  0 siblings, 1 reply; 11+ messages in thread
From: Huacai Chen @ 2014-07-11  3:06 UTC (permalink / raw)
  To: Ralf Baechle
  Cc: John Crispin, Steven J. Hill, linux-mips, Fuxin Zhang,
	Zhangjin Wu, Huacai Chen, Jie Chen, Rui Wang

In do_ade(), is_fpu_owner() isn't preempt-safe. For example, when an
unaligned ldc1 is executed, do_cpu() is called and then FPU is enabled
(TIF_USEDFPU is set for the current process). Then, do_ade() is called
because the access is unaligned. If the current process is preempted at
this time, TIF_USEDFPU will be cleard. When the process is scheduled
again, BUG_ON(!is_fpu_owner()) is triggered.

This small program can trigger this BUG in a preemptible kernel:
---
int main (int argc, char *argv[])
{
        double u64[2];

        while (1) {
                asm volatile (
                        ".set push \n\t"
                        ".set noreorder \n\t"
                        "ldc1 $f3, 4(%0) \n\t"
                        ".set pop \n\t"
                        ::"r"(u64):
                );
        }

        return 0;
}
---
Signed-off-by: Huacai Chen <chenhc@lemote.com>
Signed-off-by: Jie Chen <chenj@lemote.com>
Signed-off-by: Rui Wang <wangr@lemote.com>
---
 arch/mips/kernel/unaligned.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/mips/kernel/unaligned.c b/arch/mips/kernel/unaligned.c
index 2b35172..a6ff3c2 100644
--- a/arch/mips/kernel/unaligned.c
+++ b/arch/mips/kernel/unaligned.c
@@ -690,7 +690,8 @@ static void emulate_load_store_insn(struct pt_regs *regs,
 	case sdc1_op:
 		die_if_kernel("Unaligned FP access in kernel code", regs);
 		BUG_ON(!used_math());
-		BUG_ON(!is_fpu_owner());
+		if (!preemptible())
+			BUG_ON(!is_fpu_owner());
 
 		lose_fpu(1);	/* Save FPU state for the emulator. */
 		res = fpu_emulator_cop1Handler(regs, &current->thread.fpu, 1,
-- 
1.9.0

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] Not preempt in CP1 exception handling
  2014-07-11  3:14 ` [PATCH] Not preempt in CP1 exception handling chenj
@ 2014-07-11  3:13   ` Chen Jie
  2014-07-11 15:56     ` Paul Burton
  1 sibling, 0 replies; 11+ messages in thread
From: Chen Jie @ 2014-07-11  3:13 UTC (permalink / raw)
  To: Linux MIPS Mailing List
  Cc: 陈华才, Ralf Baechle, 王锐, chenj

Note this is another example patch to solve the problems in
http://patchwork.linux-mips.org/patch/7297/

gin 	\x04

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH] Not preempt in CP1 exception handling
  2014-07-11  3:06 [PATCH] MIPS: Don't BUG_ON(!is_fpu_owner()) in do_ade() when preemptible Huacai Chen
@ 2014-07-11  3:14 ` chenj
  2014-07-11  3:13   ` Chen Jie
  2014-07-11 15:56     ` Paul Burton
  0 siblings, 2 replies; 11+ messages in thread
From: chenj @ 2014-07-11  3:14 UTC (permalink / raw)
  To: linux-mips; +Cc: chenhc, ralf, wangr, chenj

do_ade may be invoked with preempt enabled. do_cpu will be invoked with
preempt enabled. When it's preempted(in do_ade/do_cpu), TIF_USEDFPU will be
cleared, when it returns to do_ade/do_cpu, the fpu is actually disabled.

e.g.
In do_ade()
  emulate_load_store_insn():
    BUG_ON(!is_fpu_owner()); <-- This assertion may be breaked.

In do_cpu()
  enable_restore_fp_context():
    was_fpu_owner = is_fpu_owner();

This patch simply disables interrupts in related handlers, and
disable preempt/enable interrupts in do_ade/do_cpu.
---
 arch/mips/kernel/genex.S | 4 ++--
 arch/mips/kernel/traps.c | 4 ++++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/mips/kernel/genex.S b/arch/mips/kernel/genex.S
index ac35e12..a5c6931 100644
--- a/arch/mips/kernel/genex.S
+++ b/arch/mips/kernel/genex.S
@@ -370,7 +370,7 @@ NESTED(nmi_handler, PT_SIZE, sp)
 	.macro	__build_clear_ade
 	MFC0	t0, CP0_BADVADDR
 	PTR_S	t0, PT_BVADDR(sp)
-	KMODE
+	CLI
 	.endm
 
 	.macro	__BUILD_silent exception
@@ -422,7 +422,7 @@ NESTED(nmi_handler, PT_SIZE, sp)
 	BUILD_HANDLER dbe be cli silent			/* #7  */
 	BUILD_HANDLER bp bp sti silent			/* #9  */
 	BUILD_HANDLER ri ri sti silent			/* #10 */
-	BUILD_HANDLER cpu cpu sti silent		/* #11 */
+	BUILD_HANDLER cpu cpu cli silent		/* #11 */
 	BUILD_HANDLER ov ov sti silent			/* #12 */
 	BUILD_HANDLER tr tr sti silent			/* #13 */
 	BUILD_HANDLER msa_fpe msa_fpe sti silent	/* #14 */
diff --git a/arch/mips/kernel/traps.c b/arch/mips/kernel/traps.c
index 51706d6..0e0f7de 100644
--- a/arch/mips/kernel/traps.c
+++ b/arch/mips/kernel/traps.c
@@ -1166,6 +1166,9 @@ asmlinkage void do_cpu(struct pt_regs *regs)
 	int status, err;
 	unsigned long __maybe_unused flags;
 
+	preempt_disable();
+	local_irq_enable();
+
 	prev_state = exception_enter();
 	cpid = (regs->cp0_cause >> CAUSEB_CE) & 3;
 
@@ -1258,6 +1261,7 @@ asmlinkage void do_cpu(struct pt_regs *regs)
 
 out:
 	exception_exit(prev_state);
+	preempt_enable();
 }
 
 asmlinkage void do_msa_fpe(struct pt_regs *regs)
-- 
1.9.0

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] Not preempt in CP1 exception handling
@ 2014-07-11 15:56     ` Paul Burton
  0 siblings, 0 replies; 11+ messages in thread
From: Paul Burton @ 2014-07-11 15:56 UTC (permalink / raw)
  To: chenj; +Cc: linux-mips, chenhc, ralf, wangr

On Fri, Jul 11, 2014 at 11:14:13AM +0800, chenj wrote:
> do_ade may be invoked with preempt enabled. do_cpu will be invoked with
> preempt enabled. When it's preempted(in do_ade/do_cpu), TIF_USEDFPU will be
> cleared, when it returns to do_ade/do_cpu, the fpu is actually disabled.
> 
> e.g.
> In do_ade()
>   emulate_load_store_insn():
>     BUG_ON(!is_fpu_owner()); <-- This assertion may be breaked.
> 
> In do_cpu()
>   enable_restore_fp_context():
>     was_fpu_owner = is_fpu_owner();

Preemption should indeed be disabled around the assignment & use of the
was_fpu_owner variable, but note that you can only hit the problem if
using MSA. One of the MSA fixes I just submitted also fixes this along
with another instance of the problem:

  http://patchwork.linux-mips.org/patch/7307/

I prefer my patch to this since it disables preemption for less time,
in addition to fixing the !used_math() case.

In emulate_load_store_insn I believe the correct fix is simply to remove
that BUG_ON. The code is about to give up FPU ownership anyway, so it's
not like there is any requirement being violated if it was already lost.

Thanks,
    Paul

> This patch simply disables interrupts in related handlers, and
> disable preempt/enable interrupts in do_ade/do_cpu.
> ---
>  arch/mips/kernel/genex.S | 4 ++--
>  arch/mips/kernel/traps.c | 4 ++++
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/mips/kernel/genex.S b/arch/mips/kernel/genex.S
> index ac35e12..a5c6931 100644
> --- a/arch/mips/kernel/genex.S
> +++ b/arch/mips/kernel/genex.S
> @@ -370,7 +370,7 @@ NESTED(nmi_handler, PT_SIZE, sp)
>  	.macro	__build_clear_ade
>  	MFC0	t0, CP0_BADVADDR
>  	PTR_S	t0, PT_BVADDR(sp)
> -	KMODE
> +	CLI
>  	.endm
>  
>  	.macro	__BUILD_silent exception
> @@ -422,7 +422,7 @@ NESTED(nmi_handler, PT_SIZE, sp)
>  	BUILD_HANDLER dbe be cli silent			/* #7  */
>  	BUILD_HANDLER bp bp sti silent			/* #9  */
>  	BUILD_HANDLER ri ri sti silent			/* #10 */
> -	BUILD_HANDLER cpu cpu sti silent		/* #11 */
> +	BUILD_HANDLER cpu cpu cli silent		/* #11 */
>  	BUILD_HANDLER ov ov sti silent			/* #12 */
>  	BUILD_HANDLER tr tr sti silent			/* #13 */
>  	BUILD_HANDLER msa_fpe msa_fpe sti silent	/* #14 */
> diff --git a/arch/mips/kernel/traps.c b/arch/mips/kernel/traps.c
> index 51706d6..0e0f7de 100644
> --- a/arch/mips/kernel/traps.c
> +++ b/arch/mips/kernel/traps.c
> @@ -1166,6 +1166,9 @@ asmlinkage void do_cpu(struct pt_regs *regs)
>  	int status, err;
>  	unsigned long __maybe_unused flags;
>  
> +	preempt_disable();
> +	local_irq_enable();
> +
>  	prev_state = exception_enter();
>  	cpid = (regs->cp0_cause >> CAUSEB_CE) & 3;
>  
> @@ -1258,6 +1261,7 @@ asmlinkage void do_cpu(struct pt_regs *regs)
>  
>  out:
>  	exception_exit(prev_state);
> +	preempt_enable();
>  }
>  
>  asmlinkage void do_msa_fpe(struct pt_regs *regs)
> -- 
> 1.9.0
> 
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Not preempt in CP1 exception handling
@ 2014-07-11 15:56     ` Paul Burton
  0 siblings, 0 replies; 11+ messages in thread
From: Paul Burton @ 2014-07-11 15:56 UTC (permalink / raw)
  To: chenj; +Cc: linux-mips, chenhc, ralf, wangr

On Fri, Jul 11, 2014 at 11:14:13AM +0800, chenj wrote:
> do_ade may be invoked with preempt enabled. do_cpu will be invoked with
> preempt enabled. When it's preempted(in do_ade/do_cpu), TIF_USEDFPU will be
> cleared, when it returns to do_ade/do_cpu, the fpu is actually disabled.
> 
> e.g.
> In do_ade()
>   emulate_load_store_insn():
>     BUG_ON(!is_fpu_owner()); <-- This assertion may be breaked.
> 
> In do_cpu()
>   enable_restore_fp_context():
>     was_fpu_owner = is_fpu_owner();

Preemption should indeed be disabled around the assignment & use of the
was_fpu_owner variable, but note that you can only hit the problem if
using MSA. One of the MSA fixes I just submitted also fixes this along
with another instance of the problem:

  http://patchwork.linux-mips.org/patch/7307/

I prefer my patch to this since it disables preemption for less time,
in addition to fixing the !used_math() case.

In emulate_load_store_insn I believe the correct fix is simply to remove
that BUG_ON. The code is about to give up FPU ownership anyway, so it's
not like there is any requirement being violated if it was already lost.

Thanks,
    Paul

> This patch simply disables interrupts in related handlers, and
> disable preempt/enable interrupts in do_ade/do_cpu.
> ---
>  arch/mips/kernel/genex.S | 4 ++--
>  arch/mips/kernel/traps.c | 4 ++++
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/mips/kernel/genex.S b/arch/mips/kernel/genex.S
> index ac35e12..a5c6931 100644
> --- a/arch/mips/kernel/genex.S
> +++ b/arch/mips/kernel/genex.S
> @@ -370,7 +370,7 @@ NESTED(nmi_handler, PT_SIZE, sp)
>  	.macro	__build_clear_ade
>  	MFC0	t0, CP0_BADVADDR
>  	PTR_S	t0, PT_BVADDR(sp)
> -	KMODE
> +	CLI
>  	.endm
>  
>  	.macro	__BUILD_silent exception
> @@ -422,7 +422,7 @@ NESTED(nmi_handler, PT_SIZE, sp)
>  	BUILD_HANDLER dbe be cli silent			/* #7  */
>  	BUILD_HANDLER bp bp sti silent			/* #9  */
>  	BUILD_HANDLER ri ri sti silent			/* #10 */
> -	BUILD_HANDLER cpu cpu sti silent		/* #11 */
> +	BUILD_HANDLER cpu cpu cli silent		/* #11 */
>  	BUILD_HANDLER ov ov sti silent			/* #12 */
>  	BUILD_HANDLER tr tr sti silent			/* #13 */
>  	BUILD_HANDLER msa_fpe msa_fpe sti silent	/* #14 */
> diff --git a/arch/mips/kernel/traps.c b/arch/mips/kernel/traps.c
> index 51706d6..0e0f7de 100644
> --- a/arch/mips/kernel/traps.c
> +++ b/arch/mips/kernel/traps.c
> @@ -1166,6 +1166,9 @@ asmlinkage void do_cpu(struct pt_regs *regs)
>  	int status, err;
>  	unsigned long __maybe_unused flags;
>  
> +	preempt_disable();
> +	local_irq_enable();
> +
>  	prev_state = exception_enter();
>  	cpid = (regs->cp0_cause >> CAUSEB_CE) & 3;
>  
> @@ -1258,6 +1261,7 @@ asmlinkage void do_cpu(struct pt_regs *regs)
>  
>  out:
>  	exception_exit(prev_state);
> +	preempt_enable();
>  }
>  
>  asmlinkage void do_msa_fpe(struct pt_regs *regs)
> -- 
> 1.9.0
> 
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Not preempt in CP1 exception handling
  2014-07-11 15:56     ` Paul Burton
  (?)
@ 2014-07-11 23:28     ` Chen Jie
  2014-07-12  9:10       ` Huacai Chen
  -1 siblings, 1 reply; 11+ messages in thread
From: Chen Jie @ 2014-07-11 23:28 UTC (permalink / raw)
  To: Paul Burton
  Cc: Linux MIPS Mailing List, 陈华才,
	Ralf Baechle, 王锐

2014-07-11 23:56 GMT+08:00 Paul Burton <paul.burton@imgtec.com>:
> On Fri, Jul 11, 2014 at 11:14:13AM +0800, chenj wrote:
>> do_ade may be invoked with preempt enabled. do_cpu will be invoked with
>> preempt enabled. When it's preempted(in do_ade/do_cpu), TIF_USEDFPU will be
>> cleared, when it returns to do_ade/do_cpu, the fpu is actually disabled.
>>
>> e.g.
>> In do_ade()
>>   emulate_load_store_insn():
>>     BUG_ON(!is_fpu_owner()); <-- This assertion may be breaked.
>>
>> In do_cpu()
>>   enable_restore_fp_context():
>>     was_fpu_owner = is_fpu_owner();
>
> Preemption should indeed be disabled around the assignment & use of the
> was_fpu_owner variable, but note that you can only hit the problem if
> using MSA. One of the MSA fixes I just submitted also fixes this along
> with another instance of the problem:
>
>   http://patchwork.linux-mips.org/patch/7307/
>
> I prefer my patch to this since it disables preemption for less time,
> in addition to fixing the !used_math() case.
>
> In emulate_load_store_insn I believe the correct fix is simply to remove
> that BUG_ON. The code is about to give up FPU ownership anyway, so it's
> not like there is any requirement being violated if it was already lost.
Yes, you're right.

""" /* arch/mips/kernel/unaligned.c */
lose_fpu(1);    /* Save FPU state for the emulator. */
res = fpu_emulator_cop1Handler(regs, &current->thread.fpu, 1, &fault_addr);
own_fpu(1);     /* Restore FPU state. */
"""

Going deep into the code, I find lost_fpu(1) will save fpu context if
owns fpu (otherwise, if preempted, the fpu context will be saved in
process switch), then fpu_emulator_cop1Handler manipulates the saved
fpu context, own_fpu(1) restores it.

So, remove "BUG_ON(!is_fpu_owner())" is OK.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Not preempt in CP1 exception handling
  2014-07-11 23:28     ` Chen Jie
@ 2014-07-12  9:10       ` Huacai Chen
  2014-07-12  9:30         ` Paul Burton
  0 siblings, 1 reply; 11+ messages in thread
From: Huacai Chen @ 2014-07-12  9:10 UTC (permalink / raw)
  To: Chen Jie
  Cc: Paul Burton, Linux MIPS Mailing List, Ralf Baechle, 王锐

Hi, Paul,

You means my patch (http://patchwork.linux-mips.org/patch/7297/) is
the correct way?

Another question: Your patch
(http://patchwork.linux-mips.org/patch/7307/) remove
preempt_disable()/preempt_enable() in init_fpu(). It will cause
problems if there is another function call init_fpu() because it is
previously preempt-safe. Maybe introduce a new function (e.g.
__init_fpu()) is a better way?

Huacai

On Sat, Jul 12, 2014 at 7:28 AM, Chen Jie <chenj@lemote.com> wrote:
> 2014-07-11 23:56 GMT+08:00 Paul Burton <paul.burton@imgtec.com>:
>> On Fri, Jul 11, 2014 at 11:14:13AM +0800, chenj wrote:
>>> do_ade may be invoked with preempt enabled. do_cpu will be invoked with
>>> preempt enabled. When it's preempted(in do_ade/do_cpu), TIF_USEDFPU will be
>>> cleared, when it returns to do_ade/do_cpu, the fpu is actually disabled.
>>>
>>> e.g.
>>> In do_ade()
>>>   emulate_load_store_insn():
>>>     BUG_ON(!is_fpu_owner()); <-- This assertion may be breaked.
>>>
>>> In do_cpu()
>>>   enable_restore_fp_context():
>>>     was_fpu_owner = is_fpu_owner();
>>
>> Preemption should indeed be disabled around the assignment & use of the
>> was_fpu_owner variable, but note that you can only hit the problem if
>> using MSA. One of the MSA fixes I just submitted also fixes this along
>> with another instance of the problem:
>>
>>   http://patchwork.linux-mips.org/patch/7307/
>>
>> I prefer my patch to this since it disables preemption for less time,
>> in addition to fixing the !used_math() case.
>>
>> In emulate_load_store_insn I believe the correct fix is simply to remove
>> that BUG_ON. The code is about to give up FPU ownership anyway, so it's
>> not like there is any requirement being violated if it was already lost.
> Yes, you're right.
>
> """ /* arch/mips/kernel/unaligned.c */
> lose_fpu(1);    /* Save FPU state for the emulator. */
> res = fpu_emulator_cop1Handler(regs, &current->thread.fpu, 1, &fault_addr);
> own_fpu(1);     /* Restore FPU state. */
> """
>
> Going deep into the code, I find lost_fpu(1) will save fpu context if
> owns fpu (otherwise, if preempted, the fpu context will be saved in
> process switch), then fpu_emulator_cop1Handler manipulates the saved
> fpu context, own_fpu(1) restores it.
>
> So, remove "BUG_ON(!is_fpu_owner())" is OK.
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Not preempt in CP1 exception handling
  2014-07-12  9:10       ` Huacai Chen
@ 2014-07-12  9:30         ` Paul Burton
  2014-07-14  2:22           ` Huacai Chen
  0 siblings, 1 reply; 11+ messages in thread
From: Paul Burton @ 2014-07-12  9:30 UTC (permalink / raw)
  To: Huacai Chen
  Cc: Chen Jie, Linux MIPS Mailing List, Ralf Baechle, 王锐

On Sat, Jul 12, 2014 at 05:10:35PM +0800, Huacai Chen wrote:
> Hi, Paul,
> 
> You means my patch (http://patchwork.linux-mips.org/patch/7297/) is
> the correct way?

I believe you patch will fix the problem, but I think it would be better
to remove the check for !preemptible() & the BUG_ON entirely.

> Another question: Your patch
> (http://patchwork.linux-mips.org/patch/7307/) remove
> preempt_disable()/preempt_enable() in init_fpu(). It will cause
> problems if there is another function call init_fpu() because it is
> previously preempt-safe. Maybe introduce a new function (e.g.
> __init_fpu()) is a better way?

It may cause a problem if there were other callers, but there is only
one caller of init_fpu (enable_restore_fp_context) and it needs to
disable preemption for longer than the init_fpu function anyway. I see
no value in keeping init_fpu as a wrapper that disables preemption
when there would be nothing calling it.

Thanks,
    Paul

> Huacai
> 
> On Sat, Jul 12, 2014 at 7:28 AM, Chen Jie <chenj@lemote.com> wrote:
> > 2014-07-11 23:56 GMT+08:00 Paul Burton <paul.burton@imgtec.com>:
> >> On Fri, Jul 11, 2014 at 11:14:13AM +0800, chenj wrote:
> >>> do_ade may be invoked with preempt enabled. do_cpu will be invoked with
> >>> preempt enabled. When it's preempted(in do_ade/do_cpu), TIF_USEDFPU will be
> >>> cleared, when it returns to do_ade/do_cpu, the fpu is actually disabled.
> >>>
> >>> e.g.
> >>> In do_ade()
> >>>   emulate_load_store_insn():
> >>>     BUG_ON(!is_fpu_owner()); <-- This assertion may be breaked.
> >>>
> >>> In do_cpu()
> >>>   enable_restore_fp_context():
> >>>     was_fpu_owner = is_fpu_owner();
> >>
> >> Preemption should indeed be disabled around the assignment & use of the
> >> was_fpu_owner variable, but note that you can only hit the problem if
> >> using MSA. One of the MSA fixes I just submitted also fixes this along
> >> with another instance of the problem:
> >>
> >>   http://patchwork.linux-mips.org/patch/7307/
> >>
> >> I prefer my patch to this since it disables preemption for less time,
> >> in addition to fixing the !used_math() case.
> >>
> >> In emulate_load_store_insn I believe the correct fix is simply to remove
> >> that BUG_ON. The code is about to give up FPU ownership anyway, so it's
> >> not like there is any requirement being violated if it was already lost.
> > Yes, you're right.
> >
> > """ /* arch/mips/kernel/unaligned.c */
> > lose_fpu(1);    /* Save FPU state for the emulator. */
> > res = fpu_emulator_cop1Handler(regs, &current->thread.fpu, 1, &fault_addr);
> > own_fpu(1);     /* Restore FPU state. */
> > """
> >
> > Going deep into the code, I find lost_fpu(1) will save fpu context if
> > owns fpu (otherwise, if preempted, the fpu context will be saved in
> > process switch), then fpu_emulator_cop1Handler manipulates the saved
> > fpu context, own_fpu(1) restores it.
> >
> > So, remove "BUG_ON(!is_fpu_owner())" is OK.
> >

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Not preempt in CP1 exception handling
  2014-07-12  9:30         ` Paul Burton
@ 2014-07-14  2:22           ` Huacai Chen
  2014-08-01 16:48             ` Ralf Baechle
  0 siblings, 1 reply; 11+ messages in thread
From: Huacai Chen @ 2014-07-14  2:22 UTC (permalink / raw)
  To: Paul Burton
  Cc: Chen Jie, Linux MIPS Mailing List, Ralf Baechle, 王锐

Hi, Ralf,

What do you think about? If you also prefer to totally remove the
BUG_ON(), I will send a new patch.

On Sat, Jul 12, 2014 at 5:30 PM, Paul Burton <paul.burton@imgtec.com> wrote:
> On Sat, Jul 12, 2014 at 05:10:35PM +0800, Huacai Chen wrote:
>> Hi, Paul,
>>
>> You means my patch (http://patchwork.linux-mips.org/patch/7297/) is
>> the correct way?
>
> I believe you patch will fix the problem, but I think it would be better
> to remove the check for !preemptible() & the BUG_ON entirely.
>
>> Another question: Your patch
>> (http://patchwork.linux-mips.org/patch/7307/) remove
>> preempt_disable()/preempt_enable() in init_fpu(). It will cause
>> problems if there is another function call init_fpu() because it is
>> previously preempt-safe. Maybe introduce a new function (e.g.
>> __init_fpu()) is a better way?
>
> It may cause a problem if there were other callers, but there is only
> one caller of init_fpu (enable_restore_fp_context) and it needs to
> disable preemption for longer than the init_fpu function anyway. I see
> no value in keeping init_fpu as a wrapper that disables preemption
> when there would be nothing calling it.
>
> Thanks,
>     Paul
>
>> Huacai
>>
>> On Sat, Jul 12, 2014 at 7:28 AM, Chen Jie <chenj@lemote.com> wrote:
>> > 2014-07-11 23:56 GMT+08:00 Paul Burton <paul.burton@imgtec.com>:
>> >> On Fri, Jul 11, 2014 at 11:14:13AM +0800, chenj wrote:
>> >>> do_ade may be invoked with preempt enabled. do_cpu will be invoked with
>> >>> preempt enabled. When it's preempted(in do_ade/do_cpu), TIF_USEDFPU will be
>> >>> cleared, when it returns to do_ade/do_cpu, the fpu is actually disabled.
>> >>>
>> >>> e.g.
>> >>> In do_ade()
>> >>>   emulate_load_store_insn():
>> >>>     BUG_ON(!is_fpu_owner()); <-- This assertion may be breaked.
>> >>>
>> >>> In do_cpu()
>> >>>   enable_restore_fp_context():
>> >>>     was_fpu_owner = is_fpu_owner();
>> >>
>> >> Preemption should indeed be disabled around the assignment & use of the
>> >> was_fpu_owner variable, but note that you can only hit the problem if
>> >> using MSA. One of the MSA fixes I just submitted also fixes this along
>> >> with another instance of the problem:
>> >>
>> >>   http://patchwork.linux-mips.org/patch/7307/
>> >>
>> >> I prefer my patch to this since it disables preemption for less time,
>> >> in addition to fixing the !used_math() case.
>> >>
>> >> In emulate_load_store_insn I believe the correct fix is simply to remove
>> >> that BUG_ON. The code is about to give up FPU ownership anyway, so it's
>> >> not like there is any requirement being violated if it was already lost.
>> > Yes, you're right.
>> >
>> > """ /* arch/mips/kernel/unaligned.c */
>> > lose_fpu(1);    /* Save FPU state for the emulator. */
>> > res = fpu_emulator_cop1Handler(regs, &current->thread.fpu, 1, &fault_addr);
>> > own_fpu(1);     /* Restore FPU state. */
>> > """
>> >
>> > Going deep into the code, I find lost_fpu(1) will save fpu context if
>> > owns fpu (otherwise, if preempted, the fpu context will be saved in
>> > process switch), then fpu_emulator_cop1Handler manipulates the saved
>> > fpu context, own_fpu(1) restores it.
>> >
>> > So, remove "BUG_ON(!is_fpu_owner())" is OK.
>> >
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Not preempt in CP1 exception handling
  2014-07-14  2:22           ` Huacai Chen
@ 2014-08-01 16:48             ` Ralf Baechle
  2014-08-19 15:56               ` Chen Jie
  0 siblings, 1 reply; 11+ messages in thread
From: Ralf Baechle @ 2014-08-01 16:48 UTC (permalink / raw)
  To: Huacai Chen
  Cc: Paul Burton, Chen Jie, Linux MIPS Mailing List, 王锐

On Mon, Jul 14, 2014 at 10:22:47AM +0800, Huacai Chen wrote:

> What do you think about? If you also prefer to totally remove the
> BUG_ON(), I will send a new patch.

I agree, removing the BUG_ON() is the right way.

  Ralf

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Not preempt in CP1 exception handling
  2014-08-01 16:48             ` Ralf Baechle
@ 2014-08-19 15:56               ` Chen Jie
  0 siblings, 0 replies; 11+ messages in thread
From: Chen Jie @ 2014-08-19 15:56 UTC (permalink / raw)
  To: Ralf Baechle; +Cc: Linux MIPS Mailing List

Please mark this patch as "Superseded", since patch "MIPS: Don't
BUG_ON(!is_fpu_owner()) in do_ade()"[1] was accepted :)

2014-08-02 0:48 GMT+08:00 Ralf Baechle <ralf@linux-mips.org>:
> On Mon, Jul 14, 2014 at 10:22:47AM +0800, Huacai Chen wrote:
>
>> What do you think about? If you also prefer to totally remove the
>> BUG_ON(), I will send a new patch.
>
> I agree, removing the BUG_ON() is the right way.
>
>   Ralf

---
1. http://patchwork.linux-mips.org/patch/7354/

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2014-08-19 15:57 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-07-11  3:06 [PATCH] MIPS: Don't BUG_ON(!is_fpu_owner()) in do_ade() when preemptible Huacai Chen
2014-07-11  3:14 ` [PATCH] Not preempt in CP1 exception handling chenj
2014-07-11  3:13   ` Chen Jie
2014-07-11 15:56   ` Paul Burton
2014-07-11 15:56     ` Paul Burton
2014-07-11 23:28     ` Chen Jie
2014-07-12  9:10       ` Huacai Chen
2014-07-12  9:30         ` Paul Burton
2014-07-14  2:22           ` Huacai Chen
2014-08-01 16:48             ` Ralf Baechle
2014-08-19 15:56               ` Chen Jie

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.