Skip to contentSkip to conversation

Fix exceptions handling for MIPS and i386

PATCH Superseded Pavel Dovgalyuk 3 participants 15 messages Jun 29, 2015 – Jul 1, 2015

v4 of 7 Superseded Latest is v7 →

lore
Share & tools
lore

Pavel Dovgalyuk

author
#0 lore raw

v4 cover Fix exceptions handling for MIPS and i386

QEMU targets ISAs contain instruction that can break the execution flow with exceptions. When exception breaks the execution of the translation block it may corrupt PC and icount values.

This set of patches fixes exception handling for MIPS and i386 targets.

Incorrect execution for i386 is causes by exceptions raised by MMU functions. MMU helper functions are called from generated code and other helper functions. In both cases they try to get function's return address for restoring virtual CPU state.

When MMU helper is called from some other helper function (like helper_maskmov_xmm) through cpu_st* function, the return address will point to that helper. That is why CPU state cannot be restored in the case of MMU fault.

This bug can occur when maskmov instruction is located in the middle of the translation block.

Execution sequence for this example:

TB start:
PC1: instr1
instr2
PC2: maskmov <page fault>
<page fault processing>
PC1: instr1
instr2
maskmov

At the start of TB execution guest PC points to instr1. When page fault occurs QEMU tries to restore guest PC (which should be equal to PC2). It reads host PC from the call stack and checks whether it points to TB or not. Bug in ldst helpers implementation provides incorrect host PC, which is not located within the TB. That's why QEMU cannot recover guest PC and it remains the same (PC1). After page fault processing QEMU restarts TB and executes instr1 and instr2 for the second time, because guest PC was not recovered.

Bugs in MIPS helper functions do not break the execution in regular TCG mode, because PC value is updated before calling the functions that can raise an exception. But icount value cannot be updated this way. Therefore exceptions make execution in icount mode non-determinisic.
In icount mode every translation block looks as follows:

if icount < n then exit
icount -= n
instr1
instr2
...
instrn
exit

When one of these instructions initiates an exception, icount should be restored and adjusted number of instructions should be subtracted from icount instead of initial n.

tlb_fill function passes retaddr to raise_exception, which allows restoring current instructions in TB and correct icount calculation.

When exception triggered with other function (e.g. by embedding call to exception raising helper into TB), then PC is not passed as retaddr and correct icount is not recovered. In such cases icount will be decreased by the value equal to the size of TB.

This behavior leads to incorrect values of virtual clock and non-deterministic execution of the code.

These patches passes pointer to the translation block code to the exception handler. It allows correct restoring of PC and icount values.

v4 changes:
* Fixed exceptions handling for PowerPC
* Fixed passing of mmu_idx into helpers (as suggested by Aurelien Jarno)
* Added cpu_loop_exit_restore function (as suggested by Aurelien Jarno)
* Fixed exceptions handling in compare helper functions for MIPS (as suggested by Aurelien Jarno)
* Removed several CPU state saving calls for MIPS (as suggested by Aurelien Jarno)

v3 changes:
* Modified exception handling for syscall (as suggested by Aurelien Jarno)
* Removed redundant calls to save_cpu_state (as suggested by Aurelien Jarno)
* Removed helper_call* functions from softmmu (as suggested by Paolo Bonzini)

v2 changes:
* Added softmmu functions to pass TB return value into memory operations handlers
* Fixed memory operations handling for MIPS
* Disabled updates of the PC that are overridden with cpu_restore_state
* Fixed memory operations and exceptions invoked by i386 helpers

---

Pavel Dovgalyuk (5):
softmmu: add helper function to pass through retaddr
cpu-exec: introduce loop exit with restore function
target-mips: improve exceptions handling
target-i386: fix memory operations in helpers
target-ppc: exceptions handling in icount mode

 cpu-exec.c                       |    9 
 include/exec/cpu_ldst_template.h |   59 +++
 include/exec/exec-all.h          |    1 
 softmmu_template.h               |    6 
 target-i386/cc_helper.c          |    2 
 target-i386/cpu.h                |    5 
 target-i386/excp_helper.c        |   21 +
 target-i386/fpu_helper.c         |  146 ++++----
 target-i386/helper.c             |    4 
 target-i386/int_helper.c         |   32 +-
 target-i386/mem_helper.c         |   39 +-
 target-i386/misc_helper.c        |   12 -
 target-i386/ops_sse.h            |    2 
 target-i386/seg_helper.c         |  712 +++++++++++++++++++-------------------
 target-i386/svm_helper.c         |    4 
 target-i386/translate.c          |   25 -
 target-mips/cpu.h                |   23 +
 target-mips/helper.h             |    1 
 target-mips/msa_helper.c         |  158 +++++---
 target-mips/op_helper.c          |  179 ++++------
 target-mips/translate.c          |  372 +++++++++-----------
 target-ppc/cpu.h                 |    3 
 target-ppc/excp_helper.c         |   38 ++
 target-ppc/fpu_helper.c          |  191 +++++-----
 target-ppc/helper.h              |    1 
 target-ppc/mem_helper.c          |    6 
 target-ppc/misc_helper.c         |    8 
 target-ppc/mmu-hash64.c          |   12 -
 target-ppc/mmu_helper.c          |   18 -
 target-ppc/timebase_helper.c     |   20 +
 target-ppc/translate.c           |   84 ----
 tcg/tcg.h                        |   23 +
 32 files changed, 1122 insertions(+), 1094 deletions(-)
Signature · 1 line
Pavel Dovgalyuk
author
#1 lore raw

PATCH v4 1/5 softmmu: add helper function to pass through retaddr

This patch introduces several helpers to pass return address which points to the TB. Correct return address allows correct restoring of the guest PC and icount. These functions should be used when helpers embedded into TB invoke memory operations.

Signed-off-by Pavel Dovgalyuk <[email protected]>
---
 include/exec/cpu_ldst_template.h |   59 +++++++++++++++++++++++++++++++++-----
 softmmu_template.h               |    6 ----
 tcg/tcg.h                        |   23 +++++++++++++++
 3 files changed, 74 insertions(+), 14 deletions(-)
include/exec/cpu_ldst_template.h +51 -8
index 95ab750..3091c00 100644
@@ -27,20 +27,24 @@
 #define SUFFIX q
 #define USUFFIX q
 #define DATA_TYPE uint64_t
+#define SHIFT 3
 #elif DATA_SIZE == 4
 #define SUFFIX l
 #define USUFFIX l
 #define DATA_TYPE uint32_t
+#define SHIFT 2
 #elif DATA_SIZE == 2
 #define SUFFIX w
 #define USUFFIX uw
 #define DATA_TYPE uint16_t
 #define DATA_STYPE int16_t
+#define SHIFT 1
 #elif DATA_SIZE == 1
 #define SUFFIX b
 #define USUFFIX ub
 #define DATA_TYPE uint8_t
 #define DATA_STYPE int8_t
+#define SHIFT 0
 #else
 #error unsupported data size
 #endif

@@ -54,27 +58,36 @@
 #ifdef SOFTMMU_CODE_ACCESS
 #define ADDR_READ addr_code
 #define MMUSUFFIX _cmmu
+#define URETSUFFIX SUFFIX
+#define SRETSUFFIX SUFFIX
 #else
 #define ADDR_READ addr_read
 #define MMUSUFFIX _mmu
+#define URETSUFFIX USUFFIX
+#define SRETSUFFIX glue(s, SUFFIX)
 #endif
 
 /* generic load/store macros */
 
 static inline RES_TYPE
