[Qemu-devel] [PATCH v2 00/11] tcg: Make direct jump patching thread-safe

[Qemu-devel] [PATCH v2 00/11] tcg: Make direct jump patching thread-safe

[Sergey Fedorov]

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

When patching translated code for direct block chaining/unchaining,
modification of concurrently executing code can happen in multi-threaded
execution.  Currently only user-mode is affected. To make direct block patching
safe, some care must be taken to make sure that the code modification is made
atomically and concurrently executed code is guaranteed to be consistent.

This patch series fixes all supported TCG targets which use direct patching and
documents the requirement for direct jump patching be atomic and thread-safe.

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

[1] https://github.com/sergefdrv/qemu/tree/atomic-tb-patching-2

Summary of changes in v2:
 * Take out mistakingly pulled patches [PATCH 01/11] and [PATCH 02/11]
 * Two new patches to add some handy macros for alignment
   [PATCH v2 01/11] and [PATCH v2 02/11]
 * Use new alignment macros instead of open-coding
 * Use i386 tcg_out_nopn() implementation suggested by Richard Henderson;
   rework alignment checking and gap calculation in [PATCH v2 05/11]
 * Clean up reloc_pc24_atomic() in [PATCH v2 07/11]
 * Use tcg_debug_assert() instead of assert()
 * Use deposit32() in [PATCH v2 09/11]
 * s/atomic_write/atomic_set/ in [PATCH v2 10/11]
 * Minor rewording in [PATCH v2 11/11]

Sergey Fedorov (11):
  include/qemu/osdep.h: Add a macro to check for alignment
  include/qemu/osdep.h: Add macros for pointer alignment
  tci: Make direct jump patching thread-safe
  tcg/ppc: Make direct jump patching thread-safe
  tcg/i386: Make direct jump patching thread-safe
  tcg/s390: Make direct jump patching thread-safe
  tcg/arm: Make direct jump patching thread-safe
  tcg/aarch64: Make direct jump patching thread-safe
  tcg/sparc: Make direct jump patching thread-safe
  tcg/mips: Make direct jump patching thread-safe
  tcg: Note requirement on atomic direct jump patching

 include/exec/exec-all.h      | 32 ++++++--------------------------
 include/qemu/osdep.h         | 14 ++++++++++++++
 tcg/aarch64/tcg-target.inc.c | 14 +++++++++++++-
 tcg/arm/tcg-target.inc.c     | 18 ++++++++++++++++++
 tcg/i386/tcg-target.inc.c    | 23 +++++++++++++++++++++++
 tcg/mips/tcg-target.inc.c    |  3 ++-
 tcg/ppc/tcg-target.inc.c     | 22 ++++++++++++++++++----
 tcg/s390/tcg-target.inc.c    |  8 ++++++++
 tcg/sparc/tcg-target.inc.c   |  2 +-
 tcg/tci/tcg-target.inc.c     |  2 ++
 tci.c                        |  5 ++++-
 translate-all.c              |  2 ++
 12 files changed, 111 insertions(+), 34 deletions(-)

-- 
2.8.1

[Qemu-devel] [PATCH v2 01/11] include/qemu/osdep.h: Add a macro to check for alignment

