[PATCH] Revert "ckpt/powerpc: handle syscall restart"

[PATCH] Revert "ckpt/powerpc: handle syscall restart"

[Nathan Lynch]

This reverts commit 64dab5da66f4fe212b43272ffa5e7a3403a64deb.

This way of handling syscall restart is incorrect -- it occurs too
early in the restart process, and restore_retval is doing the wrong
thing for powerpc anyway (fixed in a later patch).  While the reverted
patch managed to address the case of system calls with restart blocks,
other restartable system calls behave incorrectly after restart.
---
 arch/powerpc/kernel/checkpoint.c |   25 -------------------------
 1 files changed, 0 insertions(+), 25 deletions(-)

diff --git a/arch/powerpc/kernel/checkpoint.c b/arch/powerpc/kernel/checkpoint.c
index cd384df..2634011 100644
--- a/arch/powerpc/kernel/checkpoint.c
+++ b/arch/powerpc/kernel/checkpoint.c
@@ -307,31 +307,6 @@ static int restore_gprs(const struct ckpt_hdr_cpu *cpu_hdr,
 	regs->orig_gpr3 = cpu_hdr->orig_gpr3;
 
 	regs->msr = sanitize_msr(regs->msr);
-
-	/* The normal servicing of the freezer's fake signal is
-	 * short-circuited by checkpoint/restart.  See
-	 * arch/powerpc/kernel/signal.c::do_signal_pending().
-	 */
-	if (TRAP(regs) != 0x0C00)
-		goto out;
-	if (!(regs->ccr & 0x10000000))
-		goto out;
-
-	switch (regs->gpr[3]) {
-	case ERESTARTNOHAND:
-	case ERESTARTSYS:
-	case ERESTARTNOINTR:
-		regs->gpr[3] = regs->orig_gpr3;
-		break;
-	case ERESTART_RESTARTBLOCK:
-		regs->gpr[0] = __NR_restart_syscall;
-		break;
-	default:
-		goto out;
-	}
-
-	regs->nip -= 4;
-	regs->result = 0;
 out:
 	return rc;
 }
-- 
1.6.0.6

[PATCH] use correct ccr bit for syscall error status

[Nathan Lynch]

The powerpc implementations of syscall_get_error and
syscall_set_return_value should use CCR0:S0 (0x10000000) for testing
and setting syscall error status.  Fortunately these APIs don't seem
to be used at the moment.

Signed-off-by: Nathan Lynch <ntl-e+AXbWqSrlAAvxtiuMwx3w@public.gmane.org>
---
 arch/powerpc/include/asm/syscall.h |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/include/asm/syscall.h b/arch/powerpc/include/asm/syscall.h
