[PATCH v2 for-7.1 0/8] linux-user/nios2: Fix clone and sigreturn

[PATCH v2 for-7.1 0/8] linux-user/nios2: Fix clone and sigreturn

[Richard Henderson]

These two syscalls are the reason that the generic linux-user
tests were failing, which allows us to re-enable the tests.

Changes for v2:
  * Hoist the pc advance to apply to all EXCP_TRAP.
    Fixing this properly requires larger changes outside of linux-user;
    deferred until EIC patch set also committed.
  * Treat ret < 0 as error, per the kernel syscall path.
  * Return ESIGRETURN from badframe path.


r~


Richard Henderson (8):
  linux-user/nios2: Hoist pc advance to the top of EXCP_TRAP
  linux-user/nios2: Fix clone child return
  linux-user/nios2: Drop syscall 0 "workaround"
  linux-user/nios2: Adjust error return
  linux-user/nios2: Handle special qemu syscall return values
  linux-user/nios2: Remove do_sigreturn
  linux-user/nios2: Use QEMU_ESIGRETURN from do_rt_sigreturn
  tests/tcg/nios2: Re-enable linux-user tests

 linux-user/nios2/target_cpu.h   |  1 +
 linux-user/nios2/cpu_loop.c     | 27 ++++++++++++++++++++-------
 linux-user/nios2/signal.c       | 19 ++++---------------
 tests/tcg/nios2/Makefile.target | 11 -----------
 4 files changed, 25 insertions(+), 33 deletions(-)
 delete mode 100644 tests/tcg/nios2/Makefile.target

-- 
2.25.1

[PATCH v2 1/8] linux-user/nios2: Hoist pc advance to the top of EXCP_TRAP

[Richard Henderson]

Note that this advance *should* be done by the translator, as
that's the pc value that's supposed to be generated by hardware.
However, that's a much larger change across sysemu as well.

In the meantime, produce the correct PC for any signals raised
by the trap instruction.

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

diff --git a/linux-user/nios2/cpu_loop.c b/linux-user/nios2/cpu_loop.c
index 1e93ef34e6..2768ae61dd 100644
--- a/linux-user/nios2/cpu_loop.c
+++ b/linux-user/nios2/cpu_loop.c
@@ -40,6 +40,12 @@ void cpu_loop(CPUNios2State *env)
             break;
 
         case EXCP_TRAP:
+            /*
+             * TODO: This advance should be done in the translator, as
+             * hardware produces an advanced pc as part of all exceptions.
+             */
+            env->regs[R_PC] += 4;
+
             switch (env->error_code) {
             case 0:
                 qemu_log_mask(CPU_LOG_INT, "\nSyscall\n");
@@ -56,7 +62,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:
@@ -99,7 +104,6 @@ void cpu_loop(CPUNios2State *env)
                     o = env->regs[5];
                     n = env->regs[6];
                     env->regs[2] = qatomic_cmpxchg(h, o, n) - o;
-                    env->regs[R_PC] += 4;
                 }
                 break;
             }
-- 
2.25.1

[PATCH v2 2/8] linux-user/nios2: Fix clone child return

[Richard Henderson]

The child side of clone needs to set the secondary
syscall return value, r7, to indicate syscall success.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/nios2/target_cpu.h | 1 +
 1 file changed, 1 insertion(+)

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)
-- 
2.25.1

[PATCH v2 3/8] linux-user/nios2: Drop syscall 0 "workaround"

[Richard Henderson]

Syscall 0 is __NR_io_setup for this target; there is nothing
to work around.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Fixes: a0a839b65b6 ("nios2: Add usermode binaries emulation")
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/nios2/cpu_loop.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/linux-user/nios2/cpu_loop.c b/linux-user/nios2/cpu_loop.c
index 2768ae61dd..a285781fc5 100644
--- a/linux-user/nios2/cpu_loop.c
+++ b/linux-user/nios2/cpu_loop.c
@@ -55,10 +55,6 @@ void cpu_loop(CPUNios2State *env)
                                  env->regs[7], env->regs[8], env->regs[9],
                                  0, 0);
 
-                if (env->regs[2] == 0) {    /* FIXME: syscall 0 workaround */
-                    ret = 0;
-                }
-
                 env->regs[2] = abs(ret);
                 /* Return value is 0..4096 */
                 env->regs[7] = ret > 0xfffff000u;
-- 
2.25.1

[PATCH v2 4/8] linux-user/nios2: Adjust error return

[Richard Henderson]

Follow the kernel assembly, which considers all negative
return values to be errors.

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

diff --git a/linux-user/nios2/cpu_loop.c b/linux-user/nios2/cpu_loop.c
index a285781fc5..969460df58 100644
--- a/linux-user/nios2/cpu_loop.c
+++ b/linux-user/nios2/cpu_loop.c
@@ -55,9 +55,14 @@ void cpu_loop(CPUNios2State *env)
                                  env->regs[7], env->regs[8], env->regs[9],
                                  0, 0);
 
