[PATCH 0/5] user-cr fixes and generic stack API

[PATCH 0/5] user-cr fixes and generic stack API

[Nathan Lynch]

Hi Oren,

Here's a patch series against ckpt-v19-rc2 that fixes a couple of
minor issues for powerpc and adds a generic stack API ("genstack").

Nathan Lynch (5):
  catch attempts to build clone_ppc_.S in 64-bit mode
  user-cr: align powerpc sp for eclone, clean up __eclone
  user-cr: add generic stack API
  user-cr: use genstack API in restart.c
  user-cr: use genstack API in nsexeccwp

 Makefile     |    4 +-
 clone_ppc.c  |   12 ++++--
 clone_ppc_.S |   44 +++++++++++--------------
 genstack.c   |  101 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 genstack.h   |   22 ++++++++++++
 nsexeccwp.c  |   11 +++---
 restart.c    |   42 ++++++++++++------------
 7 files changed, 180 insertions(+), 56 deletions(-)
 create mode 100644 genstack.c
 create mode 100644 genstack.h

[PATCH 1/5] catch attempts to build clone_ppc_.S in 64-bit mode

[Nathan Lynch]

It will build fine, but runtime is another matter.

Signed-off-by: Nathan Lynch <ntl-e+AXbWqSrlAAvxtiuMwx3w@public.gmane.org>
---
 clone_ppc_.S |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/clone_ppc_.S b/clone_ppc_.S
index b736e13..ebc07f3 100644
--- a/clone_ppc_.S
+++ b/clone_ppc_.S
@@ -11,6 +11,10 @@
 #include <asm/unistd.h>
 #include "powerpc_asm.h"
 
+#ifdef __powerpc64__
+#error This code is 32-bit only!
+#endif
+
 #ifndef __NR_eclone
 #define __NR_eclone     323
 #endif
-- 
1.6.0.6

[PATCH 2/5] user-cr: align powerpc sp for eclone, clean up __eclone

[Nathan Lynch]

Using genstack in nsexeccwp exposed this bug.  The child_stack value
being passed to the kernel via clone_args was not properly aligned,
causing the child function to access the guard page.

And it turns out we needn't pass child_sp [r4] to the __eclone wrapper
at all, so remove it.

Finally, the flags value as passed to __eclone in r4 is not used after the
system call, so there is no need to save it in a nonvolatile register
(r29).

Signed-off-by: Nathan Lynch <ntl-e+AXbWqSrlAAvxtiuMwx3w@public.gmane.org>
---
 clone_ppc.c  |   12 ++++++++----
 clone_ppc_.S |   40 ++++++++++++++++------------------------
 2 files changed, 24 insertions(+), 28 deletions(-)

