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

["Edgar E. Iglesias" <edgar.iglesias@xilinx.com>]

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.


Nice! Yes, this seems to fix the problem we ran into before.
Series looks good both in review and testing except for minor typo in a comment.

With the typo in patch #4 fixed:
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Tested-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>



> 
> ----
> 
> 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.
> 
> I've added some TODO comments in these patches that are relevant.

Thanks, I'll try to dig out some details. A guest-error will likely
be needed anyway since some cores don't have exceptions enabled.
But we may want both.

Cheers,
Edgar