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

[Marek Vasut]

On 10/20/2016 04:35 PM, Richard Henderson wrote:
> On 10/20/2016 06:44 AM, Marek Vasut wrote:
>> +typedef struct Nios2Instruction {
>> +    void     (*handler)(DisasContext *dc, uint32_t code, TCGMemOp
>> flags);
>> +    uint32_t  flags;
>> +} Nios2Instruction;
> 
> I gave you some bad advice wrt the type of flags beforehand.  I had
> failed to divine that it was also used for EXCP_* and TCG_COND_*.  I
> think you were right the first time with unsigned.
> 
> My bad, sorry.

I undid the change, thanks.

>> +/* Load instructions */
>> +static void gen_ldx(DisasContext *dc, uint32_t code, TCGMemOp flags)
>> +{
>> +    I_TYPE(instr, code);
>> +
>> +    TCGv addr = tcg_temp_new();
>> +    TCGv data = tcg_temp_new();
>> +    tcg_gen_addi_tl(addr, load_gpr(dc, instr.a), instr.imm16s);
>> +    tcg_gen_qemu_ld_tl(data, addr, dc->mem_idx, flags);
>> +
>> +    /*
>> +     * WARNING: Loads into R_ZERO are ignored, but we must generate the
>> +     *          memory access itself to emulate the CPU precisely. Load
>> +     *          from a protected page to R_ZERO will cause SIGSEGV on
>> +     *          the Nios2 CPU.
>> +     */
>> +    if (likely(instr.b != R_ZERO)) {
>> +        tcg_gen_mov_tl(dc->cpu_R[instr.b], data);
>> +    }
> 
> Consider
> 
>   TCGv data;
> 
>   if (unlikely(instr.b == R_ZERO)) {
>     /* The writeback to R_ZERO is ignored, but we must generate the
>      * memory access itself to emulate the CPU precisely.  Load from
>      * a protected page to R_ZERO will cause SIGSEGV on the Nios2 CPU.
>      */
>     data = tcg_temp_new();
>   } else {
>     data = dc->cpu_R[instr.b];
>   }
> 
>   tcg_gen_qemu_ld_tl(data, addr, dc->mem_idx, flags);
> 
>   if (unlikely(instr.b == R_ZERO)) {
>     tcg_temp_free(data);
>   }
> 
> so that you don't require the mov opcode.
> 
> That's really what I do on Alpha with dest_gpr.

OK

>> +#define gen_r_div(fname,
>> insn)                                              \
>> +static void (fname)(DisasContext *dc, uint32_t code, TCGMemOp
>> flags)        \
>> +{                                                                          
>> \
>> +    R_TYPE(instr,
>> (code));                                                  \
>> +    if (likely(instr.c != R_ZERO))
>> {                                        \
>> +        TCGv val =
>> tcg_const_tl(0);                                         \
>> +        tcg_gen_setcond_tl(TCG_COND_EQ, val, load_gpr((dc), instr.b),
>> val); \
>> +        tcg_gen_or_tl(val, val, load_gpr((dc),
>> instr.b));                   \
>> +        tcg_gen_##insn((dc)->cpu_R[instr.c], load_gpr((dc), instr.a),
>> val); \
>> +       
>> tcg_temp_free(val);                                                 \
>> +   
>> }                                                                       \
>> +}
>> +
>> +gen_r_div(divs, div_tl)
> 
> For signed division, you have to protect against 0x80000000 / -1 as
> well, which raises an overflow exception on the x86 host.

You mean similar to what mips does on OPC_DIV vs OPC_DIVU , right ?

>> +    /* Set up instruction counts */
>> +    num_insns = 0;
>> +    max_insns = tb->cflags & CF_COUNT_MASK;
>> +    if (max_insns == 0) {
>> +        max_insns = CF_COUNT_MASK;
>> +    }
>> +    if (max_insns > TCG_MAX_INSNS) {
>> +        max_insns = TCG_MAX_INSNS;
>> +    }
>> +    next_page_start = (tb->pc & TARGET_PAGE_MASK) + TARGET_PAGE_SIZE;
> ...
>> +    } while (!dc->is_jmp &&
>> +             !tcg_op_buf_full() &&
>> +             !cs->singlestep_enabled &&
>> +             !singlestep &&
>> +             dc->pc < next_page_start &&
>> +             num_insns < max_insns);
> 
> Consider
> 
>   if (cs->singlestep_enabled || singlestep) {
>     max_insns = 1;
>   } else {
>     int page_insns = (TARGET_PAGE_SIZE - (tb->pc & TARGET_PAGE_MASK)) / 4;
>     max_insns = tb->cflags & CF_COUNT_MASK;
>     if (max_insns == 0) {
>       max_insns = CF_COUNT_MASK;
>     }
>     if (max_insns > page_insns) {
>       max_insns = page_insns;
>     }
>     if (max_insns > TCG_MAX_INSNS) {
>       max_insns = TCG_MAX_INSNS;
>     }
>   }
> 
> so that we collapse the last 4 loop conditions into: num_insns < max_insns.

OK, fixed.

>> +    /* End off the block */
>> +    gen_tb_end(tb, num_insns);
>> +
>> +    /* Mark instruction starts for the final generated instruction */
>> +    tb->size = dc->pc - tb->pc;
>> +    tb->icount = num_insns;
> 
> No CPU_LOG_TB_IN_ASM disassembly?  I thought patch 1 added a nios2
> disassembler.

Nope, I removed it in V4, maybe i misunderstood the review comment?
"
> +    /* 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.
"
-- 
Best regards,
Marek Vasut