Re: [Qemu-devel] [RFC PATCH v6 00/20] replay additions

[Ciro Santilli <ciro.santilli@gmail.com>]

On Mon, Feb 19, 2018 at 8:02 AM, Pavel Dovgalyuk <dovgaluk@ispras.ru> wrote:
>> From: Pavel Dovgalyuk [mailto:dovgaluk@ispras.ru]
>> > From: Peter Maydell [mailto:peter.maydell@linaro.org]
>> > On 13 February 2018 at 10:26, Pavel Dovgalyuk <dovgaluk@ispras.ru> wrote:
>> > > Then I added SCSI adapter with the option –device lsi,id=scsi0 and QEMU
>> > > failed with the following error:
>> > >
>> > > qemu: fatal: IO on conditional branch instruction
>> >
>> > > Seems, that your kernel is incomatible with QEMU, which ARM emulation is not
>> > > good enough.
>> >
>> > It seems fairly unlikely to me that the Linux driver for this
>> > SCSI adaptor is using weirdo self-modifying code of the kind
>> > that would trip up that cpu_abort(). I would suggest a bit
>> > more investigation into what's actually happening...
>>
>> Peter, I bisected this bug and figured out the following.
>>
>> icount in ARM was broken by the following commit: 9b990ee5a3cc6aa38f81266fb0c6ef37a36c45b9
>> tcg: Add CPUState cflags_next_tb
>> This commit breaks execution of Ciro's kernel with enabled icount.
>> I haven't yet figured out why this happens.
>
> The problem is in the following code.
> As far, as I can understand, original version recompiles the TB and
> continues the execution as it goes.
>
> But the modified version sets cflags for the next compilation.
> And these are the flags for the old TB which should replace the original one.
> TCG tries to use cflags for the new TB (which starts after the interrupted one)
> and fails, because these flags are inappropriate.
> That is why icount execution fails.
>
> New version also does not include recompilation of the old block, which is wrong too.
>

Awesome! Can you push it to a branch, and give the full qemu command
line so I can test it?

>
> @@ -1773,9 +1765,7 @@ void cpu_io_recompile(CPUState *cpu, uintptr_t retaddr)
>      CPUArchState *env = cpu->env_ptr;
>  #endif
>      TranslationBlock *tb;
> -    uint32_t n, cflags;
> -    target_ulong pc, cs_base;
> -    uint32_t flags;
> +    uint32_t n;
>
>      tb_lock();
>      tb = tb_find_pc(retaddr);
> @@ -1813,12 +1803,9 @@ void cpu_io_recompile(CPUState *cpu, uintptr_t retaddr)
>          cpu_abort(cpu, "TB too big during recompile");
>      }
>
> -    cflags = n | CF_LAST_IO;
> -    cflags |= curr_cflags();
> -    pc = tb->pc;
> -    cs_base = tb->cs_base;
> -    flags = tb->flags;
> -    tb_phys_invalidate(tb, -1);
> +    /* Adjust the execution state of the next TB.  */
> +    cpu->cflags_next_tb = curr_cflags() | CF_LAST_IO | n;
> +
>      if (tb->cflags & CF_NOCACHE) {
>          if (tb->orig_tb) {
>              /* Invalidate original TB if this TB was generated in
> @@ -1827,9 +1814,6 @@ void cpu_io_recompile(CPUState *cpu, uintptr_t retaddr)
>          }
>          tb_free(tb);
>      }
> -    /* FIXME: In theory this could raise an exception.  In practice
> -       we have already translated the block once so it's probably ok.  */
> -    tb_gen_code(cpu, pc, cs_base, flags, cflags);
>
>      /* TODO: If env->pc != tb->pc (i.e. the faulting instruction was not
>       * the first in the TB) then we end up generating a whole new TB and
>
>
>
> Pavel Dovgalyuk
>