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

[Michael Rolnik]

Hi Richard.

these instructions are not branches or jumps they all do skip.
however, if you think it's important I change it, I will, just show me an
example or explain.

On Fri, May 31, 2019 at 6:31 PM Richard Henderson <
address@hidden> wrote:

> 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~
>


-- 
Best Regards,
Michael Rolnik