[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>
---
 include/qemu/osdep.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 408783f532e6..e3bc50b61359 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -158,6 +158,9 @@ extern int daemon(int, int);
 /* Round number up to multiple */
 #define QEMU_ALIGN_UP(n, m) QEMU_ALIGN_DOWN((n) + (m) - 1, (m))
 
+/* Check if n is a multiple of m */
+#define QEMU_IS_ALIGNED(n, m) (((n) % (m)) == 0)
+
 #ifndef ROUND_UP
 #define ROUND_UP(n,d) (((n) + (d) - 1) & -(d))
 #endif
-- 
2.8.1

[Qemu-devel] [PATCH v2 02/11] include/qemu/osdep.h: Add macros for pointer alignment

[Sergey Fedorov]

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

These macros provide a convenient way to n-byte align pointers up and
down and check if a pointer is n-byte aligned.

Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com>
Signed-off-by: Sergey Fedorov <sergey.fedorov@linaro.org>
---
 include/qemu/osdep.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index e3bc50b61359..1e3221cbec65 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -161,6 +161,17 @@ extern int daemon(int, int);
 /* Check if n is a multiple of m */
 #define QEMU_IS_ALIGNED(n, m) (((n) % (m)) == 0)
 
+/* n-byte align pointer down */
+#define QEMU_ALIGN_PTR_DOWN(p, n) \
+    ((typeof(p))QEMU_ALIGN_DOWN((uintptr_t)(p), (n)))
+
+/* n-byte align pointer up */
+#define QEMU_ALIGN_PTR_UP(p, n) \
+    ((typeof(p))QEMU_ALIGN_UP((uintptr_t)(p), (n)))
+
+/* Check if pointer p is n-bytes aligned */
+#define QEMU_PTR_IS_ALIGNED(p, n) QEMU_IS_ALIGNED((uintptr_t)(p), (n))
+
 #ifndef ROUND_UP
 #define ROUND_UP(n,d) (((n) + (d) - 1) & -(d))
 #endif
-- 
2.8.1

[Qemu-devel] [PATCH v2 03/11] tci: Make direct jump patching thread-safe

[Sergey Fedorov]

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

Ensure direct jump patching in TCI is atomic by:
 * naturally aligning a location of direct jump address;
 * using atomic_read()/atomic_set() to load/store the address.

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

Changes in v2:
 * Use QEMU_ALIGN_PTR_UP()

 include/exec/exec-all.h  | 2 +-
 tcg/tci/tcg-target.inc.c | 2 ++
 tci.c                    | 5 ++++-
 3 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index 736209505a68..59709c9dd5c9 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -302,7 +302,7 @@ void tb_phys_invalidate(TranslationBlock *tb, tb_page_addr_t page_addr);
 static inline void tb_set_jmp_target1(uintptr_t jmp_addr, uintptr_t addr)
 {
     /* patch the branch destination */
-    *(uint32_t *)jmp_addr = addr - (jmp_addr + 4);
+    atomic_set((int32_t *)jmp_addr, addr - (jmp_addr + 4));
     /* no need to flush icache explicitly */
 }
 #elif defined(_ARCH_PPC)
diff --git a/tcg/tci/tcg-target.inc.c b/tcg/tci/tcg-target.inc.c
index 4afe4d7a8d59..08aa8c1debf5 100644
--- a/tcg/tci/tcg-target.inc.c
+++ b/tcg/tci/tcg-target.inc.c
@@ -556,6 +556,8 @@ static void tcg_out_op(TCGContext *s, TCGOpcode opc, const TCGArg *args,
         if (s->tb_jmp_offset) {
             /* Direct jump method. */
             assert(args[0] < ARRAY_SIZE(s->tb_jmp_offset));
+            /* Align for atomic patching and thread safety */
+            s->code_ptr = QEMU_ALIGN_PTR_UP(s->code_ptr, 4);
             s->tb_jmp_offset[args[0]] = tcg_current_code_size(s);
             tcg_out32(s, 0);
         } else {
diff --git a/tci.c b/tci.c
index 82705fe77295..a8939e6d3c2b 100644
--- a/tci.c
+++ b/tci.c
@@ -1089,7 +1089,10 @@ uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr)
             goto exit;
             break;
         case INDEX_op_goto_tb:
-            t0 = tci_read_i32(&tb_ptr);
+            /* Jump address is aligned */
+            tb_ptr = QEMU_ALIGN_PTR_UP(tb_ptr, 4);
+            t0 = atomic_read((int32_t *)tb_ptr);
+            tb_ptr += sizeof(int32_t);
             tci_assert(tb_ptr == old_code_ptr + op_size);
             tb_ptr += (int32_t)t0;
             continue;
-- 
2.8.1

[Qemu-devel] [PATCH v2 04/11] tcg/ppc: Make direct jump patching thread-safe

[Sergey Fedorov]

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

Ensure direct jump patching in PPC is atomic by:
 * limiting translation buffer size in 32-bit mode to be addressable by
   Branch I-form instruction;
 * using atomic_read()/atomic_set() for code patching.

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>
---
 tcg/ppc/tcg-target.inc.c | 22 ++++++++++++++++++----
 translate-all.c          |  2 ++
 2 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/tcg/ppc/tcg-target.inc.c b/tcg/ppc/tcg-target.inc.c
index 8c1c2dfa9b22..a7c65fd08854 100644
--- a/tcg/ppc/tcg-target.inc.c
+++ b/tcg/ppc/tcg-target.inc.c
@@ -1237,6 +1237,7 @@ static void tcg_out_brcond2 (TCGContext *s, const TCGArg *args,
     tcg_out_bc(s, BC | BI(7, CR_EQ) | BO_COND_TRUE, arg_label(args[5]));
 }
 
+#ifdef __powerpc64__
 void ppc_tb_set_jmp_target(uintptr_t jmp_addr, uintptr_t addr)
 {
     tcg_insn_unit i1, i2;
@@ -1265,11 +1266,18 @@ void ppc_tb_set_jmp_target(uintptr_t jmp_addr, uintptr_t addr)
     pair = (uint64_t)i2 << 32 | i1;
 #endif
 
-    /* ??? __atomic_store_8, presuming there's some way to do that
-       for 32-bit, otherwise this is good enough for 64-bit.  */
-    *(uint64_t *)jmp_addr = pair;
+    atomic_set((uint64_t *)jmp_addr, pair);
     flush_icache_range(jmp_addr, jmp_addr + 8);
 }
+#else
+void ppc_tb_set_jmp_target(uintptr_t jmp_addr, uintptr_t addr)
+{
+    intptr_t diff = addr - jmp_addr;
+    tcg_debug_assert(in_range_b(diff));
+    atomic_set((uint32_t *)jmp_addr, B | (diff & 0x3fffffc));
+    flush_icache_range(jmp_addr, jmp_addr + 4);
+}
+#endif
 
 static void tcg_out_call(TCGContext *s, tcg_insn_unit *target)
 {
@@ -1895,7 +1903,9 @@ static void tcg_out_op(TCGContext *s, TCGOpcode opc, const TCGArg *args,
         break;
     case INDEX_op_goto_tb:
         tcg_debug_assert(s->tb_jmp_offset);
-        /* Direct jump.  Ensure the next insns are 8-byte aligned. */
+        /* Direct jump. */
+#ifdef __powerpc64__
+        /* Ensure the next insns are 8-byte aligned. */
         if ((uintptr_t)s->code_ptr & 7) {
             tcg_out32(s, NOP);
         }
@@ -1904,6 +1914,10 @@ static void tcg_out_op(TCGContext *s, TCGOpcode opc, const TCGArg *args,
         s->code_ptr += 2;
         tcg_out32(s, MTSPR | RS(TCG_REG_TMP1) | CTR);
         tcg_out32(s, BCCTR | BO_ALWAYS);
+#else
+        /* To be replaced by a branch.  */
+        s->code_ptr++;
+#endif
         s->tb_next_offset[args[0]] = tcg_current_code_size(s);
         break;
     case INDEX_op_br:
diff --git a/translate-all.c b/translate-all.c
index 8329ea60eeda..fcc573515fb4 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -464,6 +464,8 @@ static inline PageDesc *page_find(tb_page_addr_t index)
 # define MAX_CODE_GEN_BUFFER_SIZE  (2ul * 1024 * 1024 * 1024)
 #elif defined(__powerpc64__)
 # define MAX_CODE_GEN_BUFFER_SIZE  (2ul * 1024 * 1024 * 1024)
+#elif defined(__powerpc__)
+# define MAX_CODE_GEN_BUFFER_SIZE  (32u * 1024 * 1024)
 #elif defined(__aarch64__)
 # define MAX_CODE_GEN_BUFFER_SIZE  (128ul * 1024 * 1024)
 #elif defined(__arm__)
-- 
2.8.1

[Qemu-devel] [PATCH v2 05/11] tcg/i386: Make direct jump patching thread-safe

[Sergey Fedorov]

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

Ensure direct jump patching in i386 is atomic by:
 * naturally aligning a location of direct jump address;
 * using atomic_read()/atomic_set() for code patching.

tcg_out_nopn() implementation:
Suggested-by: Richard Henderson <rth@twiddle.net>.

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

Changes in v2:
 * Use tcg_out_nopn() implementation suggested by Richard Henderson
 * Rework alignment checking and gap calculation

 include/exec/exec-all.h   |  2 +-
 tcg/i386/tcg-target.inc.c | 23 +++++++++++++++++++++++
 2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index 59709c9dd5c9..82399175fe80 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -312,7 +312,7 @@ void ppc_tb_set_jmp_target(uintptr_t jmp_addr, uintptr_t addr);
 static inline void tb_set_jmp_target1(uintptr_t jmp_addr, uintptr_t addr)
 {
     /* patch the branch destination */
-    stl_le_p((void*)jmp_addr, addr - (jmp_addr + 4));
+    atomic_set((int32_t *)jmp_addr, addr - (jmp_addr + 4));
     /* no need to flush icache explicitly */
 }
 #elif defined(__s390x__)
diff --git a/tcg/i386/tcg-target.inc.c b/tcg/i386/tcg-target.inc.c
index 9187d34caf6d..3ffc81a1d168 100644
--- a/tcg/i386/tcg-target.inc.c
+++ b/tcg/i386/tcg-target.inc.c
@@ -1123,6 +1123,21 @@ static void tcg_out_jmp(TCGContext *s, tcg_insn_unit *dest)
     tcg_out_branch(s, 0, dest);
 }
 
+static void tcg_out_nopn(TCGContext *s, int n)
+{
+    int i;
+    /* Emit 1 or 2 operand size prefixes for the standard one byte nop,
+     * "xchg %eax,%eax", forming "xchg %ax,%ax". All cores accept the
+     * duplicate prefix, and all of the interesting recent cores can
+     * decode and discard the duplicates in a single cycle.
+     */
+    tcg_debug_assert(n >= 1);
+    for (i = 1; i < n; ++i) {
+        tcg_out8(s, 0x66);
+    }
+    tcg_out8(s, 0x90);
+}
+
 #if defined(CONFIG_SOFTMMU)
 /* helper signature: helper_ret_ld_mmu(CPUState *env, target_ulong addr,
  *                                     int mmu_idx, uintptr_t ra)
@@ -1777,6 +1792,14 @@ static inline void tcg_out_op(TCGContext *s, TCGOpcode opc,
     case INDEX_op_goto_tb:
         if (s->tb_jmp_offset) {
             /* direct jump method */
+            int gap;
+            /* jump displacement must be aligned for atomic patching;
+             * see if we need to add extra nops before jump
+             */
+            gap = tcg_pcrel_diff(s, QEMU_ALIGN_PTR_UP(s->code_ptr + 1, 4));
+            if (gap != 1) {
+                tcg_out_nopn(s, gap - 1);
+            }
             tcg_out8(s, OPC_JMP_long); /* jmp im */
             s->tb_jmp_offset[args[0]] = tcg_current_code_size(s);
             tcg_out32(s, 0);
-- 
2.8.1

[Qemu-devel] [PATCH v2 06/11] tcg/s390: Make direct jump patching thread-safe

[Sergey Fedorov]

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

Ensure direct jump patching in s390 is atomic by:
 * naturally aligning a location of direct jump address;
 * using atomic_read()/atomic_set() for code patching.

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

Changes in v2:
 * Use QEMU_PTR_IS_ALIGNED()

 include/exec/exec-all.h   | 2 +-
 tcg/s390/tcg-target.inc.c | 8 ++++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index 82399175fe80..e18cc24e50f0 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -320,7 +320,7 @@ static inline void tb_set_jmp_target1(uintptr_t jmp_addr, uintptr_t addr)
 {
     /* patch the branch destination */
     intptr_t disp = addr - (jmp_addr - 2);
-    stl_be_p((void*)jmp_addr, disp / 2);
+    atomic_set((int32_t *)jmp_addr, disp / 2);
     /* no need to flush icache explicitly */
 }
 #elif defined(__aarch64__)
diff --git a/tcg/s390/tcg-target.inc.c b/tcg/s390/tcg-target.inc.c
index fbf97bb2e15d..339df41cd300 100644
--- a/tcg/s390/tcg-target.inc.c
+++ b/tcg/s390/tcg-target.inc.c
@@ -219,6 +219,8 @@ typedef enum S390Opcode {
     RX_ST       = 0x50,
     RX_STC      = 0x42,
     RX_STH      = 0x40,
+
+    NOP         = 0x0707,
 } S390Opcode;
 
 #ifndef NDEBUG
@@ -1716,6 +1718,12 @@ static inline void tcg_out_op(TCGContext *s, TCGOpcode opc,
 
     case INDEX_op_goto_tb:
         if (s->tb_jmp_offset) {
+            /* branch displacement must be aligned for atomic patching;
+             * see if we need to add extra nop before branch
+             */
+            if (!QEMU_PTR_IS_ALIGNED(s->code_ptr + 1, 4)) {
+                tcg_out16(s, NOP);
+            }
             tcg_out16(s, RIL_BRCL | (S390_CC_ALWAYS << 4));
             s->tb_jmp_offset[args[0]] = tcg_current_code_size(s);
             s->code_ptr += 2;
-- 
2.8.1

[Qemu-devel] [PATCH v2 07/11] tcg/arm: Make direct jump patching thread-safe

[Sergey Fedorov]

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

Ensure direct jump patching in ARM is atomic by using
atomic_read()/atomic_set() for code patching.

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

Changes in v 2:
 * Add tcg_debug_assert() to check offset
 * Use deposit32() for insturction patching

 include/exec/exec-all.h  | 25 ++-----------------------
 tcg/arm/tcg-target.inc.c | 18 ++++++++++++++++++
 2 files changed, 20 insertions(+), 23 deletions(-)

diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index e18cc24e50f0..6a054ee720a8 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -327,29 +327,8 @@ static inline void tb_set_jmp_target1(uintptr_t jmp_addr, uintptr_t addr)
 void aarch64_tb_set_jmp_target(uintptr_t jmp_addr, uintptr_t addr);
 #define tb_set_jmp_target1 aarch64_tb_set_jmp_target
 #elif defined(__arm__)
-static inline void tb_set_jmp_target1(uintptr_t jmp_addr, uintptr_t addr)
-{
-#if !QEMU_GNUC_PREREQ(4, 1)
-    register unsigned long _beg __asm ("a1");
-    register unsigned long _end __asm ("a2");
-    register unsigned long _flg __asm ("a3");
-#endif
-
-    /* we could use a ldr pc, [pc, #-4] kind of branch and avoid the flush */
-    *(uint32_t *)jmp_addr =
-        (*(uint32_t *)jmp_addr & ~0xffffff)
-        | (((addr - (jmp_addr + 8)) >> 2) & 0xffffff);
-
-#if QEMU_GNUC_PREREQ(4, 1)
-    __builtin___clear_cache((char *) jmp_addr, (char *) jmp_addr + 4);
-#else
-    /* flush icache */
-    _beg = jmp_addr;
-    _end = jmp_addr + 4;
-    _flg = 0;
-    __asm __volatile__ ("swi 0x9f0002" : : "r" (_beg), "r" (_end), "r" (_flg));
-#endif
-}
+void arm_tb_set_jmp_target(uintptr_t jmp_addr, uintptr_t addr);
+#define tb_set_jmp_target1 arm_tb_set_jmp_target
 #elif defined(__sparc__) || defined(__mips__)
 void tb_set_jmp_target1(uintptr_t jmp_addr, uintptr_t addr);
 #else
diff --git a/tcg/arm/tcg-target.inc.c b/tcg/arm/tcg-target.inc.c
index 3edf6a6f971c..2750610a54f1 100644
--- a/tcg/arm/tcg-target.inc.c
+++ b/tcg/arm/tcg-target.inc.c
@@ -121,6 +121,14 @@ static inline void reloc_pc24(tcg_insn_unit *code_ptr, tcg_insn_unit *target)
     *code_ptr = (*code_ptr & ~0xffffff) | (offset & 0xffffff);
 }
 
+static inline void reloc_pc24_atomic(tcg_insn_unit *code_ptr, tcg_insn_unit *target)
+{
+    ptrdiff_t offset = (tcg_ptr_byte_diff(target, code_ptr) - 8) >> 2;
+    tcg_insn_unit insn = atomic_read(code_ptr);
+    tcg_debug_assert(offset == sextract32(offset, 0, 24));
+    atomic_set(code_ptr, deposit32(insn, 0, 24, offset));
+}
+
 static void patch_reloc(tcg_insn_unit *code_ptr, int type,
                         intptr_t value, intptr_t addend)
 {
@@ -1038,6 +1046,16 @@ static void tcg_out_call(TCGContext *s, tcg_insn_unit *addr)
     }
 }
 
+void arm_tb_set_jmp_target(uintptr_t jmp_addr, uintptr_t addr)
+{
+    tcg_insn_unit *code_ptr = (tcg_insn_unit *)jmp_addr;
+    tcg_insn_unit *target = (tcg_insn_unit *)addr;
+
+    /* we could use a ldr pc, [pc, #-4] kind of branch and avoid the flush */
+    reloc_pc24_atomic(code_ptr, target);
+    flush_icache_range(jmp_addr, jmp_addr + 4);
+}
+
 static inline void tcg_out_goto_label(TCGContext *s, int cond, TCGLabel *l)
 {
     if (l->has_value) {
-- 
2.8.1

[Qemu-devel] [PATCH v2 08/11] tcg/aarch64: Make direct jump patching thread-safe

[Sergey Fedorov]

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

Ensure direct jump patching in AArch64 is atomic by using
atomic_read()/atomic_set() for code patching.

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

Changes in v2:
 * Use tcg_debug_assert() instead of assert()

 tcg/aarch64/tcg-target.inc.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/tcg/aarch64/tcg-target.inc.c b/tcg/aarch64/tcg-target.inc.c
index 0ed10a974121..29839765b828 100644
--- a/tcg/aarch64/tcg-target.inc.c
+++ b/tcg/aarch64/tcg-target.inc.c
@@ -73,6 +73,18 @@ static inline void reloc_pc26(tcg_insn_unit *code_ptr, tcg_insn_unit *target)
     *code_ptr = deposit32(*code_ptr, 0, 26, offset);
 }
 
+static inline void reloc_pc26_atomic(tcg_insn_unit *code_ptr,
+                                     tcg_insn_unit *target)
+{
+    ptrdiff_t offset = target - code_ptr;
+    tcg_insn_unit insn;
+    tcg_debug_assert(offset == sextract64(offset, 0, 26));
+    /* read instruction, mask away previous PC_REL26 parameter contents,
+       set the proper offset, then write back the instruction. */
+    insn = atomic_read(code_ptr);
+    atomic_set(code_ptr, deposit32(insn, 0, 26, offset));
+}
+
 static inline void reloc_pc19(tcg_insn_unit *code_ptr, tcg_insn_unit *target)
 {
     ptrdiff_t offset = target - code_ptr;
@@ -835,7 +847,7 @@ void aarch64_tb_set_jmp_target(uintptr_t jmp_addr, uintptr_t addr)
     tcg_insn_unit *code_ptr = (tcg_insn_unit *)jmp_addr;
     tcg_insn_unit *target = (tcg_insn_unit *)addr;
 
-    reloc_pc26(code_ptr, target);
+    reloc_pc26_atomic(code_ptr, target);
     flush_icache_range(jmp_addr, jmp_addr + 4);
 }
 
-- 
2.8.1

[Qemu-devel] [PATCH v2 09/11] tcg/sparc: Make direct jump patching thread-safe

[Sergey Fedorov]

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

Ensure direct jump patching in SPARC is atomic by using
atomic_read()/atomic_set() for code patching.

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>
---

Changes in v2:
 * Use deposit32() to put displacement into call instruction

 tcg/sparc/tcg-target.inc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tcg/sparc/tcg-target.inc.c b/tcg/sparc/tcg-target.inc.c
index 54df1bc42432..3b54fbd22760 100644
--- a/tcg/sparc/tcg-target.inc.c
+++ b/tcg/sparc/tcg-target.inc.c
@@ -1647,6 +1647,6 @@ void tb_set_jmp_target1(uintptr_t jmp_addr, uintptr_t addr)
        the code_gen_buffer can't be larger than 2GB.  */
     assert(disp == (int32_t)disp);
 
-    *ptr = CALL | (uint32_t)disp >> 2;
+    atomic_set(ptr, deposit32(CALL, 0, 30, disp >> 2));
     flush_icache_range(jmp_addr, jmp_addr + 4);
 }
-- 
2.8.1

[Qemu-devel] [PATCH v2 10/11] tcg/mips: Make direct jump patching thread-safe

[Sergey Fedorov]

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

Ensure direct jump patching in MIPS is atomic by using
atomic_read()/atomic_set() for code patching.

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

Changes in v2:
 * s/atomic_write/atomic_set/

 tcg/mips/tcg-target.inc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tcg/mips/tcg-target.inc.c b/tcg/mips/tcg-target.inc.c
index 682e19897db0..cefc0398018a 100644
--- a/tcg/mips/tcg-target.inc.c
+++ b/tcg/mips/tcg-target.inc.c
@@ -1886,6 +1886,7 @@ static void tcg_target_init(TCGContext *s)
 void tb_set_jmp_target1(uintptr_t jmp_addr, uintptr_t addr)
 {
     uint32_t *ptr = (uint32_t *)jmp_addr;
-    *ptr = deposit32(*ptr, 0, 26, addr >> 2);
+    uint32_t insn = atomic_read(ptr);
+    atomic_set(ptr, deposit32(insn, 0, 26, addr >> 2));
     flush_icache_range(jmp_addr, jmp_addr + 4);
 }
-- 
2.8.1

Re: [Qemu-devel] [PATCH v2 10/11] tcg/mips: Make direct jump patching thread-safe

[Aurelien Jarno]

On 2016-04-22 19:08, Sergey Fedorov wrote:
> From: Sergey Fedorov <serge.fdrv@gmail.com>
> 
> Ensure direct jump patching in MIPS is atomic by using
> atomic_read()/atomic_set() for code patching.
> 
> Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com>
> Signed-off-by: Sergey Fedorov <sergey.fedorov@linaro.org>
> ---
> 
> Changes in v2:
>  * s/atomic_write/atomic_set/
> 
>  tcg/mips/tcg-target.inc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tcg/mips/tcg-target.inc.c b/tcg/mips/tcg-target.inc.c
> index 682e19897db0..cefc0398018a 100644
> --- a/tcg/mips/tcg-target.inc.c
> +++ b/tcg/mips/tcg-target.inc.c
> @@ -1886,6 +1886,7 @@ static void tcg_target_init(TCGContext *s)
>  void tb_set_jmp_target1(uintptr_t jmp_addr, uintptr_t addr)
>  {
>      uint32_t *ptr = (uint32_t *)jmp_addr;
> -    *ptr = deposit32(*ptr, 0, 26, addr >> 2);
> +    uint32_t insn = atomic_read(ptr);
> +    atomic_set(ptr, deposit32(insn, 0, 26, addr >> 2));
>      flush_icache_range(jmp_addr, jmp_addr + 4);

Does it really make sense to read and write the value atomically? The
resulting operation is still not atomic, something can happen in
between.

Aurelien

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH v2 10/11] tcg/mips: Make direct jump patching thread-safe

[Aurelien Jarno]

On 2016-04-22 18:47, Aurelien Jarno wrote:
> On 2016-04-22 19:08, Sergey Fedorov wrote:
> > From: Sergey Fedorov <serge.fdrv@gmail.com>
> > 
> > Ensure direct jump patching in MIPS is atomic by using
> > atomic_read()/atomic_set() for code patching.
> > 
> > Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com>
> > Signed-off-by: Sergey Fedorov <sergey.fedorov@linaro.org>
> > ---
> > 
> > Changes in v2:
> >  * s/atomic_write/atomic_set/
> > 
> >  tcg/mips/tcg-target.inc.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tcg/mips/tcg-target.inc.c b/tcg/mips/tcg-target.inc.c
> > index 682e19897db0..cefc0398018a 100644
> > --- a/tcg/mips/tcg-target.inc.c
> > +++ b/tcg/mips/tcg-target.inc.c
> > @@ -1886,6 +1886,7 @@ static void tcg_target_init(TCGContext *s)
> >  void tb_set_jmp_target1(uintptr_t jmp_addr, uintptr_t addr)
> >  {
> >      uint32_t *ptr = (uint32_t *)jmp_addr;
> > -    *ptr = deposit32(*ptr, 0, 26, addr >> 2);
> > +    uint32_t insn = atomic_read(ptr);
> > +    atomic_set(ptr, deposit32(insn, 0, 26, addr >> 2));
> >      flush_icache_range(jmp_addr, jmp_addr + 4);
> 
> Does it really make sense to read and write the value atomically? The
> resulting operation is still not atomic, something can happen in
> between.

Hmm, thinking more about that, given the only instruction used is "J",
we don't have to read the value, patch it and write it. We can directly
use something like (untested):

    atomic_set(ptr, (0x02 << 26) | (addr >> 2));

Aurelien

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH v2 10/11] tcg/mips: Make direct jump patching thread-safe

[Sergey Fedorov]

On 22/04/16 19:51, Aurelien Jarno wrote:
> On 2016-04-22 18:47, Aurelien Jarno wrote:
>> On 2016-04-22 19:08, Sergey Fedorov wrote:
>>> From: Sergey Fedorov <serge.fdrv@gmail.com>
>>>
>>> Ensure direct jump patching in MIPS is atomic by using
>>> atomic_read()/atomic_set() for code patching.
>>>
>>> Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com>
>>> Signed-off-by: Sergey Fedorov <sergey.fedorov@linaro.org>
>>> ---
>>>
>>> Changes in v2:
>>>  * s/atomic_write/atomic_set/
>>>
>>>  tcg/mips/tcg-target.inc.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tcg/mips/tcg-target.inc.c b/tcg/mips/tcg-target.inc.c
>>> index 682e19897db0..cefc0398018a 100644
>>> --- a/tcg/mips/tcg-target.inc.c
>>> +++ b/tcg/mips/tcg-target.inc.c
>>> @@ -1886,6 +1886,7 @@ static void tcg_target_init(TCGContext *s)
>>>  void tb_set_jmp_target1(uintptr_t jmp_addr, uintptr_t addr)
>>>  {
>>>      uint32_t *ptr = (uint32_t *)jmp_addr;
>>> -    *ptr = deposit32(*ptr, 0, 26, addr >> 2);
>>> +    uint32_t insn = atomic_read(ptr);
>>> +    atomic_set(ptr, deposit32(insn, 0, 26, addr >> 2));
>>>      flush_icache_range(jmp_addr, jmp_addr + 4);
>> Does it really make sense to read and write the value atomically? The
>> resulting operation is still not atomic, something can happen in
>> between.
> Hmm, thinking more about that, given the only instruction used is "J",
> we don't have to read the value, patch it and write it. We can directly
> use something like (untested):
>
>     atomic_set(ptr, (0x02 << 26) | (addr >> 2));

Hmm, looking at "case INDEX_op_goto_tb:" in tcg_out_op() again I'm
thinking about:

    atomic_set(ptr, deposit32(OPC_J, 0, 26, addr >> 2));

Kind regards,
Sergey

Re: [Qemu-devel] [PATCH v2 10/11] tcg/mips: Make direct jump patching thread-safe

[Aurelien Jarno]

On 2016-04-22 20:00, Sergey Fedorov wrote:
> On 22/04/16 19:51, Aurelien Jarno wrote:
> > On 2016-04-22 18:47, Aurelien Jarno wrote:
> >> On 2016-04-22 19:08, Sergey Fedorov wrote:
> >>> From: Sergey Fedorov <serge.fdrv@gmail.com>
> >>>
> >>> Ensure direct jump patching in MIPS is atomic by using
> >>> atomic_read()/atomic_set() for code patching.
> >>>
> >>> Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com>
> >>> Signed-off-by: Sergey Fedorov <sergey.fedorov@linaro.org>
> >>> ---
> >>>
> >>> Changes in v2:
> >>>  * s/atomic_write/atomic_set/
> >>>
> >>>  tcg/mips/tcg-target.inc.c | 3 ++-
> >>>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/tcg/mips/tcg-target.inc.c b/tcg/mips/tcg-target.inc.c
> >>> index 682e19897db0..cefc0398018a 100644
> >>> --- a/tcg/mips/tcg-target.inc.c
> >>> +++ b/tcg/mips/tcg-target.inc.c
> >>> @@ -1886,6 +1886,7 @@ static void tcg_target_init(TCGContext *s)
> >>>  void tb_set_jmp_target1(uintptr_t jmp_addr, uintptr_t addr)
> >>>  {
> >>>      uint32_t *ptr = (uint32_t *)jmp_addr;
> >>> -    *ptr = deposit32(*ptr, 0, 26, addr >> 2);
> >>> +    uint32_t insn = atomic_read(ptr);
> >>> +    atomic_set(ptr, deposit32(insn, 0, 26, addr >> 2));
> >>>      flush_icache_range(jmp_addr, jmp_addr + 4);
> >> Does it really make sense to read and write the value atomically? The
> >> resulting operation is still not atomic, something can happen in
> >> between.
> > Hmm, thinking more about that, given the only instruction used is "J",
> > we don't have to read the value, patch it and write it. We can directly
> > use something like (untested):
> >
> >     atomic_set(ptr, (0x02 << 26) | (addr >> 2));
> 
> Hmm, looking at "case INDEX_op_goto_tb:" in tcg_out_op() again I'm
> thinking about:
> 
>     atomic_set(ptr, deposit32(OPC_J, 0, 26, addr >> 2));

Oh right, I forgot when reviewing the patch that this code is actually
in tcg-target.inc.c, and that the OPC_J constant is available.

Your version looks good to me.

Aurelien

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH v2 10/11] tcg/mips: Make direct jump patching thread-safe

[Sergey Fedorov]

On 22/04/16 19:47, Aurelien Jarno wrote:
> On 2016-04-22 19:08, Sergey Fedorov wrote:
>> From: Sergey Fedorov <serge.fdrv@gmail.com>
>>
>> Ensure direct jump patching in MIPS is atomic by using
>> atomic_read()/atomic_set() for code patching.
>>
>> Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com>
>> Signed-off-by: Sergey Fedorov <sergey.fedorov@linaro.org>
>> ---
>>
>> Changes in v2:
>>  * s/atomic_write/atomic_set/
>>
>>  tcg/mips/tcg-target.inc.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/tcg/mips/tcg-target.inc.c b/tcg/mips/tcg-target.inc.c
>> index 682e19897db0..cefc0398018a 100644
>> --- a/tcg/mips/tcg-target.inc.c
>> +++ b/tcg/mips/tcg-target.inc.c
>> @@ -1886,6 +1886,7 @@ static void tcg_target_init(TCGContext *s)
>>  void tb_set_jmp_target1(uintptr_t jmp_addr, uintptr_t addr)
>>  {
>>      uint32_t *ptr = (uint32_t *)jmp_addr;
>> -    *ptr = deposit32(*ptr, 0, 26, addr >> 2);
>> +    uint32_t insn = atomic_read(ptr);
>> +    atomic_set(ptr, deposit32(insn, 0, 26, addr >> 2));
>>      flush_icache_range(jmp_addr, jmp_addr + 4);
> Does it really make sense to read and write the value atomically? The
> resulting operation is still not atomic, something can happen in
> between.

Actually, it's not important to read it atomically because it's always
the target address part of the instruction gets only changed. So
whatever can happen in between is overwritten by subsequent deposit32().

Kind regards,
Sergey

[Qemu-devel] [PATCH v2 11/11] tcg: Note requirement on atomic direct jump patching

[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>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
---

Changes in v2:
 * Minor rewording

 include/exec/exec-all.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index 6a054ee720a8..67adcf137f8b 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -229,6 +229,7 @@ static inline void tlb_flush_by_mmuidx(CPUState *cpu, ...)
     || defined(__sparc__) || defined(__aarch64__) \
     || defined(__s390x__) || defined(__mips__) \
     || defined(CONFIG_TCG_INTERPRETER)
+/* NOTE: Direct jump patching must be atomic to be thread-safe. */
 #define USE_DIRECT_JUMP
 #endif
 
-- 
2.8.1

Re: [Qemu-devel] [PATCH v2 00/11] tcg: Make direct jump patching thread-safe

[Richard Henderson]

On 04/22/2016 09:08 AM, Sergey Fedorov wrote:
> When patching translated code for direct block chaining/unchaining,
> modification of concurrently executing code can happen in multi-threaded
> execution.  Currently only user-mode is affected. To make direct block patching
> safe, some care must be taken to make sure that the code modification is made
> atomically and concurrently executed code is guaranteed to be consistent.
>
> This patch series fixes all supported TCG targets which use direct patching and
> documents the requirement for direct jump patching be atomic and thread-safe.
>
> The series' tree can be found in a public git repository [1].
>
> [1]https://github.com/sergefdrv/qemu/tree/atomic-tb-patching-2
>
> Summary of changes in v2:
>   * Take out mistakingly pulled patches [PATCH 01/11] and [PATCH 02/11]
>   * Two new patches to add some handy macros for alignment
>     [PATCH v2 01/11] and [PATCH v2 02/11]
>   * Use new alignment macros instead of open-coding
>   * Use i386 tcg_out_nopn() implementation suggested by Richard Henderson;
>     rework alignment checking and gap calculation in [PATCH v2 05/11]
>   * Clean up reloc_pc24_atomic() in [PATCH v2 07/11]
>   * Use tcg_debug_assert() instead of assert()
>   * Use deposit32() in [PATCH v2 09/11]
>   * s/atomic_write/atomic_set/ in [PATCH v2 10/11]
>   * Minor rewording in [PATCH v2 11/11]

Applied all to tcg-next.  I applied the mips one-liner follow-up at the same time.


r~

Re: [Qemu-devel] [PATCH v2 00/11] tcg: Make direct jump patching thread-safe

[Sergey Fedorov]

On 25/04/16 00:36, Richard Henderson wrote:
> On 04/22/2016 09:08 AM, Sergey Fedorov wrote:
>> When patching translated code for direct block chaining/unchaining,
>> modification of concurrently executing code can happen in multi-threaded
>> execution.  Currently only user-mode is affected. To make direct
>> block patching
>> safe, some care must be taken to make sure that the code modification
>> is made
>> atomically and concurrently executed code is guaranteed to be
>> consistent.
>>
>> This patch series fixes all supported TCG targets which use direct
>> patching and
>> documents the requirement for direct jump patching be atomic and
>> thread-safe.
>>
>> The series' tree can be found in a public git repository [1].
>>
>> [1]https://github.com/sergefdrv/qemu/tree/atomic-tb-patching-2
>>
>> Summary of changes in v2:
>>   * Take out mistakingly pulled patches [PATCH 01/11] and [PATCH 02/11]
>>   * Two new patches to add some handy macros for alignment
>>     [PATCH v2 01/11] and [PATCH v2 02/11]
>>   * Use new alignment macros instead of open-coding
>>   * Use i386 tcg_out_nopn() implementation suggested by Richard
>> Henderson;
>>     rework alignment checking and gap calculation in [PATCH v2 05/11]
>>   * Clean up reloc_pc24_atomic() in [PATCH v2 07/11]
>>   * Use tcg_debug_assert() instead of assert()
>>   * Use deposit32() in [PATCH v2 09/11]
>>   * s/atomic_write/atomic_set/ in [PATCH v2 10/11]
>>   * Minor rewording in [PATCH v2 11/11]
>
> Applied all to tcg-next.  I applied the mips one-liner follow-up at
> the same time.

Cool, thanks! :)

Kind regards,
Sergey