Re: [PATCH] Not preempt in CP1 exception handling

[Paul Burton <paul.burton@imgtec.com>]

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