[Qemu-devel] [PATCH 0/8] tcg: Direct block chaining clean-up

[Qemu-devel] [PATCH 0/8] tcg: Direct block chaining clean-up

[sergey.fedorov]

From: Sergey Fedorov <serge.fdrv@gmail.com>

This series combines a set of patches which is meant to improve overall code
structure and readability of direct block chaining mechanism. The other point
is to make a step towards thread safety of TB chainig.

The series' tree can be found in a public git repository [1].

[1] https://github.com/sergefdrv/qemu/tree/tb-chaining-cleanup

Sergey Fedorov (8):
  tcg: Clean up direct block chaining data fields
  tcg: Use uintptr_t type for jmp_list_{next|first} fields of TB
  tcg: Rearrange tb_link_page() to avoid forward declaration
  tcg: Init TB's direct jumps before making it visible
  tcg: Clarify "thread safaty" check in tb_add_jump()
  tcg: Rename tb_jmp_remove() to tb_remove_from_jmp_list()
  tcg: Extract removing of jumps to TB from tb_phys_invalidate()
  tcg: Clean up tb_jmp_unlink()

 include/exec/exec-all.h      |  51 +++++---
 tcg/aarch64/tcg-target.inc.c |   7 +-
 tcg/arm/tcg-target.inc.c     |   8 +-
 tcg/i386/tcg-target.inc.c    |   8 +-
 tcg/ia64/tcg-target.inc.c    |   6 +-
 tcg/mips/tcg-target.inc.c    |   8 +-
 tcg/ppc/tcg-target.inc.c     |   6 +-
 tcg/s390/tcg-target.inc.c    |  11 +-
 tcg/sparc/tcg-target.inc.c   |   9 +-
 tcg/tcg.h                    |   6 +-
 tcg/tci/tcg-target.inc.c     |  10 +-
 translate-all.c              | 290 ++++++++++++++++++++++---------------------
 12 files changed, 224 insertions(+), 196 deletions(-)

-- 
2.7.3

[Qemu-devel] [PATCH 1/8] tcg: Clean up direct block chaining data fields

[sergey.fedorov]

From: Sergey Fedorov <serge.fdrv@gmail.com>

Briefly describe in a comment how direct block chaining is done. It
should help in understanding of the following data fields.

Rename some fields in TranslationBlock and TCGContext structures to
better reflect their purpose (dropping excessive 'tb_' prefix in
TranslationBlock but keeping it in TCGContext):
   tb_next_offset  =>  jmp_reset_offset
   tb_jmp_offset   =>  jmp_insn_offset
   tb_next         =>  jmp_target_addr
   jmp_next        =>  jmp_list_next
   jmp_first       =>  jmp_list_first

Avoid using a magic constant as an invalid offset which is used to
indicate that there's no n-th jump generated.

Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com>
Signed-off-by: Sergey Fedorov <sergey.fedorov@linaro.org>
---
 include/exec/exec-all.h      | 44 ++++++++++++++++++++++++--------------
 tcg/aarch64/tcg-target.inc.c |  7 +++---
 tcg/arm/tcg-target.inc.c     |  8 +++----
 tcg/i386/tcg-target.inc.c    |  8 +++----
 tcg/ia64/tcg-target.inc.c    |  6 +++---
 tcg/mips/tcg-target.inc.c    |  8 +++----
 tcg/ppc/tcg-target.inc.c     |  6 +++---
 tcg/s390/tcg-target.inc.c    | 11 +++++-----
 tcg/sparc/tcg-target.inc.c   |  9 ++++----
 tcg/tcg.h                    |  6 +++---
 tcg/tci/tcg-target.inc.c     | 10 ++++-----
 translate-all.c              | 51 +++++++++++++++++++++++---------------------
 12 files changed, 96 insertions(+), 78 deletions(-)

diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index 05a151da4a54..cc3d2ca25917 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -257,20 +257,32 @@ struct TranslationBlock {
     struct TranslationBlock *page_next[2];
     tb_page_addr_t page_addr[2];
 
-    /* the following data are used to directly call another TB from
-       the code of this one. */
-    uint16_t tb_next_offset[2]; /* offset of original jump target */
+    /* The following data are used to directly call another TB from
+     * the code of this one. This can be done either by emitting direct or
+     * indirect native jump instructions. These jumps are reset so that the TB
+     * just continue its execution. The TB can be linked to another one by
+     * setting one of the jump targets (or patching the jump instruction). Only
+     * two of such jumps are supported.
+     */
+    uint16_t jmp_reset_offset[2]; /* offset of original jump target */
+#define TB_JMP_RESET_OFFSET_INVALID 0xffff /* indicates no jump generated */
 #ifdef USE_DIRECT_JUMP
-    uint16_t tb_jmp_offset[2]; /* offset of jump instruction */
+    uint16_t jmp_insn_offset[2]; /* offset of native jump instruction */
 #else
-    uintptr_t tb_next[2]; /* address of jump generated code */
+    uintptr_t jmp_target_addr[2]; /* target address for indirect jump */
 #endif
-    /* list of TBs jumping to this one. This is a circular list using
-       the two least significant bits of the pointers to tell what is
-       the next pointer: 0 = jmp_next[0], 1 = jmp_next[1], 2 =
-       jmp_first */
-    struct TranslationBlock *jmp_next[2];
-    struct TranslationBlock *jmp_first;
+    /* Each TB has an assosiated circular list of TBs jumping to this one.
+     * jmp_list_first points to the first TB jumping to this one.
+     * jmp_list_next is used to point to the next TB in a list.
+     * Since each TB can have two jumps, it can participate in two lists.
+     * The two least significant bits of a pointer are used to choose which
+     * data field holds a pointer to the next TB:
+     * 0 => jmp_list_next[0], 1 => jmp_list_next[1], 2 => jmp_list_first.
+     * In other words, 0/1 tells which jump is used in the pointed TB,
+     * and 2 means that this is a pointer back to the target TB of this list.
+     */
+    struct TranslationBlock *jmp_list_next[2];
+    struct TranslationBlock *jmp_list_first;
 };
 
 #include "qemu/thread.h"
@@ -359,7 +371,7 @@ void tb_set_jmp_target1(uintptr_t jmp_addr, uintptr_t addr);
 static inline void tb_set_jmp_target(TranslationBlock *tb,
                                      int n, uintptr_t addr)
 {
-    uint16_t offset = tb->tb_jmp_offset[n];
+    uint16_t offset = tb->jmp_insn_offset[n];
     tb_set_jmp_target1((uintptr_t)(tb->tc_ptr + offset), addr);
 }
 