+                /*
+                 * See the code after translate_rc_and_ret: all negative
+                 * values are errors (aided by userspace restricted to 2G),
+                 * errno is returned positive in r2, and error indication
+                 * is a boolean in r7.
+                 */
                 env->regs[2] = abs(ret);
-                /* Return value is 0..4096 */
-                env->regs[7] = ret > 0xfffff000u;
+                env->regs[7] = ret < 0;
                 break;
 
             case 1:
-- 
2.25.1

[PATCH v2 5/8] linux-user/nios2: Handle special qemu syscall return values

[Richard Henderson]

Honor QEMU_ESIGRETURN and QEMU_ERESTARTSYS.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/nios2/cpu_loop.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/linux-user/nios2/cpu_loop.c b/linux-user/nios2/cpu_loop.c
index 969460df58..dffe48b8e8 100644
--- a/linux-user/nios2/cpu_loop.c
+++ b/linux-user/nios2/cpu_loop.c
@@ -55,6 +55,14 @@ void cpu_loop(CPUNios2State *env)
                                  env->regs[7], env->regs[8], env->regs[9],
                                  0, 0);
 
+                if (ret == -QEMU_ESIGRETURN) {
+                    /* rt_sigreturn has set all state. */
+                    break;
+                }
+                if (ret == -QEMU_ERESTARTSYS) {
+                    env->regs[R_PC] -= 4;
+                    break;
+                }
                 /*
                  * See the code after translate_rc_and_ret: all negative
                  * values are errors (aided by userspace restricted to 2G),
-- 
2.25.1

[PATCH v2 6/8] linux-user/nios2: Remove do_sigreturn

[Richard Henderson]

There is no sigreturn syscall, only rt_sigreturn.
This function is unused.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/nios2/signal.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/linux-user/nios2/signal.c b/linux-user/nios2/signal.c
index 517cd39270..133bc05673 100644
--- a/linux-user/nios2/signal.c
+++ b/linux-user/nios2/signal.c
@@ -185,13 +185,6 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
     unlock_user_struct(frame, frame_addr, 1);
 }
 
-long do_sigreturn(CPUNios2State *env)
-{
-    trace_user_do_sigreturn(env, 0);
-    qemu_log_mask(LOG_UNIMP, "do_sigreturn: not implemented\n");
-    return -TARGET_ENOSYS;
-}
-
 long do_rt_sigreturn(CPUNios2State *env)
 {
     /* Verify, can we follow the stack back */
-- 
2.25.1

[PATCH v2 7/8] linux-user/nios2: Use QEMU_ESIGRETURN from do_rt_sigreturn

[Richard Henderson]

Drop the kernel-specific "pr2" code structure and use
the qemu-specific error return value.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/nios2/signal.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/linux-user/nios2/signal.c b/linux-user/nios2/signal.c
index 133bc05673..9aa525e723 100644
--- a/linux-user/nios2/signal.c
+++ b/linux-user/nios2/signal.c
@@ -77,8 +77,7 @@ static void rt_setup_ucontext(struct target_ucontext *uc, CPUNios2State *env)
     __put_user(env->regs[R_SP], &gregs[28]);
 }
 
