On 10/19/21 12:34 AM, Xiaojuan Yang wrote:
+target_ulong helper_csr_rdq(CPULoongArchState *env, uint64_t csr)
+{
+ int64_t v;
+
+#define CASE_CSR_RDQ(csr) \
+ case LOONGARCH_CSR_ ## csr: \
+ { \
+ v = env->CSR_ ## csr; \
+ break; \
+ }; \
There's absolutely no reason to call a helper function for a simple load.
+ case LOONGARCH_CSR_PGD:
+
+ if (env->CSR_TLBRERA & 0x1) {
+ v = env->CSR_TLBRBADV;
+ } else {
+ v = env->CSR_BADV;
+ }
+
+ if ((v >> 63) & 0x1) {
+ v = env->CSR_PGDH;
+ } else {
+ v = env->CSR_PGDL;
+ }
+ break;
This is the only one that requires a helper on read.
+ if (csr == LOONGARCH_CSR_ASID) {
+ if (old_v != val) {
+ tlb_flush(env_cpu(env));
+ }
+ }
And this is the only one that requires a helper on write.
+ case LOONGARCH_CSR_ESTAT:
+ qatomic_and(&env->CSR_ESTAT, ~mask);
Why do you believe this requires an atomic update?
What is going to race with the update to a cpu private value?
+static bool trans_csrrd(DisasContext *ctx, unsigned rd, unsigned csr)
+{
+ TCGv dest = gpr_dst(ctx, rd, EXT_NONE);
+ gen_helper_csr_rdq(dest, cpu_env, tcg_constant_i64(csr));
+ return true;
+}
+
+static bool trans_csrwr(DisasContext *ctx, unsigned rd, unsigned csr)
+{
+ TCGv dest = gpr_dst(ctx, rd, EXT_NONE);
+ TCGv src1 = gpr_src(ctx, rd, EXT_NONE);
+
+ switch (csr) {
+ case LOONGARCH_CSR_CRMD:
+ tcg_gen_movi_tl(cpu_pc, ctx->base.pc_next);
+ gen_helper_csr_wrq(dest, cpu_env, src1,
tcg_constant_i64(LOONGARCH_CSR_CRMD));
+ tcg_gen_movi_tl(cpu_pc, ctx->base.pc_next + 4);
+ ctx->base.is_jmp = DISAS_EXIT;
+ break;
+ case LOONGARCH_CSR_EUEN:
+ gen_helper_csr_wrq(dest, cpu_env, src1,
tcg_constant_i64(LOONGARCH_CSR_EUEN));
+ /* Stop translation */
+ tcg_gen_movi_tl(cpu_pc, ctx->base.pc_next + 4);
+ ctx->base.is_jmp = DISAS_EXIT;
+ break;
+ default:
+ gen_helper_csr_wrq(dest, cpu_env, src1, tcg_constant_i64(csr));
+ break;
+ }
+ return true;
+}
+
+static bool trans_csrxchg(DisasContext *ctx, arg_csrxchg *a)
+{
+ TCGv dest = gpr_dst(ctx, a->rd, EXT_NONE);
+ TCGv src1 = gpr_src(ctx, a->rd, EXT_NONE);
+ TCGv src2 = gpr_src(ctx, a->rj, EXT_NONE);
+
+ if (a->rj == 0) {
+ return trans_csrrd(ctx, a->rd, a->csr);
+ } else if (a->rj == 1) {
+ return trans_csrwr(ctx, a->rd, a->csr);
+ }
These should have been decoded separately; see below.
You're missing the check for priv 0 here and in all other functions.
+
+ if (a->rd == 0) {
+ gen_helper_csr_xchgq_r0(cpu_env, src2, tcg_constant_i64(a->csr));
+ } else {
+ gen_helper_csr_xchgq(dest, cpu_env, src1, src2,
tcg_constant_i64(a->csr));
+ }
Why do you believe r0 to require a special case?
+static bool trans_iocsrrd_b(DisasContext *ctx, arg_iocsrrd_b *a)
+{
+ TCGv tmp = tcg_temp_new();
+ TCGv dest = gpr_dst(ctx, a->rd, EXT_NONE);
+ TCGv src1 = gpr_src(ctx, a->rj, EXT_NONE);
+
+ gen_helper_iocsr_read(tmp, cpu_env, src1);
+ tcg_gen_qemu_ld_tl(dest, tmp, ctx->mem_idx, MO_SB);
This is wrong. From the manual:
All IOCSR registers use independent addressing space
therefore you cannot simply read from a modified address, you must use a completely
different address space.
There are a couple of different solutions that are possible.
(1) Use helper functions calling address_space_ld/st*.
(2) Use a separate mmu_idx, which uses its own address space.
This requires bouncing through MemTxAttrs, since
cpu_asidx_from_attrs only take attrs and not mmu_idx.
The second one is may be overkill, unless there will be any cachable memory in iospace. I
would expect most of it to be device memory.
+csrxchg 0000 0100 .............. ..... ..... @fmt_rdrjcsr
{
csrrd 0000 0100 .............. 00000 ..... @fmt_rdcsr
csrwr 0000 0100 .............. 00001 ..... @fmt_rdcsr
csrxchg 0000 0100 .............. ..... ..... @fmt_rdrjcsr
}
r~
