[PATCH 0/5] linux-user/s390x: Fix psw.mask handling in signals

[PATCH 0/5] linux-user/s390x: Fix psw.mask handling in signals

[Richard Henderson]

The PSW_MASK_CC component of psw.mask was not handled properly
in the creation or restoration of signal frames.


r~


Richard Henderson (5):
  target/s390x: Expose load_psw and get_psw_mask to cpu.h
  target/s390x: Do not modify cpu state in s390_cpu_get_psw_mask
  target/s390x: Improve s390_cpu_dump_state vs cc_op
  target/s390x: Use s390_cpu_{set_psw,get_psw_mask} in gdbstub
  linux-user/s390x: Save and restore psw.mask properly

 target/s390x/cpu.h         |   3 ++
 target/s390x/internal.h    |   5 --
 linux-user/s390x/signal.c  |  37 ++++++++++++--
 target/s390x/cc_helper.c   |   2 +-
 target/s390x/excp_helper.c |  28 +++++-----
 target/s390x/gdbstub.c     |  15 +-----
 target/s390x/helper.c      | 101 ++++++++++++++++++++-----------------
 target/s390x/sigp.c        |   3 +-
 8 files changed, 110 insertions(+), 84 deletions(-)

-- 
2.25.1

[PATCH 1/5] target/s390x: Expose load_psw and get_psw_mask to cpu.h

[Richard Henderson]

Rename to s390_cpu_set_psw and s390_cpu_get_psw_mask at the
same time.  Adjust so that they compile for user-only.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/s390x/cpu.h         |  3 ++
 target/s390x/internal.h    |  5 ---
 target/s390x/cc_helper.c   |  2 +-
 target/s390x/excp_helper.c | 28 ++++++------
 target/s390x/helper.c      | 89 ++++++++++++++++++++------------------
 target/s390x/sigp.c        |  3 +-
 6 files changed, 69 insertions(+), 61 deletions(-)

diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 2464d4076c..b26ae8fff2 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -845,6 +845,9 @@ int s390_cpu_pv_mem_rw(S390CPU *cpu, unsigned int offset, void *hostbuf,
 int s390_cpu_restart(S390CPU *cpu);
 void s390_init_sigp(void);
 
+/* helper.c */
+void s390_cpu_set_psw(CPUS390XState *env, uint64_t mask, uint64_t addr);
+uint64_t s390_cpu_get_psw_mask(CPUS390XState *env);
 
 /* outside of target/s390x/ */
 S390CPU *s390_cpu_addr2state(uint16_t cpu_addr);
diff --git a/target/s390x/internal.h b/target/s390x/internal.h
index 11515bb617..4a18b74d10 100644
--- a/target/s390x/internal.h
+++ b/target/s390x/internal.h
@@ -235,10 +235,6 @@ int s390_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
 const char *cc_name(enum cc_op cc_op);
 uint32_t calc_cc(CPUS390XState *env, uint32_t cc_op, uint64_t src, uint64_t dst,
                  uint64_t vr);
-#ifndef CONFIG_USER_ONLY
-void load_psw(CPUS390XState *env, uint64_t mask, uint64_t addr);
-#endif /* CONFIG_USER_ONLY */
-
 
 /* cpu.c */
 #ifndef CONFIG_USER_ONLY
@@ -303,7 +299,6 @@ void s390_cpu_gdb_init(CPUState *cs);
 void s390_cpu_dump_state(CPUState *cpu, FILE *f, int flags);
 void do_restart_interrupt(CPUS390XState *env);
 #ifndef CONFIG_USER_ONLY
-uint64_t get_psw_mask(CPUS390XState *env);
 void s390_cpu_recompute_watchpoints(CPUState *cs);
 void s390x_tod_timer(void *opaque);
 void s390x_cpu_timer(void *opaque);
diff --git a/target/s390x/cc_helper.c b/target/s390x/cc_helper.c
index e7039d0d18..e7a74d66dd 100644
--- a/target/s390x/cc_helper.c
+++ b/target/s390x/cc_helper.c
@@ -509,7 +509,7 @@ uint32_t HELPER(calc_cc)(CPUS390XState *env, uint32_t cc_op, uint64_t src,
 #ifndef CONFIG_USER_ONLY
 void HELPER(load_psw)(CPUS390XState *env, uint64_t mask, uint64_t addr)
 {
-    load_psw(env, mask, addr);
+    s390_cpu_set_psw(env, mask, addr);
     cpu_loop_exit(env_cpu(env));
 }
 
diff --git a/target/s390x/excp_helper.c b/target/s390x/excp_helper.c
index 20625c2c8f..9c361428c8 100644
--- a/target/s390x/excp_helper.c
+++ b/target/s390x/excp_helper.c
@@ -252,7 +252,7 @@ static void do_program_interrupt(CPUS390XState *env)
 
     lowcore->pgm_ilen = cpu_to_be16(ilen);
     lowcore->pgm_code = cpu_to_be16(env->int_pgm_code);
-    lowcore->program_old_psw.mask = cpu_to_be64(get_psw_mask(env));
+    lowcore->program_old_psw.mask = cpu_to_be64(s390_cpu_get_psw_mask(env));
     lowcore->program_old_psw.addr = cpu_to_be64(env->psw.addr);
     mask = be64_to_cpu(lowcore->program_new_psw.mask);
     addr = be64_to_cpu(lowcore->program_new_psw.addr);
@@ -260,7 +260,7 @@ static void do_program_interrupt(CPUS390XState *env)
 
     cpu_unmap_lowcore(lowcore);
 
-    load_psw(env, mask, addr);
+    s390_cpu_set_psw(env, mask, addr);
 }
 
 static void do_svc_interrupt(CPUS390XState *env)
@@ -272,14 +272,14 @@ static void do_svc_interrupt(CPUS390XState *env)
 
     lowcore->svc_code = cpu_to_be16(env->int_svc_code);
     lowcore->svc_ilen = cpu_to_be16(env->int_svc_ilen);
-    lowcore->svc_old_psw.mask = cpu_to_be64(get_psw_mask(env));
+    lowcore->svc_old_psw.mask = cpu_to_be64(s390_cpu_get_psw_mask(env));
     lowcore->svc_old_psw.addr = cpu_to_be64(env->psw.addr + env->int_svc_ilen);
     mask = be64_to_cpu(lowcore->svc_new_psw.mask);
     addr = be64_to_cpu(lowcore->svc_new_psw.addr);
 
     cpu_unmap_lowcore(lowcore);
 
-    load_psw(env, mask, addr);
+    s390_cpu_set_psw(env, mask, addr);
 
     /* When a PER event is pending, the PER exception has to happen
        immediately after the SERVICE CALL one.  */
@@ -348,12 +348,12 @@ static void do_ext_interrupt(CPUS390XState *env)
 
     mask = be64_to_cpu(lowcore->external_new_psw.mask);
     addr = be64_to_cpu(lowcore->external_new_psw.addr);
-    lowcore->external_old_psw.mask = cpu_to_be64(get_psw_mask(env));
+    lowcore->external_old_psw.mask = cpu_to_be64(s390_cpu_get_psw_mask(env));
     lowcore->external_old_psw.addr = cpu_to_be64(env->psw.addr);
 
     cpu_unmap_lowcore(lowcore);
 
-    load_psw(env, mask, addr);
+    s390_cpu_set_psw(env, mask, addr);
 }
 
 static void do_io_interrupt(CPUS390XState *env)
