Re: [Qemu-devel] [PATCH 3/3] target/tricore: Use translate_loop

[Bastian Koppelmann <kbastian@mail.uni-paderborn.de>]

On 6/17/19 6:45 PM, Richard Henderson wrote:
> On 6/17/19 7:35 AM, Bastian Koppelmann wrote:
>> +static void tricore_tr_init_disas_context(DisasContextBase *dcbase,
>> +                                          CPUState *cs)
>>   {
>> +    DisasContext *ctx = container_of(dcbase, DisasContext, base);
>>       CPUTriCoreState *env = cs->env_ptr;
>> +    ctx->base.pc_next = ctx->base.pc_first;
> This is already done in generic code.
>
> I don't see an initialization of hflags & saved_hflags?
> Although I don't see that either before or afterward...
Yes, I mentioned this problem in David's patch 
(https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg01058.html). 
If he doesn't fix it, I will in a follow-up patch.
>
>> +static bool tricore_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cpu,
>> +                                      const CPUBreakpoint *bp)
>> +{
>> +    return true;
>> +}
> Not supporting breakpoints, I think it's better to return false here.
>
> Although it's not difficult -- just raise EXCP_DEBUG as an exception.
> It'd be nice to follow up and fix this afterward.
Yes, I will do that.
>
>> +static void tricore_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
>> +{
>> +    DisasContext *ctx = container_of(dcbase, DisasContext, base);
>> +    CPUTriCoreState *env = cpu->env_ptr;
>> +
>> +    ctx->opcode = cpu_ldl_code(env, ctx->base.pc_next);
>> +    decode_opc(ctx);
>> +    ctx->base.pc_next = ctx->pc_succ_insn;
>> +
>> +    if (ctx->base.is_jmp == DISAS_NEXT) {
>> +        target_ulong page_start;
>> +
>> +        page_start = ctx->base.pc_first & TARGET_PAGE_MASK;
>> +        if (ctx->base.pc_next - page_start >= TARGET_PAGE_SIZE) {
>> +            ctx->base.is_jmp = DISAS_TOO_MANY;
>>           }
> This isn't perfect as an ending, but you didn't seem to have one at all before,
> so I guess improvements can come incrementally afterward.
>
> Have a look at the end of thumb_tr_translate_insn & insn_crosses_page to see
> how to handle this properly.

I copied it more or less from target/riscv. I guess that needs fixing as 
well :)

Cheers,

Bastian