Re: [Qemu-devel] [PATCH] MIPS: exceptions handling in icount mode

[Aurelien Jarno <aurelien@aurel32.net>]

On 2015-06-10 11:33, Pavel Dovgalyuk wrote:
> This patch fixes exception handling in MIPS.
> 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 in MIPS 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.

I don't think it is correct. There is no real point of always doing
retranslation for an exception triggered from the helpers, especially
when the CPU state has been saved before anyway?

> This patch passes pointer to the translation block internals to the exception
> handler. It allows correct restoring of the icount value.

Your patch doesn't do that for all the helpers, for example all the
memory access helpers. It probably improves the situation but therefore
doesn't fix it.

From my point of view, it looks like the problem is actually elsewhere
in the common icount code. Do we know if it works correctly on other
emulated architectures? Also do you have a quick example to reproduce
the issue?


> Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> ---
>  target-mips/cpu.h        |   28 +++++++++++++++++++++++++
>  target-mips/msa_helper.c |    5 +++-
>  target-mips/op_helper.c  |   52 +++++++++++-----------------------------------
>  target-mips/translate.c  |    2 ++
>  4 files changed, 45 insertions(+), 42 deletions(-)

[ snip ]

> diff --git a/target-mips/translate.c b/target-mips/translate.c
> index fd063a2..9c2ff7c 100644
> --- a/target-mips/translate.c
> +++ b/target-mips/translate.c
> @@ -1675,6 +1675,7 @@ generate_exception_err (DisasContext *ctx, int excp, int err)
>      TCGv_i32 terr = tcg_const_i32(err);
>      save_cpu_state(ctx, 1);
>      gen_helper_raise_exception_err(cpu_env, texcp, terr);
> +    ctx->bstate = BS_STOP;
>      tcg_temp_free_i32(terr);
>      tcg_temp_free_i32(texcp);
>  }
> @@ -1684,6 +1685,7 @@ generate_exception (DisasContext *ctx, int excp)
>  {
>      save_cpu_state(ctx, 1);
>      gen_helper_0e0i(raise_exception, excp);
> +    ctx->bstate = BS_STOP;
>  }
>  

Why do we need to stop the translation here? The exception might be
conditional (for example for ADDU or SUBU).

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