Re: FP emulator patch

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

[-- Attachment #1: Type: text/plain, Size: 2158 bytes --]

As I wrote last night, it looks to me as if FPU context
management in signals was broken in all versions and
proposed patches that I've seen.  The way I see it is this:

If the current thread "owns" the FPU, we need to save the
FPU state in the sigcontext and restore it on return.  If
the signal handler uses floating point, it already has the
FPU, and can do what it wants with it.  If it doesn't use
the FPU, then we'll save and restore FPU state for
nothing, but better safe than sorry.  In either case,
the current thread retains ownership of the FPU.
There is no reason to muck with the ownership data
or the Cp0.Status.CU1 enables, apart from the
questionable bit of enabling it before the FP context
save in case it wasn't enabled.  I think that should go
away, but I don't have time to test exhastively.  In
any case, CU1 should have it's pre-signal state
going into the signal handler.

If the current thread does *not* own the FPU, we don't
need to save the thread FP state. If the signal handler
does no FP, so much the better, there's nothing to
be done.   If the signal handler uses FP, it will acquire 
the FPU by normal means. The FP context will be saved 
into the thread context of the previous owner, the signalling 
thread will acquire the FPU, and the signal handler will do 
it's FP. On return from the signal, we *must* de-allocate the
FPU and clear the CU1 bit.  If that's done, and the
thread (which had not *owned* the FPU prior to the
signal) starts doing FP again, normal mechanisms
will cause it's FP context to be restored.  If we don't,
it will start exectuing with a bogus FP context.

The code I sketched last night is essentially correct,
though it used a macro that doesn't exist.  I attach a
patch relative to the current OSS repository's signal.c.
The patch includes the stack frame tweak for the FPU
emulator that was part of previous patches, but which
is orthogonal to the problem under discussion.  I have
built a kernel using this code and run 20 simultaneous
copies of the MontaVista "stressor" program with no
problems (though I also had the "v1.5" FPU emulator
code).

            Regards,

            Kevin K. 

[-- Attachment #2: signal.c.patch --]
[-- Type: application/octet-stream, Size: 2041 bytes --]

Index: signal.c
===================================================================
RCS file: /cvs/linux/arch/mips/kernel/signal.c,v
retrieving revision 1.36
diff -u -p -r1.36 signal.c
--- signal.c	2001/06/18 22:43:35	1.36
+++ signal.c	2001/08/16 18:01:58
@@ -221,7 +221,13 @@ restore_sigcontext(struct pt_regs *regs,
 	err |= __get_user(owned_fp, &sc->sc_ownedfp);
 	if (owned_fp) {
 		err |= restore_fp_context(sc);
-		last_task_used_math = current;
+	} else {
+		if (current == last_task_used_math) {
+		/* Signal handler acquired FPU - give it back */
+			last_task_used_math = NULL;
+			current->used_math = 0;
+			regs->cp0_status &= ~ST0_CU1;
+		}
 	}
 
 	return err;
@@ -326,6 +332,7 @@ setup_sigcontext(struct pt_regs *regs, s
 	int owned_fp;
 	int err = 0;
 	u64 reg;
+	u32 tstat;
 
 	err |= __put_user(regs->cp0_epc, &sc->sc_pc);
 	err |= __put_user(regs->cp0_status, &sc->sc_status);
@@ -353,12 +360,12 @@ setup_sigcontext(struct pt_regs *regs, s
 	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.  */
+		tstat = regs->cp0_status;
+		/* This enable should not really be necessary */
 		set_cp0_status(ST0_CU1);
 		err |= save_fp_context(sc);
-		last_task_used_math = NULL;
-		regs->cp0_status &= ~ST0_CU1;
-		current->used_math = 0;
+		regs->cp0_status = tstat;
 	}
 
 	return err;
@@ -374,6 +381,16 @@ get_sigframe(struct k_sigaction *ka, str
 
 	/* Default to using normal stack */
 	sp = regs->regs[29];
+
+#ifdef CONFIG_MIPS_FPU_EMULATOR
+	/*
+	 * FPU emulator may have it's own trampoline active just
+	 * above the user stack, 16-bytes before the next lowest
+	 * 16 byte boundary.  Try to avoid trashing it.
+	 */
+	sp -= 32;	/* 32 ought to be enough, but... */
+
+#endif /* CONFIG_MIPS_FPU_EMULATOR */
 
 	/* This is the X/Open sanctioned signal stack switching.  */
 	if ((ka->sa.sa_flags & SA_ONSTACK) && ! on_sig_stack(sp))