[Qemu-devel] [PATCH 0/3] linux-user/sparc: Fixes for clone

[Qemu-devel] [PATCH 0/3] linux-user/sparc: Fixes for clone

[Richard Henderson]

There are at least 4 separate bugs preventing clone from working.

(1) cpu_copy left both cpus sharing the same register window (!)

(2) cpu_clone_regs did not initialize %o1, so the new thread path
    in the guest __clone was always taken, even for the parent
    (old %o1 value was newsp, and so non-zero).

(3) cpu_clone_regs did not advance the pc past the syscall in the
    child, which meant that the child re-executed the syscall
    (and because of (1), with essentially random inputs).

(4) clone did not flush register windows, which would cause the
    parent stack to be clobbered by the child writing out old
    windows in order to allocate a new one.
    
This is enough for Alex's atomic-test to make progress, but not
quite enough for it to actually work.  What I'm seeing now is a
legitimate SEGV for a write to a r-xp memory segment.  I'll need
to examine the testcase further to see why that is happening.


r~


Richard Henderson (4):
  linux-user: Disallow setting newsp for fork
  linux-user: Pass the parent env to cpu_clone_regs
  linux-user/sparc: Fix cpu_clone_regs
  linux-user/sparc: Flush register windows before clone

 linux-user/aarch64/target_cpu.h    |  3 ++-
 linux-user/alpha/target_cpu.h      |  3 ++-
 linux-user/arm/target_cpu.h        |  3 ++-
 linux-user/cris/target_cpu.h       |  3 ++-
 linux-user/hppa/target_cpu.h       |  3 ++-
 linux-user/i386/target_cpu.h       |  3 ++-
 linux-user/m68k/target_cpu.h       |  3 ++-
 linux-user/microblaze/target_cpu.h |  3 ++-
 linux-user/mips/target_cpu.h       |  3 ++-
 linux-user/nios2/target_cpu.h      |  3 ++-
 linux-user/openrisc/target_cpu.h   |  4 +++-
 linux-user/ppc/target_cpu.h        |  3 ++-
 linux-user/riscv/target_cpu.h      |  3 ++-
 linux-user/s390x/target_cpu.h      |  3 ++-
 linux-user/sh4/target_cpu.h        |  3 ++-
 linux-user/sparc/target_cpu.h      | 23 ++++++++++++++++++++---
 linux-user/tilegx/target_cpu.h     |  3 ++-
 linux-user/xtensa/target_cpu.h     |  3 ++-
 linux-user/sparc/cpu_loop.c        |  3 +++
 linux-user/syscall.c               |  9 ++++++---
 20 files changed, 64 insertions(+), 23 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH 1/4] linux-user: Disallow setting newsp for fork

[Richard Henderson]

