Re: [Qemu-devel] [PATCH RFC v20 5/8] target/avr: Add instruction translation

[Richard Henderson <richard.henderson@linaro.org>]

On 5/30/19 2:07 PM, Michael Rolnik wrote:
> +    /* decode first instruction */
> +    ctx.inst[0].cpc = pc_start;
> +    decode_opc(&ctx, &ctx.inst[0]);
> +    do {
> +        /* set curr/next PCs */
> +        cpc = ctx.inst[0].cpc;
> +        npc = ctx.inst[0].npc;
> +
> +        /* decode next instruction */
> +        ctx.inst[1].cpc = ctx.inst[0].npc;
> +        decode_opc(&ctx, &ctx.inst[1]);
> +
> +        /* translate current instruction */
> +        tcg_gen_insn_start(cpc);
> +        num_insns++;

I don't believe that this simultaneous decode of two instructions is correct.

Consider if ctx.inst[0] is a branch instruction that is placed as the very last
word of memory.  Ordinarily, the branch would be executed and the
TranslationBlock ended.

However, the advance read of ctx.inst[1] will cause a read from unmapped
address space (causing an exception), or read from a device (causing "Bad ram
pointer" and an abort from qemu_ram_addr_from_host_nofail).

I believe that the feature that you're attempting to support with this, skip
the next instruction, should be handled via an internal flag bit.  This would
end up looking a lot like the HPPA nullify bit, or somewhat like the ARM thumb
condexec_mask.  I can go into specifics if needed.

Such a change would also allow you to structure this code to use
"exec/translator.h", which in the future will likely be mandatory.


r~