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

[Ilya Leoshkevich]

On Thu, 2025-01-16 at 11:06 +0000, Peter Maydell wrote:
> 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

The problem is not a partial state update in an instruction, but rather
that on some targets restore_state_to_opc is more than just a
"restore" - it is also "prepare for handling an exception", i.e.:

- arm: exception.syndrome
- hppa: unwind_breg, psw_n
- mips: btarget
- openrisc: ppc
- riscv: excp_uw2
- s390x: int_pgm_ilen

Some of these may be wrong due to unfamiliarity with the respective
architectures, sorry - but this illustrates the idea.