Re: [PATCH] accel/tcg: Call tcg_tb_insert() for one-insn TBs

[Peter Maydell]

On Thu, 16 Jan 2025 at 10:52, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Ilya Leoshkevich <iii@linux.ibm.com> writes:
>
> > On Wed, 2025-01-15 at 16:08 -0800, Richard Henderson wrote:
> >> On 1/15/25 15:20, Ilya Leoshkevich wrote:
> >> > Currently single-insn TBs created from I/O memory are not added to
> >> > region_trees. Therefore, when they generate exceptions, they are
> >> > not
> >> > handled by cpu_restore_state_from_tb(). For x86 this is not a
> >> > problem,
> >> > because x86_restore_state_to_opc() only restores pc and cc, which
> >> > are
> >> > already correct. However, on several other architectures,
> >> > restore_state_to_opc() restores more registers, and guests can
> >> > notice
> >> > incorrect values.
> >> >
> >> > Fix by always calling tcg_tb_insert(). This may increase the size
> >> > of
> >> > region_trees, but tcg_region_reset_all() clears it once
> >> > code_gen_buffer
> >> > fills up, so it will not grow uncontrollably.
> >> >
> >> > Co-developed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> >> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> >> > ---
> >>
> >> This needs something else.  The reason why they're not insertted is
> >> that they're not valid
> >> for a second execution.  We need to not find them in the search tree.
> >
> > I have the impression that code_gen_buffer is append-only, so after we
> > create a new TB for the second execution, the first TB should not
> > be deleted - is this correct? At least I haven't found code_gen_ptr
> > decrements, besides the rollback at the end of tb_gen_code(). Then,
> > since region_trees are indexed by code_gen_buffer pointers, and not
> > guest pointers, this should not introduce any stale entries.
>
> We used to generate a temporary TB, execute it and then wind codeptr
> back. We simplified the code to generate the TB but then not add it to
> QHT - I think the original reasoning was saving on scarce CF_ flags and
> not over complicating the tb_gen_code function. See around 873d64ac30
> (accel/tcg: re-factor non-RAM execution code).
>
> This does have the effect of potentially regenerating the same TB
> over and over again but usually there only a few insns executed out of
> IO space.

You can't avoid regenerating the TB unless you somehow put the
entire bytes of the instruction into the hash you look up,
because you must re-read the MMIO region, and when you do the
second read you might read a different value than the first
time you read and created the TB.

The original reported problem here seems to me like it's a
problem with whatever target's frontend code this is.
This is a single instruction TB, so either:
 * the generated code for it completes the insn without
   raising an exception (no problem)
 * the generated code for it should raise an exception
   without having modified the CPU state (so there would
   be nothing to do for restore_state_to_opc)

It sounds like the target is generating code which does
something like:
 * do part of the instruction, including updating some of
   the CPU state
 * then decide it needs to raise an exception, and rely on
   the restore_state_to_opc handling to undo the state updates
   it did previously

The assumption of the "throwaway single insn TB" is that
you don't do that (i.e. that restore_state_to_opc is only
there for the benefit of multi-insn TBs).

thanks
-- PMM