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");
-
+                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:
-- 
2.25.1

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~