index efa7f0b..23913e9 100644
--- a/arch/powerpc/include/asm/syscall.h
+++ b/arch/powerpc/include/asm/syscall.h
@@ -30,7 +30,7 @@ static inline void syscall_rollback(struct task_struct *task,
 static inline long syscall_get_error(struct task_struct *task,
 				     struct pt_regs *regs)
 {
-	return (regs->ccr & 0x1000) ? -regs->gpr[3] : 0;
+	return (regs->ccr & 0x10000000) ? -regs->gpr[3] : 0;
 }
 
 static inline long syscall_get_return_value(struct task_struct *task,
@@ -44,10 +44,10 @@ static inline void syscall_set_return_value(struct task_struct *task,
 					    int error, long val)
 {
 	if (error) {
-		regs->ccr |= 0x1000L;
+		regs->ccr |= 0x10000000L;
 		regs->gpr[3] = -error;
 	} else {
-		regs->ccr &= ~0x1000L;
+		regs->ccr &= ~0x10000000L;
 		regs->gpr[3] = val;
 	}
 }
-- 
1.6.0.6

Re: [PATCH] use correct ccr bit for syscall error status

[Nathan Lynch]

On Fri, 2010-03-12 at 12:26 -0600, Nathan Lynch wrote:
> The powerpc implementations of syscall_get_error and
> syscall_set_return_value should use CCR0:S0 (0x10000000) for testing
> and setting syscall error status.  Fortunately these APIs don't seem
> to be used at the moment.

I'll be sending this to the powerpc maintainer as well, so hopefully we
don't have to carry it for too long.

[PATCH] checkpoint: use syscall_get_error

[Nathan Lynch]

restore_retval() uses the result of syscall_get_return_value() to
determine whether a syscall is to return an error.  This isn't
portable and doesn't work on powerpc.  Instead, use
syscall_get_error(), which encapsulates the arch-specific logic for
determining syscall error state.

Signed-off-by: Nathan Lynch <ntl-e+AXbWqSrlAAvxtiuMwx3w@public.gmane.org>
---
 checkpoint/restart.c |   12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/checkpoint/restart.c b/checkpoint/restart.c
index 0891952..696e4a2 100644
--- a/checkpoint/restart.c
+++ b/checkpoint/restart.c
@@ -1316,7 +1316,8 @@ static int do_restore_coord(struct ckpt_ctx *ctx, pid_t pid)
 static long restore_retval(void)
 {
 	struct pt_regs *regs = task_pt_regs(current);
-	long ret;
+	long syscall_err;
+	long syscall_nr;
 
 	/*
 	 * For the restart, we entered the kernel via sys_restart(),
@@ -1352,13 +1353,14 @@ static long restore_retval(void)
 	 */
 
 	/* needed for all 3 cases: get old value/error/retval */
-	ret = syscall_get_return_value(current, regs);
+	syscall_nr = syscall_get_nr(current, regs);
+	syscall_err = syscall_get_error(current, regs);
 
-	/* if from a syscall and returning error, kick in signal handlig */
-	if (syscall_get_nr(current, regs) >= 0 && ret < 0)
+	/* if from a syscall and returning error, kick in signal handling */
+	if (syscall_nr >= 0 && syscall_err != 0)
 		set_tsk_thread_flag(current, TIF_SIGPENDING);
 
-	return ret;
+	return syscall_get_return_value(current, regs);
 }
 
 long do_restart(struct ckpt_ctx *ctx, pid_t pid, unsigned long flags)
-- 
1.6.0.6

Re: [PATCH] checkpoint: use syscall_get_error

[Nathan Lynch]

On Fri, 2010-03-12 at 12:26 -0600, Nathan Lynch wrote:
> restore_retval() uses the result of syscall_get_return_value() to
> determine whether a syscall is to return an error.  This isn't
> portable and doesn't work on powerpc.  Instead, use
> syscall_get_error(), which encapsulates the arch-specific logic for
> determining syscall error state.

This one seems likely to change behavior on s390; Serge are you able to
check?

Re: [PATCH] checkpoint: use syscall_get_error

[Serge E. Hallyn]

Quoting Nathan Lynch (ntl-e+AXbWqSrlAAvxtiuMwx3w@public.gmane.org):
> On Fri, 2010-03-12 at 12:26 -0600, Nathan Lynch wrote:
> > restore_retval() uses the result of syscall_get_return_value() to
> > determine whether a syscall is to return an error.  This isn't
> > portable and doesn't work on powerpc.  Instead, use
> > syscall_get_error(), which encapsulates the arch-specific logic for
> > determining syscall error state.
> 
> This one seems likely to change behavior on s390; Serge are you able to
> check?
> 

Looks like it should do the right thing, and cr_tests are still passing,
so

Acked-by: Serge Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
Tested-by: Serge Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

thanks,
-serge

Re: [PATCH] Revert "ckpt/powerpc: handle syscall restart"

[Oren Laadan]

All three applied, thanks !

Oren.

Nathan Lynch wrote:
> This reverts commit 64dab5da66f4fe212b43272ffa5e7a3403a64deb.
> 
> This way of handling syscall restart is incorrect -- it occurs too
> early in the restart process, and restore_retval is doing the wrong
> thing for powerpc anyway (fixed in a later patch).  While the reverted
> patch managed to address the case of system calls with restart blocks,
> other restartable system calls behave incorrectly after restart.
> ---
>  arch/powerpc/kernel/checkpoint.c |   25 -------------------------
>  1 files changed, 0 insertions(+), 25 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/checkpoint.c b/arch/powerpc/kernel/checkpoint.c
> index cd384df..2634011 100644
> --- a/arch/powerpc/kernel/checkpoint.c
> +++ b/arch/powerpc/kernel/checkpoint.c
> @@ -307,31 +307,6 @@ static int restore_gprs(const struct ckpt_hdr_cpu *cpu_hdr,
>  	regs->orig_gpr3 = cpu_hdr->orig_gpr3;
>  
>  	regs->msr = sanitize_msr(regs->msr);
> -
> -	/* The normal servicing of the freezer's fake signal is
> -	 * short-circuited by checkpoint/restart.  See
> -	 * arch/powerpc/kernel/signal.c::do_signal_pending().
> -	 */
> -	if (TRAP(regs) != 0x0C00)
> -		goto out;
> -	if (!(regs->ccr & 0x10000000))
> -		goto out;
> -
> -	switch (regs->gpr[3]) {
> -	case ERESTARTNOHAND:
> -	case ERESTARTSYS:
> -	case ERESTARTNOINTR:
> -		regs->gpr[3] = regs->orig_gpr3;
> -		break;
> -	case ERESTART_RESTARTBLOCK:
> -		regs->gpr[0] = __NR_restart_syscall;
> -		break;
> -	default:
> -		goto out;
> -	}
> -
> -	regs->nip -= 4;
> -	regs->result = 0;
>  out:
>  	return rc;
>  }