@@ -373,7 +373,7 @@ static void do_io_interrupt(CPUS390XState *env)
     lowcore->subchannel_nr = cpu_to_be16(io->nr);
     lowcore->io_int_parm = cpu_to_be32(io->parm);
     lowcore->io_int_word = cpu_to_be32(io->word);
-    lowcore->io_old_psw.mask = cpu_to_be64(get_psw_mask(env));
+    lowcore->io_old_psw.mask = cpu_to_be64(s390_cpu_get_psw_mask(env));
     lowcore->io_old_psw.addr = cpu_to_be64(env->psw.addr);
     mask = be64_to_cpu(lowcore->io_new_psw.mask);
     addr = be64_to_cpu(lowcore->io_new_psw.addr);
@@ -381,7 +381,7 @@ static void do_io_interrupt(CPUS390XState *env)
     cpu_unmap_lowcore(lowcore);
     g_free(io);
 
-    load_psw(env, mask, addr);
+    s390_cpu_set_psw(env, mask, addr);
 }
 
 typedef struct MchkExtSaveArea {
@@ -457,14 +457,14 @@ static void do_mchk_interrupt(CPUS390XState *env)
     lowcore->clock_comp_save_area = cpu_to_be64(env->ckc >> 8);
 
     lowcore->mcic = cpu_to_be64(mcic);
-    lowcore->mcck_old_psw.mask = cpu_to_be64(get_psw_mask(env));
+    lowcore->mcck_old_psw.mask = cpu_to_be64(s390_cpu_get_psw_mask(env));
     lowcore->mcck_old_psw.addr = cpu_to_be64(env->psw.addr);
     mask = be64_to_cpu(lowcore->mcck_new_psw.mask);
     addr = be64_to_cpu(lowcore->mcck_new_psw.addr);
 
     cpu_unmap_lowcore(lowcore);
 
-    load_psw(env, mask, addr);
+    s390_cpu_set_psw(env, mask, addr);
 }
 
 void s390_cpu_do_interrupt(CPUState *cs)
@@ -592,9 +592,11 @@ void s390x_cpu_debug_excp_handler(CPUState *cs)
            and MVCS instrutions are not used.  */
         env->per_perc_atmid |= env->psw.mask & (PSW_MASK_ASC) >> 46;
 
-        /* Remove all watchpoints to re-execute the code.  A PER exception
-           will be triggered, it will call load_psw which will recompute
-           the watchpoints.  */
+        /*
+         * Remove all watchpoints to re-execute the code.  A PER exception
+         * will be triggered, it will call s390_cpu_set_psw which will
+         * recompute the watchpoints.
+         */
         cpu_watchpoint_remove_all(cs, BP_CPU);
         cpu_loop_exit_noexc(cs);
     }
diff --git a/target/s390x/helper.c b/target/s390x/helper.c
index 7678994feb..d311903b94 100644
--- a/target/s390x/helper.c
+++ b/target/s390x/helper.c
@@ -104,44 +104,6 @@ void s390_handle_wait(S390CPU *cpu)
     }
 }
 
-void load_psw(CPUS390XState *env, uint64_t mask, uint64_t addr)
-{
-    uint64_t old_mask = env->psw.mask;
-
-    env->psw.addr = addr;
-    env->psw.mask = mask;
-
-    /* KVM will handle all WAITs and trigger a WAIT exit on disabled_wait */
-    if (!tcg_enabled()) {
-        return;
-    }
-    env->cc_op = (mask >> 44) & 3;
-
-    if ((old_mask ^ mask) & PSW_MASK_PER) {
-        s390_cpu_recompute_watchpoints(env_cpu(env));
-    }
-
-    if (mask & PSW_MASK_WAIT) {
-        s390_handle_wait(env_archcpu(env));
-    }
-}
-
-uint64_t get_psw_mask(CPUS390XState *env)
-{
-    uint64_t r = env->psw.mask;
-
-    if (tcg_enabled()) {
-        env->cc_op = calc_cc(env, env->cc_op, env->cc_src, env->cc_dst,
-                             env->cc_vr);
-
-        r &= ~PSW_MASK_CC;
-        assert(!(env->cc_op & ~3));
-        r |= (uint64_t)env->cc_op << 44;
-    }
-
-    return r;
-}
-
 LowCore *cpu_map_lowcore(CPUS390XState *env)
 {
     LowCore *lowcore;
@@ -168,7 +130,7 @@ void do_restart_interrupt(CPUS390XState *env)
 
     lowcore = cpu_map_lowcore(env);
 
-    lowcore->restart_old_psw.mask = cpu_to_be64(get_psw_mask(env));
+    lowcore->restart_old_psw.mask = cpu_to_be64(s390_cpu_get_psw_mask(env));
     lowcore->restart_old_psw.addr = cpu_to_be64(env->psw.addr);
     mask = be64_to_cpu(lowcore->restart_new_psw.mask);
     addr = be64_to_cpu(lowcore->restart_new_psw.addr);
@@ -176,7 +138,7 @@ void do_restart_interrupt(CPUS390XState *env)
     cpu_unmap_lowcore(lowcore);
     env->pending_int &= ~INTERRUPT_RESTART;
 
-    load_psw(env, mask, addr);
+    s390_cpu_set_psw(env, mask, addr);
 }
 
 void s390_cpu_recompute_watchpoints(CPUState *cs)
@@ -266,7 +228,7 @@ int s390_store_status(S390CPU *cpu, hwaddr addr, bool store_arch)
         sa->grs[i] = cpu_to_be64(cpu->env.regs[i]);
     }
     sa->psw.addr = cpu_to_be64(cpu->env.psw.addr);
-    sa->psw.mask = cpu_to_be64(get_psw_mask(&cpu->env));
+    sa->psw.mask = cpu_to_be64(s390_cpu_get_psw_mask(&cpu->env));
     sa->prefix = cpu_to_be32(cpu->env.psa);
     sa->fpc = cpu_to_be32(cpu->env.fpc);
     sa->todpr = cpu_to_be32(cpu->env.todpr);
@@ -323,8 +285,53 @@ int s390_store_adtl_status(S390CPU *cpu, hwaddr addr, hwaddr len)
     cpu_physical_memory_unmap(sa, len, 1, len);
     return 0;
 }
+#else
+/* For user-only, tcg is always enabled. */
+#define tcg_enabled() true
 #endif /* CONFIG_USER_ONLY */
 
