[PATCH v2 00/15] linux-user/s390x: some signal fixes

[PATCH v2 00/15] linux-user/s390x: some signal fixes

[Richard Henderson]

Version 2 splits lazy do-it-all patch.
Patch 1 has an additional fix, so I dropped the r-b.

r~

Richard Henderson (15):
  linux-user/s390x: Fix sigframe types
  linux-user/s390x: Use uint16_t for signal retcode
  linux-user/s390x: Remove PSW_ADDR_AMODE
  linux-user/s390x: Remove restore_sigregs return value
  linux-user/s390x: Fix trace in restore_regs
  linux-user/s390x: Fix sigcontext sregs value
  linux-user/s390x: Use tswap_sigset in setup_rt_frame
  linux-user/s390x: Tidy save_sigregs
  linux-user/s390x: Clean up single-use gotos in signal.c
  linux-user/s390x: Set psw.mask properly for the signal handler
  linux-user/s390x: Add stub sigframe argument for last_break
  linux-user/s390x: Fix frame_addr corruption in setup_frame
  linux-user/s390x: Add build asserts for sigset sizes
  linux-user/s390x: Clean up signal.c
  linux-user/s390x: Handle vector regs in signal stack

 linux-user/s390x/signal.c | 280 +++++++++++++++++++++++---------------
 1 file changed, 170 insertions(+), 110 deletions(-)

-- 
2.25.1

[PATCH v2 01/15] linux-user/s390x: Fix sigframe types

[Richard Henderson]

Noticed via gitlab clang-user job:

  TEST    signals on s390x
../linux-user/s390x/signal.c:258:9: runtime error: \
  1.84467e+19 is outside the range of representable values of \
  type 'unsigned long'

Which points to the fact that we were performing a double-to-uint64_t
conversion while storing the fp registers, instead of just copying
the data across.

Turns out there are several errors:

target_ulong is the size of the target register, whereas abi_ulong
is the target 'unsigned long' type.  Not a big deal here, since we
only support 64-bit s390x, but not correct either.

In target_sigcontext and target ucontext, we used a host pointer
instead of a target pointer, aka abi_ulong.

Fixing this allows the removal of a cast to __put_user.

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

