[Linux-ia64] High fpu register corruption

[Linux-ia64] High fpu register corruption

[Andreas Schwab]

When a process clears the psr.mfh bit after using the high fpu registers
and then starts using them again it can corrupt the fpu state of another
process.  In order for this to happen there must be some context switches
inbetween (thanks to Chris Mason for tracking this down):

Processes start with psr.dfh=1, IA64_THREAD_FPH_VALID not set

proc A                               proc B
------                               ------
use fph reg
-> trap, mfh=1, dfh=0
-> fpu_owner = proc A
clear mfh (rum)

context switch
-> no registers saved
-> IA64_THREAD_FPH_VALID not set

                                     start running
                                     use fph reg
                                     -> trap, mfh=1, dfh=0
                                     -> fpu_owner = proc B

                                     context switch
                                     -> save registers, mfh=0
                                     -> set IA64_THREAD_FPH_VALID

continue running
-> IA64_THREAD_FPH_VALID not set
-> dfh not modified

modify fph reg
-> no trap
-> fpu_owner still proc B
clear mfh (rum)

context switch

                                     continue running
                                     -> fpu_owner still proc B
                                     -> dfh=0, mfh=0

At this point proc B uses the fph registers that were modified by proc A.
The problem is that dfh was not set for proc A although
IA64_THREAD_FPH_VALID wasn't set and proc A is not the fpu owner.  This
patch fixes the problem:

--- linux-2.4/include/asm-ia64/system.h.~1~	2003-05-07 15:44:44.000000000 +0200
+++ linux-2.4/include/asm-ia64/system.h	2003-05-07 15:31:47.000000000 +0200
@@ -281,7 +281,8 @@ extern void ia64_load_extra (struct task
 		} else {						\
 			ia64_psr(ia64_task_regs(next))->dfh = 1;	\
 		}							\
-	}								\
+	} else if (ia64_get_fpu_owner() != next)			\
+		ia64_psr(ia64_task_regs(next))->dfh = 1;		\
 	__switch_to(prev,next,last);					\
   } while (0)
 #else


Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux AG, Deutschherrnstr. 15-19, D-90429 Nürnberg
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

RE: [Linux-ia64] High fpu register corruption

[Mallick, Asit K]

Andreas,

The high FP save and restore in the context switch makes the assumption that user will not be modifying the psr.mfh and it will be only updated by the hardware. Without this assumption we can not optimize the FP save/restore for SMP systems (this patch will not cover all cases). If application wants the current high fpu state to be preserved then it should will not be able to modify the psr.mfh.

What kind of applications are trying to modify the mfh?

Thanks,
Asit
 

> -----Original Message-----
> From: Andreas Schwab [mailto:schwab@suse.de]
> Sent: Thursday, May 08, 2003 7:16 AM
> To: linux-ia64@linuxia64.org
> Cc: Chris Mason
> Subject: [Linux-ia64] High fpu register corruption
> 
> When a process clears the psr.mfh bit after using the high fpu registers
> and then starts using them again it can corrupt the fpu state of another
> process.  In order for this to happen there must be some context switches
> inbetween (thanks to Chris Mason for tracking this down):
> 
> Processes start with psr.dfh=1, IA64_THREAD_FPH_VALID not set
> 
> proc A                               proc B
> ------                               ------
> use fph reg
> -> trap, mfh=1, dfh=0
> -> fpu_owner = proc A
> clear mfh (rum)
> 
> context switch
> -> no registers saved
> -> IA64_THREAD_FPH_VALID not set
> 
>                                      start running
>                                      use fph reg
>                                      -> trap, mfh=1, dfh=0
>                                      -> fpu_owner = proc B
> 
>                                      context switch
>                                      -> save registers, mfh=0
>                                      -> set IA64_THREAD_FPH_VALID
> 
> continue running
> -> IA64_THREAD_FPH_VALID not set
> -> dfh not modified
> 
> modify fph reg
> -> no trap
> -> fpu_owner still proc B
> clear mfh (rum)
> 
> context switch
> 
>                                      continue running
>                                      -> fpu_owner still proc B
>                                      -> dfh=0, mfh=0
> 
> At this point proc B uses the fph registers that were modified by proc A.
> The problem is that dfh was not set for proc A although
> IA64_THREAD_FPH_VALID wasn't set and proc A is not the fpu owner.  This
> patch fixes the problem:
> 
> --- linux-2.4/include/asm-ia64/system.h.~1~	2003-05-07
> 15:44:44.000000000 +0200
> +++ linux-2.4/include/asm-ia64/system.h	2003-05-07 15:31:47.000000000
> +0200
> @@ -281,7 +281,8 @@ extern void ia64_load_extra (struct task
>  		} else {						\
>  			ia64_psr(ia64_task_regs(next))->dfh = 1;	\
>  		}							\
> -	}								\
> +	} else if (ia64_get_fpu_owner() != next)			\
> +		ia64_psr(ia64_task_regs(next))->dfh = 1;		\
>  	__switch_to(prev,next,last);					\
>    } while (0)
>  #else
> 
> 
> Andreas.
> 
> --
> Andreas Schwab, SuSE Labs, schwab@suse.de
> SuSE Linux AG, Deutschherrnstr. 15-19, D-90429 Nürnberg
> Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
> "And now for something completely different."
> 
> _______________________________________________
> Linux-IA64 mailing list
> Linux-IA64@linuxia64.org
> http://lists.linuxia64.org/lists/listinfo/linux-ia64

