Re: [PATCH 12/31] target/loongarch: Add timer related instructions support.

[Richard Henderson]

On 10/19/21 12:34 AM, Xiaojuan Yang wrote:
> This includes:
> -RDTIME{L/H}.W
> -RDTIME.D
>
> Signed-off-by: Xiaojuan Yang <yangxiaojuan@loongson.cn>
> Signed-off-by: Song Gao <gaosong@loongson.cn>
> ---
>   target/loongarch/helper.h                 |  2 ++
>   target/loongarch/insn_trans/trans_core.c  | 23 +++++++++++++++++++++
>   target/loongarch/insn_trans/trans_extra.c |  2 ++
>   target/loongarch/op_helper.c              | 25 +++++++++++++++++++++++
>   4 files changed, 52 insertions(+)
>
> diff --git a/target/loongarch/helper.h b/target/loongarch/helper.h
> index 8544771790..b4ed62896f 100644
> --- a/target/loongarch/helper.h
> +++ b/target/loongarch/helper.h
> @@ -116,4 +116,6 @@ DEF_HELPER_4(lddir, tl, env, tl, tl, i32)
>   DEF_HELPER_4(ldpte, void, env, tl, tl, i32)
>   DEF_HELPER_1(ertn, void, env)
>   DEF_HELPER_1(idle, void, env)
> +DEF_HELPER_4(rdtime_w, void, env, tl, tl, i64)
> +DEF_HELPER_3(rdtime_d, void, env, tl, tl)
>   #endif /* !CONFIG_USER_ONLY */
> diff --git a/target/loongarch/insn_trans/trans_core.c 
> b/target/loongarch/insn_trans/trans_core.c
> index 7fa13e65b9..24eb12b97a 100644
> --- a/target/loongarch/insn_trans/trans_core.c
> +++ b/target/loongarch/insn_trans/trans_core.c
> @@ -276,4 +276,27 @@ static bool trans_idle(DisasContext *ctx, arg_idle *a)
>       return true;
>   }
>   
> +static bool trans_rdtimel_w(DisasContext *ctx, arg_rdtimel_w *a)
> +{
> +    TCGv t0 = tcg_constant_tl(a->rd);
> +    TCGv t1 = tcg_constant_tl(a->rj);
> +    gen_helper_rdtime_w(cpu_env, t0, t1, tcg_constant_tl(0));
> +    return true;
> +}

Missing icount boilerplate:

    if (tb_cflags(ctx->base.tb) & CF_USE_ICOUNT) {
        gen_io_start();
        ctx->base.is_jmp = DISAS_TOO_MANY;
    }

Something similar is probably required before some of the other instructions that always 
result in i/o.

> diff --git a/target/loongarch/insn_trans/trans_extra.c 
> b/target/loongarch/insn_trans/trans_extra.c
> index 8da3b404f3..426b67f154 100644
> --- a/target/loongarch/insn_trans/trans_extra.c
> +++ b/target/loongarch/insn_trans/trans_extra.c
> @@ -36,6 +36,7 @@ static bool trans_asrtgt_d(DisasContext *ctx, arg_asrtgt_d * 
> a)
>       return true;
>   }
>   
> +#ifdef CONFIG_USER_ONLY
>   static bool trans_rdtimel_w(DisasContext *ctx, arg_rdtimel_w *a)
>   {
>       tcg_gen_movi_tl(cpu_gpr[a->rd], 0);
> @@ -53,6 +54,7 @@ static bool trans_rdtime_d(DisasContext *ctx, arg_rdtime_d *a)
>       tcg_gen_movi_tl(cpu_gpr[a->rd], 0);
>       return true;
>   }
> +#endif

You shouldn't have two copies of these.

> +void helper_rdtime_w(CPULoongArchState *env, target_ulong rd,
> +                     target_ulong rj, target_ulong high)
> +{
> +    if (rd) {
> +        if (high) {
> +            env->gpr[rd] = cpu_loongarch_get_stable_counter(env) >> 32;
> +        } else {
> +            env->gpr[rd] = cpu_loongarch_get_stable_counter(env) & 0xffffffff;
> +        }

This is incorrect -- the result should be sign-extended in RD.

> +void helper_rdtime_d(CPULoongArchState *env, target_ulong rd, target_ulong rj)
> +{
> +    if (rd) {
> +        env->gpr[rd] = cpu_loongarch_get_stable_counter(env);
> +    }
> +    if (rj) {
> +        env->gpr[rj] = env->CSR_TMID;
> +    }

You shouldn't need two functions.  Just return the full 64-bit result, placing that into 
RD, then read TMID separately.

    rd = gpr_dst(ctx, a->rd, EXT_NONE);
    gen_helper_rdtime_d(rd, cpu_env);
    if (word) {
        tcg_gen_sextract_tl(rd, rd, high ? 32 : 0, 32);
    }

    rj = gpr_dst(ctx, a->rj, EXT_NONE);
    tcg_gen_ld_i64(rj, cpu_env, offsetof(CPULoongArchState, CSR_TMID));


r~