[Qemu-devel] [PATCH 0/5] tcg: Fix mmap_lock assertion failure, take 2

[Qemu-devel] [PATCH 0/5] tcg: Fix mmap_lock assertion failure, take 2

[Richard Henderson]

While I could not replicate the failure Peter reported, the apparent
root cause -- the old magic fixed page -- should affect other guests
as well.  In particular, the old arm32 magic fixed page at 0xffff0f00,
and the hppa magic fixed page at 0.

In the arm32 and hppa cases that I just mentioned -- but notably not
the x86_64 case that Peter reported -- there is special-case code in
target/*/translate.c to handle those addresses without actually doing
the read from the unmapped address.

Therefore, until we fix these sort of address space representational
errors, we cannot even rely on page_check_range() to validate the
execute access.

Instead, modify the host signal handler to intercept this at SIGSEGV.
At this point we're sure that there is no guest special case that we
have overlooked, because we did attempt the read for execute.

Also, I noticed that we really ought to have some barriers around this
code to make sure that the modifications to helper_retaddr are in fact
visible to the host signal handler.

Also, some minor cleanups to the set of read functions that we expose
for use during translation.

Also, a trivial duplicated condition.


r~


Richard Henderson (5):
  include/qemu/atomic.h: Add signal_barrier
  tcg: Introduce set/clear_helper_retaddr
  tcg: Remove cpu_ld*_code_ra
  tcg: Remove duplicate #if !defined(CODE_ACCESS)
  tcg: Release mmap_lock on translation fault

 include/exec/cpu_ldst.h                   | 20 ++++++
 include/exec/cpu_ldst_useronly_template.h | 40 ++++++++----
 include/qemu/atomic.h                     | 11 ++++
 accel/tcg/user-exec.c                     | 76 ++++++++++++++++-------
 target/arm/helper-a64.c                   |  8 +--
 target/arm/sve_helper.c                   | 43 +++++++------
 6 files changed, 135 insertions(+), 63 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH 1/5] include/qemu/atomic.h: Add signal_barrier

[Richard Henderson]

We have some potential race conditions vs our user-exec signal
handler that will be solved with this barrier.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/qemu/atomic.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h
index a6ac188188..f9cd24c899 100644
--- a/include/qemu/atomic.h
+++ b/include/qemu/atomic.h
@@ -88,6 +88,13 @@
 #define smp_read_barrier_depends()   barrier()
 #endif
 
+/*
+ * A signal barrier forces all pending local memory ops to be observed before
+ * a SIGSEGV is delivered to the *same* thread.  In practice this is exactly
+ * the same as barrier(), but since we have the correct builtin, use it.
+ */
+#define signal_barrier()    __atomic_signal_fence(__ATOMIC_SEQ_CST)
+
 /* Sanity check that the size of an atomic operation isn't "overly large".
  * Despite the fact that e.g. i686 has 64-bit atomic operations, we do not
  * want to use them because we ought not need them, and this lets us do a
@@ -308,6 +315,10 @@
 #define smp_read_barrier_depends()   barrier()
 #endif
 
+#ifndef signal_barrier
+#define signal_barrier()    barrier()
+#endif
+
 /* These will only be atomic if the processor does the fetch or store
  * in a single issue memory operation
  */
-- 
2.17.1

Re: [Qemu-devel] [PATCH 1/5] include/qemu/atomic.h: Add signal_barrier

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> We have some potential race conditions vs our user-exec signal
> handler that will be solved with this barrier.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

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

> ---
>  include/qemu/atomic.h | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h
> index a6ac188188..f9cd24c899 100644
> --- a/include/qemu/atomic.h
> +++ b/include/qemu/atomic.h
> @@ -88,6 +88,13 @@
>  #define smp_read_barrier_depends()   barrier()
>  #endif
>
> +/*
> + * A signal barrier forces all pending local memory ops to be observed before
> + * a SIGSEGV is delivered to the *same* thread.  In practice this is exactly
> + * the same as barrier(), but since we have the correct builtin, use it.
> + */
> +#define signal_barrier()    __atomic_signal_fence(__ATOMIC_SEQ_CST)
> +
>  /* Sanity check that the size of an atomic operation isn't "overly large".
>   * Despite the fact that e.g. i686 has 64-bit atomic operations, we do not
>   * want to use them because we ought not need them, and this lets us do a
> @@ -308,6 +315,10 @@
>  #define smp_read_barrier_depends()   barrier()
>  #endif
>
> +#ifndef signal_barrier
> +#define signal_barrier()    barrier()
> +#endif
> +
>  /* These will only be atomic if the processor does the fetch or store
>   * in a single issue memory operation
>   */


--
Alex Bennée

[Qemu-devel] [PATCH 2/5] tcg: Introduce set/clear_helper_retaddr

[Richard Henderson]

At present we have a potential error in that helper_retaddr contains
data for handle_cpu_signal, but we have not ensured that those stores
will be scheduled properly before the operation that may fault.

It might be that these races are not in practice observable, due to
our use of -fno-strict-aliasing, but better safe than sorry.

Adjust all of the setters of helper_retaddr.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/exec/cpu_ldst.h                   | 20 +++++++++++
 include/exec/cpu_ldst_useronly_template.h | 12 +++----
 accel/tcg/user-exec.c                     | 11 +++---
 target/arm/helper-a64.c                   |  8 ++---
 target/arm/sve_helper.c                   | 43 +++++++++++------------
 5 files changed, 57 insertions(+), 37 deletions(-)

diff --git a/include/exec/cpu_ldst.h b/include/exec/cpu_ldst.h
index a08b11bd2c..9de8c93303 100644
--- a/include/exec/cpu_ldst.h
+++ b/include/exec/cpu_ldst.h
@@ -89,6 +89,26 @@ typedef target_ulong abi_ptr;
 
 extern __thread uintptr_t helper_retaddr;
 
+static inline void set_helper_retaddr(uintptr_t ra)
+{
+    helper_retaddr = ra;
+    /*
+     * Ensure that this write is visible to the SIGSEGV handler that
+     * may be invoked due to a subsequent invalid memory operation.
+     */
+    signal_barrier();
+}
+
+static inline void clear_helper_retaddr(void)
+{
+    /*
+     * Ensure that previous memory operations have succeeded before
+     * removing the data visible to the signal handler.
+     */
+    signal_barrier();
+    helper_retaddr = 0;
+}
+
 /* In user-only mode we provide only the _code and _data accessors. */
 
 #define MEMSUFFIX _data
