1. Patchew
  2. QEMU
  3. View series

[PATCH RESEND v2] target/riscv: Set mstatus_hs.[SD|FS] bits if Clean and V=1 in mark_fs_dirty()

frank.chang@sifive.com posted 1 patch 2 years, 7 months ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20210917093153.4067812-1-frank.chang@sifive.com
Maintainers: Alistair Francis <alistair.francis@wdc.com>, Bin Meng <bin.meng@windriver.com>, Palmer Dabbelt <palmer@dabbelt.com>
There is a newer version of this series
target/riscv/cpu.h       |  4 ++++
target/riscv/translate.c | 24 +++++++++++++++---------
2 files changed, 19 insertions(+), 9 deletions(-)
[PATCH RESEND v2] target/riscv: Set mstatus_hs.[SD|FS] bits if Clean and V=1 in mark_fs_dirty()
Posted by frank.chang@sifive.com 2 years, 7 months ago 
From: Frank Chang <frank.chang@sifive.com>

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 <frank.chang@sifive.com>
Reviewed-by: Vincent Chen <vincent.chen@sifive.com>
Tested-by: Vincent Chen <vincent.chen@sifive.com>
---
 target/riscv/cpu.h       |  4 ++++
 target/riscv/translate.c | 24 +++++++++++++++---------
 2 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index e735e53e26c..bef7c551646 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -394,6 +394,7 @@ FIELD(TB_FLAGS, SEW, 5, 3)
 FIELD(TB_FLAGS, VILL, 8, 1)
 /* Is a Hypervisor instruction load/store allowed? */
 FIELD(TB_FLAGS, HLSX, 9, 1)
+FIELD(TB_FLAGS, MSTATUS_HS_FS, 10, 2)
 
 bool riscv_cpu_is_32bit(CPURISCVState *env);
 
@@ -450,6 +451,9 @@ static inline void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong *pc,
                 get_field(env->hstatus, HSTATUS_HU))) {
             flags = FIELD_DP32(flags, TB_FLAGS, HLSX, 1);
         }
+
+        flags = FIELD_DP32(flags, TB_FLAGS, MSTATUS_HS_FS,
+                           get_field(env->mstatus_hs, MSTATUS_FS));
     }
 #endif
 
diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index 74b33fa3c90..2b48db6fd02 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -58,6 +58,7 @@ typedef struct DisasContext {
     target_ulong misa;
     uint32_t opcode;
     uint32_t mstatus_fs;
+    uint32_t mstatus_hs_fs;
     uint32_t mem_idx;
     /* Remember the rounding mode encoded in the previous fp instruction,
        which we have already installed into env->fp_status.  Or -1 for
@@ -280,26 +281,30 @@ 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 && ctx->mstatus_hs_fs != MSTATUS_FS) {
+        /* Remember the stage change for the rest of the TB. */
+        ctx->mstatus_hs_fs = MSTATUS_FS;
+
+        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
@@ -533,6 +538,7 @@ static void riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
     ctx->frm = -1;  /* unknown rounding mode */
     ctx->ext_ifencei = cpu->cfg.ext_ifencei;
     ctx->vlen = cpu->cfg.vlen;
+    ctx->mstatus_hs_fs = FIELD_EX32(tb_flags, TB_FLAGS, MSTATUS_HS_FS);
     ctx->hlsx = FIELD_EX32(tb_flags, TB_FLAGS, HLSX);
     ctx->vill = FIELD_EX32(tb_flags, TB_FLAGS, VILL);
     ctx->sew = FIELD_EX32(tb_flags, TB_FLAGS, SEW);
-- 
2.25.1
Re: [PATCH RESEND v2] target/riscv: Set mstatus_hs.[SD|FS] bits if Clean and V=1 in mark_fs_dirty()
Posted by Richard Henderson 2 years, 7 months ago 
On 9/17/21 2:31 AM, frank.chang@sifive.com wrote:
> From: Frank Chang <frank.chang@sifive.com>
> 
> 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 <frank.chang@sifive.com>
> Reviewed-by: Vincent Chen <vincent.chen@sifive.com>
> Tested-by: Vincent Chen <vincent.chen@sifive.com>
> ---
>   target/riscv/cpu.h       |  4 ++++
>   target/riscv/translate.c | 24 +++++++++++++++---------
>   2 files changed, 19 insertions(+), 9 deletions(-)


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

>   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 && ctx->mstatus_hs_fs != MSTATUS_FS) {
> +        /* Remember the stage change for the rest of the TB. */
> +        ctx->mstatus_hs_fs = MSTATUS_FS;
> +
> +        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);

While it works, it would be nicer to keep these two cases as similar as possible.


r~
Re: [PATCH RESEND v2] target/riscv: Set mstatus_hs.[SD|FS] bits if Clean and V=1 in mark_fs_dirty()
Posted by Frank Chang 2 years, 7 months ago 
On Sun, Sep 19, 2021 at 2:46 AM Richard Henderson <
richard.henderson@linaro.org> wrote:

> On 9/17/21 2:31 AM, frank.chang@sifive.com wrote:
> > From: Frank Chang <frank.chang@sifive.com>
> >
> > 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 <frank.chang@sifive.com>
> > Reviewed-by: Vincent Chen <vincent.chen@sifive.com>
> > Tested-by: Vincent Chen <vincent.chen@sifive.com>
> > ---
> >   target/riscv/cpu.h       |  4 ++++
> >   target/riscv/translate.c | 24 +++++++++++++++---------
> >   2 files changed, 19 insertions(+), 9 deletions(-)
>
>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>
> >   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 && ctx->mstatus_hs_fs != MSTATUS_FS) {
> > +        /* Remember the stage change for the rest of the TB. */
> > +        ctx->mstatus_hs_fs = MSTATUS_FS;
> > +
> > +        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);
>
> While it works, it would be nicer to keep these two cases as similar as
> possible.
>
>
Hi, Richard, thanks for the review.

Do you mean it's better to change to code sequence to something like:

static void mark_fs_dirty(DisasContext *ctx)
{
    .....

    if (ctx->virt_enabled && ctx->mstatus_hs_fs != MSTATUS_FS) {
        /* Remember the stage change for the rest of the TB. */
        ctx->mstatus_hs_fs = MSTATUS_FS;
        .....
    }

    if (ctx->mstatus_fs != MSTATUS_FS) {
         /* Remember the state change for the rest of the TB.  */
        ctx->mstatus_fs = MSTATUS_FS;
        .....
     }
}

If so, I can update and send out the v3 patch.

Regards,
Frank Chang


>
> r~
>
Re: [PATCH RESEND v2] target/riscv: Set mstatus_hs.[SD|FS] bits if Clean and V=1 in mark_fs_dirty()
Posted by Richard Henderson 2 years, 7 months ago 
On 9/18/21 7:54 PM, Frank Chang wrote:
> Do you mean it's better to change to code sequence to something like:
> 
> static void mark_fs_dirty(DisasContext *ctx)
> {
>      .....
> 
>      if (ctx->virt_enabled && ctx->mstatus_hs_fs != MSTATUS_FS) {
>          /* Remember the stage change for the rest of the TB. */
>          ctx->mstatus_hs_fs = MSTATUS_FS;
>          .....
>      }
> 
>      if (ctx->mstatus_fs != MSTATUS_FS) {
>           /* Remember the state change for the rest of the TB.  */
>          ctx->mstatus_fs = MSTATUS_FS;
>          .....
>       }
> }
> 
> If so, I can update and send out the v3 patch.

Yes, something like that.

r~

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.