@@ -369,7 +381,7 @@ static inline void tb_set_jmp_target(TranslationBlock *tb,
 static inline void tb_set_jmp_target(TranslationBlock *tb,
                                      int n, uintptr_t addr)
 {
-    tb->tb_next[n] = addr;
+    tb->jmp_target_addr[n] = addr;
 }
 
 #endif
@@ -378,13 +390,13 @@ static inline void tb_add_jump(TranslationBlock *tb, int n,
                                TranslationBlock *tb_next)
 {
     /* NOTE: this test is only needed for thread safety */
-    if (!tb->jmp_next[n]) {
+    if (!tb->jmp_list_next[n]) {
         /* patch the native jump address */
         tb_set_jmp_target(tb, n, (uintptr_t)tb_next->tc_ptr);
 
         /* add in TB jmp circular list */
-        tb->jmp_next[n] = tb_next->jmp_first;
-        tb_next->jmp_first = (TranslationBlock *)((uintptr_t)(tb) | (n));
+        tb->jmp_list_next[n] = tb_next->jmp_list_first;
+        tb_next->jmp_list_first = (TranslationBlock *)((uintptr_t)tb | n);
     }
 }
 
diff --git a/tcg/aarch64/tcg-target.inc.c b/tcg/aarch64/tcg-target.inc.c
index 0ed10a974121..08efdf41da48 100644
--- a/tcg/aarch64/tcg-target.inc.c
+++ b/tcg/aarch64/tcg-target.inc.c
@@ -1294,12 +1294,13 @@ static void tcg_out_op(TCGContext *s, TCGOpcode opc,
 #ifndef USE_DIRECT_JUMP
 #error "USE_DIRECT_JUMP required for aarch64"
 #endif
-        assert(s->tb_jmp_offset != NULL); /* consistency for USE_DIRECT_JUMP */
-        s->tb_jmp_offset[a0] = tcg_current_code_size(s);
+        /* consistency for USE_DIRECT_JUMP */
+        assert(s->tb_jmp_insn_offset != NULL);
+        s->tb_jmp_insn_offset[a0] = tcg_current_code_size(s);
         /* actual branch destination will be patched by
            aarch64_tb_set_jmp_target later, beware retranslation. */
         tcg_out_goto_noaddr(s);
-        s->tb_next_offset[a0] = tcg_current_code_size(s);
+        s->tb_jmp_reset_offset[a0] = tcg_current_code_size(s);
         break;
 
     case INDEX_op_br:
diff --git a/tcg/arm/tcg-target.inc.c b/tcg/arm/tcg-target.inc.c
index 3edf6a6f971c..a9147620b073 100644
--- a/tcg/arm/tcg-target.inc.c
+++ b/tcg/arm/tcg-target.inc.c
@@ -1647,17 +1647,17 @@ static inline void tcg_out_op(TCGContext *s, TCGOpcode opc,
         tcg_out_goto(s, COND_AL, tb_ret_addr);
         break;
     case INDEX_op_goto_tb:
-        if (s->tb_jmp_offset) {
+        if (s->tb_jmp_insn_offset) {
             /* Direct jump method */
-            s->tb_jmp_offset[args[0]] = tcg_current_code_size(s);
+            s->tb_jmp_insn_offset[args[0]] = tcg_current_code_size(s);
             tcg_out_b_noaddr(s, COND_AL);
         } else {
             /* Indirect jump method */
-            intptr_t ptr = (intptr_t)(s->tb_next + args[0]);
+            intptr_t ptr = (intptr_t)(s->tb_jmp_target_addr + args[0]);
             tcg_out_movi32(s, COND_AL, TCG_REG_R0, ptr & ~0xfff);
             tcg_out_ld32_12(s, COND_AL, TCG_REG_PC, TCG_REG_R0, ptr & 0xfff);
         }
-        s->tb_next_offset[args[0]] = tcg_current_code_size(s);
+        s->tb_jmp_reset_offset[args[0]] = tcg_current_code_size(s);
         break;
     case INDEX_op_br:
         tcg_out_goto_label(s, COND_AL, arg_label(args[0]));
diff --git a/tcg/i386/tcg-target.inc.c b/tcg/i386/tcg-target.inc.c
index 9187d34caf6d..2f98cae97f3b 100644
--- a/tcg/i386/tcg-target.inc.c
+++ b/tcg/i386/tcg-target.inc.c
@@ -1775,17 +1775,17 @@ static inline void tcg_out_op(TCGContext *s, TCGOpcode opc,
         tcg_out_jmp(s, tb_ret_addr);
         break;
     case INDEX_op_goto_tb:
-        if (s->tb_jmp_offset) {
+        if (s->tb_jmp_insn_offset) {
             /* direct jump method */
             tcg_out8(s, OPC_JMP_long); /* jmp im */
-            s->tb_jmp_offset[args[0]] = tcg_current_code_size(s);
+            s->tb_jmp_insn_offset[args[0]] = tcg_current_code_size(s);
             tcg_out32(s, 0);
         } else {
             /* indirect jump method */
             tcg_out_modrm_offset(s, OPC_GRP5, EXT5_JMPN_Ev, -1,
-                                 (intptr_t)(s->tb_next + args[0]));
+                                 (intptr_t)(s->tb_jmp_target_addr + args[0]));
         }
-        s->tb_next_offset[args[0]] = tcg_current_code_size(s);
+        s->tb_jmp_reset_offset[args[0]] = tcg_current_code_size(s);
         break;
     case INDEX_op_br:
         tcg_out_jxx(s, JCC_JMP, arg_label(args[0]), 0);
diff --git a/tcg/ia64/tcg-target.inc.c b/tcg/ia64/tcg-target.inc.c
index 62d654943c20..261861f90c3a 100644
--- a/tcg/ia64/tcg-target.inc.c
+++ b/tcg/ia64/tcg-target.inc.c
@@ -881,13 +881,13 @@ static void tcg_out_exit_tb(TCGContext *s, tcg_target_long arg)
 
 static inline void tcg_out_goto_tb(TCGContext *s, TCGArg arg)
 {
-    if (s->tb_jmp_offset) {
+    if (s->tb_jmp_insn_offset) {
         /* direct jump method */
         tcg_abort();
     } else {
         /* indirect jump method */
         tcg_out_movi(s, TCG_TYPE_PTR, TCG_REG_R2,
-                     (tcg_target_long)(s->tb_next + arg));
+                     (tcg_target_long)(s->tb_jmp_target_addr + arg));
         tcg_out_bundle(s, MmI,
                        tcg_opc_m1 (TCG_REG_P0, OPC_LD8_M1,
                                    TCG_REG_R2, TCG_REG_R2),
@@ -900,7 +900,7 @@ static inline void tcg_out_goto_tb(TCGContext *s, TCGArg arg)
                        tcg_opc_b4 (TCG_REG_P0, OPC_BR_SPTK_MANY_B4,
                                    TCG_REG_B6));
     }
-    s->tb_next_offset[arg] = tcg_current_code_size(s);
+    s->tb_jmp_reset_offset[arg] = tcg_current_code_size(s);
 }
 
 static inline void tcg_out_jmp(TCGContext *s, TCGArg addr)
diff --git a/tcg/mips/tcg-target.inc.c b/tcg/mips/tcg-target.inc.c
index 297bd00910b7..b2a839ac2cd3 100644
--- a/tcg/mips/tcg-target.inc.c
+++ b/tcg/mips/tcg-target.inc.c
@@ -1397,19 +1397,19 @@ static inline void tcg_out_op(TCGContext *s, TCGOpcode opc,
         }
         break;
     case INDEX_op_goto_tb:
-        if (s->tb_jmp_offset) {
+        if (s->tb_jmp_insn_offset) {
             /* direct jump method */
-            s->tb_jmp_offset[a0] = tcg_current_code_size(s);
+            s->tb_jmp_insn_offset[a0] = tcg_current_code_size(s);
             /* Avoid clobbering the address during retranslation.  */
             tcg_out32(s, OPC_J | (*(uint32_t *)s->code_ptr & 0x3ffffff));
         } else {
             /* indirect jump method */
             tcg_out_ld(s, TCG_TYPE_PTR, TCG_TMP0, TCG_REG_ZERO,
-                       (uintptr_t)(s->tb_next + a0));
+                       (uintptr_t)(s->tb_jmp_target_addr + a0));
             tcg_out_opc_reg(s, OPC_JR, 0, TCG_TMP0, 0);
         }
         tcg_out_nop(s);
-        s->tb_next_offset[a0] = tcg_current_code_size(s);
+        s->tb_jmp_reset_offset[a0] = tcg_current_code_size(s);
         break;
     case INDEX_op_br:
         tcg_out_brcond(s, TCG_COND_EQ, TCG_REG_ZERO, TCG_REG_ZERO,
diff --git a/tcg/ppc/tcg-target.inc.c b/tcg/ppc/tcg-target.inc.c
index 8c1c2dfa9b22..10394079ad9d 100644
--- a/tcg/ppc/tcg-target.inc.c
+++ b/tcg/ppc/tcg-target.inc.c
@@ -1894,17 +1894,17 @@ static void tcg_out_op(TCGContext *s, TCGOpcode opc, const TCGArg *args,
         tcg_out_b(s, 0, tb_ret_addr);
         break;
     case INDEX_op_goto_tb:
-        tcg_debug_assert(s->tb_jmp_offset);
+        tcg_debug_assert(s->tb_jmp_insn_offset);
         /* Direct jump.  Ensure the next insns are 8-byte aligned. */
         if ((uintptr_t)s->code_ptr & 7) {
             tcg_out32(s, NOP);
         }
-        s->tb_jmp_offset[args[0]] = tcg_current_code_size(s);
+        s->tb_jmp_insn_offset[args[0]] = tcg_current_code_size(s);
         /* To be replaced by either a branch+nop or a load into TMP1.  */
         s->code_ptr += 2;
         tcg_out32(s, MTSPR | RS(TCG_REG_TMP1) | CTR);
         tcg_out32(s, BCCTR | BO_ALWAYS);
-        s->tb_next_offset[args[0]] = tcg_current_code_size(s);
+        s->tb_jmp_reset_offset[args[0]] = tcg_current_code_size(s);
         break;
     case INDEX_op_br:
         {
diff --git a/tcg/s390/tcg-target.inc.c b/tcg/s390/tcg-target.inc.c
index fbf97bb2e15d..e95b04b0e278 100644
--- a/tcg/s390/tcg-target.inc.c
+++ b/tcg/s390/tcg-target.inc.c
@@ -1715,17 +1715,18 @@ static inline void tcg_out_op(TCGContext *s, TCGOpcode opc,
         break;
 
     case INDEX_op_goto_tb:
-        if (s->tb_jmp_offset) {
+        if (s->tb_jmp_insn_offset) {
             tcg_out16(s, RIL_BRCL | (S390_CC_ALWAYS << 4));
-            s->tb_jmp_offset[args[0]] = tcg_current_code_size(s);
+            s->tb_jmp_insn_offset[args[0]] = tcg_current_code_size(s);
             s->code_ptr += 2;
         } else {
-            /* load address stored at s->tb_next + args[0] */
-            tcg_out_ld_abs(s, TCG_TYPE_PTR, TCG_TMP0, s->tb_next + args[0]);
+            /* load address stored at s->tb_jmp_target_addr + args[0] */
+            tcg_out_ld_abs(s, TCG_TYPE_PTR, TCG_TMP0,
+                           s->tb_jmp_target_addr + args[0]);
             /* and go there */
             tcg_out_insn(s, RR, BCR, S390_CC_ALWAYS, TCG_TMP0);
         }
-        s->tb_next_offset[args[0]] = tcg_current_code_size(s);
+        s->tb_jmp_reset_offset[args[0]] = tcg_current_code_size(s);
         break;
 
     OP_32_64(ld8u):
diff --git a/tcg/sparc/tcg-target.inc.c b/tcg/sparc/tcg-target.inc.c
index 54df1bc42432..a611885a2aaf 100644
--- a/tcg/sparc/tcg-target.inc.c
+++ b/tcg/sparc/tcg-target.inc.c
@@ -1229,18 +1229,19 @@ static void tcg_out_op(TCGContext *s, TCGOpcode opc,
         }
         break;
     case INDEX_op_goto_tb:
-        if (s->tb_jmp_offset) {
+        if (s->tb_jmp_insn_offset) {
             /* direct jump method */
-            s->tb_jmp_offset[a0] = tcg_current_code_size(s);
+            s->tb_jmp_insn_offset[a0] = tcg_current_code_size(s);
             /* Make sure to preserve links during retranslation.  */
             tcg_out32(s, CALL | (*s->code_ptr & ~INSN_OP(-1)));
         } else {
             /* indirect jump method */
-            tcg_out_ld_ptr(s, TCG_REG_T1, (uintptr_t)(s->tb_next + a0));
+            tcg_out_ld_ptr(s, TCG_REG_T1,
+                           (uintptr_t)(s->tb_jmp_target_addr + a0));
             tcg_out_arithi(s, TCG_REG_G0, TCG_REG_T1, 0, JMPL);
         }
         tcg_out_nop(s);
-        s->tb_next_offset[a0] = tcg_current_code_size(s);
+        s->tb_jmp_reset_offset[a0] = tcg_current_code_size(s);
         break;
     case INDEX_op_br:
         tcg_out_bpcc(s, COND_A, BPCC_PT, arg_label(a0));
diff --git a/tcg/tcg.h b/tcg/tcg.h
index b83f76351ccd..f9d11b29442b 100644
--- a/tcg/tcg.h
+++ b/tcg/tcg.h
@@ -510,9 +510,9 @@ struct TCGContext {
 
     /* goto_tb support */
     tcg_insn_unit *code_buf;
-    uintptr_t *tb_next;
-    uint16_t *tb_next_offset;
-    uint16_t *tb_jmp_offset; /* != NULL if USE_DIRECT_JUMP */
+    uint16_t *tb_jmp_reset_offset; /* tb->jmp_reset_offset */
+    uint16_t *tb_jmp_insn_offset; /* tb->jmp_insn_offset if USE_DIRECT_JUMP */
+    uintptr_t *tb_jmp_target_addr; /* tb->jmp_target_addr if !USE_DIRECT_JUMP */
 
     /* liveness analysis */
     uint16_t *op_dead_args; /* for each operation, each bit tells if the
diff --git a/tcg/tci/tcg-target.inc.c b/tcg/tci/tcg-target.inc.c
index 4afe4d7a8d59..4e91687abd1c 100644
--- a/tcg/tci/tcg-target.inc.c
+++ b/tcg/tci/tcg-target.inc.c
@@ -553,17 +553,17 @@ static void tcg_out_op(TCGContext *s, TCGOpcode opc, const TCGArg *args,
         tcg_out64(s, args[0]);
         break;
     case INDEX_op_goto_tb:
-        if (s->tb_jmp_offset) {
+        if (s->tb_jmp_insn_offset) {
             /* Direct jump method. */
-            assert(args[0] < ARRAY_SIZE(s->tb_jmp_offset));
-            s->tb_jmp_offset[args[0]] = tcg_current_code_size(s);
+            assert(args[0] < ARRAY_SIZE(s->tb_jmp_insn_offset));
+            s->tb_jmp_insn_offset[args[0]] = tcg_current_code_size(s);
             tcg_out32(s, 0);
         } else {
             /* Indirect jump method. */
             TODO();
         }
-        assert(args[0] < ARRAY_SIZE(s->tb_next_offset));
-        s->tb_next_offset[args[0]] = tcg_current_code_size(s);
+        assert(args[0] < ARRAY_SIZE(s->tb_jmp_reset_offset));
+        s->tb_jmp_reset_offset[args[0]] = tcg_current_code_size(s);
         break;
     case INDEX_op_br:
         tci_out_label(s, arg_label(args[0]));
diff --git a/translate-all.c b/translate-all.c
index e9f409b762ab..31cdf8ae217e 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -929,7 +929,7 @@ static inline void tb_jmp_remove(TranslationBlock *tb, int n)
     TranslationBlock *tb1, **ptb;
     unsigned int n1;
 
-    ptb = &tb->jmp_next[n];
+    ptb = &tb->jmp_list_next[n];
     tb1 = *ptb;
     if (tb1) {
         /* find tb(n) in circular list */
@@ -941,15 +941,15 @@ static inline void tb_jmp_remove(TranslationBlock *tb, int n)
                 break;
             }
             if (n1 == 2) {
-                ptb = &tb1->jmp_first;
+                ptb = &tb1->jmp_list_first;
             } else {
-                ptb = &tb1->jmp_next[n1];
+                ptb = &tb1->jmp_list_next[n1];
             }
         }
         /* now we can suppress tb(n) from the list */
-        *ptb = tb->jmp_next[n];
+        *ptb = tb->jmp_list_next[n];
 
-        tb->jmp_next[n] = NULL;
+        tb->jmp_list_next[n] = NULL;
     }
 }
 
@@ -957,7 +957,8 @@ static inline void tb_jmp_remove(TranslationBlock *tb, int n)
    another TB */
 static inline void tb_reset_jump(TranslationBlock *tb, int n)
 {
-    tb_set_jmp_target(tb, n, (uintptr_t)(tb->tc_ptr + tb->tb_next_offset[n]));
+    uintptr_t addr = (uintptr_t)(tb->tc_ptr + tb->jmp_reset_offset[n]);
+    tb_set_jmp_target(tb, n, addr);
 }
 
 /* invalidate one TB */
@@ -1001,19 +1002,21 @@ void tb_phys_invalidate(TranslationBlock *tb, tb_page_addr_t page_addr)
     tb_jmp_remove(tb, 1);
 
     /* suppress any remaining jumps to this TB */
-    tb1 = tb->jmp_first;
+    tb1 = tb->jmp_list_first;
     for (;;) {
         n1 = (uintptr_t)tb1 & 3;
         if (n1 == 2) {
             break;
         }
         tb1 = (TranslationBlock *)((uintptr_t)tb1 & ~3);
-        tb2 = tb1->jmp_next[n1];
+        tb2 = tb1->jmp_list_next[n1];
         tb_reset_jump(tb1, n1);
-        tb1->jmp_next[n1] = NULL;
+        tb1->jmp_list_next[n1] = NULL;
         tb1 = tb2;
     }
-    tb->jmp_first = (TranslationBlock *)((uintptr_t)tb | 2); /* fail safe */
+
+    /* fail safe */
+    tb->jmp_list_first = (TranslationBlock *)((uintptr_t)tb | 2);
 
     tcg_ctx.tb_ctx.tb_phys_invalidate_count++;
 }
@@ -1098,15 +1101,15 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
     trace_translate_block(tb, tb->pc, tb->tc_ptr);
 
     /* generate machine code */
-    tb->tb_next_offset[0] = 0xffff;
-    tb->tb_next_offset[1] = 0xffff;
-    tcg_ctx.tb_next_offset = tb->tb_next_offset;
+    tb->jmp_reset_offset[0] = TB_JMP_RESET_OFFSET_INVALID;
+    tb->jmp_reset_offset[1] = TB_JMP_RESET_OFFSET_INVALID;
+    tcg_ctx.tb_jmp_reset_offset = tb->jmp_reset_offset;
 #ifdef USE_DIRECT_JUMP
-    tcg_ctx.tb_jmp_offset = tb->tb_jmp_offset;
-    tcg_ctx.tb_next = NULL;
+    tcg_ctx.tb_jmp_insn_offset = tb->jmp_insn_offset;
+    tcg_ctx.tb_jmp_target_addr = NULL;
 #else
-    tcg_ctx.tb_jmp_offset = NULL;
-    tcg_ctx.tb_next = tb->tb_next;
+    tcg_ctx.tb_jmp_insn_offset = NULL;
+    tcg_ctx.tb_jmp_target_addr = tb->jmp_target_addr;
 #endif
 
 #ifdef CONFIG_PROFILER
@@ -1486,15 +1489,15 @@ static void tb_link_page(TranslationBlock *tb, tb_page_addr_t phys_pc,
         tb->page_addr[1] = -1;
     }
 
-    tb->jmp_first = (TranslationBlock *)((uintptr_t)tb | 2);
-    tb->jmp_next[0] = NULL;
-    tb->jmp_next[1] = NULL;
+    tb->jmp_list_first = (TranslationBlock *)((uintptr_t)tb | 2);
+    tb->jmp_list_next[0] = NULL;
+    tb->jmp_list_next[1] = NULL;
 
     /* init original jump addresses */
-    if (tb->tb_next_offset[0] != 0xffff) {
+    if (tb->jmp_reset_offset[0] != TB_JMP_RESET_OFFSET_INVALID) {
         tb_reset_jump(tb, 0);
     }
-    if (tb->tb_next_offset[1] != 0xffff) {
+    if (tb->jmp_reset_offset[1] != TB_JMP_RESET_OFFSET_INVALID) {
         tb_reset_jump(tb, 1);
     }
 
@@ -1687,9 +1690,9 @@ void dump_exec_info(FILE *f, fprintf_function cpu_fprintf)
         if (tb->page_addr[1] != -1) {
             cross_page++;
         }
-        if (tb->tb_next_offset[0] != 0xffff) {
+        if (tb->jmp_reset_offset[0] != TB_JMP_RESET_OFFSET_INVALID) {
             direct_jmp_count++;
-            if (tb->tb_next_offset[1] != 0xffff) {
+            if (tb->jmp_reset_offset[1] != TB_JMP_RESET_OFFSET_INVALID) {
                 direct_jmp2_count++;
             }
         }
-- 
2.7.3

[Qemu-devel] [PATCH 2/8] tcg: Use uintptr_t type for jmp_list_{next|first} fields of TB

[sergey.fedorov]

From: Sergey Fedorov <serge.fdrv@gmail.com>

These fields do not contain pure pointers to a TranslationBlock
structure. So uintptr_t is the most appropriate type for them.
Also put an assert to assure that the two least significant bits of the
pointer are zero before assigning it to jmp_list_first.

Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com>
Signed-off-by: Sergey Fedorov <sergey.fedorov@linaro.org>
---
 include/exec/exec-all.h | 12 +++++++-----
 translate-all.c         | 37 +++++++++++++++++++------------------
 2 files changed, 26 insertions(+), 23 deletions(-)

diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index cc3d2ca25917..cd96219a89e7 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -275,14 +275,15 @@ struct TranslationBlock {
      * jmp_list_first points to the first TB jumping to this one.
      * jmp_list_next is used to point to the next TB in a list.
      * Since each TB can have two jumps, it can participate in two lists.
-     * The two least significant bits of a pointer are used to choose which
-     * data field holds a pointer to the next TB:
+     * jmp_list_first and jmp_list_next are 4-byte aligned pointers to a
+     * TranslationBlock structure, and the two least significant bits of them
+     * are used to encode which data field holds a pointer to the next TB:
      * 0 => jmp_list_next[0], 1 => jmp_list_next[1], 2 => jmp_list_first.
      * In other words, 0/1 tells which jump is used in the pointed TB,
      * and 2 means that this is a pointer back to the target TB of this list.
      */
-    struct TranslationBlock *jmp_list_next[2];
-    struct TranslationBlock *jmp_list_first;
+    uintptr_t jmp_list_next[2];
+    uintptr_t jmp_list_first;
 };
 
 #include "qemu/thread.h"
@@ -396,7 +397,8 @@ static inline void tb_add_jump(TranslationBlock *tb, int n,
 
         /* add in TB jmp circular list */
         tb->jmp_list_next[n] = tb_next->jmp_list_first;
-        tb_next->jmp_list_first = (TranslationBlock *)((uintptr_t)tb | n);
+        assert(((uintptr_t)tb & 3) == 0);
+        tb_next->jmp_list_first = (uintptr_t)tb | n;
     }
 }
 
diff --git a/translate-all.c b/translate-all.c
index 31cdf8ae217e..7c008927e3f3 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -926,17 +926,16 @@ static inline void tb_page_remove(TranslationBlock **ptb, TranslationBlock *tb)
 
 static inline void tb_jmp_remove(TranslationBlock *tb, int n)
 {
-    TranslationBlock *tb1, **ptb;
+    TranslationBlock *tb1;
+    uintptr_t *ptb;
     unsigned int n1;
 
     ptb = &tb->jmp_list_next[n];
-    tb1 = *ptb;
-    if (tb1) {
+    if (*ptb) {
         /* find tb(n) in circular list */
         for (;;) {
-            tb1 = *ptb;
-            n1 = (uintptr_t)tb1 & 3;
-            tb1 = (TranslationBlock *)((uintptr_t)tb1 & ~3);
+            n1 = *ptb & 3;
+            tb1 = (TranslationBlock *)(*ptb & ~3);
             if (n1 == n && tb1 == tb) {
                 break;
             }
@@ -949,7 +948,7 @@ static inline void tb_jmp_remove(TranslationBlock *tb, int n)
         /* now we can suppress tb(n) from the list */
         *ptb = tb->jmp_list_next[n];
 
-        tb->jmp_list_next[n] = NULL;
+        tb->jmp_list_next[n] = (uintptr_t)NULL;
     }
 }
 
@@ -968,7 +967,7 @@ void tb_phys_invalidate(TranslationBlock *tb, tb_page_addr_t page_addr)
     PageDesc *p;
     unsigned int h, n1;
     tb_page_addr_t phys_pc;
-    TranslationBlock *tb1, *tb2;
+    uintptr_t tb1, tb2;
 
     /* remove the TB from the hash list */
     phys_pc = tb->page_addr[0] + (tb->pc & ~TARGET_PAGE_MASK);
@@ -1004,19 +1003,20 @@ void tb_phys_invalidate(TranslationBlock *tb, tb_page_addr_t page_addr)
     /* suppress any remaining jumps to this TB */
     tb1 = tb->jmp_list_first;
     for (;;) {
-        n1 = (uintptr_t)tb1 & 3;
+        TranslationBlock *tmp_tb;
+        n1 = tb1 & 3;
         if (n1 == 2) {
             break;
         }
-        tb1 = (TranslationBlock *)((uintptr_t)tb1 & ~3);
-        tb2 = tb1->jmp_list_next[n1];
-        tb_reset_jump(tb1, n1);
-        tb1->jmp_list_next[n1] = NULL;
+        tmp_tb = (TranslationBlock *)(tb1 & ~3);
+        tb2 = tmp_tb->jmp_list_next[n1];
+        tb_reset_jump(tmp_tb, n1);
+        tmp_tb->jmp_list_next[n1] = (uintptr_t)NULL;
         tb1 = tb2;
     }
 
-    /* fail safe */
-    tb->jmp_list_first = (TranslationBlock *)((uintptr_t)tb | 2);
+    assert(((uintptr_t)tb & 3) == 0);
+    tb->jmp_list_first = (uintptr_t)tb | 2; /* fail safe */
 
     tcg_ctx.tb_ctx.tb_phys_invalidate_count++;
 }
@@ -1489,9 +1489,10 @@ static void tb_link_page(TranslationBlock *tb, tb_page_addr_t phys_pc,
         tb->page_addr[1] = -1;
     }
 
-    tb->jmp_list_first = (TranslationBlock *)((uintptr_t)tb | 2);
-    tb->jmp_list_next[0] = NULL;
-    tb->jmp_list_next[1] = NULL;
+    assert(((uintptr_t)tb & 3) == 0);
+    tb->jmp_list_first = (uintptr_t)tb | 2;
+    tb->jmp_list_next[0] = (uintptr_t)NULL;
+    tb->jmp_list_next[1] = (uintptr_t)NULL;
 
     /* init original jump addresses */
     if (tb->jmp_reset_offset[0] != TB_JMP_RESET_OFFSET_INVALID) {
-- 
2.7.3

[Qemu-devel] [PATCH 3/8] tcg: Rearrange tb_link_page() to avoid forward declaration

[sergey.fedorov]

From: Sergey Fedorov <serge.fdrv@gmail.com>

Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com>
Signed-off-by: Sergey Fedorov <sergey.fedorov@linaro.org>
---
 translate-all.c | 204 ++++++++++++++++++++++++++++----------------------------
 1 file changed, 101 insertions(+), 103 deletions(-)

diff --git a/translate-all.c b/translate-all.c
index 7c008927e3f3..ca01dd325b8d 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -153,8 +153,6 @@ void tb_lock_reset(void)
 #endif
 }
 
-static void tb_link_page(TranslationBlock *tb, tb_page_addr_t phys_pc,
-                         tb_page_addr_t phys_page2);
 static TranslationBlock *tb_find_pc(uintptr_t tc_ptr);
 
 void cpu_gen_init(void)
@@ -1050,6 +1048,107 @@ static void build_page_bitmap(PageDesc *p)
     }
 }
 
+/* add the tb in the target page and protect it if necessary
+ *
+ * Called with mmap_lock held for user-mode emulation.
+ */
+static inline void tb_alloc_page(TranslationBlock *tb,
+                                 unsigned int n, tb_page_addr_t page_addr)
+{
+    PageDesc *p;
+#ifndef CONFIG_USER_ONLY
+    bool page_already_protected;
+#endif
+
+    tb->page_addr[n] = page_addr;
+    p = page_find_alloc(page_addr >> TARGET_PAGE_BITS, 1);
+    tb->page_next[n] = p->first_tb;
+#ifndef CONFIG_USER_ONLY
+    page_already_protected = p->first_tb != NULL;
+#endif
+    p->first_tb = (TranslationBlock *)((uintptr_t)tb | n);
+    invalidate_page_bitmap(p);
+
+#if defined(CONFIG_USER_ONLY)
+    if (p->flags & PAGE_WRITE) {
+        target_ulong addr;
+        PageDesc *p2;
+        int prot;
+
+        /* force the host page as non writable (writes will have a
+           page fault + mprotect overhead) */
+        page_addr &= qemu_host_page_mask;
+        prot = 0;
+        for (addr = page_addr; addr < page_addr + qemu_host_page_size;
+            addr += TARGET_PAGE_SIZE) {
+
+            p2 = page_find(addr >> TARGET_PAGE_BITS);
+            if (!p2) {
+                continue;
+            }
+            prot |= p2->flags;
+            p2->flags &= ~PAGE_WRITE;
+          }
+        mprotect(g2h(page_addr), qemu_host_page_size,
+                 (prot & PAGE_BITS) & ~PAGE_WRITE);
+#ifdef DEBUG_TB_INVALIDATE
+        printf("protecting code page: 0x" TARGET_FMT_lx "\n",
+               page_addr);
+#endif
+    }
+#else
+    /* if some code is already present, then the pages are already
+       protected. So we handle the case where only the first TB is
+       allocated in a physical page */
+    if (!page_already_protected) {
+        tlb_protect_code(page_addr);
+    }
+#endif
+}
+
+/* add a new TB and link it to the physical page tables. phys_page2 is
+ * (-1) to indicate that only one page contains the TB.
+ *
+ * Called with mmap_lock held for user-mode emulation.
+ */
+static void tb_link_page(TranslationBlock *tb, tb_page_addr_t phys_pc,
+                         tb_page_addr_t phys_page2)
+{
+    unsigned int h;
+    TranslationBlock **ptb;
+
+    /* add in the physical hash table */
+    h = tb_phys_hash_func(phys_pc);
+    ptb = &tcg_ctx.tb_ctx.tb_phys_hash[h];
+    tb->phys_hash_next = *ptb;
+    *ptb = tb;
+
+    /* add in the page list */
+    tb_alloc_page(tb, 0, phys_pc & TARGET_PAGE_MASK);
+    if (phys_page2 != -1) {
+        tb_alloc_page(tb, 1, phys_page2);
+    } else {
+        tb->page_addr[1] = -1;
+    }
+
+    assert(((uintptr_t)tb & 3) == 0);
+    tb->jmp_list_first = (uintptr_t)tb | 2;
+    tb->jmp_list_next[0] = (uintptr_t)NULL;
+    tb->jmp_list_next[1] = (uintptr_t)NULL;
+
+    /* init original jump addresses */
+    if (tb->jmp_reset_offset[0] != TB_JMP_RESET_OFFSET_INVALID) {
+        tb_reset_jump(tb, 0);
+    }
+    if (tb->jmp_reset_offset[1] != TB_JMP_RESET_OFFSET_INVALID) {
+        tb_reset_jump(tb, 1);
+    }
+
+#ifdef DEBUG_TB_CHECK
+    tb_page_check();
+#endif
+}
+
 /* Called with mmap_lock held for user mode emulation.  */
 TranslationBlock *tb_gen_code(CPUState *cpu,
                               target_ulong pc, target_ulong cs_base,
@@ -1406,107 +1505,6 @@ static void tb_invalidate_phys_page(tb_page_addr_t addr,
 }
 #endif
 
-/* add the tb in the target page and protect it if necessary
- *
- * Called with mmap_lock held for user-mode emulation.
- */
-static inline void tb_alloc_page(TranslationBlock *tb,
-                                 unsigned int n, tb_page_addr_t page_addr)
-{
-    PageDesc *p;
-#ifndef CONFIG_USER_ONLY
-    bool page_already_protected;
-#endif
-
-    tb->page_addr[n] = page_addr;
-    p = page_find_alloc(page_addr >> TARGET_PAGE_BITS, 1);
-    tb->page_next[n] = p->first_tb;
-#ifndef CONFIG_USER_ONLY
-    page_already_protected = p->first_tb != NULL;
-#endif
-    p->first_tb = (TranslationBlock *)((uintptr_t)tb | n);
-    invalidate_page_bitmap(p);
-
-#if defined(CONFIG_USER_ONLY)
-    if (p->flags & PAGE_WRITE) {
-        target_ulong addr;
-        PageDesc *p2;
-        int prot;
-
-        /* force the host page as non writable (writes will have a
-           page fault + mprotect overhead) */
-        page_addr &= qemu_host_page_mask;
-        prot = 0;
-        for (addr = page_addr; addr < page_addr + qemu_host_page_size;
-            addr += TARGET_PAGE_SIZE) {
-
-            p2 = page_find(addr >> TARGET_PAGE_BITS);
-            if (!p2) {
-                continue;
-            }
-            prot |= p2->flags;
-            p2->flags &= ~PAGE_WRITE;
-          }
-        mprotect(g2h(page_addr), qemu_host_page_size,
-                 (prot & PAGE_BITS) & ~PAGE_WRITE);
-#ifdef DEBUG_TB_INVALIDATE
-        printf("protecting code page: 0x" TARGET_FMT_lx "\n",
-               page_addr);
-#endif
-    }
-#else
-    /* if some code is already present, then the pages are already
-       protected. So we handle the case where only the first TB is
-       allocated in a physical page */
-    if (!page_already_protected) {
-        tlb_protect_code(page_addr);
-    }
-#endif
-}
-
-/* add a new TB and link it to the physical page tables. phys_page2 is
- * (-1) to indicate that only one page contains the TB.
- *
- * Called with mmap_lock held for user-mode emulation.
- */
-static void tb_link_page(TranslationBlock *tb, tb_page_addr_t phys_pc,
-                         tb_page_addr_t phys_page2)
-{
-    unsigned int h;
-    TranslationBlock **ptb;
-
-    /* add in the physical hash table */
-    h = tb_phys_hash_func(phys_pc);
-    ptb = &tcg_ctx.tb_ctx.tb_phys_hash[h];
-    tb->phys_hash_next = *ptb;
-    *ptb = tb;
-
-    /* add in the page list */
-    tb_alloc_page(tb, 0, phys_pc & TARGET_PAGE_MASK);
-    if (phys_page2 != -1) {
-        tb_alloc_page(tb, 1, phys_page2);
-    } else {
-        tb->page_addr[1] = -1;
-    }
-
-    assert(((uintptr_t)tb & 3) == 0);
-    tb->jmp_list_first = (uintptr_t)tb | 2;
-    tb->jmp_list_next[0] = (uintptr_t)NULL;
-    tb->jmp_list_next[1] = (uintptr_t)NULL;
-
-    /* init original jump addresses */
-    if (tb->jmp_reset_offset[0] != TB_JMP_RESET_OFFSET_INVALID) {
-        tb_reset_jump(tb, 0);
-    }
-    if (tb->jmp_reset_offset[1] != TB_JMP_RESET_OFFSET_INVALID) {
-        tb_reset_jump(tb, 1);
-    }
-
-#ifdef DEBUG_TB_CHECK
-    tb_page_check();
-#endif
-}
-
 /* find the TB 'tb' such that tb[0].tc_ptr <= tc_ptr <
    tb[1].tc_ptr. Return NULL if not found */
 static TranslationBlock *tb_find_pc(uintptr_t tc_ptr)
-- 
2.7.3

[Qemu-devel] [PATCH 4/8] tcg: Init TB's direct jumps before making it visible

[sergey.fedorov]

From: Sergey Fedorov <serge.fdrv@gmail.com>

Initialize TB's direct jump list data fields and reset the jumps before
tb_link_page() puts it into the physical hash table and the physical
page list. So TB is completely initialized before it becomes visible.

Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com>
Signed-off-by: Sergey Fedorov <sergey.fedorov@linaro.org>
---
 translate-all.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/translate-all.c b/translate-all.c
index ca01dd325b8d..f68716e1819f 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -1131,19 +1131,6 @@ static void tb_link_page(TranslationBlock *tb, tb_page_addr_t phys_pc,
         tb->page_addr[1] = -1;
     }
 
-    assert(((uintptr_t)tb & 3) == 0);
-    tb->jmp_list_first = (uintptr_t)tb | 2;
-    tb->jmp_list_next[0] = (uintptr_t)NULL;
-    tb->jmp_list_next[1] = (uintptr_t)NULL;
-
-    /* init original jump addresses */
-    if (tb->jmp_reset_offset[0] != TB_JMP_RESET_OFFSET_INVALID) {
-        tb_reset_jump(tb, 0);
-    }
-    if (tb->jmp_reset_offset[1] != TB_JMP_RESET_OFFSET_INVALID) {
-        tb_reset_jump(tb, 1);
-    }
-
 #ifdef DEBUG_TB_CHECK
     tb_page_check();
 #endif
@@ -1251,6 +1238,20 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
         ROUND_UP((uintptr_t)gen_code_buf + gen_code_size + search_size,
                  CODE_GEN_ALIGN);
 
+    /* init jump list */
+    assert(((uintptr_t)tb & 3) == 0);
+    tb->jmp_list_first = (uintptr_t)tb | 2;
+    tb->jmp_list_next[0] = (uintptr_t)NULL;
+    tb->jmp_list_next[1] = (uintptr_t)NULL;
+
+    /* init original jump addresses */
+    if (tb->jmp_reset_offset[0] != TB_JMP_RESET_OFFSET_INVALID) {
+        tb_reset_jump(tb, 0);
+    }
+    if (tb->jmp_reset_offset[1] != TB_JMP_RESET_OFFSET_INVALID) {
+        tb_reset_jump(tb, 1);
+    }
+
     /* check next page if needed */
     virt_page2 = (pc + tb->size - 1) & TARGET_PAGE_MASK;
     phys_page2 = -1;
-- 
2.7.3

[Qemu-devel] [PATCH 5/8] tcg: Clarify "thread safaty" check in tb_add_jump()

[sergey.fedorov]

From: Sergey Fedorov <serge.fdrv@gmail.com>

The check does not give an absolute guarantee of thread safety because
there still may be a race condition between two threads which both have
just read zero from jmp_list_next[n] and proceed with list modification.
Clarify this in the comment to attract here some attention.

Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com>
Signed-off-by: Sergey Fedorov <sergey.fedorov@linaro.org>
---
 include/exec/exec-all.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index cd96219a89e7..4f36d109ac7f 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -390,7 +390,10 @@ static inline void tb_set_jmp_target(TranslationBlock *tb,
 static inline void tb_add_jump(TranslationBlock *tb, int n,
                                TranslationBlock *tb_next)
 {
-    /* NOTE: this test is only needed for thread safety */
+    /* FIXME: This test provides only some probablistic "thread safety" for
+     * user-mode emulation; appropriate synchronization/locking scheme should
+     * be implemented.
+     */
     if (!tb->jmp_list_next[n]) {
         /* patch the native jump address */
         tb_set_jmp_target(tb, n, (uintptr_t)tb_next->tc_ptr);
-- 
2.7.3

[Qemu-devel] [PATCH 6/8] tcg: Rename tb_jmp_remove() to tb_remove_from_jmp_list()

[sergey.fedorov]

From: Sergey Fedorov <serge.fdrv@gmail.com>

tb_jmp_remove() was only used to remove the TB from a list of all TBs
jumping to the same TB which is n-th jump destination of the given TB.
Put a comment briefly describing the function behavior and rename it to
better reflect its purpose.

Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com>
Signed-off-by: Sergey Fedorov <sergey.fedorov@linaro.org>
---
 translate-all.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/translate-all.c b/translate-all.c
index f68716e1819f..28154ed75888 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -922,7 +922,8 @@ static inline void tb_page_remove(TranslationBlock **ptb, TranslationBlock *tb)
     }
 }
 
-static inline void tb_jmp_remove(TranslationBlock *tb, int n)
+/* remove the TB from a list of TBs jumping to the n-th jump target of the TB */
+static inline void tb_remove_from_jmp_list(TranslationBlock *tb, int n)
 {
     TranslationBlock *tb1;
     uintptr_t *ptb;
@@ -995,8 +996,8 @@ void tb_phys_invalidate(TranslationBlock *tb, tb_page_addr_t page_addr)
     }
 
     /* suppress this TB from the two jump lists */
-    tb_jmp_remove(tb, 0);
-    tb_jmp_remove(tb, 1);
+    tb_remove_from_jmp_list(tb, 0);
+    tb_remove_from_jmp_list(tb, 1);
 
     /* suppress any remaining jumps to this TB */
     tb1 = tb->jmp_list_first;
-- 
2.7.3

[Qemu-devel] [PATCH 7/8] tcg: Extract removing of jumps to TB from tb_phys_invalidate()

[sergey.fedorov]

From: Sergey Fedorov <serge.fdrv@gmail.com>

Move the code for removing jumps to a TB out of tb_phys_invalidate() to
a separate static inline function tb_jmp_unlink(). This simplifies
tb_phys_invalidate() and improves code structure.

Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com>
Signed-off-by: Sergey Fedorov <sergey.fedorov@linaro.org>
---
 translate-all.c | 44 ++++++++++++++++++++++++++------------------
 1 file changed, 26 insertions(+), 18 deletions(-)

diff --git a/translate-all.c b/translate-all.c
index 28154ed75888..8b4bfa713bf7 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -959,14 +959,37 @@ static inline void tb_reset_jump(TranslationBlock *tb, int n)
     tb_set_jmp_target(tb, n, addr);
 }
 
+/* remove any jumps to the TB */
+static inline void tb_jmp_unlink(TranslationBlock *tb)
+{
+    uintptr_t tb1, tb2;
+    unsigned int n1;
+
+    tb1 = tb->jmp_list_first;
+    for (;;) {
+        TranslationBlock *tmp_tb;
+        n1 = tb1 & 3;
+        if (n1 == 2) {
+            break;
+        }
+        tmp_tb = (TranslationBlock *)(tb1 & ~3);
+        tb2 = tmp_tb->jmp_list_next[n1];
+        tb_reset_jump(tmp_tb, n1);
+        tmp_tb->jmp_list_next[n1] = (uintptr_t)NULL;
+        tb1 = tb2;
+    }
+
+    assert(((uintptr_t)tb & 3) == 0);
+    tb->jmp_list_first = (uintptr_t)tb | 2; /* fail safe */
+}
+
 /* invalidate one TB */
 void tb_phys_invalidate(TranslationBlock *tb, tb_page_addr_t page_addr)
 {
     CPUState *cpu;
     PageDesc *p;
-    unsigned int h, n1;
+    unsigned int h;
     tb_page_addr_t phys_pc;
-    uintptr_t tb1, tb2;
 
     /* remove the TB from the hash list */
     phys_pc = tb->page_addr[0] + (tb->pc & ~TARGET_PAGE_MASK);
@@ -1000,22 +1023,7 @@ void tb_phys_invalidate(TranslationBlock *tb, tb_page_addr_t page_addr)
     tb_remove_from_jmp_list(tb, 1);
 
     /* suppress any remaining jumps to this TB */
-    tb1 = tb->jmp_list_first;
-    for (;;) {
-        TranslationBlock *tmp_tb;
-        n1 = tb1 & 3;
-        if (n1 == 2) {
-            break;
-        }
-        tmp_tb = (TranslationBlock *)(tb1 & ~3);
-        tb2 = tmp_tb->jmp_list_next[n1];
-        tb_reset_jump(tmp_tb, n1);
-        tmp_tb->jmp_list_next[n1] = (uintptr_t)NULL;
-        tb1 = tb2;
-    }
-
-    assert(((uintptr_t)tb & 3) == 0);
-    tb->jmp_list_first = (uintptr_t)tb | 2; /* fail safe */
+    tb_jmp_unlink(tb);
 
     tcg_ctx.tb_ctx.tb_phys_invalidate_count++;
 }
-- 
2.7.3

[Qemu-devel] [PATCH 8/8] tcg: Clean up tb_jmp_unlink()

[sergey.fedorov]

From: Sergey Fedorov <serge.fdrv@gmail.com>

Unify the code of this function with tb_jmp_remove_from_list(). Making
these functions similar improves their readability. Also this could be a
step towards making this function thread-safe.

Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com>
Signed-off-by: Sergey Fedorov <sergey.fedorov@linaro.org>
---
 translate-all.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/translate-all.c b/translate-all.c
index 8b4bfa713bf7..56c77a72773d 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -962,25 +962,21 @@ static inline void tb_reset_jump(TranslationBlock *tb, int n)
 /* remove any jumps to the TB */
 static inline void tb_jmp_unlink(TranslationBlock *tb)
 {
-    uintptr_t tb1, tb2;
+    TranslationBlock *tb1;
+    uintptr_t *ptb;
     unsigned int n1;
 
-    tb1 = tb->jmp_list_first;
+    ptb = &tb->jmp_list_first;
     for (;;) {
-        TranslationBlock *tmp_tb;
-        n1 = tb1 & 3;
+        n1 = *ptb & 3;
+        tb1 = (TranslationBlock *)(*ptb & ~3);
         if (n1 == 2) {
             break;
         }
-        tmp_tb = (TranslationBlock *)(tb1 & ~3);
-        tb2 = tmp_tb->jmp_list_next[n1];
-        tb_reset_jump(tmp_tb, n1);
-        tmp_tb->jmp_list_next[n1] = (uintptr_t)NULL;
-        tb1 = tb2;
+        tb_reset_jump(tb1, n1);
+        *ptb = tb1->jmp_list_next[n1];
+        tb1->jmp_list_next[n1] = (uintptr_t)NULL;
     }
-
-    assert(((uintptr_t)tb & 3) == 0);
-    tb->jmp_list_first = (uintptr_t)tb | 2; /* fail safe */
 }
 
 /* invalidate one TB */
-- 
2.7.3

Re: [Qemu-devel] [PATCH 5/8] tcg: Clarify "thread safaty" check in tb_add_jump()

[Paolo Bonzini]

On 24/03/2016 11:39, sergey.fedorov@linaro.org wrote:
> +    /* FIXME: This test provides only some probablistic "thread safety" for
> +     * user-mode emulation; appropriate synchronization/locking scheme should
> +     * be implemented.
> +     */

There is appropriate locking.  This code:

       if (next_tb != 0 && tb->page_addr[1] == -1
           && !qemu_loglevel_mask(CPU_LOG_TB_NOCHAIN)) {
           tb_add_jump((TranslationBlock *)(next_tb & ~TB_EXIT_MASK),
                       next_tb & TB_EXIT_MASK, tb);
       }

in cpu-exec.c runs under tb_lock.  However, two threads can decide to
call tb_add_jump at the same time outside the lock, so we have to check
inside the lock whether someone has already done the work.

What the comment means is that, in single-threaded scenarios (current
TCG and single-threaded user emulation), tb->jmp_list_next[n] is only
set once.

Paolo

Re: [Qemu-devel] [PATCH 0/8] tcg: Direct block chaining clean-up

[Paolo Bonzini]

On 24/03/2016 11:39, sergey.fedorov@linaro.org wrote:
> From: Sergey Fedorov <serge.fdrv@gmail.com>
> 
> This series combines a set of patches which is meant to improve overall code
> structure and readability of direct block chaining mechanism. The other point
> is to make a step towards thread safety of TB chainig.
> 
> The series' tree can be found in a public git repository [1].
> 
> [1] https://github.com/sergefdrv/qemu/tree/tb-chaining-cleanup

Looks good.

Alex, can you give it a shake?  There is still time to include it in 2.6
before soft freeze.

Paolo

> Sergey Fedorov (8):
>   tcg: Clean up direct block chaining data fields
>   tcg: Use uintptr_t type for jmp_list_{next|first} fields of TB
>   tcg: Rearrange tb_link_page() to avoid forward declaration
>   tcg: Init TB's direct jumps before making it visible
>   tcg: Clarify "thread safaty" check in tb_add_jump()
>   tcg: Rename tb_jmp_remove() to tb_remove_from_jmp_list()
>   tcg: Extract removing of jumps to TB from tb_phys_invalidate()
>   tcg: Clean up tb_jmp_unlink()
> 
>  include/exec/exec-all.h      |  51 +++++---
>  tcg/aarch64/tcg-target.inc.c |   7 +-
>  tcg/arm/tcg-target.inc.c     |   8 +-
>  tcg/i386/tcg-target.inc.c    |   8 +-
>  tcg/ia64/tcg-target.inc.c    |   6 +-
>  tcg/mips/tcg-target.inc.c    |   8 +-
>  tcg/ppc/tcg-target.inc.c     |   6 +-
>  tcg/s390/tcg-target.inc.c    |  11 +-
>  tcg/sparc/tcg-target.inc.c   |   9 +-
>  tcg/tcg.h                    |   6 +-
>  tcg/tci/tcg-target.inc.c     |  10 +-
>  translate-all.c              | 290 ++++++++++++++++++++++---------------------
>  12 files changed, 224 insertions(+), 196 deletions(-)
> 

Re: [Qemu-devel] [PATCH 0/8] tcg: Direct block chaining clean-up

[Alex Bennée]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 24/03/2016 11:39, sergey.fedorov@linaro.org wrote:
>> From: Sergey Fedorov <serge.fdrv@gmail.com>
>>
>> This series combines a set of patches which is meant to improve overall code
>> structure and readability of direct block chaining mechanism. The other point
>> is to make a step towards thread safety of TB chainig.
>>
>> The series' tree can be found in a public git repository [1].
>>
>> [1] https://github.com/sergefdrv/qemu/tree/tb-chaining-cleanup
>
> Looks good.
>
> Alex, can you give it a shake?  There is still time to include it in 2.6
> before soft freeze.

OK then, I'll start looking after lunch ;-)

>
> Paolo
>
>> Sergey Fedorov (8):
>>   tcg: Clean up direct block chaining data fields
>>   tcg: Use uintptr_t type for jmp_list_{next|first} fields of TB
>>   tcg: Rearrange tb_link_page() to avoid forward declaration
>>   tcg: Init TB's direct jumps before making it visible
>>   tcg: Clarify "thread safaty" check in tb_add_jump()
>>   tcg: Rename tb_jmp_remove() to tb_remove_from_jmp_list()
>>   tcg: Extract removing of jumps to TB from tb_phys_invalidate()
>>   tcg: Clean up tb_jmp_unlink()
>>
>>  include/exec/exec-all.h      |  51 +++++---
>>  tcg/aarch64/tcg-target.inc.c |   7 +-
>>  tcg/arm/tcg-target.inc.c     |   8 +-
>>  tcg/i386/tcg-target.inc.c    |   8 +-
>>  tcg/ia64/tcg-target.inc.c    |   6 +-
>>  tcg/mips/tcg-target.inc.c    |   8 +-
>>  tcg/ppc/tcg-target.inc.c     |   6 +-
>>  tcg/s390/tcg-target.inc.c    |  11 +-
>>  tcg/sparc/tcg-target.inc.c   |   9 +-
>>  tcg/tcg.h                    |   6 +-
>>  tcg/tci/tcg-target.inc.c     |  10 +-
>>  translate-all.c              | 290 ++++++++++++++++++++++---------------------
>>  12 files changed, 224 insertions(+), 196 deletions(-)
>>


--
Alex Bennée

Re: [Qemu-devel] [PATCH 5/8] tcg: Clarify "thread safaty" check in tb_add_jump()

[Artyom Tarasenko]

s/safaty/safety/ ?

On Thu, Mar 24, 2016 at 11:39 AM,  <sergey.fedorov@linaro.org> wrote:
> From: Sergey Fedorov <serge.fdrv@gmail.com>
>
> The check does not give an absolute guarantee of thread safety because
> there still may be a race condition between two threads which both have
> just read zero from jmp_list_next[n] and proceed with list modification.
> Clarify this in the comment to attract here some attention.
>
> Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com>
> Signed-off-by: Sergey Fedorov <sergey.fedorov@linaro.org>
> ---
>  include/exec/exec-all.h | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index cd96219a89e7..4f36d109ac7f 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -390,7 +390,10 @@ static inline void tb_set_jmp_target(TranslationBlock *tb,
>  static inline void tb_add_jump(TranslationBlock *tb, int n,
>                                 TranslationBlock *tb_next)
>  {
> -    /* NOTE: this test is only needed for thread safety */
> +    /* FIXME: This test provides only some probablistic "thread safety" for
> +     * user-mode emulation; appropriate synchronization/locking scheme should
> +     * be implemented.
> +     */
>      if (!tb->jmp_list_next[n]) {
>          /* patch the native jump address */
>          tb_set_jmp_target(tb, n, (uintptr_t)tb_next->tc_ptr);
> --
> 2.7.3
>
>



-- 
Regards,
Artyom Tarasenko

SPARC and PPC PReP under qemu blog: http://tyom.blogspot.com/search/label/qemu

Re: [Qemu-devel] [PATCH 5/8] tcg: Clarify "thread safaty" check in tb_add_jump()

[Sergey Fedorov]

On 24/03/16 15:23, Artyom Tarasenko wrote:
> s/safaty/safety/ ?

Oops, silly typo :)

Thanks,
Sergey

Re: [Qemu-devel] [PATCH 5/8] tcg: Clarify "thread safaty" check in tb_add_jump()

[Sergey Fedorov]

On 24/03/16 14:31, Paolo Bonzini wrote:
> On 24/03/2016 11:39, sergey.fedorov@linaro.org wrote:
>> +    /* FIXME: This test provides only some probablistic "thread safety" for
>> +     * user-mode emulation; appropriate synchronization/locking scheme should
>> +     * be implemented.
>> +     */
> There is appropriate locking.  This code:
>
>        if (next_tb != 0 && tb->page_addr[1] == -1
>            && !qemu_loglevel_mask(CPU_LOG_TB_NOCHAIN)) {
>            tb_add_jump((TranslationBlock *)(next_tb & ~TB_EXIT_MASK),
>                        next_tb & TB_EXIT_MASK, tb);
>        }
>
> in cpu-exec.c runs under tb_lock.  However, two threads can decide to
> call tb_add_jump at the same time outside the lock, so we have to check
> inside the lock whether someone has already done the work.
>
> What the comment means is that, in single-threaded scenarios (current
> TCG and single-threaded user emulation), tb->jmp_list_next[n] is only
> set once.

Right, thanks for clarifying this. So I'm going to put mention this in
the comment, then.

Kind regards,
Sergey

Re: [Qemu-devel] [PATCH 1/8] tcg: Clean up direct block chaining data fields

[Alex Bennée]

sergey.fedorov@linaro.org writes:

> From: Sergey Fedorov <serge.fdrv@gmail.com>
>
> Briefly describe in a comment how direct block chaining is done. It
> should help in understanding of the following data fields.
>
> Rename some fields in TranslationBlock and TCGContext structures to
> better reflect their purpose (dropping excessive 'tb_' prefix in
> TranslationBlock but keeping it in TCGContext):
>    tb_next_offset  =>  jmp_reset_offset
>    tb_jmp_offset   =>  jmp_insn_offset
>    tb_next         =>  jmp_target_addr
>    jmp_next        =>  jmp_list_next
>    jmp_first       =>  jmp_list_first
>
> Avoid using a magic constant as an invalid offset which is used to
> indicate that there's no n-th jump generated.
>
> Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com>
> Signed-off-by: Sergey Fedorov <sergey.fedorov@linaro.org>
> ---
>  include/exec/exec-all.h      | 44 ++++++++++++++++++++++++--------------
>  tcg/aarch64/tcg-target.inc.c |  7 +++---
>  tcg/arm/tcg-target.inc.c     |  8 +++----
>  tcg/i386/tcg-target.inc.c    |  8 +++----
>  tcg/ia64/tcg-target.inc.c    |  6 +++---
>  tcg/mips/tcg-target.inc.c    |  8 +++----
>  tcg/ppc/tcg-target.inc.c     |  6 +++---
>  tcg/s390/tcg-target.inc.c    | 11 +++++-----
>  tcg/sparc/tcg-target.inc.c   |  9 ++++----
>  tcg/tcg.h                    |  6 +++---
>  tcg/tci/tcg-target.inc.c     | 10 ++++-----
>  translate-all.c              | 51 +++++++++++++++++++++++---------------------
>  12 files changed, 96 insertions(+), 78 deletions(-)
>
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index 05a151da4a54..cc3d2ca25917 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -257,20 +257,32 @@ struct TranslationBlock {
>      struct TranslationBlock *page_next[2];
>      tb_page_addr_t page_addr[2];
>
> -    /* the following data are used to directly call another TB from
> -       the code of this one. */
> -    uint16_t tb_next_offset[2]; /* offset of original jump target */
> +    /* The following data are used to directly call another TB from
> +     * the code of this one. This can be done either by emitting direct or
> +     * indirect native jump instructions. These jumps are reset so that the TB
> +     * just continue its execution. The TB can be linked to another one by
> +     * setting one of the jump targets (or patching the jump instruction). Only
> +     * two of such jumps are supported.
> +     */
> +    uint16_t jmp_reset_offset[2]; /* offset of original jump target */
> +#define TB_JMP_RESET_OFFSET_INVALID 0xffff /* indicates no jump generated */
>  #ifdef USE_DIRECT_JUMP
> -    uint16_t tb_jmp_offset[2]; /* offset of jump instruction */
> +    uint16_t jmp_insn_offset[2]; /* offset of native jump instruction */
>  #else
> -    uintptr_t tb_next[2]; /* address of jump generated code */
> +    uintptr_t jmp_target_addr[2]; /* target address for indirect jump */
>  #endif
> -    /* list of TBs jumping to this one. This is a circular list using
> -       the two least significant bits of the pointers to tell what is
> -       the next pointer: 0 = jmp_next[0], 1 = jmp_next[1], 2 =
> -       jmp_first */
> -    struct TranslationBlock *jmp_next[2];
> -    struct TranslationBlock *jmp_first;
> +    /* Each TB has an assosiated circular list of TBs jumping to this one.
> +     * jmp_list_first points to the first TB jumping to this one.
> +     * jmp_list_next is used to point to the next TB in a list.
> +     * Since each TB can have two jumps, it can participate in two lists.
> +     * The two least significant bits of a pointer are used to choose which
> +     * data field holds a pointer to the next TB:
> +     * 0 => jmp_list_next[0], 1 => jmp_list_next[1], 2 => jmp_list_first.
> +     * In other words, 0/1 tells which jump is used in the pointed TB,
> +     * and 2 means that this is a pointer back to the target TB of this list.
> +     */
> +    struct TranslationBlock *jmp_list_next[2];
> +    struct TranslationBlock *jmp_list_first;

OK I found that tricky to follow. Where does the value of the pointer
come from that sets these bottom bits? The TB jumping to this TB sets it?

>  };
>
>  #include "qemu/thread.h"
> @@ -359,7 +371,7 @@ void tb_set_jmp_target1(uintptr_t jmp_addr, uintptr_t addr);
>  static inline void tb_set_jmp_target(TranslationBlock *tb,
>                                       int n, uintptr_t addr)
>  {
> -    uint16_t offset = tb->tb_jmp_offset[n];
> +    uint16_t offset = tb->jmp_insn_offset[n];
>      tb_set_jmp_target1((uintptr_t)(tb->tc_ptr + offset), addr);
>  }
>
> @@ -369,7 +381,7 @@ static inline void tb_set_jmp_target(TranslationBlock *tb,
>  static inline void tb_set_jmp_target(TranslationBlock *tb,
>                                       int n, uintptr_t addr)
>  {
> -    tb->tb_next[n] = addr;
> +    tb->jmp_target_addr[n] = addr;
>  }
>
>  #endif
> @@ -378,13 +390,13 @@ static inline void tb_add_jump(TranslationBlock *tb, int n,
>                                 TranslationBlock *tb_next)
>  {
>      /* NOTE: this test is only needed for thread safety */
> -    if (!tb->jmp_next[n]) {
> +    if (!tb->jmp_list_next[n]) {
>          /* patch the native jump address */
>          tb_set_jmp_target(tb, n, (uintptr_t)tb_next->tc_ptr);
>
>          /* add in TB jmp circular list */
> -        tb->jmp_next[n] = tb_next->jmp_first;
> -        tb_next->jmp_first = (TranslationBlock *)((uintptr_t)(tb) | (n));
> +        tb->jmp_list_next[n] = tb_next->jmp_list_first;
> +        tb_next->jmp_list_first = (TranslationBlock *)((uintptr_t)tb | n);
>      }
>  }
>
> diff --git a/tcg/aarch64/tcg-target.inc.c b/tcg/aarch64/tcg-target.inc.c
> index 0ed10a974121..08efdf41da48 100644
> --- a/tcg/aarch64/tcg-target.inc.c
> +++ b/tcg/aarch64/tcg-target.inc.c
> @@ -1294,12 +1294,13 @@ static void tcg_out_op(TCGContext *s, TCGOpcode opc,
>  #ifndef USE_DIRECT_JUMP
>  #error "USE_DIRECT_JUMP required for aarch64"
>  #endif
> -        assert(s->tb_jmp_offset != NULL); /* consistency for USE_DIRECT_JUMP */
> -        s->tb_jmp_offset[a0] = tcg_current_code_size(s);
> +        /* consistency for USE_DIRECT_JUMP */
> +        assert(s->tb_jmp_insn_offset != NULL);
> +        s->tb_jmp_insn_offset[a0] = tcg_current_code_size(s);
>          /* actual branch destination will be patched by
>             aarch64_tb_set_jmp_target later, beware retranslation. */
>          tcg_out_goto_noaddr(s);
> -        s->tb_next_offset[a0] = tcg_current_code_size(s);
> +        s->tb_jmp_reset_offset[a0] = tcg_current_code_size(s);
>          break;
>
>      case INDEX_op_br:
> diff --git a/tcg/arm/tcg-target.inc.c b/tcg/arm/tcg-target.inc.c
> index 3edf6a6f971c..a9147620b073 100644
> --- a/tcg/arm/tcg-target.inc.c
> +++ b/tcg/arm/tcg-target.inc.c
> @@ -1647,17 +1647,17 @@ static inline void tcg_out_op(TCGContext *s, TCGOpcode opc,
>          tcg_out_goto(s, COND_AL, tb_ret_addr);
>          break;
>      case INDEX_op_goto_tb:
> -        if (s->tb_jmp_offset) {
> +        if (s->tb_jmp_insn_offset) {
>              /* Direct jump method */
> -            s->tb_jmp_offset[args[0]] = tcg_current_code_size(s);
> +            s->tb_jmp_insn_offset[args[0]] = tcg_current_code_size(s);
>              tcg_out_b_noaddr(s, COND_AL);
>          } else {
>              /* Indirect jump method */
> -            intptr_t ptr = (intptr_t)(s->tb_next + args[0]);
> +            intptr_t ptr = (intptr_t)(s->tb_jmp_target_addr + args[0]);
>              tcg_out_movi32(s, COND_AL, TCG_REG_R0, ptr & ~0xfff);
>              tcg_out_ld32_12(s, COND_AL, TCG_REG_PC, TCG_REG_R0, ptr & 0xfff);
>          }
> -        s->tb_next_offset[args[0]] = tcg_current_code_size(s);
> +        s->tb_jmp_reset_offset[args[0]] = tcg_current_code_size(s);
>          break;
>      case INDEX_op_br:
>          tcg_out_goto_label(s, COND_AL, arg_label(args[0]));
> diff --git a/tcg/i386/tcg-target.inc.c b/tcg/i386/tcg-target.inc.c
> index 9187d34caf6d..2f98cae97f3b 100644
> --- a/tcg/i386/tcg-target.inc.c
> +++ b/tcg/i386/tcg-target.inc.c
> @@ -1775,17 +1775,17 @@ static inline void tcg_out_op(TCGContext *s, TCGOpcode opc,
>          tcg_out_jmp(s, tb_ret_addr);
>          break;
>      case INDEX_op_goto_tb:
> -        if (s->tb_jmp_offset) {
> +        if (s->tb_jmp_insn_offset) {
>              /* direct jump method */
>              tcg_out8(s, OPC_JMP_long); /* jmp im */
> -            s->tb_jmp_offset[args[0]] = tcg_current_code_size(s);
> +            s->tb_jmp_insn_offset[args[0]] = tcg_current_code_size(s);
>              tcg_out32(s, 0);
>          } else {
>              /* indirect jump method */
>              tcg_out_modrm_offset(s, OPC_GRP5, EXT5_JMPN_Ev, -1,
> -                                 (intptr_t)(s->tb_next + args[0]));
> +                                 (intptr_t)(s->tb_jmp_target_addr + args[0]));
>          }
> -        s->tb_next_offset[args[0]] = tcg_current_code_size(s);
> +        s->tb_jmp_reset_offset[args[0]] = tcg_current_code_size(s);
>          break;
>      case INDEX_op_br:
>          tcg_out_jxx(s, JCC_JMP, arg_label(args[0]), 0);
> diff --git a/tcg/ia64/tcg-target.inc.c b/tcg/ia64/tcg-target.inc.c
> index 62d654943c20..261861f90c3a 100644
> --- a/tcg/ia64/tcg-target.inc.c
> +++ b/tcg/ia64/tcg-target.inc.c
> @@ -881,13 +881,13 @@ static void tcg_out_exit_tb(TCGContext *s, tcg_target_long arg)
>
>  static inline void tcg_out_goto_tb(TCGContext *s, TCGArg arg)
>  {
> -    if (s->tb_jmp_offset) {
> +    if (s->tb_jmp_insn_offset) {
>          /* direct jump method */
>          tcg_abort();
>      } else {
>          /* indirect jump method */
>          tcg_out_movi(s, TCG_TYPE_PTR, TCG_REG_R2,
> -                     (tcg_target_long)(s->tb_next + arg));
> +                     (tcg_target_long)(s->tb_jmp_target_addr + arg));
>          tcg_out_bundle(s, MmI,
>                         tcg_opc_m1 (TCG_REG_P0, OPC_LD8_M1,
>                                     TCG_REG_R2, TCG_REG_R2),
> @@ -900,7 +900,7 @@ static inline void tcg_out_goto_tb(TCGContext *s, TCGArg arg)
>                         tcg_opc_b4 (TCG_REG_P0, OPC_BR_SPTK_MANY_B4,
>                                     TCG_REG_B6));
>      }
> -    s->tb_next_offset[arg] = tcg_current_code_size(s);
> +    s->tb_jmp_reset_offset[arg] = tcg_current_code_size(s);
>  }
>
>  static inline void tcg_out_jmp(TCGContext *s, TCGArg addr)
> diff --git a/tcg/mips/tcg-target.inc.c b/tcg/mips/tcg-target.inc.c
> index 297bd00910b7..b2a839ac2cd3 100644
> --- a/tcg/mips/tcg-target.inc.c
> +++ b/tcg/mips/tcg-target.inc.c
> @@ -1397,19 +1397,19 @@ static inline void tcg_out_op(TCGContext *s, TCGOpcode opc,
>          }
>          break;
>      case INDEX_op_goto_tb:
> -        if (s->tb_jmp_offset) {
> +        if (s->tb_jmp_insn_offset) {
>              /* direct jump method */
> -            s->tb_jmp_offset[a0] = tcg_current_code_size(s);
> +            s->tb_jmp_insn_offset[a0] = tcg_current_code_size(s);
>              /* Avoid clobbering the address during retranslation.  */
>              tcg_out32(s, OPC_J | (*(uint32_t *)s->code_ptr & 0x3ffffff));
>          } else {
>              /* indirect jump method */
>              tcg_out_ld(s, TCG_TYPE_PTR, TCG_TMP0, TCG_REG_ZERO,
> -                       (uintptr_t)(s->tb_next + a0));
> +                       (uintptr_t)(s->tb_jmp_target_addr + a0));
>              tcg_out_opc_reg(s, OPC_JR, 0, TCG_TMP0, 0);
>          }
>          tcg_out_nop(s);
> -        s->tb_next_offset[a0] = tcg_current_code_size(s);
> +        s->tb_jmp_reset_offset[a0] = tcg_current_code_size(s);
>          break;
>      case INDEX_op_br:
>          tcg_out_brcond(s, TCG_COND_EQ, TCG_REG_ZERO, TCG_REG_ZERO,
> diff --git a/tcg/ppc/tcg-target.inc.c b/tcg/ppc/tcg-target.inc.c
> index 8c1c2dfa9b22..10394079ad9d 100644
> --- a/tcg/ppc/tcg-target.inc.c
> +++ b/tcg/ppc/tcg-target.inc.c
> @@ -1894,17 +1894,17 @@ static void tcg_out_op(TCGContext *s, TCGOpcode opc, const TCGArg *args,
>          tcg_out_b(s, 0, tb_ret_addr);
>          break;
>      case INDEX_op_goto_tb:
> -        tcg_debug_assert(s->tb_jmp_offset);
> +        tcg_debug_assert(s->tb_jmp_insn_offset);
>          /* Direct jump.  Ensure the next insns are 8-byte aligned. */
>          if ((uintptr_t)s->code_ptr & 7) {
>              tcg_out32(s, NOP);
>          }
> -        s->tb_jmp_offset[args[0]] = tcg_current_code_size(s);
> +        s->tb_jmp_insn_offset[args[0]] = tcg_current_code_size(s);
>          /* To be replaced by either a branch+nop or a load into TMP1.  */
>          s->code_ptr += 2;
>          tcg_out32(s, MTSPR | RS(TCG_REG_TMP1) | CTR);
>          tcg_out32(s, BCCTR | BO_ALWAYS);
> -        s->tb_next_offset[args[0]] = tcg_current_code_size(s);
> +        s->tb_jmp_reset_offset[args[0]] = tcg_current_code_size(s);
>          break;
>      case INDEX_op_br:
>          {
> diff --git a/tcg/s390/tcg-target.inc.c b/tcg/s390/tcg-target.inc.c
> index fbf97bb2e15d..e95b04b0e278 100644
> --- a/tcg/s390/tcg-target.inc.c
> +++ b/tcg/s390/tcg-target.inc.c
> @@ -1715,17 +1715,18 @@ static inline void tcg_out_op(TCGContext *s, TCGOpcode opc,
>          break;
>
>      case INDEX_op_goto_tb:
> -        if (s->tb_jmp_offset) {
> +        if (s->tb_jmp_insn_offset) {
>              tcg_out16(s, RIL_BRCL | (S390_CC_ALWAYS << 4));
> -            s->tb_jmp_offset[args[0]] = tcg_current_code_size(s);
> +            s->tb_jmp_insn_offset[args[0]] = tcg_current_code_size(s);
>              s->code_ptr += 2;
>          } else {
> -            /* load address stored at s->tb_next + args[0] */
> -            tcg_out_ld_abs(s, TCG_TYPE_PTR, TCG_TMP0, s->tb_next + args[0]);
> +            /* load address stored at s->tb_jmp_target_addr + args[0] */
> +            tcg_out_ld_abs(s, TCG_TYPE_PTR, TCG_TMP0,
> +                           s->tb_jmp_target_addr + args[0]);
>              /* and go there */
>              tcg_out_insn(s, RR, BCR, S390_CC_ALWAYS, TCG_TMP0);
>          }
> -        s->tb_next_offset[args[0]] = tcg_current_code_size(s);
> +        s->tb_jmp_reset_offset[args[0]] = tcg_current_code_size(s);
>          break;
>
>      OP_32_64(ld8u):
> diff --git a/tcg/sparc/tcg-target.inc.c b/tcg/sparc/tcg-target.inc.c
> index 54df1bc42432..a611885a2aaf 100644
> --- a/tcg/sparc/tcg-target.inc.c
> +++ b/tcg/sparc/tcg-target.inc.c
> @@ -1229,18 +1229,19 @@ static void tcg_out_op(TCGContext *s, TCGOpcode opc,
>          }
>          break;
>      case INDEX_op_goto_tb:
> -        if (s->tb_jmp_offset) {
> +        if (s->tb_jmp_insn_offset) {
>              /* direct jump method */
> -            s->tb_jmp_offset[a0] = tcg_current_code_size(s);
> +            s->tb_jmp_insn_offset[a0] = tcg_current_code_size(s);
>              /* Make sure to preserve links during retranslation.  */
>              tcg_out32(s, CALL | (*s->code_ptr & ~INSN_OP(-1)));
>          } else {
>              /* indirect jump method */
> -            tcg_out_ld_ptr(s, TCG_REG_T1, (uintptr_t)(s->tb_next + a0));
> +            tcg_out_ld_ptr(s, TCG_REG_T1,
> +                           (uintptr_t)(s->tb_jmp_target_addr + a0));
>              tcg_out_arithi(s, TCG_REG_G0, TCG_REG_T1, 0, JMPL);
>          }
>          tcg_out_nop(s);
> -        s->tb_next_offset[a0] = tcg_current_code_size(s);
> +        s->tb_jmp_reset_offset[a0] = tcg_current_code_size(s);
>          break;
>      case INDEX_op_br:
>          tcg_out_bpcc(s, COND_A, BPCC_PT, arg_label(a0));
> diff --git a/tcg/tcg.h b/tcg/tcg.h
> index b83f76351ccd..f9d11b29442b 100644
> --- a/tcg/tcg.h
> +++ b/tcg/tcg.h
> @@ -510,9 +510,9 @@ struct TCGContext {
>
>      /* goto_tb support */
>      tcg_insn_unit *code_buf;
> -    uintptr_t *tb_next;
> -    uint16_t *tb_next_offset;
> -    uint16_t *tb_jmp_offset; /* != NULL if USE_DIRECT_JUMP */
> +    uint16_t *tb_jmp_reset_offset; /* tb->jmp_reset_offset */
> +    uint16_t *tb_jmp_insn_offset; /* tb->jmp_insn_offset if USE_DIRECT_JUMP */
> +    uintptr_t *tb_jmp_target_addr; /* tb->jmp_target_addr if !USE_DIRECT_JUMP */
>
>      /* liveness analysis */
>      uint16_t *op_dead_args; /* for each operation, each bit tells if the
> diff --git a/tcg/tci/tcg-target.inc.c b/tcg/tci/tcg-target.inc.c
> index 4afe4d7a8d59..4e91687abd1c 100644
> --- a/tcg/tci/tcg-target.inc.c
> +++ b/tcg/tci/tcg-target.inc.c
> @@ -553,17 +553,17 @@ static void tcg_out_op(TCGContext *s, TCGOpcode opc, const TCGArg *args,
>          tcg_out64(s, args[0]);
>          break;
>      case INDEX_op_goto_tb:
> -        if (s->tb_jmp_offset) {
> +        if (s->tb_jmp_insn_offset) {
>              /* Direct jump method. */
> -            assert(args[0] < ARRAY_SIZE(s->tb_jmp_offset));
> -            s->tb_jmp_offset[args[0]] = tcg_current_code_size(s);
> +            assert(args[0] < ARRAY_SIZE(s->tb_jmp_insn_offset));
> +            s->tb_jmp_insn_offset[args[0]] = tcg_current_code_size(s);
>              tcg_out32(s, 0);
>          } else {
>              /* Indirect jump method. */
>              TODO();
>          }
> -        assert(args[0] < ARRAY_SIZE(s->tb_next_offset));
> -        s->tb_next_offset[args[0]] = tcg_current_code_size(s);
> +        assert(args[0] < ARRAY_SIZE(s->tb_jmp_reset_offset));
> +        s->tb_jmp_reset_offset[args[0]] = tcg_current_code_size(s);
>          break;
>      case INDEX_op_br:
>          tci_out_label(s, arg_label(args[0]));
> diff --git a/translate-all.c b/translate-all.c
> index e9f409b762ab..31cdf8ae217e 100644
> --- a/translate-all.c
> +++ b/translate-all.c
> @@ -929,7 +929,7 @@ static inline void tb_jmp_remove(TranslationBlock *tb, int n)
>      TranslationBlock *tb1, **ptb;
>      unsigned int n1;
>
> -    ptb = &tb->jmp_next[n];
> +    ptb = &tb->jmp_list_next[n];
>      tb1 = *ptb;
>      if (tb1) {
>          /* find tb(n) in circular list */
> @@ -941,15 +941,15 @@ static inline void tb_jmp_remove(TranslationBlock *tb, int n)
>                  break;
>              }
>              if (n1 == 2) {
> -                ptb = &tb1->jmp_first;
> +                ptb = &tb1->jmp_list_first;
>              } else {
> -                ptb = &tb1->jmp_next[n1];
> +                ptb = &tb1->jmp_list_next[n1];
>              }
>          }
>          /* now we can suppress tb(n) from the list */
> -        *ptb = tb->jmp_next[n];
> +        *ptb = tb->jmp_list_next[n];
>
> -        tb->jmp_next[n] = NULL;
> +        tb->jmp_list_next[n] = NULL;
>      }
>  }
>
> @@ -957,7 +957,8 @@ static inline void tb_jmp_remove(TranslationBlock *tb, int n)
>     another TB */
>  static inline void tb_reset_jump(TranslationBlock *tb, int n)
>  {
> -    tb_set_jmp_target(tb, n, (uintptr_t)(tb->tc_ptr + tb->tb_next_offset[n]));
> +    uintptr_t addr = (uintptr_t)(tb->tc_ptr + tb->jmp_reset_offset[n]);
> +    tb_set_jmp_target(tb, n, addr);
>  }
>
>  /* invalidate one TB */
> @@ -1001,19 +1002,21 @@ void tb_phys_invalidate(TranslationBlock *tb, tb_page_addr_t page_addr)
>      tb_jmp_remove(tb, 1);
>
>      /* suppress any remaining jumps to this TB */
> -    tb1 = tb->jmp_first;
> +    tb1 = tb->jmp_list_first;
>      for (;;) {
>          n1 = (uintptr_t)tb1 & 3;
>          if (n1 == 2) {
>              break;
>          }
>          tb1 = (TranslationBlock *)((uintptr_t)tb1 & ~3);
> -        tb2 = tb1->jmp_next[n1];
> +        tb2 = tb1->jmp_list_next[n1];
>          tb_reset_jump(tb1, n1);
> -        tb1->jmp_next[n1] = NULL;
> +        tb1->jmp_list_next[n1] = NULL;
>          tb1 = tb2;
>      }
> -    tb->jmp_first = (TranslationBlock *)((uintptr_t)tb | 2); /* fail safe */
> +
> +    /* fail safe */
> +    tb->jmp_list_first = (TranslationBlock *)((uintptr_t)tb | 2);
>
>      tcg_ctx.tb_ctx.tb_phys_invalidate_count++;
>  }
> @@ -1098,15 +1101,15 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
>      trace_translate_block(tb, tb->pc, tb->tc_ptr);
>
>      /* generate machine code */
> -    tb->tb_next_offset[0] = 0xffff;
> -    tb->tb_next_offset[1] = 0xffff;
> -    tcg_ctx.tb_next_offset = tb->tb_next_offset;
> +    tb->jmp_reset_offset[0] = TB_JMP_RESET_OFFSET_INVALID;
> +    tb->jmp_reset_offset[1] = TB_JMP_RESET_OFFSET_INVALID;
> +    tcg_ctx.tb_jmp_reset_offset = tb->jmp_reset_offset;
>  #ifdef USE_DIRECT_JUMP
> -    tcg_ctx.tb_jmp_offset = tb->tb_jmp_offset;
> -    tcg_ctx.tb_next = NULL;
> +    tcg_ctx.tb_jmp_insn_offset = tb->jmp_insn_offset;
> +    tcg_ctx.tb_jmp_target_addr = NULL;
>  #else
> -    tcg_ctx.tb_jmp_offset = NULL;
> -    tcg_ctx.tb_next = tb->tb_next;
> +    tcg_ctx.tb_jmp_insn_offset = NULL;
> +    tcg_ctx.tb_jmp_target_addr = tb->jmp_target_addr;
>  #endif
>
>  #ifdef CONFIG_PROFILER
> @@ -1486,15 +1489,15 @@ static void tb_link_page(TranslationBlock *tb, tb_page_addr_t phys_pc,
>          tb->page_addr[1] = -1;
>      }
>
> -    tb->jmp_first = (TranslationBlock *)((uintptr_t)tb | 2);
> -    tb->jmp_next[0] = NULL;
> -    tb->jmp_next[1] = NULL;
> +    tb->jmp_list_first = (TranslationBlock *)((uintptr_t)tb | 2);
> +    tb->jmp_list_next[0] = NULL;
> +    tb->jmp_list_next[1] = NULL;
>
>      /* init original jump addresses */
> -    if (tb->tb_next_offset[0] != 0xffff) {
> +    if (tb->jmp_reset_offset[0] != TB_JMP_RESET_OFFSET_INVALID) {
>          tb_reset_jump(tb, 0);
>      }
> -    if (tb->tb_next_offset[1] != 0xffff) {
> +    if (tb->jmp_reset_offset[1] != TB_JMP_RESET_OFFSET_INVALID) {
>          tb_reset_jump(tb, 1);
>      }
>
> @@ -1687,9 +1690,9 @@ void dump_exec_info(FILE *f, fprintf_function cpu_fprintf)
>          if (tb->page_addr[1] != -1) {
>              cross_page++;
>          }
> -        if (tb->tb_next_offset[0] != 0xffff) {
> +        if (tb->jmp_reset_offset[0] != TB_JMP_RESET_OFFSET_INVALID) {
>              direct_jmp_count++;
> -            if (tb->tb_next_offset[1] != 0xffff) {
> +            if (tb->jmp_reset_offset[1] != TB_JMP_RESET_OFFSET_INVALID) {
>                  direct_jmp2_count++;
>              }
>          }


--
Alex Bennée

Re: [Qemu-devel] [PATCH 1/8] tcg: Clean up direct block chaining data fields

[Sergey Fedorov]

On 24/03/16 16:42, Alex Bennée wrote:
>> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
>> > index 05a151da4a54..cc3d2ca25917 100644
>> > --- a/include/exec/exec-all.h
>> > +++ b/include/exec/exec-all.h
>> > @@ -257,20 +257,32 @@ struct TranslationBlock {
>> >      struct TranslationBlock *page_next[2];
>> >      tb_page_addr_t page_addr[2];
>> >
>> > -    /* the following data are used to directly call another TB from
>> > -       the code of this one. */
>> > -    uint16_t tb_next_offset[2]; /* offset of original jump target */
>> > +    /* The following data are used to directly call another TB from
>> > +     * the code of this one. This can be done either by emitting direct or
>> > +     * indirect native jump instructions. These jumps are reset so that the TB
>> > +     * just continue its execution. The TB can be linked to another one by
>> > +     * setting one of the jump targets (or patching the jump instruction). Only
>> > +     * two of such jumps are supported.
>> > +     */
>> > +    uint16_t jmp_reset_offset[2]; /* offset of original jump target */
>> > +#define TB_JMP_RESET_OFFSET_INVALID 0xffff /* indicates no jump generated */
>> >  #ifdef USE_DIRECT_JUMP
>> > -    uint16_t tb_jmp_offset[2]; /* offset of jump instruction */
>> > +    uint16_t jmp_insn_offset[2]; /* offset of native jump instruction */
>> >  #else
>> > -    uintptr_t tb_next[2]; /* address of jump generated code */
>> > +    uintptr_t jmp_target_addr[2]; /* target address for indirect jump */
>> >  #endif
>> > -    /* list of TBs jumping to this one. This is a circular list using
>> > -       the two least significant bits of the pointers to tell what is
>> > -       the next pointer: 0 = jmp_next[0], 1 = jmp_next[1], 2 =
>> > -       jmp_first */
>> > -    struct TranslationBlock *jmp_next[2];
>> > -    struct TranslationBlock *jmp_first;
>> > +    /* Each TB has an assosiated circular list of TBs jumping to this one.
>> > +     * jmp_list_first points to the first TB jumping to this one.
>> > +     * jmp_list_next is used to point to the next TB in a list.
>> > +     * Since each TB can have two jumps, it can participate in two lists.
>> > +     * The two least significant bits of a pointer are used to choose which
>> > +     * data field holds a pointer to the next TB:
>> > +     * 0 => jmp_list_next[0], 1 => jmp_list_next[1], 2 => jmp_list_first.
>> > +     * In other words, 0/1 tells which jump is used in the pointed TB,
>> > +     * and 2 means that this is a pointer back to the target TB of this list.
>> > +     */
>> > +    struct TranslationBlock *jmp_list_next[2];
>> > +    struct TranslationBlock *jmp_list_first;
> OK I found that tricky to follow. Where does the value of the pointer
> come from that sets these bottom bits? The TB jumping to this TB sets it?

Yeah, that's not easy to describe. Initially, we set:

    tb->jmp_list_first = tb | 2

That makes an empty list: jmp_list_first just points to the this TB and
the low bits are 2.

After that we can add a TB to the list in tb_add_jump():

    tb->jmp_list_next[n] = tb_next->jmp_list_first;
    tb_next->jmp_list_first = tb | n;

where 'tb' is going to jump to 'tb_next', 'n' (can be 0 or 1) is an
index of jump target of 'tb'.

(I simplified the code here)

Any ideas how to make it more clear in the comment?

Kind regards,
Sergey

Re: [Qemu-devel] [PATCH 2/8] tcg: Use uintptr_t type for jmp_list_{next|first} fields of TB

[Sergey Fedorov]

On 24/03/16 13:39, sergey.fedorov@linaro.org wrote:
> -     * The two least significant bits of a pointer are used to choose which
> -     * data field holds a pointer to the next TB:
> +     * jmp_list_first and jmp_list_next are 4-byte aligned pointers to a
> +     * TranslationBlock structure, and the two least significant bits of them
> +     * are used to encode which data field holds a pointer to the next TB:

Maybe would be better described like this: "..., and the two least
significant bits of them are used to encode which data field of the
pointed TB should be used to traverse the list further from that TB: ..."?

Kind regards,
Sergey

>       * 0 => jmp_list_next[0], 1 => jmp_list_next[1], 2 => jmp_list_first.
>       * In other words, 0/1 tells which jump is used in the pointed TB,
>       * and 2 means that this is a pointer back to the target TB of this list.
>       */
> -    struct TranslationBlock *jmp_list_next[2];
> -    struct TranslationBlock *jmp_list_first;
> +    uintptr_t jmp_list_next[2];
> +    uintptr_t jmp_list_first;

Re: [Qemu-devel] [PATCH 2/8] tcg: Use uintptr_t type for jmp_list_{next|first} fields of TB

[Alex Bennée]

sergey.fedorov@linaro.org writes:

> From: Sergey Fedorov <serge.fdrv@gmail.com>
>
> These fields do not contain pure pointers to a TranslationBlock
> structure. So uintptr_t is the most appropriate type for them.
> Also put an assert to assure that the two least significant bits of the
> pointer are zero before assigning it to jmp_list_first.
>
> Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com>
> Signed-off-by: Sergey Fedorov <sergey.fedorov@linaro.org>
> ---
>  include/exec/exec-all.h | 12 +++++++-----
>  translate-all.c         | 37 +++++++++++++++++++------------------
>  2 files changed, 26 insertions(+), 23 deletions(-)
>
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index cc3d2ca25917..cd96219a89e7 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -275,14 +275,15 @@ struct TranslationBlock {
>       * jmp_list_first points to the first TB jumping to this one.
>       * jmp_list_next is used to point to the next TB in a list.
>       * Since each TB can have two jumps, it can participate in two lists.
> -     * The two least significant bits of a pointer are used to choose which
> -     * data field holds a pointer to the next TB:
> +     * jmp_list_first and jmp_list_next are 4-byte aligned pointers to a
> +     * TranslationBlock structure, and the two least significant bits of them
> +     * are used to encode which data field holds a pointer to the next TB:
>       * 0 => jmp_list_next[0], 1 => jmp_list_next[1], 2 => jmp_list_first.
>       * In other words, 0/1 tells which jump is used in the pointed TB,
>       * and 2 means that this is a pointer back to the target TB of this list.
>       */

Ahh I see you anticipate my previous confusion. Does this mean each time
a jump is resolved for a particular chain the next tb could be in a
different entry in the next TB?

> -    struct TranslationBlock *jmp_list_next[2];
> -    struct TranslationBlock *jmp_list_first;
> +    uintptr_t jmp_list_next[2];
> +    uintptr_t jmp_list_first;
>  };
>
>  #include "qemu/thread.h"
> @@ -396,7 +397,8 @@ static inline void tb_add_jump(TranslationBlock *tb, int n,
>
>          /* add in TB jmp circular list */
>          tb->jmp_list_next[n] = tb_next->jmp_list_first;
> -        tb_next->jmp_list_first = (TranslationBlock *)((uintptr_t)tb | n);
> +        assert(((uintptr_t)tb & 3) == 0);
> +        tb_next->jmp_list_first = (uintptr_t)tb | n;
>      }
>  }
>
> diff --git a/translate-all.c b/translate-all.c
> index 31cdf8ae217e..7c008927e3f3 100644
> --- a/translate-all.c
> +++ b/translate-all.c
> @@ -926,17 +926,16 @@ static inline void tb_page_remove(TranslationBlock **ptb, TranslationBlock *tb)
>
>  static inline void tb_jmp_remove(TranslationBlock *tb, int n)
>  {
> -    TranslationBlock *tb1, **ptb;
> +    TranslationBlock *tb1;
> +    uintptr_t *ptb;
>      unsigned int n1;
>
>      ptb = &tb->jmp_list_next[n];
> -    tb1 = *ptb;
> -    if (tb1) {
> +    if (*ptb) {
>          /* find tb(n) in circular list */
>          for (;;) {
> -            tb1 = *ptb;
> -            n1 = (uintptr_t)tb1 & 3;
> -            tb1 = (TranslationBlock *)((uintptr_t)tb1 & ~3);
> +            n1 = *ptb & 3;
> +            tb1 = (TranslationBlock *)(*ptb & ~3);
>              if (n1 == n && tb1 == tb) {
>                  break;
>              }
> @@ -949,7 +948,7 @@ static inline void tb_jmp_remove(TranslationBlock *tb, int n)
>          /* now we can suppress tb(n) from the list */
>          *ptb = tb->jmp_list_next[n];
>
> -        tb->jmp_list_next[n] = NULL;
> +        tb->jmp_list_next[n] = (uintptr_t)NULL;
>      }
>  }
>
> @@ -968,7 +967,7 @@ void tb_phys_invalidate(TranslationBlock *tb, tb_page_addr_t page_addr)
>      PageDesc *p;
>      unsigned int h, n1;
>      tb_page_addr_t phys_pc;
> -    TranslationBlock *tb1, *tb2;
> +    uintptr_t tb1, tb2;
>
>      /* remove the TB from the hash list */
>      phys_pc = tb->page_addr[0] + (tb->pc & ~TARGET_PAGE_MASK);
> @@ -1004,19 +1003,20 @@ void tb_phys_invalidate(TranslationBlock *tb, tb_page_addr_t page_addr)
>      /* suppress any remaining jumps to this TB */
>      tb1 = tb->jmp_list_first;
>      for (;;) {
> -        n1 = (uintptr_t)tb1 & 3;
> +        TranslationBlock *tmp_tb;
> +        n1 = tb1 & 3;
>          if (n1 == 2) {
>              break;
>          }
> -        tb1 = (TranslationBlock *)((uintptr_t)tb1 & ~3);
> -        tb2 = tb1->jmp_list_next[n1];
> -        tb_reset_jump(tb1, n1);
> -        tb1->jmp_list_next[n1] = NULL;
> +        tmp_tb = (TranslationBlock *)(tb1 & ~3);
> +        tb2 = tmp_tb->jmp_list_next[n1];
> +        tb_reset_jump(tmp_tb, n1);
> +        tmp_tb->jmp_list_next[n1] = (uintptr_t)NULL;
>          tb1 = tb2;
>      }
>
> -    /* fail safe */
> -    tb->jmp_list_first = (TranslationBlock *)((uintptr_t)tb | 2);
> +    assert(((uintptr_t)tb & 3) == 0);
> +    tb->jmp_list_first = (uintptr_t)tb | 2; /* fail safe */
>
>      tcg_ctx.tb_ctx.tb_phys_invalidate_count++;
>  }
> @@ -1489,9 +1489,10 @@ static void tb_link_page(TranslationBlock *tb, tb_page_addr_t phys_pc,
>          tb->page_addr[1] = -1;
>      }
>
> -    tb->jmp_list_first = (TranslationBlock *)((uintptr_t)tb | 2);
> -    tb->jmp_list_next[0] = NULL;
> -    tb->jmp_list_next[1] = NULL;
> +    assert(((uintptr_t)tb & 3) == 0);
> +    tb->jmp_list_first = (uintptr_t)tb | 2;
> +    tb->jmp_list_next[0] = (uintptr_t)NULL;
> +    tb->jmp_list_next[1] = (uintptr_t)NULL;
>
>      /* init original jump addresses */
>      if (tb->jmp_reset_offset[0] != TB_JMP_RESET_OFFSET_INVALID) {


--
Alex Bennée

Re: [Qemu-devel] [PATCH 1/8] tcg: Clean up direct block chaining data fields

[Alex Bennée]

Sergey Fedorov <serge.fdrv@gmail.com> writes:

> On 24/03/16 16:42, Alex Bennée wrote:
>>> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
>>> > index 05a151da4a54..cc3d2ca25917 100644
>>> > --- a/include/exec/exec-all.h
>>> > +++ b/include/exec/exec-all.h
>>> > @@ -257,20 +257,32 @@ struct TranslationBlock {
>>> >      struct TranslationBlock *page_next[2];
>>> >      tb_page_addr_t page_addr[2];
>>> >
>>> > -    /* the following data are used to directly call another TB from
>>> > -       the code of this one. */
>>> > -    uint16_t tb_next_offset[2]; /* offset of original jump target */
>>> > +    /* The following data are used to directly call another TB from
>>> > +     * the code of this one. This can be done either by emitting direct or
>>> > +     * indirect native jump instructions. These jumps are reset so that the TB
>>> > +     * just continue its execution. The TB can be linked to another one by
>>> > +     * setting one of the jump targets (or patching the jump instruction). Only
>>> > +     * two of such jumps are supported.
>>> > +     */
>>> > +    uint16_t jmp_reset_offset[2]; /* offset of original jump target */
>>> > +#define TB_JMP_RESET_OFFSET_INVALID 0xffff /* indicates no jump generated */
>>> >  #ifdef USE_DIRECT_JUMP
>>> > -    uint16_t tb_jmp_offset[2]; /* offset of jump instruction */
>>> > +    uint16_t jmp_insn_offset[2]; /* offset of native jump instruction */
>>> >  #else
>>> > -    uintptr_t tb_next[2]; /* address of jump generated code */
>>> > +    uintptr_t jmp_target_addr[2]; /* target address for indirect jump */
>>> >  #endif
>>> > -    /* list of TBs jumping to this one. This is a circular list using
>>> > -       the two least significant bits of the pointers to tell what is
>>> > -       the next pointer: 0 = jmp_next[0], 1 = jmp_next[1], 2 =
>>> > -       jmp_first */
>>> > -    struct TranslationBlock *jmp_next[2];
>>> > -    struct TranslationBlock *jmp_first;
>>> > +    /* Each TB has an assosiated circular list of TBs jumping to this one.
>>> > +     * jmp_list_first points to the first TB jumping to this one.
>>> > +     * jmp_list_next is used to point to the next TB in a list.
>>> > +     * Since each TB can have two jumps, it can participate in two lists.
>>> > +     * The two least significant bits of a pointer are used to choose which
>>> > +     * data field holds a pointer to the next TB:
>>> > +     * 0 => jmp_list_next[0], 1 => jmp_list_next[1], 2 => jmp_list_first.
>>> > +     * In other words, 0/1 tells which jump is used in the pointed TB,
>>> > +     * and 2 means that this is a pointer back to the target TB of this list.
>>> > +     */
>>> > +    struct TranslationBlock *jmp_list_next[2];
>>> > +    struct TranslationBlock *jmp_list_first;
>> OK I found that tricky to follow. Where does the value of the pointer
>> come from that sets these bottom bits? The TB jumping to this TB sets it?
>
> Yeah, that's not easy to describe. Initially, we set:
>
>     tb->jmp_list_first = tb | 2
>
> That makes an empty list: jmp_list_first just points to the this TB and
> the low bits are 2.
>
> After that we can add a TB to the list in tb_add_jump():
>
>     tb->jmp_list_next[n] = tb_next->jmp_list_first;
>     tb_next->jmp_list_first = tb | n;
>
> where 'tb' is going to jump to 'tb_next', 'n' (can be 0 or 1) is an
> index of jump target of 'tb'.

Where I get confused it what is the point of jmp_list_first? If these
are two circular lists do we care which the first in the list is? The
exit condition when coming out of searching seems when ntb with index =
orig tb with index.

>
> (I simplified the code here)
>
> Any ideas how to make it more clear in the comment?
>
> Kind regards,
> Sergey


--
Alex Bennée

Re: [Qemu-devel] [PATCH 3/8] tcg: Rearrange tb_link_page() to avoid forward declaration

[Alex Bennée]

sergey.fedorov@linaro.org writes:

> From: Sergey Fedorov <serge.fdrv@gmail.com>
>
> Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com>
> Signed-off-by: Sergey Fedorov <sergey.fedorov@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  translate-all.c | 204 ++++++++++++++++++++++++++++----------------------------
>  1 file changed, 101 insertions(+), 103 deletions(-)
>
> diff --git a/translate-all.c b/translate-all.c
> index 7c008927e3f3..ca01dd325b8d 100644
> --- a/translate-all.c
> +++ b/translate-all.c
> @@ -153,8 +153,6 @@ void tb_lock_reset(void)
>  #endif
>  }
>
> -static void tb_link_page(TranslationBlock *tb, tb_page_addr_t phys_pc,
> -                         tb_page_addr_t phys_page2);
>  static TranslationBlock *tb_find_pc(uintptr_t tc_ptr);
>
>  void cpu_gen_init(void)
> @@ -1050,6 +1048,107 @@ static void build_page_bitmap(PageDesc *p)
>      }
>  }
>
> +/* add the tb in the target page and protect it if necessary
> + *
> + * Called with mmap_lock held for user-mode emulation.
> + */
> +static inline void tb_alloc_page(TranslationBlock *tb,
> +                                 unsigned int n, tb_page_addr_t page_addr)
> +{
> +    PageDesc *p;
> +#ifndef CONFIG_USER_ONLY
> +    bool page_already_protected;
> +#endif
> +
> +    tb->page_addr[n] = page_addr;
> +    p = page_find_alloc(page_addr >> TARGET_PAGE_BITS, 1);
> +    tb->page_next[n] = p->first_tb;
> +#ifndef CONFIG_USER_ONLY
> +    page_already_protected = p->first_tb != NULL;
> +#endif
> +    p->first_tb = (TranslationBlock *)((uintptr_t)tb | n);
> +    invalidate_page_bitmap(p);
> +
> +#if defined(CONFIG_USER_ONLY)
> +    if (p->flags & PAGE_WRITE) {
> +        target_ulong addr;
> +        PageDesc *p2;
> +        int prot;
> +
> +        /* force the host page as non writable (writes will have a
> +           page fault + mprotect overhead) */
> +        page_addr &= qemu_host_page_mask;
> +        prot = 0;
> +        for (addr = page_addr; addr < page_addr + qemu_host_page_size;
> +            addr += TARGET_PAGE_SIZE) {
> +
> +            p2 = page_find(addr >> TARGET_PAGE_BITS);
> +            if (!p2) {
> +                continue;
> +            }
> +            prot |= p2->flags;
> +            p2->flags &= ~PAGE_WRITE;
> +          }
> +        mprotect(g2h(page_addr), qemu_host_page_size,
> +                 (prot & PAGE_BITS) & ~PAGE_WRITE);
> +#ifdef DEBUG_TB_INVALIDATE
> +        printf("protecting code page: 0x" TARGET_FMT_lx "\n",
> +               page_addr);
> +#endif
> +    }
> +#else
> +    /* if some code is already present, then the pages are already
> +       protected. So we handle the case where only the first TB is
> +       allocated in a physical page */
> +    if (!page_already_protected) {
> +        tlb_protect_code(page_addr);
> +    }
> +#endif
> +}
> +
> +/* add a new TB and link it to the physical page tables. phys_page2 is
> + * (-1) to indicate that only one page contains the TB.
> + *
> + * Called with mmap_lock held for user-mode emulation.
> + */
> +static void tb_link_page(TranslationBlock *tb, tb_page_addr_t phys_pc,
> +                         tb_page_addr_t phys_page2)
> +{
> +    unsigned int h;
> +    TranslationBlock **ptb;
> +
> +    /* add in the physical hash table */
> +    h = tb_phys_hash_func(phys_pc);
> +    ptb = &tcg_ctx.tb_ctx.tb_phys_hash[h];
> +    tb->phys_hash_next = *ptb;
> +    *ptb = tb;
> +
> +    /* add in the page list */
> +    tb_alloc_page(tb, 0, phys_pc & TARGET_PAGE_MASK);
> +    if (phys_page2 != -1) {
> +        tb_alloc_page(tb, 1, phys_page2);
> +    } else {
> +        tb->page_addr[1] = -1;
> +    }
> +
> +    assert(((uintptr_t)tb & 3) == 0);
> +    tb->jmp_list_first = (uintptr_t)tb | 2;
> +    tb->jmp_list_next[0] = (uintptr_t)NULL;
> +    tb->jmp_list_next[1] = (uintptr_t)NULL;
> +
> +    /* init original jump addresses */
> +    if (tb->jmp_reset_offset[0] != TB_JMP_RESET_OFFSET_INVALID) {
> +        tb_reset_jump(tb, 0);
> +    }
> +    if (tb->jmp_reset_offset[1] != TB_JMP_RESET_OFFSET_INVALID) {
> +        tb_reset_jump(tb, 1);
> +    }
> +
> +#ifdef DEBUG_TB_CHECK
> +    tb_page_check();
> +#endif
> +}
> +
>  /* Called with mmap_lock held for user mode emulation.  */
>  TranslationBlock *tb_gen_code(CPUState *cpu,
>                                target_ulong pc, target_ulong cs_base,
> @@ -1406,107 +1505,6 @@ static void tb_invalidate_phys_page(tb_page_addr_t addr,
>  }
>  #endif
>
> -/* add the tb in the target page and protect it if necessary
> - *
> - * Called with mmap_lock held for user-mode emulation.
> - */
> -static inline void tb_alloc_page(TranslationBlock *tb,
> -                                 unsigned int n, tb_page_addr_t page_addr)
> -{
> -    PageDesc *p;
> -#ifndef CONFIG_USER_ONLY
> -    bool page_already_protected;
> -#endif
> -
> -    tb->page_addr[n] = page_addr;
> -    p = page_find_alloc(page_addr >> TARGET_PAGE_BITS, 1);
> -    tb->page_next[n] = p->first_tb;
> -#ifndef CONFIG_USER_ONLY
> -    page_already_protected = p->first_tb != NULL;
> -#endif
> -    p->first_tb = (TranslationBlock *)((uintptr_t)tb | n);
> -    invalidate_page_bitmap(p);
> -
> -#if defined(CONFIG_USER_ONLY)
> -    if (p->flags & PAGE_WRITE) {
> -        target_ulong addr;
> -        PageDesc *p2;
> -        int prot;
> -
> -        /* force the host page as non writable (writes will have a
> -           page fault + mprotect overhead) */
> -        page_addr &= qemu_host_page_mask;
> -        prot = 0;
> -        for (addr = page_addr; addr < page_addr + qemu_host_page_size;
> -            addr += TARGET_PAGE_SIZE) {
> -
> -            p2 = page_find(addr >> TARGET_PAGE_BITS);
> -            if (!p2) {
> -                continue;
> -            }
> -            prot |= p2->flags;
> -            p2->flags &= ~PAGE_WRITE;
> -          }
> -        mprotect(g2h(page_addr), qemu_host_page_size,
> -                 (prot & PAGE_BITS) & ~PAGE_WRITE);
> -#ifdef DEBUG_TB_INVALIDATE
> -        printf("protecting code page: 0x" TARGET_FMT_lx "\n",
> -               page_addr);
> -#endif
> -    }
> -#else
> -    /* if some code is already present, then the pages are already
> -       protected. So we handle the case where only the first TB is
> -       allocated in a physical page */
> -    if (!page_already_protected) {
> -        tlb_protect_code(page_addr);
> -    }
> -#endif
> -}
> -
> -/* add a new TB and link it to the physical page tables. phys_page2 is
> - * (-1) to indicate that only one page contains the TB.
> - *
> - * Called with mmap_lock held for user-mode emulation.
> - */
> -static void tb_link_page(TranslationBlock *tb, tb_page_addr_t phys_pc,
> -                         tb_page_addr_t phys_page2)
> -{
> -    unsigned int h;
> -    TranslationBlock **ptb;
> -
> -    /* add in the physical hash table */
> -    h = tb_phys_hash_func(phys_pc);
> -    ptb = &tcg_ctx.tb_ctx.tb_phys_hash[h];
> -    tb->phys_hash_next = *ptb;
> -    *ptb = tb;
> -
> -    /* add in the page list */
> -    tb_alloc_page(tb, 0, phys_pc & TARGET_PAGE_MASK);
> -    if (phys_page2 != -1) {
> -        tb_alloc_page(tb, 1, phys_page2);
> -    } else {
> -        tb->page_addr[1] = -1;
> -    }
> -
> -    assert(((uintptr_t)tb & 3) == 0);
> -    tb->jmp_list_first = (uintptr_t)tb | 2;
> -    tb->jmp_list_next[0] = (uintptr_t)NULL;
> -    tb->jmp_list_next[1] = (uintptr_t)NULL;
> -
> -    /* init original jump addresses */
> -    if (tb->jmp_reset_offset[0] != TB_JMP_RESET_OFFSET_INVALID) {
> -        tb_reset_jump(tb, 0);
> -    }
> -    if (tb->jmp_reset_offset[1] != TB_JMP_RESET_OFFSET_INVALID) {
> -        tb_reset_jump(tb, 1);
> -    }
> -
> -#ifdef DEBUG_TB_CHECK
> -    tb_page_check();
> -#endif
> -}
> -
>  /* find the TB 'tb' such that tb[0].tc_ptr <= tc_ptr <
>     tb[1].tc_ptr. Return NULL if not found */
>  static TranslationBlock *tb_find_pc(uintptr_t tc_ptr)


--
Alex Bennée

Re: [Qemu-devel] [PATCH 1/8] tcg: Clean up direct block chaining data fields

[Sergey Fedorov]

On 24/03/16 18:01, Alex Bennée wrote:
> Sergey Fedorov <serge.fdrv@gmail.com> writes:
>
>> On 24/03/16 16:42, Alex Bennée wrote:
>>>> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
>>>>> index 05a151da4a54..cc3d2ca25917 100644
>>>>> --- a/include/exec/exec-all.h
>>>>> +++ b/include/exec/exec-all.h
>>>>> @@ -257,20 +257,32 @@ struct TranslationBlock {
>>>>>      struct TranslationBlock *page_next[2];
>>>>>      tb_page_addr_t page_addr[2];
>>>>>
>>>>> -    /* the following data are used to directly call another TB from
>>>>> -       the code of this one. */
>>>>> -    uint16_t tb_next_offset[2]; /* offset of original jump target */
>>>>> +    /* The following data are used to directly call another TB from
>>>>> +     * the code of this one. This can be done either by emitting direct or
>>>>> +     * indirect native jump instructions. These jumps are reset so that the TB
>>>>> +     * just continue its execution. The TB can be linked to another one by
>>>>> +     * setting one of the jump targets (or patching the jump instruction). Only
>>>>> +     * two of such jumps are supported.
>>>>> +     */
>>>>> +    uint16_t jmp_reset_offset[2]; /* offset of original jump target */
>>>>> +#define TB_JMP_RESET_OFFSET_INVALID 0xffff /* indicates no jump generated */
>>>>>  #ifdef USE_DIRECT_JUMP
>>>>> -    uint16_t tb_jmp_offset[2]; /* offset of jump instruction */
>>>>> +    uint16_t jmp_insn_offset[2]; /* offset of native jump instruction */
>>>>>  #else
>>>>> -    uintptr_t tb_next[2]; /* address of jump generated code */
>>>>> +    uintptr_t jmp_target_addr[2]; /* target address for indirect jump */
>>>>>  #endif
>>>>> -    /* list of TBs jumping to this one. This is a circular list using
>>>>> -       the two least significant bits of the pointers to tell what is
>>>>> -       the next pointer: 0 = jmp_next[0], 1 = jmp_next[1], 2 =
>>>>> -       jmp_first */
>>>>> -    struct TranslationBlock *jmp_next[2];
>>>>> -    struct TranslationBlock *jmp_first;
>>>>> +    /* Each TB has an assosiated circular list of TBs jumping to this one.
>>>>> +     * jmp_list_first points to the first TB jumping to this one.
>>>>> +     * jmp_list_next is used to point to the next TB in a list.
>>>>> +     * Since each TB can have two jumps, it can participate in two lists.
>>>>> +     * The two least significant bits of a pointer are used to choose which
>>>>> +     * data field holds a pointer to the next TB:
>>>>> +     * 0 => jmp_list_next[0], 1 => jmp_list_next[1], 2 => jmp_list_first.
>>>>> +     * In other words, 0/1 tells which jump is used in the pointed TB,
>>>>> +     * and 2 means that this is a pointer back to the target TB of this list.
>>>>> +     */
>>>>> +    struct TranslationBlock *jmp_list_next[2];
>>>>> +    struct TranslationBlock *jmp_list_first;
>>> OK I found that tricky to follow. Where does the value of the pointer
>>> come from that sets these bottom bits? The TB jumping to this TB sets it?
>> Yeah, that's not easy to describe. Initially, we set:
>>
>>     tb->jmp_list_first = tb | 2
>>
>> That makes an empty list: jmp_list_first just points to the this TB and
>> the low bits are 2.
>>
>> After that we can add a TB to the list in tb_add_jump():
>>
>>     tb->jmp_list_next[n] = tb_next->jmp_list_first;
>>     tb_next->jmp_list_first = tb | n;
>>
>> where 'tb' is going to jump to 'tb_next', 'n' (can be 0 or 1) is an
>> index of jump target of 'tb'.
> Where I get confused it what is the point of jmp_list_first? If these
> are two circular lists do we care which the first in the list is? The
> exit condition when coming out of searching seems when ntb with index =
> orig tb with index.

So 'tb->jmp_list_first' points to the first TB jumping to 'tb'. Then we
use 'jmp_list_next[n]' of that TB to traverse the list further.
Eventually, we get 'jmp_list_next[n] & 3 == 2' which means
jmp_list_next[n] points back to the target TB. Hope it helps :)

Kind regards,
Sergey

Re: [Qemu-devel] [PATCH 4/8] tcg: Init TB's direct jumps before making it visible

[Alex Bennée]

sergey.fedorov@linaro.org writes:

> From: Sergey Fedorov <serge.fdrv@gmail.com>
>
> Initialize TB's direct jump list data fields and reset the jumps before
> tb_link_page() puts it into the physical hash table and the physical
> page list. So TB is completely initialized before it becomes visible.
>
> Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com>
> Signed-off-by: Sergey Fedorov <sergey.fedorov@linaro.org>
> ---
>  translate-all.c | 27 ++++++++++++++-------------
>  1 file changed, 14 insertions(+), 13 deletions(-)
>
> diff --git a/translate-all.c b/translate-all.c
> index ca01dd325b8d..f68716e1819f 100644
> --- a/translate-all.c
> +++ b/translate-all.c
> @@ -1131,19 +1131,6 @@ static void tb_link_page(TranslationBlock *tb, tb_page_addr_t phys_pc,
>          tb->page_addr[1] = -1;
>      }
>
> -    assert(((uintptr_t)tb & 3) == 0);
> -    tb->jmp_list_first = (uintptr_t)tb | 2;
> -    tb->jmp_list_next[0] = (uintptr_t)NULL;
> -    tb->jmp_list_next[1] = (uintptr_t)NULL;
> -
> -    /* init original jump addresses */
> -    if (tb->jmp_reset_offset[0] != TB_JMP_RESET_OFFSET_INVALID) {
> -        tb_reset_jump(tb, 0);
> -    }
> -    if (tb->jmp_reset_offset[1] != TB_JMP_RESET_OFFSET_INVALID) {
> -        tb_reset_jump(tb, 1);
> -    }
> -
>  #ifdef DEBUG_TB_CHECK
>      tb_page_check();
>  #endif
> @@ -1251,6 +1238,20 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
>          ROUND_UP((uintptr_t)gen_code_buf + gen_code_size + search_size,
>                   CODE_GEN_ALIGN);
>
> +    /* init jump list */
> +    assert(((uintptr_t)tb & 3) == 0);
> +    tb->jmp_list_first = (uintptr_t)tb | 2;
> +    tb->jmp_list_next[0] = (uintptr_t)NULL;
> +    tb->jmp_list_next[1] = (uintptr_t)NULL;

maybe these should be further up the function with the other jmp setting code?

> +
> +    /* init original jump addresses */
> +    if (tb->jmp_reset_offset[0] != TB_JMP_RESET_OFFSET_INVALID) {
> +        tb_reset_jump(tb, 0);
> +    }
> +    if (tb->jmp_reset_offset[1] != TB_JMP_RESET_OFFSET_INVALID) {
> +        tb_reset_jump(tb, 1);
> +    }

Why would tb->jmp_reset_offset[0] == TB_JMP_RESET_OFFSET_INVALID not be
the case as it is set a few lines further up.

> +
>      /* check next page if needed */
>      virt_page2 = (pc + tb->size - 1) & TARGET_PAGE_MASK;
>      phys_page2 = -1;


--
Alex Bennée

Re: [Qemu-devel] [PATCH 1/8] tcg: Clean up direct block chaining data fields

[Paolo Bonzini]

On 24/03/2016 16:01, Alex Bennée wrote:
>>> >> OK I found that tricky to follow. Where does the value of the pointer
>>> >> come from that sets these bottom bits? The TB jumping to this TB sets it?
>
> Where I get confused it what is the point of jmp_list_first? If these
> are two circular lists do we care which the first in the list is? The
> exit condition when coming out of searching seems when ntb with index =
> orig tb with index.

Say you have a list for blocks that jump to TB. The next pointer is in
jmp_list_next[0] for blocks whose first jump is to TB. It is in
jmp_list_next[1] for blocks whose second jump is to TB.

However, because it is a circular list, you also need TB itself to be a
part of the list. For TB, the next pointer is in jmp_list_first.

Because TB probably doesn't jump to itself, the first link of the list
of blocks that jumps to TB is not in jmp_list_next[].  Thus QEMU places
it in tb->jmp_list_first.

Say you have three tbs.  TB1's first jump and TB2's second jump lead to
TB0.  Then the list starting at tb0->jmp_list_first goes like this:

    tb0->jmp_list_first = tb1 | 0;
      .--------------------'    |
     |                 .--------'
    tb1->jmp_list_next[0] = tb2 | 1;
      .--------------------'      |
      |                 .---------'
    tb2->jmp_list_next[1] = tb0 | 2;

There is also a case where a TB jumps to itself; it then appears twice
in the list with different values in the low bits, such as this:

    tb->jmp_list_first = tb | 0;
     .--------------------'   |
     |                .-------'
    tb->jmp_list_next[0] = tb | 2;

Other blocks jumping to TB would appear in the same list, too, either
before or after the tb|0 link.

Paolo

Re: [Qemu-devel] [PATCH 2/8] tcg: Use uintptr_t type for jmp_list_{next|first} fields of TB

[Sergey Fedorov]

On 24/03/16 17:58, Alex Bennée wrote:
>> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
>> > index cc3d2ca25917..cd96219a89e7 100644
>> > --- a/include/exec/exec-all.h
>> > +++ b/include/exec/exec-all.h
>> > @@ -275,14 +275,15 @@ struct TranslationBlock {
>> >       * jmp_list_first points to the first TB jumping to this one.
>> >       * jmp_list_next is used to point to the next TB in a list.
>> >       * Since each TB can have two jumps, it can participate in two lists.
>> > -     * The two least significant bits of a pointer are used to choose which
>> > -     * data field holds a pointer to the next TB:
>> > +     * jmp_list_first and jmp_list_next are 4-byte aligned pointers to a
>> > +     * TranslationBlock structure, and the two least significant bits of them
>> > +     * are used to encode which data field holds a pointer to the next TB:
>> >       * 0 => jmp_list_next[0], 1 => jmp_list_next[1], 2 => jmp_list_first.
>> >       * In other words, 0/1 tells which jump is used in the pointed TB,
>> >       * and 2 means that this is a pointer back to the target TB of this list.
>> >       */
> Ahh I see you anticipate my previous confusion. Does this mean each time
> a jump is resolved for a particular chain the next tb could be in a
> different entry in the next TB?

I'm not sure I got your question right... When we patch the n-th jump of
a TB we use it's 'jmp_list_next[n]' to add it to the list of the TBs
jumping to the same target TB. And we use 'jmp_list_first' of the target
TB to track all those TBs jumping to it.

Kind regards,
Sergey

Re: [Qemu-devel] [PATCH 1/8] tcg: Clean up direct block chaining data fields

[Alex Bennée]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 24/03/2016 16:01, Alex Bennée wrote:
>>>> >> OK I found that tricky to follow. Where does the value of the pointer
>>>> >> come from that sets these bottom bits? The TB jumping to this TB sets it?
>>
>> Where I get confused it what is the point of jmp_list_first? If these
>> are two circular lists do we care which the first in the list is? The
>> exit condition when coming out of searching seems when ntb with index =
>> orig tb with index.
>
> Say you have a list for blocks that jump to TB. The next pointer is in
> jmp_list_next[0] for blocks whose first jump is to TB. It is in
> jmp_list_next[1] for blocks whose second jump is to TB.
>
> However, because it is a circular list, you also need TB itself to be a
> part of the list. For TB, the next pointer is in jmp_list_first.
>
> Because TB probably doesn't jump to itself, the first link of the list
> of blocks that jumps to TB is not in jmp_list_next[].  Thus QEMU places
> it in tb->jmp_list_first.
>
> Say you have three tbs.  TB1's first jump and TB2's second jump lead to
> TB0.  Then the list starting at tb0->jmp_list_first goes like this:
>
>     tb0->jmp_list_first = tb1 | 0;
>       .--------------------'    |
>      |                 .--------'
>     tb1->jmp_list_next[0] = tb2 | 1;
>       .--------------------'      |
>       |                 .---------'
>     tb2->jmp_list_next[1] = tb0 | 2;
>
> There is also a case where a TB jumps to itself; it then appears twice
> in the list with different values in the low bits, such as this:
>
>     tb->jmp_list_first = tb | 0;
>      .--------------------'   |
>      |                .-------'
>     tb->jmp_list_next[0] = tb | 2;
>
> Other blocks jumping to TB would appear in the same list, too, either
> before or after the tb|0 link.

Right I follow now. Extra ascii art always helps ;-)

--
Alex Bennée

Re: [Qemu-devel] [PATCH 6/8] tcg: Rename tb_jmp_remove() to tb_remove_from_jmp_list()

[Alex Bennée]

sergey.fedorov@linaro.org writes:

> From: Sergey Fedorov <serge.fdrv@gmail.com>
>
> tb_jmp_remove() was only used to remove the TB from a list of all TBs
> jumping to the same TB which is n-th jump destination of the given TB.
> Put a comment briefly describing the function behavior and rename it to
> better reflect its purpose.
>
> Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com>
> Signed-off-by: Sergey Fedorov <sergey.fedorov@linaro.org>
> ---
>  translate-all.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/translate-all.c b/translate-all.c
> index f68716e1819f..28154ed75888 100644
> --- a/translate-all.c
> +++ b/translate-all.c
> @@ -922,7 +922,8 @@ static inline void tb_page_remove(TranslationBlock **ptb, TranslationBlock *tb)
>      }
>  }
>
> -static inline void tb_jmp_remove(TranslationBlock *tb, int n)
> +/* remove the TB from a list of TBs jumping to the n-th jump target of the TB */
> +static inline void tb_remove_from_jmp_list(TranslationBlock *tb, int n)
>  {
>      TranslationBlock *tb1;
>      uintptr_t *ptb;
> @@ -995,8 +996,8 @@ void tb_phys_invalidate(TranslationBlock *tb, tb_page_addr_t page_addr)
>      }
>
>      /* suppress this TB from the two jump lists */
> -    tb_jmp_remove(tb, 0);
> -    tb_jmp_remove(tb, 1);
> +    tb_remove_from_jmp_list(tb, 0);
> +    tb_remove_from_jmp_list(tb, 1);
>
>      /* suppress any remaining jumps to this TB */
>      tb1 = tb->jmp_list_first;


Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

--
Alex Bennée

Re: [Qemu-devel] [PATCH 7/8] tcg: Extract removing of jumps to TB from tb_phys_invalidate()

[Alex Bennée]

sergey.fedorov@linaro.org writes:

> From: Sergey Fedorov <serge.fdrv@gmail.com>
>
> Move the code for removing jumps to a TB out of tb_phys_invalidate() to
> a separate static inline function tb_jmp_unlink(). This simplifies
> tb_phys_invalidate() and improves code structure.
>
> Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com>
> Signed-off-by: Sergey Fedorov <sergey.fedorov@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  translate-all.c | 44 ++++++++++++++++++++++++++------------------
>  1 file changed, 26 insertions(+), 18 deletions(-)
>
> diff --git a/translate-all.c b/translate-all.c
> index 28154ed75888..8b4bfa713bf7 100644
> --- a/translate-all.c
> +++ b/translate-all.c
> @@ -959,14 +959,37 @@ static inline void tb_reset_jump(TranslationBlock *tb, int n)
>      tb_set_jmp_target(tb, n, addr);
>  }
>
> +/* remove any jumps to the TB */
> +static inline void tb_jmp_unlink(TranslationBlock *tb)
> +{
> +    uintptr_t tb1, tb2;
> +    unsigned int n1;
> +
> +    tb1 = tb->jmp_list_first;
> +    for (;;) {
> +        TranslationBlock *tmp_tb;
> +        n1 = tb1 & 3;
> +        if (n1 == 2) {
> +            break;
> +        }
> +        tmp_tb = (TranslationBlock *)(tb1 & ~3);
> +        tb2 = tmp_tb->jmp_list_next[n1];
> +        tb_reset_jump(tmp_tb, n1);
> +        tmp_tb->jmp_list_next[n1] = (uintptr_t)NULL;
> +        tb1 = tb2;
> +    }
> +
> +    assert(((uintptr_t)tb & 3) == 0);
> +    tb->jmp_list_first = (uintptr_t)tb | 2; /* fail safe */
> +}
> +
>  /* invalidate one TB */
>  void tb_phys_invalidate(TranslationBlock *tb, tb_page_addr_t page_addr)
>  {
>      CPUState *cpu;
>      PageDesc *p;
> -    unsigned int h, n1;
> +    unsigned int h;
>      tb_page_addr_t phys_pc;
> -    uintptr_t tb1, tb2;
>
>      /* remove the TB from the hash list */
>      phys_pc = tb->page_addr[0] + (tb->pc & ~TARGET_PAGE_MASK);
> @@ -1000,22 +1023,7 @@ void tb_phys_invalidate(TranslationBlock *tb, tb_page_addr_t page_addr)
>      tb_remove_from_jmp_list(tb, 1);
>
>      /* suppress any remaining jumps to this TB */
> -    tb1 = tb->jmp_list_first;
> -    for (;;) {
> -        TranslationBlock *tmp_tb;
> -        n1 = tb1 & 3;
> -        if (n1 == 2) {
> -            break;
> -        }
> -        tmp_tb = (TranslationBlock *)(tb1 & ~3);
> -        tb2 = tmp_tb->jmp_list_next[n1];
> -        tb_reset_jump(tmp_tb, n1);
> -        tmp_tb->jmp_list_next[n1] = (uintptr_t)NULL;
> -        tb1 = tb2;
> -    }
> -
> -    assert(((uintptr_t)tb & 3) == 0);
> -    tb->jmp_list_first = (uintptr_t)tb | 2; /* fail safe */
> +    tb_jmp_unlink(tb);
>
>      tcg_ctx.tb_ctx.tb_phys_invalidate_count++;
>  }


--
Alex Bennée

Re: [Qemu-devel] [PATCH 4/8] tcg: Init TB's direct jumps before making it visible

[Sergey Fedorov]

On 24/03/16 18:11, Alex Bennée wrote:
> sergey.fedorov@linaro.org writes:
>> From: Sergey Fedorov <serge.fdrv@gmail.com>
>>
>> diff --git a/translate-all.c b/translate-all.c
>> index ca01dd325b8d..f68716e1819f 100644
>> --- a/translate-all.c
>> +++ b/translate-all.c
>> @@ -1131,19 +1131,6 @@ static void tb_link_page(TranslationBlock *tb, tb_page_addr_t phys_pc,
>>          tb->page_addr[1] = -1;
>>      }
>>
>> -    assert(((uintptr_t)tb & 3) == 0);
>> -    tb->jmp_list_first = (uintptr_t)tb | 2;
>> -    tb->jmp_list_next[0] = (uintptr_t)NULL;
>> -    tb->jmp_list_next[1] = (uintptr_t)NULL;
>> -
>> -    /* init original jump addresses */
>> -    if (tb->jmp_reset_offset[0] != TB_JMP_RESET_OFFSET_INVALID) {
>> -        tb_reset_jump(tb, 0);
>> -    }
>> -    if (tb->jmp_reset_offset[1] != TB_JMP_RESET_OFFSET_INVALID) {
>> -        tb_reset_jump(tb, 1);
>> -    }
>> -
>>  #ifdef DEBUG_TB_CHECK
>>      tb_page_check();
>>  #endif
>> @@ -1251,6 +1238,20 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
>>          ROUND_UP((uintptr_t)gen_code_buf + gen_code_size + search_size,
>>                   CODE_GEN_ALIGN);
>>
>> +    /* init jump list */
>> +    assert(((uintptr_t)tb & 3) == 0);
>> +    tb->jmp_list_first = (uintptr_t)tb | 2;
>> +    tb->jmp_list_next[0] = (uintptr_t)NULL;
>> +    tb->jmp_list_next[1] = (uintptr_t)NULL;
> maybe these should be further up the function with the other jmp setting code?

I meant to keep them together with the following lines.

>
>> +
>> +    /* init original jump addresses */
>> +    if (tb->jmp_reset_offset[0] != TB_JMP_RESET_OFFSET_INVALID) {
>> +        tb_reset_jump(tb, 0);
>> +    }
>> +    if (tb->jmp_reset_offset[1] != TB_JMP_RESET_OFFSET_INVALID) {
>> +        tb_reset_jump(tb, 1);
>> +    }
> Why would tb->jmp_reset_offset[0] == TB_JMP_RESET_OFFSET_INVALID not be
> the case as it is set a few lines further up.

Because tcg_gen_code() gets called in between and it is passed
'&tcg_ctx' which holds a pointer to 'tb->jmp_reset_offset'. tcg_out_op()
is called from tcg_gen_code() and sets 'tb->jmp_reset_offset[n]'
indirectly, as well as 'tb->jmp_insn_offset[n]'.

Kind regards,
Sergey

Re: [Qemu-devel] [PATCH 8/8] tcg: Clean up tb_jmp_unlink()

[Alex Bennée]

sergey.fedorov@linaro.org writes:

> From: Sergey Fedorov <serge.fdrv@gmail.com>
>
> Unify the code of this function with tb_jmp_remove_from_list(). Making
> these functions similar improves their readability. Also this could be a
> step towards making this function thread-safe.
>
> Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com>
> Signed-off-by: Sergey Fedorov <sergey.fedorov@linaro.org>
> ---
>  translate-all.c | 20 ++++++++------------
>  1 file changed, 8 insertions(+), 12 deletions(-)
>
> diff --git a/translate-all.c b/translate-all.c
> index 8b4bfa713bf7..56c77a72773d 100644
> --- a/translate-all.c
> +++ b/translate-all.c
> @@ -962,25 +962,21 @@ static inline void tb_reset_jump(TranslationBlock *tb, int n)
>  /* remove any jumps to the TB */
>  static inline void tb_jmp_unlink(TranslationBlock *tb)
>  {
> -    uintptr_t tb1, tb2;
> +    TranslationBlock *tb1;
> +    uintptr_t *ptb;
>      unsigned int n1;
>
> -    tb1 = tb->jmp_list_first;
> +    ptb = &tb->jmp_list_first;
>      for (;;) {
> -        TranslationBlock *tmp_tb;
> -        n1 = tb1 & 3;
> +        n1 = *ptb & 3;
> +        tb1 = (TranslationBlock *)(*ptb & ~3);

I would hope the compiler saw through the duplicate indirect accesses
but maybe:

    uintptr_t *ptb, ntb;

and

    ntb = *ptb;
    n1 = ntb & 3;
    tb1 = (TranslationBlock *)(ntb & ~3);

would be clearer?

>          if (n1 == 2) {
>              break;
>          }
> -        tmp_tb = (TranslationBlock *)(tb1 & ~3);
> -        tb2 = tmp_tb->jmp_list_next[n1];
> -        tb_reset_jump(tmp_tb, n1);
> -        tmp_tb->jmp_list_next[n1] = (uintptr_t)NULL;
> -        tb1 = tb2;
> +        tb_reset_jump(tb1, n1);
> +        *ptb = tb1->jmp_list_next[n1];
> +        tb1->jmp_list_next[n1] = (uintptr_t)NULL;
>      }
> -
> -    assert(((uintptr_t)tb & 3) == 0);
> -    tb->jmp_list_first = (uintptr_t)tb | 2; /* fail safe */
>  }
>
>  /* invalidate one TB */

Otherwise:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

--
Alex Bennée

Re: [Qemu-devel] [PATCH 4/8] tcg: Init TB's direct jumps before making it visible

[Alex Bennée]

Sergey Fedorov <serge.fdrv@gmail.com> writes:

> On 24/03/16 18:11, Alex Bennée wrote:
>> sergey.fedorov@linaro.org writes:
>>> From: Sergey Fedorov <serge.fdrv@gmail.com>
>>>
>>> diff --git a/translate-all.c b/translate-all.c
>>> index ca01dd325b8d..f68716e1819f 100644
>>> --- a/translate-all.c
>>> +++ b/translate-all.c
>>> @@ -1131,19 +1131,6 @@ static void tb_link_page(TranslationBlock *tb, tb_page_addr_t phys_pc,
>>>          tb->page_addr[1] = -1;
>>>      }
>>>
>>> -    assert(((uintptr_t)tb & 3) == 0);
>>> -    tb->jmp_list_first = (uintptr_t)tb | 2;
>>> -    tb->jmp_list_next[0] = (uintptr_t)NULL;
>>> -    tb->jmp_list_next[1] = (uintptr_t)NULL;
>>> -
>>> -    /* init original jump addresses */
>>> -    if (tb->jmp_reset_offset[0] != TB_JMP_RESET_OFFSET_INVALID) {
>>> -        tb_reset_jump(tb, 0);
>>> -    }
>>> -    if (tb->jmp_reset_offset[1] != TB_JMP_RESET_OFFSET_INVALID) {
>>> -        tb_reset_jump(tb, 1);
>>> -    }
>>> -
>>>  #ifdef DEBUG_TB_CHECK
>>>      tb_page_check();
>>>  #endif
>>> @@ -1251,6 +1238,20 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
>>>          ROUND_UP((uintptr_t)gen_code_buf + gen_code_size + search_size,
>>>                   CODE_GEN_ALIGN);
>>>
>>> +    /* init jump list */
>>> +    assert(((uintptr_t)tb & 3) == 0);
>>> +    tb->jmp_list_first = (uintptr_t)tb | 2;
>>> +    tb->jmp_list_next[0] = (uintptr_t)NULL;
>>> +    tb->jmp_list_next[1] = (uintptr_t)NULL;
>> maybe these should be further up the function with the other jmp setting code?
>
> I meant to keep them together with the following lines.
>
>>
>>> +
>>> +    /* init original jump addresses */
>>> +    if (tb->jmp_reset_offset[0] != TB_JMP_RESET_OFFSET_INVALID) {
>>> +        tb_reset_jump(tb, 0);
>>> +    }
>>> +    if (tb->jmp_reset_offset[1] != TB_JMP_RESET_OFFSET_INVALID) {
>>> +        tb_reset_jump(tb, 1);
>>> +    }
>> Why would tb->jmp_reset_offset[0] == TB_JMP_RESET_OFFSET_INVALID not be
>> the case as it is set a few lines further up.
>
> Because tcg_gen_code() gets called in between and it is passed
> '&tcg_ctx' which holds a pointer to 'tb->jmp_reset_offset'. tcg_out_op()
> is called from tcg_gen_code() and sets 'tb->jmp_reset_offset[n]'
> indirectly, as well as 'tb->jmp_insn_offset[n]'.

OK a quick addition to the comment: "these may have been reset in
tcg_gen_code" would be helpful here.

>
> Kind regards,
> Sergey


--
Alex Bennée

Re: [Qemu-devel] [PATCH 8/8] tcg: Clean up tb_jmp_unlink()

[Sergey Fedorov]

On 24/03/16 18:36, Alex Bennée wrote:
>> diff --git a/translate-all.c b/translate-all.c
>> > index 8b4bfa713bf7..56c77a72773d 100644
>> > --- a/translate-all.c
>> > +++ b/translate-all.c
>> > @@ -962,25 +962,21 @@ static inline void tb_reset_jump(TranslationBlock *tb, int n)
>> >  /* remove any jumps to the TB */
>> >  static inline void tb_jmp_unlink(TranslationBlock *tb)
>> >  {
>> > -    uintptr_t tb1, tb2;
>> > +    TranslationBlock *tb1;
>> > +    uintptr_t *ptb;
>> >      unsigned int n1;
>> >
>> > -    tb1 = tb->jmp_list_first;
>> > +    ptb = &tb->jmp_list_first;
>> >      for (;;) {
>> > -        TranslationBlock *tmp_tb;
>> > -        n1 = tb1 & 3;
>> > +        n1 = *ptb & 3;
>> > +        tb1 = (TranslationBlock *)(*ptb & ~3);
> I would hope the compiler saw through the duplicate indirect accesses
> but maybe:
>
>     uintptr_t *ptb, ntb;
>
> and
>
>     ntb = *ptb;
>     n1 = ntb & 3;
>     tb1 = (TranslationBlock *)(ntb & ~3);
>
> would be clearer?
>

Why not? :) Will do this.

Thanks,
Sergey

Re: [Qemu-devel] [PATCH 8/8] tcg: Clean up tb_jmp_unlink()

[Sergey Fedorov]

On 24/03/16 18:42, Sergey Fedorov wrote:
> On 24/03/16 18:36, Alex Bennée wrote:
>>> diff --git a/translate-all.c b/translate-all.c
>>>> index 8b4bfa713bf7..56c77a72773d 100644
>>>> --- a/translate-all.c
>>>> +++ b/translate-all.c
>>>> @@ -962,25 +962,21 @@ static inline void tb_reset_jump(TranslationBlock *tb, int n)
>>>>  /* remove any jumps to the TB */
>>>>  static inline void tb_jmp_unlink(TranslationBlock *tb)
>>>>  {
>>>> -    uintptr_t tb1, tb2;
>>>> +    TranslationBlock *tb1;
>>>> +    uintptr_t *ptb;
>>>>      unsigned int n1;
>>>>
>>>> -    tb1 = tb->jmp_list_first;
>>>> +    ptb = &tb->jmp_list_first;
>>>>      for (;;) {
>>>> -        TranslationBlock *tmp_tb;
>>>> -        n1 = tb1 & 3;
>>>> +        n1 = *ptb & 3;
>>>> +        tb1 = (TranslationBlock *)(*ptb & ~3);
>> I would hope the compiler saw through the duplicate indirect accesses
>> but maybe:
>>
>>     uintptr_t *ptb, ntb;
>>
>> and
>>
>>     ntb = *ptb;
>>     n1 = ntb & 3;
>>     tb1 = (TranslationBlock *)(ntb & ~3);
>>
>> would be clearer?
>>
> Why not? :) Will do this.

I'll do the same in tb_remove_from_jmp_list(). It will go to the patch 8
"tcg: Use uintptr_t type for jmp_list_{next|first} fields of TB".

-Sergey

Re: [Qemu-devel] [PATCH 4/8] tcg: Init TB's direct jumps before making it visible

[Sergey Fedorov]

On 24/03/16 18:40, Alex Bennée wrote:
> Sergey Fedorov <serge.fdrv@gmail.com> writes:
>
>> On 24/03/16 18:11, Alex Bennée wrote:
>>> sergey.fedorov@linaro.org writes:
>>>> From: Sergey Fedorov <serge.fdrv@gmail.com>
>>>>
>>>> diff --git a/translate-all.c b/translate-all.c
>>>> index ca01dd325b8d..f68716e1819f 100644
>>>> --- a/translate-all.c
>>>> +++ b/translate-all.c
>>>> @@ -1131,19 +1131,6 @@ static void tb_link_page(TranslationBlock *tb, tb_page_addr_t phys_pc,
>>>>          tb->page_addr[1] = -1;
>>>>      }
>>>>
>>>> -    assert(((uintptr_t)tb & 3) == 0);
>>>> -    tb->jmp_list_first = (uintptr_t)tb | 2;
>>>> -    tb->jmp_list_next[0] = (uintptr_t)NULL;
>>>> -    tb->jmp_list_next[1] = (uintptr_t)NULL;
>>>> -
>>>> -    /* init original jump addresses */
>>>> -    if (tb->jmp_reset_offset[0] != TB_JMP_RESET_OFFSET_INVALID) {
>>>> -        tb_reset_jump(tb, 0);
>>>> -    }
>>>> -    if (tb->jmp_reset_offset[1] != TB_JMP_RESET_OFFSET_INVALID) {
>>>> -        tb_reset_jump(tb, 1);
>>>> -    }
>>>> -
>>>>  #ifdef DEBUG_TB_CHECK
>>>>      tb_page_check();
>>>>  #endif
>>>> @@ -1251,6 +1238,20 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
>>>>          ROUND_UP((uintptr_t)gen_code_buf + gen_code_size + search_size,
>>>>                   CODE_GEN_ALIGN);
>>>>
>>>> +    /* init jump list */
>>>> +    assert(((uintptr_t)tb & 3) == 0);
>>>> +    tb->jmp_list_first = (uintptr_t)tb | 2;
>>>> +    tb->jmp_list_next[0] = (uintptr_t)NULL;
>>>> +    tb->jmp_list_next[1] = (uintptr_t)NULL;
>>> maybe these should be further up the function with the other jmp setting code?
>> I meant to keep them together with the following lines.
>>
>>>> +
>>>> +    /* init original jump addresses */
>>>> +    if (tb->jmp_reset_offset[0] != TB_JMP_RESET_OFFSET_INVALID) {
>>>> +        tb_reset_jump(tb, 0);
>>>> +    }
>>>> +    if (tb->jmp_reset_offset[1] != TB_JMP_RESET_OFFSET_INVALID) {
>>>> +        tb_reset_jump(tb, 1);
>>>> +    }
>>> Why would tb->jmp_reset_offset[0] == TB_JMP_RESET_OFFSET_INVALID not be
>>> the case as it is set a few lines further up.
>> Because tcg_gen_code() gets called in between and it is passed
>> '&tcg_ctx' which holds a pointer to 'tb->jmp_reset_offset'. tcg_out_op()
>> is called from tcg_gen_code() and sets 'tb->jmp_reset_offset[n]'
>> indirectly, as well as 'tb->jmp_insn_offset[n]'.
> OK a quick addition to the comment: "these may have been reset in
> tcg_gen_code" would be helpful here.

The other way around: 'tb->jmp_reset_offset[n]' are reset before calling
tcg_gen_code() and are set during tcg_gen_code() execution.

Kind regards,
Sergey

Re: [Qemu-devel] [PATCH 1/8] tcg: Clean up direct block chaining data fields

[Richard Henderson]

On 03/24/2016 08:11 AM, Paolo Bonzini wrote:
> There is also a case where a TB jumps to itself; it then appears twice
> in the list with different values in the low bits, such as this:
>
>      tb->jmp_list_first = tb | 0;
>       .--------------------'   |
>       |                .-------'
>      tb->jmp_list_next[0] = tb | 2;

Of course, it begs the question of why TB would be in its own list, even if it 
does jump to itself.  We only need the points-to list in order to invalidate a 
TB and unlink it.  But if TB is being invalidated, we don't need to reset the 
jump within TB itself.


r~

Re: [Qemu-devel] [PATCH 1/8] tcg: Clean up direct block chaining data fields

[Paolo Bonzini]

On 29/03/2016 00:12, Richard Henderson wrote:
>> There is also a case where a TB jumps to itself; it then appears twice
>> in the list with different values in the low bits, such as this:
>>
>>      tb->jmp_list_first = tb | 0;
>>       .--------------------'   |
>>       |                .-------'
>>      tb->jmp_list_next[0] = tb | 2;
> 
> Of course, it begs the question of why TB would be in its own list, even
> if it does jump to itself.  We only need the points-to list in order to
> invalidate a TB and unlink it.  But if TB is being invalidated, we don't
> need to reset the jump within TB itself.

Isn't it just because TB acts as the list head in the circular list?  Or
am I misunderstanding you?

Paolo

Re: [Qemu-devel] [PATCH 1/8] tcg: Clean up direct block chaining data fields

[Sergey Fedorov]

On 29/03/16 01:12, Richard Henderson wrote:
> On 03/24/2016 08:11 AM, Paolo Bonzini wrote:
>> There is also a case where a TB jumps to itself; it then appears twice
>> in the list with different values in the low bits, such as this:
>>
>>      tb->jmp_list_first = tb | 0;
>>       .--------------------'   |
>>       |                .-------'
>>      tb->jmp_list_next[0] = tb | 2;
>
> Of course, it begs the question of why TB would be in its own list,
> even if it does jump to itself.  We only need the points-to list in
> order to invalidate a TB and unlink it.  But if TB is being
> invalidated, we don't need to reset the jump within TB itself.

If we're going to move tb_phys_invalidate() outside of tb_lock, we
probably need to reset all jumps to the TB, even if it jumps to itself,
so that it eventually finish its execution.

Kind regards,
Sergey

Re: [Qemu-devel] [PATCH 1/8] tcg: Clean up direct block chaining data fields

[Paolo Bonzini]

On 29/03/2016 10:14, Paolo Bonzini wrote:
> 
> 
> On 29/03/2016 00:12, Richard Henderson wrote:
>>> There is also a case where a TB jumps to itself; it then appears twice
>>> in the list with different values in the low bits, such as this:
>>>
>>>      tb->jmp_list_first = tb | 0;
>>>       .--------------------'   |
>>>       |                .-------'
>>>      tb->jmp_list_next[0] = tb | 2;
>>
>> Of course, it begs the question of why TB would be in its own list, even
>> if it does jump to itself.  We only need the points-to list in order to
>> invalidate a TB and unlink it.  But if TB is being invalidated, we don't
>> need to reset the jump within TB itself.
> 
> Isn't it just because TB acts as the list head in the circular list?  Or
> am I misunderstanding you?

Ok, that was dumb. :) Understood it now.

Paolo

Re: [Qemu-devel] [PATCH 1/8] tcg: Clean up direct block chaining data fields

[Richard Henderson]

On 03/29/2016 01:31 AM, Sergey Fedorov wrote:
> On 29/03/16 01:12, Richard Henderson wrote:
>> On 03/24/2016 08:11 AM, Paolo Bonzini wrote:
>>> There is also a case where a TB jumps to itself; it then appears twice
>>> in the list with different values in the low bits, such as this:
>>>
>>>      tb->jmp_list_first = tb | 0;
>>>       .--------------------'   |
>>>       |                .-------'
>>>      tb->jmp_list_next[0] = tb | 2;
>>
>> Of course, it begs the question of why TB would be in its own list,
>> even if it does jump to itself.  We only need the points-to list in
>> order to invalidate a TB and unlink it.  But if TB is being
>> invalidated, we don't need to reset the jump within TB itself.
> 
> If we're going to move tb_phys_invalidate() outside of tb_lock, we
> probably need to reset all jumps to the TB, even if it jumps to itself,
> so that it eventually finish its execution.

Ah, interesting point.  Very true.


r~

Re: [Qemu-devel] [Qemu-arm] [PATCH 1/8] tcg: Clean up direct block chaining data fields

[Peter Maydell]

On 29 March 2016 at 09:31, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
> On 29/03/16 01:12, Richard Henderson wrote:
>> On 03/24/2016 08:11 AM, Paolo Bonzini wrote:
>>> There is also a case where a TB jumps to itself; it then appears twice
>>> in the list with different values in the low bits, such as this:
>>>
>>>      tb->jmp_list_first = tb | 0;
>>>       .--------------------'   |
>>>       |                .-------'
>>>      tb->jmp_list_next[0] = tb | 2;
>>
>> Of course, it begs the question of why TB would be in its own list,
>> even if it does jump to itself.  We only need the points-to list in
>> order to invalidate a TB and unlink it.  But if TB is being
>> invalidated, we don't need to reset the jump within TB itself.
>
> If we're going to move tb_phys_invalidate() outside of tb_lock, we
> probably need to reset all jumps to the TB, even if it jumps to itself,
> so that it eventually finish its execution.

This is likely also the historical reason for the current code --
originally we handled requesting a CPU exit by unlinking the TB,
so you needed to be able to detach jumps-to-self (these days we do
it by checking a flag at the start of each TB).

thanks
-- PMM

Re: [Qemu-devel] [Qemu-arm] [PATCH 1/8] tcg: Clean up direct block chaining data fields

[Sergey Fedorov]

On 29/03/16 19:26, Peter Maydell wrote:
> On 29 March 2016 at 09:31, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
>> On 29/03/16 01:12, Richard Henderson wrote:
>>> On 03/24/2016 08:11 AM, Paolo Bonzini wrote:
>>>> There is also a case where a TB jumps to itself; it then appears twice
>>>> in the list with different values in the low bits, such as this:
>>>>
>>>>      tb->jmp_list_first = tb | 0;
>>>>       .--------------------'   |
>>>>       |                .-------'
>>>>      tb->jmp_list_next[0] = tb | 2;
>>> Of course, it begs the question of why TB would be in its own list,
>>> even if it does jump to itself.  We only need the points-to list in
>>> order to invalidate a TB and unlink it.  But if TB is being
>>> invalidated, we don't need to reset the jump within TB itself.
>> If we're going to move tb_phys_invalidate() outside of tb_lock, we
>> probably need to reset all jumps to the TB, even if it jumps to itself,
>> so that it eventually finish its execution.
> This is likely also the historical reason for the current code --
> originally we handled requesting a CPU exit by unlinking the TB,
> so you needed to be able to detach jumps-to-self (these days we do
> it by checking a flag at the start of each TB).

I'm not sure if CPU exit request is raised each time TB gets invalidated...

Kind regards,
Sergey