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

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

[Richard Henderson]

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.


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_address(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);
-- 
2.7.4

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

[Benjamin Herrenschmidt]

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

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

[Leon Alrae]

On Tue, Jul 26, 2016 at 06:12:40AM +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.
> 
> 
> 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(-)

Looks good to me:

Reviewed-by: Leon Alrae <leon.alrae@imgtec.com>

Thanks,
Leon

[Qemu-devel] [PATCH] ppc: Fix fault PC reporting for lve*/stve* VMX instructions

[Benjamin Herrenschmidt]

We forgot to do gen_update_nip() for these like we do with other
helpers. Fix this, but in a more efficient way by passing the RA
to the accessors instead so the overhead is only taken on faults.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 target-ppc/mem_helper.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/target-ppc/mem_helper.c b/target-ppc/mem_helper.c
index e4de86b..e4ed377 100644
--- a/target-ppc/mem_helper.c
+++ b/target-ppc/mem_helper.c
@@ -232,16 +232,16 @@ target_ulong helper_lscbx(CPUPPCState *env, target_ulong addr, uint32_t reg,
                                                                 \
         if (needs_byteswap(env)) {                              \
             r->element[LO_IDX ? index : (adjust - index)] =     \
-                swap(access(env, addr));                        \
+                swap(access(env, addr, GETPC()));               \
         } else {                                                \
             r->element[LO_IDX ? index : (adjust - index)] =     \
-                access(env, addr);                              \
+                access(env, addr, GETPC());                     \
         }                                                       \
     }
 #define I(x) (x)
-LVE(lvebx, cpu_ldub_data, I, u8)
-LVE(lvehx, cpu_lduw_data, bswap16, u16)
-LVE(lvewx, cpu_ldl_data, bswap32, u32)
+LVE(lvebx, cpu_ldub_data_ra, I, u8)
+LVE(lvehx, cpu_lduw_data_ra, bswap16, u16)
+LVE(lvewx, cpu_ldl_data_ra, bswap32, u32)
 #undef I
 #undef LVE
 
@@ -259,16 +259,17 @@ LVE(lvewx, cpu_ldl_data, bswap32, u32)
                                                                         \
         if (needs_byteswap(env)) {                                      \
             access(env, addr, swap(r->element[LO_IDX ? index :          \
-                                              (adjust - index)]));      \
+                                              (adjust - index)]),       \
+                        GETPC());                                       \
         } else {                                                        \
             access(env, addr, r->element[LO_IDX ? index :               \
-                                         (adjust - index)]);            \
+                                         (adjust - index)], GETPC());   \
         }                                                               \
     }
 #define I(x) (x)
-STVE(stvebx, cpu_stb_data, I, u8)
-STVE(stvehx, cpu_stw_data, bswap16, u16)
-STVE(stvewx, cpu_stl_data, bswap32, u32)
+STVE(stvebx, cpu_stb_data_ra, I, u8)
+STVE(stvehx, cpu_stw_data_ra, bswap16, u16)
+STVE(stvewx, cpu_stl_data_ra, bswap32, u32)
 #undef I
 #undef LVE
 

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

[Benjamin Herrenschmidt]

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.

I haven't had a chance to review the details of the patch but I
verified that all my test OSes still boot with it applied ;-)

Cheers,
Ben.

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

[Benjamin Herrenschmidt]

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

This causes a problem with linux-user. We have cases where a store
instruction from the guest turns into *one* store instruction in
the host. The PC we get from the host segfault is exact, so the -2
in cpu_restore_state() takes us to the previous instruction.

We thus end up reporting the wrong instruction in the signal

I've tentatively fixed it with the hack below. It's not pretty
since we were trying to get rid of all those GETPC_ADJ sprinkled
all over but I don't see any obvious better way.

I'm not sure if there are other cases with softmmu for example, where
we do get an exact host PC and thus where the -2 might take us back too
far.

I don't see your patch upstream yet so feel free to fold my change into
yours.

@@ -103,13 +102,16 @@ static inline int handle_cpu_signal(uintptr_t pc, unsigned long address,
         return 0; /* not an MMU fault */
     }
     if (ret == 0) {
         return 1; /* the MMU fault was handled without causing real CPU fault */
     }
-    /* now we have a real cpu fault */
-    cpu_restore_state(cpu, pc);
-
+    /* Now we have a real cpu fault. We must undo the adjustment
+     * done by cpu_restore_state() since we aren't pointing to the
+     * next instruction but to the faulting one and going back
+     * can make us report the wrong guest PC.
+     */
+    cpu_restore_state(cpu, pc + GETPC_ADJ);
     sigprocmask(SIG_SETMASK, old_set, NULL);
     cpu_loop_exit(cpu);
 
     /* never comes here */
     return 1;

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

[Richard Henderson]

On 08/03/2016 01:12 PM, Benjamin Herrenschmidt wrote:
> I'm not sure if there are other cases with softmmu for example, where
> we do get an exact host PC and thus where the -2 might take us back too
> far.

I don't think there are.  Almost everywhere we have the call return address. 
It's only this case of signal handling where we have an exception address.

> I don't see your patch upstream yet so feel free to fold my change into
> yours.

Will do, thanks.  I think this will be waiting for 2.8.


r~