On Thu, 2025-01-16 at 11:06 +0000, Peter Maydell wrote: > 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
The problem is not a partial state update in an instruction, but rather that on some targets restore_state_to_opc is more than just a "restore" - it is also "prepare for handling an exception", i.e.: - arm: exception.syndrome - hppa: unwind_breg, psw_n - mips: btarget - openrisc: ppc - riscv: excp_uw2 - s390x: int_pgm_ilen Some of these may be wrong due to unfamiliarity with the respective architectures, sorry - but this illustrates the idea.
