[PATCH 2.5] FPU

[PATCH 2.5] FPU

[Vivien Chappelier]

Hi,

	At various places in the 2.5 kernel, the fpu is accessed in
kernel mode with CU1 not set, causing an unexpected exception. This patch
makes sure FPU can be accessed by the kernel, though it may only
be a workaround. Any comment from someone with a better understanding of
the FPU access/context switching code?

Vivien.

--- include/asm-mips64/fpu.h	2002-12-11 20:44:20.000000000 +0100
+++ include/asm-mips64/fpu.h	2002-12-11 21:51:44.000000000 +0100
@@ -109,6 +109,7 @@
 
 static inline void save_fp(struct task_struct *tsk)
 {
+	enable_fpu();
 	if (mips_cpu.options & MIPS_CPU_FPU) 
 		_save_fp(tsk);
 }
--- include/asm-mips/fpu.h	2002-12-11 20:44:20.000000000 +0100
+++ include/asm-mips/fpu.h	2002-12-11 21:51:44.000000000 +0100
@@ -109,6 +109,7 @@
 
 static inline void save_fp(struct task_struct *tsk)
 {
+	enable_fpu();
 	if (mips_cpu.options & MIPS_CPU_FPU) 
 		_save_fp(tsk);
 }
--- arch/mips64/kernel/signal.c	2002-11-09 16:10:14.000000000 +0100
+++ arch/mips64/kernel/signal.c	2003-01-14 01:35:42.000000000 +0100
@@ -162,20 +162,19 @@
 
 	err |= __put_user(current->used_math, &sc->sc_used_math);
 
-	if (!current->used_math)
-		goto out;
+	if (current->used_math) {
+
+		/*
+		 * Save FPU state to signal context.
+		 * Signal handler will "inherit" current FPU state.
+		 */
 
-	/*
-	 * Save FPU state to signal context.  Signal handler will "inherit"
-	 * current FPU state.
-	 */
-	if (!is_fpu_owner()) {
 		own_fpu();
 		restore_fp(current);
+
+		err |= save_fp_context(sc);
 	}
-	err |= save_fp_context(sc);
 
-out:
 	return err;
 }
 
--- arch/mips/kernel/signal.c	2002-11-09 16:10:08.000000000 +0100
+++ arch/mips/kernel/signal.c	2003-01-14 01:36:41.000000000 +0100
@@ -313,20 +313,19 @@
 
 	err |= __put_user(current->used_math, &sc->sc_used_math);
 
-	if (!current->used_math)
-		goto out;
+	if (current->used_math) {
+
+		/* 
+		 * Save FPU state to signal context.
+		 * Signal handler will "inherit" current FPU state.
+		 */
 
-	/* 
-	 * Save FPU state to signal context.  Signal handler will "inherit"
-	 * current FPU state.
-	 */
-	if (!is_fpu_owner()) {
 		own_fpu();
 		restore_fp(current);
+
+		err |= save_fp_context(sc);
 	}
-	err |= save_fp_context(sc);
 
-out:
 	return err;
 }
 
--- arch/mips64/kernel/signal32.c	2002-11-09 16:10:14.000000000 +0100
+++ arch/mips64/kernel/signal32.c	2003-01-14 01:34:52.000000000 +0100
@@ -457,20 +430,19 @@
 
 	err |= __put_user(current->used_math, &sc->sc_used_math);
 
-	if (!current->used_math)
-		goto out;
+	if (current->used_math) {
+
+		/* 
+		 * Save FPU state to signal context.
+		 * Signal handler will "inherit" current FPU state.
+		 */
 
-	/* 
-	 * Save FPU state to signal context.  Signal handler will "inherit"
-	 * current FPU state.
-	 */
-	if (!is_fpu_owner()) {
 		own_fpu();
 		restore_fp(current);
+
+		err |= save_fp_context(sc);
 	}
-	err |= save_fp_context(sc);
 
-out:
 	return err;
 }
 

Re: [PATCH 2.5] FPU

[Greg Lindahl]

On Sun, Jan 26, 2003 at 02:58:09AM +0100, Vivien Chappelier wrote:

