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

["Pavel Dovgalyuk" <dovgaluk@ispras.ru>]

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


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