[PATCH] RISC-V: fcvt can set fflags, so set FS accordingly

Palmer Dabbelt posted 1 patch 4 years, 5 months ago
Test asan passed
Test checkpatch passed
Test FreeBSD passed
Test docker-mingw@fedora passed
Test docker-clang@ubuntu failed
Test docker-quick@centos7 passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20191009211541.9937-1-palmer@sifive.com
Maintainers: Alistair Francis <Alistair.Francis@wdc.com>, Sagar Karandikar <sagark@eecs.berkeley.edu>, Palmer Dabbelt <palmer@sifive.com>, Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
target/riscv/insn_trans/trans_rvd.inc.c | 2 ++
target/riscv/insn_trans/trans_rvf.inc.c | 4 ++++
2 files changed, 6 insertions(+)
[PATCH] RISC-V: fcvt can set fflags, so set FS accordingly
Posted by Palmer Dabbelt 4 years, 5 months ago
A user pinged me to say "my floating point heavy code works in user mode
but not system mode", which I'm guessing is the result of a lazy FP
save/restore issue as those still crop up from time to time as long tail
bugs.  I figured it was worth giving the FP stuff a look to see if
anything jumps out, and it turns out that there is a bug: converting
float to integer can set the invalid flag, which is supposed to mark FS
as dirty, but the emulation routine doesn't do so.

This patch unconditionally marks FS as dirty for fcvt instructions that
convert into X registers (fcvt into F registers already did so).  I
haven't actually tried to manifest a bug here, but as far as I can tell
the soft float stuff does set the invalid flag.

Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
---
 target/riscv/insn_trans/trans_rvd.inc.c | 2 ++
 target/riscv/insn_trans/trans_rvf.inc.c | 4 ++++
 2 files changed, 6 insertions(+)

diff --git a/target/riscv/insn_trans/trans_rvd.inc.c b/target/riscv/insn_trans/trans_rvd.inc.c
index 393fa0248c..8611e95486 100644
--- a/target/riscv/insn_trans/trans_rvd.inc.c
+++ b/target/riscv/insn_trans/trans_rvd.inc.c
@@ -371,6 +371,7 @@ static bool trans_fcvt_l_d(DisasContext *ctx, arg_fcvt_l_d *a)
     gen_helper_fcvt_l_d(t0, cpu_env, cpu_fpr[a->rs1]);
     gen_set_gpr(a->rd, t0);
     tcg_temp_free(t0);
+    mark_fs_dirty(ctx);
     return true;
 }
 
@@ -384,6 +385,7 @@ static bool trans_fcvt_lu_d(DisasContext *ctx, arg_fcvt_lu_d *a)
     gen_helper_fcvt_lu_d(t0, cpu_env, cpu_fpr[a->rs1]);
     gen_set_gpr(a->rd, t0);
     tcg_temp_free(t0);
+    mark_fs_dirty(ctx);
     return true;
 }
 
diff --git a/target/riscv/insn_trans/trans_rvf.inc.c b/target/riscv/insn_trans/trans_rvf.inc.c
index 172dbfa919..87a250a3f2 100644
--- a/target/riscv/insn_trans/trans_rvf.inc.c
+++ b/target/riscv/insn_trans/trans_rvf.inc.c
@@ -237,6 +237,7 @@ static bool trans_fcvt_w_s(DisasContext *ctx, arg_fcvt_w_s *a)
     gen_helper_fcvt_w_s(t0, cpu_env, cpu_fpr[a->rs1]);
     gen_set_gpr(a->rd, t0);
     tcg_temp_free(t0);
+    mark_fs_dirty(ctx);
 
     return true;
 }
@@ -251,6 +252,7 @@ static bool trans_fcvt_wu_s(DisasContext *ctx, arg_fcvt_wu_s *a)
     gen_helper_fcvt_wu_s(t0, cpu_env, cpu_fpr[a->rs1]);
     gen_set_gpr(a->rd, t0);
     tcg_temp_free(t0);
+    mark_fs_dirty(ctx);
 
     return true;
 }
@@ -389,6 +391,7 @@ static bool trans_fcvt_l_s(DisasContext *ctx, arg_fcvt_l_s *a)
     gen_helper_fcvt_l_s(t0, cpu_env, cpu_fpr[a->rs1]);
     gen_set_gpr(a->rd, t0);
     tcg_temp_free(t0);
+    mark_fs_dirty(ctx);
     return true;
 }
 
@@ -402,6 +405,7 @@ static bool trans_fcvt_lu_s(DisasContext *ctx, arg_fcvt_lu_s *a)
     gen_helper_fcvt_lu_s(t0, cpu_env, cpu_fpr[a->rs1]);
     gen_set_gpr(a->rd, t0);
     tcg_temp_free(t0);
+    mark_fs_dirty(ctx);
     return true;
 }
 
-- 
2.21.0


Re: [PATCH] RISC-V: fcvt can set fflags, so set FS accordingly
Posted by Richard Henderson 4 years, 5 months ago
On 10/9/19 5:15 PM, Palmer Dabbelt wrote:
> A user pinged me to say "my floating point heavy code works in user mode
> but not system mode", which I'm guessing is the result of a lazy FP
> save/restore issue as those still crop up from time to time as long tail
> bugs.  I figured it was worth giving the FP stuff a look to see if
> anything jumps out, and it turns out that there is a bug: converting
> float to integer can set the invalid flag, which is supposed to mark FS
> as dirty, but the emulation routine doesn't do so.
> 
> This patch unconditionally marks FS as dirty for fcvt instructions that
> convert into X registers (fcvt into F registers already did so).  I
> haven't actually tried to manifest a bug here, but as far as I can tell
> the soft float stuff does set the invalid flag.
> 
> Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
> ---
>  target/riscv/insn_trans/trans_rvd.inc.c | 2 ++
>  target/riscv/insn_trans/trans_rvf.inc.c | 4 ++++
>  2 files changed, 6 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~