diff --git a/linux-user/s390x/signal.c b/linux-user/s390x/signal.c
index b68b44ae7e..707fb603d7 100644
--- a/linux-user/s390x/signal.c
+++ b/linux-user/s390x/signal.c
@@ -37,13 +37,14 @@
 
 typedef struct {
     target_psw_t psw;
-    target_ulong gprs[__NUM_GPRS];
-    unsigned int acrs[__NUM_ACRS];
+    abi_ulong gprs[__NUM_GPRS];
+    abi_uint acrs[__NUM_ACRS];
 } target_s390_regs_common;
 
 typedef struct {
-    unsigned int fpc;
-    double   fprs[__NUM_FPRS];
+    uint32_t fpc;
+    uint32_t pad;
+    uint64_t fprs[__NUM_FPRS];
 } target_s390_fp_regs;
 
 typedef struct {
@@ -51,22 +52,22 @@ typedef struct {
     target_s390_fp_regs     fpregs;
 } target_sigregs;
 
-struct target_sigcontext {
-    target_ulong   oldmask[_SIGCONTEXT_NSIG_WORDS];
-    target_sigregs *sregs;
-};
+typedef struct {
+    abi_ulong oldmask[_SIGCONTEXT_NSIG_WORDS];
+    abi_ulong sregs;
+} target_sigcontext;
 
 typedef struct {
     uint8_t callee_used_stack[__SIGNAL_FRAMESIZE];
-    struct target_sigcontext sc;
+    target_sigcontext sc;
     target_sigregs sregs;
     int signo;
     uint8_t retcode[S390_SYSCALL_SIZE];
 } sigframe;
 
 struct target_ucontext {
-    target_ulong tuc_flags;
-    struct target_ucontext *tuc_link;
+    abi_ulong tuc_flags;
+    abi_ulong tuc_link;
     target_stack_t tuc_stack;
     target_sigregs tuc_mcontext;
     target_sigset_t tuc_sigmask;   /* mask last for extensibility */
@@ -143,8 +144,7 @@ void setup_frame(int sig, struct target_sigaction *ka,
 
     save_sigregs(env, &frame->sregs);
 
-    __put_user((abi_ulong)(unsigned long)&frame->sregs,
-               (abi_ulong *)&frame->sc.sregs);
+    __put_user((abi_ulong)(unsigned long)&frame->sregs, &frame->sc.sregs);
 
     /* Set up to return from userspace.  If provided, use a stub
        already in userspace.  */
-- 
2.25.1

Re: [PATCH v2 01/15] linux-user/s390x: Fix sigframe types

[David Hildenbrand]

On 28.04.21 21:33, Richard Henderson wrote:
> Noticed via gitlab clang-user job:
> 
>    TEST    signals on s390x
> ../linux-user/s390x/signal.c:258:9: runtime error: \
>    1.84467e+19 is outside the range of representable values of \
>    type 'unsigned long'
> 
> Which points to the fact that we were performing a double-to-uint64_t
> conversion while storing the fp registers, instead of just copying
> the data across.
> 
> Turns out there are several errors:
> 
> target_ulong is the size of the target register, whereas abi_ulong
> is the target 'unsigned long' type.  Not a big deal here, since we
> only support 64-bit s390x, but not correct either.
> 
> In target_sigcontext and target ucontext, we used a host pointer
> instead of a target pointer, aka abi_ulong.
> 
> Fixing this allows the removal of a cast to __put_user.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   linux-user/s390x/signal.c | 26 +++++++++++++-------------
>   1 file changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/linux-user/s390x/signal.c b/linux-user/s390x/signal.c
> index b68b44ae7e..707fb603d7 100644
> --- a/linux-user/s390x/signal.c
> +++ b/linux-user/s390x/signal.c
> @@ -37,13 +37,14 @@
>   
>   typedef struct {
>       target_psw_t psw;
> -    target_ulong gprs[__NUM_GPRS];
> -    unsigned int acrs[__NUM_ACRS];
> +    abi_ulong gprs[__NUM_GPRS];
> +    abi_uint acrs[__NUM_ACRS];
>   } target_s390_regs_common;
>   
>   typedef struct {
> -    unsigned int fpc;
> -    double   fprs[__NUM_FPRS];
> +    uint32_t fpc;
> +    uint32_t pad;
> +    uint64_t fprs[__NUM_FPRS];
>   } target_s390_fp_regs;
>   
>   typedef struct {
> @@ -51,22 +52,22 @@ typedef struct {
>       target_s390_fp_regs     fpregs;
>   } target_sigregs;
>   
> -struct target_sigcontext {
> -    target_ulong   oldmask[_SIGCONTEXT_NSIG_WORDS];
> -    target_sigregs *sregs;
> -};
> +typedef struct {
> +    abi_ulong oldmask[_SIGCONTEXT_NSIG_WORDS];
> +    abi_ulong sregs;
> +} target_sigcontext;
>   
>   typedef struct {
>       uint8_t callee_used_stack[__SIGNAL_FRAMESIZE];
> -    struct target_sigcontext sc;
> +    target_sigcontext sc;
>       target_sigregs sregs;
>       int signo;
>       uint8_t retcode[S390_SYSCALL_SIZE];
>   } sigframe;
>   
>   struct target_ucontext {
> -    target_ulong tuc_flags;
> -    struct target_ucontext *tuc_link;
> +    abi_ulong tuc_flags;
> +    abi_ulong tuc_link;
>       target_stack_t tuc_stack;
>       target_sigregs tuc_mcontext;
>       target_sigset_t tuc_sigmask;   /* mask last for extensibility */
> @@ -143,8 +144,7 @@ void setup_frame(int sig, struct target_sigaction *ka,
>   
>       save_sigregs(env, &frame->sregs);
>   
> -    __put_user((abi_ulong)(unsigned long)&frame->sregs,
> -               (abi_ulong *)&frame->sc.sregs);
> +    __put_user((abi_ulong)(unsigned long)&frame->sregs, &frame->sc.sregs);
>   
>       /* Set up to return from userspace.  If provided, use a stub
>          already in userspace.  */
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

[PATCH v2 02/15] linux-user/s390x: Use uint16_t for signal retcode

[Richard Henderson]

Using the right type simplifies the frame setup.

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

diff --git a/linux-user/s390x/signal.c b/linux-user/s390x/signal.c
index 707fb603d7..fece8ab97b 100644
--- a/linux-user/s390x/signal.c
+++ b/linux-user/s390x/signal.c
@@ -25,7 +25,6 @@
 #define __NUM_FPRS 16
 #define __NUM_ACRS 16
 
-#define S390_SYSCALL_SIZE   2
 #define __SIGNAL_FRAMESIZE      160 /* FIXME: 31-bit mode -> 96 */
 
 #define _SIGCONTEXT_NSIG        64
@@ -62,7 +61,7 @@ typedef struct {
     target_sigcontext sc;
     target_sigregs sregs;
     int signo;
-    uint8_t retcode[S390_SYSCALL_SIZE];
+    uint16_t retcode;
 } sigframe;
 
 struct target_ucontext {
@@ -75,7 +74,7 @@ struct target_ucontext {
 
 typedef struct {
     uint8_t callee_used_stack[__SIGNAL_FRAMESIZE];
-    uint8_t retcode[S390_SYSCALL_SIZE];
+    uint16_t retcode;
     struct target_siginfo info;
     struct target_ucontext uc;
 } rt_sigframe;
@@ -155,7 +154,7 @@ void setup_frame(int sig, struct target_sigaction *ka,
         env->regs[14] = (frame_addr + offsetof(sigframe, retcode))
                         | PSW_ADDR_AMODE;
         __put_user(S390_SYSCALL_OPCODE | TARGET_NR_sigreturn,
-                   (uint16_t *)(frame->retcode));
+                   &frame->retcode);
     }
 
     /* Set up backchain. */
@@ -216,7 +215,7 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
         env->regs[14] = (frame_addr + offsetof(typeof(*frame), retcode))
                         | PSW_ADDR_AMODE;
         __put_user(S390_SYSCALL_OPCODE | TARGET_NR_rt_sigreturn,
-                   (uint16_t *)(frame->retcode));
+                   &frame->retcode);
     }
 
     /* Set up backchain. */
-- 
2.25.1

Re: [PATCH v2 02/15] linux-user/s390x: Use uint16_t for signal retcode

[David Hildenbrand]

On 28.04.21 21:33, Richard Henderson wrote:
> Using the right type simplifies the frame setup.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   linux-user/s390x/signal.c | 9 ++++-----
>   1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/linux-user/s390x/signal.c b/linux-user/s390x/signal.c
> index 707fb603d7..fece8ab97b 100644
> --- a/linux-user/s390x/signal.c
> +++ b/linux-user/s390x/signal.c
> @@ -25,7 +25,6 @@
>   #define __NUM_FPRS 16
>   #define __NUM_ACRS 16
>   
> -#define S390_SYSCALL_SIZE   2
>   #define __SIGNAL_FRAMESIZE      160 /* FIXME: 31-bit mode -> 96 */
>   
>   #define _SIGCONTEXT_NSIG        64
> @@ -62,7 +61,7 @@ typedef struct {
>       target_sigcontext sc;
>       target_sigregs sregs;
>       int signo;
> -    uint8_t retcode[S390_SYSCALL_SIZE];
> +    uint16_t retcode;
>   } sigframe;
>   
>   struct target_ucontext {
> @@ -75,7 +74,7 @@ struct target_ucontext {
>   
>   typedef struct {
>       uint8_t callee_used_stack[__SIGNAL_FRAMESIZE];
> -    uint8_t retcode[S390_SYSCALL_SIZE];
> +    uint16_t retcode;
>       struct target_siginfo info;
>       struct target_ucontext uc;
>   } rt_sigframe;
> @@ -155,7 +154,7 @@ void setup_frame(int sig, struct target_sigaction *ka,
>           env->regs[14] = (frame_addr + offsetof(sigframe, retcode))
>                           | PSW_ADDR_AMODE;
>           __put_user(S390_SYSCALL_OPCODE | TARGET_NR_sigreturn,
> -                   (uint16_t *)(frame->retcode));
> +                   &frame->retcode);
>       }
>   
>       /* Set up backchain. */
> @@ -216,7 +215,7 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
>           env->regs[14] = (frame_addr + offsetof(typeof(*frame), retcode))
>                           | PSW_ADDR_AMODE;
>           __put_user(S390_SYSCALL_OPCODE | TARGET_NR_rt_sigreturn,
> -                   (uint16_t *)(frame->retcode));
> +                   &frame->retcode);
>       }
>   
>       /* Set up backchain. */
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

[PATCH v2 03/15] linux-user/s390x: Remove PSW_ADDR_AMODE

[Richard Henderson]

This is an unnecessary complication since we only
support 64-bit mode.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/s390x/signal.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/linux-user/s390x/signal.c b/linux-user/s390x/signal.c
index fece8ab97b..1dfca71fa9 100644
--- a/linux-user/s390x/signal.c
+++ b/linux-user/s390x/signal.c
@@ -31,7 +31,6 @@
 #define _SIGCONTEXT_NSIG_BPW    64 /* FIXME: 31-bit mode -> 32 */
 #define _SIGCONTEXT_NSIG_WORDS  (_SIGCONTEXT_NSIG / _SIGCONTEXT_NSIG_BPW)
 #define _SIGMASK_COPY_SIZE    (sizeof(unsigned long)*_SIGCONTEXT_NSIG_WORDS)
-#define PSW_ADDR_AMODE            0x0000000000000000UL /* 0x80000000UL for 31-bit */
 #define S390_SYSCALL_OPCODE ((uint16_t)0x0a00)
 
 typedef struct {
@@ -148,11 +147,9 @@ void setup_frame(int sig, struct target_sigaction *ka,
     /* Set up to return from userspace.  If provided, use a stub
        already in userspace.  */
     if (ka->sa_flags & TARGET_SA_RESTORER) {
-        env->regs[14] = (unsigned long)
-                ka->sa_restorer | PSW_ADDR_AMODE;
+        env->regs[14] = ka->sa_restorer;
     } else {
-        env->regs[14] = (frame_addr + offsetof(sigframe, retcode))
-                        | PSW_ADDR_AMODE;
+        env->regs[14] = frame_addr + offsetof(sigframe, retcode);
         __put_user(S390_SYSCALL_OPCODE | TARGET_NR_sigreturn,
                    &frame->retcode);
     }
@@ -162,7 +159,7 @@ void setup_frame(int sig, struct target_sigaction *ka,
 
     /* Set up registers for signal handler */
     env->regs[15] = frame_addr;
-    env->psw.addr = (target_ulong) ka->_sa_handler | PSW_ADDR_AMODE;
+    env->psw.addr = ka->_sa_handler;
 
     env->regs[2] = sig; //map_signal(sig);
     env->regs[3] = frame_addr += offsetof(typeof(*frame), sc);
@@ -210,10 +207,9 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
     /* Set up to return from userspace.  If provided, use a stub
        already in userspace.  */
     if (ka->sa_flags & TARGET_SA_RESTORER) {
-        env->regs[14] = ka->sa_restorer | PSW_ADDR_AMODE;
+        env->regs[14] = ka->sa_restorer;
     } else {
-        env->regs[14] = (frame_addr + offsetof(typeof(*frame), retcode))
-                        | PSW_ADDR_AMODE;
+        env->regs[14] = frame_addr + offsetof(typeof(*frame), retcode);
         __put_user(S390_SYSCALL_OPCODE | TARGET_NR_rt_sigreturn,
                    &frame->retcode);
     }
@@ -223,7 +219,7 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
 
     /* Set up registers for signal handler */
     env->regs[15] = frame_addr;
-    env->psw.addr = (target_ulong) ka->_sa_handler | PSW_ADDR_AMODE;
+    env->psw.addr = ka->_sa_handler;
 
     env->regs[2] = sig; //map_signal(sig);
     env->regs[3] = frame_addr + offsetof(typeof(*frame), info);
@@ -248,7 +244,6 @@ restore_sigregs(CPUS390XState *env, target_sigregs *sc)
     trace_user_s390x_restore_sigregs(env, (unsigned long long)sc->regs.psw.addr,
                                      (unsigned long long)env->psw.addr);
     __get_user(env->psw.addr, &sc->regs.psw.addr);
-    /* FIXME: 31-bit -> | PSW_ADDR_AMODE */
 
     for (i = 0; i < 16; i++) {
         __get_user(env->aregs[i], &sc->regs.acrs[i]);
-- 
2.25.1

Re: [PATCH v2 03/15] linux-user/s390x: Remove PSW_ADDR_AMODE

[David Hildenbrand]

On 28.04.21 21:33, Richard Henderson wrote:
> This is an unnecessary complication since we only
> support 64-bit mode.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   linux-user/s390x/signal.c | 17 ++++++-----------
>   1 file changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/linux-user/s390x/signal.c b/linux-user/s390x/signal.c
> index fece8ab97b..1dfca71fa9 100644
> --- a/linux-user/s390x/signal.c
> +++ b/linux-user/s390x/signal.c
> @@ -31,7 +31,6 @@
>   #define _SIGCONTEXT_NSIG_BPW    64 /* FIXME: 31-bit mode -> 32 */
>   #define _SIGCONTEXT_NSIG_WORDS  (_SIGCONTEXT_NSIG / _SIGCONTEXT_NSIG_BPW)
>   #define _SIGMASK_COPY_SIZE    (sizeof(unsigned long)*_SIGCONTEXT_NSIG_WORDS)
> -#define PSW_ADDR_AMODE            0x0000000000000000UL /* 0x80000000UL for 31-bit */
>   #define S390_SYSCALL_OPCODE ((uint16_t)0x0a00)
>   
>   typedef struct {
> @@ -148,11 +147,9 @@ void setup_frame(int sig, struct target_sigaction *ka,
>       /* Set up to return from userspace.  If provided, use a stub
>          already in userspace.  */
>       if (ka->sa_flags & TARGET_SA_RESTORER) {
> -        env->regs[14] = (unsigned long)
> -                ka->sa_restorer | PSW_ADDR_AMODE;
> +        env->regs[14] = ka->sa_restorer;
>       } else {
> -        env->regs[14] = (frame_addr + offsetof(sigframe, retcode))
> -                        | PSW_ADDR_AMODE;
> +        env->regs[14] = frame_addr + offsetof(sigframe, retcode);
>           __put_user(S390_SYSCALL_OPCODE | TARGET_NR_sigreturn,
>                      &frame->retcode);
>       }
> @@ -162,7 +159,7 @@ void setup_frame(int sig, struct target_sigaction *ka,
>   
>       /* Set up registers for signal handler */
>       env->regs[15] = frame_addr;
> -    env->psw.addr = (target_ulong) ka->_sa_handler | PSW_ADDR_AMODE;
> +    env->psw.addr = ka->_sa_handler;
>   
>       env->regs[2] = sig; //map_signal(sig);
>       env->regs[3] = frame_addr += offsetof(typeof(*frame), sc);
> @@ -210,10 +207,9 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
>       /* Set up to return from userspace.  If provided, use a stub
>          already in userspace.  */
>       if (ka->sa_flags & TARGET_SA_RESTORER) {
> -        env->regs[14] = ka->sa_restorer | PSW_ADDR_AMODE;
> +        env->regs[14] = ka->sa_restorer;
>       } else {
> -        env->regs[14] = (frame_addr + offsetof(typeof(*frame), retcode))
> -                        | PSW_ADDR_AMODE;
> +        env->regs[14] = frame_addr + offsetof(typeof(*frame), retcode);
>           __put_user(S390_SYSCALL_OPCODE | TARGET_NR_rt_sigreturn,
>                      &frame->retcode);
>       }
> @@ -223,7 +219,7 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
>   
>       /* Set up registers for signal handler */
>       env->regs[15] = frame_addr;
> -    env->psw.addr = (target_ulong) ka->_sa_handler | PSW_ADDR_AMODE;
> +    env->psw.addr = ka->_sa_handler;
>   
>       env->regs[2] = sig; //map_signal(sig);
>       env->regs[3] = frame_addr + offsetof(typeof(*frame), info);
> @@ -248,7 +244,6 @@ restore_sigregs(CPUS390XState *env, target_sigregs *sc)
>       trace_user_s390x_restore_sigregs(env, (unsigned long long)sc->regs.psw.addr,
>                                        (unsigned long long)env->psw.addr);
>       __get_user(env->psw.addr, &sc->regs.psw.addr);
> -    /* FIXME: 31-bit -> | PSW_ADDR_AMODE */
>   
>       for (i = 0; i < 16; i++) {
>           __get_user(env->aregs[i], &sc->regs.acrs[i]);
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

[PATCH v2 04/15] linux-user/s390x: Remove restore_sigregs return value

[Richard Henderson]

The function cannot fail.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/s390x/signal.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/linux-user/s390x/signal.c b/linux-user/s390x/signal.c
index 1dfca71fa9..e455a9818d 100644
--- a/linux-user/s390x/signal.c
+++ b/linux-user/s390x/signal.c
@@ -230,10 +230,8 @@ give_sigsegv:
     force_sigsegv(sig);
 }
 
-static int
-restore_sigregs(CPUS390XState *env, target_sigregs *sc)
+static void restore_sigregs(CPUS390XState *env, target_sigregs *sc)
 {
-    int err = 0;
     int i;
 
     for (i = 0; i < 16; i++) {
@@ -251,8 +249,6 @@ restore_sigregs(CPUS390XState *env, target_sigregs *sc)
     for (i = 0; i < 16; i++) {
         __get_user(*get_freg(env, i), &sc->fpregs.fprs[i]);
     }
-
-    return err;
 }
 
 long do_sigreturn(CPUS390XState *env)
@@ -271,9 +267,7 @@ long do_sigreturn(CPUS390XState *env)
     target_to_host_sigset_internal(&set, &target_set);
     set_sigmask(&set); /* ~_BLOCKABLE? */
 
-    if (restore_sigregs(env, &frame->sregs)) {
-        goto badframe;
-    }
+    restore_sigregs(env, &frame->sregs);
 
     unlock_user_struct(frame, frame_addr, 0);
     return -TARGET_QEMU_ESIGRETURN;
@@ -297,9 +291,7 @@ long do_rt_sigreturn(CPUS390XState *env)
 
     set_sigmask(&set); /* ~_BLOCKABLE? */
 
-    if (restore_sigregs(env, &frame->uc.tuc_mcontext)) {
-        goto badframe;
-    }
+    restore_sigregs(env, &frame->uc.tuc_mcontext);
 
     target_restore_altstack(&frame->uc.tuc_stack, env);
 
-- 
2.25.1

Re: [PATCH v2 04/15] linux-user/s390x: Remove restore_sigregs return value

[David Hildenbrand]

On 28.04.21 21:33, Richard Henderson wrote:
> The function cannot fail.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   linux-user/s390x/signal.c | 14 +++-----------
>   1 file changed, 3 insertions(+), 11 deletions(-)
> 
> diff --git a/linux-user/s390x/signal.c b/linux-user/s390x/signal.c
> index 1dfca71fa9..e455a9818d 100644
> --- a/linux-user/s390x/signal.c
> +++ b/linux-user/s390x/signal.c
> @@ -230,10 +230,8 @@ give_sigsegv:
>       force_sigsegv(sig);
>   }
>   
> -static int
> -restore_sigregs(CPUS390XState *env, target_sigregs *sc)
> +static void restore_sigregs(CPUS390XState *env, target_sigregs *sc)
>   {
> -    int err = 0;
>       int i;
>   
>       for (i = 0; i < 16; i++) {
> @@ -251,8 +249,6 @@ restore_sigregs(CPUS390XState *env, target_sigregs *sc)
>       for (i = 0; i < 16; i++) {
>           __get_user(*get_freg(env, i), &sc->fpregs.fprs[i]);
>       }
> -
> -    return err;
>   }
>   
>   long do_sigreturn(CPUS390XState *env)
> @@ -271,9 +267,7 @@ long do_sigreturn(CPUS390XState *env)
>       target_to_host_sigset_internal(&set, &target_set);
>       set_sigmask(&set); /* ~_BLOCKABLE? */
>   
> -    if (restore_sigregs(env, &frame->sregs)) {
> -        goto badframe;
> -    }
> +    restore_sigregs(env, &frame->sregs);
>   
>       unlock_user_struct(frame, frame_addr, 0);
>       return -TARGET_QEMU_ESIGRETURN;
> @@ -297,9 +291,7 @@ long do_rt_sigreturn(CPUS390XState *env)
>   
>       set_sigmask(&set); /* ~_BLOCKABLE? */
>   
> -    if (restore_sigregs(env, &frame->uc.tuc_mcontext)) {
> -        goto badframe;
> -    }
> +    restore_sigregs(env, &frame->uc.tuc_mcontext);
>   
>       target_restore_altstack(&frame->uc.tuc_stack, env);
>   
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

[PATCH v2 05/15] linux-user/s390x: Fix trace in restore_regs

[Richard Henderson]

Directly reading sc->regs.psw.addr misses the bswap
that may be performed by __get_user.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/s390x/signal.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/linux-user/s390x/signal.c b/linux-user/s390x/signal.c
index e455a9818d..dcc6f7bc02 100644
--- a/linux-user/s390x/signal.c
+++ b/linux-user/s390x/signal.c
@@ -232,16 +232,17 @@ give_sigsegv:
 
 static void restore_sigregs(CPUS390XState *env, target_sigregs *sc)
 {
+    target_ulong prev_addr;
     int i;
 
     for (i = 0; i < 16; i++) {
         __get_user(env->regs[i], &sc->regs.gprs[i]);
     }
 
+    prev_addr = env->psw.addr;
     __get_user(env->psw.mask, &sc->regs.psw.mask);
-    trace_user_s390x_restore_sigregs(env, (unsigned long long)sc->regs.psw.addr,
-                                     (unsigned long long)env->psw.addr);
     __get_user(env->psw.addr, &sc->regs.psw.addr);
+    trace_user_s390x_restore_sigregs(env, env->psw.addr, prev_addr);
 
     for (i = 0; i < 16; i++) {
         __get_user(env->aregs[i], &sc->regs.acrs[i]);
-- 
2.25.1

Re: [PATCH v2 05/15] linux-user/s390x: Fix trace in restore_regs

[David Hildenbrand]

On 28.04.21 21:33, Richard Henderson wrote:
> Directly reading sc->regs.psw.addr misses the bswap
> that may be performed by __get_user.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   linux-user/s390x/signal.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/linux-user/s390x/signal.c b/linux-user/s390x/signal.c
> index e455a9818d..dcc6f7bc02 100644
> --- a/linux-user/s390x/signal.c
> +++ b/linux-user/s390x/signal.c
> @@ -232,16 +232,17 @@ give_sigsegv:
>   
>   static void restore_sigregs(CPUS390XState *env, target_sigregs *sc)
>   {
> +    target_ulong prev_addr;
>       int i;
>   
>       for (i = 0; i < 16; i++) {
>           __get_user(env->regs[i], &sc->regs.gprs[i]);
>       }
>   
> +    prev_addr = env->psw.addr;
>       __get_user(env->psw.mask, &sc->regs.psw.mask);
> -    trace_user_s390x_restore_sigregs(env, (unsigned long long)sc->regs.psw.addr,
> -                                     (unsigned long long)env->psw.addr);
>       __get_user(env->psw.addr, &sc->regs.psw.addr);
> +    trace_user_s390x_restore_sigregs(env, env->psw.addr, prev_addr);
>   
>       for (i = 0; i < 16; i++) {
>           __get_user(env->aregs[i], &sc->regs.acrs[i]);
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

[PATCH v2 06/15] linux-user/s390x: Fix sigcontext sregs value

[Richard Henderson]

Using the host address of &frame->sregs is incorrect.
We need the guest address.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/s390x/signal.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/linux-user/s390x/signal.c b/linux-user/s390x/signal.c
index dcc6f7bc02..f8515dd332 100644
--- a/linux-user/s390x/signal.c
+++ b/linux-user/s390x/signal.c
@@ -142,7 +142,7 @@ void setup_frame(int sig, struct target_sigaction *ka,
 
     save_sigregs(env, &frame->sregs);
 
-    __put_user((abi_ulong)(unsigned long)&frame->sregs, &frame->sc.sregs);
+    __put_user(frame_addr + offsetof(sigframe, sregs), &frame->sc.sregs);
 
     /* Set up to return from userspace.  If provided, use a stub
        already in userspace.  */
-- 
2.25.1

Re: [PATCH v2 06/15] linux-user/s390x: Fix sigcontext sregs value

[David Hildenbrand]

On 28.04.21 21:33, Richard Henderson wrote:
> Using the host address of &frame->sregs is incorrect.
> We need the guest address.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   linux-user/s390x/signal.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/linux-user/s390x/signal.c b/linux-user/s390x/signal.c
> index dcc6f7bc02..f8515dd332 100644
> --- a/linux-user/s390x/signal.c
> +++ b/linux-user/s390x/signal.c
> @@ -142,7 +142,7 @@ void setup_frame(int sig, struct target_sigaction *ka,
>   
>       save_sigregs(env, &frame->sregs);
>   
> -    __put_user((abi_ulong)(unsigned long)&frame->sregs, &frame->sc.sregs);
> +    __put_user(frame_addr + offsetof(sigframe, sregs), &frame->sc.sregs);
>   
>       /* Set up to return from userspace.  If provided, use a stub
>          already in userspace.  */
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

[PATCH v2 07/15] linux-user/s390x: Use tswap_sigset in setup_rt_frame

[Richard Henderson]

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/s390x/signal.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/linux-user/s390x/signal.c b/linux-user/s390x/signal.c
index f8515dd332..4dde55d4d5 100644
--- a/linux-user/s390x/signal.c
+++ b/linux-user/s390x/signal.c
@@ -182,7 +182,6 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
                     target_siginfo_t *info,
                     target_sigset_t *set, CPUS390XState *env)
 {
-    int i;
     rt_sigframe *frame;
     abi_ulong frame_addr;
 
@@ -199,10 +198,7 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
     __put_user((abi_ulong)0, (abi_ulong *)&frame->uc.tuc_link);
     target_save_altstack(&frame->uc.tuc_stack, env);
     save_sigregs(env, &frame->uc.tuc_mcontext);
-    for (i = 0; i < TARGET_NSIG_WORDS; i++) {
-        __put_user((abi_ulong)set->sig[i],
-                   (abi_ulong *)&frame->uc.tuc_sigmask.sig[i]);
-    }
+    tswap_sigset(&frame->uc.tuc_sigmask, set);
 
     /* Set up to return from userspace.  If provided, use a stub
        already in userspace.  */
-- 
2.25.1

Re: [PATCH v2 07/15] linux-user/s390x: Use tswap_sigset in setup_rt_frame

[David Hildenbrand]

On 28.04.21 21:34, Richard Henderson wrote:
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   linux-user/s390x/signal.c | 6 +-----
>   1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/linux-user/s390x/signal.c b/linux-user/s390x/signal.c
> index f8515dd332..4dde55d4d5 100644
> --- a/linux-user/s390x/signal.c
> +++ b/linux-user/s390x/signal.c
> @@ -182,7 +182,6 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
>                       target_siginfo_t *info,
>                       target_sigset_t *set, CPUS390XState *env)
>   {
> -    int i;
>       rt_sigframe *frame;
>       abi_ulong frame_addr;
>   
> @@ -199,10 +198,7 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
>       __put_user((abi_ulong)0, (abi_ulong *)&frame->uc.tuc_link);
>       target_save_altstack(&frame->uc.tuc_stack, env);
>       save_sigregs(env, &frame->uc.tuc_mcontext);
> -    for (i = 0; i < TARGET_NSIG_WORDS; i++) {
> -        __put_user((abi_ulong)set->sig[i],
> -                   (abi_ulong *)&frame->uc.tuc_sigmask.sig[i]);
> -    }
> +    tswap_sigset(&frame->uc.tuc_sigmask, set);
>   
>       /* Set up to return from userspace.  If provided, use a stub
>          already in userspace.  */
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

[PATCH v2 08/15] linux-user/s390x: Tidy save_sigregs

[Richard Henderson]

The "save" routines copied from the kernel, which are currently
commented out, are unnecessary in qemu.  We can copy from env
where the kernel needs special instructions.  Fix comment style.

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

diff --git a/linux-user/s390x/signal.c b/linux-user/s390x/signal.c
index 4dde55d4d5..eabfe4293f 100644
--- a/linux-user/s390x/signal.c
+++ b/linux-user/s390x/signal.c
@@ -104,23 +104,25 @@ get_sigframe(struct target_sigaction *ka, CPUS390XState *env, size_t frame_size)
 static void save_sigregs(CPUS390XState *env, target_sigregs *sregs)
 {
     int i;
-    //save_access_regs(current->thread.acrs); FIXME
 
-    /* Copy a 'clean' PSW mask to the user to avoid leaking
-       information about whether PER is currently on.  */
+    /*
+     * Copy a 'clean' PSW mask to the user to avoid leaking
+     * information about whether PER is currently on.
+     */
     __put_user(env->psw.mask, &sregs->regs.psw.mask);
     __put_user(env->psw.addr, &sregs->regs.psw.addr);
+
     for (i = 0; i < 16; i++) {
         __put_user(env->regs[i], &sregs->regs.gprs[i]);
     }
     for (i = 0; i < 16; i++) {
         __put_user(env->aregs[i], &sregs->regs.acrs[i]);
     }
+
     /*
      * We have to store the fp registers to current->thread.fp_regs
      * to merge them with the emulated registers.
      */
-    //save_fp_regs(&current->thread.fp_regs); FIXME
     for (i = 0; i < 16; i++) {
         __put_user(*get_freg(env, i), &sregs->fpregs.fprs[i]);
     }
-- 
2.25.1

Re: [PATCH v2 08/15] linux-user/s390x: Tidy save_sigregs

[David Hildenbrand]

On 28.04.21 21:34, Richard Henderson wrote:
> The "save" routines copied from the kernel, which are currently
> commented out, are unnecessary in qemu.  We can copy from env
> where the kernel needs special instructions.  Fix comment style.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   linux-user/s390x/signal.c | 10 ++++++----
>   1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/linux-user/s390x/signal.c b/linux-user/s390x/signal.c
> index 4dde55d4d5..eabfe4293f 100644
> --- a/linux-user/s390x/signal.c
> +++ b/linux-user/s390x/signal.c
> @@ -104,23 +104,25 @@ get_sigframe(struct target_sigaction *ka, CPUS390XState *env, size_t frame_size)
>   static void save_sigregs(CPUS390XState *env, target_sigregs *sregs)
>   {
>       int i;
> -    //save_access_regs(current->thread.acrs); FIXME
>   
> -    /* Copy a 'clean' PSW mask to the user to avoid leaking
> -       information about whether PER is currently on.  */
> +    /*
> +     * Copy a 'clean' PSW mask to the user to avoid leaking
> +     * information about whether PER is currently on.
> +     */
>       __put_user(env->psw.mask, &sregs->regs.psw.mask);
>       __put_user(env->psw.addr, &sregs->regs.psw.addr);
> +
>       for (i = 0; i < 16; i++) {
>           __put_user(env->regs[i], &sregs->regs.gprs[i]);
>       }
>       for (i = 0; i < 16; i++) {
>           __put_user(env->aregs[i], &sregs->regs.acrs[i]);
>       }
> +
>       /*
>        * We have to store the fp registers to current->thread.fp_regs
>        * to merge them with the emulated registers.
>        */
> -    //save_fp_regs(&current->thread.fp_regs); FIXME
>       for (i = 0; i < 16; i++) {
>           __put_user(*get_freg(env, i), &sregs->fpregs.fprs[i]);
>       }
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

[PATCH v2 09/15] linux-user/s390x: Clean up single-use gotos in signal.c

[Richard Henderson]

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

diff --git a/linux-user/s390x/signal.c b/linux-user/s390x/signal.c
index eabfe4293f..64a9eab097 100644
--- a/linux-user/s390x/signal.c
+++ b/linux-user/s390x/signal.c
@@ -137,7 +137,8 @@ void setup_frame(int sig, struct target_sigaction *ka,
     frame_addr = get_sigframe(ka, env, sizeof(*frame));
     trace_user_setup_frame(env, frame_addr);
     if (!lock_user_struct(VERIFY_WRITE, frame, frame_addr, 0)) {
-        goto give_sigsegv;
+        force_sigsegv(sig);
+        return;
     }
 
     __put_user(set->sig[0], &frame->sc.oldmask[0]);
@@ -174,10 +175,6 @@ void setup_frame(int sig, struct target_sigaction *ka,
     /* Place signal number on stack to allow backtrace from handler.  */
     __put_user(env->regs[2], &frame->signo);
     unlock_user_struct(frame, frame_addr, 1);
-    return;
-
-give_sigsegv:
-    force_sigsegv(sig);
 }
 
 void setup_rt_frame(int sig, struct target_sigaction *ka,
@@ -190,7 +187,8 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
     frame_addr = get_sigframe(ka, env, sizeof *frame);
     trace_user_setup_rt_frame(env, frame_addr);
     if (!lock_user_struct(VERIFY_WRITE, frame, frame_addr, 0)) {
-        goto give_sigsegv;
+        force_sigsegv(sig);
+        return;
     }
 
     tswap_siginfo(&frame->info, info);
@@ -222,10 +220,6 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
     env->regs[2] = sig; //map_signal(sig);
     env->regs[3] = frame_addr + offsetof(typeof(*frame), info);
     env->regs[4] = frame_addr + offsetof(typeof(*frame), uc);
-    return;
-
-give_sigsegv:
-    force_sigsegv(sig);
 }
 
 static void restore_sigregs(CPUS390XState *env, target_sigregs *sc)
@@ -259,7 +253,8 @@ long do_sigreturn(CPUS390XState *env)
 
     trace_user_do_sigreturn(env, frame_addr);
     if (!lock_user_struct(VERIFY_READ, frame, frame_addr, 1)) {
-        goto badframe;
+        force_sig(TARGET_SIGSEGV);
+        return -TARGET_QEMU_ESIGRETURN;
     }
     __get_user(target_set.sig[0], &frame->sc.oldmask[0]);
 
@@ -270,10 +265,6 @@ long do_sigreturn(CPUS390XState *env)
 
     unlock_user_struct(frame, frame_addr, 0);
     return -TARGET_QEMU_ESIGRETURN;
-
-badframe:
-    force_sig(TARGET_SIGSEGV);
-    return -TARGET_QEMU_ESIGRETURN;
 }
 
 long do_rt_sigreturn(CPUS390XState *env)
@@ -284,7 +275,8 @@ long do_rt_sigreturn(CPUS390XState *env)
 
     trace_user_do_rt_sigreturn(env, frame_addr);
     if (!lock_user_struct(VERIFY_READ, frame, frame_addr, 1)) {
-        goto badframe;
+        force_sig(TARGET_SIGSEGV);
+        return -TARGET_QEMU_ESIGRETURN;
     }
     target_to_host_sigset(&set, &frame->uc.tuc_sigmask);
 
@@ -296,9 +288,4 @@ long do_rt_sigreturn(CPUS390XState *env)
 
     unlock_user_struct(frame, frame_addr, 0);
     return -TARGET_QEMU_ESIGRETURN;
-
-badframe:
-    unlock_user_struct(frame, frame_addr, 0);
-    force_sig(TARGET_SIGSEGV);
-    return -TARGET_QEMU_ESIGRETURN;
 }
-- 
2.25.1

Re: [PATCH v2 09/15] linux-user/s390x: Clean up single-use gotos in signal.c

[David Hildenbrand]

On 28.04.21 21:34, Richard Henderson wrote:
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   linux-user/s390x/signal.c | 29 ++++++++---------------------
>   1 file changed, 8 insertions(+), 21 deletions(-)
> 
> diff --git a/linux-user/s390x/signal.c b/linux-user/s390x/signal.c
> index eabfe4293f..64a9eab097 100644
> --- a/linux-user/s390x/signal.c
> +++ b/linux-user/s390x/signal.c
> @@ -137,7 +137,8 @@ void setup_frame(int sig, struct target_sigaction *ka,
>       frame_addr = get_sigframe(ka, env, sizeof(*frame));
>       trace_user_setup_frame(env, frame_addr);
>       if (!lock_user_struct(VERIFY_WRITE, frame, frame_addr, 0)) {
> -        goto give_sigsegv;
> +        force_sigsegv(sig);
> +        return;
>       }
>   
>       __put_user(set->sig[0], &frame->sc.oldmask[0]);
> @@ -174,10 +175,6 @@ void setup_frame(int sig, struct target_sigaction *ka,
>       /* Place signal number on stack to allow backtrace from handler.  */
>       __put_user(env->regs[2], &frame->signo);
>       unlock_user_struct(frame, frame_addr, 1);
> -    return;
> -
> -give_sigsegv:
> -    force_sigsegv(sig);
>   }
>   
>   void setup_rt_frame(int sig, struct target_sigaction *ka,
> @@ -190,7 +187,8 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
>       frame_addr = get_sigframe(ka, env, sizeof *frame);
>       trace_user_setup_rt_frame(env, frame_addr);
>       if (!lock_user_struct(VERIFY_WRITE, frame, frame_addr, 0)) {
> -        goto give_sigsegv;
> +        force_sigsegv(sig);
> +        return;
>       }
>   
>       tswap_siginfo(&frame->info, info);
> @@ -222,10 +220,6 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
>       env->regs[2] = sig; //map_signal(sig);
>       env->regs[3] = frame_addr + offsetof(typeof(*frame), info);
>       env->regs[4] = frame_addr + offsetof(typeof(*frame), uc);
> -    return;
> -
> -give_sigsegv:
> -    force_sigsegv(sig);
>   }
>   
>   static void restore_sigregs(CPUS390XState *env, target_sigregs *sc)
> @@ -259,7 +253,8 @@ long do_sigreturn(CPUS390XState *env)
>   
>       trace_user_do_sigreturn(env, frame_addr);
>       if (!lock_user_struct(VERIFY_READ, frame, frame_addr, 1)) {
> -        goto badframe;
> +        force_sig(TARGET_SIGSEGV);
> +        return -TARGET_QEMU_ESIGRETURN;
>       }
>       __get_user(target_set.sig[0], &frame->sc.oldmask[0]);
>   
> @@ -270,10 +265,6 @@ long do_sigreturn(CPUS390XState *env)
>   
>       unlock_user_struct(frame, frame_addr, 0);
>       return -TARGET_QEMU_ESIGRETURN;
> -
> -badframe:
> -    force_sig(TARGET_SIGSEGV);
> -    return -TARGET_QEMU_ESIGRETURN;
>   }
>   
>   long do_rt_sigreturn(CPUS390XState *env)
> @@ -284,7 +275,8 @@ long do_rt_sigreturn(CPUS390XState *env)
>   
>       trace_user_do_rt_sigreturn(env, frame_addr);
>       if (!lock_user_struct(VERIFY_READ, frame, frame_addr, 1)) {
> -        goto badframe;
> +        force_sig(TARGET_SIGSEGV);
> +        return -TARGET_QEMU_ESIGRETURN;
>       }
>       target_to_host_sigset(&set, &frame->uc.tuc_sigmask);
>   
> @@ -296,9 +288,4 @@ long do_rt_sigreturn(CPUS390XState *env)
>   
>       unlock_user_struct(frame, frame_addr, 0);
>       return -TARGET_QEMU_ESIGRETURN;
> -
> -badframe:
> -    unlock_user_struct(frame, frame_addr, 0);
> -    force_sig(TARGET_SIGSEGV);
> -    return -TARGET_QEMU_ESIGRETURN;
>   }
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

[PATCH v2 10/15] linux-user/s390x: Set psw.mask properly for the signal handler

[Richard Henderson]

Note that PSW_ADDR_{64,32} are called PSW_MASK_{EA,BA}
in the kernel source.

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

diff --git a/linux-user/s390x/signal.c b/linux-user/s390x/signal.c
index 64a9eab097..17f617c655 100644
--- a/linux-user/s390x/signal.c
+++ b/linux-user/s390x/signal.c
@@ -162,6 +162,9 @@ void setup_frame(int sig, struct target_sigaction *ka,
 
     /* Set up registers for signal handler */
     env->regs[15] = frame_addr;
+    /* Force default amode and default user address space control. */
+    env->psw.mask = PSW_MASK_64 | PSW_MASK_32 | PSW_ASC_PRIMARY
+                  | (env->psw.mask & ~PSW_MASK_ASC);
     env->psw.addr = ka->_sa_handler;
 
     env->regs[2] = sig; //map_signal(sig);
@@ -215,6 +218,9 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
 
     /* Set up registers for signal handler */
     env->regs[15] = frame_addr;
+    /* Force default amode and default user address space control. */
+    env->psw.mask = PSW_MASK_64 | PSW_MASK_32 | PSW_ASC_PRIMARY
+                  | (env->psw.mask & ~PSW_MASK_ASC);
     env->psw.addr = ka->_sa_handler;
 
     env->regs[2] = sig; //map_signal(sig);
-- 
2.25.1

Re: [PATCH v2 10/15] linux-user/s390x: Set psw.mask properly for the signal handler

[David Hildenbrand]

On 28.04.21 21:34, Richard Henderson wrote:
> Note that PSW_ADDR_{64,32} are called PSW_MASK_{EA,BA}
> in the kernel source.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   linux-user/s390x/signal.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/linux-user/s390x/signal.c b/linux-user/s390x/signal.c
> index 64a9eab097..17f617c655 100644
> --- a/linux-user/s390x/signal.c
> +++ b/linux-user/s390x/signal.c
> @@ -162,6 +162,9 @@ void setup_frame(int sig, struct target_sigaction *ka,
>   
>       /* Set up registers for signal handler */
>       env->regs[15] = frame_addr;
> +    /* Force default amode and default user address space control. */
> +    env->psw.mask = PSW_MASK_64 | PSW_MASK_32 | PSW_ASC_PRIMARY
> +                  | (env->psw.mask & ~PSW_MASK_ASC);
>       env->psw.addr = ka->_sa_handler;
>   
>       env->regs[2] = sig; //map_signal(sig);
> @@ -215,6 +218,9 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
>   
>       /* Set up registers for signal handler */
>       env->regs[15] = frame_addr;
> +    /* Force default amode and default user address space control. */
> +    env->psw.mask = PSW_MASK_64 | PSW_MASK_32 | PSW_ASC_PRIMARY
> +                  | (env->psw.mask & ~PSW_MASK_ASC);
>       env->psw.addr = ka->_sa_handler;
>   
>       env->regs[2] = sig; //map_signal(sig);
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

[PATCH v2 11/15] linux-user/s390x: Add stub sigframe argument for last_break

[Richard Henderson]

In order to properly present these arguments, we need to add
code to target/s390x to record LowCore parameters for user-only.

But in the meantime, at least zero the missing last_break
argument, and fixup the comment style in the vicinity.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/s390x/signal.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/linux-user/s390x/signal.c b/linux-user/s390x/signal.c
index 17f617c655..bc41b01c5d 100644
--- a/linux-user/s390x/signal.c
+++ b/linux-user/s390x/signal.c
@@ -167,13 +167,16 @@ void setup_frame(int sig, struct target_sigaction *ka,
                   | (env->psw.mask & ~PSW_MASK_ASC);
     env->psw.addr = ka->_sa_handler;
 
-    env->regs[2] = sig; //map_signal(sig);
+    env->regs[2] = sig;
     env->regs[3] = frame_addr += offsetof(typeof(*frame), sc);
 
-    /* We forgot to include these in the sigcontext.
-       To avoid breaking binary compatibility, they are passed as args. */
-    env->regs[4] = 0; // FIXME: no clue... current->thread.trap_no;
-    env->regs[5] = 0; // FIXME: no clue... current->thread.prot_addr;
+    /*
+     * We forgot to include these in the sigcontext.
+     * To avoid breaking binary compatibility, they are passed as args.
+     */
+    env->regs[4] = 0; /* FIXME: regs->int_code & 127 */
+    env->regs[5] = 0; /* FIXME: regs->int_parm_long */
+    env->regs[6] = 0; /* FIXME: current->thread.last_break */
 
     /* Place signal number on stack to allow backtrace from handler.  */
     __put_user(env->regs[2], &frame->signo);
@@ -223,9 +226,10 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
                   | (env->psw.mask & ~PSW_MASK_ASC);
     env->psw.addr = ka->_sa_handler;
 
-    env->regs[2] = sig; //map_signal(sig);
+    env->regs[2] = sig;
     env->regs[3] = frame_addr + offsetof(typeof(*frame), info);
     env->regs[4] = frame_addr + offsetof(typeof(*frame), uc);
+    env->regs[5] = 0; /* FIXME: current->thread.last_break */
 }
 
 static void restore_sigregs(CPUS390XState *env, target_sigregs *sc)
-- 
2.25.1

Re: [PATCH v2 11/15] linux-user/s390x: Add stub sigframe argument for last_break

[David Hildenbrand]

On 28.04.21 21:34, Richard Henderson wrote:
> In order to properly present these arguments, we need to add
> code to target/s390x to record LowCore parameters for user-only.
> 
> But in the meantime, at least zero the missing last_break
> argument, and fixup the comment style in the vicinity.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   linux-user/s390x/signal.c | 16 ++++++++++------
>   1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/linux-user/s390x/signal.c b/linux-user/s390x/signal.c
> index 17f617c655..bc41b01c5d 100644
> --- a/linux-user/s390x/signal.c
> +++ b/linux-user/s390x/signal.c
> @@ -167,13 +167,16 @@ void setup_frame(int sig, struct target_sigaction *ka,
>                     | (env->psw.mask & ~PSW_MASK_ASC);
>       env->psw.addr = ka->_sa_handler;
>   
> -    env->regs[2] = sig; //map_signal(sig);
> +    env->regs[2] = sig;
>       env->regs[3] = frame_addr += offsetof(typeof(*frame), sc);
>   
> -    /* We forgot to include these in the sigcontext.
> -       To avoid breaking binary compatibility, they are passed as args. */
> -    env->regs[4] = 0; // FIXME: no clue... current->thread.trap_no;
> -    env->regs[5] = 0; // FIXME: no clue... current->thread.prot_addr;
> +    /*
> +     * We forgot to include these in the sigcontext.
> +     * To avoid breaking binary compatibility, they are passed as args.
> +     */
> +    env->regs[4] = 0; /* FIXME: regs->int_code & 127 */
> +    env->regs[5] = 0; /* FIXME: regs->int_parm_long */
> +    env->regs[6] = 0; /* FIXME: current->thread.last_break */
>   
>       /* Place signal number on stack to allow backtrace from handler.  */
>       __put_user(env->regs[2], &frame->signo);
> @@ -223,9 +226,10 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
>                     | (env->psw.mask & ~PSW_MASK_ASC);
>       env->psw.addr = ka->_sa_handler;
>   
> -    env->regs[2] = sig; //map_signal(sig);
> +    env->regs[2] = sig;
>       env->regs[3] = frame_addr + offsetof(typeof(*frame), info);
>       env->regs[4] = frame_addr + offsetof(typeof(*frame), uc);
> +    env->regs[5] = 0; /* FIXME: current->thread.last_break */
>   }
>   
>   static void restore_sigregs(CPUS390XState *env, target_sigregs *sc)
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

[PATCH v2 12/15] linux-user/s390x: Fix frame_addr corruption in setup_frame

[Richard Henderson]

The original value of frame_addr is still required for
its use in the call to unlock_user_struct below.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/s390x/signal.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/linux-user/s390x/signal.c b/linux-user/s390x/signal.c
index bc41b01c5d..81ba59b46a 100644
--- a/linux-user/s390x/signal.c
+++ b/linux-user/s390x/signal.c
@@ -168,7 +168,7 @@ void setup_frame(int sig, struct target_sigaction *ka,
     env->psw.addr = ka->_sa_handler;
 
     env->regs[2] = sig;
-    env->regs[3] = frame_addr += offsetof(typeof(*frame), sc);
+    env->regs[3] = frame_addr + offsetof(typeof(*frame), sc);
 
     /*
      * We forgot to include these in the sigcontext.
-- 
2.25.1

Re: [PATCH v2 12/15] linux-user/s390x: Fix frame_addr corruption in setup_frame

[David Hildenbrand]

On 28.04.21 21:34, Richard Henderson wrote:
> The original value of frame_addr is still required for
> its use in the call to unlock_user_struct below.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   linux-user/s390x/signal.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/linux-user/s390x/signal.c b/linux-user/s390x/signal.c
> index bc41b01c5d..81ba59b46a 100644
> --- a/linux-user/s390x/signal.c
> +++ b/linux-user/s390x/signal.c
> @@ -168,7 +168,7 @@ void setup_frame(int sig, struct target_sigaction *ka,
>       env->psw.addr = ka->_sa_handler;
>   
>       env->regs[2] = sig;
> -    env->regs[3] = frame_addr += offsetof(typeof(*frame), sc);
> +    env->regs[3] = frame_addr + offsetof(typeof(*frame), sc);
>   
>       /*
>        * We forgot to include these in the sigcontext.
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

[PATCH v2 13/15] linux-user/s390x: Add build asserts for sigset sizes

[Richard Henderson]

At point of usage, it's not immediately obvious that
we don't need a loop to copy these arrays.

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

diff --git a/linux-user/s390x/signal.c b/linux-user/s390x/signal.c
index 81ba59b46a..839a7ae4b3 100644
--- a/linux-user/s390x/signal.c
+++ b/linux-user/s390x/signal.c
@@ -141,6 +141,8 @@ void setup_frame(int sig, struct target_sigaction *ka,
         return;
     }
 
+    /* Make sure that we're initializing all of oldmask. */
+    QEMU_BUILD_BUG_ON(ARRAY_SIZE(frame->sc.oldmask) != 1);
     __put_user(set->sig[0], &frame->sc.oldmask[0]);
 
     save_sigregs(env, &frame->sregs);
@@ -266,6 +268,9 @@ long do_sigreturn(CPUS390XState *env)
         force_sig(TARGET_SIGSEGV);
         return -TARGET_QEMU_ESIGRETURN;
     }
+
+    /* Make sure that we're initializing all of target_set. */
+    QEMU_BUILD_BUG_ON(ARRAY_SIZE(target_set.sig) != 1);
     __get_user(target_set.sig[0], &frame->sc.oldmask[0]);
 
     target_to_host_sigset_internal(&set, &target_set);
-- 
2.25.1

Re: [PATCH v2 13/15] linux-user/s390x: Add build asserts for sigset sizes

[David Hildenbrand]

On 28.04.21 21:34, Richard Henderson wrote:
> At point of usage, it's not immediately obvious that
> we don't need a loop to copy these arrays.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   linux-user/s390x/signal.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/linux-user/s390x/signal.c b/linux-user/s390x/signal.c
> index 81ba59b46a..839a7ae4b3 100644
> --- a/linux-user/s390x/signal.c
> +++ b/linux-user/s390x/signal.c
> @@ -141,6 +141,8 @@ void setup_frame(int sig, struct target_sigaction *ka,
>           return;
>       }
>   
> +    /* Make sure that we're initializing all of oldmask. */
> +    QEMU_BUILD_BUG_ON(ARRAY_SIZE(frame->sc.oldmask) != 1);
>       __put_user(set->sig[0], &frame->sc.oldmask[0]);
>   
>       save_sigregs(env, &frame->sregs);
> @@ -266,6 +268,9 @@ long do_sigreturn(CPUS390XState *env)
>           force_sig(TARGET_SIGSEGV);
>           return -TARGET_QEMU_ESIGRETURN;
>       }
> +
> +    /* Make sure that we're initializing all of target_set. */
> +    QEMU_BUILD_BUG_ON(ARRAY_SIZE(target_set.sig) != 1);
>       __get_user(target_set.sig[0], &frame->sc.oldmask[0]);
>   
>       target_to_host_sigset_internal(&set, &target_set);
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

[PATCH v2 14/15] linux-user/s390x: Clean up signal.c

[Richard Henderson]

Reorder the function bodies to correspond to the kernel source.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/s390x/signal.c | 67 ++++++++++++++++++++++++---------------
 1 file changed, 41 insertions(+), 26 deletions(-)

diff --git a/linux-user/s390x/signal.c b/linux-user/s390x/signal.c
index 839a7ae4b3..9d470e4ca0 100644
--- a/linux-user/s390x/signal.c
+++ b/linux-user/s390x/signal.c
@@ -133,6 +133,7 @@ void setup_frame(int sig, struct target_sigaction *ka,
 {
     sigframe *frame;
     abi_ulong frame_addr;
+    abi_ulong restorer;
 
     frame_addr = get_sigframe(ka, env, sizeof(*frame));
     trace_user_setup_frame(env, frame_addr);
@@ -141,28 +142,39 @@ void setup_frame(int sig, struct target_sigaction *ka,
         return;
     }
 
+    /* Set up backchain. */
+    __put_user(env->regs[15], (abi_ulong *) frame);
+
+    /* Create struct sigcontext on the signal stack. */
     /* Make sure that we're initializing all of oldmask. */
     QEMU_BUILD_BUG_ON(ARRAY_SIZE(frame->sc.oldmask) != 1);
     __put_user(set->sig[0], &frame->sc.oldmask[0]);
-
-    save_sigregs(env, &frame->sregs);
-
     __put_user(frame_addr + offsetof(sigframe, sregs), &frame->sc.sregs);
 
-    /* Set up to return from userspace.  If provided, use a stub
-       already in userspace.  */
+    /* Create _sigregs on the signal stack */
+    save_sigregs(env, &frame->sregs);
+
+    /*
+     * ??? The kernel uses regs->gprs[2] here, which is not yet the signo.
+     * Moreover the comment talks about allowing backtrace, which is really
+     * done by the r15 copy above.
+     */
+    __put_user(sig, &frame->signo);
+
+    /*
+     * Set up to return from userspace.
+     * If provided, use a stub already in userspace.
+     */
     if (ka->sa_flags & TARGET_SA_RESTORER) {
-        env->regs[14] = ka->sa_restorer;
+        restorer = ka->sa_restorer;
     } else {
-        env->regs[14] = frame_addr + offsetof(sigframe, retcode);
+        restorer = frame_addr + offsetof(sigframe, retcode);
         __put_user(S390_SYSCALL_OPCODE | TARGET_NR_sigreturn,
                    &frame->retcode);
     }
 
-    /* Set up backchain. */
-    __put_user(env->regs[15], (abi_ulong *) frame);
-
     /* Set up registers for signal handler */
+    env->regs[14] = restorer;
     env->regs[15] = frame_addr;
     /* Force default amode and default user address space control. */
     env->psw.mask = PSW_MASK_64 | PSW_MASK_32 | PSW_ASC_PRIMARY
@@ -180,8 +192,6 @@ void setup_frame(int sig, struct target_sigaction *ka,
     env->regs[5] = 0; /* FIXME: regs->int_parm_long */
     env->regs[6] = 0; /* FIXME: current->thread.last_break */
 
-    /* Place signal number on stack to allow backtrace from handler.  */
-    __put_user(env->regs[2], &frame->signo);
     unlock_user_struct(frame, frame_addr, 1);
 }
 
@@ -191,6 +201,7 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
 {
     rt_sigframe *frame;
     abi_ulong frame_addr;
+    abi_ulong restorer;
 
     frame_addr = get_sigframe(ka, env, sizeof *frame);
     trace_user_setup_rt_frame(env, frame_addr);
@@ -199,29 +210,33 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
         return;
     }
 
-    tswap_siginfo(&frame->info, info);
+    /* Set up backchain. */
+    __put_user(env->regs[15], (abi_ulong *) frame);
 
-    /* Create the ucontext.  */
-    __put_user(0, &frame->uc.tuc_flags);
-    __put_user((abi_ulong)0, (abi_ulong *)&frame->uc.tuc_link);
-    target_save_altstack(&frame->uc.tuc_stack, env);
-    save_sigregs(env, &frame->uc.tuc_mcontext);
-    tswap_sigset(&frame->uc.tuc_sigmask, set);
-
-    /* Set up to return from userspace.  If provided, use a stub
-       already in userspace.  */
+    /*
+     * Set up to return from userspace.
+     * If provided, use a stub already in userspace.
+     */
     if (ka->sa_flags & TARGET_SA_RESTORER) {
-        env->regs[14] = ka->sa_restorer;
+        restorer = ka->sa_restorer;
     } else {
-        env->regs[14] = frame_addr + offsetof(typeof(*frame), retcode);
+        restorer = frame_addr + offsetof(typeof(*frame), retcode);
         __put_user(S390_SYSCALL_OPCODE | TARGET_NR_rt_sigreturn,
                    &frame->retcode);
     }
 
-    /* Set up backchain. */
-    __put_user(env->regs[15], (abi_ulong *) frame);
+    /* Create siginfo on the signal stack. */
+    tswap_siginfo(&frame->info, info);
+
+    /* Create ucontext on the signal stack. */
+    __put_user(0, &frame->uc.tuc_flags);
+    __put_user(0, &frame->uc.tuc_link);
+    target_save_altstack(&frame->uc.tuc_stack, env);
+    save_sigregs(env, &frame->uc.tuc_mcontext);
+    tswap_sigset(&frame->uc.tuc_sigmask, set);
 
     /* Set up registers for signal handler */
+    env->regs[14] = restorer;
     env->regs[15] = frame_addr;
     /* Force default amode and default user address space control. */
     env->psw.mask = PSW_MASK_64 | PSW_MASK_32 | PSW_ASC_PRIMARY
-- 
2.25.1

Re: [PATCH v2 14/15] linux-user/s390x: Clean up signal.c

[David Hildenbrand]

On 28.04.21 21:34, Richard Henderson wrote:
> Reorder the function bodies to correspond to the kernel source.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   linux-user/s390x/signal.c | 67 ++++++++++++++++++++++++---------------
>   1 file changed, 41 insertions(+), 26 deletions(-)
> 
> diff --git a/linux-user/s390x/signal.c b/linux-user/s390x/signal.c
> index 839a7ae4b3..9d470e4ca0 100644
> --- a/linux-user/s390x/signal.c
> +++ b/linux-user/s390x/signal.c
> @@ -133,6 +133,7 @@ void setup_frame(int sig, struct target_sigaction *ka,
>   {
>       sigframe *frame;
>       abi_ulong frame_addr;
> +    abi_ulong restorer;
>   
>       frame_addr = get_sigframe(ka, env, sizeof(*frame));
>       trace_user_setup_frame(env, frame_addr);
> @@ -141,28 +142,39 @@ void setup_frame(int sig, struct target_sigaction *ka,
>           return;
>       }
>   
> +    /* Set up backchain. */
> +    __put_user(env->regs[15], (abi_ulong *) frame);
> +
> +    /* Create struct sigcontext on the signal stack. */
>       /* Make sure that we're initializing all of oldmask. */
>       QEMU_BUILD_BUG_ON(ARRAY_SIZE(frame->sc.oldmask) != 1);
>       __put_user(set->sig[0], &frame->sc.oldmask[0]);
> -
> -    save_sigregs(env, &frame->sregs);
> -
>       __put_user(frame_addr + offsetof(sigframe, sregs), &frame->sc.sregs);
>   
> -    /* Set up to return from userspace.  If provided, use a stub
> -       already in userspace.  */
> +    /* Create _sigregs on the signal stack */
> +    save_sigregs(env, &frame->sregs);
> +
> +    /*
> +     * ??? The kernel uses regs->gprs[2] here, which is not yet the signo.
> +     * Moreover the comment talks about allowing backtrace, which is really
> +     * done by the r15 copy above.
> +     */
> +    __put_user(sig, &frame->signo);
> +
> +    /*
> +     * Set up to return from userspace.
> +     * If provided, use a stub already in userspace.
> +     */
>       if (ka->sa_flags & TARGET_SA_RESTORER) {
> -        env->regs[14] = ka->sa_restorer;
> +        restorer = ka->sa_restorer;
>       } else {
> -        env->regs[14] = frame_addr + offsetof(sigframe, retcode);
> +        restorer = frame_addr + offsetof(sigframe, retcode);
>           __put_user(S390_SYSCALL_OPCODE | TARGET_NR_sigreturn,
>                      &frame->retcode);
>       }
>   
> -    /* Set up backchain. */
> -    __put_user(env->regs[15], (abi_ulong *) frame);
> -
>       /* Set up registers for signal handler */
> +    env->regs[14] = restorer;
>       env->regs[15] = frame_addr;
>       /* Force default amode and default user address space control. */
>       env->psw.mask = PSW_MASK_64 | PSW_MASK_32 | PSW_ASC_PRIMARY
> @@ -180,8 +192,6 @@ void setup_frame(int sig, struct target_sigaction *ka,
>       env->regs[5] = 0; /* FIXME: regs->int_parm_long */
>       env->regs[6] = 0; /* FIXME: current->thread.last_break */
>   
> -    /* Place signal number on stack to allow backtrace from handler.  */
> -    __put_user(env->regs[2], &frame->signo);
>       unlock_user_struct(frame, frame_addr, 1);
>   }
>   
> @@ -191,6 +201,7 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
>   {
>       rt_sigframe *frame;
>       abi_ulong frame_addr;
> +    abi_ulong restorer;
>   
>       frame_addr = get_sigframe(ka, env, sizeof *frame);
>       trace_user_setup_rt_frame(env, frame_addr);
> @@ -199,29 +210,33 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
>           return;
>       }
>   
> -    tswap_siginfo(&frame->info, info);
> +    /* Set up backchain. */
> +    __put_user(env->regs[15], (abi_ulong *) frame);
>   
> -    /* Create the ucontext.  */
> -    __put_user(0, &frame->uc.tuc_flags);
> -    __put_user((abi_ulong)0, (abi_ulong *)&frame->uc.tuc_link);
> -    target_save_altstack(&frame->uc.tuc_stack, env);
> -    save_sigregs(env, &frame->uc.tuc_mcontext);
> -    tswap_sigset(&frame->uc.tuc_sigmask, set);
> -
> -    /* Set up to return from userspace.  If provided, use a stub
> -       already in userspace.  */
> +    /*
> +     * Set up to return from userspace.
> +     * If provided, use a stub already in userspace.
> +     */
>       if (ka->sa_flags & TARGET_SA_RESTORER) {
> -        env->regs[14] = ka->sa_restorer;
> +        restorer = ka->sa_restorer;
>       } else {
> -        env->regs[14] = frame_addr + offsetof(typeof(*frame), retcode);
> +        restorer = frame_addr + offsetof(typeof(*frame), retcode);
>           __put_user(S390_SYSCALL_OPCODE | TARGET_NR_rt_sigreturn,
>                      &frame->retcode);
>       }
>   
> -    /* Set up backchain. */
> -    __put_user(env->regs[15], (abi_ulong *) frame);
> +    /* Create siginfo on the signal stack. */
> +    tswap_siginfo(&frame->info, info);
> +
> +    /* Create ucontext on the signal stack. */
> +    __put_user(0, &frame->uc.tuc_flags);
> +    __put_user(0, &frame->uc.tuc_link);
> +    target_save_altstack(&frame->uc.tuc_stack, env);
> +    save_sigregs(env, &frame->uc.tuc_mcontext);
> +    tswap_sigset(&frame->uc.tuc_sigmask, set);
>   
>       /* Set up registers for signal handler */
> +    env->regs[14] = restorer;
>       env->regs[15] = frame_addr;
>       /* Force default amode and default user address space control. */
>       env->psw.mask = PSW_MASK_64 | PSW_MASK_32 | PSW_ASC_PRIMARY
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

[PATCH v2 15/15] linux-user/s390x: Handle vector regs in signal stack

[Richard Henderson]

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/s390x/signal.c | 62 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 60 insertions(+), 2 deletions(-)

diff --git a/linux-user/s390x/signal.c b/linux-user/s390x/signal.c
index 9d470e4ca0..b537646e60 100644
--- a/linux-user/s390x/signal.c
+++ b/linux-user/s390x/signal.c
@@ -50,6 +50,12 @@ typedef struct {
     target_s390_fp_regs     fpregs;
 } target_sigregs;
 
+typedef struct {
+    uint64_t vxrs_low[16];
+    uint64_t vxrs_high[16][2];
+    uint8_t reserved[128];
+} target_sigregs_ext;
+
 typedef struct {
     abi_ulong oldmask[_SIGCONTEXT_NSIG_WORDS];
     abi_ulong sregs;
@@ -60,15 +66,20 @@ typedef struct {
     target_sigcontext sc;
     target_sigregs sregs;
     int signo;
+    target_sigregs_ext sregs_ext;
     uint16_t retcode;
 } sigframe;
 
+#define TARGET_UC_VXRS 2
+
 struct target_ucontext {
     abi_ulong tuc_flags;
     abi_ulong tuc_link;
     target_stack_t tuc_stack;
     target_sigregs tuc_mcontext;
-    target_sigset_t tuc_sigmask;   /* mask last for extensibility */
+    target_sigset_t tuc_sigmask;
+    uint8_t reserved[128 - sizeof(target_sigset_t)];
+    target_sigregs_ext tuc_mcontext_ext;
 };
 
 typedef struct {
@@ -128,6 +139,24 @@ static void save_sigregs(CPUS390XState *env, target_sigregs *sregs)
     }
 }
 
+static void save_sigregs_ext(CPUS390XState *env, target_sigregs_ext *ext)
+{
+    int i;
+
+    /*
+     * if (MACHINE_HAS_VX) ...
+     * That said, we always allocate the stack storage and the
+     * space is always available in env.
+     */
+    for (i = 0; i < 16; ++i) {
+       __put_user(env->vregs[i][1], &ext->vxrs_low[i]);
+    }
+    for (i = 0; i < 16; ++i) {
+       __put_user(env->vregs[i + 16][0], &ext->vxrs_high[i][0]);
+       __put_user(env->vregs[i + 16][1], &ext->vxrs_high[i][1]);
+    }
+}
+
 void setup_frame(int sig, struct target_sigaction *ka,
                  target_sigset_t *set, CPUS390XState *env)
 {
@@ -161,6 +190,9 @@ void setup_frame(int sig, struct target_sigaction *ka,
      */
     __put_user(sig, &frame->signo);
 
+    /* Create sigregs_ext on the signal stack. */
+    save_sigregs_ext(env, &frame->sregs_ext);
+
     /*
      * Set up to return from userspace.
      * If provided, use a stub already in userspace.
@@ -202,6 +234,7 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
     rt_sigframe *frame;
     abi_ulong frame_addr;
     abi_ulong restorer;
+    abi_ulong uc_flags;
 
     frame_addr = get_sigframe(ka, env, sizeof *frame);
     trace_user_setup_rt_frame(env, frame_addr);
@@ -229,10 +262,15 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
     tswap_siginfo(&frame->info, info);
 
     /* Create ucontext on the signal stack. */
-    __put_user(0, &frame->uc.tuc_flags);
+    uc_flags = 0;
+    if (s390_has_feat(S390_FEAT_VECTOR)) {
+        uc_flags |= TARGET_UC_VXRS;
+    }
+    __put_user(uc_flags, &frame->uc.tuc_flags);
     __put_user(0, &frame->uc.tuc_link);
     target_save_altstack(&frame->uc.tuc_stack, env);
     save_sigregs(env, &frame->uc.tuc_mcontext);
+    save_sigregs_ext(env, &frame->uc.tuc_mcontext_ext);
     tswap_sigset(&frame->uc.tuc_sigmask, set);
 
     /* Set up registers for signal handler */
@@ -271,6 +309,24 @@ static void restore_sigregs(CPUS390XState *env, target_sigregs *sc)
     }
 }
 
+static void restore_sigregs_ext(CPUS390XState *env, target_sigregs_ext *ext)
+{
+    int i;
+
+    /*
+     * if (MACHINE_HAS_VX) ...
+     * That said, we always allocate the stack storage and the
+     * space is always available in env.
+     */
+    for (i = 0; i < 16; ++i) {
+       __get_user(env->vregs[i][1], &ext->vxrs_low[i]);
+    }
+    for (i = 0; i < 16; ++i) {
+       __get_user(env->vregs[i + 16][0], &ext->vxrs_high[i][0]);
+       __get_user(env->vregs[i + 16][1], &ext->vxrs_high[i][1]);
+    }
+}
+
 long do_sigreturn(CPUS390XState *env)
 {
     sigframe *frame;
@@ -292,6 +348,7 @@ long do_sigreturn(CPUS390XState *env)
     set_sigmask(&set); /* ~_BLOCKABLE? */
 
     restore_sigregs(env, &frame->sregs);
+    restore_sigregs_ext(env, &frame->sregs_ext);
 
     unlock_user_struct(frame, frame_addr, 0);
     return -TARGET_QEMU_ESIGRETURN;
@@ -313,6 +370,7 @@ long do_rt_sigreturn(CPUS390XState *env)
     set_sigmask(&set); /* ~_BLOCKABLE? */
 
     restore_sigregs(env, &frame->uc.tuc_mcontext);
+    restore_sigregs_ext(env, &frame->uc.tuc_mcontext_ext);
 
     target_restore_altstack(&frame->uc.tuc_stack, env);
 
-- 
2.25.1

Re: [PATCH v2 15/15] linux-user/s390x: Handle vector regs in signal stack

[David Hildenbrand]

On 28.04.21 21:34, Richard Henderson wrote:
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   linux-user/s390x/signal.c | 62 +++++++++++++++++++++++++++++++++++++--
>   1 file changed, 60 insertions(+), 2 deletions(-)
> 
> diff --git a/linux-user/s390x/signal.c b/linux-user/s390x/signal.c
> index 9d470e4ca0..b537646e60 100644
> --- a/linux-user/s390x/signal.c
> +++ b/linux-user/s390x/signal.c
> @@ -50,6 +50,12 @@ typedef struct {
>       target_s390_fp_regs     fpregs;
>   } target_sigregs;
>   
> +typedef struct {
> +    uint64_t vxrs_low[16];
> +    uint64_t vxrs_high[16][2];
> +    uint8_t reserved[128];
> +} target_sigregs_ext;
> +
>   typedef struct {
>       abi_ulong oldmask[_SIGCONTEXT_NSIG_WORDS];
>       abi_ulong sregs;
> @@ -60,15 +66,20 @@ typedef struct {
>       target_sigcontext sc;
>       target_sigregs sregs;
>       int signo;
> +    target_sigregs_ext sregs_ext;
>       uint16_t retcode;
>   } sigframe;
>   
> +#define TARGET_UC_VXRS 2
> +
>   struct target_ucontext {
>       abi_ulong tuc_flags;
>       abi_ulong tuc_link;
>       target_stack_t tuc_stack;
>       target_sigregs tuc_mcontext;
> -    target_sigset_t tuc_sigmask;   /* mask last for extensibility */
> +    target_sigset_t tuc_sigmask;
> +    uint8_t reserved[128 - sizeof(target_sigset_t)];

Guess I'd have used an unnamed union here

union {
	target_sigset_t tuc_sigmask;
	uint8_t reserved[128];
};

> +    target_sigregs_ext tuc_mcontext_ext;
>   };
>   
>   typedef struct {
> @@ -128,6 +139,24 @@ static void save_sigregs(CPUS390XState *env, target_sigregs *sregs)
>       }
>   }
>   
> +static void save_sigregs_ext(CPUS390XState *env, target_sigregs_ext *ext)
> +{
> +    int i;
> +
> +    /*
> +     * if (MACHINE_HAS_VX) ...
> +     * That said, we always allocate the stack storage and the
> +     * space is always available in env.
> +     */
> +    for (i = 0; i < 16; ++i) {
> +       __put_user(env->vregs[i][1], &ext->vxrs_low[i]);
> +    }
> +    for (i = 0; i < 16; ++i) {
> +       __put_user(env->vregs[i + 16][0], &ext->vxrs_high[i][0]);
> +       __put_user(env->vregs[i + 16][1], &ext->vxrs_high[i][1]);
> +    }
> +}
> +
>   void setup_frame(int sig, struct target_sigaction *ka,
>                    target_sigset_t *set, CPUS390XState *env)
>   {
> @@ -161,6 +190,9 @@ void setup_frame(int sig, struct target_sigaction *ka,
>        */
>       __put_user(sig, &frame->signo);
>   
> +    /* Create sigregs_ext on the signal stack. */
> +    save_sigregs_ext(env, &frame->sregs_ext);
> +
>       /*
>        * Set up to return from userspace.
>        * If provided, use a stub already in userspace.
> @@ -202,6 +234,7 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
>       rt_sigframe *frame;
>       abi_ulong frame_addr;
>       abi_ulong restorer;
> +    abi_ulong uc_flags;
>   
>       frame_addr = get_sigframe(ka, env, sizeof *frame);
>       trace_user_setup_rt_frame(env, frame_addr);
> @@ -229,10 +262,15 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
>       tswap_siginfo(&frame->info, info);
>   
>       /* Create ucontext on the signal stack. */
> -    __put_user(0, &frame->uc.tuc_flags);
> +    uc_flags = 0;
> +    if (s390_has_feat(S390_FEAT_VECTOR)) {
> +        uc_flags |= TARGET_UC_VXRS;
> +    }
> +    __put_user(uc_flags, &frame->uc.tuc_flags);
>       __put_user(0, &frame->uc.tuc_link);
>       target_save_altstack(&frame->uc.tuc_stack, env);
>       save_sigregs(env, &frame->uc.tuc_mcontext);
> +    save_sigregs_ext(env, &frame->uc.tuc_mcontext_ext);
>       tswap_sigset(&frame->uc.tuc_sigmask, set);
>   
>       /* Set up registers for signal handler */
> @@ -271,6 +309,24 @@ static void restore_sigregs(CPUS390XState *env, target_sigregs *sc)
>       }
>   }
>   
> +static void restore_sigregs_ext(CPUS390XState *env, target_sigregs_ext *ext)
> +{
> +    int i;
> +
> +    /*
> +     * if (MACHINE_HAS_VX) ...
> +     * That said, we always allocate the stack storage and the
> +     * space is always available in env.
> +     */
> +    for (i = 0; i < 16; ++i) {
> +       __get_user(env->vregs[i][1], &ext->vxrs_low[i]);
> +    }
> +    for (i = 0; i < 16; ++i) {
> +       __get_user(env->vregs[i + 16][0], &ext->vxrs_high[i][0]);
> +       __get_user(env->vregs[i + 16][1], &ext->vxrs_high[i][1]);
> +    }
> +}
> +
>   long do_sigreturn(CPUS390XState *env)
>   {
>       sigframe *frame;
> @@ -292,6 +348,7 @@ long do_sigreturn(CPUS390XState *env)
>       set_sigmask(&set); /* ~_BLOCKABLE? */
>   
>       restore_sigregs(env, &frame->sregs);
> +    restore_sigregs_ext(env, &frame->sregs_ext);
>   
>       unlock_user_struct(frame, frame_addr, 0);
>       return -TARGET_QEMU_ESIGRETURN;
> @@ -313,6 +370,7 @@ long do_rt_sigreturn(CPUS390XState *env)
>       set_sigmask(&set); /* ~_BLOCKABLE? */
>   
>       restore_sigregs(env, &frame->uc.tuc_mcontext);
> +    restore_sigregs_ext(env, &frame->uc.tuc_mcontext_ext);
>   
>       target_restore_altstack(&frame->uc.tuc_stack, env);
>   
> 

LGTM

Reviewed-by: David Hildenbrand <david@redhat.com>


-- 
Thanks,

David / dhildenb

Re: [PATCH v2 00/15] linux-user/s390x: some signal fixes

[Richard Henderson]

On 4/28/21 12:33 PM, Richard Henderson wrote:
> Version 2 splits lazy do-it-all patch.
> Patch 1 has an additional fix, so I dropped the r-b.

... and I realized as I hit send that this depends on the 
target_restore_altstack cleanup that's part of

https://patchew.org/QEMU/20210426025334.1168495-1-richard.henderson@linaro.org/

For avoidance of doubt:
https://gitlab.com/rth7680/qemu/-/tree/fix-signals


r~

Re: [PATCH v2 00/15] linux-user/s390x: some signal fixes

[David Hildenbrand]

On 28.04.21 21:33, Richard Henderson wrote:
> Version 2 splits lazy do-it-all patch.

Yap, that helped a lot :)


-- 
Thanks,

David / dhildenb

Re: [PATCH v2 00/15] linux-user/s390x: some signal fixes

[Cornelia Huck]

On Wed, 28 Apr 2021 12:33:53 -0700
Richard Henderson <richard.henderson@linaro.org> wrote:

> Version 2 splits lazy do-it-all patch.
> Patch 1 has an additional fix, so I dropped the r-b.
> 
> r~
> 
> Richard Henderson (15):
>   linux-user/s390x: Fix sigframe types
>   linux-user/s390x: Use uint16_t for signal retcode
>   linux-user/s390x: Remove PSW_ADDR_AMODE
>   linux-user/s390x: Remove restore_sigregs return value
>   linux-user/s390x: Fix trace in restore_regs
>   linux-user/s390x: Fix sigcontext sregs value
>   linux-user/s390x: Use tswap_sigset in setup_rt_frame
>   linux-user/s390x: Tidy save_sigregs
>   linux-user/s390x: Clean up single-use gotos in signal.c
>   linux-user/s390x: Set psw.mask properly for the signal handler
>   linux-user/s390x: Add stub sigframe argument for last_break
>   linux-user/s390x: Fix frame_addr corruption in setup_frame
>   linux-user/s390x: Add build asserts for sigset sizes
>   linux-user/s390x: Clean up signal.c
>   linux-user/s390x: Handle vector regs in signal stack
> 
>  linux-user/s390x/signal.c | 280 +++++++++++++++++++++++---------------
>  1 file changed, 170 insertions(+), 110 deletions(-)
> 

I assume the route-to-upstream for this is through the linux-user tree
and not the s390x tree, right?

Re: [PATCH v2 00/15] linux-user/s390x: some signal fixes

[Laurent Vivier]

Le 06/05/2021 à 13:54, Cornelia Huck a écrit :
> On Wed, 28 Apr 2021 12:33:53 -0700
> Richard Henderson <richard.henderson@linaro.org> wrote:
> 
>> Version 2 splits lazy do-it-all patch.
>> Patch 1 has an additional fix, so I dropped the r-b.
>>
>> r~
>>
>> Richard Henderson (15):
>>   linux-user/s390x: Fix sigframe types
>>   linux-user/s390x: Use uint16_t for signal retcode
>>   linux-user/s390x: Remove PSW_ADDR_AMODE
>>   linux-user/s390x: Remove restore_sigregs return value
>>   linux-user/s390x: Fix trace in restore_regs
>>   linux-user/s390x: Fix sigcontext sregs value
>>   linux-user/s390x: Use tswap_sigset in setup_rt_frame
>>   linux-user/s390x: Tidy save_sigregs
>>   linux-user/s390x: Clean up single-use gotos in signal.c
>>   linux-user/s390x: Set psw.mask properly for the signal handler
>>   linux-user/s390x: Add stub sigframe argument for last_break
>>   linux-user/s390x: Fix frame_addr corruption in setup_frame
>>   linux-user/s390x: Add build asserts for sigset sizes
>>   linux-user/s390x: Clean up signal.c
>>   linux-user/s390x: Handle vector regs in signal stack
>>
>>  linux-user/s390x/signal.c | 280 +++++++++++++++++++++++---------------
>>  1 file changed, 170 insertions(+), 110 deletions(-)
>>
> 
> I assume the route-to-upstream for this is through the linux-user tree
> and not the s390x tree, right?
> 

Yes, and I'll try to do a PR soon.

Thanks,
Laurent

Re: [PATCH v2 00/15] linux-user/s390x: some signal fixes

[Laurent Vivier]

Le 28/04/2021 à 21:33, Richard Henderson a écrit :
> Version 2 splits lazy do-it-all patch.
> Patch 1 has an additional fix, so I dropped the r-b.
> 
> r~
> 
> Richard Henderson (15):
>   linux-user/s390x: Fix sigframe types
>   linux-user/s390x: Use uint16_t for signal retcode
>   linux-user/s390x: Remove PSW_ADDR_AMODE
>   linux-user/s390x: Remove restore_sigregs return value
>   linux-user/s390x: Fix trace in restore_regs
>   linux-user/s390x: Fix sigcontext sregs value
>   linux-user/s390x: Use tswap_sigset in setup_rt_frame
>   linux-user/s390x: Tidy save_sigregs
>   linux-user/s390x: Clean up single-use gotos in signal.c
>   linux-user/s390x: Set psw.mask properly for the signal handler
>   linux-user/s390x: Add stub sigframe argument for last_break
>   linux-user/s390x: Fix frame_addr corruption in setup_frame
>   linux-user/s390x: Add build asserts for sigset sizes
>   linux-user/s390x: Clean up signal.c
>   linux-user/s390x: Handle vector regs in signal stack
> 
>  linux-user/s390x/signal.c | 280 +++++++++++++++++++++++---------------
>  1 file changed, 170 insertions(+), 110 deletions(-)
> 

Applied to my linux-user-for-6.1 branch.

Thanks,
Laurent