[patch] ia64: fix potential NaT bit error for sys_pipe().

[patch] ia64: fix potential NaT bit error for sys_pipe().

[David Mosberger]

While working on the syscall exit path again, I realized that in
theory it would be possible for sys_pipe() to mistakenly return a
NaTed value in r9, even though the syscall succeeded.

The problem is due to the fact that sys_pipe() sets r9, but doesn't
clear the matching NaT bit.  In fact, it can't even do so easily since
the NaT bit may be either in ar.unat or in some other location (e.g.,
saved on the stack or a stacked register).

I think the best fix for this is to return r9/r11 directly (not via
pt_regs).  The patch below changes ia64_ret_from_syscall() to save r9
and r11 along with the existing saves for r8 and r10.  There was some
fallout since kernel_thread() used to abuse r9/r11 for its own
purposes.  That was easy to fix though by using r4/r5 instead.

	--david

# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
#   2005/01/18 22:57:59-08:00 davidm@tiger.hpl.hp.com 
#   ia64: Fix potential (though extremely unlikely) NaT bit error for sys_pipe().
# 
# arch/ia64/kernel/sys_ia64.c
#   2005/01/18 22:57:48-08:00 davidm@tiger.hpl.hp.com +16 -11
#   (sys_pipe): Return second file-descriptor directly via r9.
# 
# arch/ia64/kernel/process.c
#   2005/01/18 22:57:48-08:00 davidm@tiger.hpl.hp.com +4 -3
#   (kernel_thread): Pass fn/arg in r4/r5 instead of r9/r11.
#   	For consistency, also set pSys predicate since it will look like
#   	we'll be returning from clone2().
# 
# arch/ia64/kernel/head.S
#   2005/01/18 22:57:48-08:00 davidm@tiger.hpl.hp.com +3 -3
#   (start_kernel_thread): Pick up out0/out1 from r4/r5 instead of r9/r11.
#   	Instead of an endless loop, do "break 0" if sys_exit returns
#   	unexpectedly.
# 
# arch/ia64/kernel/entry.S
#   2005/01/18 22:57:48-08:00 davidm@tiger.hpl.hp.com +5 -2
#   (ia64_ret_from_syscall): Also spill r9 and r11 so their NaT bits are valid.
# 
diff -Nru a/arch/ia64/kernel/entry.S b/arch/ia64/kernel/entry.S
--- a/arch/ia64/kernel/entry.S	2005-01-18 23:01:00 -08:00
+++ b/arch/ia64/kernel/entry.S	2005-01-18 23:01:00 -08:00
@@ -614,8 +614,11 @@
 	adds r2=PT(R8)+16,sp			// r2 = &pt_regs.r8
 	adds r3=PT(R10)+16,sp			// r3 = &pt_regs.r10
 	;;
-.mem.offset 0,0; (p6) st8.spill [r2]=r8	// store return value in slot for r8 and set unat bit
-.mem.offset 8,0; (p6) st8.spill [r3]=r0	// clear error indication in slot for r10 and set unat bit
+.mem.offset 0,0; (p6) st8.spill [r2]=r8,PT(R9)-PT(R8)	// return retval via r8/set unat bit
+.mem.offset 8,0; (p6) st8.spill [r3]=r0,PT(R11)-PT(R10)	// clear error flag via r10/set unat bit
+	;;
+.mem.offset 0,0; (p6)	st8.spill [r2]=r9	// save r9/set unat bit
+.mem.offset 8,0; (p6)	st8.spill [r3]=r11	// save r9/set unat bit
 (p7)	br.cond.spnt handle_syscall_error	// handle potential syscall failure
 END(ia64_ret_from_syscall)
 	// fall through
diff -Nru a/arch/ia64/kernel/head.S b/arch/ia64/kernel/head.S
--- a/arch/ia64/kernel/head.S	2005-01-18 23:01:00 -08:00
+++ b/arch/ia64/kernel/head.S	2005-01-18 23:01:00 -08:00
@@ -885,12 +885,12 @@
 	.save rp, r0				// this is the end of the call-chain
 	.body
 	alloc r2 = ar.pfs, 0, 0, 2, 0
