[Qemu-devel] [PATCH v2 0/2] fixing translation time segfaults

[Qemu-devel] [PATCH v2 0/2] fixing translation time segfaults

[Alex Bennée]

Hi,

This is a follow up to the patch I posted yesterday. I've made some
clean-ups to the comments and logic as suggested from the review.
Additionally I've added a clean-up patch which tidied up all the
call-sites which no longer need to check retaddr/pc before calling
cpu_restore_state.

Alex Bennée (2):
  accel/tcg/translate-all: expand cpu_restore_state addr check
  target/*helper: don't check retaddr before calling cpu_restore_state

 accel/tcg/translate-all.c                  | 52 +++++++++++++++++-------------
 include/exec/exec-all.h                    | 11 +++++++
 scripts/coccinelle/cpu_restore_state.cocci | 12 +++++++
 target/alpha/mem_helper.c                  | 12 ++-----
 target/arm/op_helper.c                     | 17 +++-------
 target/i386/svm_helper.c                   |  4 +--
 target/lm32/op_helper.c                    |  7 ++--
 target/m68k/op_helper.c                    |  7 ++--
 target/microblaze/op_helper.c              |  7 ++--
 target/moxie/helper.c                      |  4 +--
 target/nios2/mmu.c                         |  7 ++--
 target/openrisc/mmu_helper.c               |  7 ++--
 target/s390x/excp_helper.c                 |  4 +--
 target/tricore/op_helper.c                 | 11 ++-----
 target/unicore32/op_helper.c               |  7 ++--
 15 files changed, 77 insertions(+), 92 deletions(-)
 create mode 100644 scripts/coccinelle/cpu_restore_state.cocci

-- 
2.14.2

[Qemu-devel] [PATCH v2 1/2] accel/tcg/translate-all: expand cpu_restore_state addr check

[Alex Bennée]

We are still seeing signals during translation time when we walk over
a page protection boundary. This expands the check to ensure the host
PC is inside the code generation buffer. The original suggestion was
to check versus tcg_ctx.code_gen_ptr but as we now segment the
translation buffer we have to settle for just a general check for
being inside.

I've also fixed up the declaration to make it clear it can deal with
invalid addresses. A later patch will fix up the call sites.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reported-by: Peter Maydell <peter.maydell@linaro.org>
Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Cc: Richard Henderson <rth@twiddle.net>

---
v2:
  - add doc comment to exec-all.h
  - retaddr->host_pc
  - re-word comments on host_pc
  - simplify logic as per rth suggestion
---
 accel/tcg/translate-all.c | 52 ++++++++++++++++++++++++++---------------------
 include/exec/exec-all.h   | 11 ++++++++++
 2 files changed, 40 insertions(+), 23 deletions(-)

diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index 34c5e28d07..e7f0329a52 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -352,36 +352,42 @@ static int cpu_restore_state_from_tb(CPUState *cpu, TranslationBlock *tb,
     return 0;
 }
 
-bool cpu_restore_state(CPUState *cpu, uintptr_t retaddr)
+bool cpu_restore_state(CPUState *cpu, uintptr_t host_pc)
 {
     TranslationBlock *tb;
     bool r = false;
+    uintptr_t check_offset;
 
-    /* A retaddr of zero is invalid so we really shouldn't have ended
-     * up here. The target code has likely forgotten to check retaddr
-     * != 0 before attempting to restore state. We return early to
-     * avoid blowing up on a recursive tb_lock(). The target must have
-     * previously survived a failed cpu_restore_state because
-     * tb_find_pc(0) would have failed anyway. It still should be
-     * fixed though.
+    /* The host_pc has to be in the region of current code buffer. If
+     * it is not we will not be able to resolve it here. The two cases
+     * where host_pc will not be correct are:
+     *
+     *  - fault during translation (instruction fetch)
+     *  - fault from helper (not using GETPC() macro)
+     *
+     * Either way we need return early to avoid blowing up on a
+     * recursive tb_lock() as we can't resolve it here.
+     *
+     * We are using unsigned arithmetic so if host_pc <
+     * tcg_init_ctx.code_gen_buffer check_offset will wrap to way
+     * above the code_gen_buffer_size
      */
