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~