Re: FP emulator patch

["Kevin D. Kissell" <kevink@mips.com>]

> Two comments, especially since parts of this seem to be the patch I
> posted here over a month ago.
> 
> > Index: linux/arch/mips/kernel/signal.c
> 
> > @@ -353,12 +355,11 @@
> >  owned_fp = (current == last_task_used_math);
> >  err |= __put_user(owned_fp, &sc->sc_ownedfp);
> >  
> > - if (current->used_math) { /* fp is active.  */
> > + if (owned_fp) { /* fp is active.  */
> >  set_cp0_status(ST0_CU1);
> >  err |= save_fp_context(sc);
> >  last_task_used_math = NULL;
> >  regs->cp0_status &= ~ST0_CU1;
> > - current->used_math = 0;
> >  }
> >  
> >  return err;
> 
> This is absolutely not right.  It's righter than the status quo.  If we
> don't own the FP, you don't save the FP.  Then we can use FP in the
> signal handler, corrupting the process's original floating point
> context.

I only worked on the FP branch emulation fix, and hadn't looked
at the stack signal context problem until now.   If the current thread
has any FPU state, it needs to be saved ahead of signal delivery
for the reasons you suggest above.  I assume that's the reason
for the previous "if (current->used_math)" condition.  If there's
been a problem with FP register corruption in the face of signals
and context switches, it's presumably because, while we're
saving the state in the sigcontext so that it will be restored
on the signal return, we're also clearing last_task_used_math
and curren->used_math.  The former means that if
we invoke the signal handler, it returns, and we will restore the
FP context to the register file.  But if the signal handler 
*doesn't* do any FP, we've got a "dirty" FPU and no owner
for the state, and Bad Things happen.  And the way the current
code is written, if the signal handler does happen to acquire
the FPU, the thread inherits the enable, the register contents,
and an obligation to save the FPU state uselessly on the next
context switch.

I don't understand why there is any manipulation of the
FPU ownership status going on in setup_sigcontext(), nor 
any persistent modification of the Cp0.Status state.  
Shouldn't the code in setup_sigcontext() look more like:

    if(owned_fp) {
        /* Not clear to me how we could have owned_fp 
           true with CU1 off.  Double check this... */
        someaccursednewtemp = regs->cp0_status;
        set_cp0_status(ST0_CU1);
        err |= save_fp_context(sc);
        regs->cp0_status = someaccursednewtemp;
    }

Then, in the symmetric code fragment in restore_sigcontext()

    if(owned_fp) {
        err |= restore_fp_context(sc);
    } else {
        if (current == last_task_used_math) {
        /* signal handler acquired FPU - give it back */
            last_task_used_math = NULL;
            current->used_math = 0;
            clear_cp0_status(ST0_CU1);
        }
    }

Disclaimer:  The above was typed into Outlook Express
and may contain typos, but I think it expresses the idea
well enough for someone to tell me if I'm completely off
base.

            Regards,

            Kevin K.