Re: [Qemu-devel] [PATCH] tcg: Merge GETPC and GETRA

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Tue, 2016-07-26 at 06:12 +0530, Richard Henderson wrote:
> The return address argument to the softmmu template helpers was
> confused.  In the legacy case, we wanted to indicate that there
> is no return address, and so passed in NULL.  However, we then
> immediately subtracted GETPC_ADJ from NULL, resulting in a non-zero
> value, indicating the presence of an (invalid) return address.
> 
> Push the GETPC_ADJ subtraction down to the only point it's required:
> immediately before use within cpu_restore_state, after all NULL
> pointer
> checks have been completed.  This makes GETPC and GETRA identical.
> 
> Remove GETRA as the lesser used macro, replacing all uses with GETPC.
> 
> Signed-off-by: Richard Henderson <rth@twiddle.net>
> ---
> 
> Ben, this should fix the "-2" problem that you reported.  Of course,
> as also discussed in that thread, this won't fix the whole issue.

Thanks. I've fixed the rest of the issue here:

https://github.com/ozbenh/qemu/commit/5b84a90aa696a7072eaa7a2f4cf6ade7cdd10358

While I will send to the list later today hopefully, along with the
rest of my cleanups and fixes that are starting to pile up here:

https://github.com/ozbenh/qemu/commits/wip

I haven't tackled the FP exceptions yet though but that can wait.

Cheers,
Ben.

