[PATCH 0/7] linux-user/nios2: Fix clone and sigreturn

[PATCH 0/7] 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.


r~


Richard Henderson (7):
  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     | 29 ++++++++++++++++++++---------
 linux-user/nios2/signal.c       | 17 +++--------------
 tests/tcg/nios2/Makefile.target | 11 -----------
 4 files changed, 24 insertions(+), 34 deletions(-)
 delete mode 100644 tests/tcg/nios2/Makefile.target

-- 
2.25.1

[PATCH 1/7] 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.

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

[PATCH 2/7] linux-user/nios2: Drop syscall 0 "workaround"

[Richard Henderson]

There's no documentation for what the problem was.

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 a3acaa92ca..ac71f4ee47 100644
--- a/linux-user/nios2/cpu_loop.c
+++ b/linux-user/nios2/cpu_loop.c
@@ -48,10 +48,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 3/7] linux-user/nios2: Adjust error return

[Richard Henderson]

Follow syscall_set_return_value rather than the kernel assembly
in setting the syscall return values.  Only negate ret on error.

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

diff --git a/linux-user/nios2/cpu_loop.c b/linux-user/nios2/cpu_loop.c
index ac71f4ee47..2ae94f4a95 100644
--- a/linux-user/nios2/cpu_loop.c
+++ b/linux-user/nios2/cpu_loop.c
@@ -48,9 +48,18 @@ void cpu_loop(CPUNios2State *env)
                                  env->regs[7], env->regs[8], env->regs[9],
                                  0, 0);
 
-                env->regs[2] = abs(ret);
-                /* Return value is 0..4096 */
-                env->regs[7] = ret > 0xfffff000u;
+                /*
+                 * See syscall_set_return_value.
+                 * Use the QEMU traditional -515 error indication in
+                 * preference to the < 0 indication used in entry.S.
+                 */
+                if (ret > (abi_ulong)-515) {
+                    env->regs[2] = -ret;
+                    env->regs[7] = 1;
+                } else {
+                    env->regs[2] = ret;
+                    env->regs[7] = 0;
+                }
                 break;
 
             case 1:
-- 
2.25.1

[PATCH 4/7] linux-user/nios2: Handle special qemu syscall return values

[Richard Henderson]

Honor QEMU_ESIGRETURN and QEMU_ERESTARTSYS.

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 2ae94f4a95..d12c3c2852 100644
--- a/linux-user/nios2/cpu_loop.c
+++ b/linux-user/nios2/cpu_loop.c
@@ -48,6 +48,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 syscall_set_return_value.
                  * Use the QEMU traditional -515 error indication in
-- 
2.25.1

[PATCH 5/7] linux-user/nios2: Remove do_sigreturn

[Richard Henderson]

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

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 6/7] 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 | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/linux-user/nios2/signal.c b/linux-user/nios2/signal.c
index 133bc05673..4442974001 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,12 +196,12 @@ 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);
-- 
2.25.1

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

[Richard Henderson]

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

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 1/7] linux-user/nios2: Fix clone child return

[Peter Maydell]

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

Re: [PATCH 2/7] linux-user/nios2: Drop syscall 0 "workaround"

[Peter Maydell]

On Sun, 20 Mar 2022 at 16:09, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> There's no documentation for what the problem was.
>
> 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 a3acaa92ca..ac71f4ee47 100644
> --- a/linux-user/nios2/cpu_loop.c
> +++ b/linux-user/nios2/cpu_loop.c
> @@ -48,10 +48,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;
> --

syscall 0 on this architecture is __NR_io_setup, incidentally.

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

thanks
-- PMM

Re: [PATCH 3/7] linux-user/nios2: Adjust error return

[Peter Maydell]

On Sun, 20 Mar 2022 at 16:06, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Follow syscall_set_return_value rather than the kernel assembly
> in setting the syscall return values.  Only negate ret on error.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  linux-user/nios2/cpu_loop.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/linux-user/nios2/cpu_loop.c b/linux-user/nios2/cpu_loop.c
> index ac71f4ee47..2ae94f4a95 100644
> --- a/linux-user/nios2/cpu_loop.c
> +++ b/linux-user/nios2/cpu_loop.c
> @@ -48,9 +48,18 @@ void cpu_loop(CPUNios2State *env)
>                                   env->regs[7], env->regs[8], env->regs[9],
>                                   0, 0);
>
> -                env->regs[2] = abs(ret);
> -                /* Return value is 0..4096 */
> -                env->regs[7] = ret > 0xfffff000u;
> +                /*
> +                 * See syscall_set_return_value.
> +                 * Use the QEMU traditional -515 error indication in
> +                 * preference to the < 0 indication used in entry.S.
> +                 */

Well, it is traditional, in that we've used it for sparc for
instance right back to commit 060366c5ad18b3e in 2004, and
even earlier for ppc since commit 678673089d1b.
probably for about as long for ppc. But *why* do we use this?
Well, 516 is ERESTART_RESTARTBLOCK, and that's what the
arch/sparc/kernel/entry.S code is comparing against (it does a
greater-than-or-equal check, I think, hence 516, not 515).

For powerpc, however, the kernel handles setting the CCR
bit in syscall_exit_prepare(), and there it checks against
-MAX_ERRNO.

nios2, as you note in the commit message, does a < 0 check.

So I think:
 * 515 is correct for SPARC, but we really ought not to use
   a hardcoded constant there
 * we are checking against the wrong value for PPC and should
   be checking MAX_ERRNO
 * this patch does the wrong thing for nios2