-	mov out0 = r9
-	mov out1 = r11;;
+	mov out0 = r4
+	mov out1 = r5;;
 	br.call.sptk.many rp = kernel_thread_helper;;
 	mov out0 = r8
 	br.call.sptk.many rp = sys_exit;;
-1:	br.sptk.few 1b				// not reached
+	break 0					// not reached
 END(start_kernel_thread)
 
 #ifdef CONFIG_IA64_BRL_EMU
diff -Nru a/arch/ia64/kernel/process.c b/arch/ia64/kernel/process.c
--- a/arch/ia64/kernel/process.c	2005-01-18 23:01:00 -08:00
+++ b/arch/ia64/kernel/process.c	2005-01-18 23:01:00 -08:00
@@ -662,14 +662,15 @@
 	memset(&regs, 0, sizeof(regs));
 	regs.pt.cr_iip = helper_fptr[0];	/* set entry point (IP) */
 	regs.pt.r1 = helper_fptr[1];		/* set GP */
-	regs.pt.r9 = (unsigned long) fn;	/* 1st argument */
-	regs.pt.r11 = (unsigned long) arg;	/* 2nd argument */
+	regs.sw.r4 = (unsigned long) fn;	/* 1st argument */
+	regs.sw.r5 = (unsigned long) arg;	/* 2nd argument */
 	/* Preserve PSR bits, except for bits 32-34 and 37-45, which we can't read.  */
 	regs.pt.cr_ipsr = ia64_getreg(_IA64_REG_PSR) | IA64_PSR_BN;
 	regs.pt.cr_ifs = 1UL << 63;		/* mark as valid, empty frame */
 	regs.sw.ar_fpsr = regs.pt.ar_fpsr = ia64_getreg(_IA64_REG_AR_FPSR);
 	regs.sw.ar_bspstore = (unsigned long) current + IA64_RBS_OFFSET;
-	regs.sw.pr = (1 << PRED_KERNEL_STACK);
+	/* It'll look like new thread is returning from clone(): */
+	regs.sw.pr = (1 << PRED_KERNEL_STACK) | (1 << PRED_SYSCALL);
 	return do_fork(flags | CLONE_VM | CLONE_UNTRACED, 0, &regs.pt, 0, NULL, NULL);
 }
 EXPORT_SYMBOL(kernel_thread);
diff -Nru a/arch/ia64/kernel/sys_ia64.c b/arch/ia64/kernel/sys_ia64.c
--- a/arch/ia64/kernel/sys_ia64.c	2005-01-18 23:01:00 -08:00
+++ b/arch/ia64/kernel/sys_ia64.c	2005-01-18 23:01:00 -08:00
@@ -2,7 +2,7 @@
  * This file contains various system calls that have different calling
  * conventions on different platforms.
  *
- * Copyright (C) 1999-2000, 2002-2003 Hewlett-Packard Co
+ * Copyright (C) 1999-2000, 2002-2003, 2005 Hewlett-Packard Co
  *	David Mosberger-Tang <davidm@hpl.hp.com>
  */
 #include <linux/config.h>
@@ -162,21 +162,26 @@
  * On IA-64, we return the two file descriptors in ret0 and ret1 (r8
  * and r9) as this is faster than doing a copy_to_user().
  */
-asmlinkage long
-sys_pipe (long arg0, long arg1, long arg2, long arg3,
-	  long arg4, long arg5, long arg6, long arg7, long stack)
+struct sysret {
+	long r8;
+	long r9;
+};
+
+asmlinkage struct sysret
+sys_pipe (void)
 {
-	struct pt_regs *regs = (struct pt_regs *) &stack;
+	struct sysret sysret;
 	int fd[2];
-	int retval;
+	long ret;
 
-	retval = do_pipe(fd);
-	if (retval)
+	ret = do_pipe(fd);
+	if (ret)
 		goto out;
-	retval = fd[0];
-	regs->r9 = fd[1];
+	ret = fd[0];
+	sysret.r9 = fd[1];
   out:
-	return retval;
+	sysret.r8 = ret;
+	return sysret;
 }
 
 static inline unsigned long