diff --git a/clone_ppc.c b/clone_ppc.c
index 47b8290..f047a9e 100644
--- a/clone_ppc.c
+++ b/clone_ppc.c
@@ -25,7 +25,6 @@
 #include "eclone.h"
 
 extern int __eclone(int (*fn)(void *arg),
-		    void *child_sp,
 		    int flags,
 		    void *fn_arg,
 		    struct clone_args *args,
@@ -39,9 +38,15 @@ int eclone(int (*fn)(void *), void *fn_arg, int clone_flags_low,
 	unsigned long child_sp;
 	int newpid;
 
+	/* The stack pointer for the child is communicated to the
+	 * kernel via clone_args.child_stack, and to the __eclone
+	 * assembly wrapper via the child_sp argument [r4].  So we
+	 * need to align child_sp here and ensure that the wrapper and
+	 * the kernel receive the same value.
+	 */
 	if (clone_args->child_stack)
-		child_sp = clone_args->child_stack +
-			clone_args->child_stack_size - 1;
+		child_sp = (clone_args->child_stack +
+			    clone_args->child_stack_size - 1) & ~0xf;
 	else
 		child_sp = 0;
 
@@ -50,7 +55,6 @@ int eclone(int (*fn)(void *), void *fn_arg, int clone_flags_low,
 	my_args.child_stack_size = 0;
 
 	newpid = __eclone(fn,
-			  (void *)child_sp,
 			  clone_flags_low,
 			  fn_arg,
 			  &my_args,
diff --git a/clone_ppc_.S b/clone_ppc_.S
index ebc07f3..b7e50e3 100644
--- a/clone_ppc_.S
+++ b/clone_ppc_.S
@@ -20,12 +20,11 @@
 #endif
 
 /* int [r3] eclone(int (*fn)(void *arg) [r3],
- *                          void *child_sp [r4],
- *                          int flags [r5],
- *                          void *fn_arg [r6],
- *                          struct clone_args *args [r7],
- *                          size_t args_size [r8],
- *                          pid_t *pids [r9]);
+ *                          int flags [r4],
+ *                          void *fn_arg [r5],
+ *                          struct clone_args *args [r6],
+ *                          size_t args_size [r7],
+ *                          pid_t *pids [r8]);
  * Creates a child task with the pids specified by pids.
  * Returns to parent only, child execution and exit is handled here.
  * On error, returns negated errno.  On success, returns the pid of the child
@@ -40,25 +39,18 @@ __eclone:
 	/* Set up parent's stack frame. */
 	stwu	r1,-32(r1)
 
-	/* Save non-volatiles (r28-r31) which we plan to use. */
-	stmw	r28,16(r1)
+	/* Save non-volatiles (r30-r31) which we plan to use. */
+	stmw	r30,16(r1)
 
-	/* Set up child's stack frame. */
-	clrrwi	r4,r4,4
-	li	r0,0
-	stw	r0,-16(r4)
-
-	/* Save fn, stack pointer, flags, and fn_arg across system call. */
-	mr	r28,r3
-	mr	r29,r4
-	mr	r30,r5
-	mr	r31,r6
+	/* Save fn and fn_arg across system call. */
+	mr	r30,r3
+	mr	r31,r5
 
 	/* Set up arguments for system call. */
-	mr	r3,r5	/* flags */
-	mr	r4,r7	/* clone_args */
-	mr	r5,r8	/* clone_args' size */
-	mr	r6,r9	/* pids */
+	mr	r3,r4	/* flags */
+	mr	r4,r6	/* clone_args */
+	mr	r5,r7	/* clone_args' size */
+	mr	r6,r8	/* pids */
 
 	/* Do the system call */
 	li	r0,__NR_eclone
@@ -70,7 +62,7 @@ __eclone:
 	bne	cr1,eclone_parent
 
 	/* Child. Call fn. */
-	mtctr	r28
+	mtctr	r30
 	mr	r3,r31
 	bctrl
 
@@ -80,7 +72,7 @@ __eclone:
 
 eclone_parent:
 	/* Restore non-volatiles. */
-	lmw	r28,16(r1)
+	lmw	r30,16(r1)
 
 	addi	r1,r1,32
 
-- 
1.6.0.6

[PATCH 3/5] user-cr: add generic stack API

[Nathan Lynch]

A few problems exist with the management of stacks for clone/eclone in
the restart utility.

* Stacks are allocated from the heap via malloc.  This means any
  mishandling of the the stack setup by the platform eclone code can
  result in silent data corruption.  At a time when architecture
  support for eclone is an ongoing effort, this seems unwise...

* Determining a suitable stack pointer to pass to clone(2) is
  platform-dependent; we don't handle e.g. parisc where the stack
  grows upwards.

* There are at least two cases (ckpt_coordinator_pidns and
  ckpt_fork_feeder) where the stack pointer provided to
  clone(2) is outside of the region allocated.

To address these issues, introduce an opaque "genstack" (generic
stack) type and accessor functions.  Stacks are allocated via mmap()
and guard pages are instantiated at both ends.  The accessor functions
provide suitable values for passing to clone(2) and eclone(2) (via
struct clone_args in the latter case).

Signed-off-by: Nathan Lynch <ntl-e+AXbWqSrlAAvxtiuMwx3w@public.gmane.org>
---
 genstack.c |  101 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 genstack.h |   22 +++++++++++++
 2 files changed, 123 insertions(+), 0 deletions(-)
 create mode 100644 genstack.c
 create mode 100644 genstack.h

diff --git a/genstack.c b/genstack.c
new file mode 100644
index 0000000..c552d04
--- /dev/null
+++ b/genstack.c
@@ -0,0 +1,101 @@
+#include <stdbool.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <sys/mman.h>
+
+#include "genstack.h"
+
+struct genstack {
+	void *addr;  /* Return value from mmap. Accessors must account
+		      * for guard pages at both ends.
+		      */
+	size_t size; /* Size of the region as passed to mmap. */
+};
+
+static size_t page_size(void)
+{
+	return sysconf(_SC_PAGESIZE);
+}
+
+/* Allocate a stack region with guard pages (PROT_NONE) at both ends.
+ * The size requested will be rounded up to the system page size.
+ * Callers must check for errors with genstack_err().
+ */
+struct genstack *genstack_alloc(size_t sz)
+{
+	struct genstack *stk;
+	int mmap_flags = MAP_PRIVATE | MAP_ANONYMOUS;
+	int mmap_prot = PROT_READ | PROT_WRITE;
+	void *addr;
+
+	/* Align requested size up to page boundary */
+	sz = (sz + page_size() - 1) & (~(page_size() - 1));
+
+	if (sz == 0)
+		return NULL;
+
+	stk = malloc(sizeof(*stk));
+	if (!stk)
+		return NULL;
+
+	/* Make space for guard pages */
+	sz += page_size() * 2;
+
+	addr = mmap(NULL, sz, mmap_prot, mmap_flags, -1, 0);
+	if (addr == MAP_FAILED) {
+		free(stk);
+		return NULL;
+	}
+
+	/* guard pages */
+	mprotect(addr, page_size(), PROT_NONE);
+	mprotect(addr + sz - page_size(), page_size(), PROT_NONE);
+
+	stk->addr = addr;
+	stk->size = sz;
+
+	return stk;
+}
+
+/* Unmap the stack region. */
+void genstack_release(struct genstack *stk)
+{
+	munmap(stk->addr, stk->size);
+	free(stk);
+}
+
+/* Return the size of the usable stack region.  Suitable for providing
+ * the child_stack_size value for struct clone_args.
+ */
+size_t genstack_size(const struct genstack *stk)
+{
+	return stk->size - (page_size() * 2);
+}
+
+/* Return the lowest usable address in the stack region.  Suitable for
+ * providing the child_stack value for struct clone_args.
+ */
+void *genstack_base(const struct genstack *stk)
+{
+	return stk->addr + page_size();
+}
+
+/* Return a suitable stack pointer value for passing to clone(2),
+ * accounting for platform differences in stack behavior.
+ */
+void *genstack_sp(const struct  genstack *stk)
+{
+#ifdef __hppa__
+	/*
+	 * If stack grows upwards, return the lowest address between
+	 * the guard pages.
+	 */
+	return stk->addr + page_size();
+#else
+	/*
+	 * Otherwise return the highest address between the guard pages.
+	 * glibc's clone wrappers apply any necessary alignment.
+	 */
+	return (stk->addr + stk->size - page_size()) - 1;
+#endif
+}
diff --git a/genstack.h b/genstack.h
new file mode 100644
index 0000000..6970d99
--- /dev/null
+++ b/genstack.h
@@ -0,0 +1,22 @@
+#ifndef _GENSTACK_H_
+#define _GENSTACK_H_
+
+#include <stdbool.h>
+#include <stdint.h>
+
+/* A generic stack API. */
+
+struct genstack;
+typedef struct genstack *genstack;
+
+extern struct genstack *genstack_alloc(size_t size);
+
+extern void genstack_release(struct genstack *stack);
+
+extern size_t genstack_size(const struct genstack *stack);
+
+extern void *genstack_base(const struct genstack *stack);
+
+extern void *genstack_sp(const struct genstack *stack);
+
+#endif
-- 
1.6.0.6

[PATCH 4/5] user-cr: use genstack API in restart.c

[Nathan Lynch]

Fixes a couple of off-by-one bugs and makes it more portable.

Signed-off-by: Nathan Lynch <ntl-e+AXbWqSrlAAvxtiuMwx3w@public.gmane.org>
---
 Makefile  |    2 +-
 restart.c |   42 +++++++++++++++++++++---------------------
 2 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/Makefile b/Makefile
index 1a37844..b1485cb 100644
--- a/Makefile
+++ b/Makefile
@@ -37,7 +37,7 @@ restart: CFLAGS += -D__REENTRANT -pthread
 
 # eclone() is architecture specific
 ifneq ($(SUBARCH),)
-restart: clone_$(SUBARCH).o
+restart: clone_$(SUBARCH).o genstack.o
 restart: CFLAGS += -DARCH_HAS_ECLONE
 nsexeccwp: clone_$(SUBARCH).o
 nsexeccwp: CFLAGS += -DARCH_HAS_ECLONE
diff --git a/restart.c b/restart.c
index f0fa348..9d1c1d1 100644
--- a/restart.c
+++ b/restart.c
@@ -36,6 +36,7 @@
 #include <linux/checkpoint_hdr.h>
 
 #include "eclone.h"
+#include "genstack.h"
 
 static char usage_str[] =
 "usage: restart [opts]\n"
@@ -1001,9 +1002,10 @@ static int ckpt_coordinator_status(struct ckpt_ctx *ctx)
 
 static int ckpt_coordinator_pidns(struct ckpt_ctx *ctx)
 {
-	void *stk = NULL;
 	pid_t coord_pid;
 	int copy, ret;
+	genstack stk;
+	void *sp;
 
 	ckpt_dbg("forking coordinator in new pidns\n");
 
@@ -1018,18 +1020,18 @@ static int ckpt_coordinator_pidns(struct ckpt_ctx *ctx)
 		return -1;
 	}
 
-	stk = malloc(PTHREAD_STACK_MIN);
+	stk = genstack_alloc(PTHREAD_STACK_MIN);
 	if (!stk) {
-		perror("coordinator stack malloc");
+		perror("coordinator genstack_alloc");
 		return -1;
 	}
-	stk += PTHREAD_STACK_MIN;
+	sp = genstack_sp(stk);
 
 	copy = ctx->args->copy_status;
 	ctx->args->copy_status = 1;
 
-	coord_pid = clone(__ckpt_coordinator, stk, CLONE_NEWPID|SIGCHLD, ctx);
-	free(stk - PTHREAD_STACK_MIN);
+	coord_pid = clone(__ckpt_coordinator, sp, CLONE_NEWPID|SIGCHLD, ctx);
+	genstack_release(stk);
 	if (coord_pid < 0) {
 		perror("clone coordinator");
 		return coord_pid;
@@ -1883,17 +1885,16 @@ int ckpt_fork_stub(void *data)
 static pid_t ckpt_fork_child(struct ckpt_ctx *ctx, struct task *child)
 {
 	struct clone_args clone_args;
-	void *stack;
+	genstack stk;
 	unsigned long flags = SIGCHLD;
-	size_t stack_sz = PTHREAD_STACK_MIN;
 	size_t nr_pids = 1;
 	pid_t pid = 0;
 
 	ckpt_dbg("forking child vpid %d flags %#x\n", child->pid, child->flags);
 
-	stack = malloc(stack_sz);
-	if (!stack) {
-		perror("stack malloc");
+	stk = genstack_alloc(PTHREAD_STACK_MIN);
+	if (!stk) {
+		perror("ckpt_fork_child genstack_alloc");
 		return -1;
 	}
 
@@ -1921,19 +1922,19 @@ static pid_t ckpt_fork_child(struct ckpt_ctx *ctx, struct task *child)
 		child->real_parent = _getpid();
 
 	memset(&clone_args, 0, sizeof(clone_args));
-	clone_args.child_stack = (unsigned long)stack;
-	clone_args.child_stack_size = stack_sz;
+	clone_args.child_stack = (unsigned long)genstack_base(stk);
+	clone_args.child_stack_size = genstack_size(stk);
 	clone_args.nr_pids = nr_pids;
 
 	pid = eclone(ckpt_fork_stub, child, flags, &clone_args, &pid);
 	if (pid < 0) {
 		perror("eclone");
-		free(stack);
+		genstack_release(stk);
 		return -1;
 	}
 
 	if (!(child->flags & TASK_THREAD))
-		free(stack);
+		genstack_release(stk);
 
 	ckpt_dbg("forked child vpid %d (asked %d)\n", pid, child->pid);
 	return pid;
@@ -1950,7 +1951,7 @@ static pid_t ckpt_fork_child(struct ckpt_ctx *ctx, struct task *child)
  */
 static int ckpt_fork_feeder(struct ckpt_ctx *ctx)
 {
-	void *stack;
+	genstack stk;
 	pid_t pid;
 
 	if (pipe(ctx->pipe_feed)) {
@@ -1969,14 +1970,13 @@ static int ckpt_fork_feeder(struct ckpt_ctx *ctx)
 	 * this may interfere with the restart.
 	 */
 
-	stack = malloc(PTHREAD_STACK_MIN);
-	if (!stack) {
-		perror("stack malloc");
+	stk = genstack_alloc(PTHREAD_STACK_MIN);
+	if (!stk) {
+		perror("ckpt_fork_feeder genstack_alloc");
 		return -1;
 	}
-	stack += PTHREAD_STACK_MIN;
 
-	pid = clone(ckpt_do_feeder, stack,
+	pid = clone(ckpt_do_feeder, genstack_sp(stk),
 		    CLONE_THREAD | CLONE_SIGHAND | CLONE_VM, ctx);
 	if (pid < 0) {
 		perror("feeder thread");
-- 
1.6.0.6

[PATCH 5/5] user-cr: use genstack API in nsexeccwp

[Nathan Lynch]

Signed-off-by: Nathan Lynch <ntl-e+AXbWqSrlAAvxtiuMwx3w@public.gmane.org>
---
 Makefile    |    2 +-
 nsexeccwp.c |   11 ++++++-----
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/Makefile b/Makefile
index b1485cb..35188f9 100644
--- a/Makefile
+++ b/Makefile
@@ -39,7 +39,7 @@ restart: CFLAGS += -D__REENTRANT -pthread
 ifneq ($(SUBARCH),)
 restart: clone_$(SUBARCH).o genstack.o
 restart: CFLAGS += -DARCH_HAS_ECLONE
-nsexeccwp: clone_$(SUBARCH).o
+nsexeccwp: clone_$(SUBARCH).o genstack.o
 nsexeccwp: CFLAGS += -DARCH_HAS_ECLONE
 endif
 
diff --git a/nsexeccwp.c b/nsexeccwp.c
index d3853f0..859b6c3 100644
--- a/nsexeccwp.c
+++ b/nsexeccwp.c
@@ -18,6 +18,7 @@
 
 #include "clone.h"
 #include "eclone.h"
+#include "genstack.h"
 
 extern pid_t getpgid(pid_t pid);
 extern pid_t getsid(pid_t pid);
@@ -270,17 +271,17 @@ int main(int argc, char *argv[])
 
 	if (use_clone) {
 		struct clone_args clone_args;
-		int stacksize = 4*getpagesize();
-		void *stack = malloc(stacksize);
+		size_t stacksize = 4 * sysconf(_SC_PAGESIZE);
+		genstack stack = genstack_alloc(stacksize);
 
 		if (!stack) {
-			perror("malloc");
+			perror("genstack_alloc");
 			return -1;
 		}
 
 		memset(&clone_args, 0, sizeof(clone_args));
-		clone_args.child_stack = (unsigned long)stack;
-		clone_args.child_stack_size = stacksize;
+		clone_args.child_stack = (unsigned long)genstack_base(stack);
+		clone_args.child_stack_size = genstack_size(stack);
 		clone_args.nr_pids = nr_pids;
 
 		printf("about to clone with %lx\n", flags);
-- 
1.6.0.6

Re: [PATCH 0/5] user-cr fixes and generic stack API

[Oren Laadan]

Alright, pulled.

Thanks,

Oren.

Nathan Lynch wrote:
> Hi Oren,
> 
> Here's a patch series against ckpt-v19-rc2 that fixes a couple of
> minor issues for powerpc and adds a generic stack API ("genstack").
> 
> Nathan Lynch (5):
>   catch attempts to build clone_ppc_.S in 64-bit mode
>   user-cr: align powerpc sp for eclone, clean up __eclone
>   user-cr: add generic stack API
>   user-cr: use genstack API in restart.c
>   user-cr: use genstack API in nsexeccwp
> 
>  Makefile     |    4 +-
>  clone_ppc.c  |   12 ++++--
>  clone_ppc_.S |   44 +++++++++++--------------
>  genstack.c   |  101 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  genstack.h   |   22 ++++++++++++
>  nsexeccwp.c  |   11 +++---
>  restart.c    |   42 ++++++++++++------------
>  7 files changed, 180 insertions(+), 56 deletions(-)
>  create mode 100644 genstack.c
>  create mode 100644 genstack.h
> 
> 

Re: [PATCH 0/5] user-cr fixes and generic stack API

[Serge E. Hallyn]

Quoting Nathan Lynch (ntl-e+AXbWqSrlAAvxtiuMwx3w@public.gmane.org):
> Hi Oren,
> 
> Here's a patch series against ckpt-v19-rc2 that fixes a couple of
> minor issues for powerpc and adds a generic stack API ("genstack").
> 
> Nathan Lynch (5):
>   catch attempts to build clone_ppc_.S in 64-bit mode
>   user-cr: align powerpc sp for eclone, clean up __eclone
>   user-cr: add generic stack API
>   user-cr: use genstack API in restart.c
>   user-cr: use genstack API in nsexeccwp

For the moment (to keep in sync with my kernel tree) these are
queued at git://git.sr71.net/~hallyn/user-cr.git.  I intend to
keep that tree in sync with
git://git.kernel.org/pub/scm/linux/kernel/git/sergeh/linux-cr.git#cr-next

-serge

Re: [PATCH 2/5] user-cr: align powerpc sp for eclone, clean up __eclone

[Matt Helsley]

On Fri, Dec 04, 2009 at 11:02:28PM -0600, Nathan Lynch wrote:
> Using genstack in nsexeccwp exposed this bug.  The child_stack value
> being passed to the kernel via clone_args was not properly aligned,
> causing the child function to access the guard page.
> 
> And it turns out we needn't pass child_sp [r4] to the __eclone wrapper
> at all, so remove it.
> 
> Finally, the flags value as passed to __eclone in r4 is not used after the
> system call, so there is no need to save it in a nonvolatile register
> (r29).
> 
> Signed-off-by: Nathan Lynch <ntl-e+AXbWqSrlAAvxtiuMwx3w@public.gmane.org>
> ---
>  clone_ppc.c  |   12 ++++++++----
>  clone_ppc_.S |   40 ++++++++++++++++------------------------
>  2 files changed, 24 insertions(+), 28 deletions(-)
> 
> diff --git a/clone_ppc.c b/clone_ppc.c
> index 47b8290..f047a9e 100644
> --- a/clone_ppc.c
> +++ b/clone_ppc.c
> @@ -25,7 +25,6 @@
>  #include "eclone.h"
> 
>  extern int __eclone(int (*fn)(void *arg),
> -		    void *child_sp,
>  		    int flags,
>  		    void *fn_arg,
>  		    struct clone_args *args,
> @@ -39,9 +38,15 @@ int eclone(int (*fn)(void *), void *fn_arg, int clone_flags_low,
>  	unsigned long child_sp;
>  	int newpid;
> 
> +	/* The stack pointer for the child is communicated to the
> +	 * kernel via clone_args.child_stack, and to the __eclone
> +	 * assembly wrapper via the child_sp argument [r4].  So we
> +	 * need to align child_sp here and ensure that the wrapper and
> +	 * the kernel receive the same value.
> +	 */
>  	if (clone_args->child_stack)
> -		child_sp = clone_args->child_stack +
> -			clone_args->child_stack_size - 1;
> +		child_sp = (clone_args->child_stack +
> +			    clone_args->child_stack_size - 1) & ~0xf;
>  	else
>  		child_sp = 0;
> 
> @@ -50,7 +55,6 @@ int eclone(int (*fn)(void *), void *fn_arg, int clone_flags_low,
>  	my_args.child_stack_size = 0;
> 
>  	newpid = __eclone(fn,
> -			  (void *)child_sp,
>  			  clone_flags_low,
>  			  fn_arg,
>  			  &my_args,
> diff --git a/clone_ppc_.S b/clone_ppc_.S
> index ebc07f3..b7e50e3 100644
> --- a/clone_ppc_.S
> +++ b/clone_ppc_.S
> @@ -20,12 +20,11 @@
>  #endif
> 
>  /* int [r3] eclone(int (*fn)(void *arg) [r3],
> - *                          void *child_sp [r4],
> - *                          int flags [r5],
> - *                          void *fn_arg [r6],
> - *                          struct clone_args *args [r7],
> - *                          size_t args_size [r8],
> - *                          pid_t *pids [r9]);
> + *                          int flags [r4],
> + *                          void *fn_arg [r5],
> + *                          struct clone_args *args [r6],
> + *                          size_t args_size [r7],
> + *                          pid_t *pids [r8]);
>   * Creates a child task with the pids specified by pids.
>   * Returns to parent only, child execution and exit is handled here.
>   * On error, returns negated errno.  On success, returns the pid of the child
> @@ -40,25 +39,18 @@ __eclone:
>  	/* Set up parent's stack frame. */
>  	stwu	r1,-32(r1)
> 
> -	/* Save non-volatiles (r28-r31) which we plan to use. */
> -	stmw	r28,16(r1)
> +	/* Save non-volatiles (r30-r31) which we plan to use. */
> +	stmw	r30,16(r1)
> 
> -	/* Set up child's stack frame. */
> -	clrrwi	r4,r4,4
> -	li	r0,0
> -	stw	r0,-16(r4)

Am I correct that this is was clearing the "last" backchain pointer?
I guess we're assuming the mmap'd areas of the genstack APIs are filled
with 0s then. If that's the case then, strictly speaking, the genstack
patches should precede this one.

Otherwise omitting child_sp looks good.

> -
> -	/* Save fn, stack pointer, flags, and fn_arg across system call. */
> -	mr	r28,r3
> -	mr	r29,r4
> -	mr	r30,r5
> -	mr	r31,r6
> +	/* Save fn and fn_arg across system call. */
> +	mr	r30,r3
> +	mr	r31,r5
> 
>  	/* Set up arguments for system call. */
> -	mr	r3,r5	/* flags */
> -	mr	r4,r7	/* clone_args */
> -	mr	r5,r8	/* clone_args' size */
> -	mr	r6,r9	/* pids */
> +	mr	r3,r4	/* flags */
> +	mr	r4,r6	/* clone_args */
> +	mr	r5,r7	/* clone_args' size */
> +	mr	r6,r8	/* pids */

Wouldn't it be possible to avoid some mr instructions above by re-arranging
the function arguments? Then you're just saving things in the two non-volatile
gprs before making the eclone syscall. I think this amounts to making the
prototype:

int [r3] eclone(int flags [r3],
		struct clone_args *args [r4],
		size_t args_size [r6],
		pid_t *pids [r7],
		int (*fn)(void *arg) [r8],
		void *fn_arg [r9]);

Cheers,
	-Matt Helsley

Re: [PATCH 2/5] user-cr: align powerpc sp for eclone, clean up __eclone

[Matt Helsley]

On Wed, Dec 16, 2009 at 08:29:25PM -0800, Matt Helsley wrote:
> On Fri, Dec 04, 2009 at 11:02:28PM -0600, Nathan Lynch wrote:

<snip>

> >  	/* Set up arguments for system call. */
> > -	mr	r3,r5	/* flags */
> > -	mr	r4,r7	/* clone_args */
> > -	mr	r5,r8	/* clone_args' size */
> > -	mr	r6,r9	/* pids */
> > +	mr	r3,r4	/* flags */
> > +	mr	r4,r6	/* clone_args */
> > +	mr	r5,r7	/* clone_args' size */
> > +	mr	r6,r8	/* pids */
> 
> Wouldn't it be possible to avoid some mr instructions above by re-arranging

(The 4 immediately above)

> the function arguments? Then you're just saving things in the two non-volatile
> gprs before making the eclone syscall. I think this amounts to making the
> prototype:
>
> int [r3] eclone(int flags [r3],
> 		struct clone_args *args [r4],
> 		size_t args_size [r6],
> 		pid_t *pids [r7],
> 		int (*fn)(void *arg) [r8],
> 		void *fn_arg [r9]);

Argh, should've been:

int [r3] __eclone(int flags [r3],
              struct clone_args *args [r4],
              size_t args_size [r5],
              pid_t *pids [r6],
              int (*fn)(void *arg) [r7],
              void *fn_arg [r8]);

Cheers,
	-Matt