Re: [PATCH] target/riscv: Force to set mstatus_hs.[SD|FS] bits in mark_fs_dirty()

[Frank Chang]

On Tue, Sep 14, 2021 at 10:10 AM Richard Henderson <
richard.hender...@linaro.org> wrote:

> On 9/13/21 6:37 PM, frank.ch...@sifive.com wrote:
> > From: Frank Chang 
> >
> > When V=1, both vsstauts.FS and HS-level sstatus.FS are in effect.
> > Modifying the floating-point state when V=1 causes both fields to
> > be set to 3 (Dirty).
> >
> > However, it's possible that HS-level sstatus.FS is Clean and VS-level
> > vsstatus.FS is Dirty at the time mark_fs_dirty() is called when V=1.
> > We can't early return for this case because we still need to set
> > sstatus.FS to Dirty according to spec.
> >
> > Signed-off-by: Frank Chang 
> > Reviewed-by: Vincent Chen 
> > Tested-by: Vincent Chen 
> > ---
> >   target/riscv/translate.c | 19 ++-
> >   1 file changed, 10 insertions(+), 9 deletions(-)
> >
> > diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> > index e356fc6c46c..0096b098738 100644
> > --- a/target/riscv/translate.c
> > +++ b/target/riscv/translate.c
> > @@ -280,26 +280,27 @@ static void gen_jal(DisasContext *ctx, int rd,
> target_ulong imm)
> >   static void mark_fs_dirty(DisasContext *ctx)
> >   {
> >   TCGv tmp;
> > -target_ulong sd;
> > +target_ulong sd = is_32bit(ctx) ? MSTATUS32_SD : MSTATUS64_SD;
> > +
> > +if (ctx->virt_enabled) {
> > +tmp = tcg_temp_new();
> > +tcg_gen_ld_tl(tmp, cpu_env, offsetof(CPURISCVState,
> mstatus_hs));
> > +tcg_gen_ori_tl(tmp, tmp, MSTATUS_FS | sd);
> > +tcg_gen_st_tl(tmp, cpu_env, offsetof(CPURISCVState,
> mstatus_hs));
> > +tcg_temp_free(tmp);
> > +}
> >
> >   if (ctx->mstatus_fs == MSTATUS_FS) {
> >   return;
> >   }
>
> You should introduce a ctx->mstatus_hs field to track the code that you
> moved.  Otherwise
> you'll be setting this dirty bit for every fp insn.
>
>
Thanks, Richard, I was struggling with whether to introduce a new field
in DisasContext.
I will update my patch.

Regards,
Frank Chang


>
> r~
>

Re: [PATCH] target/riscv: Force to set mstatus_hs.[SD|FS] bits in mark_fs_dirty()

[Richard Henderson]

On 9/13/21 6:37 PM, frank.ch...@sifive.com wrote:

From: Frank Chang 

When V=1, both vsstauts.FS and HS-level sstatus.FS are in effect.
Modifying the floating-point state when V=1 causes both fields to
be set to 3 (Dirty).

However, it's possible that HS-level sstatus.FS is Clean and VS-level
vsstatus.FS is Dirty at the time mark_fs_dirty() is called when V=1.
We can't early return for this case because we still need to set
sstatus.FS to Dirty according to spec.

Signed-off-by: Frank Chang 
Reviewed-by: Vincent Chen 
Tested-by: Vincent Chen 
---
  target/riscv/translate.c | 19 ++-
  1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index e356fc6c46c..0096b098738 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -280,26 +280,27 @@ static void gen_jal(DisasContext *ctx, int rd, 
target_ulong imm)
  static void mark_fs_dirty(DisasContext *ctx)
  {
  TCGv tmp;
-target_ulong sd;
+target_ulong sd = is_32bit(ctx) ? MSTATUS32_SD : MSTATUS64_SD;
+
+if (ctx->virt_enabled) {
+tmp = tcg_temp_new();
+tcg_gen_ld_tl(tmp, cpu_env, offsetof(CPURISCVState, mstatus_hs));
+tcg_gen_ori_tl(tmp, tmp, MSTATUS_FS | sd);
+tcg_gen_st_tl(tmp, cpu_env, offsetof(CPURISCVState, mstatus_hs));
+tcg_temp_free(tmp);
+}
  
  if (ctx->mstatus_fs == MSTATUS_FS) {

  return;
  }


You should introduce a ctx->mstatus_hs field to track the code that you moved.  Otherwise 
you'll be setting this dirty bit for every fp insn.



r~

[PATCH] target/riscv: Force to set mstatus_hs.[SD|FS] bits in mark_fs_dirty()

[frank . chang]

From: Frank Chang 

When V=1, both vsstauts.FS and HS-level sstatus.FS are in effect.
Modifying the floating-point state when V=1 causes both fields to
be set to 3 (Dirty).

However, it's possible that HS-level sstatus.FS is Clean and VS-level
vsstatus.FS is Dirty at the time mark_fs_dirty() is called when V=1.
We can't early return for this case because we still need to set
sstatus.FS to Dirty according to spec.

Signed-off-by: Frank Chang 
Reviewed-by: Vincent Chen 
Tested-by: Vincent Chen 
---
 target/riscv/translate.c | 19 ++-
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index e356fc6c46c..0096b098738 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -280,26 +280,27 @@ static void gen_jal(DisasContext *ctx, int rd, 
target_ulong imm)
 static void mark_fs_dirty(DisasContext *ctx)
 {
 TCGv tmp;
-target_ulong sd;
+target_ulong sd = is_32bit(ctx) ? MSTATUS32_SD : MSTATUS64_SD;
+
+if (ctx->virt_enabled) {
+tmp = tcg_temp_new();
+tcg_gen_ld_tl(tmp, cpu_env, offsetof(CPURISCVState, mstatus_hs));
+tcg_gen_ori_tl(tmp, tmp, MSTATUS_FS | sd);
+tcg_gen_st_tl(tmp, cpu_env, offsetof(CPURISCVState, mstatus_hs));
+tcg_temp_free(tmp);
+}
 
 if (ctx->mstatus_fs == MSTATUS_FS) {
 return;
 }
+
 /* Remember the state change for the rest of the TB.  */
 ctx->mstatus_fs = MSTATUS_FS;
 
 tmp = tcg_temp_new();
-sd = is_32bit(ctx) ? MSTATUS32_SD : MSTATUS64_SD;
-
 tcg_gen_ld_tl(tmp, cpu_env, offsetof(CPURISCVState, mstatus));
 tcg_gen_ori_tl(tmp, tmp, MSTATUS_FS | sd);
 tcg_gen_st_tl(tmp, cpu_env, offsetof(CPURISCVState, mstatus));
-
-if (ctx->virt_enabled) {
-tcg_gen_ld_tl(tmp, cpu_env, offsetof(CPURISCVState, mstatus_hs));
-tcg_gen_ori_tl(tmp, tmp, MSTATUS_FS | sd);
-tcg_gen_st_tl(tmp, cpu_env, offsetof(CPURISCVState, mstatus_hs));
-}
 tcg_temp_free(tmp);
 }
 #else
-- 
2.25.1