-static int rt_restore_ucontext(CPUNios2State *env, struct target_ucontext *uc,
-                               int *pr2)
+static int rt_restore_ucontext(CPUNios2State *env, struct target_ucontext *uc)
 {
     int temp;
     unsigned long *gregs = uc->tuc_mcontext.gregs;
@@ -128,8 +127,6 @@ static int rt_restore_ucontext(CPUNios2State *env, struct target_ucontext *uc,
     __get_user(env->regs[R_SP], &gregs[28]);
 
     target_restore_altstack(&uc->tuc_stack, env);
-
-    *pr2 = env->regs[2];
     return 0;
 }
 
@@ -191,7 +188,6 @@ long do_rt_sigreturn(CPUNios2State *env)
     abi_ulong frame_addr = env->regs[R_SP];
     struct target_rt_sigframe *frame;
     sigset_t set;
-    int rval;
 
     if (!lock_user_struct(VERIFY_READ, frame, frame_addr, 1)) {
         goto badframe;
@@ -200,15 +196,15 @@ long do_rt_sigreturn(CPUNios2State *env)
     target_to_host_sigset(&set, &frame->uc.tuc_sigmask);
     set_sigmask(&set);
 
-    if (rt_restore_ucontext(env, &frame->uc, &rval)) {
+    if (rt_restore_ucontext(env, &frame->uc)) {
         goto badframe;
     }
 
     unlock_user_struct(frame, frame_addr, 0);
-    return rval;
+    return -QEMU_ESIGRETURN;
 
 badframe:
     unlock_user_struct(frame, frame_addr, 0);
     force_sig(TARGET_SIGSEGV);
-    return 0;
+    return -QEMU_ESIGRETURN;
 }
-- 
2.25.1

[PATCH v2 8/8] tests/tcg/nios2: Re-enable linux-user tests

[Richard Henderson]

Now that signal handling has been fixed, re-enable tests.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 tests/tcg/nios2/Makefile.target | 11 -----------
 1 file changed, 11 deletions(-)
 delete mode 100644 tests/tcg/nios2/Makefile.target

diff --git a/tests/tcg/nios2/Makefile.target b/tests/tcg/nios2/Makefile.target
deleted file mode 100644
index b38e2352b7..0000000000
--- a/tests/tcg/nios2/Makefile.target
+++ /dev/null
@@ -1,11 +0,0 @@
-# nios2 specific test tweaks
-
-# Currently nios2 signal handling is broken
-run-signals: signals
-	$(call skip-test, $<, "BROKEN")
-run-plugin-signals-with-%:
-	$(call skip-test, $<, "BROKEN")
-run-linux-test: linux-test
-	$(call skip-test, $<, "BROKEN")
-run-plugin-linux-test-with-%:
-	$(call skip-test, $<, "BROKEN")
-- 
2.25.1

Re: [PATCH v2 1/8] linux-user/nios2: Hoist pc advance to the top of EXCP_TRAP

[Peter Maydell]

On Fri, 25 Mar 2022 at 19:52, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Note that this advance *should* be done by the translator, as
> that's the pc value that's supposed to be generated by hardware.
> However, that's a much larger change across sysemu as well.
>
> In the meantime, produce the correct PC for any signals raised
> by the trap instruction.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  linux-user/nios2/cpu_loop.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/linux-user/nios2/cpu_loop.c b/linux-user/nios2/cpu_loop.c
> index 1e93ef34e6..2768ae61dd 100644
> --- a/linux-user/nios2/cpu_loop.c
> +++ b/linux-user/nios2/cpu_loop.c
> @@ -40,6 +40,12 @@ void cpu_loop(CPUNios2State *env)
>              break;
>
>          case EXCP_TRAP:
> +            /*
> +             * TODO: This advance should be done in the translator, as
> +             * hardware produces an advanced pc as part of all exceptions.
> +             */
> +            env->regs[R_PC] += 4;
> +

You need to rewind again for the TRAP_BRKPT case, as the
kernel does fp->ea -= 4 in breakpoint_c().

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH v2 4/8] linux-user/nios2: Adjust error return

[Peter Maydell]

On Fri, 25 Mar 2022 at 19:52, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Follow the kernel assembly, which considers all negative
> return values to be errors.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  linux-user/nios2/cpu_loop.c | 9 +++++++--

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH v2 7/8] linux-user/nios2: Use QEMU_ESIGRETURN from do_rt_sigreturn

[Peter Maydell]

On Fri, 25 Mar 2022 at 19:53, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Drop the kernel-specific "pr2" code structure and use
> the qemu-specific error return value.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH v2 3/8] linux-user/nios2: Drop syscall 0 "workaround"

[Richard Henderson]

Syscall 0 is __NR_io_setup for this target; there is nothing
to work around.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Fixes: a0a839b65b6 ("nios2: Add usermode binaries emulation")
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/nios2/cpu_loop.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/linux-user/nios2/cpu_loop.c b/linux-user/nios2/cpu_loop.c
index 2768ae61dd..a285781fc5 100644
--- a/linux-user/nios2/cpu_loop.c
+++ b/linux-user/nios2/cpu_loop.c
@@ -55,10 +55,6 @@ void cpu_loop(CPUNios2State *env)
                                  env->regs[7], env->regs[8], env->regs[9],
                                  0, 0);
 
-                if (env->regs[2] == 0) {    /* FIXME: syscall 0 workaround */
-                    ret = 0;
-                }
-
                 env->regs[2] = abs(ret);
                 /* Return value is 0..4096 */
                 env->regs[7] = ret > 0xfffff000u;
-- 
2.25.1

Re: [PATCH v2 1/8] linux-user/nios2: Hoist pc advance to the top of EXCP_TRAP

[Richard Henderson]

On 3/26/22 06:50, Peter Maydell wrote:
> On Fri, 25 Mar 2022 at 19:52, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> Note that this advance *should* be done by the translator, as
>> that's the pc value that's supposed to be generated by hardware.
>> However, that's a much larger change across sysemu as well.
>>
>> In the meantime, produce the correct PC for any signals raised
>> by the trap instruction.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>   linux-user/nios2/cpu_loop.c | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/linux-user/nios2/cpu_loop.c b/linux-user/nios2/cpu_loop.c
>> index 1e93ef34e6..2768ae61dd 100644
>> --- a/linux-user/nios2/cpu_loop.c
>> +++ b/linux-user/nios2/cpu_loop.c
>> @@ -40,6 +40,12 @@ void cpu_loop(CPUNios2State *env)
>>               break;
>>
>>           case EXCP_TRAP:
>> +            /*
>> +             * TODO: This advance should be done in the translator, as
>> +             * hardware produces an advanced pc as part of all exceptions.
>> +             */
>> +            env->regs[R_PC] += 4;
>> +
> 
> You need to rewind again for the TRAP_BRKPT case, as the
> kernel does fp->ea -= 4 in breakpoint_c().

Yep, thanks.


r~