Re: [PATCH 3/6] target/riscv: Correct SXL return value for RV32 in RV64 QEMU

[LIU Zhiwei]

On 2024/7/1 23:10, Philippe Mathieu-Daudé wrote:

> Hi
>       Tiancheng, Zhiwei,
>       
>       
>       On 1/7/24 05:37, LIU Zhiwei wrote:
>       
> > From: TANG Tiancheng
> >         <tangtiancheng.ttc@alibaba-inc.com>
> >         
> >         
> >         Ensure that riscv_cpu_sxl returns MXL_RV32 when runningRV32 in
> >         an
> >         
> >         RV64 QEMU.
> >         
> >         
> >         Signed-off-by: TANG Tiancheng
> >         <tangtiancheng.ttc@alibaba-inc.com>
> >         
> >         Fixes: 05e6ca5e156 ("target/riscv: Ignore reserved bits in PTE
> >         for RV64")
> >         
> >         Reviewed-by: Liu Zhiwei <zhiwei_liu@linux.alibaba.com>
> >         
> >         ---
> >         
> >           target/riscv/cpu.h | 5 ++++-
> >         
> >           1 file changed, 4 insertions(+), 1 deletion(-)
> >         
> >         
> >         diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> >         
> >         index 6fe0d712b4..36a712044a 100644
> >         
> >         --- a/target/riscv/cpu.h
> >         
> >         +++ b/target/riscv/cpu.h
> >         
> >         @@ -668,8 +668,11 @@ static inline RISCVMXL
> >         riscv_cpu_sxl(CPURISCVState *env)
> >         
> >           #ifdef CONFIG_USER_ONLY
> >         
> >               return env->misa_mxl;
> >         
> >           #else
> >         
> >         -    return get_field(env->mstatus, MSTATUS64_SXL);
> >         
> >         +    if (env->misa_mxl != MXL_RV32) {
> >         
> >         +        return get_field(env->mstatus, MSTATUS64_SXL);
> >         
> >         +    }
> >         
> >           #endif
> >         
> >         +    return MXL_RV32;
> >         
> >       
>       
>       Can we simplify the previous TARGET_RISCV32 ifdef'ry?
>       
>

I think you mean the TARGET_RISCV32 macro here:

#ifdef TARGET_RISCV32
#define riscv_cpu_sxl(env)  ((void)(env), MXL_RV32)
#else
static inline RISCVMXL riscv_cpu_sxl(CPURISCVState *env)
{
#ifdef CONFIG_USER_ONLY
    return env->misa_mxl;
#else
    return get_field(env->mstatus, MSTATUS64_SXL);
#endif
}
#endif

I think it is better to keep it here for better performance(as a constant).

If you mean whether we  can simplify all the ifdef TARGET_RISCV32 in target/riscv or hw/riscv, IMHO, it depends. I git grep the TARGET_RISCV32 from target/riscv:

target/riscv/cpu-param.h:#elif defined(TARGET_RISCV32)
target/riscv/cpu.c:#ifdef TARGET_RISCV32
target/riscv/cpu.c:#if defined(TARGET_RISCV32)
target/riscv/cpu.h:#if defined(TARGET_RISCV32)
target/riscv/cpu.h:#ifdef TARGET_RISCV32
target/riscv/cpu.h:#if defined(TARGET_RISCV32)
target/riscv/cpu.h:#if defined(TARGET_RISCV32)
target/riscv/cpu.h:#ifdef TARGET_RISCV32
target/riscv/insn_trans/trans_rvzacas.c.inc:#ifdef TARGET_RISCV32
target/riscv/kvm/kvm-cpu.c:#if defined(TARGET_RISCV32)
target/riscv/translate.c:#ifdef TARGET_RISCV32
target/riscv/translate.c:#ifdef TARGET_RISCV32
target/riscv/translate.c:#ifdef TARGET_RISCV32
target/riscv/translate.c:#ifdef TARGET_RISCV32
target/riscv/translate.c:#ifdef TARGET_RISCV32
target/riscv/translate.c:#ifdef TARGET_RISCV32
target/riscv/translate.c:#ifdef TARGET_RISCV32

One case we can remove the TARGET_RISCV32 is in translate.c.

static void gen_set_fpr_hs(DisasContext *ctx, int reg_num, TCGv_i64 t)

{

    if (!ctx->cfg_ptr->ext_zfinx) {

        tcg_gen_mov_i64(cpu_fpr[reg_num], t);

        return;

    }

    if (reg_num != 0) {

        switch (get_xl(ctx)) {

        case MXL_RV32:

#ifdef TARGET_RISCV32

            tcg_gen_extrl_i64_i32(cpu_gpr[reg_num], t);

            break;

#else

        /* fall through */

        case MXL_RV64:

            tcg_gen_mov_i64(cpu_gpr[reg_num], t);

            break;

#endif

        default:

            g_assert_not_reached();

        }

    }

}

We can simplify this code by tcg_gen_trunc_i64_tl.

One case we can't remove the TARGET_RISCV32 is in cpu.c, where we define any or max cpu with the width depending on this macro.
I don't analyze all TARGET_RISCV32 using here.  We will upstream a standalone patch for validating all its using later.

Thanks,
Zhiwei

>       
> >   }
> >         
> >           #endif
> >         
> >           
>