-glue(glue(cpu_ld, USUFFIX), MEMSUFFIX)(CPUArchState *env, target_ulong ptr)
+glue(glue(glue(cpu_ld, USUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
+                                                  target_ulong ptr,
+                                                  uintptr_t retaddr)
 {
     int page_index;
     RES_TYPE res;
     target_ulong addr;
     int mmu_idx;
+    TCGMemOpIdx oi;
 
     addr = ptr;
     page_index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
     mmu_idx = CPU_MMU_INDEX;
     if (unlikely(env->tlb_table[mmu_idx][page_index].ADDR_READ !=
                  (addr & (TARGET_PAGE_MASK | (DATA_SIZE - 1))))) {
-        res = glue(glue(helper_ld, SUFFIX), MMUSUFFIX)(env, addr, mmu_idx);
+        oi = make_memop_idx(SHIFT, mmu_idx);
+        res = glue(glue(helper_ret_ld, URETSUFFIX), MMUSUFFIX)(env, addr,
+                                                            oi, retaddr);
     } else {
         uintptr_t hostaddr = addr + env->tlb_table[mmu_idx][page_index].addend;
         res = glue(glue(ld, USUFFIX), _p)((uint8_t *)hostaddr);

@@ -82,27 +95,43 @@ glue(glue(cpu_ld, USUFFIX), MEMSUFFIX)(CPUArchState *env, target_ulong ptr)
     return res;
 }
 
+static inline RES_TYPE
+glue(glue(cpu_ld, USUFFIX), MEMSUFFIX)(CPUArchState *env, target_ulong ptr)
+{
+    return glue(glue(glue(cpu_ld, USUFFIX), MEMSUFFIX), _ra)(env, ptr, 0);
+}
+
 #if DATA_SIZE <= 2
 static inline int
-glue(glue(cpu_lds, SUFFIX), MEMSUFFIX)(CPUArchState *env, target_ulong ptr)
+glue(glue(glue(cpu_lds, SUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
+                                                  target_ulong ptr,
+                                                  uintptr_t retaddr)
 {
     int res, page_index;
     target_ulong addr;
     int mmu_idx;
+    TCGMemOpIdx oi;
 
     addr = ptr;
     page_index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
     mmu_idx = CPU_MMU_INDEX;
     if (unlikely(env->tlb_table[mmu_idx][page_index].ADDR_READ !=
                  (addr & (TARGET_PAGE_MASK | (DATA_SIZE - 1))))) {
-        res = (DATA_STYPE)glue(glue(helper_ld, SUFFIX),
-                               MMUSUFFIX)(env, addr, mmu_idx);
+        oi = make_memop_idx(SHIFT, mmu_idx);
+        res = (DATA_STYPE)glue(glue(helper_ret_ld, SRETSUFFIX),
+                               MMUSUFFIX)(env, addr, oi, retaddr);
     } else {
         uintptr_t hostaddr = addr + env->tlb_table[mmu_idx][page_index].addend;
         res = glue(glue(lds, SUFFIX), _p)((uint8_t *)hostaddr);
     }
     return res;
 }
+
+static inline int
+glue(glue(cpu_lds, SUFFIX), MEMSUFFIX)(CPUArchState *env, target_ulong ptr)
+{
+    return glue(glue(glue(cpu_lds, SUFFIX), MEMSUFFIX), _ra)(env, ptr, 0);
+}
 #endif
 
 #ifndef SOFTMMU_CODE_ACCESS

@@ -110,25 +139,36 @@ glue(glue(cpu_lds, SUFFIX), MEMSUFFIX)(CPUArchState *env, target_ulong ptr)
 /* generic store macro */
 
 static inline void
-glue(glue(cpu_st, SUFFIX), MEMSUFFIX)(CPUArchState *env, target_ulong ptr,
-                                      RES_TYPE v)
+glue(glue(glue(cpu_st, SUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
+                                                 target_ulong ptr,
+                                                 RES_TYPE v, uintptr_t retaddr)
 {
     int page_index;
     target_ulong addr;
     int mmu_idx;
+    TCGMemOpIdx oi;
 
     addr = ptr;
     page_index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
     mmu_idx = CPU_MMU_INDEX;
     if (unlikely(env->tlb_table[mmu_idx][page_index].addr_write !=
                  (addr & (TARGET_PAGE_MASK | (DATA_SIZE - 1))))) {
-        glue(glue(helper_st, SUFFIX), MMUSUFFIX)(env, addr, v, mmu_idx);
+        oi = make_memop_idx(SHIFT, mmu_idx);
+        glue(glue(helper_ret_st, SUFFIX), MMUSUFFIX)(env, addr, v, oi,
+                                                     retaddr);
     } else {
         uintptr_t hostaddr = addr + env->tlb_table[mmu_idx][page_index].addend;
         glue(glue(st, SUFFIX), _p)((uint8_t *)hostaddr, v);
     }
 }
 
+static inline void
+glue(glue(cpu_st, SUFFIX), MEMSUFFIX)(CPUArchState *env, target_ulong ptr,
+                                      RES_TYPE v)
+{
+    glue(glue(glue(cpu_st, SUFFIX), MEMSUFFIX), _ra)(env, ptr, v, 0);
+}
+
 #endif /* !SOFTMMU_CODE_ACCESS */
 
 #undef RES_TYPE

@@ -139,3 +179,6 @@ glue(glue(cpu_st, SUFFIX), MEMSUFFIX)(CPUArchState *env, target_ulong ptr,
 #undef DATA_SIZE
 #undef MMUSUFFIX
 #undef ADDR_READ
+#undef URETSUFFIX
+#undef SRETSUFFIX
+#undef SHIFT
softmmu_template.h +0 -6
index 39f571b..1d59e55 100644
@@ -165,9 +165,6 @@ static inline DATA_TYPE glue(io_read, SUFFIX)(CPUArchState *env,
 }
 #endif
 
-#ifdef SOFTMMU_CODE_ACCESS
-static __attribute__((unused))
-#endif
 WORD_TYPE helper_le_ld_name(CPUArchState *env, target_ulong addr,
                             TCGMemOpIdx oi, uintptr_t retaddr)
 {

@@ -252,9 +249,6 @@ WORD_TYPE helper_le_ld_name(CPUArchState *env, target_ulong addr,
 }
 
 #if DATA_SIZE > 1
-#ifdef SOFTMMU_CODE_ACCESS
-static __attribute__((unused))
-#endif
 WORD_TYPE helper_be_ld_name(CPUArchState *env, target_ulong addr,
                             TCGMemOpIdx oi, uintptr_t retaddr)
 {
tcg/tcg.h +23 -0
index 8098f82..31bd419 100644
@@ -981,25 +981,48 @@ void helper_be_stl_mmu(CPUArchState *env, target_ulong addr, uint32_t val,
 void helper_be_stq_mmu(CPUArchState *env, target_ulong addr, uint64_t val,
                        TCGMemOpIdx oi, uintptr_t retaddr);
 
+uint8_t helper_ret_ldb_cmmu(CPUArchState *env, target_ulong addr,
+                            TCGMemOpIdx oi, uintptr_t retaddr);
+uint16_t helper_le_ldw_cmmu(CPUArchState *env, target_ulong addr,
+                            TCGMemOpIdx oi, uintptr_t retaddr);
+uint32_t helper_le_ldl_cmmu(CPUArchState *env, target_ulong addr,
+                            TCGMemOpIdx oi, uintptr_t retaddr);
+uint64_t helper_le_ldq_cmmu(CPUArchState *env, target_ulong addr,
+                            TCGMemOpIdx oi, uintptr_t retaddr);
+uint16_t helper_be_ldw_cmmu(CPUArchState *env, target_ulong addr,
+                            TCGMemOpIdx oi, uintptr_t retaddr);
+uint32_t helper_be_ldl_cmmu(CPUArchState *env, target_ulong addr,
+                            TCGMemOpIdx oi, uintptr_t retaddr);
+uint64_t helper_be_ldq_cmmu(CPUArchState *env, target_ulong addr,
+                            TCGMemOpIdx oi, uintptr_t retaddr);
+
 /* Temporary aliases until backends are converted.  */
 #ifdef TARGET_WORDS_BIGENDIAN
 # define helper_ret_ldsw_mmu  helper_be_ldsw_mmu
 # define helper_ret_lduw_mmu  helper_be_lduw_mmu
 # define helper_ret_ldsl_mmu  helper_be_ldsl_mmu
 # define helper_ret_ldul_mmu  helper_be_ldul_mmu
+# define helper_ret_ldl_mmu   helper_be_ldul_mmu
 # define helper_ret_ldq_mmu   helper_be_ldq_mmu
 # define helper_ret_stw_mmu   helper_be_stw_mmu
 # define helper_ret_stl_mmu   helper_be_stl_mmu
 # define helper_ret_stq_mmu   helper_be_stq_mmu
+# define helper_ret_ldw_cmmu  helper_be_ldw_cmmu
+# define helper_ret_ldl_cmmu  helper_be_ldl_cmmu
+# define helper_ret_ldq_cmmu  helper_be_ldq_cmmu
 #else
 # define helper_ret_ldsw_mmu  helper_le_ldsw_mmu
 # define helper_ret_lduw_mmu  helper_le_lduw_mmu
 # define helper_ret_ldsl_mmu  helper_le_ldsl_mmu
 # define helper_ret_ldul_mmu  helper_le_ldul_mmu
+# define helper_ret_ldl_mmu   helper_le_ldul_mmu
 # define helper_ret_ldq_mmu   helper_le_ldq_mmu
 # define helper_ret_stw_mmu   helper_le_stw_mmu
 # define helper_ret_stl_mmu   helper_le_stl_mmu
 # define helper_ret_stq_mmu   helper_le_stq_mmu
+# define helper_ret_ldw_cmmu  helper_le_ldw_cmmu
+# define helper_ret_ldl_cmmu  helper_le_ldl_cmmu
+# define helper_ret_ldq_cmmu  helper_le_ldq_cmmu
 #endif
 
 #endif /* CONFIG_SOFTMMU */
Reviewed-by on v4 1/5
#2 lore raw

Re: [Qemu-devel] [PATCH v4 1/5] softmmu: add helper function to pass through retaddr

Quoted 11 lines · Pavel Dovgalyuk

This patch introduces several helpers to pass return address which points to the TB. Correct return address allows correct restoring of the guest PC and icount. These functions should be used when helpers embedded into TB invoke memory operations.

Signed-off-by Pavel Dovgalyuk<[email protected]>
---
  include/exec/cpu_ldst_template.h |   59 +++++++++++++++++++++++++++++++++-----
  softmmu_template.h               |    6 ----
  tcg/tcg.h                        |   23 +++++++++++++++
  3 files changed, 74 insertions(+), 14 deletions(-)
Reviewed-by Richard Henderson <[email protected]>

r~

#3 lore raw

Re: [Qemu-devel] [PATCH v4 1/5] softmmu: add helper function to pass through retaddr

In reply to Pavel Dovgalyuk
Quoted 11 lines · Pavel Dovgalyuk

This patch introduces several helpers to pass return address which points to the TB. Correct return address allows correct restoring of the guest PC and icount. These functions should be used when helpers embedded into TB invoke memory operations.

Signed-off-by Pavel Dovgalyuk <[email protected]>
---
 include/exec/cpu_ldst_template.h |   59 +++++++++++++++++++++++++++++++++-----
 softmmu_template.h               |    6 ----
 tcg/tcg.h                        |   23 +++++++++++++++
 3 files changed, 74 insertions(+), 14 deletions(-)

This patch doesn't provide the equivalent _ra functions for the user mode code, so it causes any linux-user target using this to fail to build. I don't think we want to special case this kind of code in the target code, so they need to be provided.

Otherwise it looks ok.

Signature · 2 lines
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
[email protected]                 http://www.aurel32.net
author
#4 lore raw

PATCH v4 2/5 cpu-exec: introduce loop exit with restore function

In reply to Pavel Dovgalyuk

This patch introduces loop exit function, which also restores guest CPU state according to the value of host program counter.

Signed-off-by Pavel Dovgalyuk <[email protected]>
---
 cpu-exec.c              |    9 +++++++++
 include/exec/exec-all.h |    1 +
 2 files changed, 10 insertions(+), 0 deletions(-)
cpu-exec.c +9 -0
index 0734af2..0b5449e 100644
@@ -134,6 +134,15 @@ void cpu_loop_exit(CPUState *cpu)
     siglongjmp(cpu->jmp_env, 1);
 }
 
+void cpu_loop_exit_restore(CPUState *cpu, uintptr_t pc)
+{
+    if (pc) {
+        cpu_restore_state(cpu, pc);
+    }
+    cpu->current_tb = NULL;
+    siglongjmp(cpu->jmp_env, 1);
+}
+
 /* exit the current TB from a signal handler. The host registers are
    restored in a state compatible with the CPU emulator
  */
include/exec/exec-all.h +1 -0
index 47169c4..18fdc17 100644
@@ -90,6 +90,7 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
                               int cflags);
 void cpu_exec_init(CPUArchState *env);
 void QEMU_NORETURN cpu_loop_exit(CPUState *cpu);
+void QEMU_NORETURN cpu_loop_exit_restore(CPUState *cpu, uintptr_t pc);
 int page_unprotect(target_ulong address, uintptr_t pc, void *puc);
 void tb_invalidate_phys_page_range(tb_page_addr_t start, tb_page_addr_t end,
                                    int is_cpu_write_access);
Reviewed-by on v4 2/5
#5 lore raw

Re: [Qemu-devel] [PATCH v4 2/5] cpu-exec: introduce loop exit with restore function

Quoted 9 lines · Pavel Dovgalyuk

This patch introduces loop exit function, which also restores guest CPU state according to the value of host program counter.

Signed-off-by Pavel Dovgalyuk<[email protected]>
---
  cpu-exec.c              |    9 +++++++++
  include/exec/exec-all.h |    1 +
  2 files changed, 10 insertions(+), 0 deletions(-)
Reviewed-by Richard Henderson <[email protected]>

r~

Reviewed-by on v4 2/5
#6 lore raw

Re: [Qemu-devel] [PATCH v4 2/5] cpu-exec: introduce loop exit with restore function

In reply to Pavel Dovgalyuk
Quoted 10 lines · Pavel Dovgalyuk

This patch introduces loop exit function, which also restores guest CPU state according to the value of host program counter.

Signed-off-by Pavel Dovgalyuk <[email protected]>
---
 cpu-exec.c              |    9 +++++++++
 include/exec/exec-all.h |    1 +
 2 files changed, 10 insertions(+), 0 deletions(-)

Thanks for this patch.

Reviewed-by Aurelien Jarno <[email protected]>
Signature · 2 lines
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
[email protected]                 http://www.aurel32.net
author
#7 lore raw

PATCH v4 3/5 target-mips: improve exceptions handling

In reply to Pavel Dovgalyuk

This patch improves exception handling in MIPS.
Instructions generate several types of exceptions.
When exception is generated, it breaks the execution of the current translation block. Implementation of the exceptions handling does not correctly restore icount for the instruction which caused the exception. In most cases icount will be decreased by the value equal to the size of TB. This patch passes pointer to the translation block internals to the exception handler. It allows correct restoring of the icount value.

v3 changes:
This patch stops translation when instruction which always generates exception is translated. This improves the performance of the patched version compared to original one.

Signed-off-by Pavel Dovgalyuk <[email protected]>
---
 target-mips/cpu.h        |   23 +++
 target-mips/helper.h     |    1 
 target-mips/msa_helper.c |  158 +++++++++++---------
 target-mips/op_helper.c  |  179 ++++++++++------------
 target-mips/translate.c  |  372 ++++++++++++++++++++++------------------------
 5 files changed, 368 insertions(+), 365 deletions(-)
target-mips/cpu.h +23 -0
index f9d2b4c..44d6e29 100644
@@ -1015,4 +1015,27 @@ static inline void cpu_mips_store_cause(CPUMIPSState *env, target_ulong val)
 }
 #endif
 
+static inline void QEMU_NORETURN do_raise_exception_err(CPUMIPSState *env,
+                                                        uint32_t exception,
+                                                        int error_code,
+                                                        uintptr_t pc)
+{
+    CPUState *cs = CPU(mips_env_get_cpu(env));
+
+    if (exception < EXCP_SC) {
+        qemu_log("%s: %d %d\n", __func__, exception, error_code);
+    }
+    cs->exception_index = exception;
+    env->error_code = error_code;
+
+    cpu_loop_exit_restore(cs, pc);
+}
+
+static inline void QEMU_NORETURN do_raise_exception(CPUMIPSState *env,
+                                                    uint32_t exception,
+                                                    uintptr_t pc)
+{
+    do_raise_exception_err(env, exception, 0, pc);
+}
+
 #endif /* !defined (__MIPS_CPU_H__) */
target-mips/helper.h +1 -0
index 3bd0b02..50c699e 100644
@@ -1,5 +1,6 @@
 DEF_HELPER_3(raise_exception_err, noreturn, env, i32, int)
 DEF_HELPER_2(raise_exception, noreturn, env, i32)
+DEF_HELPER_1(raise_exception_debug, noreturn, env)
 
 #ifdef TARGET_MIPS64
 DEF_HELPER_4(sdl, void, env, tl, tl, int)
target-mips/msa_helper.c +85 -73

586 lines not on this page · raw message

target-mips/op_helper.c +82 -97

461 lines not on this page · raw message

target-mips/translate.c +177 -195

1,435 lines not on this page · raw message

#8 lore raw

Re: [Qemu-devel] [PATCH v4 3/5] target-mips: improve exceptions handling

Quoted 22 lines · Pavel Dovgalyuk

This patch improves exception handling in MIPS.
Instructions generate several types of exceptions.
When exception is generated, it breaks the execution of the current translation block. Implementation of the exceptions handling does not correctly restore icount for the instruction which caused the exception. In most cases icount will be decreased by the value equal to the size of TB. This patch passes pointer to the translation block internals to the exception handler. It allows correct restoring of the icount value.

v3 changes:
This patch stops translation when instruction which always generates exception is translated. This improves the performance of the patched version compared to original one.

Signed-off-by Pavel Dovgalyuk <[email protected]>
---
 target-mips/cpu.h        |   23 +++
 target-mips/helper.h     |    1 
 target-mips/msa_helper.c |  158 +++++++++++---------
 target-mips/op_helper.c  |  179 ++++++++++------------
 target-mips/translate.c  |  372 ++++++++++++++++++++++------------------------
 5 files changed, 368 insertions(+), 365 deletions(-)

[ snip ]

Quoted 4 lines
target-mips/op_helper.c +0 -0
index 73a8e45..b562384 100644

[ snip ]

target-mips/op_helper.c void helper_raise_exception(CPUMIPSState *env, uint32_t exception) · line 60

Quoted 10 lines
target-mips/op_helper.c +2 -2
@@ -72,21 +54,21 @@ void helper_raise_exception(CPUMIPSState *env, uint32_t exception)
 #if defined(CONFIG_USER_ONLY)
 #define HELPER_LD(name, insn, type)                                     \
 static inline type do_##name(CPUMIPSState *env, target_ulong addr,      \
-                             int mem_idx)                               \
+                             int mem_idx, uintptr_t retaddr)            \
 {                                                                       \
-    return (type) cpu_##insn##_data(env, addr);                         \
+    return (type) cpu_##insn##_data_ra(env, addr, retaddr);             \
 }

You changed this, but as already said, your first patch doesn't providee the cpu_##insn##_data_ra function. Also you forgot to update the caller of this function, so it doesn't compile.

Quoted 37 lines
target-mips/op_helper.c +8 -8
 #else
 #define HELPER_LD(name, insn, type)                                     \
 static inline type do_##name(CPUMIPSState *env, target_ulong addr,      \
-                             int mem_idx)                               \
+                             int mem_idx, uintptr_t retaddr)            \
 {                                                                       \
     switch (mem_idx)                                                    \
     {                                                                   \
-    case 0: return (type) cpu_##insn##_kernel(env, addr); break;        \
-    case 1: return (type) cpu_##insn##_super(env, addr); break;         \
+    case 0: return (type) cpu_##insn##_kernel_ra(env, addr, retaddr);   \
+    case 1: return (type) cpu_##insn##_super_ra(env, addr, retaddr);    \
     default:                                                            \
-    case 2: return (type) cpu_##insn##_user(env, addr); break;          \
+    case 2: return (type) cpu_##insn##_user_ra(env, addr, retaddr);     \
     }                                                                   \
 }
 #endif

@@ -106,14 +88,14 @@ static inline void do_##name(CPUMIPSState *env, target_ulong addr,      \
 #else
 #define HELPER_ST(name, insn, type)                                     \
 static inline void do_##name(CPUMIPSState *env, target_ulong addr,      \
-                             type val, int mem_idx)                     \
+                             type val, int mem_idx, uintptr_t retaddr)  \
 {                                                                       \
     switch (mem_idx)                                                    \
     {                                                                   \
-    case 0: cpu_##insn##_kernel(env, addr, val); break;                 \
-    case 1: cpu_##insn##_super(env, addr, val); break;                  \
+    case 0: cpu_##insn##_kernel_ra(env, addr, val, retaddr); break;     \
+    case 1: cpu_##insn##_super_ra(env, addr, val, retaddr); break;      \
     default:                                                            \
-    case 2: cpu_##insn##_user(env, addr, val); break;                   \
+    case 2: cpu_##insn##_user_ra(env, addr, val, retaddr); break;       \
     }                                                                   \
 }
 #endif

[ snip ]

target-mips/translate.c generate_exception_err (DisasContext *ctx, int excp, int err) · line 1681

Quoted 10 lines
target-mips/translate.c +1 -0
index fd063a2..85d374c 100644
@@ -1677,15 +1677,21 @@ generate_exception_err (DisasContext *ctx, int excp, int err)
     gen_helper_raise_exception_err(cpu_env, texcp, terr);
     tcg_temp_free_i32(terr);
     tcg_temp_free_i32(texcp);
+    ctx->bstate = BS_STOP;
 }

This should be BS_EXCP. Otherwise the CPU state is saved again after the exception, and a goto_tb is generated.

Otherwise it looks fine, though I have only done some light testing so far.

Signature · 2 lines
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
[email protected]                 http://www.aurel32.net
author
#9 lore raw

PATCH v4 4/5 target-i386: fix memory operations in helpers

In reply to Pavel Dovgalyuk

This patch passes TB return address into softmmu functions that are invoked from target helpers. This allows correct PC and icount recovering while handling MMU faults.

Signed-off-by Pavel Dovgalyuk <[email protected]>
---
 target-i386/cc_helper.c   |    2 
 target-i386/cpu.h         |    5 
 target-i386/excp_helper.c |   21 +
 target-i386/fpu_helper.c  |  146 +++++----
 target-i386/helper.c      |    4 
 target-i386/int_helper.c  |   32 +-
 target-i386/mem_helper.c  |   39 +-
 target-i386/misc_helper.c |   12 -
 target-i386/ops_sse.h     |    2 
 target-i386/seg_helper.c  |  712 +++++++++++++++++++++++----------------------
 target-i386/svm_helper.c  |    4 
 target-i386/translate.c   |   25 --
 12 files changed, 503 insertions(+), 501 deletions(-)
target-i386/cc_helper.c +1 -1
index ecbf0ec..4b523db 100644
@@ -378,7 +378,7 @@ void helper_sti_vm(CPUX86State *env)
 {
     env->eflags |= VIF_MASK;
     if (env->eflags & VIP_MASK) {
-        raise_exception(env, EXCP0D_GPF);
+        raise_exception(env, EXCP0D_GPF, GETPC());
     }
 }
 #endif
target-i386/cpu.h +3 -2
index 4ee12ca..cd70c17 100644
@@ -1249,9 +1249,10 @@ void cpu_x86_inject_mce(Monitor *mon, X86CPU *cpu, int bank,
                         uint64_t misc, int flags);
 
 /* excp_helper.c */
-void QEMU_NORETURN raise_exception(CPUX86State *env, int exception_index);
+void QEMU_NORETURN raise_exception(CPUX86State *env, int exception_index,
+                                   uintptr_t retaddr);
 void QEMU_NORETURN raise_exception_err(CPUX86State *env, int exception_index,
-                                       int error_code);
+                                       int error_code, uintptr_t retaddr);
 void QEMU_NORETURN raise_interrupt(CPUX86State *nenv, int intno, int is_int,
                                    int error_code, int next_eip_addend);
