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

[yangxiaojuan]

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~