Re: [PATCH 0/6] target/microblaze: Use tcg_gen_lookup_and_goto_ptr

[Edgar E. Iglesias]

On Mon, Aug 31, 2020 at 11:40:12AM -0700, Richard Henderson wrote:
> Based-on: <20200831160601.833692-1-richard.henderson@linaro.org>
> ("[PULL 00/76] target/microblaze improvements")
> 
> Hello again, Edgar.
> 
> I had dropped the tcg_gen_lookup_and_goto_ptr patch from the
> previous omnibus patch set, as you had reported lockups.
> 
> I have identified, by inspection, two cases in which we failed
> to return to the main loop even though we should have:
> 
> (1) Return-from-exception type instructions.
> 
> I had missed these before because they hadn't set cpustate_changed.
> This still worked fine because they are all indirect branches, and
> had exited immediately.
> 
> Fixed by distinguishing these cases from normal indirect branches
> before we start using lookup_and_goto_ptr.
> 
> (2) MTS in a branch delay slot.
> 
> We did not check dc->cpustate_changed before setting
> dc->base.is_jmp to DISAS_JUMP, which lost the fact that we
> need to return to the main loop.
> 
> This mostly works fine without lookup_and_goto_ptr, because
> we either (a) finished an indirect branch and returned to the
> main loop anyway or (b) we'd return to the main loop via some
> subsequent indirect branch, which would happen "soon enough".
> 
> We should have been able to see soft-lockup with the existing
> code in the case of a cpustate_changed in the delay slot of
> a loop of direct branches that all use goto_tb.  E.g.
> 
>       brid    0
>        msrset MSR_IE
> 
> I.e. an immediate branch back to the same branch insn,
> re-enabling interrupts in the delay slot.  Probably not
> something that shows up in the wild.
> 
> ----
> 
> Follow-up question: The manual says that several classes of
> instructions are invalid in a branch delay slot, but does
> not say what action is taken, if any.
> 
> Some of these invalid cases could leave qemu in an inconsistent
> state.  Would it be legal for us to diagnose these cases with
> trap_illegal?  If not, what *should* we be doing?  We could also
> LOG_GUEST_ERROR for these either way.

What I found out is that these result in undefined and undocumented
behaviour but that the behaviour is deterministic, i.e the cores
won't lock-up or exposed security issues or anything like that.
RTL will not raise exceptions on these either.

So I think LOG_GUEST_ERROR and treating as NOP is probably a good
approach. We can fix that in follow-up patches.

Cheers,
Edgar