On Thu, 16 Jan 2025 at 15:40, Alex Bennée <alex.ben...@linaro.org> wrote: > > Richard Henderson <richard.hender...@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 <n...@linux.ibm.com> > >>>> Signed-off-by: Ilya Leoshkevich <i...@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.hender...@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
