* [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, ¤t->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, ¤t->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, ¤t->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, ¤t->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, ¤t->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.