On Thu, 2025-01-16 at 15:41 +0000, Peter Maydell wrote: > 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
Ok. I will send a v2 with docstring additions in a separate patch.
