Re: [Qemu-devel] [PATCH v3 06/10] target/arm: optimize indirect branches

[Aurelien Jarno]

On 2017-04-26 23:20, Emilio G. Cota wrote:
> On Wed, Apr 26, 2017 at 09:54:07 +0200, Richard Henderson wrote:
> > On 04/26/2017 08:23 AM, Emilio G. Cota wrote:
> > >+static bool gen_jr;...
> > >         case DISAS_JUMP:
> > >+            if (gen_jr) {
> > 
> > Why the variable?  Why not just try the goto_ptr for any DISAS_JUMP?
> 
> We have code that assumes DISAS_JUMP implies "go to exec loop", e.g.:
>             case 6: /* isb */
>                 /* We need to break the TB after this insn to execute
>                  * self-modifying code correctly and also to take
>                  * any pending interrupts immediately.
>                  */
>                 gen_lookup_tb(s);
> where gen_lookup_tb does:
> 
> /* Force a TB lookup after an instruction that changes the CPU state.  */
> static inline void gen_lookup_tb(DisasContext *s)
> {
>     tcg_gen_movi_i32(cpu_R[15], s->pc & ~1);
>     s->is_jmp = DISAS_JUMP;
> }

Changing the CPU state should already be taken into account in
lookup_tb_ptr through the cpu_get_tb_cpu_state function. Also interrupts
should be checked at the beginning of each TB.

> Also, the gen_exception_* functions set DISAS_JUMP. I suspect we want to go
> to the exec loop with those as well.

Technically all the code after the one generated by gen_exception_*
function is not executed, including the tcg_gen_exit_tb(0). Therefore
generating code using tcg_gen_exit_tb or or tcg_gen_lookup_and_goto_ptr
should no change the behaviour.

> Testing shows that I'm onto something; if I remove the variable,
> and note that I make sure DISAS_UPDATE is not falling through, I get
> easily reproducible (~1 out of 5) freezes and other instability
> (e.g. RCU lockup warnings) when booting + shutting down debian jessie
> in the guest.

I agree that always calling lookup_tb_ptr might be suboptimal in case we
know for sure that the lookup will fail or that there will be an exit
request at the beginning of the next TB. That said I found strange that
it causes issues, and I wonder if it just expose a bug somewhere.

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
address@hidden                 http://www.aurel32.net