> 	At various places in the 2.5 kernel, the fpu is accessed in
> kernel mode with CU1 not set, causing an unexpected exception.

Vivien,

One good way to start with 2.5 bugs is to compare the code to the 2.4
kernel. Often you can see places where bugs were fixed in 2.4 but the
fixes were not also made to the equivalent 2.5 code. This will keep
2.4 and 2.5 as close as possible, just like we want to keep the 64-bit
and 32-bit kernels as close as possible.

-- greg

Re: [PATCH 2.5] FPU

[Jun Sun]

On Sun, Jan 26, 2003 at 02:58:09AM +0100, Vivien Chappelier wrote:
> Hi,
> 
> 	At various places in the 2.5 kernel, the fpu is accessed in
> kernel mode with CU1 not set, causing an unexpected exception. This patch
> makes sure FPU can be accessed by the kernel, though it may only
> be a workaround. Any comment from someone with a better understanding of
> the FPU access/context switching code?
> 
> Vivien.
> 
> --- include/asm-mips64/fpu.h	2002-12-11 20:44:20.000000000 +0100
> +++ include/asm-mips64/fpu.h	2002-12-11 21:51:44.000000000 +0100
> @@ -109,6 +109,7 @@
>  
>  static inline void save_fp(struct task_struct *tsk)
>  {
> +	enable_fpu();
>  	if (mips_cpu.options & MIPS_CPU_FPU) 
>  		_save_fp(tsk);
>  }
> --- include/asm-mips/fpu.h	2002-12-11 20:44:20.000000000 +0100
> +++ include/asm-mips/fpu.h	2002-12-11 21:51:44.000000000 +0100
> @@ -109,6 +109,7 @@
>  
>  static inline void save_fp(struct task_struct *tsk)
>  {
> +	enable_fpu();
>  	if (mips_cpu.options & MIPS_CPU_FPU) 
>  		_save_fp(tsk);
>  }

The above two hunks seem to be right.

The following hunks are not right.  The checking for is_fpu_owner()
is meaningful.  If current process is FPU owner, you *don't* want to
do a restore which essentially wipe away the current FPU regsiter values
with an old snapshot of them.

I got a report saying save_fp_context() causes panic when current process
is FPU owner.  That tells me something is wrong in 2.5 that violates
that assumption that FPU is always enabled when current process is the
FPU owner.  I have it in my note and will look into it once I get to 2.5
work.  You are more than to look into it as well.  Meanwhile, just adding
a enable_fpu() to save_fp_context() should let you get rid of the crash.
(although I am afraid there might be other related bugs as this should
not be needed).

Jun

