On 5/25/26 17:22, Matt Turner wrote:
QEMU keeps the s390x floating-point control register (FPC) in env->fpc. The rounding mode bits [2:0] of FPC are reflected into the derived env->fpu_status via set_float_rounding_mode(); every architectural write to FPC goes through HELPER(sfpc) which keeps the two in sync.restore_sigregs() restored FPC with a direct assignment: __get_user(env->fpc, &sc->fpregs.fpc); This wrote env->fpc correctly but never updated env->fpu_status, so on sigreturn the interrupted code resumed with whatever rounding mode the signal handler last installed in fpu_status. Factor the two-step "write fpc + sync fpu_status" logic out of HELPER(sfpc) into cpu_s390x_load_fpc(), declare it in cpu.h, and call it from restore_sigregs() in place of the direct assignment. cpu_s390x_load_fpc() omits the specification-exception check that HELPER(sfpc) performs; raw signal frame restoration does not validate the saved state. Fixes: 2941e0fa05 ("linux-user/s390x: Save/restore fpc when handling a signal") Cc: [email protected] --- linux-user/s390x/signal.c | 6 +++++- target/s390x/cpu.h | 1 + target/s390x/tcg/fpu_helper.c | 6 ++++++ 3 files changed, 12 insertions(+), 1 deletion(-)
Just curious, how did you stumble upon this? It looks like a source of nasty intermittent bugs.
diff --git ./linux-user/s390x/signal.c ./linux-user/s390x/signal.c index 96d1c8d11c..28ad80bde4 100644 --- ./linux-user/s390x/signal.c +++ ./linux-user/s390x/signal.c @@ -332,7 +332,11 @@ static void restore_sigregs(CPUS390XState *env, target_sigregs *sc) for (i = 0; i < 16; i++) { __get_user(env->aregs[i], &sc->regs.acrs[i]); } - __get_user(env->fpc, &sc->fpregs.fpc); + { + uint32_t fpc; + __get_user(fpc, &sc->fpregs.fpc); + cpu_s390x_load_fpc(env, fpc); + } for (i = 0; i < 16; i++) { __get_user(*get_freg(env, i), &sc->fpregs.fprs[i]); } diff --git ./target/s390x/cpu.h ./target/s390x/cpu.h index 3acbe83f0f..f55b79ef8a 100644 --- ./target/s390x/cpu.h +++ ./target/s390x/cpu.h @@ -895,6 +895,7 @@ 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); +void cpu_s390x_load_fpc(CPUS390XState *env, uint32_t fpc);/* outside of target/s390x/ */S390CPU *s390_cpu_addr2state(uint16_t cpu_addr); diff --git ./target/s390x/tcg/fpu_helper.c ./target/s390x/tcg/fpu_helper.c index 6ca0b7162b..b1c61d73d7 100644 --- ./target/s390x/tcg/fpu_helper.c +++ ./target/s390x/tcg/fpu_helper.c @@ -1087,6 +1087,12 @@ static const int fpc_to_rnd[8] = { float_round_to_odd, };+void cpu_s390x_load_fpc(CPUS390XState *env, uint32_t fpc)+{ + env->fpc = fpc; + set_float_rounding_mode(fpc_to_rnd[fpc & 0x7], &env->fpu_status); +} + /* set fpc */ void HELPER(sfpc)(CPUS390XState *env, uint64_t fpc) {
Could you reuse cpu_s390x_load_fpc() in HELPER(sfpc)? The new sequence looks semantically identical to the one in the helper. Also, I think we do need to partially reuse the sanity check. Looking at kernel's fpu_lfpc_safe(), whenever the user writes a corrupt value into the signal frame and the kernel tries to restore it, an exception happens and 0 ends up being used instead.
