[Qemu-devel] [PATCH 0/2] target/arm: Fix routing of singlestep exceptions

[Qemu-devel] [PATCH 0/2] target/arm: Fix routing of singlestep exceptions

[Peter Maydell]

Bug https://bugs.launchpad.net/qemu/+bug/1838913 reports that
when doing architectural singlestepping we send the singlestep
exceptions to EL1, even if the guest has configured the debug
exception level to be EL2 or EL3.

This patchset fixes that, by putting the debug target EL into
the TB flags and sending exceptions there, rather than sending
them to the default exception level.

Patch 1 is a preliminary refactoring out of the "generate the
exception" code into translate.h; we then have a single
place to do the actual fix, which is in patch 2.

(This bug has been present for ages, and it only affects
guests that try to do debug to EL2, which is pretty rare,
so it's not 4.1 material, especially at this point in the
release cycle.)

thanks
-- PMM

Peter Maydell (2):
  target/arm: Factor out 'generate singlestep exception' function
  target/arm: Fix routing of singlestep exceptions

 target/arm/cpu.h           |  5 +++++
 target/arm/translate.h     | 34 ++++++++++++++++++++++++++++++++--
 target/arm/helper.c        |  6 ++++++
 target/arm/translate-a64.c | 21 +++------------------
 target/arm/translate.c     | 24 +++++-------------------
 5 files changed, 51 insertions(+), 39 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH 1/2] target/arm: Factor out 'generate singlestep exception' function

[Peter Maydell]

Factor out code to 'generate a singlestep exception', which is
currently repeated in four places.

To do this we need to also pull the identical copies of the
gen-exception() function out of translate-a64.c and translate.c
into translate.h.

(There is a bug in the code: we're taking the exception to the wrong
target EL.  This will be simpler to fix if there's only one place to
do it.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/translate.h     | 23 +++++++++++++++++++++++
 target/arm/translate-a64.c | 19 ++-----------------
 target/arm/translate.c     | 20 ++------------------
 3 files changed, 27 insertions(+), 35 deletions(-)

diff --git a/target/arm/translate.h b/target/arm/translate.h
index a20f6e20568..45053190baa 100644
--- a/target/arm/translate.h
+++ b/target/arm/translate.h
@@ -2,6 +2,7 @@
 #define TARGET_ARM_TRANSLATE_H
 
 #include "exec/translator.h"
+#include "internals.h"
 
 
 /* internal defines */
@@ -232,6 +233,28 @@ static inline void gen_ss_advance(DisasContext *s)
     }
 }
 
+static inline void gen_exception(int excp, uint32_t syndrome,
+                                 uint32_t target_el)
+{
+    TCGv_i32 tcg_excp = tcg_const_i32(excp);
+    TCGv_i32 tcg_syn = tcg_const_i32(syndrome);
+    TCGv_i32 tcg_el = tcg_const_i32(target_el);
+
+    gen_helper_exception_with_syndrome(cpu_env, tcg_excp,
+                                       tcg_syn, tcg_el);
+
+    tcg_temp_free_i32(tcg_el);
+    tcg_temp_free_i32(tcg_syn);
+    tcg_temp_free_i32(tcg_excp);
+}
+
+/* Generate an architectural singlestep exception */
+static inline void gen_swstep_exception(DisasContext *s, int isv, int ex)
+{
+    gen_exception(EXCP_UDEF, syn_swstep(s->ss_same_el, isv, ex),
+                  default_exception_el(s));
+}
+
 /*
  * Given a VFP floating point constant encoded into an 8 bit immediate in an
  * instruction, expand it to the actual constant value of the specified
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index d3231477a27..f6729b96fd0 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -253,19 +253,6 @@ static void gen_exception_internal(int excp)
     tcg_temp_free_i32(tcg_excp);
 }
 
-static void gen_exception(int excp, uint32_t syndrome, uint32_t target_el)
-{
-    TCGv_i32 tcg_excp = tcg_const_i32(excp);
-    TCGv_i32 tcg_syn = tcg_const_i32(syndrome);
-    TCGv_i32 tcg_el = tcg_const_i32(target_el);
-
-    gen_helper_exception_with_syndrome(cpu_env, tcg_excp,
-                                       tcg_syn, tcg_el);
-    tcg_temp_free_i32(tcg_el);
-    tcg_temp_free_i32(tcg_syn);
-    tcg_temp_free_i32(tcg_excp);
-}
-
 static void gen_exception_internal_insn(DisasContext *s, int offset, int excp)
 {
     gen_a64_set_pc_im(s->pc - offset);
@@ -305,8 +292,7 @@ static void gen_step_complete_exception(DisasContext *s)
      * of the exception, and our syndrome information is always correct.
      */
     gen_ss_advance(s);