RE: [Linux-ia64] High fpu register corruption

[Chris Mason]

On Thu, 2003-05-08 at 12:33, Mallick, Asit K wrote:
> Andreas,
> 
> The high FP save and restore in the context switch makes the
> assumption that user will not be modifying the psr.mfh and it will be
> only updated by the hardware. Without this assumption we can not
> optimize the FP save/restore for SMP systems (this patch will not
> cover all cases). If application wants the current high fpu state to
> be preserved then it should will not be able to modify the psr.mfh.
> 
> What kind of applications are trying to modify the mfh?

The ia64 ssl source has hand optimized asm that clears mfh, but that
doesn't really matter.  What matters is that any application can do it,
so it has security implications.

-chris

RE: [Linux-ia64] High fpu register corruption

[David Mosberger]

>>>>> On Thu, 8 May 2003 09:33:55 -0700, "Mallick, Asit K" <asit.k.mallick@intel.com> said:

  Asit> Andreas, The high FP save and restore in the context switch
  Asit> makes the assumption that user will not be modifying the
  Asit> psr.mfh and it will be only updated by the hardware.

Not a chance.  We can't rely on the application doing the Right Thing.
In fact, having an application clear psr.mfh makes tons of sense,
when it knows it's done using mfh.

  Asit> Without this assumption we can not optimize the FP
  Asit> save/restore for SMP systems (this patch will not cover all
  Asit> cases). If application wants the current high fpu state to be
  Asit> preserved then it should will not be able to modify the
  Asit> psr.mfh.

  Asit> What kind of applications are trying to modify the mfh?

OpenSSL does (really: some underlying crypto code).

I think you're misunderstanding the problem though: the problem is
that application A clears psr.mfh and application B gets its fph state
corrupted.

So, really, this is just a bug that needs fixing.  It won't affect the
rest of the lazy fph save/restore logic.

	--david

Re: [Linux-ia64] High fpu register corruption

[David Mosberger]

>>>>> On Thu, 08 May 2003 16:16:13 +0200, Andreas Schwab <schwab@suse.de> said:

  Andreas> When a process clears the psr.mfh bit after using the high
  Andreas> fpu registers and then starts using them again it can
  Andreas> corrupt the fpu state of another process.  In order for
  Andreas> this to happen there must be some context switches
  Andreas> inbetween (thanks to Chris Mason for tracking this down):

Ah, _now_ it makes sense.  I got a similar bug report yesterday, but
it claimed the _old_ (2.4.19) context switch was breaking and the
new one (2.4.20) was fine.  When I looked at the old code, I couldn't
find anythign wrong with it.

  Andreas> +	} else if (ia64_get_fpu_owner() != next)			\
  Andreas> +		ia64_psr(ia64_task_regs(next))->dfh = 1;		\

I suspect what we really want to do here is something along the lines
of:

  Andreas> +	ia64_psr(ia64_task_regs(next))->dfh = (ia64_get_fpu_owner() != next);		\

This expresses the invariant we're after: the next thread has DFH set
unless it owns the FPH partition.  IIRC, this is what the UP code does
already.

	--david

RE: [Linux-ia64] High fpu register corruption

[Mallick, Asit K]

David,

The save part in the context switch also needs to be fixed and we will
take a look at this.
Thanks,
Asit