-
-    if (!retaddr) {
-        return r;
-    }
-
-    tb_lock();
-    tb = tb_find_pc(retaddr);
-    if (tb) {
-        cpu_restore_state_from_tb(cpu, tb, retaddr);
-        if (tb->cflags & CF_NOCACHE) {
-            /* one-shot translation, invalidate it immediately */
-            tb_phys_invalidate(tb, -1);
-            tb_remove(tb);
+    check_offset = host_pc - (uintptr_t) tcg_init_ctx.code_gen_buffer;
+
+    if (check_offset < tcg_init_ctx.code_gen_buffer_size) {
+        tb_lock();
+        tb = tb_find_pc(host_pc);
+        if (tb) {
+            cpu_restore_state_from_tb(cpu, tb, host_pc);
+            if (tb->cflags & CF_NOCACHE) {
+                /* one-shot translation, invalidate it immediately */
+                tb_phys_invalidate(tb, -1);
+                tb_remove(tb);
+            }
+            r = true;
         }
-        r = true;
+        tb_unlock();
     }
-    tb_unlock();
 
     return r;
 }
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index 923ece3e9b..0f51c92adb 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -45,6 +45,17 @@ void restore_state_to_opc(CPUArchState *env, struct TranslationBlock *tb,
                           target_ulong *data);
 
 void cpu_gen_init(void);
+
+/**
+ * cpu_restore_state:
+ * @cpu: the vCPU state is to be restore to
+ * @searched_pc: the host PC the fault occurred at
+ * @return: true if state was restored, false otherwise
+ *
+ * Attempt to restore the state for a fault occurring in translated
+ * code. If the searched_pc is not in translated code no state is
+ * restored and the function returns false.
+ */
 bool cpu_restore_state(CPUState *cpu, uintptr_t searched_pc);
 
 void QEMU_NORETURN cpu_loop_exit_noexc(CPUState *cpu);
-- 
2.14.2

Re: [Qemu-devel] [PATCH v2 1/2] accel/tcg/translate-all: expand cpu_restore_state addr check

[Laurent Vivier]

Le 08/11/2017 à 16:32, Alex Bennée a écrit :
> We are still seeing signals during translation time when we walk over
> a page protection boundary. This expands the check to ensure the host
> PC is inside the code generation buffer. The original suggestion was
> to check versus tcg_ctx.code_gen_ptr but as we now segment the
> translation buffer we have to settle for just a general check for
> being inside.
> 
> I've also fixed up the declaration to make it clear it can deal with
> invalid addresses. A later patch will fix up the call sites.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Richard Henderson <rth@twiddle.net>
> 
> ---
> v2:
>   - add doc comment to exec-all.h
>   - retaddr->host_pc
>   - re-word comments on host_pc
>   - simplify logic as per rth suggestion
> ---
>  accel/tcg/translate-all.c | 52 ++++++++++++++++++++++++++---------------------
>  include/exec/exec-all.h   | 11 ++++++++++
>  2 files changed, 40 insertions(+), 23 deletions(-)
> 
> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> index 34c5e28d07..e7f0329a52 100644
> --- a/accel/tcg/translate-all.c
> +++ b/accel/tcg/translate-all.c
> @@ -352,36 +352,42 @@ static int cpu_restore_state_from_tb(CPUState *cpu, TranslationBlock *tb,
>      return 0;
>  }
>  
> -bool cpu_restore_state(CPUState *cpu, uintptr_t retaddr)
> +bool cpu_restore_state(CPUState *cpu, uintptr_t host_pc)
>  {
>      TranslationBlock *tb;
>      bool r = false;
> +    uintptr_t check_offset;
>  
> -    /* A retaddr of zero is invalid so we really shouldn't have ended
> -     * up here. The target code has likely forgotten to check retaddr
> -     * != 0 before attempting to restore state. We return early to
> -     * avoid blowing up on a recursive tb_lock(). The target must have
> -     * previously survived a failed cpu_restore_state because
> -     * tb_find_pc(0) would have failed anyway. It still should be
> -     * fixed though.
> +    /* The host_pc has to be in the region of current code buffer. If
> +     * it is not we will not be able to resolve it here. The two cases
> +     * where host_pc will not be correct are:
> +     *
> +     *  - fault during translation (instruction fetch)
> +     *  - fault from helper (not using GETPC() macro)
> +     *
> +     * Either way we need return early to avoid blowing up on a
> +     * recursive tb_lock() as we can't resolve it here.
> +     *
> +     * We are using unsigned arithmetic so if host_pc <
> +     * tcg_init_ctx.code_gen_buffer check_offset will wrap to way
> +     * above the code_gen_buffer_size
>       */
> -
> -    if (!retaddr) {
> -        return r;
> -    }
> -
> -    tb_lock();
> -    tb = tb_find_pc(retaddr);
> -    if (tb) {
> -        cpu_restore_state_from_tb(cpu, tb, retaddr);
> -        if (tb->cflags & CF_NOCACHE) {
> -            /* one-shot translation, invalidate it immediately */
> -            tb_phys_invalidate(tb, -1);
> -            tb_remove(tb);
> +    check_offset = host_pc - (uintptr_t) tcg_init_ctx.code_gen_buffer;
> +
> +    if (check_offset < tcg_init_ctx.code_gen_buffer_size) {
> +        tb_lock();
> +        tb = tb_find_pc(host_pc);
> +        if (tb) {
> +            cpu_restore_state_from_tb(cpu, tb, host_pc);
> +            if (tb->cflags & CF_NOCACHE) {
> +                /* one-shot translation, invalidate it immediately */
> +                tb_phys_invalidate(tb, -1);
> +                tb_remove(tb);
> +            }
> +            r = true;
>          }
> -        r = true;
> +        tb_unlock();
>      }
> -    tb_unlock();
>  
>      return r;
>  }
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index 923ece3e9b..0f51c92adb 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -45,6 +45,17 @@ void restore_state_to_opc(CPUArchState *env, struct TranslationBlock *tb,
>                            target_ulong *data);
>  
>  void cpu_gen_init(void);
> +
> +/**
> + * cpu_restore_state:
> + * @cpu: the vCPU state is to be restore to
> + * @searched_pc: the host PC the fault occurred at
> + * @return: true if state was restored, false otherwise
> + *
> + * Attempt to restore the state for a fault occurring in translated
> + * code. If the searched_pc is not in translated code no state is
> + * restored and the function returns false.
> + */
>  bool cpu_restore_state(CPUState *cpu, uintptr_t searched_pc);
>  
>  void QEMU_NORETURN cpu_loop_exit_noexc(CPUState *cpu);
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [Qemu-devel] [PATCH v2 1/2] accel/tcg/translate-all: expand cpu_restore_state addr check

[Richard Henderson]

On 11/08/2017 04:32 PM, Alex Bennée wrote:
> We are still seeing signals during translation time when we walk over
> a page protection boundary. This expands the check to ensure the host
> PC is inside the code generation buffer. The original suggestion was
> to check versus tcg_ctx.code_gen_ptr but as we now segment the
> translation buffer we have to settle for just a general check for
> being inside.
> 
> I've also fixed up the declaration to make it clear it can deal with
> invalid addresses. A later patch will fix up the call sites.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Richard Henderson <rth@twiddle.net>

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH v2 1/2] accel/tcg/translate-all: expand cpu_restore_state addr check

[Peter Maydell]

On 8 November 2017 at 15:32, Alex Bennée <alex.bennee@linaro.org> wrote:
> We are still seeing signals during translation time when we walk over
> a page protection boundary. This expands the check to ensure the host
> PC is inside the code generation buffer. The original suggestion was
> to check versus tcg_ctx.code_gen_ptr but as we now segment the
> translation buffer we have to settle for just a general check for
> being inside.
>
> I've also fixed up the declaration to make it clear it can deal with
> invalid addresses. A later patch will fix up the call sites.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Richard Henderson <rth@twiddle.net>

Thanks; this fixes my test case. Patch 2 is just cleanup and looks
like it needs rework, so I'm taking patch 1 into target-arm
to put into master for rc1.

thanks
-- PMM

[Qemu-devel] [PATCH v2 2/2] target/*helper: don't check retaddr before calling cpu_restore_state

[Alex Bennée]

cpu_restore_state officially supports being passed an address it can't
resolve the state for. As a result the checks in the helpers are
superfluous and can be removed. This makes the code consistent with
other users of cpu_restore_state.

Of course this does nothing to address what to do if cpu_restore_state
can't resolve the state but so far it seems this is handled elsewhere.

The change was made with included coccinelle script.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 scripts/coccinelle/cpu_restore_state.cocci | 12 ++++++++++++
 target/alpha/mem_helper.c                  | 12 +++---------
 target/arm/op_helper.c                     | 17 ++++-------------
 target/i386/svm_helper.c                   |  4 +---
 target/lm32/op_helper.c                    |  7 ++-----
 target/m68k/op_helper.c                    |  7 ++-----
 target/microblaze/op_helper.c              |  7 ++-----
 target/moxie/helper.c                      |  4 +---
 target/nios2/mmu.c                         |  7 ++-----
 target/openrisc/mmu_helper.c               |  7 ++-----
 target/s390x/excp_helper.c                 |  4 +---
 target/tricore/op_helper.c                 | 11 +++--------
 target/unicore32/op_helper.c               |  7 ++-----
 13 files changed, 37 insertions(+), 69 deletions(-)
 create mode 100644 scripts/coccinelle/cpu_restore_state.cocci

diff --git a/scripts/coccinelle/cpu_restore_state.cocci b/scripts/coccinelle/cpu_restore_state.cocci
new file mode 100644
index 0000000000..934a042382
--- /dev/null
+++ b/scripts/coccinelle/cpu_restore_state.cocci
@@ -0,0 +1,12 @@
+// Remove unneeded tests before calling cpu_restore_state
+//
+// spatch --macro-file scripts/cocci-macro-file.h \
+//        --sp-file ./scripts/coccinelle/cpu_restore_state.cocci \
+//        --keep-comments --in-place --use-gitgrep --dir target
+@@
+identifier A;
+expression C;
+@@
+-if (A) {
+     cpu_restore_state(C, A);
+-}
diff --git a/target/alpha/mem_helper.c b/target/alpha/mem_helper.c
index 3c06baa93a..6cf9bba17e 100644
--- a/target/alpha/mem_helper.c
+++ b/target/alpha/mem_helper.c
@@ -34,9 +34,7 @@ void alpha_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
     uint64_t pc;
     uint32_t insn;
 
-    if (retaddr) {
-        cpu_restore_state(cs, retaddr);
-    }
+    cpu_restore_state(cs, retaddr);
 
     pc = env->pc;
     insn = cpu_ldl_code(env, pc);
@@ -58,9 +56,7 @@ void alpha_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr,
     AlphaCPU *cpu = ALPHA_CPU(cs);
     CPUAlphaState *env = &cpu->env;
 
-    if (retaddr) {
-        cpu_restore_state(cs, retaddr);
-    }
+    cpu_restore_state(cs, retaddr);
 
     env->trap_arg0 = addr;
     env->trap_arg1 = access_type == MMU_DATA_STORE ? 1 : 0;
@@ -80,9 +76,7 @@ void tlb_fill(CPUState *cs, target_ulong addr, MMUAccessType access_type,
 
     ret = alpha_cpu_handle_mmu_fault(cs, addr, access_type, mmu_idx);
     if (unlikely(ret != 0)) {
-        if (retaddr) {
-            cpu_restore_state(cs, retaddr);
-        }
+        cpu_restore_state(cs, retaddr);
         /* Exception index and error code are already set */
         cpu_loop_exit(cs);
     }
diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
index a40a84ac24..504556a697 100644
--- a/target/arm/op_helper.c
+++ b/target/arm/op_helper.c
@@ -175,11 +175,8 @@ void tlb_fill(CPUState *cs, target_ulong addr, MMUAccessType access_type,
     if (unlikely(ret)) {
         ARMCPU *cpu = ARM_CPU(cs);
         uint32_t fsc;
-
-        if (retaddr) {
-            /* now we have a real cpu fault */
-            cpu_restore_state(cs, retaddr);
-        }
+        /* now we have a real cpu fault */
+        cpu_restore_state(cs, retaddr);
 
         if (fsr & (1 << 9)) {
             /* LPAE format fault status register : bottom 6 bits are
@@ -210,11 +207,8 @@ void arm_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
     uint32_t fsr, fsc;
     ARMMMUFaultInfo fi = {};
     ARMMMUIdx arm_mmu_idx = core_to_arm_mmu_idx(env, mmu_idx);
-
-    if (retaddr) {
         /* now we have a real cpu fault */
-        cpu_restore_state(cs, retaddr);
-    }
+    cpu_restore_state(cs, retaddr);
 
     /* the DFSR for an alignment fault depends on whether we're using
      * the LPAE long descriptor format, or the short descriptor format
@@ -244,11 +238,8 @@ void arm_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr,
     uint32_t fsr, fsc;
     ARMMMUFaultInfo fi = {};
     ARMMMUIdx arm_mmu_idx = core_to_arm_mmu_idx(env, mmu_idx);
-
-    if (retaddr) {
         /* now we have a real cpu fault */
-        cpu_restore_state(cs, retaddr);
-    }
+    cpu_restore_state(cs, retaddr);
 
     /* The EA bit in syndromes and fault status registers is an
      * IMPDEF classification of external aborts. ARM implementations
diff --git a/target/i386/svm_helper.c b/target/i386/svm_helper.c
index f479239875..303106981c 100644
--- a/target/i386/svm_helper.c
+++ b/target/i386/svm_helper.c
@@ -584,9 +584,7 @@ void cpu_vmexit(CPUX86State *env, uint32_t exit_code, uint64_t exit_info_1,
 {
     CPUState *cs = CPU(x86_env_get_cpu(env));
 
-    if (retaddr) {
-        cpu_restore_state(cs, retaddr);
-    }
+    cpu_restore_state(cs, retaddr);
 
     qemu_log_mask(CPU_LOG_TB_IN_ASM, "vmexit(%08x, %016" PRIx64 ", %016"
                   PRIx64 ", " TARGET_FMT_lx ")!\n",
diff --git a/target/lm32/op_helper.c b/target/lm32/op_helper.c
index 2177c8ad12..7b800bbeab 100644
--- a/target/lm32/op_helper.c
+++ b/target/lm32/op_helper.c
@@ -150,11 +150,8 @@ void tlb_fill(CPUState *cs, target_ulong addr, MMUAccessType access_type,
     int ret;
 
     ret = lm32_cpu_handle_mmu_fault(cs, addr, access_type, mmu_idx);
-    if (unlikely(ret)) {
-        if (retaddr) {
-            /* now we have a real cpu fault */
-            cpu_restore_state(cs, retaddr);
-        }
+    if (unlikely(ret)) {/* now we have a real cpu fault */
+        cpu_restore_state(cs, retaddr);
         cpu_loop_exit(cs);
     }
 }
