Re: [PATCH 09/31] target/loongarch: Add other core instructions support

[Richard Henderson]

On 10/19/21 12:34 AM, Xiaojuan Yang wrote:
> diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
> index afd186abac..7fa3851251 100644
> --- a/target/loongarch/cpu.c
> +++ b/target/loongarch/cpu.c
> @@ -45,6 +45,7 @@ static const char * const excp_names[EXCP_LAST + 1] = {
>       [EXCP_TLBPE] = "TLB priviledged error",
>       [EXCP_TLBNX] = "TLB execute-inhibit",
>       [EXCP_TLBNR] = "TLB read-inhibit",
> +    [EXCP_DBP] = "debug breakpoint",
>   };
>   
>   const char *loongarch_exception_name(int32_t exception)
> diff --git a/target/loongarch/cpu.h b/target/loongarch/cpu.h
> index 567fc7620d..d39c618d6b 100644
> --- a/target/loongarch/cpu.h
> +++ b/target/loongarch/cpu.h
> @@ -166,10 +166,13 @@ enum {
>       EXCP_TLBPE,
>       EXCP_TLBNX,
>       EXCP_TLBNR,
> +    EXCP_DBP,
>   
> -    EXCP_LAST = EXCP_TLBNR,
> +    EXCP_LAST = EXCP_DBP,
>   };

These should have been added with the other exceptions earlier.

> @@ -173,6 +179,11 @@ static bool trans_iocsrwr_d(DisasContext *ctx, 
> arg_iocsrwr_d *a)
>       return true;
>   }
>   
> +static bool trans_cacop(DisasContext *ctx, arg_cacop *a)
> +{
> +    return false;
> +}

I think you meant to return true here, so that cacop is interpreted as a nop.  You would 
add a comment to that effect.

> +static bool trans_ldpte(DisasContext *ctx, arg_ldpte *a)
> +{
> +    TCGv_i32 mem_idx = tcg_constant_i32(ctx->mem_idx);
> +    TCGv src1 = gpr_src(ctx, a->rj, EXT_NONE);
> +
> +    gen_helper_ldpte(cpu_env, src1, tcg_constant_tl(a->seq), mem_idx);
> +    return true;
> +}

Missing priv checks again.

> +static bool trans_idle(DisasContext *ctx, arg_idle *a)
> +{
> +    ctx->base.pc_next += 4;
> +    tcg_gen_movi_tl(cpu_pc, ctx->base.pc_next);
> +    ctx->base.pc_next -= 4;

Why would you add and subtract like this?

> +#ifndef CONFIG_USER_ONLY
> +static inline void exception_return(CPULoongArchState *env)
> +{
> +    if (env->CSR_TLBRERA & 0x1) {
> +        env->CSR_CRMD = FIELD_DP64(env->CSR_CRMD, CSR_CRMD, PLV,
> +                                   FIELD_EX64(env->CSR_TLBRPRMD, CSR_TLBRPRMD, 
> PPLV));
> +        env->CSR_CRMD = FIELD_DP64(env->CSR_CRMD, CSR_CRMD, IE,
> +                                   FIELD_EX64(env->CSR_TLBRPRMD, CSR_TLBRPRMD, 
> PIE));
> +        /* Clear Refill flag and set pc */
> +        env->CSR_TLBRERA &= (~0x1);
> +        env->pc = env->CSR_TLBRERA;
> +        if (qemu_loglevel_mask(CPU_LOG_INT)) {
> +            qemu_log("%s: TLBRERA 0x%lx\n", __func__, env->CSR_TLBRERA);
> +        }
> +    } else {
> +        env->CSR_CRMD = FIELD_DP64(env->CSR_CRMD, CSR_CRMD, PLV,
> +                                   FIELD_EX64(env->CSR_PRMD, CSR_PRMD, PPLV));
> +        env->CSR_CRMD = FIELD_DP64(env->CSR_CRMD, CSR_CRMD, IE,
> +                                   FIELD_EX64(env->CSR_PRMD, CSR_PRMD, PIE));
> +        /* set pc*/
> +        env->pc = env->CSR_ERA;
> +        if (qemu_loglevel_mask(CPU_LOG_INT)) {
> +            qemu_log("%s: ERA 0x%lx\n", __func__, env->CSR_ERA);
> +        }
> +    }
> +}
> +
> +void helper_ertn(CPULoongArchState *env)
> +{
> +    exception_return(env);
> +    env->lladdr = 1;
> +}

Any reason to have exception_return be a separate function?

> +target_ulong helper_lddir(CPULoongArchState *env, target_ulong base,
> +                          target_ulong level, uint32_t mem_idx)
> +{
> +    target_ulong badvaddr;
> +    target_ulong index;
> +    target_ulong vaddr;
> +    int shift;
> +    uint64_t dir1_base, dir1_width;
> +    uint64_t dir3_base, dir3_width;
> +
> +    badvaddr = env->CSR_TLBRBADV;
> +
> +    /* 0:8B, 1:16B, 2:32B, 3:64B */
> +    shift = FIELD_EX64(env->CSR_PWCL, CSR_PWCL, PTEWIDTH);
> +    shift = (shift + 1) * 3;
> +
> +    switch (level) {
> +    case 1:
> +        dir1_base = FIELD_EX64(env->CSR_PWCL, CSR_PWCL, DIR1_BASE);
> +        dir1_width = FIELD_EX64(env->CSR_PWCL, CSR_PWCL, DIR1_WIDTH);
> +        index = (badvaddr >> dir1_base) & ((1 << dir1_width) - 1);
> +        break;
> +    case 3:
> +        dir3_base = FIELD_EX64(env->CSR_PWCH, CSR_PWCH, DIR3_BASE);
> +        dir3_width = FIELD_EX64(env->CSR_PWCH, CSR_PWCH, DIR3_WIDTH);
> +        index = (badvaddr >> dir3_base) & ((1 << dir3_width) - 1);
> +        break;
> +    default:
> +        do_raise_exception(env, EXCP_INE, GETPC());
> +        return 0;
> +    }
> +
> +    vaddr = base | index << shift;
> +    return cpu_ldq_mmuidx_ra(env, vaddr, mem_idx, GETPC());

A load from mem_idx is incorrect -- you have a physical address (thus the name "vaddr" is 
at best misleading). There are two possible solutions:

(1) ldq_phys.

(2) Add a separate MMU_PHYS_IDX, which you could use here.  You would also return this 
value from cpu_mem_index when CRMD.{DA,PG} indicates that direct address translation is 
enabled.

> +void helper_ldpte(CPULoongArchState *env, target_ulong base, target_ulong odd,
> +                  uint32_t mem_idx)
> +{
...
> +        vaddr = base | (odd ? ptoffset1 : ptoffset0);
> +        tmp0 = cpu_ldq_mmuidx_ra(env, vaddr, mem_idx, GETPC());

Likewise.


r~