Re: [PATCH v1 25/43] target/loongarch: Add LoongArch CSR instruction

[Richard Henderson]

On 4/15/22 02:40, Xiaojuan Yang wrote:
> +int cpu_csr_offset(unsigned csr_num);
...
> +static const uint64_t csr_offsets[] = {

There's no reason for this array to be uint64_t.
It really should match the function.

> +target_ulong helper_csrwr_estat(CPULoongArchState *env, target_ulong val)
> +{
> +    int64_t old_v = env->CSR_ESTAT;
> +
> +    /* Only IS[1:0] can be written */
> +    env->CSR_ESTAT = FIELD_DP64(env->CSR_ESTAT, CSR_ESTAT, IS, val & 0x3);
> +
> +    return old_v;
> +}

This is incorrect.  You're writing to all 13 bits of ESTAT.IS with the low 2 bits of val 
-- i.e. clearing bits [12:2].

You want to use: env->CSR_ESTAT = deposit64(env->CSR_ESTAT, 0, 2, val);

> +target_ulong helper_csrwr_asid(CPULoongArchState *env, target_ulong val)
> +{
> +    int64_t old_v = env->CSR_ASID;
> +
> +    /* Only ASID filed of CSR_ASID can be written */
> +    env->CSR_ASID = FIELD_DP64(env->CSR_ASID, CSR_ASID, ASID,
> +                               val & R_CSR_ASID_ASID_MASK);

You do not need to mask the 4th argument of FIELD_DP64 -- that happens as part of the 
deposit operation.

> +    if (old_v != val) {
> +        tlb_flush(env_cpu(env));
> +    }

You shouldn't be comparing val to old_v, but old_v to the updated CSR_ASID.

> +void  helper_csr_update(CPULoongArchState *env, target_ulong new_val,
> +                        target_ulong csr_offset)
> +{
> +    uint64_t *csr = (void *)env + csr_offset;
> +
> +    *csr = new_val;
> +}

This function should not exist.


> +static void output_r_csr(DisasContext *ctx, arg_r_csr *a,
> +                         const char *mnemonic)
> +{
> +    output(ctx, mnemonic, "r%d, %d", a->rd, a->csr);
> +}
> +
> +static void output_rr_csr(DisasContext *ctx, arg_rr_csr *a,
> +                          const char *mnemonic)
> +{
> +    output(ctx, mnemonic, "r%d, r%d, %d", a->rd, a->rj, a->csr);
> +}

While this is fine, it would be nicer to print the name of CSR, when valid.

> +static void gen_disas_exit(DisasContext *ctx)
> +{
> +    tcg_gen_movi_tl(cpu_pc, ctx->base.pc_next + 4);
> +    ctx->base.is_jmp = DISAS_EXIT;
> +}

Why this function, and not simply place the movi in loongarch_tr_tb_stop too?
Or even just put the tcg_gen_exit_tb() here, and set DISAS_NORETURN.
I would say: one way or the other but not this mix...

> +static bool trans_csrrd(DisasContext *ctx, arg_csrrd *a)
> +{
> +    TCGv dest = gpr_dst(ctx, a->rd, EXT_NONE);
> +
> +    if (check_plv(ctx)) {
> +        return false;
> +    }
> +
> +    unsigned csr_offset = cpu_csr_offset(a->csr);

This is incorrect -- csr_offset must be signed, 'int', to match cpu_csr_offset and the 
single negative value that exists within (CPUID).

> +static bool trans_csrwr(DisasContext *ctx, arg_csrwr *a)
> +{
> +    TCGv dest = gpr_dst(ctx, a->rd, EXT_NONE);
> +    TCGv src1 = gpr_src(ctx, a->rd, EXT_NONE);
> +
> +    if (check_plv(ctx) || ro_csr(a->csr)) {
> +        return false;
> +    }
> +
> +    unsigned csr_offset = cpu_csr_offset(a->csr);

Again, 'int'.

> +    if (csr_offset == 0) {
> +        /* CSR is undefined: write ignored. */
> +        return true;
> +    }
> +
> +    switch (a->csr) {
> +    case LOONGARCH_CSR_ESTAT:
> +        gen_helper_csrwr_estat(dest, cpu_env, src1);
> +        break;
> +    case LOONGARCH_CSR_ASID:
> +        gen_helper_csrwr_asid(dest, cpu_env, src1);
> +        gen_disas_exit(ctx);
> +        break;
> +    case LOONGARCH_CSR_TCFG:
> +        gen_helper_csrwr_tcfg(dest, cpu_env, src1);
> +        break;
> +    case LOONGARCH_CSR_TICLR:
> +        gen_helper_csrwr_ticlr(dest, cpu_env, src1);
> +        break;
> +    default:
> +        {
> +            TCGv temp = tcg_temp_new();
> +            tcg_gen_ld_tl(temp, cpu_env, csr_offset);
> +            tcg_gen_st_tl(src1, cpu_env, csr_offset);
> +            tcg_gen_mov_tl(dest, temp);
> +            tcg_temp_free(temp);
> +
> +            /* Cpu state may be changed, need exit */
> +            if ((a->csr == LOONGARCH_CSR_CRMD) ||
> +                (a->csr == LOONGARCH_CSR_EUEN)) {
> +                gen_disas_exit(ctx);
> +            }
> +        }
> +    }

I said before that you needed to split out the body of this function for re-use 
by csrxchg.

> +    tcg_gen_not_tl(t1, mask);
> +    tcg_gen_and_tl(t1, old_val, t1);

This is tcg_gen_andc_t1 (t1, old_val, mask).

> +    switch (a->csr) {
> +    case LOONGARCH_CSR_ESTAT:
> +        gen_helper_csrwr_estat(dest, cpu_env, new_val);
> +        break;
> +    case LOONGARCH_CSR_ASID:
> +        gen_helper_csrwr_asid(dest, cpu_env, new_val);
> +        break;
> +    case LOONGARCH_CSR_TCFG:
> +        gen_helper_csrwr_tcfg(dest, cpu_env, new_val);
> +        break;
> +    case LOONGARCH_CSR_TICLR:
> +        gen_helper_csrwr_ticlr(dest, cpu_env, new_val);
> +        break;
> +    default:
> +        tcg_gen_mov_tl(dest, old_val);
> +    }
> +
> +    gen_helper_csr_update(cpu_env, new_val, tcg_constant_tl(csr_offset));

Note that helper_csr_update is nothing more than the store to csr_offset.

> +
> +    if ((a->csr == LOONGARCH_CSR_ASID) || (a->csr == LOONGARCH_CSR_CRMD) ||
> +        (a->csr == LOONGARCH_CSR_EUEN)) {
> +        gen_disas_exit(ctx);
> +    }

Note that this list does not match the list in trans_csrwr.  This is *exactly* why I said 
that you should split out a function for use between csrwr and csrxchg.



r~