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

[Aurelien Jarno]

On 2015-06-29 10:23, Pavel Dovgalyuk wrote:
> 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 <address@hidden>
> ---
>  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 ]

> diff --git a/target-mips/op_helper.c b/target-mips/op_helper.c
> index 73a8e45..b562384 100644
> --- a/target-mips/op_helper.c
> +++ b/target-mips/op_helper.c

[ snip ]

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

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

> diff --git a/target-mips/translate.c b/target-mips/translate.c
> index fd063a2..85d374c 100644
> --- a/target-mips/translate.c
> +++ b/target-mips/translate.c
> @@ -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.

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
address@hidden                 http://www.aurel32.net