Re: [PATCH 07/31] target/loongarch: Add loongarch csr/iocsr instruction support

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

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~