> -----Original Message-----
> From: David Mosberger [mailto:davidm@napali.hpl.hp.com]
> Sent: Thursday, May 08, 2003 10:03 AM
> To: Andreas Schwab
> Cc: linux-ia64@linuxia64.org; Chris Mason
> Subject: Re: [Linux-ia64] High fpu register corruption
> 
> >>>>> On Thu, 08 May 2003 16:16:13 +0200, Andreas Schwab
<schwab@suse.de>
> said:
> 
>   Andreas> When a process clears the psr.mfh bit after using the high
>   Andreas> fpu registers and then starts using them again it can
>   Andreas> corrupt the fpu state of another process.  In order for
>   Andreas> this to happen there must be some context switches
>   Andreas> inbetween (thanks to Chris Mason for tracking this down):
> 
> Ah, _now_ it makes sense.  I got a similar bug report yesterday, but
> it claimed the _old_ (2.4.19) context switch was breaking and the
> new one (2.4.20) was fine.  When I looked at the old code, I couldn't
> find anythign wrong with it.
> 
>   Andreas> +	} else if (ia64_get_fpu_owner() != next)
\
>   Andreas> +		ia64_psr(ia64_task_regs(next))->dfh = 1;
\
> 
> I suspect what we really want to do here is something along the lines
> of:
> 
>   Andreas> +	ia64_psr(ia64_task_regs(next))->dfh =
> (ia64_get_fpu_owner() != next);		\
> 
> This expresses the invariant we're after: the next thread has DFH set
> unless it owns the FPH partition.  IIRC, this is what the UP code does
> already.
> 
> 	--david
> 
> _______________________________________________
> Linux-IA64 mailing list
> Linux-IA64@linuxia64.org
> http://lists.linuxia64.org/lists/listinfo/linux-ia64

RE: [Linux-ia64] High fpu register corruption

[David Mosberger]

>>>>> On Thu, 8 May 2003 10:14:20 -0700, "Mallick, Asit K" <asit.k.mallick@intel.com> said:

  Asit> David, The save part in the context switch also needs to be
  Asit> fixed and we will take a look at this.

Are you sure?  It looks fine to me.  What's the scenario you're
worried about?  Note that if an application clears psr.mfh it says: I
no longer care about the state, next time I touch it, give me whatever
contents you have (either all zeroes or an earlier snapshot of fph;
never the contents of fph from another thread, of course).

Note that the fph registers are all scratch registers.  Are you
worried about someone implementing a non-standard calling convention?
Even then I think an application that clears psr.mfh is saying that
the fph contents is no longer relevant, so I think we'd still be OK.

	--david

RE: [Linux-ia64] High fpu register corruption (PATCH)

[Mallick, Asit K]

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


Attached is a patch to fix the high FPU corruption (thanks to Andreas
and Chris). This patch also unifies the FPU save/restore for SMP and UP
and also fixes another problem with FPH save/restore path due to an
unaligned fault. Sorry for the delay.

This patch based on 2.4.21-rc2 (latest bktree).

Thanks,
Asit



> From: Mallick, Asit K
> Sent: Thursday, May 08, 2003 10:14 AM
> To: davidm@hpl.hp.com; Andreas Schwab
> Cc: linux-ia64@linuxia64.org; Chris Mason
> Subject: RE: [Linux-ia64] High fpu register corruption
> 
> David,
> 
> The save part in the context switch also needs to be fixed and we will
> take a look at this.
> Thanks,
> Asit
> 
> 
> > -----Original Message-----
> > From: David Mosberger [mailto:davidm@napali.hpl.hp.com]
> > Sent: Thursday, May 08, 2003 10:03 AM
> > To: Andreas Schwab
> > Cc: linux-ia64@linuxia64.org; Chris Mason
> > Subject: Re: [Linux-ia64] High fpu register corruption
> >
> > >>>>> On Thu, 08 May 2003 16:16:13 +0200, Andreas Schwab
> <schwab@suse.de>
> > said:
> >
> >   Andreas> When a process clears the psr.mfh bit after using the
high
> >   Andreas> fpu registers and then starts using them again it can
> >   Andreas> corrupt the fpu state of another process.  In order for
> >   Andreas> this to happen there must be some context switches
> >   Andreas> inbetween (thanks to Chris Mason for tracking this down):
> >
> > Ah, _now_ it makes sense.  I got a similar bug report yesterday, but
> > it claimed the _old_ (2.4.19) context switch was breaking and the
> > new one (2.4.20) was fine.  When I looked at the old code, I
couldn't
> > find anythign wrong with it.
> >
> >   Andreas> +	} else if (ia64_get_fpu_owner() != next)
> \
> >   Andreas> +		ia64_psr(ia64_task_regs(next))->dfh = 1;
> \
> >
> > I suspect what we really want to do here is something along the
lines
> > of:
> >
> >   Andreas> +	ia64_psr(ia64_task_regs(next))->dfh =
> > (ia64_get_fpu_owner() != next);		\
> >
> > This expresses the invariant we're after: the next thread has DFH
set
> > unless it owns the FPH partition.  IIRC, this is what the UP code
does
> > already.
> >
> > 	--david
> >
> > _______________________________________________
> > Linux-IA64 mailing list
> > Linux-IA64@linuxia64.org
> > http://lists.linuxia64.org/lists/listinfo/linux-ia64
> 
> _______________________________________________
> Linux-IA64 mailing list
> Linux-IA64@linuxia64.org
> http://lists.linuxia64.org/lists/listinfo/linux-ia64

