Re: [PATCH v2 06/22] target/loongarch: Add main translation routines

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

On 7/20/21 11:53 PM, Song Gao wrote:
> +/* General purpose registers moves. */
> +void gen_load_gpr(TCGv t, int reg)
> +{
> +    if (reg == 0) {
> +        tcg_gen_movi_tl(t, 0);
> +    } else {
> +        tcg_gen_mov_tl(t, cpu_gpr[reg]);
> +    }
> +}

Please have a look at

https://patchew.org/QEMU/20210709042608.883256-1-richard.henderson@linaro.org/

for a better way to handle the zero register.


> +static inline void save_cpu_state(DisasContext *ctx, int do_save_pc)
> +{
> +    if (do_save_pc && ctx->base.pc_next != ctx->saved_pc) {
> +        gen_save_pc(ctx->base.pc_next);
> +        ctx->saved_pc = ctx->base.pc_next;
> +    }
> +    if (ctx->hflags != ctx->saved_hflags) {
> +        tcg_gen_movi_i32(hflags, ctx->hflags);
> +        ctx->saved_hflags = ctx->hflags;
> +        switch (ctx->hflags & LOONGARCH_HFLAG_BMASK) {
> +        case LOONGARCH_HFLAG_BR:
> +            break;
> +        case LOONGARCH_HFLAG_BC:
> +        case LOONGARCH_HFLAG_B:
> +            tcg_gen_movi_tl(btarget, ctx->btarget);
> +            break;
> +        }
> +    }
> +}

Drop all the hflags handling.
It's all copied from mips delay slot handling.

> +
> +static inline void restore_cpu_state(CPULoongArchState *env, DisasContext *ctx)
> +{
> +    ctx->saved_hflags = ctx->hflags;
> +    switch (ctx->hflags & LOONGARCH_HFLAG_BMASK) {
> +    case LOONGARCH_HFLAG_BR:
> +        break;
> +    case LOONGARCH_HFLAG_BC:
> +    case LOONGARCH_HFLAG_B:
> +        ctx->btarget = env->btarget;
> +        break;
> +    }
> +}

Likewise.

> +static void gen_load_fpr32h(TCGv_i32 t, int reg)
> +{
> +    tcg_gen_extrh_i64_i32(t, fpu_f64[reg]);
> +}
> +
> +static void gen_store_fpr32h(TCGv_i32 t, int reg)
> +{
> +    TCGv_i64 t64 = tcg_temp_new_i64();
> +    tcg_gen_extu_i32_i64(t64, t);
> +    tcg_gen_deposit_i64(fpu_f64[reg], fpu_f64[reg], t64, 32, 32);
> +    tcg_temp_free_i64(t64);
> +}

There is no general-purpose high-part fpr stuff.  There's only movgr2frh and movfrh2gr, 
and you can simplify both if you drop the transition through TCGv_i32.

> +void gen_op_addr_add(TCGv ret, TCGv arg0, TCGv arg1)
> +{
> +    tcg_gen_add_tl(ret, arg0, arg1);
> +}

No point in this, since loongarch has no 32-bit address mode.

> +void gen_base_offset_addr(TCGv addr, int base, int offset)
> +{
> +    if (base == 0) {
> +        tcg_gen_movi_tl(addr, offset);
> +    } else if (offset == 0) {
> +        gen_load_gpr(addr, base);
> +    } else {
> +        tcg_gen_movi_tl(addr, offset);
> +        gen_op_addr_add(addr, cpu_gpr[base], addr);
> +    }
> +}

Using the interfaces I quote above from my riscv cleanup,
this can be tidied to

     tcg_gen_addi_tl(addr, gpr_src(base), offset);

> +static inline bool use_goto_tb(DisasContext *ctx, target_ulong dest)
> +{
> +    return true;
> +}

You must now use translate_use_goto_tb, which will not always return true.  You will see 
assertion failures otherwise.

> +static inline void clear_branch_hflags(DisasContext *ctx)
> +{
> +    ctx->hflags &= ~LOONGARCH_HFLAG_BMASK;
> +    if (ctx->base.is_jmp == DISAS_NEXT) {
> +        save_cpu_state(ctx, 0);
> +    } else {
> +        /*
> +         * It is not safe to save ctx->hflags as hflags may be changed
> +         * in execution time.
> +         */
> +        tcg_gen_andi_i32(hflags, hflags, ~LOONGARCH_HFLAG_BMASK);
> +    }
> +}

Not required.

> +static void gen_branch(DisasContext *ctx, int insn_bytes)
> +{
> +    if (ctx->hflags & LOONGARCH_HFLAG_BMASK) {
> +        int proc_hflags = ctx->hflags & LOONGARCH_HFLAG_BMASK;
> +        /* Branches completion */
> +        clear_branch_hflags(ctx);
> +        ctx->base.is_jmp = DISAS_NORETURN;
> +        switch (proc_hflags & LOONGARCH_HFLAG_BMASK) {
> +        case LOONGARCH_HFLAG_B:
> +            /* unconditional branch */
> +            gen_goto_tb(ctx, 0, ctx->btarget);
> +            break;
> +        case LOONGARCH_HFLAG_BC:
> +            /* Conditional branch */
> +            {
> +                TCGLabel *l1 = gen_new_label();
> +
> +                tcg_gen_brcondi_tl(TCG_COND_NE, bcond, 0, l1);
> +                gen_goto_tb(ctx, 1, ctx->base.pc_next + insn_bytes);
> +                gen_set_label(l1);
> +                gen_goto_tb(ctx, 0, ctx->btarget);
> +            }
> +            break;
> +        case LOONGARCH_HFLAG_BR:
> +            /* unconditional branch to register */
> +            tcg_gen_mov_tl(cpu_PC, btarget);
> +            tcg_gen_lookup_and_goto_ptr();
> +            break;
> +        default:
> +            fprintf(stderr, "unknown branch 0x%x\n", proc_hflags);
> +            abort();
> +        }
> +    }
> +}