Or really, just clone devolving into fork.  This should not ever happen
in practice.  We do want to reserve calling cpu_clone_regs for the case
in which we are actually performing a clone.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/syscall.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index dfc851cc35..5bf8d13de7 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -6502,10 +6502,14 @@ static int do_fork(CPUArchState *env, unsigned int flags, abi_ulong newsp,
         pthread_mutex_destroy(&info.mutex);
         pthread_mutex_unlock(&clone_lock);
     } else {
-        /* if no CLONE_VM, we consider it is a fork */
+        /* If no CLONE_VM, we consider it is a fork.  */
         if (flags & CLONE_INVALID_FORK_FLAGS) {
             return -TARGET_EINVAL;
         }
+        /* As a fork, setting a new sp does not make sense.  */
+        if (newsp) {
+            return -TARGET_EINVAL;
+        }
 
         /* We can't support custom termination signals */
         if ((flags & CSIGNAL) != TARGET_SIGCHLD) {
@@ -6520,7 +6524,6 @@ static int do_fork(CPUArchState *env, unsigned int flags, abi_ulong newsp,
         ret = fork();
         if (ret == 0) {
             /* Child Process.  */
-            cpu_clone_regs(env, newsp);
             fork_end(1);
             /* There is a race condition here.  The parent process could
                theoretically read the TID in the child process before the child
-- 
2.17.1

[Qemu-devel] [PATCH 2/4] linux-user: Pass the parent env to cpu_clone_regs

[Richard Henderson]

Implementing clone for sparc requires that we make modifications
to both the parent and child cpu state.  In all other cases, the
new argument can be ignored.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/aarch64/target_cpu.h    | 3 ++-
 linux-user/alpha/target_cpu.h      | 3 ++-
 linux-user/arm/target_cpu.h        | 3 ++-
 linux-user/cris/target_cpu.h       | 3 ++-
 linux-user/hppa/target_cpu.h       | 3 ++-
 linux-user/i386/target_cpu.h       | 3 ++-
 linux-user/m68k/target_cpu.h       | 3 ++-
 linux-user/microblaze/target_cpu.h | 3 ++-
 linux-user/mips/target_cpu.h       | 3 ++-
 linux-user/nios2/target_cpu.h      | 3 ++-
 linux-user/openrisc/target_cpu.h   | 4 +++-
 linux-user/ppc/target_cpu.h        | 3 ++-
 linux-user/riscv/target_cpu.h      | 3 ++-
 linux-user/s390x/target_cpu.h      | 3 ++-
 linux-user/sh4/target_cpu.h        | 3 ++-
 linux-user/sparc/target_cpu.h      | 3 ++-
 linux-user/tilegx/target_cpu.h     | 3 ++-
 linux-user/xtensa/target_cpu.h     | 3 ++-
 linux-user/syscall.c               | 2 +-
 19 files changed, 38 insertions(+), 19 deletions(-)

diff --git a/linux-user/aarch64/target_cpu.h b/linux-user/aarch64/target_cpu.h
index a021c95fa4..130177115e 100644
--- a/linux-user/aarch64/target_cpu.h
+++ b/linux-user/aarch64/target_cpu.h
@@ -19,7 +19,8 @@
 #ifndef AARCH64_TARGET_CPU_H
 #define AARCH64_TARGET_CPU_H
 
-static inline void cpu_clone_regs(CPUARMState *env, target_ulong newsp)
+static inline void cpu_clone_regs(CPUARMState *env, CPUARMState *old_env,
+                                  target_ulong newsp)
 {
     if (newsp) {
         env->xregs[31] = newsp;
diff --git a/linux-user/alpha/target_cpu.h b/linux-user/alpha/target_cpu.h
index ac4d255ae7..750ffb50d7 100644
--- a/linux-user/alpha/target_cpu.h
+++ b/linux-user/alpha/target_cpu.h
@@ -19,7 +19,8 @@
 #ifndef ALPHA_TARGET_CPU_H
 #define ALPHA_TARGET_CPU_H
 
-static inline void cpu_clone_regs(CPUAlphaState *env, target_ulong newsp)
+static inline void cpu_clone_regs(CPUAlphaState *env, CPUAlphaState *old_env,
+                                  target_ulong newsp)
 {
     if (newsp) {
         env->ir[IR_SP] = newsp;
diff --git a/linux-user/arm/target_cpu.h b/linux-user/arm/target_cpu.h
index 8a3764919a..5538b6cb29 100644
--- a/linux-user/arm/target_cpu.h
+++ b/linux-user/arm/target_cpu.h
@@ -23,7 +23,8 @@
    See validate_guest_space in linux-user/elfload.c.  */
 #define MAX_RESERVED_VA  0xffff0000ul
 
-static inline void cpu_clone_regs(CPUARMState *env, target_ulong newsp)
+static inline void cpu_clone_regs(CPUARMState *env, CPUARMState *old_env,
+                                  target_ulong newsp)
 {
     if (newsp) {
         env->regs[13] = newsp;
diff --git a/linux-user/cris/target_cpu.h b/linux-user/cris/target_cpu.h
index 2309343979..baf842b400 100644
--- a/linux-user/cris/target_cpu.h
+++ b/linux-user/cris/target_cpu.h
@@ -20,7 +20,8 @@
 #ifndef CRIS_TARGET_CPU_H
 #define CRIS_TARGET_CPU_H
 
-static inline void cpu_clone_regs(CPUCRISState *env, target_ulong newsp)
+static inline void cpu_clone_regs(CPUCRISState *env, CPUCRISState *old_env,
+                                  target_ulong newsp)
 {
     if (newsp) {
         env->regs[14] = newsp;
diff --git a/linux-user/hppa/target_cpu.h b/linux-user/hppa/target_cpu.h
index 1c539bdbd6..7cd8d168a7 100644
--- a/linux-user/hppa/target_cpu.h
+++ b/linux-user/hppa/target_cpu.h
@@ -19,7 +19,8 @@
 #ifndef HPPA_TARGET_CPU_H
 #define HPPA_TARGET_CPU_H
 
-static inline void cpu_clone_regs(CPUHPPAState *env, target_ulong newsp)
+static inline void cpu_clone_regs(CPUHPPAState *env, CPUHPPAState *old_env,
+                                  target_ulong newsp)
 {
     if (newsp) {
         env->gr[30] = newsp;
diff --git a/linux-user/i386/target_cpu.h b/linux-user/i386/target_cpu.h
index ece04d0966..8fbe36670f 100644
--- a/linux-user/i386/target_cpu.h
+++ b/linux-user/i386/target_cpu.h
@@ -20,7 +20,8 @@
 #ifndef I386_TARGET_CPU_H
 #define I386_TARGET_CPU_H
 
-static inline void cpu_clone_regs(CPUX86State *env, target_ulong newsp)
+static inline void cpu_clone_regs(CPUX86State *env, CPUX86State *old_env,
+                                  target_ulong newsp)
 {
     if (newsp) {
         env->regs[R_ESP] = newsp;
diff --git a/linux-user/m68k/target_cpu.h b/linux-user/m68k/target_cpu.h
index 611df065ca..1f0939aea7 100644
--- a/linux-user/m68k/target_cpu.h
+++ b/linux-user/m68k/target_cpu.h
@@ -21,7 +21,8 @@
 #ifndef M68K_TARGET_CPU_H
 #define M68K_TARGET_CPU_H
 
-static inline void cpu_clone_regs(CPUM68KState *env, target_ulong newsp)
+static inline void cpu_clone_regs(CPUM68KState *env, CPUM68KState *old_env,
+                                  target_ulong newsp)
 {
     if (newsp) {
         env->aregs[7] = newsp;
diff --git a/linux-user/microblaze/target_cpu.h b/linux-user/microblaze/target_cpu.h
index 73e139938c..3394e98918 100644
--- a/linux-user/microblaze/target_cpu.h
+++ b/linux-user/microblaze/target_cpu.h
@@ -19,7 +19,8 @@
 #ifndef MICROBLAZE_TARGET_CPU_H
 #define MICROBLAZE_TARGET_CPU_H
 
-static inline void cpu_clone_regs(CPUMBState *env, target_ulong newsp)
+static inline void cpu_clone_regs(CPUMBState *env, CPUMBState *old_env,
+                                  target_ulong newsp)
 {
     if (newsp) {
         env->regs[R_SP] = newsp;
diff --git a/linux-user/mips/target_cpu.h b/linux-user/mips/target_cpu.h
index 02cf5eeff7..109348a5c9 100644
--- a/linux-user/mips/target_cpu.h
+++ b/linux-user/mips/target_cpu.h
@@ -19,7 +19,8 @@
 #ifndef MIPS_TARGET_CPU_H
 #define MIPS_TARGET_CPU_H
 
-static inline void cpu_clone_regs(CPUMIPSState *env, target_ulong newsp)
+static inline void cpu_clone_regs(CPUMIPSState *env, CPUMIPSState *old_env,
+                                  target_ulong newsp)
 {
     if (newsp) {
         env->active_tc.gpr[29] = newsp;
diff --git a/linux-user/nios2/target_cpu.h b/linux-user/nios2/target_cpu.h
index 14f63338fa..09d2db74dc 100644
--- a/linux-user/nios2/target_cpu.h
+++ b/linux-user/nios2/target_cpu.h
@@ -20,7 +20,8 @@
 #ifndef TARGET_CPU_H
 #define TARGET_CPU_H
 
-static inline void cpu_clone_regs(CPUNios2State *env, target_ulong newsp)
+static inline void cpu_clone_regs(CPUNios2State *env, CPUNios2State *old_env,
+                                  target_ulong newsp)
 {
     if (newsp) {
         env->regs[R_SP] = newsp;
diff --git a/linux-user/openrisc/target_cpu.h b/linux-user/openrisc/target_cpu.h
index d1ea4506e2..5ea3e1b1a6 100644
--- a/linux-user/openrisc/target_cpu.h
+++ b/linux-user/openrisc/target_cpu.h
@@ -20,7 +20,9 @@
 #ifndef OPENRISC_TARGET_CPU_H
 #define OPENRISC_TARGET_CPU_H
 
-static inline void cpu_clone_regs(CPUOpenRISCState *env, target_ulong newsp)
+static inline void cpu_clone_regs(CPUOpenRISCState *env,
+                                  CPUOpenRISCState *old_env,
+                                  target_ulong newsp)
 {
     if (newsp) {
         cpu_set_gpr(env, 1, newsp);
diff --git a/linux-user/ppc/target_cpu.h b/linux-user/ppc/target_cpu.h
index c4641834e7..f42e266047 100644
--- a/linux-user/ppc/target_cpu.h
+++ b/linux-user/ppc/target_cpu.h
@@ -19,7 +19,8 @@
 #ifndef PPC_TARGET_CPU_H
 #define PPC_TARGET_CPU_H
 
-static inline void cpu_clone_regs(CPUPPCState *env, target_ulong newsp)
+static inline void cpu_clone_regs(CPUPPCState *env, CPUPPCState *old_env,
+                                  target_ulong newsp)
 {
     if (newsp) {
         env->gpr[1] = newsp;
diff --git a/linux-user/riscv/target_cpu.h b/linux-user/riscv/target_cpu.h
index 7e090f376a..b112832d95 100644
--- a/linux-user/riscv/target_cpu.h
+++ b/linux-user/riscv/target_cpu.h
@@ -1,7 +1,8 @@
 #ifndef TARGET_CPU_H
 #define TARGET_CPU_H
 
-static inline void cpu_clone_regs(CPURISCVState *env, target_ulong newsp)
+static inline void cpu_clone_regs(CPURISCVState *env, CPURISCVState *old_env,
+                                  target_ulong newsp)
 {
     if (newsp) {
         env->gpr[xSP] = newsp;
diff --git a/linux-user/s390x/target_cpu.h b/linux-user/s390x/target_cpu.h
index 66ef8aa8c2..b31b9ad09d 100644
--- a/linux-user/s390x/target_cpu.h
+++ b/linux-user/s390x/target_cpu.h
@@ -22,7 +22,8 @@
 #ifndef S390X_TARGET_CPU_H
 #define S390X_TARGET_CPU_H
 
-static inline void cpu_clone_regs(CPUS390XState *env, target_ulong newsp)
+static inline void cpu_clone_regs(CPUS390XState *env, CPUS390XState *old_env,
+                                  target_ulong newsp)
 {
     if (newsp) {
         env->regs[15] = newsp;
diff --git a/linux-user/sh4/target_cpu.h b/linux-user/sh4/target_cpu.h
index 1a647ddb98..7f09ed4c3a 100644
--- a/linux-user/sh4/target_cpu.h
+++ b/linux-user/sh4/target_cpu.h
@@ -19,7 +19,8 @@
 #ifndef SH4_TARGET_CPU_H
 #define SH4_TARGET_CPU_H
 
-static inline void cpu_clone_regs(CPUSH4State *env, target_ulong newsp)
+static inline void cpu_clone_regs(CPUSH4State *env, CPUSH4State *old_env,
+                                  target_ulong newsp)
 {
     if (newsp) {
         env->gregs[15] = newsp;
diff --git a/linux-user/sparc/target_cpu.h b/linux-user/sparc/target_cpu.h
index 1ffc0ae9f2..a92748cae3 100644
--- a/linux-user/sparc/target_cpu.h
+++ b/linux-user/sparc/target_cpu.h
@@ -20,7 +20,8 @@
 #ifndef SPARC_TARGET_CPU_H
 #define SPARC_TARGET_CPU_H
 
-static inline void cpu_clone_regs(CPUSPARCState *env, target_ulong newsp)
+static inline void cpu_clone_regs(CPUSPARCState *env, CPUSPARCState *old_env,
+                                  target_ulong newsp)
 {
     if (newsp) {
         env->regwptr[22] = newsp;
diff --git a/linux-user/tilegx/target_cpu.h b/linux-user/tilegx/target_cpu.h
index d1aa5824f2..35100a3d43 100644
--- a/linux-user/tilegx/target_cpu.h
+++ b/linux-user/tilegx/target_cpu.h
@@ -19,7 +19,8 @@
 #ifndef TILEGX_TARGET_CPU_H
 #define TILEGX_TARGET_CPU_H
 
-static inline void cpu_clone_regs(CPUTLGState *env, target_ulong newsp)
+static inline void cpu_clone_regs(CPUTLGState *env, CPUTLGState *old_env,
+                                  target_ulong newsp)
 {
     if (newsp) {
         env->regs[TILEGX_R_SP] = newsp;
diff --git a/linux-user/xtensa/target_cpu.h b/linux-user/xtensa/target_cpu.h
index e31efe3ea0..0e9681e9f9 100644
--- a/linux-user/xtensa/target_cpu.h
+++ b/linux-user/xtensa/target_cpu.h
@@ -4,7 +4,8 @@
 #ifndef XTENSA_TARGET_CPU_H
 #define XTENSA_TARGET_CPU_H
 
-static inline void cpu_clone_regs(CPUXtensaState *env, target_ulong newsp)
+static inline void cpu_clone_regs(CPUXtensaState *env, CPUXtensaState *old_env,
+                                  target_ulong newsp)
 {
     if (newsp) {
         env->regs[1] = newsp;
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 5bf8d13de7..7273a2fe54 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -6442,7 +6442,7 @@ static int do_fork(CPUArchState *env, unsigned int flags, abi_ulong newsp,
         /* we create a new CPU instance. */
         new_env = cpu_copy(env);
         /* Init regs that differ from the parent.  */
-        cpu_clone_regs(new_env, newsp);
+        cpu_clone_regs(new_env, env, newsp);
         new_cpu = ENV_GET_CPU(new_env);
         new_cpu->opaque = ts;
         ts->bprm = parent_ts->bprm;
-- 
2.17.1

[Qemu-devel] [PATCH 3/4] linux-user/sparc: Fix cpu_clone_regs

[Richard Henderson]

We failed to set the secondary return value in %o1
we failed to advance the PC past the syscall, and
we failed to adjust regwptr into the new structure.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/sparc/target_cpu.h | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/linux-user/sparc/target_cpu.h b/linux-user/sparc/target_cpu.h
index a92748cae3..c223f865e9 100644
--- a/linux-user/sparc/target_cpu.h
+++ b/linux-user/sparc/target_cpu.h
@@ -23,11 +23,21 @@
 static inline void cpu_clone_regs(CPUSPARCState *env, CPUSPARCState *old_env,
                                   target_ulong newsp)
 {
+    /*
+     * After cpu_copy, env->regwptr is pointing into old_env.
+     * Update the new cpu to use its own register window.
+     */
+    env->regwptr = env->regbase + (env->cwp * 16);
+
+    /* Set a new stack, if requested.  */
     if (newsp) {
         env->regwptr[22] = newsp;
     }
-    /* syscall return for clone child: 0, and clear CF since
-     * this counts as a success return value.
+
+    /*
+     * Syscall return for clone child: %o0 = 0 and clear CF since
+     * this counts as a success return value.  %o1 = 1 to indicate
+     * this is the child.  Advance the PC past the syscall.
      */
     env->regwptr[0] = 0;
 #if defined(TARGET_SPARC64) && !defined(TARGET_ABI32)
@@ -35,6 +45,12 @@ static inline void cpu_clone_regs(CPUSPARCState *env, CPUSPARCState *old_env,
 #else
     env->psr &= ~PSR_CARRY;
 #endif
+    env->regwptr[1] = 1;
+    env->pc = env->npc;
+    env->npc = env->npc + 4;
+
+    /* Set the second return value for the parent: %o1 = 0.  */
+    old_env->regwptr[1] = 0;
 }
 
 static inline void cpu_set_tls(CPUSPARCState *env, target_ulong newtls)
-- 
2.17.1

[Qemu-devel] [PATCH 4/4] linux-user/sparc: Flush register windows before clone

[Richard Henderson]

As seen as the very first instruction of sys_clone in the kernel.

Ideally this would be done in or before cpu_copy, and not with a
separate explicit test vs the syscall number, but this is a more
minimal solution.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/sparc/cpu_loop.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/linux-user/sparc/cpu_loop.c b/linux-user/sparc/cpu_loop.c
index 91f714afc6..fe83f25686 100644
--- a/linux-user/sparc/cpu_loop.c
+++ b/linux-user/sparc/cpu_loop.c
@@ -169,6 +169,9 @@ void cpu_loop (CPUSPARCState *env)
         case 0x110:
         case 0x16d:
 #endif
+            if (env->gregs[1] == TARGET_NR_clone) {
+                flush_windows(env);
+            }
             ret = do_syscall (env, env->gregs[1],
                               env->regwptr[0], env->regwptr[1],
                               env->regwptr[2], env->regwptr[3],
-- 
2.17.1

Re: [Qemu-devel] [PATCH 0/3] linux-user/sparc: Fixes for clone

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> There are at least 4 separate bugs preventing clone from working.
>
> (1) cpu_copy left both cpus sharing the same register window (!)
>
> (2) cpu_clone_regs did not initialize %o1, so the new thread path
>     in the guest __clone was always taken, even for the parent
>     (old %o1 value was newsp, and so non-zero).
>
> (3) cpu_clone_regs did not advance the pc past the syscall in the
>     child, which meant that the child re-executed the syscall
>     (and because of (1), with essentially random inputs).
>
> (4) clone did not flush register windows, which would cause the
>     parent stack to be clobbered by the child writing out old
>     windows in order to allocate a new one.
>
> This is enough for Alex's atomic-test to make progress, but not
> quite enough for it to actually work.  What I'm seeing now is a
> legitimate SEGV for a write to a r-xp memory segment.  I'll need
> to examine the testcase further to see why that is happening.


Hmm and testthread now reliably bombs with:

  thread2: 10 hello2
  testthread: allocatestack.c:384: advise_stack_range: Assertion `freesize < size' failed.
  fish: “./qemu-sparc64 -d trace:user_qu…” terminated by signal SIGABRT (Abort)

However the behaviour of the atomic test now looks similar to the
occasional failure I was seeing in testthread before, i.e. a crash
during atomic operations.

>
>
> r~
>
>
> Richard Henderson (4):
>   linux-user: Disallow setting newsp for fork
>   linux-user: Pass the parent env to cpu_clone_regs
>   linux-user/sparc: Fix cpu_clone_regs
>   linux-user/sparc: Flush register windows before clone
>
>  linux-user/aarch64/target_cpu.h    |  3 ++-
>  linux-user/alpha/target_cpu.h      |  3 ++-
>  linux-user/arm/target_cpu.h        |  3 ++-
>  linux-user/cris/target_cpu.h       |  3 ++-
>  linux-user/hppa/target_cpu.h       |  3 ++-
>  linux-user/i386/target_cpu.h       |  3 ++-
>  linux-user/m68k/target_cpu.h       |  3 ++-
>  linux-user/microblaze/target_cpu.h |  3 ++-
>  linux-user/mips/target_cpu.h       |  3 ++-
>  linux-user/nios2/target_cpu.h      |  3 ++-
>  linux-user/openrisc/target_cpu.h   |  4 +++-
>  linux-user/ppc/target_cpu.h        |  3 ++-
>  linux-user/riscv/target_cpu.h      |  3 ++-
>  linux-user/s390x/target_cpu.h      |  3 ++-
>  linux-user/sh4/target_cpu.h        |  3 ++-
>  linux-user/sparc/target_cpu.h      | 23 ++++++++++++++++++++---
>  linux-user/tilegx/target_cpu.h     |  3 ++-
>  linux-user/xtensa/target_cpu.h     |  3 ++-
>  linux-user/sparc/cpu_loop.c        |  3 +++
>  linux-user/syscall.c               |  9 ++++++---
>  20 files changed, 64 insertions(+), 23 deletions(-)


--
Alex Bennée

Re: [Qemu-devel] [PATCH 2/4] linux-user: Pass the parent env to cpu_clone_regs

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> Implementing clone for sparc requires that we make modifications
> to both the parent and child cpu state.  In all other cases, the
> new argument can be ignored.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  linux-user/aarch64/target_cpu.h    | 3 ++-
>  linux-user/alpha/target_cpu.h      | 3 ++-
>  linux-user/arm/target_cpu.h        | 3 ++-
>  linux-user/cris/target_cpu.h       | 3 ++-
>  linux-user/hppa/target_cpu.h       | 3 ++-
>  linux-user/i386/target_cpu.h       | 3 ++-
>  linux-user/m68k/target_cpu.h       | 3 ++-
>  linux-user/microblaze/target_cpu.h | 3 ++-
>  linux-user/mips/target_cpu.h       | 3 ++-
>  linux-user/nios2/target_cpu.h      | 3 ++-
>  linux-user/openrisc/target_cpu.h   | 4 +++-
>  linux-user/ppc/target_cpu.h        | 3 ++-
>  linux-user/riscv/target_cpu.h      | 3 ++-
>  linux-user/s390x/target_cpu.h      | 3 ++-
>  linux-user/sh4/target_cpu.h        | 3 ++-
>  linux-user/sparc/target_cpu.h      | 3 ++-
>  linux-user/tilegx/target_cpu.h     | 3 ++-
>  linux-user/xtensa/target_cpu.h     | 3 ++-
>  linux-user/syscall.c               | 2 +-
>  19 files changed, 38 insertions(+), 19 deletions(-)
>
> diff --git a/linux-user/aarch64/target_cpu.h b/linux-user/aarch64/target_cpu.h
> index a021c95fa4..130177115e 100644
> --- a/linux-user/aarch64/target_cpu.h
> +++ b/linux-user/aarch64/target_cpu.h
> @@ -19,7 +19,8 @@
>  #ifndef AARCH64_TARGET_CPU_H
>  #define AARCH64_TARGET_CPU_H
>
> -static inline void cpu_clone_regs(CPUARMState *env, target_ulong newsp)
> +static inline void cpu_clone_regs(CPUARMState *env, CPUARMState *old_env,
> +                                  target_ulong newsp)
>  {
>      if (newsp) {
>          env->xregs[31] = newsp;
> diff --git a/linux-user/alpha/target_cpu.h b/linux-user/alpha/target_cpu.h
> index ac4d255ae7..750ffb50d7 100644
> --- a/linux-user/alpha/target_cpu.h
> +++ b/linux-user/alpha/target_cpu.h
> @@ -19,7 +19,8 @@
>  #ifndef ALPHA_TARGET_CPU_H
>  #define ALPHA_TARGET_CPU_H
>
> -static inline void cpu_clone_regs(CPUAlphaState *env, target_ulong newsp)
> +static inline void cpu_clone_regs(CPUAlphaState *env, CPUAlphaState *old_env,
> +                                  target_ulong newsp)
>  {
>      if (newsp) {
>          env->ir[IR_SP] = newsp;
> diff --git a/linux-user/arm/target_cpu.h b/linux-user/arm/target_cpu.h
> index 8a3764919a..5538b6cb29 100644
> --- a/linux-user/arm/target_cpu.h
> +++ b/linux-user/arm/target_cpu.h
> @@ -23,7 +23,8 @@
>     See validate_guest_space in linux-user/elfload.c.  */
>  #define MAX_RESERVED_VA  0xffff0000ul
>
> -static inline void cpu_clone_regs(CPUARMState *env, target_ulong newsp)
> +static inline void cpu_clone_regs(CPUARMState *env, CPUARMState *old_env,
> +                                  target_ulong newsp)
>  {
>      if (newsp) {
>          env->regs[13] = newsp;
> diff --git a/linux-user/cris/target_cpu.h b/linux-user/cris/target_cpu.h
> index 2309343979..baf842b400 100644
> --- a/linux-user/cris/target_cpu.h
> +++ b/linux-user/cris/target_cpu.h
> @@ -20,7 +20,8 @@
>  #ifndef CRIS_TARGET_CPU_H
>  #define CRIS_TARGET_CPU_H
>
> -static inline void cpu_clone_regs(CPUCRISState *env, target_ulong newsp)
> +static inline void cpu_clone_regs(CPUCRISState *env, CPUCRISState *old_env,
> +                                  target_ulong newsp)
>  {
>      if (newsp) {
>          env->regs[14] = newsp;
> diff --git a/linux-user/hppa/target_cpu.h b/linux-user/hppa/target_cpu.h
> index 1c539bdbd6..7cd8d168a7 100644
> --- a/linux-user/hppa/target_cpu.h
> +++ b/linux-user/hppa/target_cpu.h
> @@ -19,7 +19,8 @@
>  #ifndef HPPA_TARGET_CPU_H
>  #define HPPA_TARGET_CPU_H
>
> -static inline void cpu_clone_regs(CPUHPPAState *env, target_ulong newsp)
> +static inline void cpu_clone_regs(CPUHPPAState *env, CPUHPPAState *old_env,
> +                                  target_ulong newsp)
>  {
>      if (newsp) {
>          env->gr[30] = newsp;
> diff --git a/linux-user/i386/target_cpu.h b/linux-user/i386/target_cpu.h
> index ece04d0966..8fbe36670f 100644
> --- a/linux-user/i386/target_cpu.h
> +++ b/linux-user/i386/target_cpu.h
> @@ -20,7 +20,8 @@
>  #ifndef I386_TARGET_CPU_H
>  #define I386_TARGET_CPU_H
>
> -static inline void cpu_clone_regs(CPUX86State *env, target_ulong newsp)
> +static inline void cpu_clone_regs(CPUX86State *env, CPUX86State *old_env,
> +                                  target_ulong newsp)
>  {
>      if (newsp) {
>          env->regs[R_ESP] = newsp;
> diff --git a/linux-user/m68k/target_cpu.h b/linux-user/m68k/target_cpu.h
> index 611df065ca..1f0939aea7 100644
> --- a/linux-user/m68k/target_cpu.h
> +++ b/linux-user/m68k/target_cpu.h
> @@ -21,7 +21,8 @@
>  #ifndef M68K_TARGET_CPU_H
>  #define M68K_TARGET_CPU_H
>
> -static inline void cpu_clone_regs(CPUM68KState *env, target_ulong newsp)
> +static inline void cpu_clone_regs(CPUM68KState *env, CPUM68KState *old_env,
> +                                  target_ulong newsp)
>  {
>      if (newsp) {
>          env->aregs[7] = newsp;
> diff --git a/linux-user/microblaze/target_cpu.h b/linux-user/microblaze/target_cpu.h
> index 73e139938c..3394e98918 100644
> --- a/linux-user/microblaze/target_cpu.h
> +++ b/linux-user/microblaze/target_cpu.h
> @@ -19,7 +19,8 @@
>  #ifndef MICROBLAZE_TARGET_CPU_H
>  #define MICROBLAZE_TARGET_CPU_H
>
> -static inline void cpu_clone_regs(CPUMBState *env, target_ulong newsp)
> +static inline void cpu_clone_regs(CPUMBState *env, CPUMBState *old_env,
> +                                  target_ulong newsp)
>  {
>      if (newsp) {
>          env->regs[R_SP] = newsp;
> diff --git a/linux-user/mips/target_cpu.h b/linux-user/mips/target_cpu.h
> index 02cf5eeff7..109348a5c9 100644
> --- a/linux-user/mips/target_cpu.h
> +++ b/linux-user/mips/target_cpu.h
> @@ -19,7 +19,8 @@
>  #ifndef MIPS_TARGET_CPU_H
>  #define MIPS_TARGET_CPU_H
>
> -static inline void cpu_clone_regs(CPUMIPSState *env, target_ulong newsp)
> +static inline void cpu_clone_regs(CPUMIPSState *env, CPUMIPSState *old_env,
> +                                  target_ulong newsp)
>  {
>      if (newsp) {
>          env->active_tc.gpr[29] = newsp;
> diff --git a/linux-user/nios2/target_cpu.h b/linux-user/nios2/target_cpu.h
> index 14f63338fa..09d2db74dc 100644
> --- a/linux-user/nios2/target_cpu.h
> +++ b/linux-user/nios2/target_cpu.h
> @@ -20,7 +20,8 @@
>  #ifndef TARGET_CPU_H
>  #define TARGET_CPU_H
>
> -static inline void cpu_clone_regs(CPUNios2State *env, target_ulong newsp)
> +static inline void cpu_clone_regs(CPUNios2State *env, CPUNios2State *old_env,
> +                                  target_ulong newsp)
>  {
>      if (newsp) {
>          env->regs[R_SP] = newsp;
> diff --git a/linux-user/openrisc/target_cpu.h b/linux-user/openrisc/target_cpu.h
> index d1ea4506e2..5ea3e1b1a6 100644
> --- a/linux-user/openrisc/target_cpu.h
> +++ b/linux-user/openrisc/target_cpu.h
> @@ -20,7 +20,9 @@
>  #ifndef OPENRISC_TARGET_CPU_H
>  #define OPENRISC_TARGET_CPU_H
>
> -static inline void cpu_clone_regs(CPUOpenRISCState *env, target_ulong newsp)
> +static inline void cpu_clone_regs(CPUOpenRISCState *env,
> +                                  CPUOpenRISCState *old_env,
> +                                  target_ulong newsp)
>  {
>      if (newsp) {
>          cpu_set_gpr(env, 1, newsp);
> diff --git a/linux-user/ppc/target_cpu.h b/linux-user/ppc/target_cpu.h
> index c4641834e7..f42e266047 100644
> --- a/linux-user/ppc/target_cpu.h
> +++ b/linux-user/ppc/target_cpu.h
> @@ -19,7 +19,8 @@
>  #ifndef PPC_TARGET_CPU_H
>  #define PPC_TARGET_CPU_H
>
> -static inline void cpu_clone_regs(CPUPPCState *env, target_ulong newsp)
> +static inline void cpu_clone_regs(CPUPPCState *env, CPUPPCState *old_env,
> +                                  target_ulong newsp)
>  {
>      if (newsp) {
>          env->gpr[1] = newsp;
> diff --git a/linux-user/riscv/target_cpu.h b/linux-user/riscv/target_cpu.h
> index 7e090f376a..b112832d95 100644
> --- a/linux-user/riscv/target_cpu.h
> +++ b/linux-user/riscv/target_cpu.h
> @@ -1,7 +1,8 @@
>  #ifndef TARGET_CPU_H
>  #define TARGET_CPU_H
>
> -static inline void cpu_clone_regs(CPURISCVState *env, target_ulong newsp)
> +static inline void cpu_clone_regs(CPURISCVState *env, CPURISCVState *old_env,
> +                                  target_ulong newsp)
>  {
>      if (newsp) {
>          env->gpr[xSP] = newsp;
> diff --git a/linux-user/s390x/target_cpu.h b/linux-user/s390x/target_cpu.h
> index 66ef8aa8c2..b31b9ad09d 100644
> --- a/linux-user/s390x/target_cpu.h
> +++ b/linux-user/s390x/target_cpu.h
> @@ -22,7 +22,8 @@
>  #ifndef S390X_TARGET_CPU_H
>  #define S390X_TARGET_CPU_H
>
> -static inline void cpu_clone_regs(CPUS390XState *env, target_ulong newsp)
> +static inline void cpu_clone_regs(CPUS390XState *env, CPUS390XState *old_env,
> +                                  target_ulong newsp)
>  {
>      if (newsp) {
>          env->regs[15] = newsp;
> diff --git a/linux-user/sh4/target_cpu.h b/linux-user/sh4/target_cpu.h
> index 1a647ddb98..7f09ed4c3a 100644
> --- a/linux-user/sh4/target_cpu.h
> +++ b/linux-user/sh4/target_cpu.h
> @@ -19,7 +19,8 @@
>  #ifndef SH4_TARGET_CPU_H
>  #define SH4_TARGET_CPU_H
>
> -static inline void cpu_clone_regs(CPUSH4State *env, target_ulong newsp)
> +static inline void cpu_clone_regs(CPUSH4State *env, CPUSH4State *old_env,
> +                                  target_ulong newsp)
>  {
>      if (newsp) {
>          env->gregs[15] = newsp;
> diff --git a/linux-user/sparc/target_cpu.h b/linux-user/sparc/target_cpu.h
> index 1ffc0ae9f2..a92748cae3 100644
> --- a/linux-user/sparc/target_cpu.h
> +++ b/linux-user/sparc/target_cpu.h
> @@ -20,7 +20,8 @@
>  #ifndef SPARC_TARGET_CPU_H
>  #define SPARC_TARGET_CPU_H
>
> -static inline void cpu_clone_regs(CPUSPARCState *env, target_ulong newsp)
> +static inline void cpu_clone_regs(CPUSPARCState *env, CPUSPARCState *old_env,
> +                                  target_ulong newsp)
>  {
>      if (newsp) {
>          env->regwptr[22] = newsp;
> diff --git a/linux-user/tilegx/target_cpu.h b/linux-user/tilegx/target_cpu.h
> index d1aa5824f2..35100a3d43 100644
> --- a/linux-user/tilegx/target_cpu.h
> +++ b/linux-user/tilegx/target_cpu.h
> @@ -19,7 +19,8 @@
>  #ifndef TILEGX_TARGET_CPU_H
>  #define TILEGX_TARGET_CPU_H
>
> -static inline void cpu_clone_regs(CPUTLGState *env, target_ulong newsp)
> +static inline void cpu_clone_regs(CPUTLGState *env, CPUTLGState *old_env,
> +                                  target_ulong newsp)
>  {
>      if (newsp) {
>          env->regs[TILEGX_R_SP] = newsp;
> diff --git a/linux-user/xtensa/target_cpu.h b/linux-user/xtensa/target_cpu.h
> index e31efe3ea0..0e9681e9f9 100644
> --- a/linux-user/xtensa/target_cpu.h
> +++ b/linux-user/xtensa/target_cpu.h
> @@ -4,7 +4,8 @@
>  #ifndef XTENSA_TARGET_CPU_H
>  #define XTENSA_TARGET_CPU_H
>
> -static inline void cpu_clone_regs(CPUXtensaState *env, target_ulong newsp)
> +static inline void cpu_clone_regs(CPUXtensaState *env, CPUXtensaState *old_env,
> +                                  target_ulong newsp)
>  {
>      if (newsp) {
>          env->regs[1] = newsp;
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 5bf8d13de7..7273a2fe54 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -6442,7 +6442,7 @@ static int do_fork(CPUArchState *env, unsigned int flags, abi_ulong newsp,
>          /* we create a new CPU instance. */
>          new_env = cpu_copy(env);
>          /* Init regs that differ from the parent.  */
> -        cpu_clone_regs(new_env, newsp);
> +        cpu_clone_regs(new_env, env, newsp);
>          new_cpu = ENV_GET_CPU(new_env);
>          new_cpu->opaque = ts;
>          ts->bprm = parent_ts->bprm;


--
Alex Bennée

Re: [Qemu-devel] [PATCH 1/4] linux-user: Disallow setting newsp for fork

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> Or really, just clone devolving into fork.  This should not ever happen
> in practice.  We do want to reserve calling cpu_clone_regs for the case
> in which we are actually performing a clone.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  linux-user/syscall.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index dfc851cc35..5bf8d13de7 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -6502,10 +6502,14 @@ static int do_fork(CPUArchState *env, unsigned int flags, abi_ulong newsp,
>          pthread_mutex_destroy(&info.mutex);
>          pthread_mutex_unlock(&clone_lock);
>      } else {
> -        /* if no CLONE_VM, we consider it is a fork */
> +        /* If no CLONE_VM, we consider it is a fork.  */
>          if (flags & CLONE_INVALID_FORK_FLAGS) {
>              return -TARGET_EINVAL;
>          }
> +        /* As a fork, setting a new sp does not make sense.  */
> +        if (newsp) {
> +            return -TARGET_EINVAL;
> +        }
>
>          /* We can't support custom termination signals */
>          if ((flags & CSIGNAL) != TARGET_SIGCHLD) {
> @@ -6520,7 +6524,6 @@ static int do_fork(CPUArchState *env, unsigned int flags, abi_ulong newsp,
>          ret = fork();
>          if (ret == 0) {
>              /* Child Process.  */
> -            cpu_clone_regs(env, newsp);
>              fork_end(1);
>              /* There is a race condition here.  The parent process could
>                 theoretically read the TID in the child process before the child


--
Alex Bennée