[-- Attachment #2: lazyfp1_7-24.txt --]
[-- Type: text/plain, Size: 9619 bytes --]

# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher.
# This patch includes the following deltas:
#	           ChangeSet	1.1130  -> 1.1133 
#	arch/ia64/kernel/setup.c	1.12    -> 1.14   
#	include/asm-ia64/processor.h	1.19    -> 1.21   
#	include/asm-ia64/system.h	1.18    -> 1.19   
#	arch/ia64/kernel/process.c	1.18    -> 1.19   
#	arch/ia64/kernel/signal.c	1.14    -> 1.15   
#	arch/ia64/kernel/traps.c	1.13    -> 1.15   
#	arch/ia64/kernel/ptrace.c	1.17    -> 1.19   
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 03/05/21	adsharma@linux-t08.(none)	1.1131
# lazyfp1.5-24
# --------------------------------------------
# 03/05/21	adsharma@linux-t08.(none)	1.1132
# fph corruption fix
# --------------------------------------------
# 03/05/22	adsharma@linux-t08.(none)	1.1133
# UP fixes
# --------------------------------------------
#
diff -Nru a/arch/ia64/kernel/process.c b/arch/ia64/kernel/process.c
--- a/arch/ia64/kernel/process.c	Thu May 22 14:25:13 2003
+++ b/arch/ia64/kernel/process.c	Thu May 22 14:25:13 2003
@@ -333,6 +333,7 @@
 #	define THREAD_FLAGS_TO_SET	0
 	p->thread.flags = ((current->thread.flags & ~THREAD_FLAGS_TO_CLEAR)
 			   | THREAD_FLAGS_TO_SET);
+	ia64_drop_fpu(p);	/* don't pick up stale state from a CPU's fph */
 #ifdef CONFIG_IA32_SUPPORT
 	/*
 	 * If we're cloning an IA32 task then save the IA32 extra
@@ -517,11 +518,7 @@
 {
 	/* drop floating-point and debug-register state if it exists: */
 	current->thread.flags &= ~(IA64_THREAD_FPH_VALID | IA64_THREAD_DBG_VALID);
-
-#ifndef CONFIG_SMP
-	if (ia64_get_fpu_owner() == current)
-		ia64_set_fpu_owner(0);
-#endif
+	ia64_drop_fpu(current);
 }
 
 #ifdef CONFIG_PERFMON