diff --git a/target/m68k/op_helper.c b/target/m68k/op_helper.c
index 63089511cb..3079e04c7d 100644
--- a/target/m68k/op_helper.c
+++ b/target/m68k/op_helper.c
@@ -45,11 +45,8 @@ void tlb_fill(CPUState *cs, target_ulong addr, MMUAccessType access_type,
     int ret;
 
     ret = m68k_cpu_handle_mmu_fault(cs, addr, access_type, mmu_idx);
-    if (unlikely(ret)) {
-        if (retaddr) {
-            /* now we have a real cpu fault */
-            cpu_restore_state(cs, retaddr);
-        }
+    if (unlikely(ret)) {/* now we have a real cpu fault */
+        cpu_restore_state(cs, retaddr);
         cpu_loop_exit(cs);
     }
 }
diff --git a/target/microblaze/op_helper.c b/target/microblaze/op_helper.c
index 1e07e21c1c..3b862faaa1 100644
--- a/target/microblaze/op_helper.c
+++ b/target/microblaze/op_helper.c
@@ -39,11 +39,8 @@ void tlb_fill(CPUState *cs, target_ulong addr, MMUAccessType access_type,
     int ret;
 
     ret = mb_cpu_handle_mmu_fault(cs, addr, access_type, mmu_idx);
-    if (unlikely(ret)) {
-        if (retaddr) {
-            /* now we have a real cpu fault */
-            cpu_restore_state(cs, retaddr);
-        }
+    if (unlikely(ret)) {/* now we have a real cpu fault */
+        cpu_restore_state(cs, retaddr);
         cpu_loop_exit(cs);
     }
 }
diff --git a/target/moxie/helper.c b/target/moxie/helper.c
index 330299f5a7..2ecee89f11 100644
--- a/target/moxie/helper.c
+++ b/target/moxie/helper.c
@@ -36,9 +36,7 @@ void tlb_fill(CPUState *cs, target_ulong addr, MMUAccessType access_type,
 
     ret = moxie_cpu_handle_mmu_fault(cs, addr, access_type, mmu_idx);
     if (unlikely(ret)) {
-        if (retaddr) {
-            cpu_restore_state(cs, retaddr);
-        }
+        cpu_restore_state(cs, retaddr);
     }
     cpu_loop_exit(cs);
 }
diff --git a/target/nios2/mmu.c b/target/nios2/mmu.c
index fe9298af50..6d66a5702d 100644
--- a/target/nios2/mmu.c
+++ b/target/nios2/mmu.c
@@ -41,11 +41,8 @@ void tlb_fill(CPUState *cs, target_ulong addr, MMUAccessType access_type,
     int ret;
 
     ret = nios2_cpu_handle_mmu_fault(cs, addr, access_type, mmu_idx);
-    if (unlikely(ret)) {
-        if (retaddr) {
-            /* now we have a real cpu fault */
-            cpu_restore_state(cs, retaddr);
-        }
+    if (unlikely(ret)) {/* now we have a real cpu fault */
+        cpu_restore_state(cs, retaddr);
         cpu_loop_exit(cs);
     }
 }
diff --git a/target/openrisc/mmu_helper.c b/target/openrisc/mmu_helper.c
index a44d0aa51a..47cd7775b6 100644
--- a/target/openrisc/mmu_helper.c
+++ b/target/openrisc/mmu_helper.c
@@ -32,11 +32,8 @@ void tlb_fill(CPUState *cs, target_ulong addr, MMUAccessType access_type,
 
     ret = openrisc_cpu_handle_mmu_fault(cs, addr, access_type, mmu_idx);
 
-    if (ret) {
-        if (retaddr) {
-            /* now we have a real cpu fault.  */
-            cpu_restore_state(cs, retaddr);
-        }
+    if (ret) {/* now we have a real cpu fault.  */
+        cpu_restore_state(cs, retaddr);
         /* Raise Exception.  */
         cpu_loop_exit(cs);
     }
diff --git a/target/s390x/excp_helper.c b/target/s390x/excp_helper.c
index e04b670663..8584ec43c1 100644
--- a/target/s390x/excp_helper.c
+++ b/target/s390x/excp_helper.c
@@ -554,9 +554,7 @@ void s390x_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
     S390CPU *cpu = S390_CPU(cs);
     CPUS390XState *env = &cpu->env;
 
-    if (retaddr) {
-        cpu_restore_state(cs, retaddr);
-    }
+    cpu_restore_state(cs, retaddr);
     program_interrupt(env, PGM_SPECIFICATION, ILEN_AUTO);
 }
 
diff --git a/target/tricore/op_helper.c b/target/tricore/op_helper.c
index 7af202c8c0..b0307de1ea 100644
--- a/target/tricore/op_helper.c
+++ b/target/tricore/op_helper.c
@@ -31,9 +31,7 @@ raise_exception_sync_internal(CPUTriCoreState *env, uint32_t class, int tin,
 {
     CPUState *cs = CPU(tricore_env_get_cpu(env));
     /* in case we come from a helper-call we need to restore the PC */
-    if (pc) {
-        cpu_restore_state(cs, pc);
-    }
+    cpu_restore_state(cs, pc);
 
     /* Tin is loaded into d[15] */
     env->gpr_d[15] = tin;
@@ -2804,11 +2802,8 @@ static inline void QEMU_NORETURN do_raise_exception_err(CPUTriCoreState *env,
     CPUState *cs = CPU(tricore_env_get_cpu(env));
     cs->exception_index = exception;
     env->error_code = error_code;
-
-    if (pc) {
-        /* now we have a real cpu fault */
-        cpu_restore_state(cs, pc);
-    }
+    /* now we have a real cpu fault */
+    cpu_restore_state(cs, pc);
 
     cpu_loop_exit(cs);
 }
diff --git a/target/unicore32/op_helper.c b/target/unicore32/op_helper.c
index 0872c29faa..5a826b0e31 100644
--- a/target/unicore32/op_helper.c
+++ b/target/unicore32/op_helper.c
@@ -250,11 +250,8 @@ void tlb_fill(CPUState *cs, target_ulong addr, MMUAccessType access_type,
     int ret;
 
     ret = uc32_cpu_handle_mmu_fault(cs, addr, access_type, mmu_idx);
-    if (unlikely(ret)) {
-        if (retaddr) {
-            /* now we have a real cpu fault */
-            cpu_restore_state(cs, retaddr);
-        }
+    if (unlikely(ret)) {/* now we have a real cpu fault */
+        cpu_restore_state(cs, retaddr);
         cpu_loop_exit(cs);
     }
 }
-- 
2.14.2

Re: [Qemu-devel] [PATCH v2 2/2] target/*helper: don't check retaddr before calling cpu_restore_state

[Laurent Vivier]

Le 08/11/2017 à 16:32, Alex Bennée a écrit :
> cpu_restore_state officially supports being passed an address it can't
> resolve the state for. As a result the checks in the helpers are
> superfluous and can be removed. This makes the code consistent with
> other users of cpu_restore_state.
> 
> Of course this does nothing to address what to do if cpu_restore_state
> can't resolve the state but so far it seems this is handled elsewhere.
> 
> The change was made with included coccinelle script.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  scripts/coccinelle/cpu_restore_state.cocci | 12 ++++++++++++
>  target/alpha/mem_helper.c                  | 12 +++---------
>  target/arm/op_helper.c                     | 17 ++++-------------
>  target/i386/svm_helper.c                   |  4 +---
>  target/lm32/op_helper.c                    |  7 ++-----
>  target/m68k/op_helper.c                    |  7 ++-----
>  target/microblaze/op_helper.c              |  7 ++-----
>  target/moxie/helper.c                      |  4 +---
>  target/nios2/mmu.c                         |  7 ++-----
>  target/openrisc/mmu_helper.c               |  7 ++-----
>  target/s390x/excp_helper.c                 |  4 +---
>  target/tricore/op_helper.c                 | 11 +++--------
>  target/unicore32/op_helper.c               |  7 ++-----
>  13 files changed, 37 insertions(+), 69 deletions(-)
>  create mode 100644 scripts/coccinelle/cpu_restore_state.cocci
> 
> diff --git a/scripts/coccinelle/cpu_restore_state.cocci b/scripts/coccinelle/cpu_restore_state.cocci
> new file mode 100644
> index 0000000000..934a042382
> --- /dev/null
> +++ b/scripts/coccinelle/cpu_restore_state.cocci
> @@ -0,0 +1,12 @@
> +// Remove unneeded tests before calling cpu_restore_state
> +//
> +// spatch --macro-file scripts/cocci-macro-file.h \
> +//        --sp-file ./scripts/coccinelle/cpu_restore_state.cocci \
> +//        --keep-comments --in-place --use-gitgrep --dir target
> +@@
> +identifier A;
> +expression C;
> +@@
> +-if (A) {
> +     cpu_restore_state(C, A);
> +-}
> diff --git a/target/alpha/mem_helper.c b/target/alpha/mem_helper.c
> index 3c06baa93a..6cf9bba17e 100644
> --- a/target/alpha/mem_helper.c
> +++ b/target/alpha/mem_helper.c
> @@ -34,9 +34,7 @@ void alpha_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
>      uint64_t pc;
>      uint32_t insn;
>  
> -    if (retaddr) {
> -        cpu_restore_state(cs, retaddr);
> -    }
> +    cpu_restore_state(cs, retaddr);
>  
>      pc = env->pc;
>      insn = cpu_ldl_code(env, pc);
> @@ -58,9 +56,7 @@ void alpha_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr,
>      AlphaCPU *cpu = ALPHA_CPU(cs);
>      CPUAlphaState *env = &cpu->env;
>  
> -    if (retaddr) {
> -        cpu_restore_state(cs, retaddr);
> -    }
> +    cpu_restore_state(cs, retaddr);
>  
>      env->trap_arg0 = addr;
>      env->trap_arg1 = access_type == MMU_DATA_STORE ? 1 : 0;
> @@ -80,9 +76,7 @@ void tlb_fill(CPUState *cs, target_ulong addr, MMUAccessType access_type,
>  
>      ret = alpha_cpu_handle_mmu_fault(cs, addr, access_type, mmu_idx);
>      if (unlikely(ret != 0)) {
> -        if (retaddr) {
> -            cpu_restore_state(cs, retaddr);
> -        }
> +        cpu_restore_state(cs, retaddr);
>          /* Exception index and error code are already set */
>          cpu_loop_exit(cs);
>      }
> diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
> index a40a84ac24..504556a697 100644
> --- a/target/arm/op_helper.c
> +++ b/target/arm/op_helper.c
> @@ -175,11 +175,8 @@ void tlb_fill(CPUState *cs, target_ulong addr, MMUAccessType access_type,
>      if (unlikely(ret)) {
>          ARMCPU *cpu = ARM_CPU(cs);
>          uint32_t fsc;
> -
> -        if (retaddr) {
> -            /* now we have a real cpu fault */
> -            cpu_restore_state(cs, retaddr);
> -        }
> +        /* now we have a real cpu fault */
> +        cpu_restore_state(cs, retaddr);
>  
>          if (fsr & (1 << 9)) {
>              /* LPAE format fault status register : bottom 6 bits are
> @@ -210,11 +207,8 @@ void arm_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
>      uint32_t fsr, fsc;
>      ARMMMUFaultInfo fi = {};
>      ARMMMUIdx arm_mmu_idx = core_to_arm_mmu_idx(env, mmu_idx);
> -
> -    if (retaddr) {
>          /* now we have a real cpu fault */
> -        cpu_restore_state(cs, retaddr);
> -    }
> +    cpu_restore_state(cs, retaddr);
>  
>      /* the DFSR for an alignment fault depends on whether we're using
>       * the LPAE long descriptor format, or the short descriptor format
> @@ -244,11 +238,8 @@ void arm_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr,
>      uint32_t fsr, fsc;
>      ARMMMUFaultInfo fi = {};
>      ARMMMUIdx arm_mmu_idx = core_to_arm_mmu_idx(env, mmu_idx);
> -
> -    if (retaddr) {
>          /* now we have a real cpu fault */
> -        cpu_restore_state(cs, retaddr);
> -    }
> +    cpu_restore_state(cs, retaddr);
>  
>      /* The EA bit in syndromes and fault status registers is an
>       * IMPDEF classification of external aborts. ARM implementations
> diff --git a/target/i386/svm_helper.c b/target/i386/svm_helper.c
> index f479239875..303106981c 100644
> --- a/target/i386/svm_helper.c
> +++ b/target/i386/svm_helper.c
> @@ -584,9 +584,7 @@ void cpu_vmexit(CPUX86State *env, uint32_t exit_code, uint64_t exit_info_1,
>  {
>      CPUState *cs = CPU(x86_env_get_cpu(env));
>  
> -    if (retaddr) {
> -        cpu_restore_state(cs, retaddr);
> -    }
> +    cpu_restore_state(cs, retaddr);
>  
>      qemu_log_mask(CPU_LOG_TB_IN_ASM, "vmexit(%08x, %016" PRIx64 ", %016"
>                    PRIx64 ", " TARGET_FMT_lx ")!\n",
> diff --git a/target/lm32/op_helper.c b/target/lm32/op_helper.c
> index 2177c8ad12..7b800bbeab 100644
> --- a/target/lm32/op_helper.c
> +++ b/target/lm32/op_helper.c
> @@ -150,11 +150,8 @@ void tlb_fill(CPUState *cs, target_ulong addr, MMUAccessType access_type,
>      int ret;
>  
>      ret = lm32_cpu_handle_mmu_fault(cs, addr, access_type, mmu_idx);
> -    if (unlikely(ret)) {
> -        if (retaddr) {
> -            /* now we have a real cpu fault */
> -            cpu_restore_state(cs, retaddr);
> -        }
> +    if (unlikely(ret)) {/* now we have a real cpu fault */
> +        cpu_restore_state(cs, retaddr);
>          cpu_loop_exit(cs);
>      }
>  }
> diff --git a/target/m68k/op_helper.c b/target/m68k/op_helper.c
> index 63089511cb..3079e04c7d 100644
> --- a/target/m68k/op_helper.c
> +++ b/target/m68k/op_helper.c
> @@ -45,11 +45,8 @@ void tlb_fill(CPUState *cs, target_ulong addr, MMUAccessType access_type,
>      int ret;
>  
>      ret = m68k_cpu_handle_mmu_fault(cs, addr, access_type, mmu_idx);
> -    if (unlikely(ret)) {
> -        if (retaddr) {
> -            /* now we have a real cpu fault */
> -            cpu_restore_state(cs, retaddr);
> -        }
> +    if (unlikely(ret)) {/* now we have a real cpu fault */
> +        cpu_restore_state(cs, retaddr);
>          cpu_loop_exit(cs);
>      }
>  }
> diff --git a/target/microblaze/op_helper.c b/target/microblaze/op_helper.c
> index 1e07e21c1c..3b862faaa1 100644
> --- a/target/microblaze/op_helper.c
> +++ b/target/microblaze/op_helper.c
> @@ -39,11 +39,8 @@ void tlb_fill(CPUState *cs, target_ulong addr, MMUAccessType access_type,
>      int ret;
>  
>      ret = mb_cpu_handle_mmu_fault(cs, addr, access_type, mmu_idx);
> -    if (unlikely(ret)) {
> -        if (retaddr) {
> -            /* now we have a real cpu fault */
> -            cpu_restore_state(cs, retaddr);
> -        }
> +    if (unlikely(ret)) {/* now we have a real cpu fault */
> +        cpu_restore_state(cs, retaddr);
>          cpu_loop_exit(cs);
>      }
>  }
> diff --git a/target/moxie/helper.c b/target/moxie/helper.c
> index 330299f5a7..2ecee89f11 100644
> --- a/target/moxie/helper.c
> +++ b/target/moxie/helper.c
> @@ -36,9 +36,7 @@ void tlb_fill(CPUState *cs, target_ulong addr, MMUAccessType access_type,
>  
>      ret = moxie_cpu_handle_mmu_fault(cs, addr, access_type, mmu_idx);
>      if (unlikely(ret)) {
> -        if (retaddr) {
> -            cpu_restore_state(cs, retaddr);
> -        }
> +        cpu_restore_state(cs, retaddr);
>      }
>      cpu_loop_exit(cs);
>  }
> diff --git a/target/nios2/mmu.c b/target/nios2/mmu.c
> index fe9298af50..6d66a5702d 100644
> --- a/target/nios2/mmu.c
> +++ b/target/nios2/mmu.c
> @@ -41,11 +41,8 @@ void tlb_fill(CPUState *cs, target_ulong addr, MMUAccessType access_type,
>      int ret;
>  
>      ret = nios2_cpu_handle_mmu_fault(cs, addr, access_type, mmu_idx);
> -    if (unlikely(ret)) {
> -        if (retaddr) {
> -            /* now we have a real cpu fault */
> -            cpu_restore_state(cs, retaddr);
> -        }
> +    if (unlikely(ret)) {/* now we have a real cpu fault */
> +        cpu_restore_state(cs, retaddr);
>          cpu_loop_exit(cs);
>      }
>  }
> diff --git a/target/openrisc/mmu_helper.c b/target/openrisc/mmu_helper.c
> index a44d0aa51a..47cd7775b6 100644
> --- a/target/openrisc/mmu_helper.c
> +++ b/target/openrisc/mmu_helper.c
> @@ -32,11 +32,8 @@ void tlb_fill(CPUState *cs, target_ulong addr, MMUAccessType access_type,
>  
>      ret = openrisc_cpu_handle_mmu_fault(cs, addr, access_type, mmu_idx);
>  
> -    if (ret) {
> -        if (retaddr) {
> -            /* now we have a real cpu fault.  */
> -            cpu_restore_state(cs, retaddr);
> -        }
> +    if (ret) {/* now we have a real cpu fault.  */
> +        cpu_restore_state(cs, retaddr);
>          /* Raise Exception.  */
>          cpu_loop_exit(cs);
>      }
> diff --git a/target/s390x/excp_helper.c b/target/s390x/excp_helper.c
> index e04b670663..8584ec43c1 100644
> --- a/target/s390x/excp_helper.c
> +++ b/target/s390x/excp_helper.c
> @@ -554,9 +554,7 @@ void s390x_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
>      S390CPU *cpu = S390_CPU(cs);
>      CPUS390XState *env = &cpu->env;
>  
> -    if (retaddr) {
> -        cpu_restore_state(cs, retaddr);
> -    }
> +    cpu_restore_state(cs, retaddr);
>      program_interrupt(env, PGM_SPECIFICATION, ILEN_AUTO);
>  }
>  
> diff --git a/target/tricore/op_helper.c b/target/tricore/op_helper.c
> index 7af202c8c0..b0307de1ea 100644
> --- a/target/tricore/op_helper.c
> +++ b/target/tricore/op_helper.c
> @@ -31,9 +31,7 @@ raise_exception_sync_internal(CPUTriCoreState *env, uint32_t class, int tin,
>  {
>      CPUState *cs = CPU(tricore_env_get_cpu(env));
>      /* in case we come from a helper-call we need to restore the PC */
> -    if (pc) {
> -        cpu_restore_state(cs, pc);
> -    }
> +    cpu_restore_state(cs, pc);
>  
>      /* Tin is loaded into d[15] */
>      env->gpr_d[15] = tin;
> @@ -2804,11 +2802,8 @@ static inline void QEMU_NORETURN do_raise_exception_err(CPUTriCoreState *env,
>      CPUState *cs = CPU(tricore_env_get_cpu(env));
>      cs->exception_index = exception;
>      env->error_code = error_code;
> -
> -    if (pc) {
> -        /* now we have a real cpu fault */
> -        cpu_restore_state(cs, pc);
> -    }
> +    /* now we have a real cpu fault */
> +    cpu_restore_state(cs, pc);
>  
>      cpu_loop_exit(cs);
>  }
> diff --git a/target/unicore32/op_helper.c b/target/unicore32/op_helper.c
> index 0872c29faa..5a826b0e31 100644
> --- a/target/unicore32/op_helper.c
> +++ b/target/unicore32/op_helper.c
> @@ -250,11 +250,8 @@ void tlb_fill(CPUState *cs, target_ulong addr, MMUAccessType access_type,
>      int ret;
>  
>      ret = uc32_cpu_handle_mmu_fault(cs, addr, access_type, mmu_idx);
> -    if (unlikely(ret)) {
> -        if (retaddr) {
> -            /* now we have a real cpu fault */
> -            cpu_restore_state(cs, retaddr);
> -        }
> +    if (unlikely(ret)) {/* now we have a real cpu fault */
> +        cpu_restore_state(cs, retaddr);
>          cpu_loop_exit(cs);
>      }
>  }
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [Qemu-devel] [PATCH v2 2/2] target/*helper: don't check retaddr before calling cpu_restore_state

[Richard Henderson]

On 11/08/2017 04:32 PM, Alex Bennée wrote:
> -        if (retaddr) {
> -            cpu_restore_state(cs, retaddr);
> -        }
> +        cpu_restore_state(cs, retaddr);
>          /* Exception index and error code are already set */
>          cpu_loop_exit(cs);

Not that this is wrong, but while we're cleaning things up,
this call combo is cpu_loop_exit_restore.

You create at least 8 of these pairs in this patch.

> +++ b/target/moxie/helper.c
> @@ -36,9 +36,7 @@ void tlb_fill(CPUState *cs, target_ulong addr, MMUAccessType access_type,
>  
>      ret = moxie_cpu_handle_mmu_fault(cs, addr, access_type, mmu_idx);
>      if (unlikely(ret)) {
> -        if (retaddr) {
> -            cpu_restore_state(cs, retaddr);
> -        }
> +        cpu_restore_state(cs, retaddr);
>      }
>      cpu_loop_exit(cs);
>  }


This one *should* have been such a pair, but there's an existing bug in moxie
that should have restricted the cpu_loop_exit to inside the if.

All that said,

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~