Re: [Qemu-devel] [PATCH 1/2] target-arm: Unify AArch32 exception return generating functions

[Peter Maydell]

On 19 April 2016 at 19:07, Sergey Sorokin <address@hidden> wrote:
> There is a duplicating code in AArch32 exception return implementation.
> The patch rearranges the code to avoid this.
>
> Signed-off-by: Sergey Sorokin <address@hidden>
> ---
>  target-arm/translate.c | 36 +++++++++++++++++++++---------------
>  1 file changed, 21 insertions(+), 15 deletions(-)
>
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index 940ec8d..68671b7 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -4349,24 +4349,27 @@ static void gen_mrs_banked(DisasContext *s, int r, 
> int sysm, int rn)
>      s->is_jmp = DISAS_UPDATE;
>  }
>
> -/* Generate an old-style exception return. Marks pc as dead. */
> -static void gen_exception_return(DisasContext *s, TCGv_i32 pc)
> +/* Generate an exception return. Marks both values as dead. */
> +static void gen_exception_return_internal(DisasContext *s, TCGv_i32 pc,
> +                                          TCGv_i32 cpsr)
>  {
> -    TCGv_i32 tmp;
> +    gen_helper_cpsr_write_eret(cpu_env, cpsr);
> +    tcg_temp_free_i32(cpsr);
>      store_reg(s, 15, pc);
> -    tmp = load_cpu_field(spsr);
> -    gen_helper_cpsr_write_eret(cpu_env, tmp);
> -    tcg_temp_free_i32(tmp);
>      s->is_jmp = DISAS_JUMP;
>  }
>
> +/* Generate an old-style exception return. Marks pc as dead. */
> +static void gen_exception_return(DisasContext *s, TCGv_i32 pc)
> +{
> +    TCGv_i32 tmp = load_cpu_field(spsr);
> +    gen_exception_return_internal(s, pc, tmp);
> +}
> +
>  /* Generate a v6 exception return.  Marks both values as dead.  */
>  static void gen_rfe(DisasContext *s, TCGv_i32 pc, TCGv_i32 cpsr)
>  {
> -    gen_helper_cpsr_write_eret(cpu_env, cpsr);
> -    tcg_temp_free_i32(cpsr);
> -    store_reg(s, 15, pc);
> -    s->is_jmp = DISAS_JUMP;
> +    gen_exception_return_internal(s, pc, cpsr);
>  }

This rearrangement of code is OK.

>  static void gen_nop_hint(DisasContext *s, int val)
> @@ -9378,11 +9381,14 @@ static void disas_arm_insn(DisasContext *s, unsigned 
> int insn)
>                      store_reg(s, rn, loaded_var);
>                  }
>                  if (exc_return) {
> -                    /* Restore CPSR from SPSR.  */
> -                    tmp = load_cpu_field(spsr);
> -                    gen_helper_cpsr_write_eret(cpu_env, tmp);
> -                    tcg_temp_free_i32(tmp);
> -                    s->is_jmp = DISAS_JUMP;
> +                    /* Restore CPSR from SPSR.
> +                     * Since gen_exception_return() function updates r15
> +                     * we need just pass current r15 value into it.
> +                     */
> +                    tmp = tcg_temp_new_i32();
> +                    tcg_gen_mov_i32(tmp, cpu_R[15]);
> +                    /* It marks tmp as dead. */
> +                    gen_exception_return(s, tmp);

Won't this now emit TCG ops to load the PC from the state struct,
AND it with ~1, and then write it back again ? That seebms worse than the
current code.

>                  }
>              }
>              break;

thanks
-- PMM