Re: [PATCH] Not preempt in CP1 exception handling

[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.

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
> 
>