diff --git a/include/exec/cpu_ldst_useronly_template.h b/include/exec/cpu_ldst_useronly_template.h
index bc45e2b8d4..e65733f7e2 100644
--- a/include/exec/cpu_ldst_useronly_template.h
+++ b/include/exec/cpu_ldst_useronly_template.h
@@ -78,9 +78,9 @@ glue(glue(glue(cpu_ld, USUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
                                                   uintptr_t retaddr)
 {
     RES_TYPE ret;
-    helper_retaddr = retaddr;
+    set_helper_retaddr(retaddr);
     ret = glue(glue(cpu_ld, USUFFIX), MEMSUFFIX)(env, ptr);
-    helper_retaddr = 0;
+    clear_helper_retaddr();
     return ret;
 }
 
@@ -102,9 +102,9 @@ glue(glue(glue(cpu_lds, SUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
                                                   uintptr_t retaddr)
 {
     int ret;
-    helper_retaddr = retaddr;
+    set_helper_retaddr(retaddr);
     ret = glue(glue(cpu_lds, SUFFIX), MEMSUFFIX)(env, ptr);
-    helper_retaddr = 0;
+    clear_helper_retaddr();
     return ret;
 }
 #endif
@@ -128,9 +128,9 @@ glue(glue(glue(cpu_st, SUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
                                                   RES_TYPE v,
                                                   uintptr_t retaddr)
 {
-    helper_retaddr = retaddr;
+    set_helper_retaddr(retaddr);
     glue(glue(cpu_st, SUFFIX), MEMSUFFIX)(env, ptr, v);
-    helper_retaddr = 0;
+    clear_helper_retaddr();
 }
 #endif
 
diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
index cb5f4b19c5..4384b59a4d 100644
--- a/accel/tcg/user-exec.c
+++ b/accel/tcg/user-exec.c
@@ -134,7 +134,7 @@ static inline int handle_cpu_signal(uintptr_t pc, siginfo_t *info,
              * currently executing TB was modified and must be exited
              * immediately.  Clear helper_retaddr for next execution.
              */
-            helper_retaddr = 0;
+            clear_helper_retaddr();
             cpu_exit_tb_from_sighandler(cpu, old_set);
             /* NORETURN */
 
@@ -152,7 +152,7 @@ static inline int handle_cpu_signal(uintptr_t pc, siginfo_t *info,
      * an exception.  Undo signal and retaddr state prior to longjmp.
      */
     sigprocmask(SIG_SETMASK, old_set, NULL);
-    helper_retaddr = 0;
+    clear_helper_retaddr();
 
     cc = CPU_GET_CLASS(cpu);
     access_type = is_write ? MMU_DATA_STORE : MMU_DATA_LOAD;
@@ -682,14 +682,15 @@ static void *atomic_mmu_lookup(CPUArchState *env, target_ulong addr,
     if (unlikely(addr & (size - 1))) {
         cpu_loop_exit_atomic(env_cpu(env), retaddr);
     }
-    helper_retaddr = retaddr;
-    return g2h(addr);
+    void *ret = g2h(addr);
+    set_helper_retaddr(retaddr);
+    return ret;
 }
 
 /* Macro to call the above, with local variables from the use context.  */
 #define ATOMIC_MMU_DECLS do {} while (0)
 #define ATOMIC_MMU_LOOKUP  atomic_mmu_lookup(env, addr, DATA_SIZE, GETPC())
-#define ATOMIC_MMU_CLEANUP do { helper_retaddr = 0; } while (0)
+#define ATOMIC_MMU_CLEANUP do { clear_helper_retaddr(); } while (0)
 
 #define ATOMIC_NAME(X)   HELPER(glue(glue(atomic_ ## X, SUFFIX), END))
 #define EXTRA_ARGS
diff --git a/target/arm/helper-a64.c b/target/arm/helper-a64.c
index 44e45a8037..060699b901 100644
--- a/target/arm/helper-a64.c
+++ b/target/arm/helper-a64.c
@@ -554,7 +554,7 @@ uint64_t HELPER(paired_cmpxchg64_le)(CPUARMState *env, uint64_t addr,
     /* ??? Enforce alignment.  */
     uint64_t *haddr = g2h(addr);
 
-    helper_retaddr = ra;
+    set_helper_retaddr(ra);
     o0 = ldq_le_p(haddr + 0);
     o1 = ldq_le_p(haddr + 1);
     oldv = int128_make128(o0, o1);
@@ -564,7 +564,7 @@ uint64_t HELPER(paired_cmpxchg64_le)(CPUARMState *env, uint64_t addr,
         stq_le_p(haddr + 0, int128_getlo(newv));
         stq_le_p(haddr + 1, int128_gethi(newv));
     }
-    helper_retaddr = 0;
+    clear_helper_retaddr();
 #else
     int mem_idx = cpu_mmu_index(env, false);
     TCGMemOpIdx oi0 = make_memop_idx(MO_LEQ | MO_ALIGN_16, mem_idx);
@@ -624,7 +624,7 @@ uint64_t HELPER(paired_cmpxchg64_be)(CPUARMState *env, uint64_t addr,
     /* ??? Enforce alignment.  */
     uint64_t *haddr = g2h(addr);
 
-    helper_retaddr = ra;
+    set_helper_retaddr(ra);
     o1 = ldq_be_p(haddr + 0);
     o0 = ldq_be_p(haddr + 1);
     oldv = int128_make128(o0, o1);
@@ -634,7 +634,7 @@ uint64_t HELPER(paired_cmpxchg64_be)(CPUARMState *env, uint64_t addr,
         stq_be_p(haddr + 0, int128_gethi(newv));
         stq_be_p(haddr + 1, int128_getlo(newv));
     }
-    helper_retaddr = 0;
+    clear_helper_retaddr();
 #else
     int mem_idx = cpu_mmu_index(env, false);
     TCGMemOpIdx oi0 = make_memop_idx(MO_BEQ | MO_ALIGN_16, mem_idx);
diff --git a/target/arm/sve_helper.c b/target/arm/sve_helper.c
index fd434c66ea..fc0c1755d2 100644
--- a/target/arm/sve_helper.c
+++ b/target/arm/sve_helper.c
@@ -4125,12 +4125,11 @@ static intptr_t max_for_page(target_ulong base, intptr_t mem_off,
     return MIN(split, mem_max - mem_off) + mem_off;
 }
 
-static inline void set_helper_retaddr(uintptr_t ra)
-{
-#ifdef CONFIG_USER_ONLY
-    helper_retaddr = ra;
+#ifndef CONFIG_USER_ONLY
+/* These are normally defined only for CONFIG_USER_ONLY in <exec/cpu_ldst.h> */
+static inline void set_helper_retaddr(uintptr_t ra) { }
+static inline void clear_helper_retaddr(void) { }
 #endif
-}
 
 /*
  * The result of tlb_vaddr_to_host for user-only is just g2h(x),
@@ -4188,7 +4187,7 @@ static void sve_ld1_r(CPUARMState *env, void *vg, const target_ulong addr,
         if (test_host_page(host)) {
             mem_off = host_fn(vd, vg, host - mem_off, mem_off, mem_max);
             tcg_debug_assert(mem_off == mem_max);
-            set_helper_retaddr(0);
+            clear_helper_retaddr();
             /* After having taken any fault, zero leading inactive elements. */
             swap_memzero(vd, reg_off);
             return;
@@ -4239,7 +4238,7 @@ static void sve_ld1_r(CPUARMState *env, void *vg, const target_ulong addr,
     }
 #endif
 
-    set_helper_retaddr(0);
+    clear_helper_retaddr();
     memcpy(vd, &scratch, reg_max);
 }
 
@@ -4312,7 +4311,7 @@ static void sve_ld2_r(CPUARMState *env, void *vg, target_ulong addr,
             addr += 2 * size;
         } while (i & 15);
     }
-    set_helper_retaddr(0);
+    clear_helper_retaddr();
 
     /* Wait until all exceptions have been raised to write back.  */
     memcpy(&env->vfp.zregs[rd], &scratch[0], oprsz);
@@ -4341,7 +4340,7 @@ static void sve_ld3_r(CPUARMState *env, void *vg, target_ulong addr,
             addr += 3 * size;
         } while (i & 15);
     }
-    set_helper_retaddr(0);
+    clear_helper_retaddr();
 
     /* Wait until all exceptions have been raised to write back.  */
     memcpy(&env->vfp.zregs[rd], &scratch[0], oprsz);
@@ -4372,7 +4371,7 @@ static void sve_ld4_r(CPUARMState *env, void *vg, target_ulong addr,
             addr += 4 * size;
         } while (i & 15);
     }
-    set_helper_retaddr(0);
+    clear_helper_retaddr();
 
     /* Wait until all exceptions have been raised to write back.  */
     memcpy(&env->vfp.zregs[rd], &scratch[0], oprsz);
@@ -4494,7 +4493,7 @@ static void sve_ldff1_r(CPUARMState *env, void *vg, const target_ulong addr,
         if (test_host_page(host)) {
             mem_off = host_fn(vd, vg, host - mem_off, mem_off, mem_max);
             tcg_debug_assert(mem_off == mem_max);
-            set_helper_retaddr(0);
+            clear_helper_retaddr();
             /* After any fault, zero any leading inactive elements.  */
             swap_memzero(vd, reg_off);
             return;
@@ -4537,7 +4536,7 @@ static void sve_ldff1_r(CPUARMState *env, void *vg, const target_ulong addr,
     }
 #endif
 
-    set_helper_retaddr(0);
+    clear_helper_retaddr();
     record_fault(env, reg_off, reg_max);
 }
 
@@ -4740,7 +4739,7 @@ static void sve_st1_r(CPUARMState *env, void *vg, target_ulong addr,
             addr += msize;
         } while (i & 15);
     }
-    set_helper_retaddr(0);
+    clear_helper_retaddr();
 }
 
 static void sve_st2_r(CPUARMState *env, void *vg, target_ulong addr,
@@ -4766,7 +4765,7 @@ static void sve_st2_r(CPUARMState *env, void *vg, target_ulong addr,
             addr += 2 * msize;
         } while (i & 15);
     }
-    set_helper_retaddr(0);
+    clear_helper_retaddr();
 }
 
 static void sve_st3_r(CPUARMState *env, void *vg, target_ulong addr,
@@ -4794,7 +4793,7 @@ static void sve_st3_r(CPUARMState *env, void *vg, target_ulong addr,
             addr += 3 * msize;
         } while (i & 15);
     }
-    set_helper_retaddr(0);
+    clear_helper_retaddr();
 }
 
 static void sve_st4_r(CPUARMState *env, void *vg, target_ulong addr,
@@ -4824,7 +4823,7 @@ static void sve_st4_r(CPUARMState *env, void *vg, target_ulong addr,
             addr += 4 * msize;
         } while (i & 15);
     }
-    set_helper_retaddr(0);
+    clear_helper_retaddr();
 }
 
 #define DO_STN_1(N, NAME, ESIZE) \
@@ -4932,7 +4931,7 @@ static void sve_ld1_zs(CPUARMState *env, void *vd, void *vg, void *vm,
             i += 4, pg >>= 4;
         } while (i & 15);
     }
-    set_helper_retaddr(0);
+    clear_helper_retaddr();
 
     /* Wait until all exceptions have been raised to write back.  */
     memcpy(vd, &scratch, oprsz);
@@ -4955,7 +4954,7 @@ static void sve_ld1_zd(CPUARMState *env, void *vd, void *vg, void *vm,
             tlb_fn(env, &scratch, i * 8, base + (off << scale), oi, ra);
         }
     }