-    gen_exception(EXCP_UDEF, syn_swstep(s->ss_same_el, 1, s->is_ldex),
-                  default_exception_el(s));
+    gen_swstep_exception(s, 1, s->is_ldex);
     s->base.is_jmp = DISAS_NORETURN;
 }
 
@@ -14261,8 +14247,7 @@ static void aarch64_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
          * bits should be zero.
          */
         assert(dc->base.num_insns == 1);
-        gen_exception(EXCP_UDEF, syn_swstep(dc->ss_same_el, 0, 0),
-                      default_exception_el(dc));
+        gen_swstep_exception(dc, 0, 0);
         dc->base.is_jmp = DISAS_NORETURN;
     } else {
         disas_a64_insn(env, dc);
diff --git a/target/arm/translate.c b/target/arm/translate.c
index 7853462b21b..19b9d8f2725 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -282,20 +282,6 @@ static void gen_exception_internal(int excp)
     tcg_temp_free_i32(tcg_excp);
 }
 
-static void gen_exception(int excp, uint32_t syndrome, uint32_t target_el)
-{
-    TCGv_i32 tcg_excp = tcg_const_i32(excp);
-    TCGv_i32 tcg_syn = tcg_const_i32(syndrome);
-    TCGv_i32 tcg_el = tcg_const_i32(target_el);
-
-    gen_helper_exception_with_syndrome(cpu_env, tcg_excp,
-                                       tcg_syn, tcg_el);
-
-    tcg_temp_free_i32(tcg_el);
-    tcg_temp_free_i32(tcg_syn);
-    tcg_temp_free_i32(tcg_excp);
-}
-
 static void gen_step_complete_exception(DisasContext *s)
 {
     /* We just completed step of an insn. Move from Active-not-pending
@@ -308,8 +294,7 @@ static void gen_step_complete_exception(DisasContext *s)
      * of the exception, and our syndrome information is always correct.
      */
     gen_ss_advance(s);
-    gen_exception(EXCP_UDEF, syn_swstep(s->ss_same_el, 1, s->is_ldex),
-                  default_exception_el(s));
+    gen_swstep_exception(s, 1, s->is_ldex);
     s->base.is_jmp = DISAS_NORETURN;
 }
 
@@ -12024,8 +12009,7 @@ static bool arm_pre_translate_insn(DisasContext *dc)
          * bits should be zero.
          */
         assert(dc->base.num_insns == 1);
-        gen_exception(EXCP_UDEF, syn_swstep(dc->ss_same_el, 0, 0),
-                      default_exception_el(dc));
+        gen_swstep_exception(dc, 0, 0);
         dc->base.is_jmp = DISAS_NORETURN;
         return true;
     }
-- 
2.20.1

[Qemu-devel] [PATCH 2/2] target/arm: Fix routing of singlestep exceptions

[Peter Maydell]

When generating an architectural single-step exception we were
routing it to the "default exception level", which is to say
the same exception level we execute at except that EL0 exceptions
go to EL1. This is incorrect because the debug exception level
can be configured by the guest for situations such as single
stepping of EL0 and EL1 code by EL2.

We have to track the target debug exception level in the TB
flags, because it is dependent on CPU state like HCR_EL2.TGE
and MDCR_EL2.TDE. (That we were previously calling the
arm_debug_target_el() function to determine dc->ss_same_el
is itself a bug, though one that would only have manifested
as incorrect syndrome information.) Since we are out of TB
flag bits unless we want to expand into the cs_base field,
we share some bits with the M-profile only HANDLER and
STACKCHECK bits, since only A-profile has this singlestep.

Fixes: https://bugs.launchpad.net/qemu/+bug/1838913
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
In theory it would be possible to use just a single TB flag
bit, because other than the route_to_el2 bool, all the
state arm_debug_target_el() checks is either constant or
known from other TB flags. But I think trying to do this
would be pretty hard to maintain and might well break
anyway with future architectural changes.

