Hi, Richard: On 10/20/2021 09:36 AM, Richard Henderson wrote: > 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? > OK, I have modified all above.
>> +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. > (1) For the iocsr registers, most of them act on the interrupt controller, the read and write will go to interrupt's mmio read/write. So I modified the addr to their mmio range. The ext interrupt controller use the sysbus's function to handle the interrupt cascade and sysbus_mmio_map to map the address which use the system memory region. So if I use a different address space, I realize a different sysbus_mmio_map function use a different address space, is it feasible ? (2)Can you help me review the remaining patches? Thanks. >> +csrxchg 0000 0100 .............. ..... ..... @fmt_rdrjcsr > > { > csrrd 0000 0100 .............. 00000 ..... @fmt_rdcsr > csrwr 0000 0100 .............. 00001 ..... @fmt_rdcsr > csrxchg 0000 0100 .............. ..... ..... @fmt_rdrjcsr > } > > > r~