@@ -559,10 +556,7 @@
 void
 exit_thread (void)
 {
-#ifndef CONFIG_SMP
-	if (ia64_get_fpu_owner() == current)
-		ia64_set_fpu_owner(0);
-#endif
+	ia64_drop_fpu(current);
 #ifdef CONFIG_PERFMON
        /* stop monitoring */
 	if (current->thread.pfm_context)
diff -Nru a/arch/ia64/kernel/ptrace.c b/arch/ia64/kernel/ptrace.c
--- a/arch/ia64/kernel/ptrace.c	Thu May 22 14:25:13 2003
+++ b/arch/ia64/kernel/ptrace.c	Thu May 22 14:25:13 2003
@@ -597,16 +597,11 @@
 ia64_flush_fph (struct task_struct *task)
 {
 	struct ia64_psr *psr = ia64_psr(ia64_task_regs(task));
-#ifdef CONFIG_SMP
-	struct task_struct *fpu_owner = current;
-#else
-	struct task_struct *fpu_owner = ia64_get_fpu_owner();
-#endif
 
-	if (task == fpu_owner && psr->mfh) {
+	if (ia64_is_local_fpu_owner(task) && psr->mfh) {
 		psr->mfh = 0;
-		ia64_save_fpu(&task->thread.fph[0]);
 		task->thread.flags |= IA64_THREAD_FPH_VALID;
+		ia64_save_fpu(&task->thread.fph[0]);
 	}
 }
 
@@ -628,10 +623,7 @@
 		task->thread.flags |= IA64_THREAD_FPH_VALID;
 		memset(&task->thread.fph, 0, sizeof(task->thread.fph));
 	}
-#ifndef CONFIG_SMP
-	if (ia64_get_fpu_owner() == task)
-		ia64_set_fpu_owner(0);
-#endif
+	ia64_drop_fpu(task);
 	psr->dfh = 1;
 }
 
diff -Nru a/arch/ia64/kernel/setup.c b/arch/ia64/kernel/setup.c
--- a/arch/ia64/kernel/setup.c	Thu May 22 14:25:13 2003
+++ b/arch/ia64/kernel/setup.c	Thu May 22 14:25:13 2003
@@ -619,6 +619,8 @@
 	/* Clear the stack memory reserved for pt_regs: */
 	memset(ia64_task_regs(current), 0, sizeof(struct pt_regs));
 
+	ia64_set_kr(IA64_KR_FPU_OWNER, 0);
+
 	/*
 	 * Initialize default control register to defer all speculative faults.  The
 	 * kernel MUST NOT depend on a particular setting of these bits (in other words,
@@ -629,10 +631,6 @@
 	 */
 	ia64_set_dcr(  IA64_DCR_DP | IA64_DCR_DK | IA64_DCR_DX | IA64_DCR_DR
 		     | IA64_DCR_DA | IA64_DCR_DD | IA64_DCR_LC);
-#ifndef CONFIG_SMP
-	ia64_set_fpu_owner(0);
-#endif
-
 	atomic_inc(&init_mm.mm_count);
 	current->active_mm = &init_mm;
 
diff -Nru a/arch/ia64/kernel/signal.c b/arch/ia64/kernel/signal.c
--- a/arch/ia64/kernel/signal.c	Thu May 22 14:25:13 2003
+++ b/arch/ia64/kernel/signal.c	Thu May 22 14:25:13 2003
@@ -142,8 +142,12 @@
 
 		__copy_from_user(current->thread.fph, &sc->sc_fr[32], 96*16);
 		psr->mfh = 0;	/* drop signal handler's fph contents... */
-		if (!psr->dfh)
+		if (!psr->dfh) {
 			__ia64_load_fpu(current->thread.fph);
+			ia64_set_local_fpu_owner(current);
+		} else {
+			ia64_drop_fpu(current);
+		}
 	}
 	return err;
 }
diff -Nru a/arch/ia64/kernel/traps.c b/arch/ia64/kernel/traps.c
--- a/arch/ia64/kernel/traps.c	Thu May 22 14:25:13 2003
+++ b/arch/ia64/kernel/traps.c	Thu May 22 14:25:13 2003
@@ -245,9 +245,9 @@
 	psr->dfh = 0;
 #ifndef CONFIG_SMP
 	{
-		struct task_struct *fpu_owner = ia64_get_fpu_owner();
+		struct task_struct *fpu_owner = (struct task_struct *)ia64_get_kr(IA64_KR_FPU_OWNER);
 
-		if (fpu_owner == current)
+		if (ia64_is_local_fpu_owner(current))
 			return;
 
 		if (fpu_owner)
@@ -255,7 +255,7 @@
 
 	}
 #endif /* !CONFIG_SMP */
-	ia64_set_fpu_owner(current);
+	ia64_set_local_fpu_owner(current);
 	if ((current->thread.flags & IA64_THREAD_FPH_VALID) != 0) {
 		__ia64_load_fpu(current->thread.fph);
 		psr->mfh = 0;
diff -Nru a/include/asm-ia64/processor.h b/include/asm-ia64/processor.h
--- a/include/asm-ia64/processor.h	Thu May 22 14:25:13 2003
+++ b/include/asm-ia64/processor.h	Thu May 22 14:25:13 2003
@@ -302,7 +302,8 @@
 	INIT_THREAD_PM					\
 	{0, },				/* dbr */	\
 	{0, },				/* ibr */	\
-	{{{{0}}}, }			/* fph */	\
+	{{{{0}}}, },			/* fph */	\
+	-1				/* last_fph_cpu*/	\
 }
 
 #define start_thread(regs,new_ip,new_sp) do {							\