Slightly less painfully we could reclaim the existing
TBFLAG_ANY_SS_ACTIVE, since the debug target EL can't
be 0 and is irrelevant if SS is not active, so we
could arrange for SS_ACTIVE to be DEBUG_TARGET_EL == 0.
But we're going to have to overspill into cs_base pretty
soon anyway so I'm not too keen on being very stingy with
the current flags word at the expense of maintainability.
---
 target/arm/cpu.h           |  5 +++++
 target/arm/translate.h     | 15 +++++++++++----
 target/arm/helper.c        |  6 ++++++
 target/arm/translate-a64.c |  2 +-
 target/arm/translate.c     |  4 +++-
 5 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 94c990cddbd..23ca6c79144 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -3142,6 +3142,11 @@ FIELD(TBFLAG_ANY, PSTATE_SS, 26, 1)
 /* Target EL if we take a floating-point-disabled exception */
 FIELD(TBFLAG_ANY, FPEXC_EL, 24, 2)
 FIELD(TBFLAG_ANY, BE_DATA, 23, 1)
+/*
+ * For A-profile only, target EL for debug exceptions.
+ * Note that this overlaps with the M-profile-only HANDLER and STACKCHECK bits.
+ */
+FIELD(TBFLAG_ANY, DEBUG_TARGET_EL, 21, 2)
 
 /* Bit usage when in AArch32 state: */
 FIELD(TBFLAG_A32, THUMB, 0, 1)