If we do the same < 0 check that the kernel does, does anything
break ?

thanks
-- PMM

Re: [PATCH 4/7] linux-user/nios2: Handle special qemu syscall return values

[Peter Maydell]

On Sun, 20 Mar 2022 at 16:06, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Honor QEMU_ESIGRETURN and QEMU_ERESTARTSYS.
>
> 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 2ae94f4a95..d12c3c2852 100644
> --- a/linux-user/nios2/cpu_loop.c
> +++ b/linux-user/nios2/cpu_loop.c
> @@ -48,6 +48,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;
> +                }

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

thanks
-- PMM

Re: [PATCH 5/7] linux-user/nios2: Remove do_sigreturn

[Peter Maydell]

On Sun, 20 Mar 2022 at 16:09, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> There is no sigreturn syscall, only rt_sigreturn.
> This function is unused.
>
> 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;
> -}
>

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

thanks
-- PMM

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

[Peter Maydell]

On Sun, 20 Mar 2022 at 16:06, 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>
> ---
>  linux-user/nios2/signal.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/linux-user/nios2/signal.c b/linux-user/nios2/signal.c
> index 133bc05673..4442974001 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,12 +196,12 @@ 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);

Don't you also need to return -QEMU_ESIGRETURN in the badframe
error-handling case? The other guest architecture implementations
of do_sigreturn seem to do that.

thanks
-- PMM

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

[Peter Maydell]

On Sun, 20 Mar 2022 at 16:14, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Now that signal handling has been fixed, re-enable tests.
>
> 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

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

thanks
-- PMM

Re: [PATCH 3/7] linux-user/nios2: Adjust error return

[Peter Maydell]

On Fri, 25 Mar 2022 at 12:12, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Sun, 20 Mar 2022 at 16:06, Richard Henderson
> <richard.henderson@linaro.org> wrote:
> >
> > Follow syscall_set_return_value rather than the kernel assembly
> > in setting the syscall return values.  Only negate ret on error.
> >
> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> > ---
> >  linux-user/nios2/cpu_loop.c | 15 ++++++++++++---
> >  1 file changed, 12 insertions(+), 3 deletions(-)
> >
> > diff --git a/linux-user/nios2/cpu_loop.c b/linux-user/nios2/cpu_loop.c
> > index ac71f4ee47..2ae94f4a95 100644
> > --- a/linux-user/nios2/cpu_loop.c
> > +++ b/linux-user/nios2/cpu_loop.c
> > @@ -48,9 +48,18 @@ void cpu_loop(CPUNios2State *env)
> >                                   env->regs[7], env->regs[8], env->regs[9],
> >                                   0, 0);
> >
> > -                env->regs[2] = abs(ret);
> > -                /* Return value is 0..4096 */
> > -                env->regs[7] = ret > 0xfffff000u;
> > +                /*
> > +                 * See syscall_set_return_value.
> > +                 * Use the QEMU traditional -515 error indication in
> > +                 * preference to the < 0 indication used in entry.S.
> > +                 */
>
> Well, it is traditional, in that we've used it for sparc for
> instance right back to commit 060366c5ad18b3e in 2004, and
> even earlier for ppc since commit 678673089d1b.
> probably for about as long for ppc. But *why* do we use this?
> Well, 516 is ERESTART_RESTARTBLOCK, and that's what the
> arch/sparc/kernel/entry.S code is comparing against (it does a
> greater-than-or-equal check, I think, hence 516, not 515).
>
> For powerpc, however, the kernel handles setting the CCR
> bit in syscall_exit_prepare(), and there it checks against
> -MAX_ERRNO.

This turns out to be because in 2015 kernel commit c3525940cca5
switched powerpc from checking against 515/516 and instead made
them check MAX_ERRNO (4095).

(If anybody cared about seccomp on sparc hosts they'd probably
want to fix the sparc kernel similarly, but presumably nobody
does :-))

The kernel commit message mentions some infrastructure in
the form of force_successful_syscall_return() where syscall
implementations can force that a value above -MAX_ERRNO
is still treated as "success". In theory perhaps we should
have something similar...

-- PMM

Re: [PATCH 1/7] linux-user/nios2: Fix clone child return

[Richard Henderson]

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~

Re: [PATCH 3/7] linux-user/nios2: Adjust error return

[Richard Henderson]

On 3/25/22 07:37, Peter Maydell wrote:
> This turns out to be because in 2015 kernel commit c3525940cca5
> switched powerpc from checking against 515/516 and instead made
> them check MAX_ERRNO (4095).
> 
> (If anybody cared about seccomp on sparc hosts they'd probably
> want to fix the sparc kernel similarly, but presumably nobody
> does :-))

Indeed, thanks for the archaeology.

> The kernel commit message mentions some infrastructure in
> the form of force_successful_syscall_return() where syscall
> implementations can force that a value above -MAX_ERRNO
> is still treated as "success". In theory perhaps we should
> have something similar...

In theory, yes.  It affects 3 or 4 syscalls.
That said, nios2 doesn't define force_successful_syscall_return. :-P


r~

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

[Richard Henderson]

On 3/25/22 06:16, Peter Maydell wrote:
>> +    return -QEMU_ESIGRETURN;
>>
>>   badframe:
>>       unlock_user_struct(frame, frame_addr, 0);
> 
> Don't you also need to return -QEMU_ESIGRETURN in the badframe
> error-handling case? The other guest architecture implementations
> of do_sigreturn seem to do that.

Yep, good catch.

r~