-    set_helper_retaddr(0);
+    clear_helper_retaddr();
 
     /* Wait until all exceptions have been raised to write back.  */
     memcpy(vd, &scratch, oprsz * 8);
@@ -5133,7 +5132,7 @@ static inline void sve_ldff1_zs(CPUARMState *env, void *vd, void *vg, void *vm,
         tlb_fn(env, vd, reg_off, addr, oi, ra);
 
         /* The rest of the reads will be non-faulting.  */
-        set_helper_retaddr(0);
+        clear_helper_retaddr();
     }
 
     /* After any fault, zero the leading predicated false elements.  */
@@ -5175,7 +5174,7 @@ static inline void sve_ldff1_zd(CPUARMState *env, void *vd, void *vg, void *vm,
         tlb_fn(env, vd, reg_off, addr, oi, ra);
 
         /* The rest of the reads will be non-faulting.  */
-        set_helper_retaddr(0);
+        clear_helper_retaddr();
     }
 
     /* After any fault, zero the leading predicated false elements.  */
@@ -5299,7 +5298,7 @@ static void sve_st1_zs(CPUARMState *env, void *vd, void *vg, void *vm,
             i += 4, pg >>= 4;
         } while (i & 15);
     }
-    set_helper_retaddr(0);
+    clear_helper_retaddr();
 }
 
 static void sve_st1_zd(CPUARMState *env, void *vd, void *vg, void *vm,
@@ -5318,7 +5317,7 @@ static void sve_st1_zd(CPUARMState *env, void *vd, void *vg, void *vm,
             tlb_fn(env, vd, i * 8, base + (off << scale), oi, ra);
         }
     }
-    set_helper_retaddr(0);
+    clear_helper_retaddr();
 }
 
 #define DO_ST1_ZPZ_S(MEM, OFS) \
-- 
2.17.1