target-i386/excp_helper.c +11 -10
index 99fca84..ca49186 100644
@@ -23,10 +23,10 @@
 #include "exec/helper-proto.h"
 
 #if 0
-#define raise_exception_err(env, a, b)                                  \
+#define raise_exception_err(env, a, b, c)                               \
     do {                                                                \
         qemu_log("raise_exception line=%d\n", __LINE__);                \
-        (raise_exception_err)(env, a, b);                               \
+        (raise_exception_err)(env, a, b, c);                            \
     } while (0)
 #endif
 

@@ -37,7 +37,7 @@ void helper_raise_interrupt(CPUX86State *env, int intno, int next_eip_addend)
 
 void helper_raise_exception(CPUX86State *env, int exception_index)
 {
-    raise_exception(env, exception_index);
+    raise_exception(env, exception_index, 0);
 }
 
 /*

@@ -92,7 +92,8 @@ static int check_exception(CPUX86State *env, int intno, int *error_code)
  */
 static void QEMU_NORETURN raise_interrupt2(CPUX86State *env, int intno,
                                            int is_int, int error_code,
-                                           int next_eip_addend)
+                                           int next_eip_addend,
+                                           uintptr_t retaddr)
 {
     CPUState *cs = CPU(x86_env_get_cpu(env));
 

@@ -108,7 +109,7 @@ static void QEMU_NORETURN raise_interrupt2(CPUX86State *env, int intno,
     env->error_code = error_code;
     env->exception_is_int = is_int;
     env->exception_next_eip = env->eip + next_eip_addend;
-    cpu_loop_exit(cs);
+    cpu_loop_exit_restore(cs, retaddr);
 }
 
 /* shortcuts to generate exceptions */

@@ -116,16 +117,16 @@ static void QEMU_NORETURN raise_interrupt2(CPUX86State *env, int intno,
 void QEMU_NORETURN raise_interrupt(CPUX86State *env, int intno, int is_int,
                                    int error_code, int next_eip_addend)
 {
-    raise_interrupt2(env, intno, is_int, error_code, next_eip_addend);
+    raise_interrupt2(env, intno, is_int, error_code, next_eip_addend, 0);
 }
 
 void raise_exception_err(CPUX86State *env, int exception_index,
-                         int error_code)
+                         int error_code, uintptr_t retaddr)
 {
-    raise_interrupt2(env, exception_index, 0, error_code, 0);
+    raise_interrupt2(env, exception_index, 0, error_code, 0, retaddr);
 }
 
-void raise_exception(CPUX86State *env, int exception_index)
+void raise_exception(CPUX86State *env, int exception_index, uintptr_t retaddr)
 {
-    raise_interrupt2(env, exception_index, 0, 0, 0);
+    raise_interrupt2(env, exception_index, 0, 0, 0, retaddr);
 }
target-i386/fpu_helper.c +80 -66

318 lines not on this page · raw message

target-i386/helper.c +2 -2
index 4f1ddf7..8fdc884 100644
@@ -1014,7 +1014,7 @@ void breakpoint_handler(CPUState *cs)
         if (cs->watchpoint_hit->flags & BP_CPU) {
             cs->watchpoint_hit = NULL;
             if (check_hw_breakpoints(env, false)) {
-                raise_exception(env, EXCP01_DB);
+                raise_exception(env, EXCP01_DB, 0);
             } else {
                 cpu_resume_from_signal(cs, NULL);
             }

@@ -1024,7 +1024,7 @@ void breakpoint_handler(CPUState *cs)
             if (bp->pc == env->eip) {
                 if (bp->flags & BP_CPU) {
                     check_hw_breakpoints(env, true);
-                    raise_exception(env, EXCP01_DB);
+                    raise_exception(env, EXCP01_DB, 0);
                 }
                 break;
             }
target-i386/int_helper.c +16 -16
index b0d78e6..eacce79 100644
@@ -48,11 +48,11 @@ void helper_divb_AL(CPUX86State *env, target_ulong t0)
     num = (env->regs[R_EAX] & 0xffff);
     den = (t0 & 0xff);
     if (den == 0) {
-        raise_exception(env, EXCP00_DIVZ);
+        raise_exception(env, EXCP00_DIVZ, GETPC());
     }
     q = (num / den);
     if (q > 0xff) {
-        raise_exception(env, EXCP00_DIVZ);
+        raise_exception(env, EXCP00_DIVZ, GETPC());
     }
     q &= 0xff;
     r = (num % den) & 0xff;

@@ -66,11 +66,11 @@ void helper_idivb_AL(CPUX86State *env, target_ulong t0)
     num = (int16_t)env->regs[R_EAX];
     den = (int8_t)t0;
     if (den == 0) {
-        raise_exception(env, EXCP00_DIVZ);
+        raise_exception(env, EXCP00_DIVZ, GETPC());
     }
     q = (num / den);
     if (q != (int8_t)q) {
-        raise_exception(env, EXCP00_DIVZ);
+        raise_exception(env, EXCP00_DIVZ, GETPC());
     }
     q &= 0xff;
     r = (num % den) & 0xff;

@@ -84,11 +84,11 @@ void helper_divw_AX(CPUX86State *env, target_ulong t0)
     num = (env->regs[R_EAX] & 0xffff) | ((env->regs[R_EDX] & 0xffff) << 16);
     den = (t0 & 0xffff);
     if (den == 0) {
-        raise_exception(env, EXCP00_DIVZ);
+        raise_exception(env, EXCP00_DIVZ, GETPC());
     }
     q = (num / den);
     if (q > 0xffff) {
-        raise_exception(env, EXCP00_DIVZ);
+        raise_exception(env, EXCP00_DIVZ, GETPC());
     }
     q &= 0xffff;
     r = (num % den) & 0xffff;

@@ -103,11 +103,11 @@ void helper_idivw_AX(CPUX86State *env, target_ulong t0)
     num = (env->regs[R_EAX] & 0xffff) | ((env->regs[R_EDX] & 0xffff) << 16);
     den = (int16_t)t0;
     if (den == 0) {
-        raise_exception(env, EXCP00_DIVZ);
+        raise_exception(env, EXCP00_DIVZ, GETPC());
     }
     q = (num / den);
     if (q != (int16_t)q) {
-        raise_exception(env, EXCP00_DIVZ);
+        raise_exception(env, EXCP00_DIVZ, GETPC());
     }
     q &= 0xffff;
     r = (num % den) & 0xffff;

@@ -123,12 +123,12 @@ void helper_divl_EAX(CPUX86State *env, target_ulong t0)
     num = ((uint32_t)env->regs[R_EAX]) | ((uint64_t)((uint32_t)env->regs[R_EDX]) << 32);
     den = t0;
     if (den == 0) {
-        raise_exception(env, EXCP00_DIVZ);
+        raise_exception(env, EXCP00_DIVZ, GETPC());
     }
     q = (num / den);
     r = (num % den);
     if (q > 0xffffffff) {
-        raise_exception(env, EXCP00_DIVZ);
+        raise_exception(env, EXCP00_DIVZ, GETPC());
     }
     env->regs[R_EAX] = (uint32_t)q;
     env->regs[R_EDX] = (uint32_t)r;

@@ -142,12 +142,12 @@ void helper_idivl_EAX(CPUX86State *env, target_ulong t0)
     num = ((uint32_t)env->regs[R_EAX]) | ((uint64_t)((uint32_t)env->regs[R_EDX]) << 32);
     den = t0;
     if (den == 0) {
-        raise_exception(env, EXCP00_DIVZ);
+        raise_exception(env, EXCP00_DIVZ, GETPC());
     }
     q = (num / den);
     r = (num % den);
     if (q != (int32_t)q) {
-        raise_exception(env, EXCP00_DIVZ);
+        raise_exception(env, EXCP00_DIVZ, GETPC());
     }
     env->regs[R_EAX] = (uint32_t)q;
     env->regs[R_EDX] = (uint32_t)r;

@@ -379,12 +379,12 @@ void helper_divq_EAX(CPUX86State *env, target_ulong t0)
     uint64_t r0, r1;
 
     if (t0 == 0) {
-        raise_exception(env, EXCP00_DIVZ);
+        raise_exception(env, EXCP00_DIVZ, GETPC());
     }
     r0 = env->regs[R_EAX];
     r1 = env->regs[R_EDX];
     if (div64(&r0, &r1, t0)) {
-        raise_exception(env, EXCP00_DIVZ);
+        raise_exception(env, EXCP00_DIVZ, GETPC());
     }
     env->regs[R_EAX] = r0;
     env->regs[R_EDX] = r1;

@@ -395,12 +395,12 @@ void helper_idivq_EAX(CPUX86State *env, target_ulong t0)
     uint64_t r0, r1;
 
     if (t0 == 0) {
-        raise_exception(env, EXCP00_DIVZ);
+        raise_exception(env, EXCP00_DIVZ, GETPC());
     }
     r0 = env->regs[R_EAX];
     r1 = env->regs[R_EDX];
     if (idiv64(&r0, &r1, t0)) {
-        raise_exception(env, EXCP00_DIVZ);
+        raise_exception(env, EXCP00_DIVZ, GETPC());
     }
     env->regs[R_EAX] = r0;
     env->regs[R_EDX] = r1;
target-i386/mem_helper.c +18 -21
index 1aec8a5..182935b 100644
@@ -41,13 +41,14 @@ void helper_cmpxchg8b(CPUX86State *env, target_ulong a0)
     int eflags;
 
     eflags = cpu_cc_compute_all(env, CC_OP);
-    d = cpu_ldq_data(env, a0);
+    d = cpu_ldq_data_ra(env, a0, GETPC());
     if (d == (((uint64_t)env->regs[R_EDX] << 32) | (uint32_t)env->regs[R_EAX])) {
-        cpu_stq_data(env, a0, ((uint64_t)env->regs[R_ECX] << 32) | (uint32_t)env->regs[R_EBX]);
+        cpu_stq_data_ra(env, a0, ((uint64_t)env->regs[R_ECX] << 32)
+                                  | (uint32_t)env->regs[R_EBX], GETPC());
         eflags |= CC_Z;
     } else {
         /* always do the store */
-        cpu_stq_data(env, a0, d);
+        cpu_stq_data_ra(env, a0, d, GETPC());
         env->regs[R_EDX] = (uint32_t)(d >> 32);
         env->regs[R_EAX] = (uint32_t)d;
         eflags &= ~CC_Z;

@@ -62,19 +63,19 @@ void helper_cmpxchg16b(CPUX86State *env, target_ulong a0)
     int eflags;
 
     if ((a0 & 0xf) != 0) {
-        raise_exception(env, EXCP0D_GPF);
+        raise_exception(env, EXCP0D_GPF, GETPC());
     }
     eflags = cpu_cc_compute_all(env, CC_OP);
-    d0 = cpu_ldq_data(env, a0);
-    d1 = cpu_ldq_data(env, a0 + 8);
+    d0 = cpu_ldq_data_ra(env, a0, GETPC());
+    d1 = cpu_ldq_data_ra(env, a0 + 8, GETPC());
     if (d0 == env->regs[R_EAX] && d1 == env->regs[R_EDX]) {
-        cpu_stq_data(env, a0, env->regs[R_EBX]);
-        cpu_stq_data(env, a0 + 8, env->regs[R_ECX]);
+        cpu_stq_data_ra(env, a0, env->regs[R_EBX], GETPC());
+        cpu_stq_data_ra(env, a0 + 8, env->regs[R_ECX], GETPC());
         eflags |= CC_Z;
     } else {
         /* always do the store */
-        cpu_stq_data(env, a0, d0);
-        cpu_stq_data(env, a0 + 8, d1);
+        cpu_stq_data_ra(env, a0, d0, GETPC());
+        cpu_stq_data_ra(env, a0 + 8, d1, GETPC());
         env->regs[R_EDX] = d1;
         env->regs[R_EAX] = d0;
         eflags &= ~CC_Z;

@@ -87,11 +88,11 @@ void helper_boundw(CPUX86State *env, target_ulong a0, int v)
 {
     int low, high;
 
-    low = cpu_ldsw_data(env, a0);
-    high = cpu_ldsw_data(env, a0 + 2);
+    low = cpu_ldsw_data_ra(env, a0, GETPC());
+    high = cpu_ldsw_data_ra(env, a0 + 2, GETPC());
     v = (int16_t)v;
     if (v < low || v > high) {
-        raise_exception(env, EXCP05_BOUND);
+        raise_exception(env, EXCP05_BOUND, GETPC());
     }
 }
 

@@ -99,10 +100,10 @@ void helper_boundl(CPUX86State *env, target_ulong a0, int v)
 {
     int low, high;
 
-    low = cpu_ldl_data(env, a0);
-    high = cpu_ldl_data(env, a0 + 4);
+    low = cpu_ldl_data_ra(env, a0, GETPC());
+    high = cpu_ldl_data_ra(env, a0 + 4, GETPC());
     if (v < low || v > high) {
-        raise_exception(env, EXCP05_BOUND);
+        raise_exception(env, EXCP05_BOUND, GETPC());
     }
 }
 

@@ -122,11 +123,7 @@ void tlb_fill(CPUState *cs, target_ulong addr, int is_write, int mmu_idx,
         X86CPU *cpu = X86_CPU(cs);
         CPUX86State *env = &cpu->env;
 
-        if (retaddr) {
-            /* now we have a real cpu fault */
-            cpu_restore_state(cs, retaddr);
-        }
-        raise_exception_err(env, cs->exception_index, env->error_code);
+        raise_exception_err(env, cs->exception_index, env->error_code, retaddr);
     }
 }
 #endif
target-i386/misc_helper.c +6 -6
index 4aaf1e4..cb1ea8d 100644
@@ -68,7 +68,7 @@ void helper_single_step(CPUX86State *env)
     check_hw_breakpoints(env, true);
     env->dr[6] |= DR6_BS;
 #endif
-    raise_exception(env, EXCP01_DB);
+    raise_exception(env, EXCP01_DB, 0);
 }
 
 void helper_cpuid(CPUX86State *env)

@@ -187,7 +187,7 @@ void helper_rdtsc(CPUX86State *env)
     uint64_t val;
 
     if ((env->cr[4] & CR4_TSD_MASK) && ((env->hflags & HF_CPL_MASK) != 0)) {
-        raise_exception(env, EXCP0D_GPF);
+        raise_exception(env, EXCP0D_GPF, GETPC());
     }
     cpu_svm_check_intercept_param(env, SVM_EXIT_RDTSC, 0);
 

@@ -205,13 +205,13 @@ void helper_rdtscp(CPUX86State *env)
 void helper_rdpmc(CPUX86State *env)
 {
     if ((env->cr[4] & CR4_PCE_MASK) && ((env->hflags & HF_CPL_MASK) != 0)) {
-        raise_exception(env, EXCP0D_GPF);
+        raise_exception(env, EXCP0D_GPF, GETPC());
     }
     cpu_svm_check_intercept_param(env, SVM_EXIT_RDPMC, 0);
 
     /* currently unimplemented */
     qemu_log_mask(LOG_UNIMP, "x86: unimplemented rdpmc\n");
-    raise_exception_err(env, EXCP06_ILLOP, 0);
+    raise_exception_err(env, EXCP06_ILLOP, 0, GETPC());
 }
 
 #if defined(CONFIG_USER_ONLY)

@@ -556,7 +556,7 @@ void helper_hlt(CPUX86State *env, int next_eip_addend)
 void helper_monitor(CPUX86State *env, target_ulong ptr)
 {
     if ((uint32_t)env->regs[R_ECX] != 0) {
-        raise_exception(env, EXCP0D_GPF);
+        raise_exception(env, EXCP0D_GPF, GETPC());
     }
     /* XXX: store address? */
     cpu_svm_check_intercept_param(env, SVM_EXIT_MONITOR, 0);

@@ -568,7 +568,7 @@ void helper_mwait(CPUX86State *env, int next_eip_addend)
     X86CPU *cpu;
 
     if ((uint32_t)env->regs[R_ECX] != 0) {
-        raise_exception(env, EXCP0D_GPF);
+        raise_exception(env, EXCP0D_GPF, GETPC());
     }
     cpu_svm_check_intercept_param(env, SVM_EXIT_MWAIT, 0);
     env->eip += next_eip_addend;
target-i386/ops_sse.h +1 -1
index 0765073..8c8e53b 100644
@@ -483,7 +483,7 @@ void glue(helper_maskmov, SUFFIX)(CPUX86State *env, Reg *d, Reg *s,
 
     for (i = 0; i < (8 << SHIFT); i++) {
         if (s->B(i) & 0x80) {
-            cpu_stb_data(env, a0 + i, d->B(i));
+            cpu_stb_data_ra(env, a0 + i, d->B(i), GETPC());
         }
     }
 }
target-i386/seg_helper.c +363 -349

1,614 lines not on this page · raw message

target-i386/svm_helper.c +2 -2
index 429d029..f5c6c13 100644
@@ -354,7 +354,7 @@ void helper_vmrun(CPUX86State *env, int aflag, int next_eip_addend)
 void helper_vmmcall(CPUX86State *env)
 {
     cpu_svm_check_intercept_param(env, SVM_EXIT_VMMCALL, 0);
-    raise_exception(env, EXCP06_ILLOP);
+    raise_exception(env, EXCP06_ILLOP, GETPC());
 }
 
 void helper_vmload(CPUX86State *env, int aflag)

@@ -457,7 +457,7 @@ void helper_skinit(CPUX86State *env)
 {
     cpu_svm_check_intercept_param(env, SVM_EXIT_SKINIT, 0);
     /* XXX: not implemented */
-    raise_exception(env, EXCP06_ILLOP);
+    raise_exception(env, EXCP06_ILLOP, GETPC());
 }
 
 void helper_invlpga(CPUX86State *env, int aflag)
target-i386/translate.c +0 -25
index 305ce50..9238e1b 100644
@@ -2303,7 +2303,6 @@ static void gen_movl_seg_T0(DisasContext *s, int seg_reg, target_ulong cur_eip)
     if (s->pe && !s->vm86) {
         /* XXX: optimize by finding processor state dynamically */
         gen_update_cc_op(s);
-        gen_jmp_im(cur_eip);
         tcg_gen_trunc_tl_i32(cpu_tmp2_i32, cpu_T[0]);
         gen_helper_load_seg(cpu_env, tcg_const_i32(seg_reg), cpu_tmp2_i32);
         /* abort translation because the addseg value may change or

@@ -4842,21 +4841,17 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
         case 6: /* div */
             switch(ot) {
             case MO_8:
-                gen_jmp_im(pc_start - s->cs_base);
                 gen_helper_divb_AL(cpu_env, cpu_T[0]);
                 break;
             case MO_16:
-                gen_jmp_im(pc_start - s->cs_base);
                 gen_helper_divw_AX(cpu_env, cpu_T[0]);
                 break;
             default:
             case MO_32:
-                gen_jmp_im(pc_start - s->cs_base);
                 gen_helper_divl_EAX(cpu_env, cpu_T[0]);
                 break;
 #ifdef TARGET_X86_64
             case MO_64:
-                gen_jmp_im(pc_start - s->cs_base);
                 gen_helper_divq_EAX(cpu_env, cpu_T[0]);
                 break;
 #endif

@@ -4865,21 +4860,17 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
         case 7: /* idiv */
             switch(ot) {
             case MO_8:
-                gen_jmp_im(pc_start - s->cs_base);
                 gen_helper_idivb_AL(cpu_env, cpu_T[0]);
                 break;
             case MO_16:
-                gen_jmp_im(pc_start - s->cs_base);
                 gen_helper_idivw_AX(cpu_env, cpu_T[0]);
                 break;
             default:
             case MO_32:
-                gen_jmp_im(pc_start - s->cs_base);
                 gen_helper_idivl_EAX(cpu_env, cpu_T[0]);
                 break;
 #ifdef TARGET_X86_64
             case MO_64:
-                gen_jmp_im(pc_start - s->cs_base);
                 gen_helper_idivq_EAX(cpu_env, cpu_T[0]);
                 break;
 #endif

@@ -5212,7 +5203,6 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
         if (dflag == MO_64) {
             if (!(s->cpuid_ext_features & CPUID_EXT_CX16))
                 goto illegal_op;
-            gen_jmp_im(pc_start - s->cs_base);
             gen_update_cc_op(s);
             gen_lea_modrm(env, s, modrm);
             gen_helper_cmpxchg16b(cpu_env, cpu_A0);

@@ -5221,7 +5211,6 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
         {
             if (!(s->cpuid_features & CPUID_CX8))
                 goto illegal_op;
-            gen_jmp_im(pc_start - s->cs_base);
             gen_update_cc_op(s);
             gen_lea_modrm(env, s, modrm);
             gen_helper_cmpxchg8b(cpu_env, cpu_A0);

@@ -5838,7 +5827,6 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
                 break;
             case 0x0c: /* fldenv mem */
                 gen_update_cc_op(s);
-                gen_jmp_im(pc_start - s->cs_base);
                 gen_helper_fldenv(cpu_env, cpu_A0, tcg_const_i32(dflag - 1));
                 break;
             case 0x0d: /* fldcw mem */

@@ -5848,7 +5836,6 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
                 break;
             case 0x0e: /* fnstenv mem */
                 gen_update_cc_op(s);
-                gen_jmp_im(pc_start - s->cs_base);
                 gen_helper_fstenv(cpu_env, cpu_A0, tcg_const_i32(dflag - 1));
                 break;
             case 0x0f: /* fnstcw mem */

@@ -5858,23 +5845,19 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
                 break;
             case 0x1d: /* fldt mem */
                 gen_update_cc_op(s);
-                gen_jmp_im(pc_start - s->cs_base);
                 gen_helper_fldt_ST0(cpu_env, cpu_A0);
                 break;
             case 0x1f: /* fstpt mem */
                 gen_update_cc_op(s);
-                gen_jmp_im(pc_start - s->cs_base);
                 gen_helper_fstt_ST0(cpu_env, cpu_A0);
                 gen_helper_fpop(cpu_env);
                 break;
             case 0x2c: /* frstor mem */
                 gen_update_cc_op(s);
-                gen_jmp_im(pc_start - s->cs_base);
                 gen_helper_frstor(cpu_env, cpu_A0, tcg_const_i32(dflag - 1));
                 break;
             case 0x2e: /* fnsave mem */
                 gen_update_cc_op(s);
-                gen_jmp_im(pc_start - s->cs_base);
                 gen_helper_fsave(cpu_env, cpu_A0, tcg_const_i32(dflag - 1));
                 break;
             case 0x2f: /* fnstsw mem */

@@ -5889,7 +5872,6 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
                 break;
             case 0x3e: /* fbstp */
                 gen_update_cc_op(s);
-                gen_jmp_im(pc_start - s->cs_base);
                 gen_helper_fbst_ST0(cpu_env, cpu_A0);
                 gen_helper_fpop(cpu_env);
                 break;

@@ -5925,7 +5907,6 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
                 case 0: /* fnop */
                     /* check exceptions (FreeBSD FPU probe) */
                     gen_update_cc_op(s);
-                    gen_jmp_im(pc_start - s->cs_base);
                     gen_helper_fwait(cpu_env);
                     break;
                 default:

@@ -6896,7 +6877,6 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
             gen_exception(s, EXCP07_PREX, pc_start - s->cs_base);
         } else {
             gen_update_cc_op(s);
-            gen_jmp_im(pc_start - s->cs_base);
             gen_helper_fwait(cpu_env);
         }
         break;

@@ -6980,7 +6960,6 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
             goto illegal_op;
         gen_op_mov_v_reg(ot, cpu_T[0], reg);
         gen_lea_modrm(env, s, modrm);
-        gen_jmp_im(pc_start - s->cs_base);
         tcg_gen_trunc_tl_i32(cpu_tmp2_i32, cpu_T[0]);
         if (ot == MO_16) {
             gen_helper_boundw(cpu_env, cpu_A0, cpu_tmp2_i32);

@@ -7172,7 +7151,6 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
             } else {
                 gen_svm_check_intercept(s, pc_start, SVM_EXIT_LDTR_WRITE);
                 gen_ldst_modrm(env, s, modrm, MO_16, OR_TMP0, 0);
-                gen_jmp_im(pc_start - s->cs_base);
                 tcg_gen_trunc_tl_i32(cpu_tmp2_i32, cpu_T[0]);
                 gen_helper_lldt(cpu_env, cpu_tmp2_i32);
             }

@@ -7193,7 +7171,6 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
             } else {
                 gen_svm_check_intercept(s, pc_start, SVM_EXIT_TR_WRITE);
                 gen_ldst_modrm(env, s, modrm, MO_16, OR_TMP0, 0);
-                gen_jmp_im(pc_start - s->cs_base);
                 tcg_gen_trunc_tl_i32(cpu_tmp2_i32, cpu_T[0]);
                 gen_helper_ltr(cpu_env, cpu_tmp2_i32);
             }

@@ -7727,7 +7704,6 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
             }
             gen_lea_modrm(env, s, modrm);
             gen_update_cc_op(s);
-            gen_jmp_im(pc_start - s->cs_base);
             gen_helper_fxsave(cpu_env, cpu_A0, tcg_const_i32(dflag == MO_64));
             break;
         case 1: /* fxrstor */

@@ -7740,7 +7716,6 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
             }
             gen_lea_modrm(env, s, modrm);
             gen_update_cc_op(s);
-            gen_jmp_im(pc_start - s->cs_base);
             gen_helper_fxrstor(cpu_env, cpu_A0, tcg_const_i32(dflag == MO_64));
             break;
         case 2: /* ldmxcsr */
#10 lore raw

Re: [Qemu-devel] [PATCH v4 4/5] target-i386: fix memory operations in helpers

Quoted 5 lines · Pavel Dovgalyuk

This patch passes TB return address into softmmu functions that are invoked from target helpers. This allows correct PC and icount recovering while handling MMU faults.

Signed-off-by Pavel Dovgalyuk<[email protected]>
Quoted 14 lines
---
  target-i386/cc_helper.c   |    2
  target-i386/cpu.h         |    5
  target-i386/excp_helper.c |   21 +
  target-i386/fpu_helper.c  |  146 +++++----
  target-i386/helper.c      |    4
  target-i386/int_helper.c  |   32 +-
  target-i386/mem_helper.c  |   39 +-
  target-i386/misc_helper.c |   12 -
  target-i386/ops_sse.h     |    2
  target-i386/seg_helper.c  |  712 +++++++++++++++++++++++----------------------
  target-i386/svm_helper.c  |    4
  target-i386/translate.c   |   25 --
  12 files changed, 503 insertions(+), 501 deletions(-)

This patch is too big. It really needs to be split into several patches.

(1) Introduce raise_exception_ra that accepts the GETPC argument, so that
(a) you can stage the changes in and
(b) most of the seg_helper changes from do_interrupt et al aren't needed.

(2) Stage in fixes for each of the (groups of) helpers callable from translate.c. E.g. fld, fst in one group, division in another.

And while this patch set fixes icount, do you have any evidence that we ever got incorrect PC values handling mmu faults?

r~

author
#11 lore raw

Re: [Qemu-devel] [PATCH v4 4/5] target-i386: fix memory operations in helpers

Quoted 24 lines

From: Richard Henderson [mailto:[email protected]] On Behalf Of Richard Henderson

Quoted 5 lines · Pavel Dovgalyuk

This patch passes TB return address into softmmu functions that are invoked from target helpers. This allows correct PC and icount recovering while handling MMU faults.

Signed-off-by Pavel Dovgalyuk<[email protected]>
Quoted 14 lines
---
  target-i386/cc_helper.c   |    2
  target-i386/cpu.h         |    5
  target-i386/excp_helper.c |   21 +
  target-i386/fpu_helper.c  |  146 +++++----
  target-i386/helper.c      |    4
  target-i386/int_helper.c  |   32 +-
  target-i386/mem_helper.c  |   39 +-
  target-i386/misc_helper.c |   12 -
  target-i386/ops_sse.h     |    2
  target-i386/seg_helper.c  |  712 +++++++++++++++++++++++----------------------
  target-i386/svm_helper.c  |    4
  target-i386/translate.c   |   25 --
  12 files changed, 503 insertions(+), 501 deletions(-)

This patch is too big. It really needs to be split into several patches.

Quoted 7 lines

(1) Introduce raise_exception_ra that accepts the GETPC argument, so that
(a) you can stage the changes in and
(b) most of the seg_helper changes from do_interrupt et al aren't needed.

(2) Stage in fixes for each of the (groups of) helpers callable from translate.c. E.g. fld, fst in one group, division in another.

Ok, I'll split this patch.

Quoted 2 lines

And while this patch set fixes icount, do you have any evidence that we ever got incorrect PC values handling mmu faults?

Indeed. I described it here: http://lists.nongnu.org/archive/html/qemu-devel/2014-09/msg02960.html

One of the applications used maskmov_xmm, which caused MMU fault. Then execution of TB restarted instead of continuing from maskmov.

Pavel Dovgalyuk

author
#13 lore raw

PATCH v4 5/5 target-ppc: exceptions handling in icount mode

In reply to Pavel Dovgalyuk

This patch fixes exception handling in PowerPC.
Instructions generate several types of exceptions.
When exception is generated, it breaks the execution of the current translation block. Implementation of the exceptions handling does not correctly restore icount for the instruction which caused the exception. In most cases icount will be decreased by the value equal to the size of TB. This patch passes pointer to the translation block internals to the exception handler. It allows correct restoring of the icount value.

Signed-off-by Pavel Dovgalyuk <[email protected]>
---
 target-ppc/cpu.h             |    3 +
 target-ppc/excp_helper.c     |   38 ++++++--
 target-ppc/fpu_helper.c      |  191 ++++++++++++++++++++++--------------------
 target-ppc/helper.h          |    1 
 target-ppc/mem_helper.c      |    6 +
 target-ppc/misc_helper.c     |    8 +-
 target-ppc/mmu-hash64.c      |   12 +--
 target-ppc/mmu_helper.c      |   18 ++--
 target-ppc/timebase_helper.c |   20 ++--
 target-ppc/translate.c       |   84 +-----------------
 10 files changed, 167 insertions(+), 214 deletions(-)
target-ppc/cpu.h +3 -0
index c05c503..27aaff8 100644
@@ -2328,4 +2328,7 @@ int ppc_get_vcpu_dt_id(PowerPCCPU *cpu);
  */
 PowerPCCPU *ppc_get_vcpu_by_dt_id(int cpu_dt_id);
 
+void raise_exception_err(CPUPPCState *env, uint32_t exception,
+                         uint32_t error_code, uintptr_t pc);
+
 #endif /* !defined (__CPU_PPC_H__) */
target-ppc/excp_helper.c +29 -9
index b803475..5402e0d 100644
@@ -846,8 +846,8 @@ static void cpu_dump_rfi(target_ulong RA, target_ulong msr)
 /*****************************************************************************/
 /* Exceptions processing helpers */
 
-void helper_raise_exception_err(CPUPPCState *env, uint32_t exception,
-                                uint32_t error_code)
+void raise_exception_err(CPUPPCState *env, uint32_t exception,
+                         uint32_t error_code, uintptr_t pc)
 {
     CPUState *cs = CPU(ppc_env_get_cpu(env));
 

@@ -856,12 +856,29 @@ void helper_raise_exception_err(CPUPPCState *env, uint32_t exception,
 #endif
     cs->exception_index = exception;
     env->error_code = error_code;
-    cpu_loop_exit(cs);
+    cpu_loop_exit_restore(cs, pc);
+}
+
+void helper_raise_exception_err(CPUPPCState *env, uint32_t exception,
+                                uint32_t error_code)
+{
+    raise_exception_err(env, exception, error_code, GETPC());
+}
+
+void helper_raise_exception_end(CPUPPCState *env, uint32_t exception,
+                                uint32_t error_code)
+{
+    raise_exception_err(env, exception, error_code, 0);
 }
 
 void helper_raise_exception(CPUPPCState *env, uint32_t exception)
 {
-    helper_raise_exception_err(env, exception, 0);
+    raise_exception_err(env, exception, 0, GETPC());
+}
+
+static void raise_exception(CPUPPCState *env, uint32_t exception, uintptr_t pc)
+{
+    raise_exception_err(env, exception, 0, pc);
 }
 
 #if !defined(CONFIG_USER_ONLY)

@@ -873,7 +890,8 @@ void helper_store_msr(CPUPPCState *env, target_ulong val)
     if (val != 0) {
         cs = CPU(ppc_env_get_cpu(env));
         cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
-        helper_raise_exception(env, val);
+        /* nip is updated by generated code */
+        raise_exception(env, val, 0);
     }
 }
 

@@ -971,8 +989,9 @@ void helper_tw(CPUPPCState *env, target_ulong arg1, target_ulong arg2,
                   ((int32_t)arg1 == (int32_t)arg2 && (flags & 0x04)) ||
                   ((uint32_t)arg1 < (uint32_t)arg2 && (flags & 0x02)) ||
                   ((uint32_t)arg1 > (uint32_t)arg2 && (flags & 0x01))))) {
-        helper_raise_exception_err(env, POWERPC_EXCP_PROGRAM,
-                                   POWERPC_EXCP_TRAP);
+        /* nip is updated in TB */
+        raise_exception_err(env, POWERPC_EXCP_PROGRAM,
+                            POWERPC_EXCP_TRAP, 0);
     }
 }
 

@@ -985,8 +1004,9 @@ void helper_td(CPUPPCState *env, target_ulong arg1, target_ulong arg2,
                   ((int64_t)arg1 == (int64_t)arg2 && (flags & 0x04)) ||
                   ((uint64_t)arg1 < (uint64_t)arg2 && (flags & 0x02)) ||
                   ((uint64_t)arg1 > (uint64_t)arg2 && (flags & 0x01))))) {
-        helper_raise_exception_err(env, POWERPC_EXCP_PROGRAM,
-                                   POWERPC_EXCP_TRAP);
+        /* nip is updated in TB */
+        raise_exception_err(env, POWERPC_EXCP_PROGRAM,
+                            POWERPC_EXCP_TRAP, 0);
     }
 }
 #endif
