[PATCH 0/2] Fix defect in TranslationBlock insertion procedure

[PATCH 0/2] Fix defect in TranslationBlock insertion procedure

[Liren Wei]

TranslationBlocks are now inserted first into QHT and then into its
corresponding tcg_region_tree. This is problematic under MTTCG, as
other vCPU threads may immediately execute it, and even bailout before
the TB itself is inserted into its tcg_region_tree, resulting in an
incorrect CPUState after rewinding.

Liren Wei (2):
  accel/tcg: Hoist tcg_tb_insert() up above tb_link_page()
  tcg: Bake tb_destroy() into tcg_region_tree

 accel/tcg/translate-all.c | 15 ++++++++-------
 include/tcg/tcg.h         |  1 -
 tcg/region.c              | 18 +++++++-----------
 3 files changed, 15 insertions(+), 19 deletions(-)

-- 
2.32.0

[PATCH 1/2] accel/tcg: Hoist tcg_tb_insert() up above tb_link_page()

[Liren Wei]

TranslationBlocks not inserted into the corresponding region
tree shall be regarded as partially initialized objects, and
needs to be finalized first before inserting into QHT.

Signed-off-by: Liren Wei <lrwei@bupt.edu.cn>
---
 accel/tcg/translate-all.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index 7929a7e320..75e4d06557 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -1657,6 +1657,13 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
         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);
+
     /* check next page if needed */
     virt_page2 = (pc + tb->size - 1) & TARGET_PAGE_MASK;
     phys_page2 = -1;
@@ -1675,9 +1682,9 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
         orig_aligned -= ROUND_UP(sizeof(*tb), qemu_icache_linesize);
         qatomic_set(&tcg_ctx->code_gen_ptr, (void *)orig_aligned);
         tb_destroy(tb);
+        tcg_tb_remove(tb);
         return existing_tb;
     }
-    tcg_tb_insert(tb);
     return tb;
 }
 
-- 
2.32.0

Re: [PATCH 1/2] accel/tcg: Hoist tcg_tb_insert() up above tb_link_page()

[Richard Henderson]

On 7/4/21 7:31 AM, Liren Wei wrote:
> TranslationBlocks not inserted into the corresponding region
> tree shall be regarded as partially initialized objects, and
> needs to be finalized first before inserting into QHT.
> 
> Signed-off-by: Liren Wei<lrwei@bupt.edu.cn>
> ---
>   accel/tcg/translate-all.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)

Queued, thanks.

r~

[PATCH 2/2] tcg: Bake tb_destroy() into tcg_region_tree

[Liren Wei]

The function is called only at tcg_gen_code() when duplicated TBs
are translated by different threads, and when the tcg_region_tree
is reset. Bake it into the underlying GTree as its value destroy
function to unite these situations.
Also remove tcg_region_tree_traverse() which now becomes useless.

Signed-off-by: Liren Wei <lrwei@bupt.edu.cn>
---
 accel/tcg/translate-all.c |  6 ------
 include/tcg/tcg.h         |  1 -
 tcg/region.c              | 18 +++++++-----------
 3 files changed, 7 insertions(+), 18 deletions(-)

diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index 75e4d06557..57455d8639 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -378,11 +378,6 @@ static int cpu_restore_state_from_tb(CPUState *cpu, TranslationBlock *tb,
     return 0;
 }
 