Re: [Qemu-devel] [PATCH 2/5] tcg: Introduce set/clear_helper_retaddr

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> At present we have a potential error in that helper_retaddr contains
> data for handle_cpu_signal, but we have not ensured that those stores
> will be scheduled properly before the operation that may fault.
>
> It might be that these races are not in practice observable, due to
> our use of -fno-strict-aliasing, but better safe than sorry.
>
> Adjust all of the setters of helper_retaddr.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  include/exec/cpu_ldst.h                   | 20 +++++++++++
>  include/exec/cpu_ldst_useronly_template.h | 12 +++----
>  accel/tcg/user-exec.c                     | 11 +++---
>  target/arm/helper-a64.c                   |  8 ++---
>  target/arm/sve_helper.c                   | 43 +++++++++++------------
>  5 files changed, 57 insertions(+), 37 deletions(-)
>
> diff --git a/include/exec/cpu_ldst.h b/include/exec/cpu_ldst.h
> index a08b11bd2c..9de8c93303 100644
> --- a/include/exec/cpu_ldst.h
> +++ b/include/exec/cpu_ldst.h
> @@ -89,6 +89,26 @@ typedef target_ulong abi_ptr;
>
>  extern __thread uintptr_t helper_retaddr;
>
> +static inline void set_helper_retaddr(uintptr_t ra)
> +{
> +    helper_retaddr = ra;
> +    /*
> +     * Ensure that this write is visible to the SIGSEGV handler that
> +     * may be invoked due to a subsequent invalid memory operation.
> +     */
> +    signal_barrier();
> +}
> +
> +static inline void clear_helper_retaddr(void)
> +{
> +    /*
> +     * Ensure that previous memory operations have succeeded before
> +     * removing the data visible to the signal handler.
> +     */
> +    signal_barrier();
> +    helper_retaddr = 0;
> +}
> +
>  /* In user-only mode we provide only the _code and _data accessors. */
>
>  #define MEMSUFFIX _data
> diff --git a/include/exec/cpu_ldst_useronly_template.h b/include/exec/cpu_ldst_useronly_template.h
> index bc45e2b8d4..e65733f7e2 100644
> --- a/include/exec/cpu_ldst_useronly_template.h
> +++ b/include/exec/cpu_ldst_useronly_template.h
> @@ -78,9 +78,9 @@ glue(glue(glue(cpu_ld, USUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
>                                                    uintptr_t retaddr)
>  {
>      RES_TYPE ret;
> -    helper_retaddr = retaddr;
> +    set_helper_retaddr(retaddr);
>      ret = glue(glue(cpu_ld, USUFFIX), MEMSUFFIX)(env, ptr);
> -    helper_retaddr = 0;
> +    clear_helper_retaddr();
>      return ret;
>  }
>
> @@ -102,9 +102,9 @@ glue(glue(glue(cpu_lds, SUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
>                                                    uintptr_t retaddr)
>  {
>      int ret;
> -    helper_retaddr = retaddr;
> +    set_helper_retaddr(retaddr);
>      ret = glue(glue(cpu_lds, SUFFIX), MEMSUFFIX)(env, ptr);
> -    helper_retaddr = 0;
> +    clear_helper_retaddr();
>      return ret;
>  }
>  #endif
> @@ -128,9 +128,9 @@ glue(glue(glue(cpu_st, SUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
>                                                    RES_TYPE v,
>                                                    uintptr_t retaddr)
>  {
> -    helper_retaddr = retaddr;
> +    set_helper_retaddr(retaddr);
>      glue(glue(cpu_st, SUFFIX), MEMSUFFIX)(env, ptr, v);
> -    helper_retaddr = 0;
> +    clear_helper_retaddr();
>  }
>  #endif
>
> diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
> index cb5f4b19c5..4384b59a4d 100644
> --- a/accel/tcg/user-exec.c
> +++ b/accel/tcg/user-exec.c
> @@ -134,7 +134,7 @@ static inline int handle_cpu_signal(uintptr_t pc, siginfo_t *info,
>               * currently executing TB was modified and must be exited
>               * immediately.  Clear helper_retaddr for next execution.
>               */
> -            helper_retaddr = 0;
> +            clear_helper_retaddr();
>              cpu_exit_tb_from_sighandler(cpu, old_set);
>              /* NORETURN */
>
> @@ -152,7 +152,7 @@ static inline int handle_cpu_signal(uintptr_t pc, siginfo_t *info,
>       * an exception.  Undo signal and retaddr state prior to longjmp.
>       */
>      sigprocmask(SIG_SETMASK, old_set, NULL);
> -    helper_retaddr = 0;
> +    clear_helper_retaddr();
>
>      cc = CPU_GET_CLASS(cpu);
>      access_type = is_write ? MMU_DATA_STORE : MMU_DATA_LOAD;
> @@ -682,14 +682,15 @@ static void *atomic_mmu_lookup(CPUArchState *env, target_ulong addr,
>      if (unlikely(addr & (size - 1))) {
>          cpu_loop_exit_atomic(env_cpu(env), retaddr);
>      }
> -    helper_retaddr = retaddr;
> -    return g2h(addr);
> +    void *ret = g2h(addr);
> +    set_helper_retaddr(retaddr);
> +    return ret;
>  }
>
>  /* Macro to call the above, with local variables from the use context.  */
>  #define ATOMIC_MMU_DECLS do {} while (0)
>  #define ATOMIC_MMU_LOOKUP  atomic_mmu_lookup(env, addr, DATA_SIZE, GETPC())
> -#define ATOMIC_MMU_CLEANUP do { helper_retaddr = 0; } while (0)
> +#define ATOMIC_MMU_CLEANUP do { clear_helper_retaddr(); } while (0)
>
>  #define ATOMIC_NAME(X)   HELPER(glue(glue(atomic_ ## X, SUFFIX), END))
>  #define EXTRA_ARGS
> diff --git a/target/arm/helper-a64.c b/target/arm/helper-a64.c
> index 44e45a8037..060699b901 100644
> --- a/target/arm/helper-a64.c
> +++ b/target/arm/helper-a64.c
> @@ -554,7 +554,7 @@ uint64_t HELPER(paired_cmpxchg64_le)(CPUARMState *env, uint64_t addr,
>      /* ??? Enforce alignment.  */
>      uint64_t *haddr = g2h(addr);
>
> -    helper_retaddr = ra;
> +    set_helper_retaddr(ra);
>      o0 = ldq_le_p(haddr + 0);
>      o1 = ldq_le_p(haddr + 1);
>      oldv = int128_make128(o0, o1);
> @@ -564,7 +564,7 @@ uint64_t HELPER(paired_cmpxchg64_le)(CPUARMState *env, uint64_t addr,
>          stq_le_p(haddr + 0, int128_getlo(newv));
>          stq_le_p(haddr + 1, int128_gethi(newv));
>      }
> -    helper_retaddr = 0;
> +    clear_helper_retaddr();
>  #else
>      int mem_idx = cpu_mmu_index(env, false);
>      TCGMemOpIdx oi0 = make_memop_idx(MO_LEQ | MO_ALIGN_16, mem_idx);
> @@ -624,7 +624,7 @@ uint64_t HELPER(paired_cmpxchg64_be)(CPUARMState *env, uint64_t addr,
>      /* ??? Enforce alignment.  */
>      uint64_t *haddr = g2h(addr);
>
> -    helper_retaddr = ra;
> +    set_helper_retaddr(ra);
>      o1 = ldq_be_p(haddr + 0);
>      o0 = ldq_be_p(haddr + 1);
>      oldv = int128_make128(o0, o1);
> @@ -634,7 +634,7 @@ uint64_t HELPER(paired_cmpxchg64_be)(CPUARMState *env, uint64_t addr,
>          stq_be_p(haddr + 0, int128_gethi(newv));
>          stq_be_p(haddr + 1, int128_getlo(newv));
>      }
> -    helper_retaddr = 0;
> +    clear_helper_retaddr();
>  #else
>      int mem_idx = cpu_mmu_index(env, false);
>      TCGMemOpIdx oi0 = make_memop_idx(MO_BEQ | MO_ALIGN_16, mem_idx);
> diff --git a/target/arm/sve_helper.c b/target/arm/sve_helper.c
> index fd434c66ea..fc0c1755d2 100644
> --- a/target/arm/sve_helper.c
> +++ b/target/arm/sve_helper.c
> @@ -4125,12 +4125,11 @@ static intptr_t max_for_page(target_ulong base, intptr_t mem_off,
>      return MIN(split, mem_max - mem_off) + mem_off;
>  }
>
> -static inline void set_helper_retaddr(uintptr_t ra)
> -{
> -#ifdef CONFIG_USER_ONLY
> -    helper_retaddr = ra;
> +#ifndef CONFIG_USER_ONLY
> +/* These are normally defined only for CONFIG_USER_ONLY in <exec/cpu_ldst.h> */
> +static inline void set_helper_retaddr(uintptr_t ra) { }
> +static inline void clear_helper_retaddr(void) { }

Why aren't these stubs in the #else leg of cpu_ldst.h?

With that:

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

--
Alex Bennée

Re: [Qemu-devel] [PATCH 2/5] tcg: Introduce set/clear_helper_retaddr

[Richard Henderson]

On 7/9/19 12:07 PM, Alex Bennée wrote:
> 
> Richard Henderson <richard.henderson@linaro.org> writes:
> 
>> At present we have a potential error in that helper_retaddr contains
>> data for handle_cpu_signal, but we have not ensured that those stores
>> will be scheduled properly before the operation that may fault.
>>
>> It might be that these races are not in practice observable, due to
>> our use of -fno-strict-aliasing, but better safe than sorry.
>>
>> Adjust all of the setters of helper_retaddr.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>  include/exec/cpu_ldst.h                   | 20 +++++++++++
>>  include/exec/cpu_ldst_useronly_template.h | 12 +++----
>>  accel/tcg/user-exec.c                     | 11 +++---
>>  target/arm/helper-a64.c                   |  8 ++---
>>  target/arm/sve_helper.c                   | 43 +++++++++++------------
>>  5 files changed, 57 insertions(+), 37 deletions(-)
>>
>> diff --git a/include/exec/cpu_ldst.h b/include/exec/cpu_ldst.h
>> index a08b11bd2c..9de8c93303 100644
>> --- a/include/exec/cpu_ldst.h
>> +++ b/include/exec/cpu_ldst.h
>> @@ -89,6 +89,26 @@ typedef target_ulong abi_ptr;
>>
>>  extern __thread uintptr_t helper_retaddr;
>>
>> +static inline void set_helper_retaddr(uintptr_t ra)
>> +{
>> +    helper_retaddr = ra;
>> +    /*
>> +     * Ensure that this write is visible to the SIGSEGV handler that
>> +     * may be invoked due to a subsequent invalid memory operation.
>> +     */
>> +    signal_barrier();
>> +}
>> +
>> +static inline void clear_helper_retaddr(void)
>> +{
>> +    /*
>> +     * Ensure that previous memory operations have succeeded before
>> +     * removing the data visible to the signal handler.
>> +     */
>> +    signal_barrier();
>> +    helper_retaddr = 0;
>> +}
>> +
>>  /* In user-only mode we provide only the _code and _data accessors. */
>>
>>  #define MEMSUFFIX _data
>> diff --git a/include/exec/cpu_ldst_useronly_template.h b/include/exec/cpu_ldst_useronly_template.h
>> index bc45e2b8d4..e65733f7e2 100644
>> --- a/include/exec/cpu_ldst_useronly_template.h
>> +++ b/include/exec/cpu_ldst_useronly_template.h
>> @@ -78,9 +78,9 @@ glue(glue(glue(cpu_ld, USUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
>>                                                    uintptr_t retaddr)
>>  {
>>      RES_TYPE ret;
>> -    helper_retaddr = retaddr;
>> +    set_helper_retaddr(retaddr);
>>      ret = glue(glue(cpu_ld, USUFFIX), MEMSUFFIX)(env, ptr);
>> -    helper_retaddr = 0;
>> +    clear_helper_retaddr();
>>      return ret;
>>  }
>>
>> @@ -102,9 +102,9 @@ glue(glue(glue(cpu_lds, SUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
>>                                                    uintptr_t retaddr)
>>  {
>>      int ret;
>> -    helper_retaddr = retaddr;
>> +    set_helper_retaddr(retaddr);
>>      ret = glue(glue(cpu_lds, SUFFIX), MEMSUFFIX)(env, ptr);
>> -    helper_retaddr = 0;
>> +    clear_helper_retaddr();
>>      return ret;
>>  }
>>  #endif
>> @@ -128,9 +128,9 @@ glue(glue(glue(cpu_st, SUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
>>                                                    RES_TYPE v,
>>                                                    uintptr_t retaddr)
>>  {
>> -    helper_retaddr = retaddr;
>> +    set_helper_retaddr(retaddr);
>>      glue(glue(cpu_st, SUFFIX), MEMSUFFIX)(env, ptr, v);
>> -    helper_retaddr = 0;
>> +    clear_helper_retaddr();
>>  }
>>  #endif
>>
>> diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
>> index cb5f4b19c5..4384b59a4d 100644
>> --- a/accel/tcg/user-exec.c
>> +++ b/accel/tcg/user-exec.c
>> @@ -134,7 +134,7 @@ static inline int handle_cpu_signal(uintptr_t pc, siginfo_t *info,
>>               * currently executing TB was modified and must be exited
>>               * immediately.  Clear helper_retaddr for next execution.
>>               */
>> -            helper_retaddr = 0;
>> +            clear_helper_retaddr();
>>              cpu_exit_tb_from_sighandler(cpu, old_set);
>>              /* NORETURN */
>>
>> @@ -152,7 +152,7 @@ static inline int handle_cpu_signal(uintptr_t pc, siginfo_t *info,
>>       * an exception.  Undo signal and retaddr state prior to longjmp.
>>       */
>>      sigprocmask(SIG_SETMASK, old_set, NULL);
>> -    helper_retaddr = 0;
>> +    clear_helper_retaddr();
>>
>>      cc = CPU_GET_CLASS(cpu);
>>      access_type = is_write ? MMU_DATA_STORE : MMU_DATA_LOAD;
>> @@ -682,14 +682,15 @@ static void *atomic_mmu_lookup(CPUArchState *env, target_ulong addr,
>>      if (unlikely(addr & (size - 1))) {
>>          cpu_loop_exit_atomic(env_cpu(env), retaddr);
>>      }
>> -    helper_retaddr = retaddr;
>> -    return g2h(addr);
>> +    void *ret = g2h(addr);
>> +    set_helper_retaddr(retaddr);
>> +    return ret;
>>  }
>>
>>  /* Macro to call the above, with local variables from the use context.  */
>>  #define ATOMIC_MMU_DECLS do {} while (0)
>>  #define ATOMIC_MMU_LOOKUP  atomic_mmu_lookup(env, addr, DATA_SIZE, GETPC())
>> -#define ATOMIC_MMU_CLEANUP do { helper_retaddr = 0; } while (0)
>> +#define ATOMIC_MMU_CLEANUP do { clear_helper_retaddr(); } while (0)
>>
>>  #define ATOMIC_NAME(X)   HELPER(glue(glue(atomic_ ## X, SUFFIX), END))
>>  #define EXTRA_ARGS
>> diff --git a/target/arm/helper-a64.c b/target/arm/helper-a64.c
>> index 44e45a8037..060699b901 100644
>> --- a/target/arm/helper-a64.c
>> +++ b/target/arm/helper-a64.c
>> @@ -554,7 +554,7 @@ uint64_t HELPER(paired_cmpxchg64_le)(CPUARMState *env, uint64_t addr,
>>      /* ??? Enforce alignment.  */
>>      uint64_t *haddr = g2h(addr);
>>
>> -    helper_retaddr = ra;
>> +    set_helper_retaddr(ra);
>>      o0 = ldq_le_p(haddr + 0);
>>      o1 = ldq_le_p(haddr + 1);
>>      oldv = int128_make128(o0, o1);
>> @@ -564,7 +564,7 @@ uint64_t HELPER(paired_cmpxchg64_le)(CPUARMState *env, uint64_t addr,
>>          stq_le_p(haddr + 0, int128_getlo(newv));
>>          stq_le_p(haddr + 1, int128_gethi(newv));
>>      }
>> -    helper_retaddr = 0;
>> +    clear_helper_retaddr();
>>  #else
>>      int mem_idx = cpu_mmu_index(env, false);
>>      TCGMemOpIdx oi0 = make_memop_idx(MO_LEQ | MO_ALIGN_16, mem_idx);
>> @@ -624,7 +624,7 @@ uint64_t HELPER(paired_cmpxchg64_be)(CPUARMState *env, uint64_t addr,
>>      /* ??? Enforce alignment.  */
>>      uint64_t *haddr = g2h(addr);
>>
>> -    helper_retaddr = ra;
>> +    set_helper_retaddr(ra);
>>      o1 = ldq_be_p(haddr + 0);
>>      o0 = ldq_be_p(haddr + 1);
>>      oldv = int128_make128(o0, o1);
>> @@ -634,7 +634,7 @@ uint64_t HELPER(paired_cmpxchg64_be)(CPUARMState *env, uint64_t addr,
>>          stq_be_p(haddr + 0, int128_gethi(newv));
>>          stq_be_p(haddr + 1, int128_getlo(newv));
>>      }
>> -    helper_retaddr = 0;
>> +    clear_helper_retaddr();
>>  #else
>>      int mem_idx = cpu_mmu_index(env, false);
>>      TCGMemOpIdx oi0 = make_memop_idx(MO_BEQ | MO_ALIGN_16, mem_idx);
>> diff --git a/target/arm/sve_helper.c b/target/arm/sve_helper.c
>> index fd434c66ea..fc0c1755d2 100644
>> --- a/target/arm/sve_helper.c
>> +++ b/target/arm/sve_helper.c
>> @@ -4125,12 +4125,11 @@ static intptr_t max_for_page(target_ulong base, intptr_t mem_off,
>>      return MIN(split, mem_max - mem_off) + mem_off;
>>  }
>>
>> -static inline void set_helper_retaddr(uintptr_t ra)
>> -{
>> -#ifdef CONFIG_USER_ONLY
>> -    helper_retaddr = ra;
>> +#ifndef CONFIG_USER_ONLY
>> +/* These are normally defined only for CONFIG_USER_ONLY in <exec/cpu_ldst.h> */
>> +static inline void set_helper_retaddr(uintptr_t ra) { }
>> +static inline void clear_helper_retaddr(void) { }
> 
> Why aren't these stubs in the #else leg of cpu_ldst.h?

I'm not sure it makes sense to spread these around generically, since they are
no-ops which require the extra help of the "host_fn" pointers within that file.

In particular, the softmmu host_fn continues to use ra, while the linux-user
host_fn does not.  Indeed, the whole point of sve_helper.c using
set_helper_retaddr is to hoist the setting of helper_retaddr that would be done
for each occurrence of cpu_ld_data_ra() et al.


r~

Re: [Qemu-devel] [PATCH 2/5] tcg: Introduce set/clear_helper_retaddr

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> On 7/9/19 12:07 PM, Alex Bennée wrote:
>>
>> Richard Henderson <richard.henderson@linaro.org> writes:
>>
>>> At present we have a potential error in that helper_retaddr contains
>>> data for handle_cpu_signal, but we have not ensured that those stores
>>> will be scheduled properly before the operation that may fault.
>>>
>>> It might be that these races are not in practice observable, due to
>>> our use of -fno-strict-aliasing, but better safe than sorry.
>>>
>>> Adjust all of the setters of helper_retaddr.
>>>
>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>>> ---
>>>  include/exec/cpu_ldst.h                   | 20 +++++++++++
>>>  include/exec/cpu_ldst_useronly_template.h | 12 +++----
>>>  accel/tcg/user-exec.c                     | 11 +++---
>>>  target/arm/helper-a64.c                   |  8 ++---
>>>  target/arm/sve_helper.c                   | 43 +++++++++++------------
>>>  5 files changed, 57 insertions(+), 37 deletions(-)
>>>
>>> diff --git a/include/exec/cpu_ldst.h b/include/exec/cpu_ldst.h
>>> index a08b11bd2c..9de8c93303 100644
>>> --- a/include/exec/cpu_ldst.h
>>> +++ b/include/exec/cpu_ldst.h
>>> @@ -89,6 +89,26 @@ typedef target_ulong abi_ptr;
>>>
>>>  extern __thread uintptr_t helper_retaddr;
>>>
>>> +static inline void set_helper_retaddr(uintptr_t ra)
>>> +{
>>> +    helper_retaddr = ra;
>>> +    /*
>>> +     * Ensure that this write is visible to the SIGSEGV handler that
>>> +     * may be invoked due to a subsequent invalid memory operation.
>>> +     */
>>> +    signal_barrier();
>>> +}
>>> +
>>> +static inline void clear_helper_retaddr(void)
>>> +{
>>> +    /*
>>> +     * Ensure that previous memory operations have succeeded before
>>> +     * removing the data visible to the signal handler.
>>> +     */
>>> +    signal_barrier();
>>> +    helper_retaddr = 0;
>>> +}
>>> +
>>>  /* In user-only mode we provide only the _code and _data accessors. */
>>>
>>>  #define MEMSUFFIX _data
>>> diff --git a/include/exec/cpu_ldst_useronly_template.h b/include/exec/cpu_ldst_useronly_template.h
>>> index bc45e2b8d4..e65733f7e2 100644
>>> --- a/include/exec/cpu_ldst_useronly_template.h
>>> +++ b/include/exec/cpu_ldst_useronly_template.h
>>> @@ -78,9 +78,9 @@ glue(glue(glue(cpu_ld, USUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
>>>                                                    uintptr_t retaddr)
>>>  {
>>>      RES_TYPE ret;
>>> -    helper_retaddr = retaddr;
>>> +    set_helper_retaddr(retaddr);
>>>      ret = glue(glue(cpu_ld, USUFFIX), MEMSUFFIX)(env, ptr);
>>> -    helper_retaddr = 0;
>>> +    clear_helper_retaddr();
>>>      return ret;
>>>  }
>>>
>>> @@ -102,9 +102,9 @@ glue(glue(glue(cpu_lds, SUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
>>>                                                    uintptr_t retaddr)
>>>  {
>>>      int ret;
>>> -    helper_retaddr = retaddr;
>>> +    set_helper_retaddr(retaddr);
>>>      ret = glue(glue(cpu_lds, SUFFIX), MEMSUFFIX)(env, ptr);
>>> -    helper_retaddr = 0;
>>> +    clear_helper_retaddr();
>>>      return ret;
>>>  }
>>>  #endif
>>> @@ -128,9 +128,9 @@ glue(glue(glue(cpu_st, SUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
>>>                                                    RES_TYPE v,
>>>                                                    uintptr_t retaddr)
>>>  {
>>> -    helper_retaddr = retaddr;
>>> +    set_helper_retaddr(retaddr);
>>>      glue(glue(cpu_st, SUFFIX), MEMSUFFIX)(env, ptr, v);
>>> -    helper_retaddr = 0;
>>> +    clear_helper_retaddr();
>>>  }
>>>  #endif
>>>
>>> diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
>>> index cb5f4b19c5..4384b59a4d 100644
>>> --- a/accel/tcg/user-exec.c
>>> +++ b/accel/tcg/user-exec.c
>>> @@ -134,7 +134,7 @@ static inline int handle_cpu_signal(uintptr_t pc, siginfo_t *info,
>>>               * currently executing TB was modified and must be exited
>>>               * immediately.  Clear helper_retaddr for next execution.
>>>               */
>>> -            helper_retaddr = 0;
>>> +            clear_helper_retaddr();
>>>              cpu_exit_tb_from_sighandler(cpu, old_set);
>>>              /* NORETURN */
>>>
>>> @@ -152,7 +152,7 @@ static inline int handle_cpu_signal(uintptr_t pc, siginfo_t *info,
>>>       * an exception.  Undo signal and retaddr state prior to longjmp.
>>>       */
>>>      sigprocmask(SIG_SETMASK, old_set, NULL);
>>> -    helper_retaddr = 0;
>>> +    clear_helper_retaddr();
>>>
>>>      cc = CPU_GET_CLASS(cpu);
>>>      access_type = is_write ? MMU_DATA_STORE : MMU_DATA_LOAD;
>>> @@ -682,14 +682,15 @@ static void *atomic_mmu_lookup(CPUArchState *env, target_ulong addr,
>>>      if (unlikely(addr & (size - 1))) {
>>>          cpu_loop_exit_atomic(env_cpu(env), retaddr);
>>>      }
>>> -    helper_retaddr = retaddr;
>>> -    return g2h(addr);
>>> +    void *ret = g2h(addr);
>>> +    set_helper_retaddr(retaddr);
>>> +    return ret;
>>>  }
>>>
>>>  /* Macro to call the above, with local variables from the use context.  */
>>>  #define ATOMIC_MMU_DECLS do {} while (0)
>>>  #define ATOMIC_MMU_LOOKUP  atomic_mmu_lookup(env, addr, DATA_SIZE, GETPC())
>>> -#define ATOMIC_MMU_CLEANUP do { helper_retaddr = 0; } while (0)
>>> +#define ATOMIC_MMU_CLEANUP do { clear_helper_retaddr(); } while (0)
>>>
>>>  #define ATOMIC_NAME(X)   HELPER(glue(glue(atomic_ ## X, SUFFIX), END))
>>>  #define EXTRA_ARGS
>>> diff --git a/target/arm/helper-a64.c b/target/arm/helper-a64.c
>>> index 44e45a8037..060699b901 100644
>>> --- a/target/arm/helper-a64.c
>>> +++ b/target/arm/helper-a64.c
>>> @@ -554,7 +554,7 @@ uint64_t HELPER(paired_cmpxchg64_le)(CPUARMState *env, uint64_t addr,
>>>      /* ??? Enforce alignment.  */
>>>      uint64_t *haddr = g2h(addr);
>>>
>>> -    helper_retaddr = ra;
>>> +    set_helper_retaddr(ra);
>>>      o0 = ldq_le_p(haddr + 0);
>>>      o1 = ldq_le_p(haddr + 1);
>>>      oldv = int128_make128(o0, o1);
>>> @@ -564,7 +564,7 @@ uint64_t HELPER(paired_cmpxchg64_le)(CPUARMState *env, uint64_t addr,
>>>          stq_le_p(haddr + 0, int128_getlo(newv));
>>>          stq_le_p(haddr + 1, int128_gethi(newv));
>>>      }
>>> -    helper_retaddr = 0;
>>> +    clear_helper_retaddr();
>>>  #else
>>>      int mem_idx = cpu_mmu_index(env, false);
>>>      TCGMemOpIdx oi0 = make_memop_idx(MO_LEQ | MO_ALIGN_16, mem_idx);
>>> @@ -624,7 +624,7 @@ uint64_t HELPER(paired_cmpxchg64_be)(CPUARMState *env, uint64_t addr,
>>>      /* ??? Enforce alignment.  */
>>>      uint64_t *haddr = g2h(addr);
>>>
>>> -    helper_retaddr = ra;
>>> +    set_helper_retaddr(ra);
>>>      o1 = ldq_be_p(haddr + 0);
>>>      o0 = ldq_be_p(haddr + 1);
>>>      oldv = int128_make128(o0, o1);
>>> @@ -634,7 +634,7 @@ uint64_t HELPER(paired_cmpxchg64_be)(CPUARMState *env, uint64_t addr,
>>>          stq_be_p(haddr + 0, int128_gethi(newv));
>>>          stq_be_p(haddr + 1, int128_getlo(newv));
>>>      }
>>> -    helper_retaddr = 0;
>>> +    clear_helper_retaddr();
>>>  #else
>>>      int mem_idx = cpu_mmu_index(env, false);
>>>      TCGMemOpIdx oi0 = make_memop_idx(MO_BEQ | MO_ALIGN_16, mem_idx);
>>> diff --git a/target/arm/sve_helper.c b/target/arm/sve_helper.c
>>> index fd434c66ea..fc0c1755d2 100644
>>> --- a/target/arm/sve_helper.c
>>> +++ b/target/arm/sve_helper.c
>>> @@ -4125,12 +4125,11 @@ static intptr_t max_for_page(target_ulong base, intptr_t mem_off,
>>>      return MIN(split, mem_max - mem_off) + mem_off;
>>>  }
>>>
>>> -static inline void set_helper_retaddr(uintptr_t ra)
>>> -{
>>> -#ifdef CONFIG_USER_ONLY
>>> -    helper_retaddr = ra;
>>> +#ifndef CONFIG_USER_ONLY
>>> +/* These are normally defined only for CONFIG_USER_ONLY in <exec/cpu_ldst.h> */
>>> +static inline void set_helper_retaddr(uintptr_t ra) { }
>>> +static inline void clear_helper_retaddr(void) { }
>>
>> Why aren't these stubs in the #else leg of cpu_ldst.h?
>
> I'm not sure it makes sense to spread these around generically, since they are
> no-ops which require the extra help of the "host_fn" pointers within that file.
>
> In particular, the softmmu host_fn continues to use ra, while the linux-user
> host_fn does not.  Indeed, the whole point of sve_helper.c using
> set_helper_retaddr is to hoist the setting of helper_retaddr that would be done
> for each occurrence of cpu_ld_data_ra() et al.

Fair enough, keep the r-b.

--
Alex Bennée

[Qemu-devel] [PATCH 3/5] tcg: Remove cpu_ld*_code_ra

[Richard Henderson]

These functions are not used, and are not usable in the
context of code generation, because we never have a helper
return address to pass in to them.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/exec/cpu_ldst_useronly_template.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/include/exec/cpu_ldst_useronly_template.h b/include/exec/cpu_ldst_useronly_template.h
index e65733f7e2..8c7a2c6cd7 100644
--- a/include/exec/cpu_ldst_useronly_template.h
+++ b/include/exec/cpu_ldst_useronly_template.h
@@ -72,6 +72,7 @@ glue(glue(cpu_ld, USUFFIX), MEMSUFFIX)(CPUArchState *env, abi_ptr ptr)
     return glue(glue(ld, USUFFIX), _p)(g2h(ptr));
 }
 
+#ifndef CODE_ACCESS
 static inline RES_TYPE
 glue(glue(glue(cpu_ld, USUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
                                                   abi_ptr ptr,
@@ -83,6 +84,7 @@ glue(glue(glue(cpu_ld, USUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
     clear_helper_retaddr();
     return ret;
 }
+#endif
 
 #if DATA_SIZE <= 2
 static inline int
@@ -96,6 +98,7 @@ glue(glue(cpu_lds, SUFFIX), MEMSUFFIX)(CPUArchState *env, abi_ptr ptr)
     return glue(glue(lds, SUFFIX), _p)(g2h(ptr));
 }
 
+#ifndef CODE_ACCESS
 static inline int
 glue(glue(glue(cpu_lds, SUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
                                                   abi_ptr ptr,
@@ -107,7 +110,8 @@ glue(glue(glue(cpu_lds, SUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
     clear_helper_retaddr();
     return ret;
 }
-#endif
+#endif /* CODE_ACCESS */
+#endif /* DATA_SIZE <= 2 */
 
 #ifndef CODE_ACCESS
 static inline void
-- 
2.17.1

Re: [Qemu-devel] [PATCH 3/5] tcg: Remove cpu_ld*_code_ra

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> These functions are not used, and are not usable in the
> context of code generation, because we never have a helper
> return address to pass in to them.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

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

--
Alex Bennée

[Qemu-devel] [PATCH 4/5] tcg: Remove duplicate #if !defined(CODE_ACCESS)

[Richard Henderson]

This code block is already surrounded by #ifndef CODE_ACCESS.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/exec/cpu_ldst_useronly_template.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/include/exec/cpu_ldst_useronly_template.h b/include/exec/cpu_ldst_useronly_template.h
index 8c7a2c6cd7..d663826ac2 100644
--- a/include/exec/cpu_ldst_useronly_template.h
+++ b/include/exec/cpu_ldst_useronly_template.h
@@ -118,11 +118,9 @@ static inline void
 glue(glue(cpu_st, SUFFIX), MEMSUFFIX)(CPUArchState *env, abi_ptr ptr,
                                       RES_TYPE v)
 {
-#if !defined(CODE_ACCESS)
     trace_guest_mem_before_exec(
         env_cpu(env), ptr,
         trace_mem_build_info(SHIFT, false, MO_TE, true));
-#endif
     glue(glue(st, SUFFIX), _p)(g2h(ptr), v);
 }
 
-- 
2.17.1

Re: [Qemu-devel] [PATCH 4/5] tcg: Remove duplicate #if !defined(CODE_ACCESS)

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> This code block is already surrounded by #ifndef CODE_ACCESS.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

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

> ---
>  include/exec/cpu_ldst_useronly_template.h | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/include/exec/cpu_ldst_useronly_template.h b/include/exec/cpu_ldst_useronly_template.h
> index 8c7a2c6cd7..d663826ac2 100644
> --- a/include/exec/cpu_ldst_useronly_template.h
> +++ b/include/exec/cpu_ldst_useronly_template.h
> @@ -118,11 +118,9 @@ static inline void
>  glue(glue(cpu_st, SUFFIX), MEMSUFFIX)(CPUArchState *env, abi_ptr ptr,
>                                        RES_TYPE v)
>  {
> -#if !defined(CODE_ACCESS)
>      trace_guest_mem_before_exec(
>          env_cpu(env), ptr,
>          trace_mem_build_info(SHIFT, false, MO_TE, true));
> -#endif
>      glue(glue(st, SUFFIX), _p)(g2h(ptr), v);
>  }


--
Alex Bennée

[Qemu-devel] [PATCH 5/5] tcg: Release mmap_lock on translation fault

[Richard Henderson]

Turn helper_retaddr into a multi-state flag that may now also
indicate when we're performing a read on behalf of the translator.
In this case, release the mmap_lock before the longjmp back to
the main cpu loop, and thereby avoid a failing assert therein.

Fixes: https://bugs.launchpad.net/qemu/+bug/1832353
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/exec/cpu_ldst_useronly_template.h | 20 +++++--
 accel/tcg/user-exec.c                     | 65 ++++++++++++++++-------
 2 files changed, 62 insertions(+), 23 deletions(-)

diff --git a/include/exec/cpu_ldst_useronly_template.h b/include/exec/cpu_ldst_useronly_template.h
index d663826ac2..35caae8ca6 100644
--- a/include/exec/cpu_ldst_useronly_template.h
+++ b/include/exec/cpu_ldst_useronly_template.h
@@ -64,12 +64,18 @@
 static inline RES_TYPE
 glue(glue(cpu_ld, USUFFIX), MEMSUFFIX)(CPUArchState *env, abi_ptr ptr)
 {
-#if !defined(CODE_ACCESS)
+#ifdef CODE_ACCESS
+    RES_TYPE ret;
+    set_helper_retaddr(1);
+    ret = glue(glue(ld, USUFFIX), _p)(g2h(ptr));
+    clear_helper_retaddr();
+    return ret;
+#else
     trace_guest_mem_before_exec(
         env_cpu(env), ptr,
         trace_mem_build_info(SHIFT, false, MO_TE, false));
-#endif
     return glue(glue(ld, USUFFIX), _p)(g2h(ptr));
+#endif
 }
 
 #ifndef CODE_ACCESS
@@ -90,12 +96,18 @@ glue(glue(glue(cpu_ld, USUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
 static inline int
 glue(glue(cpu_lds, SUFFIX), MEMSUFFIX)(CPUArchState *env, abi_ptr ptr)
 {
-#if !defined(CODE_ACCESS)
+#ifdef CODE_ACCESS
+    int ret;
+    set_helper_retaddr(1);
+    ret = glue(glue(ld, USUFFIX), _p)(g2h(ptr));
+    clear_helper_retaddr();
+    return ret;
+#else
     trace_guest_mem_before_exec(
         env_cpu(env), ptr,
         trace_mem_build_info(SHIFT, true, MO_TE, false));
-#endif
     return glue(glue(lds, SUFFIX), _p)(g2h(ptr));
+#endif
 }
 
 #ifndef CODE_ACCESS
diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
index 4384b59a4d..5adea629de 100644
--- a/accel/tcg/user-exec.c
+++ b/accel/tcg/user-exec.c
@@ -64,27 +64,55 @@ static inline int handle_cpu_signal(uintptr_t pc, siginfo_t *info,
     CPUState *cpu = current_cpu;
     CPUClass *cc;
     unsigned long address = (unsigned long)info->si_addr;
-    MMUAccessType access_type;
+    MMUAccessType access_type = is_write ? MMU_DATA_STORE : MMU_DATA_LOAD;
 
-    /* We must handle PC addresses from two different sources:
-     * a call return address and a signal frame address.
-     *
-     * Within cpu_restore_state_from_tb we assume the former and adjust
-     * the address by -GETPC_ADJ so that the address is within the call
-     * insn so that addr does not accidentally match the beginning of the
-     * next guest insn.
-     *
-     * However, when the PC comes from the signal frame, it points to
-     * the actual faulting host insn and not a call insn.  Subtracting
-     * GETPC_ADJ in that case may accidentally match the previous guest insn.
-     *
-     * So for the later case, adjust forward to compensate for what
-     * will be done later by cpu_restore_state_from_tb.
-     */
-    if (helper_retaddr) {
+    switch (helper_retaddr) {
+    default:
+        /*
+         * Fault during host memory operation within a helper function.
+         * The helper's host return address, saved here, gives us a
+         * pointer into the generated code that will unwind to the
+         * correct guest pc.
+         */
         pc = helper_retaddr;
-    } else {
+        break;
+
+    case 0:
+        /*
+         * Fault during host memory operation within generated code.
+         * (Or, a unrelated bug within qemu, but we can't tell from here).
+         *
+         * We take the host pc from the signal frame.  However, we cannot
+         * use that value directly.  Within cpu_restore_state_from_tb, we
+         * assume PC comes from GETPC(), as used by the helper functions,
+         * so we adjust the address by -GETPC_ADJ to form an address that
+         * is within the call insn, so that the address does not accidentially
+         * match the beginning of the next guest insn.  However, when the
+         * pc comes fromt he signal frame it points to the actual faulting
+         * host memory insn and not a call insn.
+         *
+         * Therefore, adjust to compensate for what will be done later
+         * by cpu_restore_state_from_tb.
+         */
         pc += GETPC_ADJ;
+        break;
+
+    case 1:
+        /*
+         * Fault during host read for translation, or loosely, "execution".
+         * 
+         * The guest pc is already pointing to the start of the TB for which
+         * code is being generated.  If the guest translator manages the
+         * page crossings correctly, this is exactly the correct address
+         * (and if it doesn't there's little we can do about that here).
+         * Therefore, do not trigger the unwinder.
+         *
+         * Like tb_gen_code, release the memory lock before cpu_loop_exit.
+         */
+        pc = 0;
+        access_type = MMU_INST_FETCH;
+        mmap_unlock();
+        break;
     }
 
     /* For synchronous signals we expect to be coming from the vCPU
@@ -155,7 +183,6 @@ static inline int handle_cpu_signal(uintptr_t pc, siginfo_t *info,
     clear_helper_retaddr();
 
     cc = CPU_GET_CLASS(cpu);
-    access_type = is_write ? MMU_DATA_STORE : MMU_DATA_LOAD;
     cc->tlb_fill(cpu, address, 0, access_type, MMU_USER_IDX, false, pc);
     g_assert_not_reached();
 }
-- 
2.17.1

Re: [Qemu-devel] [PATCH 5/5] tcg: Release mmap_lock on translation fault

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> Turn helper_retaddr into a multi-state flag that may now also
> indicate when we're performing a read on behalf of the translator.
> In this case, release the mmap_lock before the longjmp back to
> the main cpu loop, and thereby avoid a failing assert therein.
>
> Fixes: https://bugs.launchpad.net/qemu/+bug/1832353
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

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

> ---
>  include/exec/cpu_ldst_useronly_template.h | 20 +++++--
>  accel/tcg/user-exec.c                     | 65 ++++++++++++++++-------
>  2 files changed, 62 insertions(+), 23 deletions(-)
>
> diff --git a/include/exec/cpu_ldst_useronly_template.h b/include/exec/cpu_ldst_useronly_template.h
> index d663826ac2..35caae8ca6 100644
> --- a/include/exec/cpu_ldst_useronly_template.h
> +++ b/include/exec/cpu_ldst_useronly_template.h
> @@ -64,12 +64,18 @@
>  static inline RES_TYPE
>  glue(glue(cpu_ld, USUFFIX), MEMSUFFIX)(CPUArchState *env, abi_ptr ptr)
>  {
> -#if !defined(CODE_ACCESS)
> +#ifdef CODE_ACCESS
> +    RES_TYPE ret;
> +    set_helper_retaddr(1);
> +    ret = glue(glue(ld, USUFFIX), _p)(g2h(ptr));
> +    clear_helper_retaddr();
> +    return ret;
> +#else
>      trace_guest_mem_before_exec(
>          env_cpu(env), ptr,
>          trace_mem_build_info(SHIFT, false, MO_TE, false));
> -#endif
>      return glue(glue(ld, USUFFIX), _p)(g2h(ptr));
> +#endif
>  }
>
>  #ifndef CODE_ACCESS
> @@ -90,12 +96,18 @@ glue(glue(glue(cpu_ld, USUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
>  static inline int
>  glue(glue(cpu_lds, SUFFIX), MEMSUFFIX)(CPUArchState *env, abi_ptr ptr)
>  {
> -#if !defined(CODE_ACCESS)
> +#ifdef CODE_ACCESS
> +    int ret;
> +    set_helper_retaddr(1);
> +    ret = glue(glue(ld, USUFFIX), _p)(g2h(ptr));
> +    clear_helper_retaddr();
> +    return ret;
> +#else
>      trace_guest_mem_before_exec(
>          env_cpu(env), ptr,
>          trace_mem_build_info(SHIFT, true, MO_TE, false));
> -#endif
>      return glue(glue(lds, SUFFIX), _p)(g2h(ptr));
> +#endif
>  }
>
>  #ifndef CODE_ACCESS
> diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
> index 4384b59a4d..5adea629de 100644
> --- a/accel/tcg/user-exec.c
> +++ b/accel/tcg/user-exec.c
> @@ -64,27 +64,55 @@ static inline int handle_cpu_signal(uintptr_t pc, siginfo_t *info,
>      CPUState *cpu = current_cpu;
>      CPUClass *cc;
>      unsigned long address = (unsigned long)info->si_addr;
> -    MMUAccessType access_type;
> +    MMUAccessType access_type = is_write ? MMU_DATA_STORE : MMU_DATA_LOAD;
>
> -    /* We must handle PC addresses from two different sources:
> -     * a call return address and a signal frame address.
> -     *
> -     * Within cpu_restore_state_from_tb we assume the former and adjust
> -     * the address by -GETPC_ADJ so that the address is within the call
> -     * insn so that addr does not accidentally match the beginning of the
> -     * next guest insn.
> -     *
> -     * However, when the PC comes from the signal frame, it points to
> -     * the actual faulting host insn and not a call insn.  Subtracting
> -     * GETPC_ADJ in that case may accidentally match the previous guest insn.
> -     *
> -     * So for the later case, adjust forward to compensate for what
> -     * will be done later by cpu_restore_state_from_tb.
> -     */
> -    if (helper_retaddr) {
> +    switch (helper_retaddr) {
> +    default:
> +        /*
> +         * Fault during host memory operation within a helper function.
> +         * The helper's host return address, saved here, gives us a
> +         * pointer into the generated code that will unwind to the
> +         * correct guest pc.
> +         */
>          pc = helper_retaddr;
> -    } else {
> +        break;
> +
> +    case 0:
> +        /*
> +         * Fault during host memory operation within generated code.
> +         * (Or, a unrelated bug within qemu, but we can't tell from here).
> +         *
> +         * We take the host pc from the signal frame.  However, we cannot
> +         * use that value directly.  Within cpu_restore_state_from_tb, we
> +         * assume PC comes from GETPC(), as used by the helper functions,
> +         * so we adjust the address by -GETPC_ADJ to form an address that
> +         * is within the call insn, so that the address does not accidentially
> +         * match the beginning of the next guest insn.  However, when the
> +         * pc comes fromt he signal frame it points to the actual faulting
> +         * host memory insn and not a call insn.
> +         *
> +         * Therefore, adjust to compensate for what will be done later
> +         * by cpu_restore_state_from_tb.
> +         */
>          pc += GETPC_ADJ;
> +        break;
> +
> +    case 1:
> +        /*
> +         * Fault during host read for translation, or loosely, "execution".
> +         *
> +         * The guest pc is already pointing to the start of the TB for which
> +         * code is being generated.  If the guest translator manages the
> +         * page crossings correctly, this is exactly the correct address
> +         * (and if it doesn't there's little we can do about that here).
> +         * Therefore, do not trigger the unwinder.
> +         *
> +         * Like tb_gen_code, release the memory lock before cpu_loop_exit.
> +         */
> +        pc = 0;
> +        access_type = MMU_INST_FETCH;
> +        mmap_unlock();
> +        break;
>      }
>
>      /* For synchronous signals we expect to be coming from the vCPU
> @@ -155,7 +183,6 @@ static inline int handle_cpu_signal(uintptr_t pc, siginfo_t *info,
>      clear_helper_retaddr();
>
>      cc = CPU_GET_CLASS(cpu);
> -    access_type = is_write ? MMU_DATA_STORE : MMU_DATA_LOAD;
>      cc->tlb_fill(cpu, address, 0, access_type, MMU_USER_IDX, false, pc);
>      g_assert_not_reached();
>  }


--
Alex Bennée

Re: [Qemu-devel] [PATCH 0/5] tcg: Fix mmap_lock assertion failure, take 2

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190709092049.13771-1-richard.henderson@linaro.org/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 20190709092049.13771-1-richard.henderson@linaro.org
Subject: [Qemu-devel] [PATCH 0/5] tcg: Fix mmap_lock assertion failure, take 2
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

From https://github.com/patchew-project/qemu
 - [tag update]      patchew/20190709092049.13771-1-richard.henderson@linaro.org -> patchew/20190709092049.13771-1-richard.henderson@linaro.org
Switched to a new branch 'test'
8e9e968 tcg: Release mmap_lock on translation fault
985478b tcg: Remove duplicate #if !defined(CODE_ACCESS)
34999be tcg: Remove cpu_ld*_code_ra
8fb8bbc tcg: Introduce set/clear_helper_retaddr
e581a29 include/qemu/atomic.h: Add signal_barrier

=== OUTPUT BEGIN ===
1/5 Checking commit e581a29ee379 (include/qemu/atomic.h: Add signal_barrier)
2/5 Checking commit 8fb8bbc8fad6 (tcg: Introduce set/clear_helper_retaddr)
3/5 Checking commit 34999be96896 (tcg: Remove cpu_ld*_code_ra)
4/5 Checking commit 985478b1b086 (tcg: Remove duplicate #if !defined(CODE_ACCESS))
5/5 Checking commit 8e9e9683a62a (tcg: Release mmap_lock on translation fault)
ERROR: trailing whitespace
#78: FILE: accel/tcg/user-exec.c:103:
+         * $

total: 1 errors, 0 warnings, 120 lines checked

Patch 5/5 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20190709092049.13771-1-richard.henderson@linaro.org/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com