diff --git a/target/arm/translate.h b/target/arm/translate.h
index 45053190baa..b65954c669b 100644
--- a/target/arm/translate.h
+++ b/target/arm/translate.h
@@ -50,6 +50,8 @@ typedef struct DisasContext {
     uint32_t svc_imm;
     int aarch64;
     int current_el;
+    /* Debug target exception level for single-step exceptions */
+    int debug_target_el;
     GHashTable *cp_regs;
     uint64_t features; /* CPU features bits */
     /* Because unallocated encodings generate different exception syndrome
@@ -70,8 +72,6 @@ typedef struct DisasContext {
      * ie A64 LDX*, LDAX*, A32/T32 LDREX*, LDAEX*.
      */
     bool is_ldex;
-    /* True if a single-step exception will be taken to the current EL */
-    bool ss_same_el;
     /* True if v8.3-PAuth is active.  */
     bool pauth_active;
     /* True with v8.5-BTI and SCTLR_ELx.BT* set.  */
@@ -251,8 +251,15 @@ static inline void gen_exception(int excp, uint32_t syndrome,
 /* Generate an architectural singlestep exception */
 static inline void gen_swstep_exception(DisasContext *s, int isv, int ex)
 {
-    gen_exception(EXCP_UDEF, syn_swstep(s->ss_same_el, isv, ex),
-                  default_exception_el(s));
+    bool same_el = (s->debug_target_el == s->current_el);
+
+    /*
+     * If singlestep is targeting a lower EL than the current one,
+     * then s->ss_active must be false and we can never get here.
+     */
+    assert(s->debug_target_el >= s->current_el);
+
+    gen_exception(EXCP_UDEF, syn_swstep(same_el, isv, ex), s->debug_target_el);
 }
 
 /*
diff --git a/target/arm/helper.c b/target/arm/helper.c
index b74c23a9bc0..24806c16ca2 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -11170,6 +11170,12 @@ void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,
         }
     }
 
+    if (!arm_feature(env, ARM_FEATURE_M)) {
+        int target_el = arm_debug_target_el(env);
+
+        flags = FIELD_DP32(flags, TBFLAG_ANY, DEBUG_TARGET_EL, target_el);
+    }
+
     *pflags = flags;
     *cs_base = 0;
 }
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index f6729b96fd0..90850eadc1b 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -14180,7 +14180,7 @@ static void aarch64_tr_init_disas_context(DisasContextBase *dcbase,
     dc->ss_active = FIELD_EX32(tb_flags, TBFLAG_ANY, SS_ACTIVE);
     dc->pstate_ss = FIELD_EX32(tb_flags, TBFLAG_ANY, PSTATE_SS);
     dc->is_ldex = false;
-    dc->ss_same_el = (arm_debug_target_el(env) == dc->current_el);
+    dc->debug_target_el = FIELD_EX32(tb_flags, TBFLAG_ANY, DEBUG_TARGET_EL);
 
     /* Bound the number of insns to execute to those left on the page.  */
     bound = -(dc->base.pc_first | TARGET_PAGE_MASK) / 4;
diff --git a/target/arm/translate.c b/target/arm/translate.c
index 19b9d8f2725..b32508cd2f9 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -11882,7 +11882,9 @@ static void arm_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
     dc->ss_active = FIELD_EX32(tb_flags, TBFLAG_ANY, SS_ACTIVE);
     dc->pstate_ss = FIELD_EX32(tb_flags, TBFLAG_ANY, PSTATE_SS);
     dc->is_ldex = false;
-    dc->ss_same_el = false; /* Can't be true since EL_d must be AArch64 */
+    if (!arm_feature(env, ARM_FEATURE_M)) {
+        dc->debug_target_el = FIELD_EX32(tb_flags, TBFLAG_ANY, DEBUG_TARGET_EL);
+    }
 
     dc->page_start = dc->base.pc_first & TARGET_PAGE_MASK;
 
-- 
2.20.1

Re: [Qemu-devel] [PATCH 1/2] target/arm: Factor out 'generate singlestep exception' function

[Philippe Mathieu-Daudé]

On 8/5/19 3:09 PM, Peter Maydell wrote:
> Factor out code to 'generate a singlestep exception', which is
> currently repeated in four places.
> 
> To do this we need to also pull the identical copies of the
> gen-exception() function out of translate-a64.c and translate.c
> into translate.h.
> 
> (There is a bug in the code: we're taking the exception to the wrong
> target EL.  This will be simpler to fix if there's only one place to
> do it.)
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  target/arm/translate.h     | 23 +++++++++++++++++++++++
>  target/arm/translate-a64.c | 19 ++-----------------
>  target/arm/translate.c     | 20 ++------------------
>  3 files changed, 27 insertions(+), 35 deletions(-)
> 
> diff --git a/target/arm/translate.h b/target/arm/translate.h
> index a20f6e20568..45053190baa 100644
> --- a/target/arm/translate.h
> +++ b/target/arm/translate.h
> @@ -2,6 +2,7 @@
>  #define TARGET_ARM_TRANSLATE_H
>  
>  #include "exec/translator.h"
> +#include "internals.h"
>  
>  
>  /* internal defines */
> @@ -232,6 +233,28 @@ static inline void gen_ss_advance(DisasContext *s)
>      }
>  }
>  
> +static inline void gen_exception(int excp, uint32_t syndrome,
> +                                 uint32_t target_el)
> +{
> +    TCGv_i32 tcg_excp = tcg_const_i32(excp);
> +    TCGv_i32 tcg_syn = tcg_const_i32(syndrome);
> +    TCGv_i32 tcg_el = tcg_const_i32(target_el);
> +
> +    gen_helper_exception_with_syndrome(cpu_env, tcg_excp,
> +                                       tcg_syn, tcg_el);
> +
> +    tcg_temp_free_i32(tcg_el);
> +    tcg_temp_free_i32(tcg_syn);
> +    tcg_temp_free_i32(tcg_excp);
> +}
> +
> +/* Generate an architectural singlestep exception */
> +static inline void gen_swstep_exception(DisasContext *s, int isv, int ex)
> +{
> +    gen_exception(EXCP_UDEF, syn_swstep(s->ss_same_el, isv, ex),
> +                  default_exception_el(s));
> +}
> +
>  /*
>   * Given a VFP floating point constant encoded into an 8 bit immediate in an
>   * instruction, expand it to the actual constant value of the specified
> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
> index d3231477a27..f6729b96fd0 100644
> --- a/target/arm/translate-a64.c
> +++ b/target/arm/translate-a64.c
> @@ -253,19 +253,6 @@ static void gen_exception_internal(int excp)
>      tcg_temp_free_i32(tcg_excp);
>  }
>  
> -static void gen_exception(int excp, uint32_t syndrome, uint32_t target_el)
> -{
> -    TCGv_i32 tcg_excp = tcg_const_i32(excp);
> -    TCGv_i32 tcg_syn = tcg_const_i32(syndrome);
> -    TCGv_i32 tcg_el = tcg_const_i32(target_el);
> -
> -    gen_helper_exception_with_syndrome(cpu_env, tcg_excp,
> -                                       tcg_syn, tcg_el);
> -    tcg_temp_free_i32(tcg_el);
> -    tcg_temp_free_i32(tcg_syn);
> -    tcg_temp_free_i32(tcg_excp);
> -}
> -
>  static void gen_exception_internal_insn(DisasContext *s, int offset, int excp)
>  {
>      gen_a64_set_pc_im(s->pc - offset);
> @@ -305,8 +292,7 @@ static void gen_step_complete_exception(DisasContext *s)
>       * of the exception, and our syndrome information is always correct.
>       */
>      gen_ss_advance(s);
> -    gen_exception(EXCP_UDEF, syn_swstep(s->ss_same_el, 1, s->is_ldex),
> -                  default_exception_el(s));
> +    gen_swstep_exception(s, 1, s->is_ldex);
>      s->base.is_jmp = DISAS_NORETURN;
>  }
>  
> @@ -14261,8 +14247,7 @@ static void aarch64_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
>           * bits should be zero.
>           */
>          assert(dc->base.num_insns == 1);
> -        gen_exception(EXCP_UDEF, syn_swstep(dc->ss_same_el, 0, 0),
> -                      default_exception_el(dc));
> +        gen_swstep_exception(dc, 0, 0);
>          dc->base.is_jmp = DISAS_NORETURN;
>      } else {
>          disas_a64_insn(env, dc);
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index 7853462b21b..19b9d8f2725 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -282,20 +282,6 @@ static void gen_exception_internal(int excp)
>      tcg_temp_free_i32(tcg_excp);
>  }
>  
> -static void gen_exception(int excp, uint32_t syndrome, uint32_t target_el)
> -{
> -    TCGv_i32 tcg_excp = tcg_const_i32(excp);
> -    TCGv_i32 tcg_syn = tcg_const_i32(syndrome);
> -    TCGv_i32 tcg_el = tcg_const_i32(target_el);
> -
> -    gen_helper_exception_with_syndrome(cpu_env, tcg_excp,
> -                                       tcg_syn, tcg_el);
> -
> -    tcg_temp_free_i32(tcg_el);
> -    tcg_temp_free_i32(tcg_syn);
> -    tcg_temp_free_i32(tcg_excp);
> -}
> -
>  static void gen_step_complete_exception(DisasContext *s)
>  {
>      /* We just completed step of an insn. Move from Active-not-pending
> @@ -308,8 +294,7 @@ static void gen_step_complete_exception(DisasContext *s)
>       * of the exception, and our syndrome information is always correct.
>       */
>      gen_ss_advance(s);
> -    gen_exception(EXCP_UDEF, syn_swstep(s->ss_same_el, 1, s->is_ldex),
> -                  default_exception_el(s));
> +    gen_swstep_exception(s, 1, s->is_ldex);
>      s->base.is_jmp = DISAS_NORETURN;
>  }
>  
> @@ -12024,8 +12009,7 @@ static bool arm_pre_translate_insn(DisasContext *dc)
>           * bits should be zero.
>           */
>          assert(dc->base.num_insns == 1);
> -        gen_exception(EXCP_UDEF, syn_swstep(dc->ss_same_el, 0, 0),
> -                      default_exception_el(dc));
> +        gen_swstep_exception(dc, 0, 0);
>          dc->base.is_jmp = DISAS_NORETURN;
>          return true;
>      }
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH 1/2] target/arm: Factor out 'generate singlestep exception' function

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> Factor out code to 'generate a singlestep exception', which is
> currently repeated in four places.
>
> To do this we need to also pull the identical copies of the
> gen-exception() function out of translate-a64.c and translate.c
> into translate.h.
>
> (There is a bug in the code: we're taking the exception to the wrong
> target EL.  This will be simpler to fix if there's only one place to
> do it.)
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  target/arm/translate.h     | 23 +++++++++++++++++++++++
>  target/arm/translate-a64.c | 19 ++-----------------
>  target/arm/translate.c     | 20 ++------------------
>  3 files changed, 27 insertions(+), 35 deletions(-)
>
> diff --git a/target/arm/translate.h b/target/arm/translate.h
> index a20f6e20568..45053190baa 100644
> --- a/target/arm/translate.h
> +++ b/target/arm/translate.h
> @@ -2,6 +2,7 @@
>  #define TARGET_ARM_TRANSLATE_H
>
>  #include "exec/translator.h"
> +#include "internals.h"
>
>
>  /* internal defines */
> @@ -232,6 +233,28 @@ static inline void gen_ss_advance(DisasContext *s)
>      }
>  }
>
> +static inline void gen_exception(int excp, uint32_t syndrome,
> +                                 uint32_t target_el)
> +{
> +    TCGv_i32 tcg_excp = tcg_const_i32(excp);
> +    TCGv_i32 tcg_syn = tcg_const_i32(syndrome);
> +    TCGv_i32 tcg_el = tcg_const_i32(target_el);
> +
> +    gen_helper_exception_with_syndrome(cpu_env, tcg_excp,
> +                                       tcg_syn, tcg_el);
> +
> +    tcg_temp_free_i32(tcg_el);
> +    tcg_temp_free_i32(tcg_syn);
> +    tcg_temp_free_i32(tcg_excp);
> +}
> +
> +/* Generate an architectural singlestep exception */
> +static inline void gen_swstep_exception(DisasContext *s, int isv, int ex)
> +{
> +    gen_exception(EXCP_UDEF, syn_swstep(s->ss_same_el, isv, ex),
> +                  default_exception_el(s));
> +}
> +
>  /*
>   * Given a VFP floating point constant encoded into an 8 bit immediate in an
>   * instruction, expand it to the actual constant value of the specified
> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
> index d3231477a27..f6729b96fd0 100644
> --- a/target/arm/translate-a64.c
> +++ b/target/arm/translate-a64.c
> @@ -253,19 +253,6 @@ static void gen_exception_internal(int excp)
>      tcg_temp_free_i32(tcg_excp);
>  }
>
> -static void gen_exception(int excp, uint32_t syndrome, uint32_t target_el)
> -{
> -    TCGv_i32 tcg_excp = tcg_const_i32(excp);
> -    TCGv_i32 tcg_syn = tcg_const_i32(syndrome);
> -    TCGv_i32 tcg_el = tcg_const_i32(target_el);
> -
> -    gen_helper_exception_with_syndrome(cpu_env, tcg_excp,
> -                                       tcg_syn, tcg_el);
> -    tcg_temp_free_i32(tcg_el);
> -    tcg_temp_free_i32(tcg_syn);
> -    tcg_temp_free_i32(tcg_excp);
> -}
> -
>  static void gen_exception_internal_insn(DisasContext *s, int offset, int excp)
>  {
>      gen_a64_set_pc_im(s->pc - offset);
> @@ -305,8 +292,7 @@ static void gen_step_complete_exception(DisasContext *s)
>       * of the exception, and our syndrome information is always correct.
>       */
>      gen_ss_advance(s);
> -    gen_exception(EXCP_UDEF, syn_swstep(s->ss_same_el, 1, s->is_ldex),
> -                  default_exception_el(s));
> +    gen_swstep_exception(s, 1, s->is_ldex);
>      s->base.is_jmp = DISAS_NORETURN;
>  }
>
> @@ -14261,8 +14247,7 @@ static void aarch64_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
>           * bits should be zero.
>           */
>          assert(dc->base.num_insns == 1);
> -        gen_exception(EXCP_UDEF, syn_swstep(dc->ss_same_el, 0, 0),
> -                      default_exception_el(dc));
> +        gen_swstep_exception(dc, 0, 0);
>          dc->base.is_jmp = DISAS_NORETURN;
>      } else {
>          disas_a64_insn(env, dc);
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index 7853462b21b..19b9d8f2725 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -282,20 +282,6 @@ static void gen_exception_internal(int excp)
>      tcg_temp_free_i32(tcg_excp);
>  }
>
> -static void gen_exception(int excp, uint32_t syndrome, uint32_t target_el)
> -{
> -    TCGv_i32 tcg_excp = tcg_const_i32(excp);
> -    TCGv_i32 tcg_syn = tcg_const_i32(syndrome);
> -    TCGv_i32 tcg_el = tcg_const_i32(target_el);
> -
> -    gen_helper_exception_with_syndrome(cpu_env, tcg_excp,
> -                                       tcg_syn, tcg_el);
> -
> -    tcg_temp_free_i32(tcg_el);
> -    tcg_temp_free_i32(tcg_syn);
> -    tcg_temp_free_i32(tcg_excp);
> -}
> -
>  static void gen_step_complete_exception(DisasContext *s)
>  {
>      /* We just completed step of an insn. Move from Active-not-pending
> @@ -308,8 +294,7 @@ static void gen_step_complete_exception(DisasContext *s)
>       * of the exception, and our syndrome information is always correct.
>       */
>      gen_ss_advance(s);
> -    gen_exception(EXCP_UDEF, syn_swstep(s->ss_same_el, 1, s->is_ldex),
> -                  default_exception_el(s));
> +    gen_swstep_exception(s, 1, s->is_ldex);
>      s->base.is_jmp = DISAS_NORETURN;
>  }
>
> @@ -12024,8 +12009,7 @@ static bool arm_pre_translate_insn(DisasContext *dc)
>           * bits should be zero.
>           */
>          assert(dc->base.num_insns == 1);
> -        gen_exception(EXCP_UDEF, syn_swstep(dc->ss_same_el, 0, 0),
> -                      default_exception_el(dc));
> +        gen_swstep_exception(dc, 0, 0);
>          dc->base.is_jmp = DISAS_NORETURN;
>          return true;
>      }