target-ppc/fpu_helper.c +98 -93

645 lines not on this page · raw message

target-ppc/helper.h +1 -0
index 869be15..fee2dd1 100644
@@ -1,4 +1,5 @@
 DEF_HELPER_3(raise_exception_err, void, env, i32, i32)
+DEF_HELPER_3(raise_exception_end, void, env, i32, i32)
 DEF_HELPER_2(raise_exception, void, env, i32)
 DEF_HELPER_4(tw, void, env, tl, tl, i32)
 #if defined(TARGET_PPC64)
target-ppc/mem_helper.c +3 -3
index 6d37dae..1c67562 100644
@@ -102,9 +102,9 @@ void helper_lswx(CPUPPCState *env, target_ulong addr, uint32_t reg,
     if (likely(xer_bc != 0)) {
         if (unlikely((ra != 0 && reg < ra && (reg + xer_bc) > ra) ||
                      (reg < rb && (reg + xer_bc) > rb))) {
-            helper_raise_exception_err(env, POWERPC_EXCP_PROGRAM,
-                                       POWERPC_EXCP_INVAL |
-                                       POWERPC_EXCP_INVAL_LSWX);
+            raise_exception_err(env, POWERPC_EXCP_PROGRAM,
+                                POWERPC_EXCP_INVAL |
+                                POWERPC_EXCP_INVAL_LSWX, GETPC());
         } else {
             helper_lsw(env, addr, xer_bc, reg);
         }
target-ppc/misc_helper.c +4 -4
index 6b12ca8..1596187 100644
@@ -37,7 +37,7 @@ void helper_store_dump_spr(CPUPPCState *env, uint32_t sprn)
 
 #ifdef TARGET_PPC64
 static void raise_fu_exception(CPUPPCState *env, uint32_t bit,
-                               uint32_t sprn, uint32_t cause)
+                               uint32_t sprn, uint32_t cause, uintptr_t retaddr)
 {
     qemu_log("Facility SPR %d is unavailable (SPR FSCR:%d)\n", sprn, bit);
 

@@ -45,7 +45,7 @@ static void raise_fu_exception(CPUPPCState *env, uint32_t bit,
     cause &= FSCR_IC_MASK;
     env->spr[SPR_FSCR] |= (target_ulong)cause << FSCR_IC_POS;
 
-    helper_raise_exception_err(env, POWERPC_EXCP_FU, 0);
+    raise_exception_err(env, POWERPC_EXCP_FU, 0, retaddr);
 }
 #endif
 

@@ -57,7 +57,7 @@ void helper_fscr_facility_check(CPUPPCState *env, uint32_t bit,
         /* Facility is enabled, continue */
         return;
     }
-    raise_fu_exception(env, bit, sprn, cause);
+    raise_fu_exception(env, bit, sprn, cause, GETPC());
 #endif
 }
 

@@ -69,7 +69,7 @@ void helper_msr_facility_check(CPUPPCState *env, uint32_t bit,
         /* Facility is enabled, continue */
         return;
     }
-    raise_fu_exception(env, bit, sprn, cause);
+    raise_fu_exception(env, bit, sprn, cause, GETPC());
 #endif
 }