> --- arch/mips64/kernel/signal.c	2002-11-09 16:10:14.000000000 +0100
> +++ arch/mips64/kernel/signal.c	2003-01-14 01:35:42.000000000 +0100
> @@ -162,20 +162,19 @@
>  
>  	err |= __put_user(current->used_math, &sc->sc_used_math);
>  
> -	if (!current->used_math)
> -		goto out;
> +	if (current->used_math) {
> +
> +		/*
> +		 * Save FPU state to signal context.
> +		 * Signal handler will "inherit" current FPU state.
> +		 */
>  
> -	/*
> -	 * Save FPU state to signal context.  Signal handler will "inherit"
> -	 * current FPU state.
> -	 */
> -	if (!is_fpu_owner()) {
>  		own_fpu();
>  		restore_fp(current);
> +
> +		err |= save_fp_context(sc);
>  	}
> -	err |= save_fp_context(sc);
>  
> -out:
>  	return err;
>  }
>  
> --- arch/mips/kernel/signal.c	2002-11-09 16:10:08.000000000 +0100
> +++ arch/mips/kernel/signal.c	2003-01-14 01:36:41.000000000 +0100
> @@ -313,20 +313,19 @@
>  
>  	err |= __put_user(current->used_math, &sc->sc_used_math);
>  
> -	if (!current->used_math)
> -		goto out;
> +	if (current->used_math) {
> +
> +		/* 
> +		 * Save FPU state to signal context.
> +		 * Signal handler will "inherit" current FPU state.
> +		 */
>  
> -	/* 
> -	 * Save FPU state to signal context.  Signal handler will "inherit"
> -	 * current FPU state.
> -	 */
> -	if (!is_fpu_owner()) {
>  		own_fpu();
>  		restore_fp(current);
> +
> +		err |= save_fp_context(sc);
>  	}
> -	err |= save_fp_context(sc);
>  
> -out:
>  	return err;
>  }
>  
> --- arch/mips64/kernel/signal32.c	2002-11-09 16:10:14.000000000 +0100
> +++ arch/mips64/kernel/signal32.c	2003-01-14 01:34:52.000000000 +0100
> @@ -457,20 +430,19 @@
>  
>  	err |= __put_user(current->used_math, &sc->sc_used_math);
>  
> -	if (!current->used_math)
> -		goto out;
> +	if (current->used_math) {
> +
> +		/* 
> +		 * Save FPU state to signal context.
> +		 * Signal handler will "inherit" current FPU state.
> +		 */
>  
> -	/* 
> -	 * Save FPU state to signal context.  Signal handler will "inherit"
> -	 * current FPU state.
> -	 */
> -	if (!is_fpu_owner()) {
>  		own_fpu();
>  		restore_fp(current);
> +
> +		err |= save_fp_context(sc);
>  	}
> -	err |= save_fp_context(sc);
>  
> -out:
>  	return err;
>  }
>  
> 
> 

Re: [PATCH 2.5] FPU

[Ralf Baechle]

On Sat, Jan 25, 2003 at 07:35:29PM -0800, Greg Lindahl wrote:

> One good way to start with 2.5 bugs is to compare the code to the 2.4
> kernel. Often you can see places where bugs were fixed in 2.4 but the
> fixes were not also made to the equivalent 2.5 code. This will keep
> 2.4 and 2.5 as close as possible, just like we want to keep the 64-bit
> and 32-bit kernels as close as possible.

While that is fundamentaly true in reality it's often made much harder
because the 2.4 and 2.5 codebases have diverged so much and will diverge
even more so.

  Ralf

Re: [PATCH 2.5] FPU

[Jun Sun]

On Mon, Jan 27, 2003 at 10:29:29AM -0800, Jun Sun wrote:
> On Sun, Jan 26, 2003 at 02:58:09AM +0100, Vivien Chappelier wrote:
> > Hi,
> > 
> > 	At various places in the 2.5 kernel, the fpu is accessed in
> > kernel mode with CU1 not set, causing an unexpected exception. This patch
> > makes sure FPU can be accessed by the kernel, though it may only
> > be a workaround. Any comment from someone with a better understanding of
> > the FPU access/context switching code?
> > 
> > Vivien.
> > 
> > --- include/asm-mips64/fpu.h	2002-12-11 20:44:20.000000000 +0100
> > +++ include/asm-mips64/fpu.h	2002-12-11 21:51:44.000000000 +0100
> > @@ -109,6 +109,7 @@
> >  
> >  static inline void save_fp(struct task_struct *tsk)
> >  {
> > +	enable_fpu();
> >  	if (mips_cpu.options & MIPS_CPU_FPU) 
> >  		_save_fp(tsk);
> >  }
> > --- include/asm-mips/fpu.h	2002-12-11 20:44:20.000000000 +0100
> > +++ include/asm-mips/fpu.h	2002-12-11 21:51:44.000000000 +0100
> > @@ -109,6 +109,7 @@
> >  
> >  static inline void save_fp(struct task_struct *tsk)
> >  {
> > +	enable_fpu();
> >  	if (mips_cpu.options & MIPS_CPU_FPU) 
> >  		_save_fp(tsk);
> >  }
> 
> The above two hunks seem to be right.
>

There are two places which call save_fp().  Just verified that in
both places current process should be fpu owner and therefore
FPU *should* be enabled.

Basically whenever current process is fpu owner, the FPU should be
enabled.  Apparently something in 2.5 breaks that fundamental assumption.
Will look into it later.

Jun