On 3/28/22 06:57, Xiaojuan Yang wrote:
+#define CSR_OFF(X) \
+ [LOONGARCH_CSR_##X] = offsetof(CPULoongArchState, CSR_##X)
+#define CSR_OFF_ARRAY(X, N) \
+ [LOONGARCH_CSR_##X(N)] = offsetof(CPULoongArchState, CSR_##X[N])
+
+static const int csr_offsets[] = {
You cannot put a variable data definition into a header file like this.
It has put this data structure into every object file.
This belongs in csr_helper.c, probably.
You should add
[LOONGARCH_CSR_CPUID] = offsetof(CPUState, cpu_index) - offsetof(ArchCPU,
env),
rather than special-casing this in helper_csr_rdq.
+static inline int cpu_csr_offset(unsigned csr_num)
+{
+ if (csr_num < ARRAY_SIZE(csr_offsets)) {
+ return csr_offsets[csr_num];
+ }
+ return 0;
+}
This does not need to be inline, and could easily live in csr_helper.c.
+target_ulong helper_csr_rdq(CPULoongArchState *env, uint64_t csr)
+{
+ LoongArchCPU *cpu;
+ int64_t v;
+
+ switch (csr) {
+ 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;
+ case LOONGARCH_CSR_CPUID:
+ v = (env_cpu(env))->cpu_index;
+ break;
+ case LOONGARCH_CSR_TVAL:
+ cpu = LOONGARCH_CPU(env_cpu(env));
+ v = cpu_loongarch_get_constant_timer_ticks(cpu);
+ break;
+ default:
+ break;
+ }
+
+ return v;
+}
You should have seen a compiler warning for 'v' uninitialized here, via the
default path.
The default path should not be reachable, because of code in trans_csrrd, and so could be
written as g_assert_not_reachable(). However, I strongly suggest you split this function
so that you do not need a switch here at all. With CPUID now handled via cpu_csr_offset,
there are only two helpers needed.
+target_ulong helper_csr_wrq(CPULoongArchState *env, target_ulong val,
+ uint64_t csr)
+{
+ LoongArchCPU *cpu;
+ int64_t old_v = -1;
+
+ switch (csr) {
+ case LOONGARCH_CSR_ESTAT:
+ /* Only IS[1:0] can be write */
"can be written", and then again below.
+ env->CSR_ESTAT = FIELD_DP64(env->CSR_ESTAT, CSR_ESTAT, IS, val & 0x3);
+ break;
+ case LOONGARCH_CSR_ASID:
+ old_v = env->CSR_ASID;
+ /* Only ASID filed of CSR_ASID can be write. */
+ env->CSR_ASID = FIELD_DP64(env->CSR_ASID, CSR_ASID, ASID,
+ val & R_CSR_ASID_ASID_MASK);
+ if (old_v != val) {
+ tlb_flush(env_cpu(env));
+ }
+ break;
+ case LOONGARCH_CSR_TCFG:
+ cpu = LOONGARCH_CPU(env_cpu(env));
+ old_v = env->CSR_TCFG;
+ cpu_loongarch_store_constant_timer_config(cpu, val);
+ break;
+ case LOONGARCH_CSR_TICLR:
+ old_v = 0;
+ env->CSR_ESTAT &= ~(1 << IRQ_TIMER);
+ cpu_reset_interrupt(env_cpu(env), CPU_INTERRUPT_HARD);
Surely the TIMER irq is not the only interrupt.
The placement of the reset looks incorrect.
And again, I suggest that you *not* use a switch, but use separate helper
functions.
+target_ulong helper_csr_xchgq(CPULoongArchState *env, target_ulong new_val,
+ target_ulong mask, uint64_t csr_num)
+{
+ unsigned csr_offset = cpu_csr_offset(csr_num);
+ if (csr_offset == 0) {
+ /* Undefined CSR: read as 0, writes are ignored. */
+ return 0;
+ }
+
+ uint64_t *csr = (void *)env + csr_offset;
+ uint64_t old_val = *csr;
+
+ new_val = (new_val & mask) | (old_val & ~mask);
+
+ if (csr_num == LOONGARCH_CSR_TCFG) {
+ LoongArchCPU *cpu = LOONGARCH_CPU(env_cpu(env));
+ cpu_loongarch_store_constant_timer_config(cpu, new_val);
+ } else {
+ *csr = new_val;
You're only handling one of the special cases from helper_csr_wrq, so I cannot believe
this is correct.
I think you should not have a helper_csr_xchgq function, but reuse the read/write
infrastructure from the other csr access instructions. Note this would also fix...
+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);
+ if (csr_offset == 0) {
+ /* CSR is undefined: write ignored. */
+ return true;
+ }
+
+ if ((a->csr == LOONGARCH_CSR_ASID) || (a->csr == LOONGARCH_CSR_TCFG) ||
+ (a->csr == LOONGARCH_CSR_TICLR) || (a->csr == LOONGARCH_CSR_ESTAT)) {
+ gen_helper_csr_wrq(dest, cpu_env, src1, tcg_constant_i64(a->csr));
ASID change may result in page translation changes (which is why you did tlb_flush). This
also means that the page you are now executing could change translation, so you have to
exit the translation block.
+ } else {
+ 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)) {
+ tcg_gen_movi_tl(cpu_pc, ctx->base.pc_next + 4);
+ ctx->base.is_jmp = DISAS_EXIT;
... like this.
+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 (check_plv(ctx) || ro_csr(a->csr)) {
+ return false;
+ }
+ gen_helper_csr_xchgq(dest, cpu_env, src1, src2, tcg_constant_i64(a->csr));
+ return true;
+}
... back to xchg, you're not exiting the TB for any of the special cases above.
r~