On Thu, 16 Jan 2025 at 10:52, Alex Bennée <alex.ben...@linaro.org> wrote: > > Ilya Leoshkevich <i...@linux.ibm.com> writes: > > > On Wed, 2025-01-15 at 16:08 -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> > >> > --- > >> > >> This needs something else. The reason why they're not insertted is > >> that they're not valid > >> for a second execution. We need to not find them in the search tree. > > > > I have the impression that code_gen_buffer is append-only, so after we > > create a new TB for the second execution, the first TB should not > > be deleted - is this correct? At least I haven't found code_gen_ptr > > decrements, besides the rollback at the end of tb_gen_code(). Then, > > since region_trees are indexed by code_gen_buffer pointers, and not > > guest pointers, this should not introduce any stale entries. > > We used to generate a temporary TB, execute it and then wind codeptr > back. We simplified the code to generate the TB but then not add it to > QHT - I think the original reasoning was saving on scarce CF_ flags and > not over complicating the tb_gen_code function. See around 873d64ac30 > (accel/tcg: re-factor non-RAM execution code). > > This does have the effect of potentially regenerating the same TB > over and over again but usually there only a few insns executed out of > IO space.
You can't avoid regenerating the TB unless you somehow put the entire bytes of the instruction into the hash you look up, because you must re-read the MMIO region, and when you do the second read you might read a different value than the first time you read and created the TB. The original reported problem here seems to me like it's a problem with whatever target's frontend code this is. This is a single instruction TB, so either: * the generated code for it completes the insn without raising an exception (no problem) * the generated code for it should raise an exception without having modified the CPU state (so there would be nothing to do for restore_state_to_opc) It sounds like the target is generating code which does something like: * do part of the instruction, including updating some of the CPU state * then decide it needs to raise an exception, and rely on the restore_state_to_opc handling to undo the state updates it did previously The assumption of the "throwaway single insn TB" is that you don't do that (i.e. that restore_state_to_opc is only there for the benefit of multi-insn TBs). thanks -- PMM
