Create and record the two signal trampolines.
Use them when the guest does not use SA_RESTORER.

Cc: qemu-s390x@nongnu.org
Tested-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/s390x/target_signal.h |  2 ++
 linux-user/s390x/signal.c        | 24 ++++++++++++++++--------
 2 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/linux-user/s390x/target_signal.h b/linux-user/s390x/target_signal.h
index bbfc464d44..64f5f42201 100644
--- a/linux-user/s390x/target_signal.h
+++ b/linux-user/s390x/target_signal.h
@@ -19,4 +19,6 @@ typedef struct target_sigaltstack {
 #include "../generic/signal.h"
 
 #define TARGET_ARCH_HAS_SETUP_FRAME
+#define TARGET_ARCH_HAS_SIGTRAMP_PAGE 1
+
 #endif /* S390X_TARGET_SIGNAL_H */
diff --git a/linux-user/s390x/signal.c b/linux-user/s390x/signal.c
index 80f34086d7..676b948147 100644
--- a/linux-user/s390x/signal.c
+++ b/linux-user/s390x/signal.c
@@ -68,7 +68,6 @@ typedef struct {
     target_sigregs sregs;
     int signo;
     target_sigregs_ext sregs_ext;
-    uint16_t retcode;
 } sigframe;
 
 #define TARGET_UC_VXRS 2
@@ -85,7 +84,6 @@ struct target_ucontext {
 
 typedef struct {
     uint8_t callee_used_stack[__SIGNAL_FRAMESIZE];
-    uint16_t retcode;
     struct target_siginfo info;
     struct target_ucontext uc;
 } rt_sigframe;
@@ -209,9 +207,7 @@ void setup_frame(int sig, struct target_sigaction *ka,
     if (ka->sa_flags & TARGET_SA_RESTORER) {
         restorer = ka->sa_restorer;
     } else {
-        restorer = frame_addr + offsetof(sigframe, retcode);
-        __put_user(S390_SYSCALL_OPCODE | TARGET_NR_sigreturn,
-                   &frame->retcode);
+        restorer = default_sigreturn;
     }
 
     /* Set up registers for signal handler */
@@ -262,9 +258,7 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
     if (ka->sa_flags & TARGET_SA_RESTORER) {
         restorer = ka->sa_restorer;
     } else {
-        restorer = frame_addr + offsetof(typeof(*frame), retcode);
-        __put_user(S390_SYSCALL_OPCODE | TARGET_NR_rt_sigreturn,
-                   &frame->retcode);
+        restorer = default_rt_sigreturn;
     }
 
     /* Create siginfo on the signal stack. */
@@ -405,3 +399,17 @@ long do_rt_sigreturn(CPUS390XState *env)
     unlock_user_struct(frame, frame_addr, 0);
     return -TARGET_QEMU_ESIGRETURN;
 }
+
+void setup_sigtramp(abi_ulong sigtramp_page)
+{
+    uint16_t *tramp = lock_user(VERIFY_WRITE, sigtramp_page, 2 + 2, 0);
+    assert(tramp != NULL);
+
+    default_sigreturn = sigtramp_page;
+    __put_user(S390_SYSCALL_OPCODE | TARGET_NR_sigreturn, &tramp[0]);
+
+    default_rt_sigreturn = sigtramp_page + 2;
+    __put_user(S390_SYSCALL_OPCODE | TARGET_NR_rt_sigreturn, &tramp[1]);
+
+    unlock_user(tramp, sigtramp_page, 2 + 2);
+}
-- 
2.25.1

Richard Henderson <richard.henderson@linaro.org> wrote:

>Create and record the two signal trampolines.
>Use them when the guest does not use SA_RESTORER.

This patch caused a regression when running the wasmtime CI under qemu:
https://github.com/bytecodealliance/wasmtime/pull/4076

The problem is that this part:

>diff --git a/linux-user/s390x/signal.c b/linux-user/s390x/signal.c
>index 80f34086d7..676b948147 100644
>--- a/linux-user/s390x/signal.c
>+++ b/linux-user/s390x/signal.c
>@@ -68,7 +68,6 @@ typedef struct {
>     target_sigregs sregs;
>     int signo;
>     target_sigregs_ext sregs_ext;
>-    uint16_t retcode;
> } sigframe;
> 
> #define TARGET_UC_VXRS 2
>@@ -85,7 +84,6 @@ struct target_ucontext {
> 
> typedef struct {
>     uint8_t callee_used_stack[__SIGNAL_FRAMESIZE];
>-    uint16_t retcode;
>     struct target_siginfo info;
>     struct target_ucontext uc;
> } rt_sigframe;

changes the layout of the signal stack frame that is visible from user
space.  Some user space code, in particular the GCC unwinder
(s390_fallback_frame_state in libgcc), relies on that layout and no
longer works correctly if it is changed.


Reverting just those two hunks above on top of QEMU 7.0 makes the
wasmtime CI pass again.  (Actually, just the second hunk is enough; the
first hunk is not visible since the removed variable is at the very top
of the frame.)


Bye,
Ulrich

On 4/28/22 11:15, Ulrich Weigand wrote:
> Richard Henderson <richard.henderson@linaro.org> wrote:
> 
>> Create and record the two signal trampolines.
>> Use them when the guest does not use SA_RESTORER.
> 
> This patch caused a regression when running the wasmtime CI under qemu:
> https://github.com/bytecodealliance/wasmtime/pull/4076
> 
> The problem is that this part:
> 
>> diff --git a/linux-user/s390x/signal.c b/linux-user/s390x/signal.c
>> index 80f34086d7..676b948147 100644
>> --- a/linux-user/s390x/signal.c
>> +++ b/linux-user/s390x/signal.c
>> @@ -68,7 +68,6 @@ typedef struct {
>>      target_sigregs sregs;
>>      int signo;
>>      target_sigregs_ext sregs_ext;
>> -    uint16_t retcode;
>> } sigframe;
>>
>> #define TARGET_UC_VXRS 2
>> @@ -85,7 +84,6 @@ struct target_ucontext {
>>
>> typedef struct {
>>      uint8_t callee_used_stack[__SIGNAL_FRAMESIZE];
>> -    uint16_t retcode;
>>      struct target_siginfo info;
>>      struct target_ucontext uc;
>> } rt_sigframe;
> 
> changes the layout of the signal stack frame that is visible from user
> space.  Some user space code, in particular the GCC unwinder
> (s390_fallback_frame_state in libgcc), relies on that layout and no
> longer works correctly if it is changed.
> 
> 
> Reverting just those two hunks above on top of QEMU 7.0 makes the
> wasmtime CI pass again.  (Actually, just the second hunk is enough; the
> first hunk is not visible since the removed variable is at the very top
> of the frame.)

Ah, quite right -- I had read the comment for sigframe,

         __u16 svc_insn;         /* Offset of svc_insn is NOT fixed! */


and incorrectly assumed that applied to rt_sigframe too.
So, yes, the second hunk should be reverted, with a comment that it is not used and not 
even initialized by the kernel.


r~