Hi, Richard.
On 07/23/2021 07:50 AM, Richard Henderson wrote:
> 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.hender...@linaro.org/
>
> for a better way to handle the zero register.
> >
OK, I'll look at it carefully.
>> +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.
>
OK.
>> +
>> +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.
>
OK.
>> +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.
>
OK.
>> +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);
>
'riscv cleanup' series at
https://patchew.org/QEMU/20210709042608.883256-1-richard.hender...@linaro.org/
, Right?
>> +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.
>
I see the patch already.
>> +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.
>
>
OK.
>> +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.
>
OK.
>> + if (ctx->hflags & LOONGARCH_HFLAG_BMASK) {
>> + gen_branch(ctx, insn_bytes);
>> + }
>
> Drop this, as I mentioned above.
>
OK.
>> +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.
>
Yes.
>> +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.
OK.
Thanks for you kindly help.
Thanks
Song Gao.