@@ -426,17 +427,16 @@
 	}
 }
 
-static inline struct task_struct *
-ia64_get_fpu_owner (void)
-{
-	return (struct task_struct *) ia64_get_kr(IA64_KR_FPU_OWNER);
-}
+#define ia64_is_local_fpu_owner(t)	(((t)->thread.last_fph_cpu == smp_processor_id()) && \
+			((t) == (struct task_struct *)ia64_get_kr(IA64_KR_FPU_OWNER)))		
 
-static inline void
-ia64_set_fpu_owner (struct task_struct *t)
-{
-	ia64_set_kr(IA64_KR_FPU_OWNER, (unsigned long) t);
-}
+#define ia64_set_local_fpu_owner(t)	do {			\
+	t->thread.last_fph_cpu = smp_processor_id();		\
+	ia64_set_kr(IA64_KR_FPU_OWNER, (unsigned long) (t));	\
+} while(0)
+
+/* Mark the fph partition of task T as being invalid on all CPUs.  */
+#define ia64_drop_fpu(t)	((t)->thread.last_fph_cpu = -1)
 
 extern void __ia64_init_fpu (void);
 extern void __ia64_save_fpu (struct ia64_fpreg *fph);
diff -Nru a/include/asm-ia64/system.h b/include/asm-ia64/system.h
--- a/include/asm-ia64/system.h	Thu May 22 14:25:13 2003
+++ b/include/asm-ia64/system.h	Thu May 22 14:25:13 2003
@@ -244,13 +244,16 @@
 # define PERFMON_IS_SYSWIDE() (0)
 #endif
 
+#define IA64_HAS_EXTRA_STATE(t)                                                        \
+	((t)->thread.flags & (IA64_THREAD_DBG_VALID|IA64_THREAD_PM_VALID)       \
+	|| IS_IA32_PROCESS(ia64_task_regs(t)) || PERFMON_IS_SYSWIDE())
+
 #define __switch_to(prev,next,last) do {						\
-	if (((prev)->thread.flags & (IA64_THREAD_DBG_VALID|IA64_THREAD_PM_VALID))	\
-	    || IS_IA32_PROCESS(ia64_task_regs(prev)) || PERFMON_IS_SYSWIDE())	\
-		ia64_save_extra(prev);							\
-	if (((next)->thread.flags & (IA64_THREAD_DBG_VALID|IA64_THREAD_PM_VALID))	\
-	    || IS_IA32_PROCESS(ia64_task_regs(next)) || PERFMON_IS_SYSWIDE())	\
+	if (IA64_HAS_EXTRA_STATE(prev))							\
+		ia64_save_extra(prev); 							\
+	if (IA64_HAS_EXTRA_STATE(next))							\
 		ia64_load_extra(next);							\
+	ia64_psr(ia64_task_regs(next))->dfh = !ia64_is_local_fpu_owner(next);		\
 	(last) = ia64_switch_to((next));						\
 } while (0)
 
@@ -260,35 +263,21 @@
 #define arch_consoles_callable() (cpu_online_map & (1UL << smp_processor_id()))
 
 /*
- * In the SMP case, we save the fph state when context-switching
- * away from a thread that modified fph.  This way, when the thread
- * gets scheduled on another CPU, the CPU can pick up the state from
- * task->thread.fph, avoiding the complication of having to fetch
- * the latest fph state from another CPU.
+ * In the SMP case, we save the fph state when context-switching away from a thread that
+ * modified fph.  This way, when the thread gets scheduled on another CPU, the CPU can
+ * pick up the state from task->thread.fph, avoiding the complication of having to fetch
+ * the latest fph state from another CPU.  In other words: eager save, lazy restore.
  */
 # define switch_to(prev,next,last) do {					\
