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

[Frank Chang <frank.chang@sifive.com>]

[-- Attachment #1: Type: text/plain, Size: 3193 bytes --]

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~
>

[-- Attachment #2: Type: text/html, Size: 5010 bytes --]