Re: [PATCH v3 2/4] target/riscv: Reuse tb->flags.FS

[Weiwei Li]

On 2023/4/29 16:54, Richard Henderson wrote:
> On 4/28/23 17:52, Mayuresh Chitale wrote:
> > When misa.F is 0 tb->flags.FS field is unused and can be used to save
> > the current state of smstateen0.FCSR check which is needed by the
> > floating point translation routines.
> >
> > Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
> > ---
> >   target/riscv/cpu_helper.c |  9 +++++++++
> >   target/riscv/translate.c  | 12 +++++++++++-
> >   2 files changed, 20 insertions(+), 1 deletion(-)
> >
> > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > index b68dcfe7b6..126ac221a0 100644
> > --- a/target/riscv/cpu_helper.c
> > +++ b/target/riscv/cpu_helper.c
> > @@ -119,6 +119,15 @@ void cpu_get_tb_cpu_state(CPURISCVState *env, 
> > target_ulong *pc,
> >           vs = MIN(vs, get_field(env->mstatus_hs, MSTATUS_VS));
> >       }
> >   +    /*
> > +     * If misa.F is 0 then the FS field of the tb->flags can be used 
> > to pass
> > +     * the current state of the smstateen.FCSR bit which must be 
> > checked for
> > +     * in the floating point translation routines.
> > +     */
> > +    if (!riscv_has_ext(env, RVF)) {
> > +        fs = (smstateen_acc_ok(env, 0, SMSTATEEN0_FCSR) == 
> > RISCV_EXCP_NONE);
> > +    }
>
> You have misunderstood my suggestion:
>
>     /* With Zfinx, floating point is enabled/disabled by Smstateen. */
>     if (!riscv_has_ext(env, RVF)) {
>         fs = (smstateen_acc_ok(env, 0, SMSTATEEN0_FCSR)
>               ? EXT_STATUS_DIRTY : EXT_STATUS_DISABLED);
>     }
>
> > +    bool smstateen_fcsr_ok;
>
> Not needed.
>
> > -    ctx->mstatus_fs = FIELD_EX32(tb_flags, TB_FLAGS, FS);
> > +    if (has_ext(ctx, RVF)) {
> > +        ctx->mstatus_fs = FIELD_EX32(tb_flags, TB_FLAGS, FS);
> > +    } else {
> > +        ctx->mstatus_fs = 0;
> > +    }
>
> Not needed.
>
> In patch 3, which should be merged with this, there are no changes to 
> REQUIRE_ZFINX_OR_F, no additional smstateen_fcsr_check, and 
> REQUIRE_FPU reduces to
>
> #define REQUIRE_FPU do {                          \
>     if (ctx->mstatus_fs == EXT_STATUS_DISABLED) { \
>         return false;                             \
>     }                                             \
> } while (0)
>
> This makes the DisasContext version of fs be the single gate for 
> floating point.
> No extra checks required.

Yeah. It's better to merge with REQUIRE_FPU.

However,  virtual instruction exception will be triggered in VS/VU mode 
if smstateen check fails, which is different from normal REQUIRE_FPU.

So, we may need to modify REQUIRE_FPU to distinguish them:

#define REQUIRE_FPU do {                          \
    if (ctx->mstatus_fs == EXT_STATUS_DISABLED) { \
          ctx->virt_inst_excp = ctx->virt_enabled && 
ctx->cfg_ptr->ext_zfinx;  \
          return false;                             \
    }                                             \
} while (0)

Regards,

Weiwei Li

> r~