target-ppc/mmu-hash64.c +6 -6
index 7df6ede..7f22706 100644
@@ -197,8 +197,8 @@ static int ppc_load_slb_vsid(CPUPPCState *env, target_ulong rb,
 void helper_store_slb(CPUPPCState *env, target_ulong rb, target_ulong rs)
 {
     if (ppc_store_slb(env, rb, rs) < 0) {
-        helper_raise_exception_err(env, POWERPC_EXCP_PROGRAM,
-                                   POWERPC_EXCP_INVAL);
+        raise_exception_err(env, POWERPC_EXCP_PROGRAM,
+                            POWERPC_EXCP_INVAL, GETPC());
     }
 }
 

@@ -207,8 +207,8 @@ target_ulong helper_load_slb_esid(CPUPPCState *env, target_ulong rb)
     target_ulong rt = 0;
 
     if (ppc_load_slb_esid(env, rb, &rt) < 0) {
-        helper_raise_exception_err(env, POWERPC_EXCP_PROGRAM,
-                                   POWERPC_EXCP_INVAL);
+        raise_exception_err(env, POWERPC_EXCP_PROGRAM,
+                            POWERPC_EXCP_INVAL, GETPC());
     }
     return rt;
 }

@@ -218,8 +218,8 @@ target_ulong helper_load_slb_vsid(CPUPPCState *env, target_ulong rb)
     target_ulong rt = 0;
 
     if (ppc_load_slb_vsid(env, rb, &rt) < 0) {
-        helper_raise_exception_err(env, POWERPC_EXCP_PROGRAM,
-                                   POWERPC_EXCP_INVAL);
+        raise_exception_err(env, POWERPC_EXCP_PROGRAM,
+                            POWERPC_EXCP_INVAL, GETPC());
     }
     return rt;
 }
