On Sun, Sep 19, 2021 at 2:46 AM Richard Henderson <
richard.hender...@linaro.org> wrote:
> On 9/17/21 2:31 AM, frank.ch...@sifive.com wrote:
> > From: Frank Chang <frank.ch...@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.ch...@sifive.com>
> > Reviewed-by: Vincent Chen <vincent.c...@sifive.com>
> > Tested-by: Vincent Chen <vincent.c...@sifive.com>
> > ---
> > target/riscv/cpu.h | 4 ++++
> > target/riscv/translate.c | 24 +++++++++++++++---------
> > 2 files changed, 19 insertions(+), 9 deletions(-)
>
>
> Reviewed-by: Richard Henderson <richard.hender...@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~
>