Split this up into the various trans_* branch routines, without the setting of HFLAG.

> +static void loongarch_tr_init_disas_context(DisasContextBase *dcbase,
> +                                            CPUState *cs)
> +{
> +    DisasContext *ctx = container_of(dcbase, DisasContext, base);
> +    CPULoongArchState *env = cs->env_ptr;
> +
> +    ctx->page_start = ctx->base.pc_first & TARGET_PAGE_MASK;
> +    ctx->saved_pc = -1;
> +    ctx->btarget = 0;
> +    /* Restore state from the tb context.  */
> +    ctx->hflags = (uint32_t)ctx->base.tb->flags;
> +    restore_cpu_state(env, ctx);
> +    ctx->mem_idx = LOONGARCH_HFLAG_UM;

This is not an mmu index.  You didn't notice the error because you're only doing user-mode.

You're missing a check for page crossing.
Generally, for fixed-width ISAs like this, we do

     /* Bound the number of insns to execute to those left on the page.  */
     int bound = -(ctx->base.pc_first | TARGET_PAGE_MASK) / 4;
     ctx->base.max_insns = MIN(ctx->base.max_insns, bound);

here in init_disas_context.

> +static void loongarch_tr_insn_start(DisasContextBase *dcbase, CPUState *cs)
> +{
> +    DisasContext *ctx = container_of(dcbase, DisasContext, base);
> +
> +    tcg_gen_insn_start(ctx->base.pc_next, ctx->hflags & LOONGARCH_HFLAG_BMASK,
> +                       ctx->btarget);

No hflags/btarget stuff.  Drop TARGET_INSN_START_EXTRA_WORDS.

> +static bool loongarch_tr_breakpoint_check(DisasContextBase *dcbase,
> +                                          CPUState *cs,
> +                                          const CPUBreakpoint *bp)
> +{
> +    return true;
> +}

Broken, but now handled generically, so remove it.


> +static void loongarch_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs)
> +{
> +    CPULoongArchState *env = cs->env_ptr;
> +    DisasContext *ctx = container_of(dcbase, DisasContext, base);
> +    int insn_bytes = 4;
> +
> +    ctx->opcode = cpu_ldl_code(env, ctx->base.pc_next);
> +
> +    if (!decode(ctx, ctx->opcode)) {
> +        fprintf(stderr, "Error: unkown opcode. 0x%lx: 0x%x\n",
> +                ctx->base.pc_next, ctx->opcode);

No fprintfs.  Use qemu_log_mask with LOG_UNIMP or LOG_GUEST_ERROR.

> +    if (ctx->hflags & LOONGARCH_HFLAG_BMASK) {
> +        gen_branch(ctx, insn_bytes);
> +    }

Drop this, as I mentioned above.

> +static void fpu_dump_state(CPULoongArchState *env, FILE * f, int flags)
> +{
> +    int i;
> +    int is_fpu64 = 1;
> +
> +#define printfpr(fp)                                              \
> +    do {                                                          \
> +        if (is_fpu64)                                             \
> +            qemu_fprintf(f, "w:%08x d:%016" PRIx64                \
> +                        " fd:%13g fs:%13g psu: %13g\n",           \
> +                        (fp)->w[FP_ENDIAN_IDX], (fp)->d,          \
> +                        (double)(fp)->fd,                         \
> +                        (double)(fp)->fs[FP_ENDIAN_IDX],          \
> +                        (double)(fp)->fs[!FP_ENDIAN_IDX]);        \
> +        else {                                                    \
> +            fpr_t tmp;                                            \
> +            tmp.w[FP_ENDIAN_IDX] = (fp)->w[FP_ENDIAN_IDX];        \
> +            tmp.w[!FP_ENDIAN_IDX] = ((fp) + 1)->w[FP_ENDIAN_IDX]; \
> +            qemu_fprintf(f, "w:%08x d:%016" PRIx64                \
> +                        " fd:%13g fs:%13g psu:%13g\n",            \
> +                        tmp.w[FP_ENDIAN_IDX], tmp.d,              \
> +                        (double)tmp.fd,                           \
> +                        (double)tmp.fs[FP_ENDIAN_IDX],            \
> +                        (double)tmp.fs[!FP_ENDIAN_IDX]);          \
> +        }                                                         \
> +    } while (0)

This is broken.  You're performing an integer to fp conversion of something that is 
already a floating-point value, not printing the floating-point value itself.  It's broken 
in the mips code as well.

In addition, is_fpu64 is pointless for loongarch.

> +void loongarch_tcg_init(void)
> +{
> +    int i;
> +
> +    for (i = 0; i < 32; i++)
> +        cpu_gpr[i] = tcg_global_mem_new(cpu_env,
> +                                        offsetof(CPULoongArchState,
> +                                                 active_tc.gpr[i]),
> +                                        regnames[i]);

Missing braces.
Do not create a temp for the zero register.

> +    bcond = tcg_global_mem_new(cpu_env,
> +                               offsetof(CPULoongArchState, bcond), "bcond");
> +    btarget = tcg_global_mem_new(cpu_env,
> +                                 offsetof(CPULoongArchState, btarget),
> +                                 "btarget");
> +    hflags = tcg_global_mem_new_i32(cpu_env,
> +                                    offsetof(CPULoongArchState, hflags),
> +                                    "hflags");

Drop these.


r~