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

[Marek Vasut]

On 10/19/2016 01:04 AM, Richard Henderson wrote:
> 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.

Fixed

>> +    }
>> +
>> +/* I-Type instruction parsing */
>> +#define R_TYPE(instr, code)              \
> 
> Typo in the comment.

Fixed, thanks

>> +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.

Fixed

>> +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.

Fixed

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

Hrm, I cannot find good explanation. Must be a remnant from the old
code, so removed.

>> +/*
>> + * 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.

What about [1] page 10, quote:

The Nios II architecture provides thirty-two 32-bit general-purpose
registers, r0 through r31. Some registers have names recognized by the
assembler. For example, the zero register (r0) always returns the
value zero, and writing to zero has no effect.
[...]

[1]
https://www.altera.com/content/dam/altera-www/global/en_US/pdfs/literature/hb/nios2/n2cpu_nii5v1.pdf

> You should use the TCGMemOp type for flags.

Fixed

>> +/* 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)

I disagree, the 0xfffc in my opinion clearly states it removes bottom
two bits. Doing bitops with signed number looks real iffy.

>> +/* ctlN <- rA */
>> +static void wrctl(DisasContext *dc, uint32_t code, uint32_t flags)
>> +{
>> +    R_TYPE(instr, code);
>> +
>> +    TCGLabel *l1 = gen_new_label();

I can also drop this 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.

Fixed

>> +/* 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?

Hmmmm, seems meaningless. I don't need the whole branch, so I can remove it.

>> +#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.

OK

>> +#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?

Fixed

>> +    /* 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.

Dropped, thanks

-- 
Best regards,
Marek Vasut