-	if (ia64_psr(ia64_task_regs(prev))->mfh) {			\
+  	if (ia64_psr(ia64_task_regs(prev))->mfh) {			\
 		ia64_psr(ia64_task_regs(prev))->mfh = 0;		\
 		(prev)->thread.flags |= IA64_THREAD_FPH_VALID;		\
 		__ia64_save_fpu((prev)->thread.fph);			\
-		(prev)->thread.last_fph_cpu = smp_processor_id();	\
-	}								\
-	if ((next)->thread.flags & IA64_THREAD_FPH_VALID) {		\
-		if (((next)->thread.last_fph_cpu == smp_processor_id()) \
-		    && (ia64_get_fpu_owner() == next)) {		\
-			ia64_psr(ia64_task_regs(next))->dfh = 0;	\
-			ia64_psr(ia64_task_regs(next))->mfh = 0;	\
-		} else {						\
-			ia64_psr(ia64_task_regs(next))->dfh = 1;	\
-		}							\
 	}								\
-	__switch_to(prev,next,last);					\
+	__switch_to(prev, next, last);					\
   } while (0)
 #else
-# define switch_to(prev,next,last) do {					\
-	ia64_psr(ia64_task_regs(next))->dfh = (ia64_get_fpu_owner() != (next));	\
-	__switch_to(prev,next,last);					\
-} while (0)
+# define switch_to(prev,next,last) __switch_to(prev,next,last)
 #endif
 
 #endif /* __KERNEL__ */

Re: [Linux-ia64] High fpu register corruption (PATCH)

[Bjorn Helgaas]

On Thursday 22 May 2003 3:55 pm, Mallick, Asit K wrote:
> Attached is a patch to fix the high FPU corruption (thanks to Andreas
> and Chris). This patch also unifies the FPU save/restore for SMP and UP
> and also fixes another problem with FPH save/restore path due to an
> unaligned fault.

I'm comparing 2.4 (including your patch) and David's current
2.5 BK tree.  I'd like to keep them as close as possible, but
I see several clearly gratuitous differences (like whitespace)
and some that aren't obvious to me.

arch/ia64/kernel/ptrace.c:
	2.4 calls ia64_drop_fpu() from ia64_sync_fph().
	2.5 calls ia64_drop_fpu() from ia64_flush_fph().
	Is there a reason for the difference?

arch/ia64/kernel/setup.c:
	cpu_init(): 2.4 added ia64_set_kr() and removed
	ia64_set_fpu_owner(); 2.5 doesn't have this change.
	This looks like at least partly a bug in 2.5 --
	ia64_set_fpu_owner is called under "#ifndef CONFIG_SMP"
	but not defined anywhere.

arch/ia64/kernel/signal.c:
	restore_sigcontext() changes look equivalent, but have
	gratuitous differences from 2.5 (reversed sense of test,
	comment)

arch/ia64/kernel/traps.c:
	disabled_fph_fault(): 2.4 uses ia64_is_local_fpu_owner();
	2.5 uses "fpu_owner = current".  Is there a reason for
	this?

	Also gratuitous whitespace differences.

include/asm-ia64/processor.h:
	ia64_is_local_fpu_owner(), ia64_set_local_fpu_owner():
	these look functionally equivalent in 2.4 and 2.5.
	Can they be made identical?

include/asm-ia64/system.h:
	IA64_HAS_EXTRA_STATE() and switch_to() appear to be
	identical in 2.4 and 2.5 except for whitespace changes.
	Can they be made identical?

Any insight would be appreciated!

Bjorn

Re: [Linux-ia64] High fpu register corruption (PATCH)

[David Mosberger]

>>>>> On Wed, 28 May 2003 21:53:13 -0600, Bjorn Helgaas <bjorn_helgaas@hp.com> said:

  Bjorn> I'm comparing 2.4 (including your patch) and David's current
  Bjorn> 2.5 BK tree.  I'd like to keep them as close as possible, but
  Bjorn> I see several clearly gratuitous differences (like
  Bjorn> whitespace) and some that aren't obvious to me.

  Bjorn> arch/ia64/kernel/ptrace.c: 2.4 calls ia64_drop_fpu() from
  Bjorn> ia64_sync_fph().  2.5 calls ia64_drop_fpu() from
  Bjorn> ia64_flush_fph().  Is there a reason for the difference?

Which tree are you looking at? to-linus-2.5 or linux-ia64-2.5?  I have
gotten quite lazy in pushing the former, since the only reason now to
do a push there is when I want Linus to do a pull.  The latter tree is
much more active.

	--david

Re: [Linux-ia64] High fpu register corruption (PATCH)

[Bjorn Helgaas]

