* [PATCH v2 2/2] accel/tcg: Call tcg_tb_insert() for one-insn TBs
2025-01-16 21:31 [PATCH v2 1/2] tcg: Document tb_lookup() and tcg_tb_lookup() Ilya Leoshkevich
@ 2025-01-16 21:31 ` Ilya Leoshkevich
2025-01-17 10:38 ` Alex Bennée
2025-01-17 4:44 ` [PATCH v2 1/2] tcg: Document tb_lookup() and tcg_tb_lookup() Richard Henderson
` (2 subsequent siblings)
3 siblings, 1 reply; 6+ messages in thread
From: Ilya Leoshkevich @ 2025-01-16 21:31 UTC (permalink / raw)
To: Richard Henderson, Paolo Bonzini, Alex Bennée, Peter Maydell,
Philippe Mathieu-Daudé
Cc: qemu-devel, Ilya Leoshkevich, Nina Schoetterl-Glausch
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 <nsg@linux.ibm.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
accel/tcg/translate-all.c | 27 ++++++++++++++++++---------
1 file changed, 18 insertions(+), 9 deletions(-)
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.
--
2.47.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v2 2/2] accel/tcg: Call tcg_tb_insert() for one-insn TBs
2025-01-16 21:31 ` [PATCH v2 2/2] accel/tcg: Call tcg_tb_insert() for one-insn TBs Ilya Leoshkevich
@ 2025-01-17 10:38 ` Alex Bennée
0 siblings, 0 replies; 6+ messages in thread
From: Alex Bennée @ 2025-01-17 10:38 UTC (permalink / raw)
To: Ilya Leoshkevich
Cc: Richard Henderson, Paolo Bonzini, Peter Maydell,
Philippe Mathieu-Daudé, qemu-devel, Nina Schoetterl-Glausch
Ilya Leoshkevich <iii@linux.ibm.com> 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 <nsg@linux.ibm.com>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] tcg: Document tb_lookup() and tcg_tb_lookup()
2025-01-16 21:31 [PATCH v2 1/2] tcg: Document tb_lookup() and tcg_tb_lookup() Ilya Leoshkevich
2025-01-16 21:31 ` [PATCH v2 2/2] accel/tcg: Call tcg_tb_insert() for one-insn TBs Ilya Leoshkevich
@ 2025-01-17 4:44 ` Richard Henderson
2025-01-17 10:38 ` Alex Bennée
2025-01-17 16:28 ` Richard Henderson
3 siblings, 0 replies; 6+ messages in thread
From: Richard Henderson @ 2025-01-17 4:44 UTC (permalink / raw)
To: Ilya Leoshkevich, Paolo Bonzini, Alex Bennée, Peter Maydell,
Philippe Mathieu-Daudé
Cc: qemu-devel
On 1/16/25 13:31, Ilya Leoshkevich wrote:
> These similarly named functions serve different purposes; add
> docstrings to highlight them.
>
> Suggested-by: Alex Bennée <alex.bennee@linaro.org>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
> accel/tcg/cpu-exec.c | 15 ++++++++++++++-
> include/tcg/tcg.h | 41 +++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 55 insertions(+), 1 deletion(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
>
> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> index d48b82a9325..8b773d88478 100644
> --- a/accel/tcg/cpu-exec.c
> +++ b/accel/tcg/cpu-exec.c
> @@ -249,7 +249,20 @@ static TranslationBlock *tb_htable_lookup(CPUState *cpu, vaddr pc,
> return qht_lookup_custom(&tb_ctx.htable, &desc, h, tb_lookup_cmp);
> }
>
> -/* Might cause an exception, so have a longjmp destination ready */
> +/**
> + * tb_lookup:
> + * @cpu: CPU that will execute the returned translation block
> + * @pc: guest PC
> + * @cs_base: arch-specific value associated with translation block
> + * @flags: arch-specific translation block flags
> + * @cflags: CF_* flags
> + *
> + * Look up a translation block inside the QHT using @pc, @cs_base, @flags and
> + * @cflags. Uses @cpu's tb_jmp_cache. Might cause an exception, so have a
> + * longjmp destination ready.
> + *
> + * Returns: an existing translation block or NULL.
> + */
> static inline TranslationBlock *tb_lookup(CPUState *cpu, vaddr pc,
> uint64_t cs_base, uint32_t flags,
> uint32_t cflags)
> diff --git a/include/tcg/tcg.h b/include/tcg/tcg.h
> index a77ed12b9dc..057df6c4599 100644
> --- a/include/tcg/tcg.h
> +++ b/include/tcg/tcg.h
> @@ -746,10 +746,51 @@ void tcg_region_reset_all(void);
> size_t tcg_code_size(void);
> size_t tcg_code_capacity(void);
>
> +/**
> + * tcg_tb_insert:
> + * @tb: translation block to insert
> + *
> + * Insert @tb into the region trees.
> + */
> void tcg_tb_insert(TranslationBlock *tb);
> +
> +/**
> + * tcg_tb_remove:
> + * @tb: translation block to remove
> + *
> + * Remove @tb from the region trees.
> + */
> void tcg_tb_remove(TranslationBlock *tb);
> +
> +/**
> + * tcg_tb_lookup:
> + * @tc_ptr: host PC to look up
> + *
> + * Look up a translation block inside the region trees by @tc_ptr. This is
> + * useful for exception handling, but must not be used for the purposes of
> + * executing the returned translation block. See struct tb_tc for more
> + * information.
> + *
> + * Returns: a translation block previously inserted into the region trees,
> + * such that @tc_ptr points anywhere inside the code generated for it, or
> + * NULL.
> + */
> TranslationBlock *tcg_tb_lookup(uintptr_t tc_ptr);
> +
> +/**
> + * tcg_tb_foreach:
> + * @func: callback
> + * @user_data: opaque value to pass to @callback
> + *
> + * Call @func for each translation block inserted into the region trees.
> + */
> void tcg_tb_foreach(GTraverseFunc func, gpointer user_data);
> +
> +/**
> + * tcg_nb_tbs:
> + *
> + * Returns: the number of translation blocks inserted into the region trees.
> + */
> size_t tcg_nb_tbs(void);
>
> /* user-mode: Called with mmap_lock held. */
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] tcg: Document tb_lookup() and tcg_tb_lookup()
2025-01-16 21:31 [PATCH v2 1/2] tcg: Document tb_lookup() and tcg_tb_lookup() Ilya Leoshkevich
2025-01-16 21:31 ` [PATCH v2 2/2] accel/tcg: Call tcg_tb_insert() for one-insn TBs Ilya Leoshkevich
2025-01-17 4:44 ` [PATCH v2 1/2] tcg: Document tb_lookup() and tcg_tb_lookup() Richard Henderson
@ 2025-01-17 10:38 ` Alex Bennée
2025-01-17 16:28 ` Richard Henderson
3 siblings, 0 replies; 6+ messages in thread
From: Alex Bennée @ 2025-01-17 10:38 UTC (permalink / raw)
To: Ilya Leoshkevich
Cc: Richard Henderson, Paolo Bonzini, Peter Maydell,
Philippe Mathieu-Daudé, qemu-devel
Ilya Leoshkevich <iii@linux.ibm.com> writes:
> These similarly named functions serve different purposes; add
> docstrings to highlight them.
>
> Suggested-by: Alex Bennée <alex.bennee@linaro.org>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] tcg: Document tb_lookup() and tcg_tb_lookup()
2025-01-16 21:31 [PATCH v2 1/2] tcg: Document tb_lookup() and tcg_tb_lookup() Ilya Leoshkevich
` (2 preceding siblings ...)
2025-01-17 10:38 ` Alex Bennée
@ 2025-01-17 16:28 ` Richard Henderson
3 siblings, 0 replies; 6+ messages in thread
From: Richard Henderson @ 2025-01-17 16:28 UTC (permalink / raw)
To: Ilya Leoshkevich, Paolo Bonzini, Alex Bennée, Peter Maydell,
Philippe Mathieu-Daudé
Cc: qemu-devel
On 1/16/25 13:31, Ilya Leoshkevich wrote:
> These similarly named functions serve different purposes; add
> docstrings to highlight them.
>
> Suggested-by: Alex Bennée <alex.bennee@linaro.org>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
> accel/tcg/cpu-exec.c | 15 ++++++++++++++-
> include/tcg/tcg.h | 41 +++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 55 insertions(+), 1 deletion(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
And queued, thanks.
r~
>
> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> index d48b82a9325..8b773d88478 100644
> --- a/accel/tcg/cpu-exec.c
> +++ b/accel/tcg/cpu-exec.c
> @@ -249,7 +249,20 @@ static TranslationBlock *tb_htable_lookup(CPUState *cpu, vaddr pc,
> return qht_lookup_custom(&tb_ctx.htable, &desc, h, tb_lookup_cmp);
> }
>
> -/* Might cause an exception, so have a longjmp destination ready */
> +/**
> + * tb_lookup:
> + * @cpu: CPU that will execute the returned translation block
> + * @pc: guest PC
> + * @cs_base: arch-specific value associated with translation block
> + * @flags: arch-specific translation block flags
> + * @cflags: CF_* flags
> + *
> + * Look up a translation block inside the QHT using @pc, @cs_base, @flags and
> + * @cflags. Uses @cpu's tb_jmp_cache. Might cause an exception, so have a
> + * longjmp destination ready.
> + *
> + * Returns: an existing translation block or NULL.
> + */
> static inline TranslationBlock *tb_lookup(CPUState *cpu, vaddr pc,
> uint64_t cs_base, uint32_t flags,
> uint32_t cflags)
> diff --git a/include/tcg/tcg.h b/include/tcg/tcg.h
> index a77ed12b9dc..057df6c4599 100644
> --- a/include/tcg/tcg.h
> +++ b/include/tcg/tcg.h
> @@ -746,10 +746,51 @@ void tcg_region_reset_all(void);
> size_t tcg_code_size(void);
> size_t tcg_code_capacity(void);
>
> +/**
> + * tcg_tb_insert:
> + * @tb: translation block to insert
> + *
> + * Insert @tb into the region trees.
> + */
> void tcg_tb_insert(TranslationBlock *tb);
> +
> +/**
> + * tcg_tb_remove:
> + * @tb: translation block to remove
> + *
> + * Remove @tb from the region trees.
> + */
> void tcg_tb_remove(TranslationBlock *tb);
> +
> +/**
> + * tcg_tb_lookup:
> + * @tc_ptr: host PC to look up
> + *
> + * Look up a translation block inside the region trees by @tc_ptr. This is
> + * useful for exception handling, but must not be used for the purposes of
> + * executing the returned translation block. See struct tb_tc for more
> + * information.
> + *
> + * Returns: a translation block previously inserted into the region trees,
> + * such that @tc_ptr points anywhere inside the code generated for it, or
> + * NULL.
> + */
> TranslationBlock *tcg_tb_lookup(uintptr_t tc_ptr);
> +
> +/**
> + * tcg_tb_foreach:
> + * @func: callback
> + * @user_data: opaque value to pass to @callback
> + *
> + * Call @func for each translation block inserted into the region trees.
> + */
> void tcg_tb_foreach(GTraverseFunc func, gpointer user_data);
> +
> +/**
> + * tcg_nb_tbs:
> + *
> + * Returns: the number of translation blocks inserted into the region trees.
> + */
> size_t tcg_nb_tbs(void);
>
> /* user-mode: Called with mmap_lock held. */
^ permalink raw reply [flat|nested] 6+ messages in thread