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

["Mallick, Asit K" <asit.k.mallick@intel.com>]

[-- 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__ */