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

[Richard Henderson <richard.henderson@linaro.org>]

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~