On Mon, Feb 19, 2018 at 8:02 AM, Pavel Dovgalyuk <dovga...@ispras.ru> wrote:
>> From: Pavel Dovgalyuk [mailto:dovga...@ispras.ru]
>> > From: Peter Maydell [mailto:peter.mayd...@linaro.org]
>> > On 13 February 2018 at 10:26, Pavel Dovgalyuk <dovga...@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
>