> Which tree are you looking at? to-linus-2.5 or linux-ia64-2.5?  I have
> gotten quite lazy in pushing the former, since the only reason now to
> do a push there is when I want Linus to do a pull.  The latter tree is
> much more active.

The latter (linux-ia64-2.5).  But ... argh!  I've been doing "bk changes -R"
to watch for your updates, and saw none.  BUT, I just did a "bk pull"
and got a lot of them.

OK, ignore my questions about ptrace.c and setup.c.  Questions still
open:

arch/ia64/kernel/signal.c:
        restore_sigcontext() changes look equivalent, but have
        gratuitous differences from 2.5 (reversed sense of test,
        comment)

arch/ia64/kernel/traps.c:
        Gratuitous whitespace differences.

include/asm-ia64/processor.h:
        ia64_is_local_fpu_owner(), ia64_set_local_fpu_owner():
        these look functionally equivalent in 2.4 and 2.5.
        Can they be made identical?

include/asm-ia64/system.h:
        IA64_HAS_EXTRA_STATE() and switch_to() appear to be
        identical in 2.4 and 2.5 except for whitespace changes.
        Can they be made identical?

Bjorn

Re: [Linux-ia64] High fpu register corruption (PATCH)

[David Mosberger]

>>>>> On Wed, 28 May 2003 22:25:37 -0600, Bjorn Helgaas <bjorn_helgaas@hp.com> said:

  Bjorn> OK, ignore my questions about ptrace.c and setup.c.  Questions still
  Bjorn> open:

  Bjorn> arch/ia64/kernel/signal.c:
  Bjorn> restore_sigcontext() changes look equivalent, but have
  Bjorn> gratuitous differences from 2.5 (reversed sense of test,
  Bjorn> comment)

  Bjorn> arch/ia64/kernel/traps.c:
  Bjorn> Gratuitous whitespace differences.

  Bjorn> include/asm-ia64/processor.h:
  Bjorn> ia64_is_local_fpu_owner(), ia64_set_local_fpu_owner():
  Bjorn> these look functionally equivalent in 2.4 and 2.5.
  Bjorn> Can they be made identical?

  Bjorn> include/asm-ia64/system.h:
  Bjorn> IA64_HAS_EXTRA_STATE() and switch_to() appear to be
  Bjorn> identical in 2.4 and 2.5 except for whitespace changes.
  Bjorn> Can they be made identical?

Not sure if those are intentional.  I of course would prefer if 2.5
were followed. ;-) Asit?

	--daivd

RE: [Linux-ia64] High fpu register corruption (PATCH)

[Mallick, Asit K]

Yes, it can be same as 2.5.
Thanks,
Asit


> -----Original Message-----
> From: David Mosberger [mailto:davidm@napali.hpl.hp.com] 
> Sent: Wednesday, May 28, 2003 9:41 PM
> To: Bjorn Helgaas
> Cc: davidm@hpl.hp.com; David Mosberger; Mallick, Asit K; 
> linux-ia64@linuxia64.org; Chris Mason; dstownse@us.ibm.com
> Subject: Re: [Linux-ia64] High fpu register corruption (PATCH)
> 
> 
> >>>>> On Wed, 28 May 2003 22:25:37 -0600, Bjorn Helgaas 
> <bjorn_helgaas@hp.com> said:
> 
>   Bjorn> OK, ignore my questions about ptrace.c and setup.c.  
> Questions still
>   Bjorn> open:
> 
>   Bjorn> arch/ia64/kernel/signal.c:
>   Bjorn> restore_sigcontext() changes look equivalent, but have
>   Bjorn> gratuitous differences from 2.5 (reversed sense of test,
>   Bjorn> comment)
> 
>   Bjorn> arch/ia64/kernel/traps.c:
>   Bjorn> Gratuitous whitespace differences.
> 
>   Bjorn> include/asm-ia64/processor.h:
>   Bjorn> ia64_is_local_fpu_owner(), ia64_set_local_fpu_owner():
>   Bjorn> these look functionally equivalent in 2.4 and 2.5.
>   Bjorn> Can they be made identical?
> 
>   Bjorn> include/asm-ia64/system.h:
>   Bjorn> IA64_HAS_EXTRA_STATE() and switch_to() appear to be
>   Bjorn> identical in 2.4 and 2.5 except for whitespace changes.
>   Bjorn> Can they be made identical?
> 
> Not sure if those are intentional.  I of course would prefer if 2.5
> were followed. ;-) Asit?
> 
> 	--daivd
>