target-ppc/mmu_helper.c +7 -11
index 527c6ad..acd266f 100644
@@ -2635,9 +2635,9 @@ void helper_booke206_tlbwe(CPUPPCState *env)
     tlb = booke206_cur_tlb(env);
 
     if (!tlb) {
-        helper_raise_exception_err(env, POWERPC_EXCP_PROGRAM,
-                                   POWERPC_EXCP_INVAL |
-                                   POWERPC_EXCP_INVAL_INVAL);
+        raise_exception_err(env, POWERPC_EXCP_PROGRAM,
+                            POWERPC_EXCP_INVAL |
+                            POWERPC_EXCP_INVAL_INVAL, GETPC());
     }
 
     /* check that we support the targeted size */

@@ -2645,9 +2645,9 @@ void helper_booke206_tlbwe(CPUPPCState *env)
     size_ps = booke206_tlbnps(env, tlbn);
     if ((env->spr[SPR_BOOKE_MAS1] & MAS1_VALID) && (tlbncfg & TLBnCFG_AVAIL) &&
         !(size_ps & (1 << size_tlb))) {
-        helper_raise_exception_err(env, POWERPC_EXCP_PROGRAM,
-                                   POWERPC_EXCP_INVAL |
-                                   POWERPC_EXCP_INVAL_INVAL);
+        raise_exception_err(env, POWERPC_EXCP_PROGRAM,
+                            POWERPC_EXCP_INVAL |
+                            POWERPC_EXCP_INVAL_INVAL, GETPC());
     }
 
     if (msr_gs) {

@@ -2924,10 +2924,6 @@ void tlb_fill(CPUState *cs, target_ulong addr, int is_write, int mmu_idx,
         ret = cpu_ppc_handle_mmu_fault(env, addr, is_write, mmu_idx);
     }
     if (unlikely(ret != 0)) {
-        if (likely(retaddr)) {
-            /* now we have a real cpu fault */
-            cpu_restore_state(cs, retaddr);
-        }
-        helper_raise_exception_err(env, cs->exception_index, env->error_code);
+        raise_exception_err(env, cs->exception_index, env->error_code, retaddr);
     }
 }
