Re: [Qemu-devel] [PATCH v2 2/3] target-mips: exceptions handling in icount mode

[Aurelien Jarno]

On 2015-06-17 15:42, Pavel Dovgalyuk wrote:
> This patch fixes 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.
> 
> Signed-off-by: Pavel Dovgalyuk <address@hidden>
> ---
>  target-mips/cpu.h        |   28 +++++++
>  target-mips/helper.h     |    1 
>  target-mips/msa_helper.c |    5 +
>  target-mips/op_helper.c  |  183 
> ++++++++++++++++++++++------------------------
>  target-mips/translate.c  |   46 ++++++------
>  5 files changed, 141 insertions(+), 122 deletions(-)

[ snip ]

> diff --git a/target-mips/op_helper.c b/target-mips/op_helper.c
> index 73a8e45..2815c60 100644
> --- a/target-mips/op_helper.c
> +++ b/target-mips/op_helper.c
> @@ -30,41 +30,23 @@ static inline void cpu_mips_tlb_flush (CPUMIPSState *env, 
> int flush_global);
>  
> /*****************************************************************************/
>  /* Exceptions processing helpers */
>  
> -static inline void QEMU_NORETURN do_raise_exception_err(CPUMIPSState *env,
> -                                                        uint32_t exception,
> -                                                        int error_code,
> -                                                        uintptr_t pc)
> +void helper_raise_exception_err(CPUMIPSState *env, uint32_t exception,
> +                                int error_code)
>  {
> -    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;
> -
> -    if (pc) {
> -        /* now we have a real cpu fault */
> -        cpu_restore_state(cs, pc);
> -    }
> -
> -    cpu_loop_exit(cs);
> +    do_raise_exception_err(env, exception, error_code, GETPC());
>  }
>  
> -static inline void QEMU_NORETURN do_raise_exception(CPUMIPSState *env,
> -                                                    uint32_t exception,
> -                                                    uintptr_t pc)
> +void helper_raise_exception(CPUMIPSState *env, uint32_t exception)
>  {
> -    do_raise_exception_err(env, exception, 0, pc);
> +    do_raise_exception(env, exception, GETPC());
>  }

raise_exception is used to implement the SYSCALL instruction on MIPS.
With this change, this mean that each TB containing a syscall will have
to be translated at least twice, probably more. That's not something
acceptable performance wise.

> diff --git a/target-mips/translate.c b/target-mips/translate.c
> index fd063a2..0de9244 100644
> --- a/target-mips/translate.c
> +++ b/target-mips/translate.c
> @@ -1673,7 +1673,7 @@ generate_exception_err (DisasContext *ctx, int excp, 
> int err)
>  {
>      TCGv_i32 texcp = tcg_const_i32(excp);
>      TCGv_i32 terr = tcg_const_i32(err);
> -    save_cpu_state(ctx, 1);
> +    save_cpu_state(ctx, 0);

If retranslation is used, you don't even need to save the branch status,
restore_state_to_opc can restore it.

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