Re: [RFC PATCH v7 07/29] target/loongarch: Add LoongArch CSR instruction

[Richard Henderson <richard.henderson@linaro.org>]

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~