target-ppc/timebase_helper.c +10 -10
index 865dcbe..229523c 100644
@@ -131,14 +131,14 @@ target_ulong helper_load_dcr(CPUPPCState *env, target_ulong dcrn)
 
     if (unlikely(env->dcr_env == NULL)) {
         qemu_log("No DCR environment\n");
-        helper_raise_exception_err(env, POWERPC_EXCP_PROGRAM,
-                                   POWERPC_EXCP_INVAL |
-                                   POWERPC_EXCP_INVAL_INVAL);
+        raise_exception_err(env, POWERPC_EXCP_PROGRAM,
+                            POWERPC_EXCP_INVAL |
+                            POWERPC_EXCP_INVAL_INVAL, GETPC());
     } else if (unlikely(ppc_dcr_read(env->dcr_env,
                                      (uint32_t)dcrn, &val) != 0)) {
         qemu_log("DCR read error %d %03x\n", (uint32_t)dcrn, (uint32_t)dcrn);
-        helper_raise_exception_err(env, POWERPC_EXCP_PROGRAM,
-                                   POWERPC_EXCP_INVAL | POWERPC_EXCP_PRIV_REG);
+        raise_exception_err(env, POWERPC_EXCP_PROGRAM,
+                            POWERPC_EXCP_INVAL | POWERPC_EXCP_PRIV_REG, GETPC());
     }
     return val;
 }

