| Message ID | 20220320160009.2665152-2-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | linux-user/nios2: Fix clone and sigreturn | expand |
On Sun, 20 Mar 2022 at 16:12, Richard Henderson <richard.henderson@linaro.org> wrote: > > The child side of clone needs to set the secondary > syscall return value, r7, to indicate syscall success. > > Advance the pc before do_syscall, so that the new thread > does not re-execute the clone syscall. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > linux-user/nios2/target_cpu.h | 1 + > linux-user/nios2/cpu_loop.c | 4 +--- > 2 files changed, 2 insertions(+), 3 deletions(-) > > diff --git a/linux-user/nios2/target_cpu.h b/linux-user/nios2/target_cpu.h > index 2d2008f002..830b4c0741 100644 > --- a/linux-user/nios2/target_cpu.h > +++ b/linux-user/nios2/target_cpu.h > @@ -27,6 +27,7 @@ static inline void cpu_clone_regs_child(CPUNios2State *env, target_ulong newsp, > env->regs[R_SP] = newsp; > } > env->regs[R_RET0] = 0; > + env->regs[7] = 0; > } > > static inline void cpu_clone_regs_parent(CPUNios2State *env, unsigned flags) > diff --git a/linux-user/nios2/cpu_loop.c b/linux-user/nios2/cpu_loop.c > index 1e93ef34e6..a3acaa92ca 100644 > --- a/linux-user/nios2/cpu_loop.c > +++ b/linux-user/nios2/cpu_loop.c > @@ -42,8 +42,7 @@ void cpu_loop(CPUNios2State *env) > case EXCP_TRAP: > switch (env->error_code) { > case 0: > - qemu_log_mask(CPU_LOG_INT, "\nSyscall\n"); > - Are you deliberately dropping this logging? If so, at least mention it in the commit message, but it doesn't really seem related to the rest of the patch... > + env->regs[R_PC] += 4; > ret = do_syscall(env, env->regs[2], > env->regs[4], env->regs[5], env->regs[6], > env->regs[7], env->regs[8], env->regs[9], > @@ -56,7 +55,6 @@ void cpu_loop(CPUNios2State *env) > env->regs[2] = abs(ret); > /* Return value is 0..4096 */ > env->regs[7] = ret > 0xfffff000u; > - env->regs[R_PC] += 4; > break; It feels a bit odd to be advancing the PC in the cpu-loop, because on the real hardware you get this for free because 'trap' sets ea to PC+4 and you just return to ea. But it works, I guess. Reviewed-by: Peter Maydell <peter.maydell@linaro.org> (The nios2 "use r2 and r7 for syscall return information" API seems like an unnecessary use of the architectural weirdness budget on their part, but whatever.) thanks -- PMM
On 3/25/22 05:49, Peter Maydell wrote: > On Sun, 20 Mar 2022 at 16:12, Richard Henderson > <richard.henderson@linaro.org> wrote: >> >> The child side of clone needs to set the secondary >> syscall return value, r7, to indicate syscall success. >> >> Advance the pc before do_syscall, so that the new thread >> does not re-execute the clone syscall. >> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >> --- >> linux-user/nios2/target_cpu.h | 1 + >> linux-user/nios2/cpu_loop.c | 4 +--- >> 2 files changed, 2 insertions(+), 3 deletions(-) >> >> diff --git a/linux-user/nios2/target_cpu.h b/linux-user/nios2/target_cpu.h >> index 2d2008f002..830b4c0741 100644 >> --- a/linux-user/nios2/target_cpu.h >> +++ b/linux-user/nios2/target_cpu.h >> @@ -27,6 +27,7 @@ static inline void cpu_clone_regs_child(CPUNios2State *env, target_ulong newsp, >> env->regs[R_SP] = newsp; >> } >> env->regs[R_RET0] = 0; >> + env->regs[7] = 0; >> } >> >> static inline void cpu_clone_regs_parent(CPUNios2State *env, unsigned flags) >> diff --git a/linux-user/nios2/cpu_loop.c b/linux-user/nios2/cpu_loop.c >> index 1e93ef34e6..a3acaa92ca 100644 >> --- a/linux-user/nios2/cpu_loop.c >> +++ b/linux-user/nios2/cpu_loop.c >> @@ -42,8 +42,7 @@ void cpu_loop(CPUNios2State *env) >> case EXCP_TRAP: >> switch (env->error_code) { >> case 0: >> - qemu_log_mask(CPU_LOG_INT, "\nSyscall\n"); >> - > > Are you deliberately dropping this logging? If so, at least > mention it in the commit message, but it doesn't really seem > related to the rest of the patch... It was intentional, but I meant to do it separately. >> @@ -56,7 +55,6 @@ void cpu_loop(CPUNios2State *env) >> env->regs[2] = abs(ret); >> /* Return value is 0..4096 */ >> env->regs[7] = ret > 0xfffff000u; >> - env->regs[R_PC] += 4; >> break; > > It feels a bit odd to be advancing the PC in the cpu-loop, because > on the real hardware you get this for free because 'trap' sets > ea to PC+4 and you just return to ea. But it works, I guess. I thought of this in relation to the other trap patch set. I think we should indeed raise the exception with the pc already advanced, as per hw. This would avoid the need for nios2_cpu_do_interrupt to do it, except in the case of EXCP_IRQ. But at present the translator is raising EXCP_TRAP with pc = trap insn. > (The nios2 "use r2 and r7 for syscall return information" API > seems like an unnecessary use of the architectural weirdness > budget on their part, but whatever.) Indeed. r~
diff --git a/linux-user/nios2/target_cpu.h b/linux-user/nios2/target_cpu.h index 2d2008f002..830b4c0741 100644 --- a/linux-user/nios2/target_cpu.h +++ b/linux-user/nios2/target_cpu.h @@ -27,6 +27,7 @@ static inline void cpu_clone_regs_child(CPUNios2State *env, target_ulong newsp, env->regs[R_SP] = newsp; } env->regs[R_RET0] = 0; + env->regs[7] = 0; } static inline void cpu_clone_regs_parent(CPUNios2State *env, unsigned flags) diff --git a/linux-user/nios2/cpu_loop.c b/linux-user/nios2/cpu_loop.c index 1e93ef34e6..a3acaa92ca 100644 --- a/linux-user/nios2/cpu_loop.c +++ b/linux-user/nios2/cpu_loop.c @@ -42,8 +42,7 @@ void cpu_loop(CPUNios2State *env) case EXCP_TRAP: switch (env->error_code) { case 0: - qemu_log_mask(CPU_LOG_INT, "\nSyscall\n"); - + env->regs[R_PC] += 4; ret = do_syscall(env, env->regs[2], env->regs[4], env->regs[5], env->regs[6], env->regs[7], env->regs[8], env->regs[9], @@ -56,7 +55,6 @@ void cpu_loop(CPUNios2State *env) env->regs[2] = abs(ret); /* Return value is 0..4096 */ env->regs[7] = ret > 0xfffff000u; - env->regs[R_PC] += 4; break; case 1:
The child side of clone needs to set the secondary syscall return value, r7, to indicate syscall success. Advance the pc before do_syscall, so that the new thread does not re-execute the clone syscall. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- linux-user/nios2/target_cpu.h | 1 + linux-user/nios2/cpu_loop.c | 4 +--- 2 files changed, 2 insertions(+), 3 deletions(-)