Re: [PATCH 2.5] FPU

[Jun Sun <jsun@mvista.com>]

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