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

[Ilya Leoshkevich <iii@linux.ibm.com>]

On Thu, 2025-01-16 at 15:41 +0000, Peter Maydell wrote:
> On Thu, 16 Jan 2025 at 15:40, Alex Bennée <alex.bennee@linaro.org>
> wrote:
> > 
> > Richard Henderson <richard.henderson@linaro.org> writes:
> > 
> > > On 1/16/25 07:06, Ilya Leoshkevich wrote:
> > > > On Thu, 2025-01-16 at 06:54 -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>
> > > > > > ---
> > > > > >    accel/tcg/translate-all.c | 16 ++++++++--------
> > > > > >    1 file changed, 8 insertions(+), 8 deletions(-)
> > > > > > 
> > > > > > diff --git a/accel/tcg/translate-all.c
> > > > > > b/accel/tcg/translate-all.c
> > > > > > index 453eb20ec95..6333302813e 100644
> > > > > > --- a/accel/tcg/translate-all.c
> > > > > > +++ b/accel/tcg/translate-all.c
> > > > > > @@ -531,23 +531,23 @@ TranslationBlock
> > > > > > *tb_gen_code(CPUState *cpu,
> > > > > >            tb_reset_jump(tb, 1);
> > > > > >        }
> > > > > >    +    /*
> > > > > > +     * Insert TB into the corresponding region tree before
> > > > > > publishing it
> > > > > > +     * through QHT. Otherwise rewinding happened in the TB
> > > > > > might
> > > > > > fail to
> > > > > > +     * lookup itself using host PC.
> > > > > > +     */
> > > > > > +    tcg_tb_insert(tb);
> > > > > 
> > > > > I think what we need is to mark the tb CF_INVALID before
> > > > > inserting
> > > > > it. That way we'll
> > > > > never match in tb_lookup (comparing guest state, including
> > > > > cflags),
> > > > > but *will* find it in
> > > > > tcg_tb_lookup (comparing host_pc).
> > > > > 
> > > > > 
> > > > > r~
> > > > How can tb_lookup() find it? With this change, it is inserted
> > > > into
> > > > region_trees, but not into tb_ctx.htable - this is done by
> > > > tb_link_page(), which is not called. And because it's not in
> > > > tb_ctx.htable, it can't end up in tb_jmp_cache either.
> > > 
> > > You're absolutely right.
> > > 
> > > Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> > 
> > It would still be nice to update the docstrings on the two lookup
> > functions to make it clear what they are for though.
> 
> Given this thread, perhaps also consider briefly mentioning some
> of the details in the commit message and/or the comment?
> 
> -- PMM

Ok. I will send a v2 with docstring additions in a separate patch.