@@ -147,13 +147,13 @@ void helper_store_dcr(CPUPPCState *env, target_ulong dcrn, target_ulong val)
 {
     if (unlikely(env->dcr_env == NULL)) {
         qemu_log("No DCR environment\n");
-        helper_raise_exception_err(env, POWERPC_EXCP_PROGRAM,
-                                   POWERPC_EXCP_INVAL |
-                                   POWERPC_EXCP_INVAL_INVAL);
+        raise_exception_err(env, POWERPC_EXCP_PROGRAM,
+                            POWERPC_EXCP_INVAL |
+                            POWERPC_EXCP_INVAL_INVAL, GETPC());
     } else if (unlikely(ppc_dcr_write(env->dcr_env, (uint32_t)dcrn,
                                       (uint32_t)val) != 0)) {
         qemu_log("DCR write error %d %03x\n", (uint32_t)dcrn, (uint32_t)dcrn);
-        helper_raise_exception_err(env, POWERPC_EXCP_PROGRAM,
-                                   POWERPC_EXCP_INVAL | POWERPC_EXCP_PRIV_REG);
+        raise_exception_err(env, POWERPC_EXCP_PROGRAM,
+                            POWERPC_EXCP_INVAL | POWERPC_EXCP_PRIV_REG, GETPC());
     }
 }
target-ppc/translate.c +6 -78

312 lines not on this page · raw message

author
#14 lore raw
In reply to Pavel Dovgalyuk

Forgot to fix the subject.
These patches also fix exceptions handling for PowerPC.

Pavel Dovgalyuk

Quoted 143 lines

-----Original Message-----
From: Pavel Dovgalyuk [mailto:[email protected]]
Sent: Monday, June 29, 2015 10:23 AM
To: [email protected]
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: [PATCH v4 0/5] Fix exceptions handling for MIPS and i386

QEMU targets ISAs contain instruction that can break the execution flow with exceptions. When exception breaks the execution of the translation block it may corrupt PC and icount values.

This set of patches fixes exception handling for MIPS and i386 targets.

Incorrect execution for i386 is causes by exceptions raised by MMU functions. MMU helper functions are called from generated code and other helper functions. In both cases they try to get function's return address for restoring virtual CPU state.

When MMU helper is called from some other helper function (like helper_maskmov_xmm) through cpu_st* function, the return address will point to that helper. That is why CPU state cannot be restored in the case of MMU fault.

This bug can occur when maskmov instruction is located in the middle of the translation block.

Execution sequence for this example:

TB start:
PC1: instr1
instr2
PC2: maskmov <page fault>
<page fault processing>
PC1: instr1
instr2
maskmov

At the start of TB execution guest PC points to instr1. When page fault occurs QEMU tries to restore guest PC (which should be equal to PC2). It reads host PC from the call stack and checks whether it points to TB or not. Bug in ldst helpers implementation provides incorrect host PC, which is not located within the TB. That's why QEMU cannot recover guest PC and it remains the same (PC1). After page fault processing QEMU restarts TB and executes instr1 and instr2 for the second time, because guest PC was not recovered.

Bugs in MIPS helper functions do not break the execution in regular TCG mode, because PC value is updated before calling the functions that can raise an exception. But icount value cannot be updated this way. Therefore exceptions make execution in icount mode non-determinisic.
In icount mode every translation block looks as follows:

if icount < n then exit
icount -= n
instr1
instr2
...
instrn
exit

When one of these instructions initiates an exception, icount should be restored and adjusted number of instructions should be subtracted from icount instead of initial n.

tlb_fill function passes retaddr to raise_exception, which allows restoring current instructions in TB and correct icount calculation.

When exception triggered with other function (e.g. by embedding call to exception raising helper into TB), then PC is not passed as retaddr and correct icount is not recovered. In such cases icount will be decreased by the value equal to the size of TB.

This behavior leads to incorrect values of virtual clock and non-deterministic execution of the code.

These patches passes pointer to the translation block code to the exception handler. It allows correct restoring of PC and icount values.

v4 changes:
* Fixed exceptions handling for PowerPC
* Fixed passing of mmu_idx into helpers (as suggested by Aurelien Jarno)
* Added cpu_loop_exit_restore function (as suggested by Aurelien Jarno)
* Fixed exceptions handling in compare helper functions for MIPS (as suggested by Aurelien
Jarno)
* Removed several CPU state saving calls for MIPS (as suggested by Aurelien Jarno)

v3 changes:
* Modified exception handling for syscall (as suggested by Aurelien Jarno)
* Removed redundant calls to save_cpu_state (as suggested by Aurelien Jarno)
* Removed helper_call* functions from softmmu (as suggested by Paolo Bonzini)

v2 changes:
* Added softmmu functions to pass TB return value into memory operations handlers
* Fixed memory operations handling for MIPS
* Disabled updates of the PC that are overridden with cpu_restore_state
* Fixed memory operations and exceptions invoked by i386 helpers

---

Pavel Dovgalyuk (5):
softmmu: add helper function to pass through retaddr
cpu-exec: introduce loop exit with restore function
target-mips: improve exceptions handling
target-i386: fix memory operations in helpers
target-ppc: exceptions handling in icount mode

 cpu-exec.c                       |    9
 include/exec/cpu_ldst_template.h |   59 +++
 include/exec/exec-all.h          |    1
 softmmu_template.h               |    6
 target-i386/cc_helper.c          |    2
 target-i386/cpu.h                |    5
 target-i386/excp_helper.c        |   21 +
 target-i386/fpu_helper.c         |  146 ++++----
 target-i386/helper.c             |    4
 target-i386/int_helper.c         |   32 +-
 target-i386/mem_helper.c         |   39 +-
 target-i386/misc_helper.c        |   12 -
 target-i386/ops_sse.h            |    2
 target-i386/seg_helper.c         |  712 +++++++++++++++++++-------------------
 target-i386/svm_helper.c         |    4
 target-i386/translate.c          |   25 -
 target-mips/cpu.h                |   23 +
 target-mips/helper.h             |    1
 target-mips/msa_helper.c         |  158 +++++---
 target-mips/op_helper.c          |  179 ++++------
 target-mips/translate.c          |  372 +++++++++-----------
 target-ppc/cpu.h                 |    3
 target-ppc/excp_helper.c         |   38 ++
 target-ppc/fpu_helper.c          |  191 +++++-----
 target-ppc/helper.h              |    1
 target-ppc/mem_helper.c          |    6
 target-ppc/misc_helper.c         |    8
 target-ppc/mmu-hash64.c          |   12 -
 target-ppc/mmu_helper.c          |   18 -
 target-ppc/timebase_helper.c     |   20 +
 target-ppc/translate.c           |   84 ----
 tcg/tcg.h                        |   23 +
 32 files changed, 1122 insertions(+), 1094 deletions(-)

--
Pavel Dovgalyuk