+void s390_cpu_set_psw(CPUS390XState *env, uint64_t mask, uint64_t addr)
+{
+#ifndef CONFIG_USER_ONLY
+    uint64_t old_mask = env->psw.mask;
+#endif
+
+    env->psw.addr = addr;
+    env->psw.mask = mask;
+
+    /* KVM will handle all WAITs and trigger a WAIT exit on disabled_wait */
+    if (!tcg_enabled()) {
+        return;
+    }
+    env->cc_op = (mask >> 44) & 3;
+
+#ifndef CONFIG_USER_ONLY
+    if ((old_mask ^ mask) & PSW_MASK_PER) {
+        s390_cpu_recompute_watchpoints(env_cpu(env));
+    }
+
+    if (mask & PSW_MASK_WAIT) {
+        s390_handle_wait(env_archcpu(env));
+    }
+#endif
+}
+
+uint64_t s390_cpu_get_psw_mask(CPUS390XState *env)
+{
+    uint64_t r = env->psw.mask;
+
+    if (tcg_enabled()) {
+        env->cc_op = calc_cc(env, env->cc_op, env->cc_src, env->cc_dst,
+                             env->cc_vr);
+
+        r &= ~PSW_MASK_CC;
+        assert(!(env->cc_op & ~3));
+        r |= (uint64_t)env->cc_op << 44;
+    }
+
+    return r;
+}
+
 void s390_cpu_dump_state(CPUState *cs, FILE *f, int flags)
 {
     S390CPU *cpu = S390_CPU(cs);
diff --git a/target/s390x/sigp.c b/target/s390x/sigp.c
index c604f17710..c2d5cdf061 100644
--- a/target/s390x/sigp.c
+++ b/target/s390x/sigp.c
@@ -235,7 +235,8 @@ static void sigp_restart(CPUState *cs, run_on_cpu_data arg)
         cpu_synchronize_state(cs);
         /*
          * Set OPERATING (and unhalting) before loading the restart PSW.
-         * load_psw() will then properly halt the CPU again if necessary (TCG).
+         * s390_cpu_set_psw() will then properly halt the CPU again if
+         * necessary (TCG).
          */
         s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
         do_restart_interrupt(&cpu->env);
-- 
2.25.1

[PATCH 2/5] target/s390x: Do not modify cpu state in s390_cpu_get_psw_mask

[Richard Henderson]

We want to use this function for debugging, and debug should
not modify cpu state (even non-architectural cpu state) lest
we introduce heisenbugs.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/s390x/helper.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/target/s390x/helper.c b/target/s390x/helper.c
index d311903b94..559fc3573f 100644
--- a/target/s390x/helper.c
+++ b/target/s390x/helper.c
@@ -321,12 +321,12 @@ uint64_t s390_cpu_get_psw_mask(CPUS390XState *env)
     uint64_t r = env->psw.mask;
 
     if (tcg_enabled()) {
-        env->cc_op = calc_cc(env, env->cc_op, env->cc_src, env->cc_dst,
-                             env->cc_vr);
+        uint64_t cc = calc_cc(env, env->cc_op, env->cc_src,
+                              env->cc_dst, env->cc_vr);
 
+        assert(cc <= 3);
         r &= ~PSW_MASK_CC;
-        assert(!(env->cc_op & ~3));
-        r |= (uint64_t)env->cc_op << 44;
+        r |= cc << 44;
     }
 
     return r;
-- 
2.25.1

[PATCH 3/5] target/s390x: Improve s390_cpu_dump_state vs cc_op

[Richard Henderson]

Use s390_cpu_get_psw_mask so that we print the correct
architectural value of psw.mask.  Do not print cc_op
unless tcg_enabled.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/s390x/helper.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/target/s390x/helper.c b/target/s390x/helper.c
index 559fc3573f..1445b74451 100644
--- a/target/s390x/helper.c
+++ b/target/s390x/helper.c
@@ -338,12 +338,14 @@ void s390_cpu_dump_state(CPUState *cs, FILE *f, int flags)
     CPUS390XState *env = &cpu->env;
     int i;
 
-    if (env->cc_op > 3) {
-        qemu_fprintf(f, "PSW=mask %016" PRIx64 " addr %016" PRIx64 " cc %15s\n",
-                     env->psw.mask, env->psw.addr, cc_name(env->cc_op));
+    qemu_fprintf(f, "PSW=mask %016" PRIx64 " addr %016" PRIx64,
+                 s390_cpu_get_psw_mask(env), env->psw.addr);
+    if (!tcg_enabled()) {
+        qemu_fprintf(f, "\n");
+    } else if (env->cc_op > 3) {
+        qemu_fprintf(f, " cc %15s\n", cc_name(env->cc_op));
     } else {
-        qemu_fprintf(f, "PSW=mask %016" PRIx64 " addr %016" PRIx64 " cc %02x\n",
-                     env->psw.mask, env->psw.addr, env->cc_op);
+        qemu_fprintf(f, " cc %02x\n", env->cc_op);
     }
 
     for (i = 0; i < 16; i++) {
-- 
2.25.1

[PATCH 4/5] target/s390x: Use s390_cpu_{set_psw, get_psw_mask} in gdbstub

[Richard Henderson]

No change in behaviour, as gdbstub was correctly written to
install and extract the cc value.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/s390x/gdbstub.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/target/s390x/gdbstub.c b/target/s390x/gdbstub.c
index d6fce5ff1e..5b4e38a13b 100644
--- a/target/s390x/gdbstub.c
+++ b/target/s390x/gdbstub.c
@@ -31,18 +31,10 @@ int s390_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
 {
     S390CPU *cpu = S390_CPU(cs);
     CPUS390XState *env = &cpu->env;
-    uint64_t val;
-    int cc_op;
 
     switch (n) {
     case S390_PSWM_REGNUM:
-        if (tcg_enabled()) {
-            cc_op = calc_cc(env, env->cc_op, env->cc_src, env->cc_dst,
-                            env->cc_vr);
-            val = deposit64(env->psw.mask, 44, 2, cc_op);
-            return gdb_get_regl(mem_buf, val);
-        }
-        return gdb_get_regl(mem_buf, env->psw.mask);
+        return gdb_get_regl(mem_buf, s390_cpu_get_psw_mask(env));
     case S390_PSWA_REGNUM:
         return gdb_get_regl(mem_buf, env->psw.addr);
     case S390_R0_REGNUM ... S390_R15_REGNUM:
@@ -59,10 +51,7 @@ int s390_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
 
     switch (n) {
     case S390_PSWM_REGNUM:
-        env->psw.mask = tmpl;
-        if (tcg_enabled()) {
-            env->cc_op = extract64(tmpl, 44, 2);
-        }
+        s390_cpu_set_psw(env, tmpl, env->psw.addr);
         break;
     case S390_PSWA_REGNUM:
         env->psw.addr = tmpl;
-- 
2.25.1

[PATCH 5/5] linux-user/s390x: Save and restore psw.mask properly

[Richard Henderson]

At present, we're referencing env->psw.mask directly, which
fails to ensure that env->cc_op is incorporated or updated.
Use s390_cpu_{set_psw,get_psw_mask} to fix this.

Mirror the kernel's cleaning of the psw.mask in save_sigregs
and restore_sigregs.  Ignore PSW_MASK_RI for now, as qemu does
not support that.

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

diff --git a/linux-user/s390x/signal.c b/linux-user/s390x/signal.c
index ef136dae33..bf8a8fbfe9 100644
--- a/linux-user/s390x/signal.c
+++ b/linux-user/s390x/signal.c
@@ -112,15 +112,23 @@ get_sigframe(struct target_sigaction *ka, CPUS390XState *env, size_t frame_size)
     return (sp - frame_size) & -8ul;
 }
 
+#define PSW_USER_BITS   (PSW_MASK_DAT | PSW_MASK_IO | PSW_MASK_EXT | \
+                         PSW_MASK_MCHECK | PSW_MASK_PSTATE | PSW_ASC_PRIMARY)
+#define PSW_MASK_USER   (PSW_MASK_ASC | PSW_MASK_CC | PSW_MASK_PM | \
+                         PSW_MASK_64 | PSW_MASK_32)
+
 static void save_sigregs(CPUS390XState *env, target_sigregs *sregs)
 {
+    uint64_t psw_mask = s390_cpu_get_psw_mask(env);
     int i;
 
     /*
      * Copy a 'clean' PSW mask to the user to avoid leaking
      * information about whether PER is currently on.
+     * TODO: qemu does not support PSW_MASK_RI; it will never be set.
      */
-    __put_user(env->psw.mask, &sregs->regs.psw.mask);
+    psw_mask = PSW_USER_BITS | (psw_mask & PSW_MASK_USER);
+    __put_user(psw_mask, &sregs->regs.psw.mask);
     __put_user(env->psw.addr, &sregs->regs.psw.addr);
 
     for (i = 0; i < 16; i++) {
@@ -289,7 +297,7 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
 
 static void restore_sigregs(CPUS390XState *env, target_sigregs *sc)
 {
-    target_ulong prev_addr;
+    uint64_t prev_addr, prev_mask, mask, addr;
     int i;
 
     for (i = 0; i < 16; i++) {
@@ -297,9 +305,28 @@ static void restore_sigregs(CPUS390XState *env, target_sigregs *sc)
     }
 
     prev_addr = env->psw.addr;
-    __get_user(env->psw.mask, &sc->regs.psw.mask);
-    __get_user(env->psw.addr, &sc->regs.psw.addr);
-    trace_user_s390x_restore_sigregs(env, env->psw.addr, prev_addr);
+    __get_user(mask, &sc->regs.psw.mask);
+    __get_user(addr, &sc->regs.psw.addr);
+    trace_user_s390x_restore_sigregs(env, addr, prev_addr);
+
+    /*
+     * Use current psw.mask to preserve PER bit.
+     * TODO:
+     *  if (!is_ri_task(current) && (user_sregs.regs.psw.mask & PSW_MASK_RI))
+     *          return -EINVAL;
+     * Simply do not allow it to be set in mask.
+     */
+    prev_mask = s390_cpu_get_psw_mask(env);
+    mask = (prev_mask & ~PSW_MASK_USER) | (mask & PSW_MASK_USER);
+    /* Check for invalid user address space control. */
+    if ((mask & PSW_MASK_ASC) == PSW_ASC_HOME) {
+        mask = (mask & ~PSW_MASK_ASC) | PSW_ASC_PRIMARY;
+    }
+    /* Check for invalid amode. */
+    if (mask & PSW_MASK_64) {
+        mask |= PSW_MASK_32;
+    }
+    s390_cpu_set_psw(env, mask, addr);
 
     for (i = 0; i < 16; i++) {
         __get_user(env->aregs[i], &sc->regs.acrs[i]);
-- 
2.25.1

Re: [PATCH 1/5] target/s390x: Expose load_psw and get_psw_mask to cpu.h

[David Hildenbrand]

On 15.06.21 05:07, Richard Henderson wrote:
> Rename to s390_cpu_set_psw and s390_cpu_get_psw_mask at the
> same time.  Adjust so that they compile for user-only.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

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


-- 
Thanks,

David / dhildenb

Re: [PATCH 2/5] target/s390x: Do not modify cpu state in s390_cpu_get_psw_mask

[David Hildenbrand]

On 15.06.21 05:07, Richard Henderson wrote:
> We want to use this function for debugging, and debug should
> not modify cpu state (even non-architectural cpu state) lest
> we introduce heisenbugs.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/s390x/helper.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/target/s390x/helper.c b/target/s390x/helper.c
> index d311903b94..559fc3573f 100644
> --- a/target/s390x/helper.c
> +++ b/target/s390x/helper.c
> @@ -321,12 +321,12 @@ uint64_t s390_cpu_get_psw_mask(CPUS390XState *env)
>       uint64_t r = env->psw.mask;
>   
>       if (tcg_enabled()) {
> -        env->cc_op = calc_cc(env, env->cc_op, env->cc_src, env->cc_dst,
> -                             env->cc_vr);
> +        uint64_t cc = calc_cc(env, env->cc_op, env->cc_src,
> +                              env->cc_dst, env->cc_vr);
>   
> +        assert(cc <= 3);
>           r &= ~PSW_MASK_CC;
> -        assert(!(env->cc_op & ~3));
> -        r |= (uint64_t)env->cc_op << 44;
> +        r |= cc << 44;
>       }
>   
>       return r;
> 

I wonder if this can actually suboptimal performance-wise, because we 
might be recomputing the same thing. However, I guess it's rather a 
corner case

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

-- 
Thanks,

David / dhildenb

Re: [PATCH 3/5] target/s390x: Improve s390_cpu_dump_state vs cc_op

[David Hildenbrand]

On 15.06.21 05:07, Richard Henderson wrote:
> Use s390_cpu_get_psw_mask so that we print the correct
> architectural value of psw.mask.  Do not print cc_op
> unless tcg_enabled.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/s390x/helper.c | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/target/s390x/helper.c b/target/s390x/helper.c
> index 559fc3573f..1445b74451 100644
> --- a/target/s390x/helper.c
> +++ b/target/s390x/helper.c
> @@ -338,12 +338,14 @@ void s390_cpu_dump_state(CPUState *cs, FILE *f, int flags)
>       CPUS390XState *env = &cpu->env;
>       int i;
>   
> -    if (env->cc_op > 3) {
> -        qemu_fprintf(f, "PSW=mask %016" PRIx64 " addr %016" PRIx64 " cc %15s\n",
> -                     env->psw.mask, env->psw.addr, cc_name(env->cc_op));
> +    qemu_fprintf(f, "PSW=mask %016" PRIx64 " addr %016" PRIx64,
> +                 s390_cpu_get_psw_mask(env), env->psw.addr);
> +    if (!tcg_enabled()) {
> +        qemu_fprintf(f, "\n");
> +    } else if (env->cc_op > 3) {
> +        qemu_fprintf(f, " cc %15s\n", cc_name(env->cc_op));
>       } else {
> -        qemu_fprintf(f, "PSW=mask %016" PRIx64 " addr %016" PRIx64 " cc %02x\n",
> -                     env->psw.mask, env->psw.addr, env->cc_op);
> +        qemu_fprintf(f, " cc %02x\n", env->cc_op);
>       }

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

-- 
Thanks,

David / dhildenb

Re: [PATCH 4/5] target/s390x: Use s390_cpu_{set_psw, get_psw_mask} in gdbstub

[David Hildenbrand]

On 15.06.21 05:07, Richard Henderson wrote:
> No change in behaviour, as gdbstub was correctly written to
> install and extract the cc value.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/s390x/gdbstub.c | 15 ++-------------
>   1 file changed, 2 insertions(+), 13 deletions(-)
> 
> diff --git a/target/s390x/gdbstub.c b/target/s390x/gdbstub.c
> index d6fce5ff1e..5b4e38a13b 100644
> --- a/target/s390x/gdbstub.c
> +++ b/target/s390x/gdbstub.c
> @@ -31,18 +31,10 @@ int s390_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
>   {
>       S390CPU *cpu = S390_CPU(cs);
>       CPUS390XState *env = &cpu->env;
> -    uint64_t val;
> -    int cc_op;
>   
>       switch (n) {
>       case S390_PSWM_REGNUM:
> -        if (tcg_enabled()) {
> -            cc_op = calc_cc(env, env->cc_op, env->cc_src, env->cc_dst,
> -                            env->cc_vr);
> -            val = deposit64(env->psw.mask, 44, 2, cc_op);
> -            return gdb_get_regl(mem_buf, val);
> -        }
> -        return gdb_get_regl(mem_buf, env->psw.mask);
> +        return gdb_get_regl(mem_buf, s390_cpu_get_psw_mask(env));
>       case S390_PSWA_REGNUM:
>           return gdb_get_regl(mem_buf, env->psw.addr);
>       case S390_R0_REGNUM ... S390_R15_REGNUM:
> @@ -59,10 +51,7 @@ int s390_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
>   
>       switch (n) {
>       case S390_PSWM_REGNUM:
> -        env->psw.mask = tmpl;
> -        if (tcg_enabled()) {
> -            env->cc_op = extract64(tmpl, 44, 2);
> -        }
> +        s390_cpu_set_psw(env, tmpl, env->psw.addr);
>           break;
>       case S390_PSWA_REGNUM:
>           env->psw.addr = tmpl;
> 

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

-- 
Thanks,

David / dhildenb

Re: [PATCH 5/5] linux-user/s390x: Save and restore psw.mask properly

[David Hildenbrand]

On 15.06.21 05:07, Richard Henderson wrote:
> At present, we're referencing env->psw.mask directly, which
> fails to ensure that env->cc_op is incorporated or updated.
> Use s390_cpu_{set_psw,get_psw_mask} to fix this.
> 
> Mirror the kernel's cleaning of the psw.mask in save_sigregs
> and restore_sigregs.  Ignore PSW_MASK_RI for now, as qemu does
> not support that.

s/qemu/qemu\/tcg/ ? because qemu/kvm supports it .

Maybe reference the bug reports this fixes?

> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   linux-user/s390x/signal.c | 37 ++++++++++++++++++++++++++++++++-----
>   1 file changed, 32 insertions(+), 5 deletions(-)
> 
> diff --git a/linux-user/s390x/signal.c b/linux-user/s390x/signal.c
> index ef136dae33..bf8a8fbfe9 100644
> --- a/linux-user/s390x/signal.c
> +++ b/linux-user/s390x/signal.c
> @@ -112,15 +112,23 @@ get_sigframe(struct target_sigaction *ka, CPUS390XState *env, size_t frame_size)
>       return (sp - frame_size) & -8ul;
>   }
>   
> +#define PSW_USER_BITS   (PSW_MASK_DAT | PSW_MASK_IO | PSW_MASK_EXT | \
> +                         PSW_MASK_MCHECK | PSW_MASK_PSTATE | PSW_ASC_PRIMARY)
> +#define PSW_MASK_USER   (PSW_MASK_ASC | PSW_MASK_CC | PSW_MASK_PM | \
> +                         PSW_MASK_64 | PSW_MASK_32)
> +
>   static void save_sigregs(CPUS390XState *env, target_sigregs *sregs)
>   {
> +    uint64_t psw_mask = s390_cpu_get_psw_mask(env);
>       int i;
>   
>       /*
>        * Copy a 'clean' PSW mask to the user to avoid leaking
>        * information about whether PER is currently on.
> +     * TODO: qemu does not support PSW_MASK_RI; it will never be set.
>        */
> -    __put_user(env->psw.mask, &sregs->regs.psw.mask);
> +    psw_mask = PSW_USER_BITS | (psw_mask & PSW_MASK_USER);
> +    __put_user(psw_mask, &sregs->regs.psw.mask);
>       __put_user(env->psw.addr, &sregs->regs.psw.addr);
>   
>       for (i = 0; i < 16; i++) {
> @@ -289,7 +297,7 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
>   
>   static void restore_sigregs(CPUS390XState *env, target_sigregs *sc)
>   {
> -    target_ulong prev_addr;
> +    uint64_t prev_addr, prev_mask, mask, addr;
>       int i;
>   
>       for (i = 0; i < 16; i++) {
> @@ -297,9 +305,28 @@ static void restore_sigregs(CPUS390XState *env, target_sigregs *sc)
>       }
>   
>       prev_addr = env->psw.addr;
> -    __get_user(env->psw.mask, &sc->regs.psw.mask);
> -    __get_user(env->psw.addr, &sc->regs.psw.addr);
> -    trace_user_s390x_restore_sigregs(env, env->psw.addr, prev_addr);
> +    __get_user(mask, &sc->regs.psw.mask);
> +    __get_user(addr, &sc->regs.psw.addr);
> +    trace_user_s390x_restore_sigregs(env, addr, prev_addr);
> +
> +    /*
> +     * Use current psw.mask to preserve PER bit.
> +     * TODO:
> +     *  if (!is_ri_task(current) && (user_sregs.regs.psw.mask & PSW_MASK_RI))
> +     *          return -EINVAL;
> +     * Simply do not allow it to be set in mask.
> +     */
> +    prev_mask = s390_cpu_get_psw_mask(env);
> +    mask = (prev_mask & ~PSW_MASK_USER) | (mask & PSW_MASK_USER);
> +    /* Check for invalid user address space control. */
> +    if ((mask & PSW_MASK_ASC) == PSW_ASC_HOME) {
> +        mask = (mask & ~PSW_MASK_ASC) | PSW_ASC_PRIMARY;
> +    }
> +    /* Check for invalid amode. */
> +    if (mask & PSW_MASK_64) {
> +        mask |= PSW_MASK_32;
> +    }
> +    s390_cpu_set_psw(env, mask, addr);
>   
>       for (i = 0; i < 16; i++) {
>           __get_user(env->aregs[i], &sc->regs.acrs[i]);
> 

LGTM

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

-- 
Thanks,

David / dhildenb

Re: [PATCH 1/5] target/s390x: Expose load_psw and get_psw_mask to cpu.h

[Alex Bennée]

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

> Rename to s390_cpu_set_psw and s390_cpu_get_psw_mask at the
> same time.  Adjust so that they compile for user-only.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée

Re: [PATCH 0/5] linux-user/s390x: Fix psw.mask handling in signals

[Christian Borntraeger]

On 15.06.21 05:07, Richard Henderson wrote:
> The PSW_MASK_CC component of psw.mask was not handled properly
> in the creation or restoration of signal frames.
> 


Maybe add a Reported-by: jonathan.albrecht@linux.vnet.ibm.com
in the right patches?

  
> Richard Henderson (5):
>    target/s390x: Expose load_psw and get_psw_mask to cpu.h
>    target/s390x: Do not modify cpu state in s390_cpu_get_psw_mask
>    target/s390x: Improve s390_cpu_dump_state vs cc_op
>    target/s390x: Use s390_cpu_{set_psw,get_psw_mask} in gdbstub
>    linux-user/s390x: Save and restore psw.mask properly
> 
>   target/s390x/cpu.h         |   3 ++
>   target/s390x/internal.h    |   5 --
>   linux-user/s390x/signal.c  |  37 ++++++++++++--
>   target/s390x/cc_helper.c   |   2 +-
>   target/s390x/excp_helper.c |  28 +++++-----
>   target/s390x/gdbstub.c     |  15 +-----
>   target/s390x/helper.c      | 101 ++++++++++++++++++++-----------------
>   target/s390x/sigp.c        |   3 +-
>   8 files changed, 110 insertions(+), 84 deletions(-)
> 

Re: [PATCH 0/5] linux-user/s390x: Fix psw.mask handling in signals

[Alex Bennée]

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

> The PSW_MASK_CC component of psw.mask was not handled properly
> in the creation or restoration of signal frames.

Still seeing issues running on s390x machine:

  05:00:29 [ajb@qemu01:~/l/q/b/debug] s390x/signal-fixes|… 38 + retry.py -n 100 -c -- ./qemu-s390x ./tests/tcg/s390x-linux-user/signals
  ...
  ...
  Results summary:
  0: 62 times (62.00%), avg time 2.253 (0.00 varience/0.00 deviation)
  -11: 38 times (38.00%), avg time 0.251 (0.00 varience/0.00 deviation)
  Ran command 100 times, 62 passes

I don't get much from the backtrace, maybe the atomic triggered the seg?

  #0  0x0000000001016244 in vfprintf ()
  [Current thread is 1 (Thread 0x4001001910 (LWP 27308))]
  (gdb) bt
  #0  0x0000000001016244 in vfprintf ()
  #1  0x000000000101d484 in printf ()
  #2  0x0000000001000b2e in background_thread_func (arg=0x10a3620) at /home/ajb/lsrc/qemu.git/tests/tcg/multiarch/signals.c:65
  #3  0x0000000001003150 in start_thread (arg=0x4001001910) at pthread_create.c:463
  #4  0x0000000001035b40 in thread_start ()
  (gdb) frame 2
  #2  0x0000000001000b2e in background_thread_func (arg=0x10a3620) at /home/ajb/lsrc/qemu.git/tests/tcg/multiarch/signals.c:65
  65          printf("thread%d: started\n", job->number);
  (gdb) info locals
  job = 0x10a3620
  (gdb) p job
  $1 = (ThreadJob *) 0x10a3620
  (gdb) p job->number
  $2 = 0
  (gdb) frame 0
  #0  0x0000000001016244 in vfprintf ()
  (gdb) x/5i $pc
  => 0x1016244 <vfprintf+1316>:   asi     224(%r11),1
     0x101624a <vfprintf+1322>:   cgijne  %r1,0,0x1017570 <vfprintf+6224>
     0x1016250 <vfprintf+1328>:   lg      %r1,336(%r11)
     0x1016256 <vfprintf+1334>:   lghi    %r3,37
     0x101625a <vfprintf+1338>:   aghik   %r6,%r1,1
  (gdb) p/x $r11
  $3 = 0x4001000708
  (gdb) p/x $r11 + 224
  $4 = 0x40010007e8
  (gdb) x/1g $4
  0x40010007e8:   0x0000000000000000
  (gdb)

However running on x86 backend everything seems to be fine.

  Results summary:
  0: 200 times (100.00%), avg time 2.255 (0.00 varience/0.00 deviation)
  Ran command 200 times, 200 passes

>
>
> r~
>
>
> Richard Henderson (5):
>   target/s390x: Expose load_psw and get_psw_mask to cpu.h
>   target/s390x: Do not modify cpu state in s390_cpu_get_psw_mask
>   target/s390x: Improve s390_cpu_dump_state vs cc_op
>   target/s390x: Use s390_cpu_{set_psw,get_psw_mask} in gdbstub
>   linux-user/s390x: Save and restore psw.mask properly
>
>  target/s390x/cpu.h         |   3 ++
>  target/s390x/internal.h    |   5 --
>  linux-user/s390x/signal.c  |  37 ++++++++++++--
>  target/s390x/cc_helper.c   |   2 +-
>  target/s390x/excp_helper.c |  28 +++++-----
>  target/s390x/gdbstub.c     |  15 +-----
>  target/s390x/helper.c      | 101 ++++++++++++++++++++-----------------
>  target/s390x/sigp.c        |   3 +-
>  8 files changed, 110 insertions(+), 84 deletions(-)

-- 
Alex Bennée

Re: [PATCH 0/5] linux-user/s390x: Fix psw.mask handling in signals

[Alex Bennée]

Alex Bennée <alex.bennee@linaro.org> writes:

> Richard Henderson <richard.henderson@linaro.org> writes:
>
>> The PSW_MASK_CC component of psw.mask was not handled properly
>> in the creation or restoration of signal frames.
>
> Still seeing issues running on s390x machine:
>
>   05:00:29 [ajb@qemu01:~/l/q/b/debug] s390x/signal-fixes|… 38 + retry.py -n 100 -c -- ./qemu-s390x ./tests/tcg/s390x-linux-user/signals
>   ...
>   ...
>   Results summary:
>   0: 62 times (62.00%), avg time 2.253 (0.00 varience/0.00 deviation)
>   -11: 38 times (38.00%), avg time 0.251 (0.00 varience/0.00 deviation)
>   Ran command 100 times, 62 passes
>
> I don't get much from the backtrace, maybe the atomic triggered the seg?
>
>   #0  0x0000000001016244 in vfprintf ()
>   [Current thread is 1 (Thread 0x4001001910 (LWP 27308))]
>   (gdb) bt
>   #0  0x0000000001016244 in vfprintf ()
>   #1  0x000000000101d484 in printf ()
>   #2  0x0000000001000b2e in background_thread_func (arg=0x10a3620) at /home/ajb/lsrc/qemu.git/tests/tcg/multiarch/signals.c:65
>   #3  0x0000000001003150 in start_thread (arg=0x4001001910) at pthread_create.c:463
>   #4  0x0000000001035b40 in thread_start ()
>   (gdb) frame 2
>   #2  0x0000000001000b2e in background_thread_func (arg=0x10a3620) at /home/ajb/lsrc/qemu.git/tests/tcg/multiarch/signals.c:65
>   65          printf("thread%d: started\n", job->number);
>   (gdb) info locals
>   job = 0x10a3620
>   (gdb) p job
>   $1 = (ThreadJob *) 0x10a3620
>   (gdb) p job->number
>   $2 = 0
>   (gdb) frame 0
>   #0  0x0000000001016244 in vfprintf ()
>   (gdb) x/5i $pc
>   => 0x1016244 <vfprintf+1316>:   asi     224(%r11),1
>      0x101624a <vfprintf+1322>:   cgijne  %r1,0,0x1017570 <vfprintf+6224>
>      0x1016250 <vfprintf+1328>:   lg      %r1,336(%r11)
>      0x1016256 <vfprintf+1334>:   lghi    %r3,37
>      0x101625a <vfprintf+1338>:   aghik   %r6,%r1,1
>   (gdb) p/x $r11
>   $3 = 0x4001000708
>   (gdb) p/x $r11 + 224
>   $4 = 0x40010007e8
>   (gdb) x/1g $4
>   0x40010007e8:   0x0000000000000000
>   (gdb)
>
> However running on x86 backend everything seems to be fine.
>
>   Results summary:
>   0: 200 times (100.00%), avg time 2.255 (0.00 varience/0.00 deviation)
>   Ran command 200 times, 200 passes

It's hard to desegregate the SEGVs we get during normal runs but a pass
followed by a fail shows:

  qemu: SIGSEGV pc=0x3fff473bb1e address=40007ff000 w=1 oldset=0x00000000
  qemu: SIGSEGV pc=0x3fff473bb1e address=40007ff000 w=1 oldset=0x00000000
  qemu: SIGSEGV pc=0x3fff473bb1e address=40007ff000 w=1 oldset=0x00000000
  qemu: SIGSEGV pc=0x3fff473bb1e address=40007ff000 w=1 oldset=0x00000000
  shutting down after: 2000 signals
  qemu: SIGSEGV pc=0x3fff484bd1e address=40007ff000 w=1 oldset=0x00000000
  qemu: SIGSEGV pc=0x3fff4848f1a address=40007ff000 w=1 oldset=0x00000000
  qemu: SIGSEGV pc=0x3fff47477f2 address=40007ff000 w=1 oldset=0x00000000
  [Thread 0x3fffdff1780 (LWP 32928) exited]
  [Inferior 1 (process 32928) exited normally]
  (gdb) r
  Starting program: /home/ajb/lsrc/qemu.git/builds/debug/qemu-s390x ./tests/tcg/s390x-linux-user/signals
  [Thread debugging using libthread_db enabled]
  Using host libthread_db library "/lib/s390x-linux-gnu/libthread_db.so.1".
  [New Thread 0x3fffceff910 (LWP 32964)]
  qemu: SIGSEGV pc=0x3fff4752f2a address=40007ff000 w=1 oldset=0x00000000
  qemu: SIGSEGV pc=0x3fff471fa1e address=40007ff000 w=1 oldset=0x00000000
  qemu: SIGSEGV pc=0x3fff475c49c address=40007ff000 w=1 oldset=0x00000000
  [New Thread 0x3fffdff0910 (LWP 32965)]
  qemu: SIGSEGV pc=0x3fff4703b18 address=40007ff000 w=1 oldset=0x00000000
  qemu: SIGSEGV pc=0x3fffd812efe address=4001000000 w=1 oldset=0x00000000
  qemu: SIGSEGV pc=0x3fff471081e address=40007ff000 w=1 oldset=0x00000000
  qemu: SIGSEGV pc=0x3fff4715ee6 address=40007ff000 w=1 oldset=0x00000000
  qemu: SIGSEGV pc=0x3fff471a02a address=40007ff000 w=1 oldset=0x00000000
  qemu: SIGSEGV pc=0x3fff472491e address=40007ff000 w=1 oldset=0x00000000
  qemu: SIGSEGV pc=0x3fff4725d1e address=4001000000 w=1 oldset=0x00000000
  qemu: SIGSEGV pc=0x3fff4730536 address=4001000000 w=0 oldset=0x00000000
  qemu: SIGSEGV pc=0x3fff473171e address=40007ff000 w=1 oldset=0x00000000
  [Thread 0x3fffdff0910 (LWP 32965) exited]
  [Thread 0x3fffceff910 (LWP 32964) exited]

  Program terminated with signal SIGSEGV, Segmentation fault.
  The program no longer exists.
  (gdb)

So something is going astray to 4001000000 which otherwise doesn't.

>
>>
>>
>> r~
>>
>>
>> Richard Henderson (5):
>>   target/s390x: Expose load_psw and get_psw_mask to cpu.h
>>   target/s390x: Do not modify cpu state in s390_cpu_get_psw_mask
>>   target/s390x: Improve s390_cpu_dump_state vs cc_op
>>   target/s390x: Use s390_cpu_{set_psw,get_psw_mask} in gdbstub
>>   linux-user/s390x: Save and restore psw.mask properly
>>
>>  target/s390x/cpu.h         |   3 ++
>>  target/s390x/internal.h    |   5 --
>>  linux-user/s390x/signal.c  |  37 ++++++++++++--
>>  target/s390x/cc_helper.c   |   2 +-
>>  target/s390x/excp_helper.c |  28 +++++-----
>>  target/s390x/gdbstub.c     |  15 +-----
>>  target/s390x/helper.c      | 101 ++++++++++++++++++++-----------------
>>  target/s390x/sigp.c        |   3 +-
>>  8 files changed, 110 insertions(+), 84 deletions(-)

-- 
Alex Bennée

Re: [PATCH 0/5] linux-user/s390x: Fix psw.mask handling in signals

[jonathan.albrecht]

On 2021-06-14 11:07 pm, Richard Henderson wrote:
> The PSW_MASK_CC component of psw.mask was not handled properly
> in the creation or restoration of signal frames.
> 

Thanks Richard! Peter and I tested this series against:
  * https://bugs.launchpad.net/qemu/+bug/1886793
  * https://bugs.launchpad.net/qemu/+bug/1893040
and they look fixed now.

Appreciate your time on this,

Jon

> 
> r~
> 
> 
> Richard Henderson (5):
>   target/s390x: Expose load_psw and get_psw_mask to cpu.h
>   target/s390x: Do not modify cpu state in s390_cpu_get_psw_mask
>   target/s390x: Improve s390_cpu_dump_state vs cc_op
>   target/s390x: Use s390_cpu_{set_psw,get_psw_mask} in gdbstub
>   linux-user/s390x: Save and restore psw.mask properly
> 
>  target/s390x/cpu.h         |   3 ++
>  target/s390x/internal.h    |   5 --
>  linux-user/s390x/signal.c  |  37 ++++++++++++--
>  target/s390x/cc_helper.c   |   2 +-
>  target/s390x/excp_helper.c |  28 +++++-----
>  target/s390x/gdbstub.c     |  15 +-----
>  target/s390x/helper.c      | 101 ++++++++++++++++++++-----------------
>  target/s390x/sigp.c        |   3 +-
>  8 files changed, 110 insertions(+), 84 deletions(-)

Re: [PATCH 0/5] linux-user/s390x: Fix psw.mask handling in signals

[Cornelia Huck]

On Tue, Jun 15 2021, Alex Bennée <alex.bennee@linaro.org> wrote:

> Richard Henderson <richard.henderson@linaro.org> writes:
>
>> The PSW_MASK_CC component of psw.mask was not handled properly
>> in the creation or restoration of signal frames.
>
> Still seeing issues running on s390x machine:

(...)

> However running on x86 backend everything seems to be fine.

(...)

So, in summary, this improves things, although there are some remaining
problems?

Asking because I'm looking to queue this.

Re: [PATCH 0/5] linux-user/s390x: Fix psw.mask handling in signals

[Richard Henderson]

On 6/16/21 3:38 AM, Cornelia Huck wrote:
> On Tue, Jun 15 2021, Alex Bennée <alex.bennee@linaro.org> wrote:
> 
>> Richard Henderson <richard.henderson@linaro.org> writes:
>>
>>> The PSW_MASK_CC component of psw.mask was not handled properly
>>> in the creation or restoration of signal frames.
>>
>> Still seeing issues running on s390x machine:
> 
> (...)
> 
>> However running on x86 backend everything seems to be fine.
> 
> (...)
> 
> So, in summary, this improves things, although there are some remaining
> problems?
> 
> Asking because I'm looking to queue this.

Alex is seeing something that I believe is unrelated.
And also, I have a patch set out for that as well.  :-)

r~

Re: [PATCH 0/5] linux-user/s390x: Fix psw.mask handling in signals

[Cornelia Huck]

On Wed, Jun 16 2021, Richard Henderson <richard.henderson@linaro.org> wrote:

> On 6/16/21 3:38 AM, Cornelia Huck wrote:
>> On Tue, Jun 15 2021, Alex Bennée <alex.bennee@linaro.org> wrote:
>> 
>>> Richard Henderson <richard.henderson@linaro.org> writes:
>>>
>>>> The PSW_MASK_CC component of psw.mask was not handled properly
>>>> in the creation or restoration of signal frames.
>>>
>>> Still seeing issues running on s390x machine:
>> 
>> (...)
>> 
>>> However running on x86 backend everything seems to be fine.
>> 
>> (...)
>> 
>> So, in summary, this improves things, although there are some remaining
>> problems?
>> 
>> Asking because I'm looking to queue this.
>
> Alex is seeing something that I believe is unrelated.
> And also, I have a patch set out for that as well.  :-)

Good to hear :)

Re: [PATCH 0/5] linux-user/s390x: Fix psw.mask handling in signals

[Cornelia Huck]

On Tue, Jun 15 2021, Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 15.06.21 05:07, Richard Henderson wrote:
>> The PSW_MASK_CC component of psw.mask was not handled properly
>> in the creation or restoration of signal frames.
>> 
>
>
> Maybe add a Reported-by: jonathan.albrecht@linux.vnet.ibm.com
> in the right patches?

Let me know to which ones, and I will add them.

Re: [PATCH 0/5] linux-user/s390x: Fix psw.mask handling in signals

[Cornelia Huck]

On Tue, Jun 15 2021, "jonathan.albrecht" <jonathan.albrecht@linux.vnet.ibm.com> wrote:

> On 2021-06-14 11:07 pm, Richard Henderson wrote:
>> The PSW_MASK_CC component of psw.mask was not handled properly
>> in the creation or restoration of signal frames.
>> 
>
> Thanks Richard! Peter and I tested this series against:
>   * https://bugs.launchpad.net/qemu/+bug/1886793
>   * https://bugs.launchpad.net/qemu/+bug/1893040
> and they look fixed now.

May I add some Tested-by: tags for this? (I don't know who Peter is.)

Re: [PATCH 0/5] linux-user/s390x: Fix psw.mask handling in signals

[jonathan.albrecht]

On 2021-06-16 11:01 am, Cornelia Huck wrote:
> On Tue, Jun 15 2021, "jonathan.albrecht"
> <jonathan.albrecht@linux.vnet.ibm.com> wrote:
> 
>> On 2021-06-14 11:07 pm, Richard Henderson wrote:
>>> The PSW_MASK_CC component of psw.mask was not handled properly
>>> in the creation or restoration of signal frames.
>>> 
>> 
>> Thanks Richard! Peter and I tested this series against:
>>   * https://bugs.launchpad.net/qemu/+bug/1886793
>>   * https://bugs.launchpad.net/qemu/+bug/1893040
>> and they look fixed now.
> 
> May I add some Tested-by: tags for this? (I don't know who Peter is.)

Yes, thanks. Peter is ruixin.bao@ibm.com

Re: [PATCH 0/5] linux-user/s390x: Fix psw.mask handling in signals

[Cornelia Huck]

On Mon, Jun 14 2021, Richard Henderson <richard.henderson@linaro.org> wrote:

> The PSW_MASK_CC component of psw.mask was not handled properly
> in the creation or restoration of signal frames.
>
>
> r~
>
>
> Richard Henderson (5):
>   target/s390x: Expose load_psw and get_psw_mask to cpu.h
>   target/s390x: Do not modify cpu state in s390_cpu_get_psw_mask
>   target/s390x: Improve s390_cpu_dump_state vs cc_op
>   target/s390x: Use s390_cpu_{set_psw,get_psw_mask} in gdbstub
>   linux-user/s390x: Save and restore psw.mask properly
>
>  target/s390x/cpu.h         |   3 ++
>  target/s390x/internal.h    |   5 --
>  linux-user/s390x/signal.c  |  37 ++++++++++++--
>  target/s390x/cc_helper.c   |   2 +-
>  target/s390x/excp_helper.c |  28 +++++-----
>  target/s390x/gdbstub.c     |  15 +-----
>  target/s390x/helper.c      | 101 ++++++++++++++++++++-----------------
>  target/s390x/sigp.c        |   3 +-
>  8 files changed, 110 insertions(+), 84 deletions(-)

Thanks, applied.