Re: [Qemu-devel] [Qemu-arm] [PATCH for-2.12 1/4] target/arm: Honour MDCR_EL2.TDE when routing exceptions due to BKPT/BRK

[Philippe Mathieu-Daudé]

Hi Peter,

On 03/20/2018 02:41 PM, Peter Maydell wrote:
> The MDCR_EL2.TDE bit allows the exception level targeted by debug
> exceptions to be set to EL2 for code executing at EL0.  We handle
> this in the arm_debug_target_el() function, but this is only used for
> hardware breakpoint and watchpoint exceptions, not for the exception
> generated when the guest executes an AArch32 BKPT or AArch64 BRK
> instruction.  We don't have enough information for a translate-time
> equivalent of arm_debug_target_el(), so instead make BKPT and BRK
> call a special purpose helper which can do the routing, rather than
> the generic exception_with_syndrome helper.
> 
> Signed-off-by: Peter Maydell <address@hidden>
> ---
>  target/arm/helper.h        |  1 +
>  target/arm/op_helper.c     |  8 ++++++++
>  target/arm/translate-a64.c | 15 +++++++++++++--
>  target/arm/translate.c     | 19 ++++++++++++++-----
>  4 files changed, 36 insertions(+), 7 deletions(-)
> 
> diff --git a/target/arm/helper.h b/target/arm/helper.h
> index 0d2094f2be..34e8cc8904 100644
> --- a/target/arm/helper.h
> +++ b/target/arm/helper.h
> @@ -47,6 +47,7 @@ DEF_HELPER_FLAGS_3(sel_flags, TCG_CALL_NO_RWG_SE,
>                     i32, i32, i32, i32)
>  DEF_HELPER_2(exception_internal, void, env, i32)
>  DEF_HELPER_4(exception_with_syndrome, void, env, i32, i32, i32)
> +DEF_HELPER_2(exception_bkpt_insn, void, env, i32)
>  DEF_HELPER_1(setend, void, env)
>  DEF_HELPER_2(wfi, void, env, i32)
>  DEF_HELPER_1(wfe, void, env)
> diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
> index 7a88fd2c92..4b123d2bd6 100644
> --- a/target/arm/op_helper.c
> +++ b/target/arm/op_helper.c
> @@ -483,6 +483,14 @@ void HELPER(exception_with_syndrome)(CPUARMState *env, 
> uint32_t excp,
>      raise_exception(env, excp, syndrome, target_el);
>  }
>  
> +/* Raise an EXCP_BKPT with the specified syndrome register value,
> + * targeting the correct exception level for debug exceptions.
> + */
> +void HELPER(exception_bkpt_insn)(CPUARMState *env, uint32_t syndrome)
> +{
> +    raise_exception(env, EXCP_BKPT, syndrome, arm_debug_target_el(env));
> +}
> +
>  uint32_t HELPER(cpsr_read)(CPUARMState *env)
>  {
>      return cpsr_read(env) & ~(CPSR_EXEC | CPSR_RESERVED);
> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
> index 31ff0479e6..510951f7c7 100644
> --- a/target/arm/translate-a64.c
> +++ b/target/arm/translate-a64.c
> @@ -321,6 +321,18 @@ static void gen_exception_insn(DisasContext *s, int 
> offset, int excp,
>      s->base.is_jmp = DISAS_NORETURN;
>  }
>  
> +static void gen_exception_bkpt_insn(DisasContext *s, int offset,
> +                                    uint32_t syndrome)
> +{
> +    TCGv_i32 tcg_syn;
> +
> +    gen_a64_set_pc_im(s->pc - offset);
> +    tcg_syn = tcg_const_i32(syndrome);
> +    gen_helper_exception_bkpt_insn(cpu_env, tcg_syn);
> +    tcg_temp_free_i32(tcg_syn);
> +    s->base.is_jmp = DISAS_NORETURN;
> +}
> +
>  static void gen_ss_advance(DisasContext *s)
>  {
>      /* If the singlestep state is Active-not-pending, advance to
> @@ -1839,8 +1851,7 @@ static void disas_exc(DisasContext *s, uint32_t insn)
>              break;
>          }
>          /* BRK */
> -        gen_exception_insn(s, 4, EXCP_BKPT, syn_aa64_bkpt(imm16),
> -                           default_exception_el(s));
> +        gen_exception_bkpt_insn(s, 4, syn_aa64_bkpt(imm16));
>          break;
>      case 2:
>          if (op2_ll != 0) {
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index ba6ab7d287..68f0c585f4 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -1248,6 +1248,18 @@ static void gen_exception_insn(DisasContext *s, int 
> offset, int excp,
>      s->base.is_jmp = DISAS_NORETURN;
>  }
>  
> +static void gen_exception_bkpt_insn(DisasContext *s, int offset, int syn)

Can you use the same Aa64 prototype when applying please?
(int syn -> uint32_t syndrome)

Reviewed-by: Philippe Mathieu-Daudé <address@hidden>

> +{
> +    TCGv_i32 tcg_syn;
> +
> +    gen_set_condexec(s);
> +    gen_set_pc_im(s, s->pc - offset);
> +    tcg_syn = tcg_const_i32(syn);
> +    gen_helper_exception_bkpt_insn(cpu_env, tcg_syn);
> +    tcg_temp_free_i32(tcg_syn);
> +    s->base.is_jmp = DISAS_NORETURN;
> +}
> +
>  /* Force a TB lookup after an instruction that changes the CPU state.  */
>  static inline void gen_lookup_tb(DisasContext *s)
>  {
> @@ -8774,9 +8786,7 @@ static void disas_arm_insn(DisasContext *s, unsigned 
> int insn)
>              case 1:
>                  /* bkpt */
>                  ARCH(5);
> -                gen_exception_insn(s, 4, EXCP_BKPT,
> -                                   syn_aa32_bkpt(imm16, false),
> -                                   default_exception_el(s));
> +                gen_exception_bkpt_insn(s, 4, syn_aa32_bkpt(imm16, false));
>                  break;
>              case 2:
>                  /* Hypervisor call (v7) */
> @@ -11983,8 +11993,7 @@ static void disas_thumb_insn(DisasContext *s, 
> uint32_t insn)
>          {
>              int imm8 = extract32(insn, 0, 8);
>              ARCH(5);
> -            gen_exception_insn(s, 2, EXCP_BKPT, syn_aa32_bkpt(imm8, true),
> -                               default_exception_el(s));
> +            gen_exception_bkpt_insn(s, 2, syn_aa32_bkpt(imm8, true));
>              break;
>          }
>  
>