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

[Frank Chang]

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~