[v2,2/2] accel/tcg: Call tcg_tb_insert() for one-insn TBs

Commit Message

Ilya Leoshkevich Jan. 16, 2025, 9:31 p.m. UTC

Currently one-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 already have the correct values if the first
TB instruction causes an exception. However, on several other
architectures, restore_state_to_opc() is not stricly limited to state
restoration and affects some exception-related registers, where guests
can notice incorrect values, for example:

- arm's exception.syndrome;
- hppa's unwind_breg;
- riscv's excp_uw2;
- s390x's int_pgm_ilen.

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.

Do not call tb_link_page(), which would add such TBs to the QHT, to
prevent tb_lookup() from finding them. These TBs are single-use, since
subsequent reads from I/O memory may return different values; they are
not removed from code_gen_buffer only in order to keep things simple.

Co-developed-by: Nina Schoetterl-Glausch <[e-mail]>
Reviewed-by: Richard Henderson <[e-mail]>
Signed-off-by: Ilya Leoshkevich <[e-mail]>
---
 accel/tcg/translate-all.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

Comments

Alex Bennée Jan. 17, 2025, 10:38 a.m. UTC | #1

Ilya Leoshkevich <[e-mail]> writes:

> Currently one-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 already have the correct values if the first
> TB instruction causes an exception. However, on several other
> architectures, restore_state_to_opc() is not stricly limited to state
> restoration and affects some exception-related registers, where guests
> can notice incorrect values, for example:
>
> - arm's exception.syndrome;
> - hppa's unwind_breg;
> - riscv's excp_uw2;
> - s390x's int_pgm_ilen.
>
> 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.
>
> Do not call tb_link_page(), which would add such TBs to the QHT, to
> prevent tb_lookup() from finding them. These TBs are single-use, since
> subsequent reads from I/O memory may return different values; they are
> not removed from code_gen_buffer only in order to keep things simple.
>
> Co-developed-by: Nina Schoetterl-Glausch <[e-mail]>
> Reviewed-by: Richard Henderson <[e-mail]>
> Signed-off-by: Ilya Leoshkevich <[e-mail]>

Reviewed-by: Alex Bennée <[e-mail]>

Patch

diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index 453eb20ec95..7ec1c53f240 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -531,23 +531,32 @@  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);
+
     /*
      * If the TB is not associated with a physical RAM page then it must be
-     * a temporary one-insn TB, and we have nothing left to do. Return early
-     * before attempting to link to other TBs or add to the lookup table.
+     * a temporary one-insn TB.
+     *
+     * Such TBs must be added to region trees in order to make sure that
+     * restore_state_to_opc() - which on some architectures is not limited to
+     * rewinding, but also affects exception handling! - is called when such a
+     * TB causes an exception.
+     *
+     * At the same time, temporary one-insn TBs must be executed at most once,
+     * because subsequent reads from, e.g., I/O memory may return different
+     * values. So return early before attempting to link to other TBs or add
+     * to the QHT.
      */
     if (tb_page_addr0(tb) == -1) {
         assert_no_pages_locked();
         return tb;
     }
 
-    /*
-     * 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);
-
     /*
      * No explicit memory barrier is required -- tb_link_page() makes the
      * TB visible in a consistent state.