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

["Alex Bennée" <alex.bennee@linaro.org>]

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.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro