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

[Peter Maydell]

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