-void tb_destroy(TranslationBlock *tb)
-{
-    qemu_spin_destroy(&tb->jmp_lock);
-}
-
 bool cpu_restore_state(CPUState *cpu, uintptr_t host_pc, bool will_exit)
 {
     /*
@@ -1681,7 +1676,6 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
 
         orig_aligned -= ROUND_UP(sizeof(*tb), qemu_icache_linesize);
         qatomic_set(&tcg_ctx->code_gen_ptr, (void *)orig_aligned);
-        tb_destroy(tb);
         tcg_tb_remove(tb);
         return existing_tb;
     }
diff --git a/include/tcg/tcg.h b/include/tcg/tcg.h
index 899493701c..dedb86939a 100644
--- a/include/tcg/tcg.h
+++ b/include/tcg/tcg.h
@@ -808,7 +808,6 @@ void *tcg_malloc_internal(TCGContext *s, int size);
 void tcg_pool_reset(TCGContext *s);
 TranslationBlock *tcg_tb_alloc(TCGContext *s);
 
-void tb_destroy(TranslationBlock *tb);
 void tcg_region_reset_all(void);
 
 size_t tcg_code_size(void);
diff --git a/tcg/region.c b/tcg/region.c
index 00b0c3b091..956a5ae483 100644
--- a/tcg/region.c
+++ b/tcg/region.c
@@ -112,7 +112,7 @@ static int ptr_cmp_tb_tc(const void *ptr, const struct tb_tc *s)
     return 0;
 }
 
-static gint tb_tc_cmp(gconstpointer ap, gconstpointer bp)
+static gint tb_tc_cmp(gconstpointer ap, gconstpointer bp, gpointer _)
 {
     const struct tb_tc *a = ap;
     const struct tb_tc *b = bp;
@@ -143,6 +143,11 @@ static gint tb_tc_cmp(gconstpointer ap, gconstpointer bp)
     return ptr_cmp_tb_tc(b->ptr, a);
 }
 
+static void tb_destroy(gpointer tb)
+{
+    qemu_spin_destroy(&((TranslationBlock *) tb)->jmp_lock);
+}
+
 static void tcg_region_trees_init(void)
 {
     size_t i;
@@ -153,7 +158,7 @@ static void tcg_region_trees_init(void)
         struct tcg_region_tree *rt = region_trees + i * tree_size;
 
         qemu_mutex_init(&rt->lock);
-        rt->tree = g_tree_new(tb_tc_cmp);
+        rt->tree = g_tree_new_full(tb_tc_cmp, NULL, NULL, tb_destroy);
     }
 }
 
@@ -277,14 +282,6 @@ size_t tcg_nb_tbs(void)
     return nb_tbs;
 }
 
-static gboolean tcg_region_tree_traverse(gpointer k, gpointer v, gpointer data)
-{
-    TranslationBlock *tb = v;
-
-    tb_destroy(tb);
-    return FALSE;
-}
-
 static void tcg_region_tree_reset_all(void)
 {
     size_t i;
@@ -293,7 +290,6 @@ static void tcg_region_tree_reset_all(void)
     for (i = 0; i < region.n; i++) {
         struct tcg_region_tree *rt = region_trees + i * tree_size;
 
-        g_tree_foreach(rt->tree, tcg_region_tree_traverse, NULL);
         /* Increment the refcount first so that destroy acts as a reset */
         g_tree_ref(rt->tree);
         g_tree_destroy(rt->tree);
-- 
2.32.0

Re: [PATCH 2/2] tcg: Bake tb_destroy() into tcg_region_tree

[Richard Henderson]

On 7/4/21 7:31 AM, Liren Wei wrote:
> -static gint tb_tc_cmp(gconstpointer ap, gconstpointer bp)
> +static gint tb_tc_cmp(gconstpointer ap, gconstpointer bp, gpointer _)

Using _ here as the variable name isn't ideal.  I guess if this were c++ we would actually 
omit the name, which is kinda the same.  But I think it's just as easy to name it 
userdata, as per glib docs.

I'll fix that up while queuing, thanks.

I'm not keen that the spinlock init and destroy are in different places, but surely that 
should be fixed by moving the init to tcg_tb_alloc, probably moving it to tcg/region.c as 
well.


r~

Re: [PATCH 2/2] tcg: Bake tb_destroy() into tcg_region_tree

[Liren Wei]

On 7/7/21 8:34 AM, Richard Henderson wrote:
> On 7/4/21 7:31 AM, Liren Wei wrote:
>> -static gint tb_tc_cmp(gconstpointer ap, gconstpointer bp)
>> +static gint tb_tc_cmp(gconstpointer ap, gconstpointer bp, gpointer _)
>
> Using _ here as the variable name isn't ideal.  I guess if this were 
> c++ we would actually omit the name, which is kinda the same.  But I 
> think it's just as easy to name it userdata, as per glib docs.
>
> I'll fix that up while queuing, thanks.
>
Got it, thanks.
> I'm not keen that the spinlock init and destroy are in different 
> places, but surely that should be fixed by moving the init to 
> tcg_tb_alloc, probably moving it to tcg/region.c as well.
>
>
> r~
Indeed, that would be much more clear. But I kind of feel that 
initialization of TB spinlock is deliberately placed after 
tcg_gen_code() in the current implementation to prevent buffer_overflow 
or any rewinding from leaking the initialized spinlock (, through it 
seems to me that there is nothing to leak for a spinlock whatsoever).

Liren Wei