* [Qemu-devel] [PATCH 0/3] linux-user/sparc: Fixes for clone
@ 2018-07-30 20:15 Richard Henderson
2018-07-30 20:15 ` [Qemu-devel] [PATCH 1/4] linux-user: Disallow setting newsp for fork Richard Henderson
` (4 more replies)
0 siblings, 5 replies; 8+ messages in thread
From: Richard Henderson @ 2018-07-30 20:15 UTC (permalink / raw)
To: qemu-devel; +Cc: alex.bennee, laurent, mark.cave-ayland, atar4qemu
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
^ permalink raw reply [flat|nested] 8+ messages in thread* [Qemu-devel] [PATCH 1/4] linux-user: Disallow setting newsp for fork 2018-07-30 20:15 [Qemu-devel] [PATCH 0/3] linux-user/sparc: Fixes for clone Richard Henderson @ 2018-07-30 20:15 ` Richard Henderson 2018-07-31 10:54 ` Alex Bennée 2018-07-30 20:15 ` [Qemu-devel] [PATCH 2/4] linux-user: Pass the parent env to cpu_clone_regs Richard Henderson ` (3 subsequent siblings) 4 siblings, 1 reply; 8+ messages in thread From: Richard Henderson @ 2018-07-30 20:15 UTC (permalink / raw) To: qemu-devel; +Cc: alex.bennee, laurent, mark.cave-ayland, atar4qemu 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH 1/4] linux-user: Disallow setting newsp for fork 2018-07-30 20:15 ` [Qemu-devel] [PATCH 1/4] linux-user: Disallow setting newsp for fork Richard Henderson @ 2018-07-31 10:54 ` Alex Bennée 0 siblings, 0 replies; 8+ messages in thread From: Alex Bennée @ 2018-07-31 10:54 UTC (permalink / raw) To: Richard Henderson; +Cc: qemu-devel, laurent, mark.cave-ayland, atar4qemu 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [Qemu-devel] [PATCH 2/4] linux-user: Pass the parent env to cpu_clone_regs 2018-07-30 20:15 [Qemu-devel] [PATCH 0/3] linux-user/sparc: Fixes for clone Richard Henderson 2018-07-30 20:15 ` [Qemu-devel] [PATCH 1/4] linux-user: Disallow setting newsp for fork Richard Henderson @ 2018-07-30 20:15 ` Richard Henderson 2018-07-31 10:45 ` Alex Bennée 2018-07-30 20:15 ` [Qemu-devel] [PATCH 3/4] linux-user/sparc: Fix cpu_clone_regs Richard Henderson ` (2 subsequent siblings) 4 siblings, 1 reply; 8+ messages in thread From: Richard Henderson @ 2018-07-30 20:15 UTC (permalink / raw) To: qemu-devel; +Cc: alex.bennee, laurent, mark.cave-ayland, atar4qemu 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH 2/4] linux-user: Pass the parent env to cpu_clone_regs 2018-07-30 20:15 ` [Qemu-devel] [PATCH 2/4] linux-user: Pass the parent env to cpu_clone_regs Richard Henderson @ 2018-07-31 10:45 ` Alex Bennée 0 siblings, 0 replies; 8+ messages in thread From: Alex Bennée @ 2018-07-31 10:45 UTC (permalink / raw) To: Richard Henderson; +Cc: qemu-devel, laurent, mark.cave-ayland, atar4qemu 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [Qemu-devel] [PATCH 3/4] linux-user/sparc: Fix cpu_clone_regs 2018-07-30 20:15 [Qemu-devel] [PATCH 0/3] linux-user/sparc: Fixes for clone Richard Henderson 2018-07-30 20:15 ` [Qemu-devel] [PATCH 1/4] linux-user: Disallow setting newsp for fork Richard Henderson 2018-07-30 20:15 ` [Qemu-devel] [PATCH 2/4] linux-user: Pass the parent env to cpu_clone_regs Richard Henderson @ 2018-07-30 20:15 ` Richard Henderson 2018-07-30 20:15 ` [Qemu-devel] [PATCH 4/4] linux-user/sparc: Flush register windows before clone Richard Henderson 2018-07-31 7:09 ` [Qemu-devel] [PATCH 0/3] linux-user/sparc: Fixes for clone Alex Bennée 4 siblings, 0 replies; 8+ messages in thread From: Richard Henderson @ 2018-07-30 20:15 UTC (permalink / raw) To: qemu-devel; +Cc: alex.bennee, laurent, mark.cave-ayland, atar4qemu 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [Qemu-devel] [PATCH 4/4] linux-user/sparc: Flush register windows before clone 2018-07-30 20:15 [Qemu-devel] [PATCH 0/3] linux-user/sparc: Fixes for clone Richard Henderson ` (2 preceding siblings ...) 2018-07-30 20:15 ` [Qemu-devel] [PATCH 3/4] linux-user/sparc: Fix cpu_clone_regs Richard Henderson @ 2018-07-30 20:15 ` Richard Henderson 2018-07-31 7:09 ` [Qemu-devel] [PATCH 0/3] linux-user/sparc: Fixes for clone Alex Bennée 4 siblings, 0 replies; 8+ messages in thread From: Richard Henderson @ 2018-07-30 20:15 UTC (permalink / raw) To: qemu-devel; +Cc: alex.bennee, laurent, mark.cave-ayland, atar4qemu 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH 0/3] linux-user/sparc: Fixes for clone 2018-07-30 20:15 [Qemu-devel] [PATCH 0/3] linux-user/sparc: Fixes for clone Richard Henderson ` (3 preceding siblings ...) 2018-07-30 20:15 ` [Qemu-devel] [PATCH 4/4] linux-user/sparc: Flush register windows before clone Richard Henderson @ 2018-07-31 7:09 ` Alex Bennée 4 siblings, 0 replies; 8+ messages in thread From: Alex Bennée @ 2018-07-31 7:09 UTC (permalink / raw) To: Richard Henderson; +Cc: qemu-devel, laurent, mark.cave-ayland, atar4qemu 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-07-31 10:54 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-07-30 20:15 [Qemu-devel] [PATCH 0/3] linux-user/sparc: Fixes for clone Richard Henderson 2018-07-30 20:15 ` [Qemu-devel] [PATCH 1/4] linux-user: Disallow setting newsp for fork Richard Henderson 2018-07-31 10:54 ` Alex Bennée 2018-07-30 20:15 ` [Qemu-devel] [PATCH 2/4] linux-user: Pass the parent env to cpu_clone_regs Richard Henderson 2018-07-31 10:45 ` Alex Bennée 2018-07-30 20:15 ` [Qemu-devel] [PATCH 3/4] linux-user/sparc: Fix cpu_clone_regs Richard Henderson 2018-07-30 20:15 ` [Qemu-devel] [PATCH 4/4] linux-user/sparc: Flush register windows before clone Richard Henderson 2018-07-31 7:09 ` [Qemu-devel] [PATCH 0/3] linux-user/sparc: Fixes for clone Alex Bennée
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.