> r~
> 
> ---
>  cputlb.c                |  6 ++----
>  include/exec/exec-all.h |  9 +++------
>  softmmu_template.h      | 32 ++++++--------------------------
>  target-arm/helper.c     |  6 +++---
>  target-mips/op_helper.c | 18 +++++++++---------
>  translate-all.c         |  1 +
>  6 files changed, 24 insertions(+), 48 deletions(-)
> 
> diff --git a/cputlb.c b/cputlb.c
> index d068ee5..3c99c34 100644
> --- a/cputlb.c
> +++ b/cputlb.c
> @@ -543,10 +543,8 @@ static bool victim_tlb_hit(CPUArchState *env,
> size_t mmu_idx, size_t index,
>  #undef MMUSUFFIX
>  
>  #define MMUSUFFIX _cmmu
> -#undef GETPC_ADJ
> -#define GETPC_ADJ 0
> -#undef GETRA
> -#define GETRA() ((uintptr_t)0)
> +#undef GETPC
> +#define GETPC() ((uintptr_t)0)
>  #define SOFTMMU_CODE_ACCESS
>  
>  #define SHIFT 0
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index acda7b6..6a317db 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -335,13 +335,12 @@ static inline void tb_add_jump(TranslationBlock
> *tb, int n,
>      tb_next->jmp_list_first = (uintptr_t)tb | n;
>  }
>  
> -/* GETRA is the true target of the return instruction that we'll
> execute,
> -   defined here for simplicity of defining the follow-up macros.  */
> +/* GETPC is the true target of the return instruction that we'll
> execute.  */
>  #if defined(CONFIG_TCG_INTERPRETER)
>  extern uintptr_t tci_tb_ptr;
> -# define GETRA() tci_tb_ptr
> +# define GETPC() tci_tb_ptr
>  #else
> -# define GETRA() \
> +# define GETPC() \
>      ((uintptr_t)__builtin_extract_return_addr(__builtin_return_addre
> ss(0)))
>  #endif
>  
> @@ -354,8 +353,6 @@ extern uintptr_t tci_tb_ptr;
>     smaller than 4 bytes, so we don't worry about special-casing
> this.  */
>  #define GETPC_ADJ   2
>  
> -#define GETPC()  (GETRA() - GETPC_ADJ)
> -
>  #if !defined(CONFIG_USER_ONLY)
>  
>  struct MemoryRegion *iotlb_to_region(CPUState *cpu,
> diff --git a/softmmu_template.h b/softmmu_template.h
> index 284ab2c..4cbb714 100644
> --- a/softmmu_template.h
> +++ b/softmmu_template.h
> @@ -150,9 +150,6 @@ WORD_TYPE helper_le_ld_name(CPUArchState *env,
> target_ulong addr,
>      uintptr_t haddr;
>      DATA_TYPE res;
>  
> -    /* Adjust the given return address.  */
> -    retaddr -= GETPC_ADJ;
> -
>      if (a_bits > 0 && (addr & ((1 << a_bits) - 1)) != 0) {
>          cpu_unaligned_access(ENV_GET_CPU(env), addr,
> READ_ACCESS_TYPE,
>                               mmu_idx, retaddr);
> @@ -193,10 +190,8 @@ WORD_TYPE helper_le_ld_name(CPUArchState *env,
> target_ulong addr,
>      do_unaligned_access:
>          addr1 = addr & ~(DATA_SIZE - 1);
>          addr2 = addr1 + DATA_SIZE;
> -        /* Note the adjustment at the beginning of the function.
> -           Undo that for the recursion.  */
> -        res1 = helper_le_ld_name(env, addr1, oi, retaddr +
> GETPC_ADJ);
> -        res2 = helper_le_ld_name(env, addr2, oi, retaddr +
> GETPC_ADJ);
> +        res1 = helper_le_ld_name(env, addr1, oi, retaddr);
> +        res2 = helper_le_ld_name(env, addr2, oi, retaddr);
>          shift = (addr & (DATA_SIZE - 1)) * 8;
>  
>          /* Little-endian combine.  */
> @@ -224,9 +219,6 @@ WORD_TYPE helper_be_ld_name(CPUArchState *env,
> target_ulong addr,
>      uintptr_t haddr;
>      DATA_TYPE res;
>  
> -    /* Adjust the given return address.  */
> -    retaddr -= GETPC_ADJ;
> -
>      if (a_bits > 0 && (addr & ((1 << a_bits) - 1)) != 0) {
>          cpu_unaligned_access(ENV_GET_CPU(env), addr,
> READ_ACCESS_TYPE,
>                               mmu_idx, retaddr);
> @@ -267,10 +259,8 @@ WORD_TYPE helper_be_ld_name(CPUArchState *env,
> target_ulong addr,
>      do_unaligned_access:
>          addr1 = addr & ~(DATA_SIZE - 1);
>          addr2 = addr1 + DATA_SIZE;
> -        /* Note the adjustment at the beginning of the function.
> -           Undo that for the recursion.  */
> -        res1 = helper_be_ld_name(env, addr1, oi, retaddr +
> GETPC_ADJ);
> -        res2 = helper_be_ld_name(env, addr2, oi, retaddr +
> GETPC_ADJ);
> +        res1 = helper_be_ld_name(env, addr1, oi, retaddr);
> +        res2 = helper_be_ld_name(env, addr2, oi, retaddr);
>          shift = (addr & (DATA_SIZE - 1)) * 8;
>  
>          /* Big-endian combine.  */
> @@ -334,9 +324,6 @@ void helper_le_st_name(CPUArchState *env,
> target_ulong addr, DATA_TYPE val,
>      int a_bits = get_alignment_bits(get_memop(oi));
>      uintptr_t haddr;
>  
> -    /* Adjust the given return address.  */
> -    retaddr -= GETPC_ADJ;
> -
>      if (a_bits > 0 && (addr & ((1 << a_bits) - 1)) != 0) {
>          cpu_unaligned_access(ENV_GET_CPU(env), addr, MMU_DATA_STORE,
>                               mmu_idx, retaddr);
> @@ -391,10 +378,8 @@ void helper_le_st_name(CPUArchState *env,
> target_ulong addr, DATA_TYPE val,
>          for (i = 0; i < DATA_SIZE; ++i) {
>              /* Little-endian extract.  */
>              uint8_t val8 = val >> (i * 8);
> -            /* Note the adjustment at the beginning of the function.
> -               Undo that for the recursion.  */
>              glue(helper_ret_stb, MMUSUFFIX)(env, addr + i, val8,
> -                                            oi, retaddr +
> GETPC_ADJ);
> +                                            oi, retaddr);
>          }
>          return;
>      }
> @@ -417,9 +402,6 @@ void helper_be_st_name(CPUArchState *env,
> target_ulong addr, DATA_TYPE val,
>      int a_bits = get_alignment_bits(get_memop(oi));
>      uintptr_t haddr;
>  
> -    /* Adjust the given return address.  */
> -    retaddr -= GETPC_ADJ;
> -
>      if (a_bits > 0 && (addr & ((1 << a_bits) - 1)) != 0) {
>          cpu_unaligned_access(ENV_GET_CPU(env), addr, MMU_DATA_STORE,
>                               mmu_idx, retaddr);
> @@ -474,10 +456,8 @@ void helper_be_st_name(CPUArchState *env,
> target_ulong addr, DATA_TYPE val,
>          for (i = 0; i < DATA_SIZE; ++i) {
>              /* Big-endian extract.  */
>              uint8_t val8 = val >> (((DATA_SIZE - 1) * 8) - (i * 8));
> -            /* Note the adjustment at the beginning of the function.
> -               Undo that for the recursion.  */
>              glue(helper_ret_stb, MMUSUFFIX)(env, addr + i, val8,
> -                                            oi, retaddr +
> GETPC_ADJ);
> +                                            oi, retaddr);
>          }
>          return;
>      }
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index bdb842c..915fe0f 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -8310,12 +8310,12 @@ void HELPER(dc_zva)(CPUARMState *env,
> uint64_t vaddr_in)
>               * this purpose use the actual register value passed to
> us
>               * so that we get the fault address right.
>               */
> -            helper_ret_stb_mmu(env, vaddr_in, 0, oi, GETRA());
> +            helper_ret_stb_mmu(env, vaddr_in, 0, oi, GETPC());
>              /* Now we can populate the other TLB entries, if any */
>              for (i = 0; i < maxidx; i++) {
>                  uint64_t va = vaddr + TARGET_PAGE_SIZE * i;
>                  if (va != (vaddr_in & TARGET_PAGE_MASK)) {
> -                    helper_ret_stb_mmu(env, va, 0, oi, GETRA());
> +                    helper_ret_stb_mmu(env, va, 0, oi, GETPC());
>                  }
>              }
>          }
> @@ -8332,7 +8332,7 @@ void HELPER(dc_zva)(CPUARMState *env, uint64_t
> vaddr_in)
>           *    bounce buffer was in use
>           */
>          for (i = 0; i < blocklen; i++) {
> -            helper_ret_stb_mmu(env, vaddr + i, 0, oi, GETRA());
> +            helper_ret_stb_mmu(env, vaddr + i, 0, oi, GETPC());
>          }
>      }
>  #else
> diff --git a/target-mips/op_helper.c b/target-mips/op_helper.c
> index ea2f2ab..7af4c2f 100644
> --- a/target-mips/op_helper.c
> +++ b/target-mips/op_helper.c
> @@ -4122,10 +4122,10 @@ void helper_msa_ld_ ## TYPE(CPUMIPSState
> *env, uint32_t wd,             \
>  }
>  
>  #if !defined(CONFIG_USER_ONLY)
> -MSA_LD_DF(DF_BYTE,   b, helper_ret_ldub_mmu, oi, GETRA())
> -MSA_LD_DF(DF_HALF,   h, helper_ret_lduw_mmu, oi, GETRA())
> -MSA_LD_DF(DF_WORD,   w, helper_ret_ldul_mmu, oi, GETRA())
> -MSA_LD_DF(DF_DOUBLE, d, helper_ret_ldq_mmu,  oi, GETRA())
> +MSA_LD_DF(DF_BYTE,   b, helper_ret_ldub_mmu, oi, GETPC())
> +MSA_LD_DF(DF_HALF,   h, helper_ret_lduw_mmu, oi, GETPC())
> +MSA_LD_DF(DF_WORD,   w, helper_ret_ldul_mmu, oi, GETPC())
> +MSA_LD_DF(DF_DOUBLE, d, helper_ret_ldq_mmu,  oi, GETPC())
>  #else
>  MSA_LD_DF(DF_BYTE,   b, cpu_ldub_data)
>  MSA_LD_DF(DF_HALF,   h, cpu_lduw_data)
> @@ -4161,17 +4161,17 @@ void helper_msa_st_ ## TYPE(CPUMIPSState
> *env, uint32_t wd,             \
>      int mmu_idx = cpu_mmu_index(env, false);				
> \
>      int
> i;                                                              \
>      MEMOP_IDX(DF)                                                   
>     \
> -    ensure_writable_pages(env, addr, mmu_idx,
> GETRA());                 \
> +    ensure_writable_pages(env, addr, mmu_idx,
> GETPC());                 \
>      for (i = 0; i < DF_ELEMENTS(DF); i++)
> {                             \
>          ST_INSN(env, addr + (i << DF), pwd->TYPE[i],
> ##__VA_ARGS__);    \
>      }                                                               
>     \
>  }
>  
>  #if !defined(CONFIG_USER_ONLY)
> -MSA_ST_DF(DF_BYTE,   b, helper_ret_stb_mmu, oi, GETRA())
> -MSA_ST_DF(DF_HALF,   h, helper_ret_stw_mmu, oi, GETRA())
> -MSA_ST_DF(DF_WORD,   w, helper_ret_stl_mmu, oi, GETRA())
> -MSA_ST_DF(DF_DOUBLE, d, helper_ret_stq_mmu, oi, GETRA())
> +MSA_ST_DF(DF_BYTE,   b, helper_ret_stb_mmu, oi, GETPC())
> +MSA_ST_DF(DF_HALF,   h, helper_ret_stw_mmu, oi, GETPC())
> +MSA_ST_DF(DF_WORD,   w, helper_ret_stl_mmu, oi, GETPC())
> +MSA_ST_DF(DF_DOUBLE, d, helper_ret_stq_mmu, oi, GETPC())
>  #else
>  MSA_ST_DF(DF_BYTE,   b, cpu_stb_data)
>  MSA_ST_DF(DF_HALF,   h, cpu_stw_data)
> diff --git a/translate-all.c b/translate-all.c
> index 0d47c1c..644d081 100644
> --- a/translate-all.c
> +++ b/translate-all.c
> @@ -299,6 +299,7 @@ bool cpu_restore_state(CPUState *cpu, uintptr_t
> retaddr)
>  {
>      TranslationBlock *tb;
>  
> +    retaddr -= GETPC_ADJ;
>      tb = tb_find_pc(retaddr);
>      if (tb) {
>          cpu_restore_state_from_tb(cpu, tb, retaddr);