| Message ID | 20211124172523.3598396-2-lucas.araujo@eldorado.org.br |
|---|---|
| State | New |
| Headers | show |
| Series | Fix mtfsf, mtfsfi and mtfsb1 bug | expand |
On Wed, 24 Nov 2021, Lucas Mateus Castro (alqotel) wrote: > mtfsf, mtfsfi and mtfsb1 instructions call helper_float_check_status > after updating the value of FPSCR, but helper_float_check_status > checks fp_status and fp_status isn't updated based on FPSCR and > since the value of fp_status is reset earlier in the instruction, > it's always 0. > > Because of this helper_float_check_status would change the FI bit to 0 > as this bit checks if the last operation was inexact and > float_flag_inexact is always 0. > > These instructions also don't throw exceptions correctly since > helper_float_check_status throw exceptions based on fp_status. > > This commit created a new helper, helper_fpscr_check_status that checks > FPSCR value instead of fp_status and checks for a larger variety of > exceptions than do_float_check_status. > > Since fp_status isn't used, gen_reset_fpstatus() was removed. > > The hardware used to compare QEMU's behavior to was a Power9. > > Signed-off-by: Lucas Mateus Castro (alqotel) <lucas.araujo@eldorado.org.br> > --- > target/ppc/fpu_helper.c | 48 ++++++++++++++++++++++++++++++ > target/ppc/helper.h | 1 + > target/ppc/translate/fp-impl.c.inc | 9 ++---- > 3 files changed, 52 insertions(+), 6 deletions(-) > > diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c > index c4896cecc8..bb72715827 100644 > --- a/target/ppc/fpu_helper.c > +++ b/target/ppc/fpu_helper.c > @@ -414,6 +414,54 @@ void helper_store_fpscr(CPUPPCState *env, uint64_t val, uint32_t nibbles) > ppc_store_fpscr(env, val); > } > > +void helper_fpscr_check_status(CPUPPCState *env) > +{ > + CPUState *cs = env_cpu(env); > + target_ulong fpscr = env->fpscr; > + int error = 0; > + > + if ((fpscr & FP_OX) && (fpscr & FP_OE)) { > + error = POWERPC_EXCP_FP_OX; > + } else if ((fpscr & FP_UX) && (fpscr & FP_UE)) { > + error = POWERPC_EXCP_FP_UX; > + } else if ((fpscr & FP_XX) && (fpscr & FP_XE)) { > + error = POWERPC_EXCP_FP_XX; > + } else if ((fpscr & FP_ZX) && (fpscr & FP_ZE)) { I wonder if these tests could be simplified by combining the masks if you want to test for both bits set so e.g. fpscr & (FP_ZX | FP_ZE) should be the same, shouldn't it? Regards, BALATON Zoltan > + error = POWERPC_EXCP_FP_ZX; > + } else if (fpscr & FP_VE) { > + if (fpscr & FP_VXSOFT) { > + error = POWERPC_EXCP_FP_VXSOFT; > + } else if (fpscr & FP_VXSNAN) { > + error = POWERPC_EXCP_FP_VXSNAN; > + } else if (fpscr & FP_VXISI) { > + error = POWERPC_EXCP_FP_VXISI; > + } else if (fpscr & FP_VXIDI) { > + error = POWERPC_EXCP_FP_VXIDI; > + } else if (fpscr & FP_VXZDZ) { > + error = POWERPC_EXCP_FP_VXZDZ; > + } else if (fpscr & FP_VXIMZ) { > + error = POWERPC_EXCP_FP_VXIMZ; > + } else if (fpscr & FP_VXVC) { > + error = POWERPC_EXCP_FP_VXVC; > + } else if (fpscr & FP_VXSQRT) { > + error = POWERPC_EXCP_FP_VXSQRT; > + } else if (fpscr & FP_VXCVI) { > + error = POWERPC_EXCP_FP_VXCVI; > + } else { > + return; > + } > + } else { > + return; > + } > + cs->exception_index = POWERPC_EXCP_PROGRAM; > + env->error_code = error | POWERPC_EXCP_FP; > + /* Deferred floating-point exception after target FPSCR update */ > + if (fp_exceptions_enabled(env)) { > + raise_exception_err_ra(env, cs->exception_index, > + env->error_code, GETPC()); > + } > +} > + > static void do_float_check_status(CPUPPCState *env, uintptr_t raddr) > { > CPUState *cs = env_cpu(env); > diff --git a/target/ppc/helper.h b/target/ppc/helper.h > index 627811cefc..632a81c676 100644 > --- a/target/ppc/helper.h > +++ b/target/ppc/helper.h > @@ -63,6 +63,7 @@ DEF_HELPER_FLAGS_1(cntlzw32, TCG_CALL_NO_RWG_SE, i32, i32) > DEF_HELPER_FLAGS_2(brinc, TCG_CALL_NO_RWG_SE, tl, tl, tl) > > DEF_HELPER_1(float_check_status, void, env) > +DEF_HELPER_1(fpscr_check_status, void, env) > DEF_HELPER_1(reset_fpstatus, void, env) > DEF_HELPER_2(compute_fprf_float64, void, env, i64) > DEF_HELPER_3(store_fpscr, void, env, i64, i32) > diff --git a/target/ppc/translate/fp-impl.c.inc b/target/ppc/translate/fp-impl.c.inc > index d1dbb1b96b..ca195fd9d2 100644 > --- a/target/ppc/translate/fp-impl.c.inc > +++ b/target/ppc/translate/fp-impl.c.inc > @@ -769,7 +769,6 @@ static void gen_mtfsb1(DisasContext *ctx) > return; > } > crb = 31 - crbD(ctx->opcode); > - gen_reset_fpstatus(); > /* XXX: we pretend we can only do IEEE floating-point computations */ > if (likely(crb != FPSCR_FEX && crb != FPSCR_VX && crb != FPSCR_NI)) { > TCGv_i32 t0; > @@ -782,7 +781,7 @@ static void gen_mtfsb1(DisasContext *ctx) > tcg_gen_shri_i32(cpu_crf[1], cpu_crf[1], FPSCR_OX); > } > /* We can raise a deferred exception */ > - gen_helper_float_check_status(cpu_env); > + gen_helper_fpscr_check_status(cpu_env); > } > > /* mtfsf */ > @@ -803,7 +802,6 @@ static void gen_mtfsf(DisasContext *ctx) > gen_inval_exception(ctx, POWERPC_EXCP_INVAL_INVAL); > return; > } > - gen_reset_fpstatus(); > if (l) { > t0 = tcg_const_i32((ctx->insns_flags2 & PPC2_ISA205) ? 0xffff : 0xff); > } else { > @@ -818,7 +816,7 @@ static void gen_mtfsf(DisasContext *ctx) > tcg_gen_shri_i32(cpu_crf[1], cpu_crf[1], FPSCR_OX); > } > /* We can raise a deferred exception */ > - gen_helper_float_check_status(cpu_env); > + gen_helper_fpscr_check_status(cpu_env); > tcg_temp_free_i64(t1); > } > > @@ -840,7 +838,6 @@ static void gen_mtfsfi(DisasContext *ctx) > return; > } > sh = (8 * w) + 7 - bf; > - gen_reset_fpstatus(); > t0 = tcg_const_i64(((uint64_t)FPIMM(ctx->opcode)) << (4 * sh)); > t1 = tcg_const_i32(1 << sh); > gen_helper_store_fpscr(cpu_env, t0, t1); > @@ -851,7 +848,7 @@ static void gen_mtfsfi(DisasContext *ctx) > tcg_gen_shri_i32(cpu_crf[1], cpu_crf[1], FPSCR_OX); > } > /* We can raise a deferred exception */ > - gen_helper_float_check_status(cpu_env); > + gen_helper_fpscr_check_status(cpu_env); > } > > static void gen_qemu_ld32fs(DisasContext *ctx, TCGv_i64 dest, TCGv addr) >
On Thu, Nov 25, 2021 at 01:49:46AM +0100, BALATON Zoltan wrote: > On Wed, 24 Nov 2021, Lucas Mateus Castro (alqotel) wrote: > > mtfsf, mtfsfi and mtfsb1 instructions call helper_float_check_status > > after updating the value of FPSCR, but helper_float_check_status > > checks fp_status and fp_status isn't updated based on FPSCR and > > since the value of fp_status is reset earlier in the instruction, > > it's always 0. > > > > Because of this helper_float_check_status would change the FI bit to 0 > > as this bit checks if the last operation was inexact and > > float_flag_inexact is always 0. > > > > These instructions also don't throw exceptions correctly since > > helper_float_check_status throw exceptions based on fp_status. > > > > This commit created a new helper, helper_fpscr_check_status that checks > > FPSCR value instead of fp_status and checks for a larger variety of > > exceptions than do_float_check_status. > > > > Since fp_status isn't used, gen_reset_fpstatus() was removed. > > > > The hardware used to compare QEMU's behavior to was a Power9. > > > > Signed-off-by: Lucas Mateus Castro (alqotel) <lucas.araujo@eldorado.org.br> > > --- > > target/ppc/fpu_helper.c | 48 ++++++++++++++++++++++++++++++ > > target/ppc/helper.h | 1 + > > target/ppc/translate/fp-impl.c.inc | 9 ++---- > > 3 files changed, 52 insertions(+), 6 deletions(-) > > > > diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c > > index c4896cecc8..bb72715827 100644 > > --- a/target/ppc/fpu_helper.c > > +++ b/target/ppc/fpu_helper.c > > @@ -414,6 +414,54 @@ void helper_store_fpscr(CPUPPCState *env, uint64_t val, uint32_t nibbles) > > ppc_store_fpscr(env, val); > > } > > > > +void helper_fpscr_check_status(CPUPPCState *env) > > +{ > > + CPUState *cs = env_cpu(env); > > + target_ulong fpscr = env->fpscr; > > + int error = 0; > > + > > + if ((fpscr & FP_OX) && (fpscr & FP_OE)) { > > + error = POWERPC_EXCP_FP_OX; > > + } else if ((fpscr & FP_UX) && (fpscr & FP_UE)) { > > + error = POWERPC_EXCP_FP_UX; > > + } else if ((fpscr & FP_XX) && (fpscr & FP_XE)) { > > + error = POWERPC_EXCP_FP_XX; > > + } else if ((fpscr & FP_ZX) && (fpscr & FP_ZE)) { > > I wonder if these tests could be simplified by combining the masks if you > want to test for both bits set so e.g. fpscr & (FP_ZX | FP_ZE) should be the > same, shouldn't it? No, it's not. In fact your version is equivalent as a boolean to ((fpscr & FP_ZX) || (fpscr & FP_ZE))
On 11/24/21 6:25 PM, Lucas Mateus Castro (alqotel) wrote: > mtfsf, mtfsfi and mtfsb1 instructions call helper_float_check_status > after updating the value of FPSCR, but helper_float_check_status > checks fp_status and fp_status isn't updated based on FPSCR and > since the value of fp_status is reset earlier in the instruction, > it's always 0. > > Because of this helper_float_check_status would change the FI bit to 0 > as this bit checks if the last operation was inexact and > float_flag_inexact is always 0. > > These instructions also don't throw exceptions correctly since > helper_float_check_status throw exceptions based on fp_status. > > This commit created a new helper, helper_fpscr_check_status that checks > FPSCR value instead of fp_status and checks for a larger variety of > exceptions than do_float_check_status. > > Since fp_status isn't used, gen_reset_fpstatus() was removed. > > The hardware used to compare QEMU's behavior to was a Power9. > > Signed-off-by: Lucas Mateus Castro (alqotel) <lucas.araujo@eldorado.org.br> > --- > target/ppc/fpu_helper.c | 48 ++++++++++++++++++++++++++++++ > target/ppc/helper.h | 1 + > target/ppc/translate/fp-impl.c.inc | 9 ++---- > 3 files changed, 52 insertions(+), 6 deletions(-) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On Thu, 25 Nov 2021, David Gibson wrote: > On Thu, Nov 25, 2021 at 01:49:46AM +0100, BALATON Zoltan wrote: >> On Wed, 24 Nov 2021, Lucas Mateus Castro (alqotel) wrote: >>> mtfsf, mtfsfi and mtfsb1 instructions call helper_float_check_status >>> after updating the value of FPSCR, but helper_float_check_status >>> checks fp_status and fp_status isn't updated based on FPSCR and >>> since the value of fp_status is reset earlier in the instruction, >>> it's always 0. >>> >>> Because of this helper_float_check_status would change the FI bit to 0 >>> as this bit checks if the last operation was inexact and >>> float_flag_inexact is always 0. >>> >>> These instructions also don't throw exceptions correctly since >>> helper_float_check_status throw exceptions based on fp_status. >>> >>> This commit created a new helper, helper_fpscr_check_status that checks >>> FPSCR value instead of fp_status and checks for a larger variety of >>> exceptions than do_float_check_status. >>> >>> Since fp_status isn't used, gen_reset_fpstatus() was removed. >>> >>> The hardware used to compare QEMU's behavior to was a Power9. >>> >>> Signed-off-by: Lucas Mateus Castro (alqotel) <lucas.araujo@eldorado.org.br> >>> --- >>> target/ppc/fpu_helper.c | 48 ++++++++++++++++++++++++++++++ >>> target/ppc/helper.h | 1 + >>> target/ppc/translate/fp-impl.c.inc | 9 ++---- >>> 3 files changed, 52 insertions(+), 6 deletions(-) >>> >>> diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c >>> index c4896cecc8..bb72715827 100644 >>> --- a/target/ppc/fpu_helper.c >>> +++ b/target/ppc/fpu_helper.c >>> @@ -414,6 +414,54 @@ void helper_store_fpscr(CPUPPCState *env, uint64_t val, uint32_t nibbles) >>> ppc_store_fpscr(env, val); >>> } >>> >>> +void helper_fpscr_check_status(CPUPPCState *env) >>> +{ >>> + CPUState *cs = env_cpu(env); >>> + target_ulong fpscr = env->fpscr; >>> + int error = 0; >>> + >>> + if ((fpscr & FP_OX) && (fpscr & FP_OE)) { >>> + error = POWERPC_EXCP_FP_OX; >>> + } else if ((fpscr & FP_UX) && (fpscr & FP_UE)) { >>> + error = POWERPC_EXCP_FP_UX; >>> + } else if ((fpscr & FP_XX) && (fpscr & FP_XE)) { >>> + error = POWERPC_EXCP_FP_XX; >>> + } else if ((fpscr & FP_ZX) && (fpscr & FP_ZE)) { >> >> I wonder if these tests could be simplified by combining the masks if you >> want to test for both bits set so e.g. fpscr & (FP_ZX | FP_ZE) should be the >> same, shouldn't it? > > No, it's not. In fact your version is equivalent as a boolean to > ((fpscr & FP_ZX) || (fpscr & FP_ZE)) Indeed, it was too late when I wrote. I was really thinking (fprscr & (FP_ZX | FP_ZE)) == (FP_ZX | FP_ZE) but that's not simpler so that answers my question. Regards, BALATON Zoltan
diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c index c4896cecc8..bb72715827 100644 --- a/target/ppc/fpu_helper.c +++ b/target/ppc/fpu_helper.c @@ -414,6 +414,54 @@ void helper_store_fpscr(CPUPPCState *env, uint64_t val, uint32_t nibbles) ppc_store_fpscr(env, val); } +void helper_fpscr_check_status(CPUPPCState *env) +{ + CPUState *cs = env_cpu(env); + target_ulong fpscr = env->fpscr; + int error = 0; + + if ((fpscr & FP_OX) && (fpscr & FP_OE)) { + error = POWERPC_EXCP_FP_OX; + } else if ((fpscr & FP_UX) && (fpscr & FP_UE)) { + error = POWERPC_EXCP_FP_UX; + } else if ((fpscr & FP_XX) && (fpscr & FP_XE)) { + error = POWERPC_EXCP_FP_XX; + } else if ((fpscr & FP_ZX) && (fpscr & FP_ZE)) { + error = POWERPC_EXCP_FP_ZX; + } else if (fpscr & FP_VE) { + if (fpscr & FP_VXSOFT) { + error = POWERPC_EXCP_FP_VXSOFT; + } else if (fpscr & FP_VXSNAN) { + error = POWERPC_EXCP_FP_VXSNAN; + } else if (fpscr & FP_VXISI) { + error = POWERPC_EXCP_FP_VXISI; + } else if (fpscr & FP_VXIDI) { + error = POWERPC_EXCP_FP_VXIDI; + } else if (fpscr & FP_VXZDZ) { + error = POWERPC_EXCP_FP_VXZDZ; + } else if (fpscr & FP_VXIMZ) { + error = POWERPC_EXCP_FP_VXIMZ; + } else if (fpscr & FP_VXVC) { + error = POWERPC_EXCP_FP_VXVC; + } else if (fpscr & FP_VXSQRT) { + error = POWERPC_EXCP_FP_VXSQRT; + } else if (fpscr & FP_VXCVI) { + error = POWERPC_EXCP_FP_VXCVI; + } else { + return; + } + } else { + return; + } + cs->exception_index = POWERPC_EXCP_PROGRAM; + env->error_code = error | POWERPC_EXCP_FP; + /* Deferred floating-point exception after target FPSCR update */ + if (fp_exceptions_enabled(env)) { + raise_exception_err_ra(env, cs->exception_index, + env->error_code, GETPC()); + } +} + static void do_float_check_status(CPUPPCState *env, uintptr_t raddr) { CPUState *cs = env_cpu(env); diff --git a/target/ppc/helper.h b/target/ppc/helper.h index 627811cefc..632a81c676 100644 --- a/target/ppc/helper.h +++ b/target/ppc/helper.h @@ -63,6 +63,7 @@ DEF_HELPER_FLAGS_1(cntlzw32, TCG_CALL_NO_RWG_SE, i32, i32) DEF_HELPER_FLAGS_2(brinc, TCG_CALL_NO_RWG_SE, tl, tl, tl) DEF_HELPER_1(float_check_status, void, env) +DEF_HELPER_1(fpscr_check_status, void, env) DEF_HELPER_1(reset_fpstatus, void, env) DEF_HELPER_2(compute_fprf_float64, void, env, i64) DEF_HELPER_3(store_fpscr, void, env, i64, i32) diff --git a/target/ppc/translate/fp-impl.c.inc b/target/ppc/translate/fp-impl.c.inc index d1dbb1b96b..ca195fd9d2 100644 --- a/target/ppc/translate/fp-impl.c.inc +++ b/target/ppc/translate/fp-impl.c.inc @@ -769,7 +769,6 @@ static void gen_mtfsb1(DisasContext *ctx) return; } crb = 31 - crbD(ctx->opcode); - gen_reset_fpstatus(); /* XXX: we pretend we can only do IEEE floating-point computations */ if (likely(crb != FPSCR_FEX && crb != FPSCR_VX && crb != FPSCR_NI)) { TCGv_i32 t0; @@ -782,7 +781,7 @@ static void gen_mtfsb1(DisasContext *ctx) tcg_gen_shri_i32(cpu_crf[1], cpu_crf[1], FPSCR_OX); } /* We can raise a deferred exception */ - gen_helper_float_check_status(cpu_env); + gen_helper_fpscr_check_status(cpu_env); } /* mtfsf */ @@ -803,7 +802,6 @@ static void gen_mtfsf(DisasContext *ctx) gen_inval_exception(ctx, POWERPC_EXCP_INVAL_INVAL); return; } - gen_reset_fpstatus(); if (l) { t0 = tcg_const_i32((ctx->insns_flags2 & PPC2_ISA205) ? 0xffff : 0xff); } else { @@ -818,7 +816,7 @@ static void gen_mtfsf(DisasContext *ctx) tcg_gen_shri_i32(cpu_crf[1], cpu_crf[1], FPSCR_OX); } /* We can raise a deferred exception */ - gen_helper_float_check_status(cpu_env); + gen_helper_fpscr_check_status(cpu_env); tcg_temp_free_i64(t1); } @@ -840,7 +838,6 @@ static void gen_mtfsfi(DisasContext *ctx) return; } sh = (8 * w) + 7 - bf; - gen_reset_fpstatus(); t0 = tcg_const_i64(((uint64_t)FPIMM(ctx->opcode)) << (4 * sh)); t1 = tcg_const_i32(1 << sh); gen_helper_store_fpscr(cpu_env, t0, t1); @@ -851,7 +848,7 @@ static void gen_mtfsfi(DisasContext *ctx) tcg_gen_shri_i32(cpu_crf[1], cpu_crf[1], FPSCR_OX); } /* We can raise a deferred exception */ - gen_helper_float_check_status(cpu_env); + gen_helper_fpscr_check_status(cpu_env); } static void gen_qemu_ld32fs(DisasContext *ctx, TCGv_i64 dest, TCGv addr)
mtfsf, mtfsfi and mtfsb1 instructions call helper_float_check_status after updating the value of FPSCR, but helper_float_check_status checks fp_status and fp_status isn't updated based on FPSCR and since the value of fp_status is reset earlier in the instruction, it's always 0. Because of this helper_float_check_status would change the FI bit to 0 as this bit checks if the last operation was inexact and float_flag_inexact is always 0. These instructions also don't throw exceptions correctly since helper_float_check_status throw exceptions based on fp_status. This commit created a new helper, helper_fpscr_check_status that checks FPSCR value instead of fp_status and checks for a larger variety of exceptions than do_float_check_status. Since fp_status isn't used, gen_reset_fpstatus() was removed. The hardware used to compare QEMU's behavior to was a Power9. Signed-off-by: Lucas Mateus Castro (alqotel) <lucas.araujo@eldorado.org.br> --- target/ppc/fpu_helper.c | 48 ++++++++++++++++++++++++++++++ target/ppc/helper.h | 1 + target/ppc/translate/fp-impl.c.inc | 9 ++---- 3 files changed, 52 insertions(+), 6 deletions(-)