--
Alex Bennée

Re: [Qemu-devel] [Qemu-arm] [PATCH 2/2] target/arm: Fix routing of singlestep exceptions

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> When generating an architectural single-step exception we were
> routing it to the "default exception level", which is to say
> the same exception level we execute at except that EL0 exceptions
> go to EL1. This is incorrect because the debug exception level
> can be configured by the guest for situations such as single
> stepping of EL0 and EL1 code by EL2.
>
> We have to track the target debug exception level in the TB
> flags, because it is dependent on CPU state like HCR_EL2.TGE
> and MDCR_EL2.TDE. (That we were previously calling the
> arm_debug_target_el() function to determine dc->ss_same_el
> is itself a bug, though one that would only have manifested
> as incorrect syndrome information.) Since we are out of TB
> flag bits unless we want to expand into the cs_base field,
> we share some bits with the M-profile only HANDLER and
> STACKCHECK bits, since only A-profile has this singlestep.
>
> Fixes: https://bugs.launchpad.net/qemu/+bug/1838913
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Tested-by: Alex Bennée <alex.bennee@linaro.org>

> ---
> In theory it would be possible to use just a single TB flag
> bit, because other than the route_to_el2 bool, all the
> state arm_debug_target_el() checks is either constant or
> known from other TB flags. But I think trying to do this
> would be pretty hard to maintain and might well break
> anyway with future architectural changes.
>
> Slightly less painfully we could reclaim the existing
> TBFLAG_ANY_SS_ACTIVE, since the debug target EL can't
> be 0 and is irrelevant if SS is not active, so we
> could arrange for SS_ACTIVE to be DEBUG_TARGET_EL == 0.
> But we're going to have to overspill into cs_base pretty
> soon anyway so I'm not too keen on being very stingy with
> the current flags word at the expense of maintainability.
> ---
>  target/arm/cpu.h           |  5 +++++
>  target/arm/translate.h     | 15 +++++++++++----
>  target/arm/helper.c        |  6 ++++++
>  target/arm/translate-a64.c |  2 +-
>  target/arm/translate.c     |  4 +++-
>  5 files changed, 26 insertions(+), 6 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 94c990cddbd..23ca6c79144 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -3142,6 +3142,11 @@ FIELD(TBFLAG_ANY, PSTATE_SS, 26, 1)
>  /* Target EL if we take a floating-point-disabled exception */
>  FIELD(TBFLAG_ANY, FPEXC_EL, 24, 2)
>  FIELD(TBFLAG_ANY, BE_DATA, 23, 1)
> +/*
> + * For A-profile only, target EL for debug exceptions.
> + * Note that this overlaps with the M-profile-only HANDLER and STACKCHECK bits.
> + */
> +FIELD(TBFLAG_ANY, DEBUG_TARGET_EL, 21, 2)
>
>  /* Bit usage when in AArch32 state: */
>  FIELD(TBFLAG_A32, THUMB, 0, 1)
> diff --git a/target/arm/translate.h b/target/arm/translate.h
> index 45053190baa..b65954c669b 100644
> --- a/target/arm/translate.h
> +++ b/target/arm/translate.h
> @@ -50,6 +50,8 @@ typedef struct DisasContext {
>      uint32_t svc_imm;
>      int aarch64;
>      int current_el;
> +    /* Debug target exception level for single-step exceptions */
> +    int debug_target_el;
>      GHashTable *cp_regs;
>      uint64_t features; /* CPU features bits */
>      /* Because unallocated encodings generate different exception syndrome
> @@ -70,8 +72,6 @@ typedef struct DisasContext {
>       * ie A64 LDX*, LDAX*, A32/T32 LDREX*, LDAEX*.
>       */
>      bool is_ldex;
> -    /* True if a single-step exception will be taken to the current EL */
> -    bool ss_same_el;
>      /* True if v8.3-PAuth is active.  */
>      bool pauth_active;
>      /* True with v8.5-BTI and SCTLR_ELx.BT* set.  */
> @@ -251,8 +251,15 @@ static inline void gen_exception(int excp, uint32_t syndrome,
>  /* Generate an architectural singlestep exception */
>  static inline void gen_swstep_exception(DisasContext *s, int isv, int ex)
>  {
> -    gen_exception(EXCP_UDEF, syn_swstep(s->ss_same_el, isv, ex),
> -                  default_exception_el(s));
> +    bool same_el = (s->debug_target_el == s->current_el);
> +
> +    /*
> +     * If singlestep is targeting a lower EL than the current one,
> +     * then s->ss_active must be false and we can never get here.
> +     */
> +    assert(s->debug_target_el >= s->current_el);
> +
> +    gen_exception(EXCP_UDEF, syn_swstep(same_el, isv, ex), s->debug_target_el);
>  }
>
>  /*
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index b74c23a9bc0..24806c16ca2 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -11170,6 +11170,12 @@ void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,
>          }
>      }
>
> +    if (!arm_feature(env, ARM_FEATURE_M)) {
> +        int target_el = arm_debug_target_el(env);
> +
> +        flags = FIELD_DP32(flags, TBFLAG_ANY, DEBUG_TARGET_EL, target_el);
> +    }
> +
>      *pflags = flags;
>      *cs_base = 0;
>  }
> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
> index f6729b96fd0..90850eadc1b 100644
> --- a/target/arm/translate-a64.c
> +++ b/target/arm/translate-a64.c
> @@ -14180,7 +14180,7 @@ static void aarch64_tr_init_disas_context(DisasContextBase *dcbase,
>      dc->ss_active = FIELD_EX32(tb_flags, TBFLAG_ANY, SS_ACTIVE);
>      dc->pstate_ss = FIELD_EX32(tb_flags, TBFLAG_ANY, PSTATE_SS);
>      dc->is_ldex = false;
> -    dc->ss_same_el = (arm_debug_target_el(env) == dc->current_el);
> +    dc->debug_target_el = FIELD_EX32(tb_flags, TBFLAG_ANY, DEBUG_TARGET_EL);
>
>      /* Bound the number of insns to execute to those left on the page.  */
>      bound = -(dc->base.pc_first | TARGET_PAGE_MASK) / 4;
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index 19b9d8f2725..b32508cd2f9 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -11882,7 +11882,9 @@ static void arm_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
>      dc->ss_active = FIELD_EX32(tb_flags, TBFLAG_ANY, SS_ACTIVE);
>      dc->pstate_ss = FIELD_EX32(tb_flags, TBFLAG_ANY, PSTATE_SS);
>      dc->is_ldex = false;
> -    dc->ss_same_el = false; /* Can't be true since EL_d must be AArch64 */
> +    if (!arm_feature(env, ARM_FEATURE_M)) {
> +        dc->debug_target_el = FIELD_EX32(tb_flags, TBFLAG_ANY, DEBUG_TARGET_EL);
> +    }
>
>      dc->page_start = dc->base.pc_first & TARGET_PAGE_MASK;


--
Alex Bennée