RE: [patch] ia64: fix potential NaT bit error for sys_pipe().

[David Mosberger]

Tony,

Please disregard the patch named in $SUBJECT.  It appears to be
breaking light-weight syscalls.  That's rather odd, but since Rohit
has some better ideas on how to fix this issue, I'd rather drop this
patch for the time being (the root problem is still there and IMHO,
needs to be fixed urgently).

	--david

RE: [patch] ia64: fix potential NaT bit error for sys_pipe().

[Seth, Rohit]

David Mosberger <mailto:davidm@napali.hpl.hp.com> wrote on Thursday,
January 20, 2005 9:53 AM:

> Tony,
> 
> Please disregard the patch named in $SUBJECT.  It appears to be
> breaking light-weight syscalls.  That's rather odd, but since Rohit
> has some better ideas on how to fix this issue, I'd rather drop this
> patch for the time being (the root problem is still there and IMHO,
> needs to be fixed urgently).
> 
> 	--david

I will send out the first rev of patch today to address the
functionality issue.  This patch will not worry about exporting the NAT
bits in r8-r11 to user.  So, the ld.fills will be replaced by simple ld.


Though, as per the disussions with David, there is scope of further
optimization in the way we are saving/restoring r8-r11 in this code.
This will probably need little more analysis to ensure best
placement/scheduling of new code. Will send that later...

Thanks, 
-rohit

RE: [patch] ia64: fix potential NaT bit error for sys_pipe().

[Seth, Rohit]

Seth, Rohit <> wrote on Thursday, January 20, 2005 11:01 AM:

> David Mosberger <mailto:davidm@napali.hpl.hp.com> wrote on Thursday,
> January 20, 2005 9:53 AM:
> 
>> Tony,
>> 
>> Please disregard the patch named in $SUBJECT.  It appears to be
>> breaking light-weight syscalls.  That's rather odd, but since Rohit
>> has some better ideas on how to fix this issue, I'd rather drop this
>> patch for the time being (the root problem is still there and IMHO,
>> needs to be fixed urgently). 
>> 
>> 	--david
> 
> I will send out the first rev of patch today to address the
> functionality issue.  This patch will not worry about exporting the
> NAT bits in r8-r11 to user.  So, the ld.fills will be replaced by
> simple ld. 
> 

We are also thinking of reseting the mfh bit on every syscall entry as
an optimization.

Comments?

Thanks, rohit

RE: [patch] ia64: fix potential NaT bit error for sys_pipe().

[David Mosberger]

>>>>> On Thu, 20 Jan 2005 15:22:59 -0800, "Seth, Rohit" <rohit.seth@intel.com> said:

  Rohit> We are also thinking of reseting the mfh bit on every syscall
  Rohit> entry as an optimization.

  Rohit> Comments?

Sounds OK with me.  We should delete this comment from process.c then,
too:

        /*
         * NOTE: The calling convention considers all floating point
         * registers in the high partition (fph) to be scratch.  Since
         * the only way to get to this point is through a system call,
         * we know that the values in fph are all dead.  Hence, there
         * is no need to inherit the fph state from the parent to the
         * child and all we have to do is to make sure that
         * IA64_THREAD_FPH_VALID is cleared in the child.
         *
         * XXX We could push this optimization a bit further by
         * clearing IA64_THREAD_FPH_VALID on ANY system call.
         * However, it's not clear this is worth doing.  Also, it
         * would be a slight deviation from the normal Linux system
         * call behavior where scratch registers are preserved across
         * system calls (unless used by the system call itself).
         */

and change:

#define THREAD_FLAGS_TO_CLEAR (IA64_THREAD_FPH_VALID | IA64_THREAD_DBG_VALID \
                               | IA64_THREAD_PM_VALID)

to:

#define THREAD_FLAGS_TO_CLEAR (IA64_THREAD_DBG_VALID | IA64_THREAD_PM_VALID)

	--david

RE: [patch] ia64: fix potential NaT bit error for sys_pipe().

[Chen, Kenneth W]

>>>>> On Thu, 20 Jan 2005 15:22:59 -0800, "Seth, Rohit" <rohit.seth@intel.com> said:
>
>   Rohit> We are also thinking of reseting the mfh bit on every syscall
>   Rohit> entry as an optimization.
>
>   Rohit> Comments?

David Mosberger wrote on Thursday, January 20, 2005 3:51 PM
> Sounds OK with me.  We should delete this comment from process.c then,
> ....
> and change:
>
> #define THREAD_FLAGS_TO_CLEAR (IA64_THREAD_FPH_VALID | IA64_THREAD_DBG_VALID \
>                                | IA64_THREAD_PM_VALID)
>
> to:
>
> #define THREAD_FLAGS_TO_CLEAR (IA64_THREAD_DBG_VALID | IA64_THREAD_PM_VALID)

We would rather not to change THREAD_FLAGS_TO_CLEAR definition.  mfh bit in child
pt_regs is cleared right above where the comment is.  I think it is better as is
so that child thread fph partition stays invalidated.

- Ken

RE: [patch] ia64: fix potential NaT bit error for sys_pipe().

[Chen, Kenneth W]

David Mosberger  wrote on Thursday, January 20, 2005 9:53 AM:
> Please disregard the patch named in $SUBJECT.  It appears to be
> breaking light-weight syscalls.  That's rather odd, but since Rohit
> has some better ideas on how to fix this issue, I'd rather drop this
> patch for the time being (the root problem is still there and IMHO,
> needs to be fixed urgently).
>

Seth, Rohit wrote on Thursday, January 20, 2005 11:01 AM
> I will send out the first rev of patch today to address the
> functionality issue.  This patch will not worry about exporting the NAT
> bits in r8-r11 to user.  So, the ld.fills will be replaced by simple ld.
>
> Though, as per the disussions with David, there is scope of further
> optimization in the way we are saving/restoring r8-r11 in this code.
> This will probably need little more analysis to ensure best
> placement/scheduling of new code. Will send that later...


Here is the first part of the patch that Rohit mentioned earlier.

--- 1.1/arch/ia64/kernel/entry.S	Mon Oct 18 14:53:44 2004
+++ edited/arch/ia64/kernel/entry.S	Thu Jan 20 15:21:50 2005
@@ -614,8 +614,8 @@
 	adds r2=PT(R8)+16,sp			// r2 = &pt_regs.r8
 	adds r3=PT(R10)+16,sp			// r3 = &pt_regs.r10
 	;;
-.mem.offset 0,0; (p6) st8.spill [r2]=r8	// store return value in slot for r8 and set unat bit
-.mem.offset 8,0; (p6) st8.spill [r3]=r0	// clear error indication in slot for r10 and set unat bit
+.mem.offset 0,0; (p6) st8 [r2]=r8		// store return value in slot for r8
+.mem.offset 8,0; (p6) st8 [r3]=r0		// clear error indication in slot for r10
 (p7)	br.cond.spnt handle_syscall_error	// handle potential syscall failure
 END(ia64_ret_from_syscall)
 	// fall through
@@ -708,12 +708,12 @@
 (p6)	br.cond.spnt .work_pending
 	;;
 	// start restoring the state saved on the kernel stack (struct pt_regs):
-	ld8.fill r8=[r16],16
-	ld8.fill r9=[r17],16
+	ld8 r8=[r16],16
+	ld8 r9=[r17],16
 	mov f6ð		// clear f6
 	;;
-	ld8.fill r10=[r16],16
-	ld8.fill r11=[r17],16
+	ld8 r10=[r16],16
+	ld8 r11=[r17],16
 	mov f7ð		// clear f7
 	;;
 	ld8 r29=[r16],16	// load cr.ipsr

