Re: [Qemu-devel] [PATCH V3 2/7] nios2: Add architecture emulation support

[Richard Henderson]

On 10/18/2016 02:50 PM, Marek Vasut wrote:
> +/* Special R-Type instruction opcode */
> +#define INSN_R_TYPE 0x3A
> +
> +/* I-Type instruction parsing */
> +#define I_TYPE(instr, code)              \
> +    struct {                             \
> +        uint8_t op;                      \
> +        union {                          \
> +            uint16_t imm16;              \
> +            int16_t imm16s;              \
> +        };                               \
> +        uint8_t b;                       \
> +        uint8_t a;                       \
> +    } (instr) = {                        \
> +        .op = ((code) >> 0) & 0x3f,      \
> +        .imm16 = ((code) >> 6) & 0xffff, \
> +        .b = ((code) >> 22) & 0x1f,      \
> +        .a = ((code) >> 27) & 0x1f,      \

It would be preferable to use extract32.

> +    }
> +
> +/* I-Type instruction parsing */
> +#define R_TYPE(instr, code)              \

Typo in the comment.

> +static void gen_goto_tb(DisasContext *dc, int n, uint32_t dest)
> +{
> +    TranslationBlock *tb = dc->tb;
> +
> +    if ((tb->pc & TARGET_PAGE_MASK) == (dest & TARGET_PAGE_MASK)) {

Normally we also suppress goto_tb when single-stepping.

> +static void gen_check_supervisor(DisasContext *dc, TCGLabel *label)
> +{
> +    if (dc->tb->flags & CR_STATUS_U) {    /* CPU in user mode */
> +        t_gen_helper_raise_exception(dc, EXCP_SUPERI);
> +        tcg_gen_br(label);
> +    }

There's no point in the label or branch, because raise_exception does not 
return.

> +
> +    /* Update the PC to the next instruction */
> +    tcg_gen_movi_tl(dc->cpu_R[R_PC], dc->pc + 4);
> +}

Why?

> +/*
> + * I-Type instructions
> + */
> +/* Load instructions */
> +static void gen_ldx(DisasContext *dc, uint32_t code, uint32_t flags)
> +{
> +    I_TYPE(instr, code);
> +
> +    /* Loads into R_ZERO are ignored */
> +    if (unlikely(instr.b == R_ZERO)) {
> +        return;
> +    } else if (instr.a == R_ZERO) {
> +        /* Loads from R_ZERO + offset are loaded directly from offset */
> +        tcg_gen_qemu_ld_tl(dc->cpu_R[instr.b], tcg_const_i32(instr.imm16s),
> +                           dc->mem_idx, flags);
> +    } else { /* Regular loads */
> +        TCGv addr = tcg_temp_new();
> +        tcg_gen_addi_tl(addr, dc->cpu_R[instr.a], instr.imm16s);

This is where it becomes cleaner to just use load_gpr, and let the TCG 
optimizer fold 0 + C -> C.

The documentation appears less than clear about whether or not loads into r0 
recognize exceptions from the load, as opposed to simply not modifying r0.

You should use the TCGMemOp type for flags.

> +/* Branch instructions */
> +static void br(DisasContext *dc, uint32_t code, uint32_t flags)
> +{
> +    I_TYPE(instr, code);
> +
> +    gen_goto_tb(dc, 0, dc->pc + 4 + (int16_t)(instr.imm16 & 0xFFFC));

Probably clearer as pc + 4 + (instr.imm16s & -4)

> +/* ctlN <- rA */
> +static void wrctl(DisasContext *dc, uint32_t code, uint32_t flags)
> +{
> +    R_TYPE(instr, code);
> +
> +    TCGLabel *l1 = gen_new_label();
> +    gen_check_supervisor(dc, l1);
> +
> +    switch (instr.imm5 + 32) {
> +    case CR_PTEADDR:
> +    case CR_TLBACC:
> +    case CR_TLBMISC:
> +    {
> +#if !defined(CONFIG_USER_ONLY)
> +        TCGv_i32 tmp = tcg_const_i32(instr.imm5 + 32);
> +        gen_helper_mmu_write(dc->cpu_env, tmp, dc->cpu_R[instr.a]);

load_gpr.

> +/* Comparison instructions */
> +static void gen_cmpxx(DisasContext *dc, uint32_t code, uint32_t flags)
> +{
> +    R_TYPE(instr, code);
> +    if (likely(instr.c != R_ZERO)) {
> +        tcg_gen_setcond_tl(flags, dc->cpu_R[instr.c], dc->cpu_R[instr.a],
> +                           dc->cpu_R[instr.b]);
> +    } else {
> +        TCGv_i32 tmp = tcg_const_i32(0);
> +        tcg_gen_setcond_tl(flags, tmp, dc->cpu_R[instr.a],
> +                           dc->cpu_R[instr.b]);
> +        tcg_temp_free_i32(tmp);

Why compute this into a tmp?

> +#define gen_r_math_logic_zeropt(fname, insn, op3)                            \
> +static void (fname)(DisasContext *dc, uint32_t code, uint32_t flags)       \
> +{                                                                          \
> +    R_TYPE(instr, (code));                                                 \
> +    /* NOP and store to R_ZERO optimization */                             \
> +    if (instr.c == R_ZERO) {                                               \
> +        return;                                                            \
> +    } else if ((instr.a == R_ZERO) && (instr.b == R_ZERO)) {               \
> +        tcg_gen_movi_tl(dc->cpu_R[instr.c], 0);                            \
> +    } else if (instr.a == R_ZERO) {                                        \
> +        tcg_gen_mov_tl(dc->cpu_R[instr.c], load_gpr(dc, instr.b));         \
> +    } else if (instr.b == R_ZERO) {                                        \
> +        tcg_gen_mov_tl(dc->cpu_R[instr.c], load_gpr(dc, instr.a));         \

These two are in general wrong.  Use load_gpr.

> +#define gen_r_div(fname, insn, op3)                            \
> +static void (fname)(DisasContext *dc, uint32_t code, uint32_t flags)       \
> +{                                                                          \
> +    R_TYPE(instr, (code));                                                 \
> +    /* NOP and store to R_ZERO optimization */                             \
> +    if (likely(instr.c != R_ZERO)) {                                       \
> +        TCGv val = tcg_const_tl(0);                                        \
> +        tcg_gen_setcond_tl(TCG_COND_EQ, val, (dc)->cpu_R[instr.b], val);   \
> +        tcg_gen_or_tl(val, val, (dc)->cpu_R[instr.b]);                     \
> +        tcg_gen_##insn((dc)->cpu_R[instr.c], (dc)->cpu_R[instr.a], val);   \
> +        tcg_temp_free(val);                                                \
> +    }                                                                      \
> +}
> +
> +gen_r_div(divs, div_tl,   dc->cpu_R[instr.b])
> +gen_r_div(divu, divu_tl,  dc->cpu_R[instr.b])

Unused op3?

> +    /* Dump the CPU state to the log */
> +    if (qemu_loglevel_mask(CPU_LOG_TB_IN_ASM)) {
> +        qemu_log("--------------\n");
> +        log_cpu_state(cs, 0);
> +    }

Cpu state is dumped by -d cpu generically.


r~