Signed-off-by: Ken Chen <kenneth.w.chen@intel.com>
Signed-off-by: Rohit Seth <rohit.seth@intel.com>

RE: [patch] ia64: fix potential NaT bit error for sys_pipe().

[David Mosberger]

>>>>> On Fri, 21 Jan 2005 10:48:46 -0800, "Chen, Kenneth W" <kenneth.w.chen@intel.com> said:

  Ken> +.mem.offset 0,0; (p6) st8 [r2]=r8		// store return value in slot for r8
  Ken> +.mem.offset 8,0; (p6) st8 [r3]=r0		// clear error indication in slot for r10

Without the .mem.offset, please?

	--david

RE: [patch] ia64: fix potential NaT bit error for sys_pipe().

[David Mosberger]

>>>>> On Thu, 20 Jan 2005 19:32:48 -0800, "Chen, Kenneth W" <kenneth.w.chen@intel.com> said:

  Ken> We would rather not to change THREAD_FLAGS_TO_CLEAR definition.
  Ken> mfh bit in child pt_regs is cleared right above where the
  Ken> comment is.  I think it is better as is so that child thread
  Ken> fph partition stays invalidated.

Sorry, I was a bit in a rush when I typed that mail and made some
errors as a result.  I think what you propose is reasonable, since
just clearing PSR.MFH will take care of avoiding needless
context-switching of f32-f127.  However, I think it'd still be
worthwhile to update the comment in copy_thread() to reflect this
thinking (basically, the paragraph starting with "XXX" would have to
be updated).

	--david

RE: [patch] ia64: fix potential NaT bit error for sys_pipe().

[Chen, Kenneth W]

>>>>> On Thu, 20 Jan 2005 19:32:48 -0800, Chen, Kenneth W
  Ken> We would rather not to change THREAD_FLAGS_TO_CLEAR definition.
  Ken> mfh bit in child pt_regs is cleared right above where the
  Ken> comment is.  I think it is better as is so that child thread
  Ken> fph partition stays invalidated.

David Mosberger Monday, January 24, 2005 11:18 AM
> Sorry, I was a bit in a rush when I typed that mail and made some
> errors as a result.  I think what you propose is reasonable, since
> just clearing PSR.MFH will take care of avoiding needless
> context-switching of f32-f127.  However, I think it'd still be
> worthwhile to update the comment in copy_thread() to reflect this
> thinking (basically, the paragraph starting with "XXX" would have to
> be updated).

Absolutely agreeing with updating the comments.  Also in a way, what
you said earlier also make sense.  Since clearing psr.mfh is only half
of the optimization.  That would only optimize away the storing part
of context switch.  However, if later we take a dfh fault, if thread fph
valid bit is on, we end up loading from memory instead of a simple zeroing.
So I better do both, clearing psr.mfh and fph valid bit in thread.flags.

- Ken

RE: [patch] ia64: fix potential NaT bit error for sys_pipe().

[David Mosberger]

>>>>> On Mon, 24 Jan 2005 11:32:27 -0800, "Chen, Kenneth W" <kenneth.w.chen@intel.com> said:

  Ken> Absolutely agreeing with updating the comments.  Also in a way,
  Ken> what you said earlier also make sense.  Since clearing psr.mfh
  Ken> is only half of the optimization.  That would only optimize
  Ken> away the storing part of context switch.  However, if later we
  Ken> take a dfh fault, if thread fph valid bit is on, we end up
  Ken> loading from memory instead of a simple zeroing.  So I better
  Ken> do both, clearing psr.mfh and fph valid bit in thread.flags.

OK, I had assumed that you had tested this already.  Clearing the
FPH_VALID bit in the syscall path does potentially increase the
syscall overhead as it requires a read-modify-write.  Cache-wise we
should be OK, since the neighboring "on_ustack" byte is being touched
anyhow.  If the code turns out to be difficult to schedule in the
syscall path, an alternate option would be to make FPH_VALID a
separate byte member, right next to on_ustack, so it can be cleared
with "st1 [rXX]=r0".

	--david