[PATCH 00/18] target/arm: tidy exception routing

[PATCH 00/18] target/arm: tidy exception routing

[Richard Henderson]

The target el for raising an exception currently lives in at
least 3 places: exception_target_el, arm_debug_target_el, and
in {sve,fp}_exception_el.

This patch set aims to put all of the routing into the same place.

For the purposes of prep for SME, the goal is the last patch,
where we do not confuse the level at which SVE exceptions are
trapped with the level to which exceptions are delivered.

I suspect that the existing SME prep patch where I remove the
fp checks and then compare fp vs sve el, is flawed while this
route_to_el2 line is still present.

The end result for debug exceptions isn't quite as clean as I
was hoping, but hopefully it's still better than before.


r~


Richard Henderson (18):
  target/arm: Allow raise_exception to handle finding target EL
  target/arm: Use arm_current_el for simple exceptions
  target/arm: Move and expand parameters to exception_target_el
  target/arm: Move HCR_TGE check into exception_target_el
  target/arm: Move arm_singlestep_active out of line
  target/arm: Move arm_generate_debug_exceptions out of line
  target/arm: Hoist arm_current_el in arm_generate_debug_exceptions
  target/arm: Use is_a64 in arm_generate_debug_exceptions
  target/arm: Move exception_bkpt_insn to debug_helper.c
  target/arm: Move arm_debug_exception_fsr to debug_helper.c
  target/arm: Move arm_debug_target_el to internals.h
  target/arm: Create raise_exception_debug
  target/arm: Move MDCR_TDE test into exception_target_el
  target/arm: Mark exception helpers as noreturn
  target/arm: Create helper_exception_swstep
  target/arm: Remove TBFLAG_ANY.DEBUG_TARGET_EL
  target/arm: Add cur_el parameter to arm_generate_debug_exceptions
  target/arm: Remove route_to_el2 case from sve_exception_el

 target/arm/cpu.h           | 128 +---------------------------
 target/arm/helper.h        |   7 +-
 target/arm/internals.h     |  64 +++++---------
 target/arm/translate.h     |  14 +---
 target/arm/debug_helper.c  | 167 ++++++++++++++++++++++++++++++++++---
 target/arm/helper-a64.c    |   7 +-
 target/arm/helper.c        |  25 ++----
 target/arm/mte_helper.c    |   7 +-
 target/arm/op_helper.c     | 128 ++++++++++++++++++----------
 target/arm/tlb_helper.c    |  10 ++-
 target/arm/translate-a64.c |   1 -
 target/arm/translate.c     |   1 -
 12 files changed, 289 insertions(+), 270 deletions(-)

-- 
2.34.1

[PATCH 01/18] target/arm: Allow raise_exception to handle finding target EL

[Richard Henderson]

The work of finding the correct target EL for an exception is
currently split between raise_exception and target_exception_el.
Begin merging these by allowing the input to raise_exception
to be zero and use exception_target_el for that case.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/internals.h | 11 ++++++-----
 target/arm/op_helper.c | 13 +++++++++----
 2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/target/arm/internals.h b/target/arm/internals.h
index b654bee468..03363b0f32 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -111,18 +111,19 @@ FIELD(DBGWCR, SSCE, 29, 1)
 /**
  * raise_exception: Raise the specified exception.
  * Raise a guest exception with the specified value, syndrome register
- * and target exception level. This should be called from helper functions,
- * and never returns because we will longjump back up to the CPU main loop.
+ * and the current or target exception level. This should be called from
+ * helper functions, and never returns because we will longjump back up
+ * to the CPU main loop.
  */
 G_NORETURN void raise_exception(CPUARMState *env, uint32_t excp,
-                                uint32_t syndrome, uint32_t target_el);
+                                uint32_t syndrome, uint32_t cur_or_target_el);
 
 /*
  * Similarly, but also use unwinding to restore cpu state.
  */
 G_NORETURN void raise_exception_ra(CPUARMState *env, uint32_t excp,
-                                      uint32_t syndrome, uint32_t target_el,
-                                      uintptr_t ra);
+                                   uint32_t syndrome,
+                                   uint32_t cur_or_target_el, uintptr_t ra);
 
 /*
  * For AArch64, map a given EL to an index in the banked_spsr array.
diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
index c4bd668870..6b9141b79a 100644
--- a/target/arm/op_helper.c
+++ b/target/arm/op_helper.c
@@ -28,10 +28,15 @@
 #define SIGNBIT (uint32_t)0x80000000
 #define SIGNBIT64 ((uint64_t)1 << 63)
 
-void raise_exception(CPUARMState *env, uint32_t excp,
-                     uint32_t syndrome, uint32_t target_el)
+void raise_exception(CPUARMState *env, uint32_t excp, uint32_t syndrome,
+                     uint32_t cur_or_target_el)
 {
     CPUState *cs = env_cpu(env);
+    int target_el = cur_or_target_el;
+
+    if (cur_or_target_el == 0) {
+        target_el = exception_target_el(env);
+    }
 
     if (target_el == 1 && (arm_hcr_el2_eff(env) & HCR_TGE)) {
         /*
@@ -54,7 +59,7 @@ void raise_exception(CPUARMState *env, uint32_t excp,
 }
 
 void raise_exception_ra(CPUARMState *env, uint32_t excp, uint32_t syndrome,
-                        uint32_t target_el, uintptr_t ra)
+                        uint32_t cur_or_target_el, uintptr_t ra)
 {
     CPUState *cs = env_cpu(env);
 
@@ -64,7 +69,7 @@ void raise_exception_ra(CPUARMState *env, uint32_t excp, uint32_t syndrome,
      * the caller passed us, and cannot use cpu_loop_exit_restore().
      */
     cpu_restore_state(cs, ra, true);
-    raise_exception(env, excp, syndrome, target_el);
+    raise_exception(env, excp, syndrome, cur_or_target_el);
 }
 
 uint64_t HELPER(neon_tbl)(CPUARMState *env, uint32_t desc,
-- 
2.34.1

[PATCH 02/18] target/arm: Use arm_current_el for simple exceptions

[Richard Henderson]

For these cases, the syndrome does not depend on the
origin or target EL, so we can simply defer selection
of the target EL to raise_exception.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/helper-a64.c |  5 +++--
 target/arm/helper.c     | 10 +++-------
 target/arm/mte_helper.c |  7 +++----
 target/arm/op_helper.c  | 13 ++++++-------
 4 files changed, 15 insertions(+), 20 deletions(-)

diff --git a/target/arm/helper-a64.c b/target/arm/helper-a64.c
index 77a8502b6b..22db213aab 100644
--- a/target/arm/helper-a64.c
+++ b/target/arm/helper-a64.c
@@ -70,12 +70,13 @@ static void daif_check(CPUARMState *env, uint32_t op,
                        uint32_t imm, uintptr_t ra)
 {
     /* DAIF update to PSTATE. This is OK from EL0 only if UMA is set.  */
-    if (arm_current_el(env) == 0 && !(arm_sctlr(env, 0) & SCTLR_UMA)) {
+    int el = arm_current_el(env);
+    if (el == 0 && !(arm_sctlr(env, 0) & SCTLR_UMA)) {
         raise_exception_ra(env, EXCP_UDEF,
                            syn_aa64_sysregtrap(0, extract32(op, 0, 3),
                                                extract32(op, 3, 3), 4,
                                                imm, 0x1f, 0),
-                           exception_target_el(env), ra);
+                           el, ra);
     }
 }
 
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 40da63913c..e0be96b988 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -3232,14 +3232,10 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t value,
              * Synchronous external aborts during a translation table walk
              * are taken as Data Abort exceptions.
              */
-            if (fi.stage2) {
-                if (current_el == 3) {
-                    target_el = 3;
-                } else {
-                    target_el = 2;
-                }
+            if (fi.stage2 && current_el < 2) {
+                target_el = 2;
             } else {
-                target_el = exception_target_el(env);
+                target_el = current_el;
             }
             take_exc = true;
         }
diff --git a/target/arm/mte_helper.c b/target/arm/mte_helper.c
index d11a8c70d0..98f2a3215d 100644
--- a/target/arm/mte_helper.c
+++ b/target/arm/mte_helper.c
@@ -540,14 +540,13 @@ void HELPER(stzgm_tags)(CPUARMState *env, uint64_t ptr, uint64_t val)
 static void mte_sync_check_fail(CPUARMState *env, uint32_t desc,
                                 uint64_t dirty_ptr, uintptr_t ra)
 {
-    int is_write, syn;
+    int is_write, syn, el = arm_current_el(env);
 
     env->exception.vaddress = dirty_ptr;
 
     is_write = FIELD_EX32(desc, MTEDESC, WRITE);
-    syn = syn_data_abort_no_iss(arm_current_el(env) != 0, 0, 0, 0, 0, is_write,
-                                0x11);
-    raise_exception_ra(env, EXCP_DATA_ABORT, syn, exception_target_el(env), ra);
+    syn = syn_data_abort_no_iss(el != 0, 0, 0, 0, 0, is_write, 0x11);
+    raise_exception_ra(env, EXCP_DATA_ABORT, syn, el, ra);
     g_assert_not_reached();
 }
 
diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
index 6b9141b79a..61e9c1d903 100644
--- a/target/arm/op_helper.c
+++ b/target/arm/op_helper.c
@@ -503,7 +503,7 @@ uint32_t HELPER(get_r13_banked)(CPUARMState *env, uint32_t mode)
          * Other UNPREDICTABLE and UNDEF cases were caught at translate time.
          */
         raise_exception(env, EXCP_UDEF, syn_uncategorized(),
-                        exception_target_el(env));
+                        arm_current_el(env));
     }
 
     if ((env->uncached_cpsr & CPSR_M) == mode) {
@@ -567,8 +567,7 @@ static void msr_mrs_banked_exc_checks(CPUARMState *env, uint32_t tgtmode,
     return;
 
 undef:
-    raise_exception(env, EXCP_UDEF, syn_uncategorized(),
-                    exception_target_el(env));
+    raise_exception(env, EXCP_UDEF, syn_uncategorized(), arm_current_el(env));
 }
 
 void HELPER(msr_banked)(CPUARMState *env, uint32_t value, uint32_t tgtmode,
@@ -697,7 +696,7 @@ void HELPER(access_check_cp_reg)(CPUARMState *env, void *rip, uint32_t syndrome,
     target_el = res & CP_ACCESS_EL_MASK;
     switch (target_el) {
     case 0:
-        target_el = exception_target_el(env);
+        target_el = arm_current_el(env);
         break;
     case 2:
         assert(arm_current_el(env) != 3);
@@ -808,7 +807,7 @@ void HELPER(pre_hvc)(CPUARMState *env)
 
     if (undef) {
         raise_exception(env, EXCP_UDEF, syn_uncategorized(),
-                        exception_target_el(env));
+                        arm_current_el(env));
     }
 }
 
@@ -870,7 +869,7 @@ void HELPER(pre_smc)(CPUARMState *env, uint32_t syndrome)
          * This handles the very last line of the previous table.
          */
         raise_exception(env, EXCP_UDEF, syn_uncategorized(),
-                        exception_target_el(env));
+                        arm_current_el(env));
     }
 
     if (cur_el == 1 && (arm_hcr_el2_eff(env) & HCR_TSC)) {
@@ -889,7 +888,7 @@ void HELPER(pre_smc)(CPUARMState *env, uint32_t syndrome)
     if (!arm_is_psci_call(cpu, EXCP_SMC) &&
         (smd || !arm_feature(env, ARM_FEATURE_EL3))) {
         raise_exception(env, EXCP_UDEF, syn_uncategorized(),
-                        exception_target_el(env));
+                        arm_current_el(env));
     }
 }
 
-- 
2.34.1

[PATCH 03/18] target/arm: Move and expand parameters to exception_target_el

[Richard Henderson]

Move exception_target_el out of line.
Add cur_el parameter, because 2 of 3 users already have that handy.
Add psyn parameter in preparation for more code movement.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/internals.h  | 15 +--------------
 target/arm/op_helper.c  | 17 ++++++++++++++++-
 target/arm/tlb_helper.c | 10 ++++++----
 3 files changed, 23 insertions(+), 19 deletions(-)

diff --git a/target/arm/internals.h b/target/arm/internals.h
index 03363b0f32..a71f795628 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -1091,20 +1091,7 @@ typedef struct ARMVAParameters {
 ARMVAParameters aa64_va_parameters(CPUARMState *env, uint64_t va,
                                    ARMMMUIdx mmu_idx, bool data);
 
-static inline int exception_target_el(CPUARMState *env)
-{
-    int target_el = MAX(1, arm_current_el(env));
-
-    /*
-     * No such thing as secure EL1 if EL3 is aarch32,
-     * so update the target EL to EL3 in this case.
-     */
-    if (arm_is_secure(env) && !arm_el_is_aa64(env, 3) && target_el == 1) {
-        target_el = 3;
-    }
-
-    return target_el;
-}
+int exception_target_el(CPUARMState *env, int cur_el, uint32_t *psyn);
 
 /* Determine if allocation tags are available.  */
 static inline bool allocation_tag_access_enabled(CPUARMState *env, int el,
diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
index 61e9c1d903..6858b8980d 100644
--- a/target/arm/op_helper.c
+++ b/target/arm/op_helper.c
@@ -28,6 +28,21 @@
 #define SIGNBIT (uint32_t)0x80000000
 #define SIGNBIT64 ((uint64_t)1 << 63)
 
+int exception_target_el(CPUARMState *env, int cur_el, uint32_t *psyn)
+{
+    int target_el = MAX(1, cur_el);
+
+    /*
+     * No such thing as secure EL1 if EL3 is aarch32,
+     * so update the target EL to EL3 in this case.
+     */
+    if (arm_is_secure(env) && !arm_el_is_aa64(env, 3) && target_el == 1) {
+        target_el = 3;
+    }
+
+    return target_el;
+}
+
 void raise_exception(CPUARMState *env, uint32_t excp, uint32_t syndrome,
                      uint32_t cur_or_target_el)
 {
@@ -35,7 +50,7 @@ void raise_exception(CPUARMState *env, uint32_t excp, uint32_t syndrome,
     int target_el = cur_or_target_el;
 
     if (cur_or_target_el == 0) {
-        target_el = exception_target_el(env);
+        target_el = exception_target_el(env, 0, &syndrome);
     }
 
     if (target_el == 1 && (arm_hcr_el2_eff(env) & HCR_TGE)) {
diff --git a/target/arm/tlb_helper.c b/target/arm/tlb_helper.c
index 6421e16202..573e18f830 100644
--- a/target/arm/tlb_helper.c
+++ b/target/arm/tlb_helper.c
@@ -85,11 +85,13 @@ void arm_deliver_fault(ARMCPU *cpu, vaddr addr,
                        int mmu_idx, ARMMMUFaultInfo *fi)
 {
     CPUARMState *env = &cpu->env;
-    int target_el;
+    int cur_el, target_el;
     bool same_el;
     uint32_t syn, exc, fsr, fsc;
 
-    target_el = exception_target_el(env);
+    cur_el = arm_current_el(env);
+    target_el = exception_target_el(env, cur_el, NULL);
+
     if (fi->stage2) {
         target_el = 2;
         env->cp15.hpfar_el2 = extract64(fi->s2addr, 12, 47) << 4;
@@ -97,7 +99,7 @@ void arm_deliver_fault(ARMCPU *cpu, vaddr addr,
             env->cp15.hpfar_el2 |= HPFAR_NS;
         }
     }
-    same_el = (arm_current_el(env) == target_el);
+    same_el = cur_el == target_el;
 
     fsr = compute_fsr_fsc(env, fi, target_el, mmu_idx, &fsc);
 
@@ -139,7 +141,7 @@ void arm_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
 void helper_exception_pc_alignment(CPUARMState *env, target_ulong pc)
 {
     ARMMMUFaultInfo fi = { .type = ARMFault_Alignment };
-    int target_el = exception_target_el(env);
+    int target_el = exception_target_el(env, arm_current_el(env), NULL);
     int mmu_idx = cpu_mmu_index(env, true);
     uint32_t fsc;
 
-- 
2.34.1

[PATCH 04/18] target/arm: Move HCR_TGE check into exception_target_el

[Richard Henderson]

Move the TGE test from raise_exception into
exception_target_el to consolidate tests in one place.
Note that this ought to apply only to origin of EL0,
but that cannot be confirmed at this time.
Update the AdvSIMDFPAccessTrap doc reference to DDI0478H.a.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/op_helper.c | 47 +++++++++++++++++++++++++-----------------
 1 file changed, 28 insertions(+), 19 deletions(-)

diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
index 6858b8980d..55440dfa84 100644
--- a/target/arm/op_helper.c
+++ b/target/arm/op_helper.c
@@ -30,17 +30,39 @@
 
 int exception_target_el(CPUARMState *env, int cur_el, uint32_t *psyn)
 {
-    int target_el = MAX(1, cur_el);
+    /*
+     * FIXME: The following tests really apply to an EL0 origin,
+     * not to a target of EL1.  However, the origin will never be
+     * EL1 for these cases (no aa32 secure EL1, can't enter EL1
+     * with TGE set).  Delay fixing this until all places that
+     * might perform MAX(cur_el, 1) are audited.
+     */
+    if (cur_el >= 2) {
+        return 2;
+    }
 
     /*
      * No such thing as secure EL1 if EL3 is aarch32,
      * so update the target EL to EL3 in this case.
      */
-    if (arm_is_secure(env) && !arm_el_is_aa64(env, 3) && target_el == 1) {
-        target_el = 3;
+    if (arm_is_secure(env) && !arm_el_is_aa64(env, 3)) {
+        return 3;
     }
 
-    return target_el;
+    if (arm_hcr_el2_eff(env) & HCR_TGE) {
+        /*
+         * Redirect NS EL1 exceptions to NS EL2. These are reported with
+         * their original syndrome register value, with the exception of
+         * SIMD/FP access traps, which are reported as uncategorized
+         * (see DDI0487 H.a rule RJNBTN).
+         */
+        if (psyn && syn_get_ec(*psyn) == EC_ADVSIMDFPACCESSTRAP) {
+            *psyn = syn_uncategorized();
+        }
+        return 2;
+    }
+
+    return 1;
 }
 
 void raise_exception(CPUARMState *env, uint32_t excp, uint32_t syndrome,
@@ -49,21 +71,8 @@ void raise_exception(CPUARMState *env, uint32_t excp, uint32_t syndrome,
     CPUState *cs = env_cpu(env);
     int target_el = cur_or_target_el;
 
-    if (cur_or_target_el == 0) {
-        target_el = exception_target_el(env, 0, &syndrome);
-    }
-
-    if (target_el == 1 && (arm_hcr_el2_eff(env) & HCR_TGE)) {
-        /*
-         * Redirect NS EL1 exceptions to NS EL2. These are reported with
-         * their original syndrome register value, with the exception of
-         * SIMD/FP access traps, which are reported as uncategorized
-         * (see DDI0478C.a D1.10.4)
-         */
-        target_el = 2;
-        if (syn_get_ec(syndrome) == EC_ADVSIMDFPACCESSTRAP) {
-            syndrome = syn_uncategorized();
-        }
+    if (cur_or_target_el <= 1) {
+        target_el = exception_target_el(env, cur_or_target_el, &syndrome);
     }
 
     assert(!excp_is_internal(excp));
-- 
2.34.1

[PATCH 05/18] target/arm: Move arm_singlestep_active out of line

[Richard Henderson]

Move the function to debug_helper.c, and the
declaration to internals.h.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/cpu.h          | 10 ----------
 target/arm/internals.h    |  1 +
 target/arm/debug_helper.c | 12 ++++++++++++
 3 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index c1865ad5da..2e115a0281 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -3087,16 +3087,6 @@ static inline bool arm_generate_debug_exceptions(CPUARMState *env)
     }
 }
 
-/* Is single-stepping active? (Note that the "is EL_D AArch64?" check
- * implicitly means this always returns false in pre-v8 CPUs.)
- */
-static inline bool arm_singlestep_active(CPUARMState *env)
-{
-    return extract32(env->cp15.mdscr_el1, 0, 1)
-        && arm_el_is_aa64(env, arm_debug_target_el(env))
-        && arm_generate_debug_exceptions(env);
-}
-
 static inline bool arm_sctlr_b(CPUARMState *env)
 {
     return
diff --git a/target/arm/internals.h b/target/arm/internals.h
index a71f795628..b447d850ae 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -1317,5 +1317,6 @@ void define_cortex_a72_a57_a53_cp_reginfo(ARMCPU *cpu);
 #endif
 
 void aa32_max_features(ARMCPU *cpu);
+bool arm_singlestep_active(CPUARMState *env);
 
 #endif
diff --git a/target/arm/debug_helper.c b/target/arm/debug_helper.c
index 46893697cc..1abf41c5f8 100644
--- a/target/arm/debug_helper.c
+++ b/target/arm/debug_helper.c
@@ -11,6 +11,18 @@
 #include "exec/exec-all.h"
 #include "exec/helper-proto.h"
 
+
+/*
+ * Is single-stepping active? (Note that the "is EL_D AArch64?" check
+ * implicitly means this always returns false in pre-v8 CPUs.)
+ */
+bool arm_singlestep_active(CPUARMState *env)
+{
+    return extract32(env->cp15.mdscr_el1, 0, 1)
+        && arm_el_is_aa64(env, arm_debug_target_el(env))
+        && arm_generate_debug_exceptions(env);
+}
+
 /* Return true if the linked breakpoint entry lbn passes its checks */
 static bool linked_bp_matches(ARMCPU *cpu, int lbn)
 {
-- 
2.34.1

[PATCH 06/18] target/arm: Move arm_generate_debug_exceptions out of line

[Richard Henderson]

Move arm_generate_debug_exceptions and its two subroutines,
{aa32,aa64}_generate_debug_exceptions into debug_helper.c,
and the one interface declaration to internals.h.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/cpu.h          | 91 -------------------------------------
 target/arm/internals.h    |  1 +
 target/arm/debug_helper.c | 94 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 95 insertions(+), 91 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 2e115a0281..92c9758e86 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -2996,97 +2996,6 @@ static inline bool arm_v7m_csselr_razwi(ARMCPU *cpu)
     return (cpu->clidr & R_V7M_CLIDR_CTYPE_ALL_MASK) != 0;
 }
 
-/* See AArch64.GenerateDebugExceptionsFrom() in ARM ARM pseudocode */
-static inline bool aa64_generate_debug_exceptions(CPUARMState *env)
-{
-    int cur_el = arm_current_el(env);
-    int debug_el;
-
-    if (cur_el == 3) {
-        return false;
-    }
-
-    /* MDCR_EL3.SDD disables debug events from Secure state */
-    if (arm_is_secure_below_el3(env)
-        && extract32(env->cp15.mdcr_el3, 16, 1)) {
-        return false;
-    }
-
-    /*
-     * Same EL to same EL debug exceptions need MDSCR_KDE enabled
-     * while not masking the (D)ebug bit in DAIF.
-     */
-    debug_el = arm_debug_target_el(env);
-
-    if (cur_el == debug_el) {
-        return extract32(env->cp15.mdscr_el1, 13, 1)
-            && !(env->daif & PSTATE_D);
-    }
-
-    /* Otherwise the debug target needs to be a higher EL */
-    return debug_el > cur_el;
-}
-
-static inline bool aa32_generate_debug_exceptions(CPUARMState *env)
-{
-    int el = arm_current_el(env);
-
-    if (el == 0 && arm_el_is_aa64(env, 1)) {
-        return aa64_generate_debug_exceptions(env);
-    }
-
-    if (arm_is_secure(env)) {
-        int spd;
-
-        if (el == 0 && (env->cp15.sder & 1)) {
-            /* SDER.SUIDEN means debug exceptions from Secure EL0
-             * are always enabled. Otherwise they are controlled by
-             * SDCR.SPD like those from other Secure ELs.
-             */
-            return true;
-        }
-
-        spd = extract32(env->cp15.mdcr_el3, 14, 2);
-        switch (spd) {
-        case 1:
-            /* SPD == 0b01 is reserved, but behaves as 0b00. */
-        case 0:
-            /* For 0b00 we return true if external secure invasive debug
-             * is enabled. On real hardware this is controlled by external
-             * signals to the core. QEMU always permits debug, and behaves
-             * as if DBGEN, SPIDEN, NIDEN and SPNIDEN are all tied high.
-             */
-            return true;
-        case 2:
-            return false;
-        case 3:
-            return true;
-        }
-    }
-
-    return el != 2;
-}
-
-/* Return true if debugging exceptions are currently enabled.
- * This corresponds to what in ARM ARM pseudocode would be
- *    if UsingAArch32() then
- *        return AArch32.GenerateDebugExceptions()
- *    else
- *        return AArch64.GenerateDebugExceptions()
- * We choose to push the if() down into this function for clarity,
- * since the pseudocode has it at all callsites except for the one in
- * CheckSoftwareStep(), where it is elided because both branches would
- * always return the same value.
- */
-static inline bool arm_generate_debug_exceptions(CPUARMState *env)
-{
-    if (env->aarch64) {
-        return aa64_generate_debug_exceptions(env);
-    } else {
-        return aa32_generate_debug_exceptions(env);
-    }
-}
-
 static inline bool arm_sctlr_b(CPUARMState *env)
 {
     return
diff --git a/target/arm/internals.h b/target/arm/internals.h
index b447d850ae..91702b3ff7 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -1318,5 +1318,6 @@ void define_cortex_a72_a57_a53_cp_reginfo(ARMCPU *cpu);
 
 void aa32_max_features(ARMCPU *cpu);
 bool arm_singlestep_active(CPUARMState *env);
+bool arm_generate_debug_exceptions(CPUARMState *env);
 
 #endif
diff --git a/target/arm/debug_helper.c b/target/arm/debug_helper.c
index 1abf41c5f8..20a0e4261a 100644
--- a/target/arm/debug_helper.c
+++ b/target/arm/debug_helper.c
@@ -12,6 +12,100 @@
 #include "exec/helper-proto.h"
 
 
+/* See AArch64.GenerateDebugExceptionsFrom() in ARM ARM pseudocode */
+static bool aa64_generate_debug_exceptions(CPUARMState *env)
+{
+    int cur_el = arm_current_el(env);
+    int debug_el;
+
+    if (cur_el == 3) {
+        return false;
+    }
+
+    /* MDCR_EL3.SDD disables debug events from Secure state */
+    if (arm_is_secure_below_el3(env)
+        && extract32(env->cp15.mdcr_el3, 16, 1)) {
+        return false;
+    }
+
+    /*
+     * Same EL to same EL debug exceptions need MDSCR_KDE enabled
+     * while not masking the (D)ebug bit in DAIF.
+     */
+    debug_el = arm_debug_target_el(env);
+
+    if (cur_el == debug_el) {
+        return extract32(env->cp15.mdscr_el1, 13, 1)
+            && !(env->daif & PSTATE_D);
+    }
+
+    /* Otherwise the debug target needs to be a higher EL */
+    return debug_el > cur_el;
+}
+
+static bool aa32_generate_debug_exceptions(CPUARMState *env)
+{
+    int el = arm_current_el(env);
+
+    if (el == 0 && arm_el_is_aa64(env, 1)) {
+        return aa64_generate_debug_exceptions(env);
+    }
+
+    if (arm_is_secure(env)) {
+        int spd;
+
+        if (el == 0 && (env->cp15.sder & 1)) {
+            /*
+             * SDER.SUIDEN means debug exceptions from Secure EL0
+             * are always enabled. Otherwise they are controlled by
+             * SDCR.SPD like those from other Secure ELs.
+             */
+            return true;
+        }
+
+        spd = extract32(env->cp15.mdcr_el3, 14, 2);
+        switch (spd) {
+        case 1:
+            /* SPD == 0b01 is reserved, but behaves as 0b00. */
+        case 0:
+            /*
+             * For 0b00 we return true if external secure invasive debug
+             * is enabled. On real hardware this is controlled by external
+             * signals to the core. QEMU always permits debug, and behaves
+             * as if DBGEN, SPIDEN, NIDEN and SPNIDEN are all tied high.
+             */
+            return true;
+        case 2:
+            return false;
+        case 3:
+            return true;
+        }
+    }
+
+    return el != 2;
+}
+
+/*
+ * Return true if debugging exceptions are currently enabled.
+ * This corresponds to what in ARM ARM pseudocode would be
+ *    if UsingAArch32() then
+ *        return AArch32.GenerateDebugExceptions()
+ *    else
+ *        return AArch64.GenerateDebugExceptions()
+ * We choose to push the if() down into this function for clarity,
+ * since the pseudocode has it at all callsites except for the one in
+ * CheckSoftwareStep(), where it is elided because both branches would
+ * always return the same value.
+ */
+bool arm_generate_debug_exceptions(CPUARMState *env)
+{
+    if (env->aarch64) {
+        return aa64_generate_debug_exceptions(env);
+    } else {
+        return aa32_generate_debug_exceptions(env);
+    }
+}
+
 /*
  * Is single-stepping active? (Note that the "is EL_D AArch64?" check
  * implicitly means this always returns false in pre-v8 CPUs.)
-- 
2.34.1

[PATCH 07/18] target/arm: Hoist arm_current_el in arm_generate_debug_exceptions

[Richard Henderson]

Read this value once in the main function, and pass it
around between the subroutines.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/debug_helper.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/target/arm/debug_helper.c b/target/arm/debug_helper.c
index 20a0e4261a..2bbf065b3a 100644
--- a/target/arm/debug_helper.c
+++ b/target/arm/debug_helper.c
@@ -13,9 +13,8 @@
 
 
 /* See AArch64.GenerateDebugExceptionsFrom() in ARM ARM pseudocode */
-static bool aa64_generate_debug_exceptions(CPUARMState *env)
+static bool aa64_generate_debug_exceptions(CPUARMState *env, int cur_el)
 {
-    int cur_el = arm_current_el(env);
     int debug_el;
 
     if (cur_el == 3) {
@@ -43,18 +42,16 @@ static bool aa64_generate_debug_exceptions(CPUARMState *env)
     return debug_el > cur_el;
 }
 
-static bool aa32_generate_debug_exceptions(CPUARMState *env)
+static bool aa32_generate_debug_exceptions(CPUARMState *env, int cur_el)
 {
-    int el = arm_current_el(env);
-
-    if (el == 0 && arm_el_is_aa64(env, 1)) {
-        return aa64_generate_debug_exceptions(env);
+    if (cur_el == 0 && arm_el_is_aa64(env, 1)) {
+        return aa64_generate_debug_exceptions(env, cur_el);
     }
 
     if (arm_is_secure(env)) {
         int spd;
 
-        if (el == 0 && (env->cp15.sder & 1)) {
+        if (cur_el == 0 && (env->cp15.sder & 1)) {
             /*
              * SDER.SUIDEN means debug exceptions from Secure EL0
              * are always enabled. Otherwise they are controlled by
@@ -82,7 +79,7 @@ static bool aa32_generate_debug_exceptions(CPUARMState *env)
         }
     }
 
-    return el != 2;
+    return cur_el != 2;
 }
 
 /*
@@ -99,10 +96,12 @@ static bool aa32_generate_debug_exceptions(CPUARMState *env)
  */
 bool arm_generate_debug_exceptions(CPUARMState *env)
 {
+    int cur_el = arm_current_el(env);
+
     if (env->aarch64) {
-        return aa64_generate_debug_exceptions(env);
+        return aa64_generate_debug_exceptions(env, cur_el);
     } else {
-        return aa32_generate_debug_exceptions(env);
+        return aa32_generate_debug_exceptions(env, cur_el);
     }
 }
 
-- 
2.34.1

[PATCH 08/18] target/arm: Use is_a64 in arm_generate_debug_exceptions

[Richard Henderson]

Use the accessor rather than the raw structure member.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/debug_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/arm/debug_helper.c b/target/arm/debug_helper.c
index 2bbf065b3a..3a86901779 100644
--- a/target/arm/debug_helper.c
+++ b/target/arm/debug_helper.c
@@ -98,7 +98,7 @@ bool arm_generate_debug_exceptions(CPUARMState *env)
 {
     int cur_el = arm_current_el(env);
 
-    if (env->aarch64) {
+    if (is_a64(env)) {
         return aa64_generate_debug_exceptions(env, cur_el);
     } else {
         return aa32_generate_debug_exceptions(env, cur_el);
-- 
2.34.1

[PATCH 09/18] target/arm: Move exception_bkpt_insn to debug_helper.c

[Richard Henderson]

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/debug_helper.c | 31 +++++++++++++++++++++++++++++++
 target/arm/op_helper.c    | 29 -----------------------------
 2 files changed, 31 insertions(+), 29 deletions(-)

diff --git a/target/arm/debug_helper.c b/target/arm/debug_helper.c
index 3a86901779..bdcd5f36d6 100644
--- a/target/arm/debug_helper.c
+++ b/target/arm/debug_helper.c
@@ -429,6 +429,37 @@ void arm_debug_excp_handler(CPUState *cs)
     }
 }
 
+/*
+ * 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)
+{
+    int debug_el = arm_debug_target_el(env);
+    int cur_el = arm_current_el(env);
+
+    /* FSR will only be used if the debug target EL is AArch32. */
+    env->exception.fsr = arm_debug_exception_fsr(env);
+    /*
+     * FAR is UNKNOWN: clear vaddress to avoid potentially exposing
+     * values to the guest that it shouldn't be able to see at its
+     * exception/security level.
+     */
+    env->exception.vaddress = 0;
+    /*
+     * Other kinds of architectural debug exception are ignored if
+     * they target an exception level below the current one (in QEMU
+     * this is checked by arm_generate_debug_exceptions()). Breakpoint
+     * instructions are special because they always generate an exception
+     * to somewhere: if they can't go to the configured debug exception
+     * level they are taken to the current exception level.
+     */
+    if (debug_el < cur_el) {
+        debug_el = cur_el;
+    }
+    raise_exception(env, EXCP_BKPT, syndrome, debug_el);
+}
+
 #if !defined(CONFIG_USER_ONLY)
 
 vaddr arm_adjust_watchpoint_address(CPUState *cs, vaddr addr, int len)
diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
index 55440dfa84..0a50dbf274 100644
--- a/target/arm/op_helper.c
+++ b/target/arm/op_helper.c
@@ -413,35 +413,6 @@ 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)
-{
-    int debug_el = arm_debug_target_el(env);
-    int cur_el = arm_current_el(env);
-
-    /* FSR will only be used if the debug target EL is AArch32. */
-    env->exception.fsr = arm_debug_exception_fsr(env);
-    /* FAR is UNKNOWN: clear vaddress to avoid potentially exposing
-     * values to the guest that it shouldn't be able to see at its
-     * exception/security level.
-     */
-    env->exception.vaddress = 0;
-    /*
-     * Other kinds of architectural debug exception are ignored if
-     * they target an exception level below the current one (in QEMU
-     * this is checked by arm_generate_debug_exceptions()). Breakpoint
-     * instructions are special because they always generate an exception
-     * to somewhere: if they can't go to the configured debug exception
-     * level they are taken to the current exception level.
-     */
-    if (debug_el < cur_el) {
-        debug_el = cur_el;
-    }
-    raise_exception(env, EXCP_BKPT, syndrome, debug_el);
-}
-
 uint32_t HELPER(cpsr_read)(CPUARMState *env)
 {
     return cpsr_read(env) & ~CPSR_EXEC;
-- 
2.34.1

[PATCH 10/18] target/arm: Move arm_debug_exception_fsr to debug_helper.c

[Richard Henderson]

This function now now only used in debug_helper.c, so there is
no reason to have a declaration in a header.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/internals.h    | 25 -------------------------
 target/arm/debug_helper.c | 26 ++++++++++++++++++++++++++
 2 files changed, 26 insertions(+), 25 deletions(-)

diff --git a/target/arm/internals.h b/target/arm/internals.h
index 91702b3ff7..bb45100f06 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -794,31 +794,6 @@ static inline TCR *regime_tcr(CPUARMState *env, ARMMMUIdx mmu_idx)
     return &env->cp15.tcr_el[regime_el(env, mmu_idx)];
 }
 
-/* Return the FSR value for a debug exception (watchpoint, hardware
- * breakpoint or BKPT insn) targeting the specified exception level.
- */
-static inline uint32_t arm_debug_exception_fsr(CPUARMState *env)
-{
-    ARMMMUFaultInfo fi = { .type = ARMFault_Debug };
-    int target_el = arm_debug_target_el(env);
-    bool using_lpae = false;
-
-    if (target_el == 2 || arm_el_is_aa64(env, target_el)) {
-        using_lpae = true;
-    } else {
-        if (arm_feature(env, ARM_FEATURE_LPAE) &&
-            (env->cp15.tcr_el[target_el].raw_tcr & TTBCR_EAE)) {
-            using_lpae = true;
-        }
-    }
-
-    if (using_lpae) {
-        return arm_fi_to_lfsc(&fi);
-    } else {
-        return arm_fi_to_sfsc(&fi);
-    }
-}
-
 /**
  * arm_num_brps: Return number of implemented breakpoints.
  * Note that the ID register BRPS field is "number of bps - 1",
diff --git a/target/arm/debug_helper.c b/target/arm/debug_helper.c
index bdcd5f36d6..08d461fd19 100644
--- a/target/arm/debug_helper.c
+++ b/target/arm/debug_helper.c
@@ -378,6 +378,32 @@ bool arm_debug_check_watchpoint(CPUState *cs, CPUWatchpoint *wp)
     return check_watchpoints(cpu);
 }
 
+/*
+ * Return the FSR value for a debug exception (watchpoint, hardware
+ * breakpoint or BKPT insn) targeting the specified exception level.
+ */
+static uint32_t arm_debug_exception_fsr(CPUARMState *env)
+{
+    ARMMMUFaultInfo fi = { .type = ARMFault_Debug };
+    int target_el = arm_debug_target_el(env);
+    bool using_lpae = false;
+
+    if (target_el == 2 || arm_el_is_aa64(env, target_el)) {
+        using_lpae = true;
+    } else {
+        if (arm_feature(env, ARM_FEATURE_LPAE) &&
+            (env->cp15.tcr_el[target_el].raw_tcr & TTBCR_EAE)) {
+            using_lpae = true;
+        }
+    }
+
+    if (using_lpae) {
+        return arm_fi_to_lfsc(&fi);
+    } else {
+        return arm_fi_to_sfsc(&fi);
+    }
+}
+
 void arm_debug_excp_handler(CPUState *cs)
 {
     /*
-- 
2.34.1

[PATCH 11/18] target/arm: Move arm_debug_target_el to internals.h

[Richard Henderson]

This function is private to the implementation.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/cpu.h       | 21 ---------------------
 target/arm/internals.h | 21 +++++++++++++++++++++
 2 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 92c9758e86..90cdc7b1de 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -2967,27 +2967,6 @@ typedef enum ARMASIdx {
     ARMASIdx_TagS = 3,
 } ARMASIdx;
 
-/* Return the Exception Level targeted by debug exceptions. */
-static inline int arm_debug_target_el(CPUARMState *env)
-{
-    bool secure = arm_is_secure(env);
-    bool route_to_el2 = false;
-
-    if (arm_is_el2_enabled(env)) {
-        route_to_el2 = env->cp15.hcr_el2 & HCR_TGE ||
-                       env->cp15.mdcr_el2 & MDCR_TDE;
-    }
-
-    if (route_to_el2) {
-        return 2;
-    } else if (arm_feature(env, ARM_FEATURE_EL3) &&
-               !arm_el_is_aa64(env, 3) && secure) {
-        return 3;
-    } else {
-        return 1;
-    }
-}
-
 static inline bool arm_v7m_csselr_razwi(ARMCPU *cpu)
 {
     /* If all the CLIDR.Ctypem bits are 0 there are no caches, and
diff --git a/target/arm/internals.h b/target/arm/internals.h
index bb45100f06..685214503b 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -1068,6 +1068,27 @@ ARMVAParameters aa64_va_parameters(CPUARMState *env, uint64_t va,
 
 int exception_target_el(CPUARMState *env, int cur_el, uint32_t *psyn);
 
+/* Return the Exception Level targeted by debug exceptions. */
+static inline int arm_debug_target_el(CPUARMState *env)
+{
+    bool secure = arm_is_secure(env);
+    bool route_to_el2 = false;
+
+    if (arm_is_el2_enabled(env)) {
+        route_to_el2 = env->cp15.hcr_el2 & HCR_TGE ||
+                       env->cp15.mdcr_el2 & MDCR_TDE;
+    }
+
+    if (route_to_el2) {
+        return 2;
+    } else if (arm_feature(env, ARM_FEATURE_EL3) &&
+               !arm_el_is_aa64(env, 3) && secure) {
+        return 3;
+    } else {
+        return 1;
+    }
+}
+
 /* Determine if allocation tags are available.  */
 static inline bool allocation_tag_access_enabled(CPUARMState *env, int el,
                                                  uint64_t sctlr)
-- 
2.34.1

[PATCH 12/18] target/arm: Create raise_exception_debug

[Richard Henderson]

Handle EL testing for debug exceptions in a single place.
Split out raise_exception_int as a common helper.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/internals.h    |  8 ++++++++
 target/arm/debug_helper.c | 27 ++++--------------------
 target/arm/op_helper.c    | 43 ++++++++++++++++++++++++++++++++-------
 3 files changed, 48 insertions(+), 30 deletions(-)

diff --git a/target/arm/internals.h b/target/arm/internals.h
index 685214503b..6df38db836 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -125,6 +125,14 @@ G_NORETURN void raise_exception_ra(CPUARMState *env, uint32_t excp,
                                    uint32_t syndrome,
                                    uint32_t cur_or_target_el, uintptr_t ra);
 
+/**
+ * raise_exception_debug:
+ * Similarly.  If @excp != EXCPBKPT, modify syndrome to indicate
+ * when origin and target EL are the same.
+ */
+G_NORETURN void raise_exception_debug(CPUARMState *env, uint32_t excp,
+                                      uint32_t syndrome);
+
 /*
  * For AArch64, map a given EL to an index in the banked_spsr array.
  * Note that this mapping and the AArch32 mapping defined in bank_number()
diff --git a/target/arm/debug_helper.c b/target/arm/debug_helper.c
index 08d461fd19..181ba7b042 100644
--- a/target/arm/debug_helper.c
+++ b/target/arm/debug_helper.c
@@ -417,19 +417,16 @@ void arm_debug_excp_handler(CPUState *cs)
     if (wp_hit) {
         if (wp_hit->flags & BP_CPU) {
             bool wnr = (wp_hit->flags & BP_WATCHPOINT_HIT_WRITE) != 0;
-            bool same_el = arm_debug_target_el(env) == arm_current_el(env);
 
             cs->watchpoint_hit = NULL;
 
             env->exception.fsr = arm_debug_exception_fsr(env);
             env->exception.vaddress = wp_hit->hitaddr;
-            raise_exception(env, EXCP_DATA_ABORT,
-                    syn_watchpoint(same_el, 0, wnr),
-                    arm_debug_target_el(env));
+            raise_exception_debug(env, EXCP_DATA_ABORT,
+                                  syn_watchpoint(0, 0, wnr));
         }
     } else {
         uint64_t pc = is_a64(env) ? env->pc : env->regs[15];
-        bool same_el = (arm_debug_target_el(env) == arm_current_el(env));
 
         /*
          * (1) GDB breakpoints should be handled first.
@@ -449,9 +446,7 @@ void arm_debug_excp_handler(CPUState *cs)
          * exception/security level.
          */
         env->exception.vaddress = 0;
-        raise_exception(env, EXCP_PREFETCH_ABORT,
-                        syn_breakpoint(same_el),
-                        arm_debug_target_el(env));
+        raise_exception_debug(env, EXCP_PREFETCH_ABORT, syn_breakpoint(0));
     }
 }
 
@@ -461,9 +456,6 @@ void arm_debug_excp_handler(CPUState *cs)
  */
 void HELPER(exception_bkpt_insn)(CPUARMState *env, uint32_t syndrome)
 {
-    int debug_el = arm_debug_target_el(env);
-    int cur_el = arm_current_el(env);
-
     /* FSR will only be used if the debug target EL is AArch32. */
     env->exception.fsr = arm_debug_exception_fsr(env);
     /*
@@ -472,18 +464,7 @@ void HELPER(exception_bkpt_insn)(CPUARMState *env, uint32_t syndrome)
      * exception/security level.
      */
     env->exception.vaddress = 0;
-    /*
-     * Other kinds of architectural debug exception are ignored if
-     * they target an exception level below the current one (in QEMU
-     * this is checked by arm_generate_debug_exceptions()). Breakpoint
-     * instructions are special because they always generate an exception
-     * to somewhere: if they can't go to the configured debug exception
-     * level they are taken to the current exception level.
-     */
-    if (debug_el < cur_el) {
-        debug_el = cur_el;
-    }
-    raise_exception(env, EXCP_BKPT, syndrome, debug_el);
+    raise_exception_debug(env, EXCP_BKPT, syndrome);
 }
 
 #if !defined(CONFIG_USER_ONLY)
diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
index 0a50dbf274..c4988b6c41 100644
--- a/target/arm/op_helper.c
+++ b/target/arm/op_helper.c
@@ -65,15 +65,11 @@ int exception_target_el(CPUARMState *env, int cur_el, uint32_t *psyn)
     return 1;
 }
 
-void raise_exception(CPUARMState *env, uint32_t excp, uint32_t syndrome,
-                     uint32_t cur_or_target_el)
+G_NORETURN static
+void raise_exception_int(CPUARMState *env, uint32_t excp,
+                         uint32_t syndrome, uint32_t target_el)
 {
     CPUState *cs = env_cpu(env);
-    int target_el = cur_or_target_el;
-
-    if (cur_or_target_el <= 1) {
-        target_el = exception_target_el(env, cur_or_target_el, &syndrome);
-    }
 
     assert(!excp_is_internal(excp));
     cs->exception_index = excp;
@@ -82,6 +78,39 @@ void raise_exception(CPUARMState *env, uint32_t excp, uint32_t syndrome,
     cpu_loop_exit(cs);
 }
 
+void raise_exception(CPUARMState *env, uint32_t excp, uint32_t syndrome,
+                     uint32_t cur_or_target_el)
+{
+    int target_el = cur_or_target_el;
+    if (cur_or_target_el <= 1) {
+        target_el = exception_target_el(env, cur_or_target_el, &syndrome);
+    }
+    raise_exception_int(env, excp, syndrome, target_el);
+}
+
+void raise_exception_debug(CPUARMState *env, uint32_t excp, uint32_t syndrome)
+{
+    int cur_el = arm_current_el(env);
+    int debug_el = arm_debug_target_el(env);
+
+    /*
+     * Most kinds of architectural debug exception are ignored if
+     * they target an exception level below the current (in QEMU
+     * this is checked by arm_generate_debug_exceptions()).
+     * Breakpoint instructions are special because they always generate
+     * an exception to somewhere: if they can't go to the configured
+     * debug exception level they are taken to the current exception level.
+     */
+    if (excp == EXCP_BKPT) {
+        debug_el = MAX(cur_el, debug_el);
+    } else {
+        assert(debug_el >= cur_el);
+        syndrome |= (debug_el == cur_el) << ARM_EL_EC_SHIFT;
+    }
+
+    raise_exception_int(env, excp, syndrome, debug_el);
+}
+
 void raise_exception_ra(CPUARMState *env, uint32_t excp, uint32_t syndrome,
                         uint32_t cur_or_target_el, uintptr_t ra)
 {
-- 
2.34.1

[PATCH 13/18] target/arm: Move MDCR_TDE test into exception_target_el

[Richard Henderson]

Add a debug parameter, and when true test MDCR_EL2.TDE.
Use this in arm_debug_target_el.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/internals.h  | 20 +++-----------------
 target/arm/op_helper.c  | 12 ++++++++++--
 target/arm/tlb_helper.c |  4 ++--
 3 files changed, 15 insertions(+), 21 deletions(-)

diff --git a/target/arm/internals.h b/target/arm/internals.h
index 6df38db836..fbb69e6919 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -1074,27 +1074,13 @@ typedef struct ARMVAParameters {
 ARMVAParameters aa64_va_parameters(CPUARMState *env, uint64_t va,
                                    ARMMMUIdx mmu_idx, bool data);
 
-int exception_target_el(CPUARMState *env, int cur_el, uint32_t *psyn);
+int exception_target_el(CPUARMState *env, int cur_el,
+                        uint32_t *psyn, bool debug);
 
 /* Return the Exception Level targeted by debug exceptions. */
 static inline int arm_debug_target_el(CPUARMState *env)
 {
-    bool secure = arm_is_secure(env);
-    bool route_to_el2 = false;
-
-    if (arm_is_el2_enabled(env)) {
-        route_to_el2 = env->cp15.hcr_el2 & HCR_TGE ||
-                       env->cp15.mdcr_el2 & MDCR_TDE;
-    }
-
-    if (route_to_el2) {
-        return 2;
-    } else if (arm_feature(env, ARM_FEATURE_EL3) &&
-               !arm_el_is_aa64(env, 3) && secure) {
-        return 3;
-    } else {
-        return 1;
-    }
+    return exception_target_el(env, 0, NULL, true);
 }
 
 /* Determine if allocation tags are available.  */
diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
index c4988b6c41..9fc9ab3d20 100644
--- a/target/arm/op_helper.c
+++ b/target/arm/op_helper.c
@@ -28,7 +28,8 @@
 #define SIGNBIT (uint32_t)0x80000000
 #define SIGNBIT64 ((uint64_t)1 << 63)
 
-int exception_target_el(CPUARMState *env, int cur_el, uint32_t *psyn)
+int exception_target_el(CPUARMState *env, int cur_el,
+                        uint32_t *psyn, bool debug)
 {
     /*
      * FIXME: The following tests really apply to an EL0 origin,
@@ -62,6 +63,12 @@ int exception_target_el(CPUARMState *env, int cur_el, uint32_t *psyn)
         return 2;
     }
 
+    if (debug
+        && (env->cp15.mdcr_el2 & MDCR_TDE)
+        && arm_is_el2_enabled(env)) {
+        return 2;
+    }
+
     return 1;
 }
 
@@ -83,7 +90,8 @@ void raise_exception(CPUARMState *env, uint32_t excp, uint32_t syndrome,
 {
     int target_el = cur_or_target_el;
     if (cur_or_target_el <= 1) {
-        target_el = exception_target_el(env, cur_or_target_el, &syndrome);
+        target_el = exception_target_el(env, cur_or_target_el,
+                                        &syndrome, false);
     }
     raise_exception_int(env, excp, syndrome, target_el);
 }
diff --git a/target/arm/tlb_helper.c b/target/arm/tlb_helper.c
index 573e18f830..3bf4107faa 100644
--- a/target/arm/tlb_helper.c
+++ b/target/arm/tlb_helper.c
@@ -90,7 +90,7 @@ void arm_deliver_fault(ARMCPU *cpu, vaddr addr,
     uint32_t syn, exc, fsr, fsc;
 
     cur_el = arm_current_el(env);
-    target_el = exception_target_el(env, cur_el, NULL);
+    target_el = exception_target_el(env, cur_el, NULL, false);
 
     if (fi->stage2) {
         target_el = 2;
@@ -141,7 +141,7 @@ void arm_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
 void helper_exception_pc_alignment(CPUARMState *env, target_ulong pc)
 {
     ARMMMUFaultInfo fi = { .type = ARMFault_Alignment };
-    int target_el = exception_target_el(env, arm_current_el(env), NULL);
+    int target_el = exception_target_el(env, arm_current_el(env), NULL, false);
     int mmu_idx = cpu_mmu_index(env, true);
     uint32_t fsc;
 
-- 
2.34.1

[PATCH 14/18] target/arm: Mark exception helpers as noreturn

[Richard Henderson]

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/helper.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/target/arm/helper.h b/target/arm/helper.h
index b1334e0c42..5161cdf73d 100644
--- a/target/arm/helper.h
+++ b/target/arm/helper.h
@@ -44,9 +44,9 @@ DEF_HELPER_FLAGS_2(usad8, TCG_CALL_NO_RWG_SE, i32, i32, i32)
 
 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_2(exception_internal, noreturn, env, i32)
+DEF_HELPER_4(exception_with_syndrome, noreturn, env, i32, i32, i32)
+DEF_HELPER_2(exception_bkpt_insn, noreturn, env, i32)
 DEF_HELPER_2(exception_pc_alignment, noreturn, env, tl)
 DEF_HELPER_1(setend, void, env)
 DEF_HELPER_2(wfi, void, env, i32)
-- 
2.34.1

[PATCH 15/18] target/arm: Create helper_exception_swstep

[Richard Henderson]

Move the computation from gen_swstep_exception into a helper.
The assert removed here is present in raise_exception_debug.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/helper.h       |  1 +
 target/arm/translate.h    | 12 +++---------
 target/arm/debug_helper.c |  5 +++++
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/target/arm/helper.h b/target/arm/helper.h
index 5161cdf73d..f3fd53f3f9 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,
 DEF_HELPER_2(exception_internal, noreturn, env, i32)
 DEF_HELPER_4(exception_with_syndrome, noreturn, env, i32, i32, i32)
 DEF_HELPER_2(exception_bkpt_insn, noreturn, env, i32)
+DEF_HELPER_2(exception_swstep, noreturn, env, i32)
 DEF_HELPER_2(exception_pc_alignment, noreturn, env, tl)
 DEF_HELPER_1(setend, void, env)
 DEF_HELPER_2(wfi, void, env, i32)
diff --git a/target/arm/translate.h b/target/arm/translate.h
index 6f0ebdc88e..c03dbfb618 100644
--- a/target/arm/translate.h
+++ b/target/arm/translate.h
@@ -340,15 +340,9 @@ 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)
 {
-    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);
+    /* Fill in the same_el field of the syndrome in the helper. */
+    uint32_t syn = syn_swstep(false, isv, ex);
+    gen_helper_exception_swstep(cpu_env, tcg_constant_i32(syn));
 }
 
 /*
diff --git a/target/arm/debug_helper.c b/target/arm/debug_helper.c
index 181ba7b042..8d87b65a8d 100644
--- a/target/arm/debug_helper.c
+++ b/target/arm/debug_helper.c
@@ -467,6 +467,11 @@ void HELPER(exception_bkpt_insn)(CPUARMState *env, uint32_t syndrome)
     raise_exception_debug(env, EXCP_BKPT, syndrome);
 }
 
+void HELPER(exception_swstep)(CPUARMState *env, uint32_t syndrome)
+{
+    raise_exception_debug(env, EXCP_UDEF, syndrome);
+}
+
 #if !defined(CONFIG_USER_ONLY)
 
 vaddr arm_adjust_watchpoint_address(CPUState *cs, vaddr addr, int len)
-- 
2.34.1

[PATCH 16/18] target/arm: Remove TBFLAG_ANY.DEBUG_TARGET_EL

[Richard Henderson]

We no longer need this value during translation,
as it is now handled within the helpers.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/cpu.h           |  6 ++----
 target/arm/translate.h     |  2 --
 target/arm/helper.c        | 12 ++----------
 target/arm/translate-a64.c |  1 -
 target/arm/translate.c     |  1 -
 5 files changed, 4 insertions(+), 18 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 90cdc7b1de..5bc6382fce 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -3064,11 +3064,9 @@ FIELD(TBFLAG_ANY, BE_DATA, 3, 1)
 FIELD(TBFLAG_ANY, MMUIDX, 4, 4)
 /* Target EL if we take a floating-point-disabled exception */
 FIELD(TBFLAG_ANY, FPEXC_EL, 8, 2)
-/* For A-profile only, target EL for debug exceptions.  */
-FIELD(TBFLAG_ANY, DEBUG_TARGET_EL, 10, 2)
 /* Memory operations require alignment: SCTLR_ELx.A or CCR.UNALIGN_TRP */
-FIELD(TBFLAG_ANY, ALIGN_MEM, 12, 1)
-FIELD(TBFLAG_ANY, PSTATE__IL, 13, 1)
+FIELD(TBFLAG_ANY, ALIGN_MEM, 10, 1)
+FIELD(TBFLAG_ANY, PSTATE__IL, 11, 1)
 
 /*
  * Bit usage when in AArch32 state, both A- and M-profile.
diff --git a/target/arm/translate.h b/target/arm/translate.h
index c03dbfb618..cd9ee41bbd 100644
--- a/target/arm/translate.h
+++ b/target/arm/translate.h
@@ -59,8 +59,6 @@ typedef struct DisasContext {
      */
     uint32_t svc_imm;
     int current_el;
-    /* Debug target exception level for single-step exceptions */
-    int debug_target_el;
     GHashTable *cp_regs;
     uint64_t features; /* CPU features bits */
     bool aarch64;
diff --git a/target/arm/helper.c b/target/arm/helper.c
index e0be96b988..63c3fee5ff 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -13626,18 +13626,10 @@ static CPUARMTBFlags rebuild_hflags_m32(CPUARMState *env, int fp_el,
     return rebuild_hflags_common_32(env, fp_el, mmu_idx, flags);
 }
 
-static CPUARMTBFlags rebuild_hflags_aprofile(CPUARMState *env)
-{
-    CPUARMTBFlags flags = {};
-
-    DP_TBFLAG_ANY(flags, DEBUG_TARGET_EL, arm_debug_target_el(env));
-    return flags;
-}
-
 static CPUARMTBFlags rebuild_hflags_a32(CPUARMState *env, int fp_el,
                                         ARMMMUIdx mmu_idx)
 {
-    CPUARMTBFlags flags = rebuild_hflags_aprofile(env);
+    CPUARMTBFlags flags = {};
     int el = arm_current_el(env);
 
     if (arm_sctlr(env, el) & SCTLR_A) {
@@ -13663,7 +13655,7 @@ static CPUARMTBFlags rebuild_hflags_a32(CPUARMState *env, int fp_el,
 static CPUARMTBFlags rebuild_hflags_a64(CPUARMState *env, int el, int fp_el,
                                         ARMMMUIdx mmu_idx)
 {
-    CPUARMTBFlags flags = rebuild_hflags_aprofile(env);
+    CPUARMTBFlags flags = {};
     ARMMMUIdx stage1 = stage_1_mmu_idx(mmu_idx);
     uint64_t tcr = regime_tcr(env, mmu_idx)->raw_tcr;
     uint64_t sctlr;
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index f502545307..cc9344b015 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -14645,7 +14645,6 @@ static void aarch64_tr_init_disas_context(DisasContextBase *dcbase,
     dc->ss_active = EX_TBFLAG_ANY(tb_flags, SS_ACTIVE);
     dc->pstate_ss = EX_TBFLAG_ANY(tb_flags, PSTATE__SS);
     dc->is_ldex = false;
-    dc->debug_target_el = EX_TBFLAG_ANY(tb_flags, 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 87a899d638..59d7542a48 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -9354,7 +9354,6 @@ static void arm_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
         dc->v7m_lspact = EX_TBFLAG_M32(tb_flags, LSPACT);
         dc->mve_no_pred = EX_TBFLAG_M32(tb_flags, MVE_NO_PRED);
     } else {
-        dc->debug_target_el = EX_TBFLAG_ANY(tb_flags, DEBUG_TARGET_EL);
         dc->sctlr_b = EX_TBFLAG_A32(tb_flags, SCTLR__B);
         dc->hstr_active = EX_TBFLAG_A32(tb_flags, HSTR_ACTIVE);
         dc->ns = EX_TBFLAG_A32(tb_flags, NS);
-- 
2.34.1

[PATCH 17/18] target/arm: Add cur_el parameter to arm_generate_debug_exceptions

[Richard Henderson]

We often have this value already handy in the caller.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/internals.h    |  2 +-
 target/arm/debug_helper.c | 11 +++++------
 target/arm/helper-a64.c   |  2 +-
 3 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/target/arm/internals.h b/target/arm/internals.h
index fbb69e6919..09d25612af 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -1308,6 +1308,6 @@ void define_cortex_a72_a57_a53_cp_reginfo(ARMCPU *cpu);
 
 void aa32_max_features(ARMCPU *cpu);
 bool arm_singlestep_active(CPUARMState *env);
-bool arm_generate_debug_exceptions(CPUARMState *env);
+bool arm_generate_debug_exceptions(CPUARMState *env, int cur_el);
 
 #endif
diff --git a/target/arm/debug_helper.c b/target/arm/debug_helper.c
index 8d87b65a8d..a5363a5048 100644
--- a/target/arm/debug_helper.c
+++ b/target/arm/debug_helper.c
@@ -94,10 +94,8 @@ static bool aa32_generate_debug_exceptions(CPUARMState *env, int cur_el)
  * CheckSoftwareStep(), where it is elided because both branches would
  * always return the same value.
  */
-bool arm_generate_debug_exceptions(CPUARMState *env)
+bool arm_generate_debug_exceptions(CPUARMState *env, int cur_el)
 {
-    int cur_el = arm_current_el(env);
-
     if (is_a64(env)) {
         return aa64_generate_debug_exceptions(env, cur_el);
     } else {
@@ -111,9 +109,10 @@ bool arm_generate_debug_exceptions(CPUARMState *env)
  */
 bool arm_singlestep_active(CPUARMState *env)
 {
+    int cur_el = arm_current_el(env);
     return extract32(env->cp15.mdscr_el1, 0, 1)
         && arm_el_is_aa64(env, arm_debug_target_el(env))
-        && arm_generate_debug_exceptions(env);
+        && arm_generate_debug_exceptions(env, cur_el);
 }
 
 /* Return true if the linked breakpoint entry lbn passes its checks */
@@ -309,7 +308,7 @@ static bool check_watchpoints(ARMCPU *cpu)
      * exceptions here then watchpoint firings are ignored.
      */
     if (extract32(env->cp15.mdscr_el1, 15, 1) == 0
-        || !arm_generate_debug_exceptions(env)) {
+        || !arm_generate_debug_exceptions(env, arm_current_el(env))) {
         return false;
     }
 
@@ -333,7 +332,7 @@ bool arm_debug_check_breakpoint(CPUState *cs)
      * exceptions here then breakpoint firings are ignored.
      */
     if (extract32(env->cp15.mdscr_el1, 15, 1) == 0
-        || !arm_generate_debug_exceptions(env)) {
+        || !arm_generate_debug_exceptions(env, arm_current_el(env))) {
         return false;
     }
 
diff --git a/target/arm/helper-a64.c b/target/arm/helper-a64.c
index 22db213aab..fe2a0aa261 100644
--- a/target/arm/helper-a64.c
+++ b/target/arm/helper-a64.c
@@ -924,7 +924,7 @@ void HELPER(exception_return)(CPUARMState *env, uint64_t new_pc)
      * We check 1 here and 2 after we've done the pstate/cpsr write() to
      * transition to the EL we're going to.
      */
-    if (arm_generate_debug_exceptions(env)) {
+    if (arm_generate_debug_exceptions(env, cur_el)) {
         spsr &= ~PSTATE_SS;
     }
 
-- 
2.34.1

[PATCH 18/18] target/arm: Remove route_to_el2 case from sve_exception_el

[Richard Henderson]

This adjustment is handled by exception_target_el.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/helper.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 63c3fee5ff..5c875927cf 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6155,8 +6155,7 @@ int sve_exception_el(CPUARMState *env, int el)
             /* fall through */
         case 0:
         case 2:
-            /* route_to_el2 */
-            return hcr_el2 & HCR_TGE ? 2 : 1;
+            return 1;
         }
 
         /* Check CPACR.FPEN.  */
-- 
2.34.1

Re: [PATCH 02/18] target/arm: Use arm_current_el for simple exceptions

[Peter Maydell]

On Mon, 23 May 2022 at 21:49, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> For these cases, the syndrome does not depend on the
> origin or target EL, so we can simply defer selection
> of the target EL to raise_exception.

The commit message says "defer to raise_exception()", but
that would mean passing 0 as the cur_or_target_el, which
mostly these changes aren't doing.

Instead it looks like what we're relying on is that
if arm_current_el() != 0 then exception_target_el() == arm_current_el(),
and if arm_current_el() is 0 then raise_exception() will
call exception_target_el() for us. Which is true, but not
really what the commit message is saying.

-- PMM

Re: [PATCH 01/18] target/arm: Allow raise_exception to handle finding target EL

[Peter Maydell]

On Mon, 23 May 2022 at 21:49, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> The work of finding the correct target EL for an exception is
> currently split between raise_exception and target_exception_el.
> Begin merging these by allowing the input to raise_exception
> to be zero and use exception_target_el for that case.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/internals.h | 11 ++++++-----
>  target/arm/op_helper.c | 13 +++++++++----
>  2 files changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index b654bee468..03363b0f32 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -111,18 +111,19 @@ FIELD(DBGWCR, SSCE, 29, 1)
>  /**
>   * raise_exception: Raise the specified exception.
>   * Raise a guest exception with the specified value, syndrome register
> - * and target exception level. This should be called from helper functions,
> - * and never returns because we will longjump back up to the CPU main loop.
> + * and the current or target exception level. This should be called from
> + * helper functions, and never returns because we will longjump back up
> + * to the CPU main loop.
>   */
>  G_NORETURN void raise_exception(CPUARMState *env, uint32_t excp,
> -                                uint32_t syndrome, uint32_t target_el);
> +                                uint32_t syndrome, uint32_t cur_or_target_el);

"cur_or_target_el" is odd, because it's mixing what the architecture
sets up as two distinct things: the state the exception is
"taken from", and the state the exception is "taken to". I was
hoping this was just a temporary thing for the purposes of the
refactoring and it would go away near the end of the series, but
it doesn't seem to.

-- PMM

Re: [PATCH 01/18] target/arm: Allow raise_exception to handle finding target EL

[Richard Henderson]

On 5/30/22 05:44, Peter Maydell wrote:
>>   G_NORETURN void raise_exception(CPUARMState *env, uint32_t excp,
>> -                                uint32_t syndrome, uint32_t target_el);
>> +                                uint32_t syndrome, uint32_t cur_or_target_el);
> 
> "cur_or_target_el" is odd, because it's mixing what the architecture
> sets up as two distinct things: the state the exception is
> "taken from", and the state the exception is "taken to". I was
> hoping this was just a temporary thing for the purposes of the
> refactoring and it would go away near the end of the series, but
> it doesn't seem to.

No, sorry.  Most of the time it's cur_el, except from cpregs, where we get directed to a 
specific higher el.  There may be some way to split the helpers...

I'll have another go at this reorg this week.  If it still doesn't feel cleaner, we can 
drop it, and I'll make some changes to the SME patch set building on this.


r~

Re: [PATCH 01/18] target/arm: Allow raise_exception to handle finding target EL

[Peter Maydell]

On Mon, 30 May 2022 at 17:39, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 5/30/22 05:44, Peter Maydell wrote:
> >>   G_NORETURN void raise_exception(CPUARMState *env, uint32_t excp,
> >> -                                uint32_t syndrome, uint32_t target_el);
> >> +                                uint32_t syndrome, uint32_t cur_or_target_el);
> >
> > "cur_or_target_el" is odd, because it's mixing what the architecture
> > sets up as two distinct things: the state the exception is
> > "taken from", and the state the exception is "taken to". I was
> > hoping this was just a temporary thing for the purposes of the
> > refactoring and it would go away near the end of the series, but
> > it doesn't seem to.
>
> No, sorry.  Most of the time it's cur_el, except from cpregs, where we get directed to a
> specific higher el.  There may be some way to split the helpers...
>
> I'll have another go at this reorg this week.  If it still doesn't feel cleaner, we can
> drop it, and I'll make some changes to the SME patch set building on this.

I was wondering if it would work better the other way around, so that
raise_exception() doesn't mess with the target_el at all, and all the
"work out which EL to take the exception to" is done in
target_exception_el() and similar ?

-- PMM

Re: [PATCH 01/18] target/arm: Allow raise_exception to handle finding target EL

[Richard Henderson]

On 5/30/22 12:01, Peter Maydell wrote:
>> I'll have another go at this reorg this week.  If it still doesn't feel cleaner, we can
>> drop it, and I'll make some changes to the SME patch set building on this.
> 
> I was wondering if it would work better the other way around, so that
> raise_exception() doesn't mess with the target_el at all, and all the
> "work out which EL to take the exception to" is done in
> target_exception_el() and similar ?

We need some place to put the EC_ADVSIMDFPACCESSTRAP special case, and it has to be done 
at the same time we're handling HCR_EL2.TGE.  Perhaps that can be separated out to its own 
helper.


r~

Re: [PATCH 05/18] target/arm: Move arm_singlestep_active out of line

[Peter Maydell]

On Mon, 23 May 2022 at 21:53, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Move the function to debug_helper.c, and the
> declaration to internals.h.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 06/18] target/arm: Move arm_generate_debug_exceptions out of line

[Peter Maydell]

On Mon, 23 May 2022 at 21:53, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Move arm_generate_debug_exceptions and its two subroutines,
> {aa32,aa64}_generate_debug_exceptions into debug_helper.c,
> and the one interface declaration to internals.h.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 07/18] target/arm: Hoist arm_current_el in arm_generate_debug_exceptions

[Peter Maydell]

On Mon, 23 May 2022 at 21:58, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Read this value once in the main function, and pass it
> around between the subroutines.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---

What's the benefit from doing this ?

thanks
-- PMM

Re: [PATCH 08/18] target/arm: Use is_a64 in arm_generate_debug_exceptions

[Peter Maydell]

On Mon, 23 May 2022 at 21:59, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Use the accessor rather than the raw structure member.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 11/18] target/arm: Move arm_debug_target_el to internals.h

[Peter Maydell]

On Mon, 23 May 2022 at 22:05, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> This function is private to the implementation.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 14/18] target/arm: Mark exception helpers as noreturn

[Peter Maydell]

On Mon, 23 May 2022 at 22:06, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 17/18] target/arm: Add cur_el parameter to arm_generate_debug_exceptions

[Peter Maydell]

On Mon, 23 May 2022 at 22:07, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> We often have this value already handy in the caller.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

True, but it makes the function clunkier to use. Does it really
make a noticeable difference to performance ?

thanks
-- PMM

Re: [PATCH 07/18] target/arm: Hoist arm_current_el in arm_generate_debug_exceptions

[Richard Henderson]

On 5/31/22 05:04, Peter Maydell wrote:
> On Mon, 23 May 2022 at 21:58, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> Read this value once in the main function, and pass it
>> around between the subroutines.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
> 
> What's the benefit from doing this ?

Just trying to reduce the number of times that arm_current_el is computed within these 
tightly coupled functions.


r~

Re: [PATCH 17/18] target/arm: Add cur_el parameter to arm_generate_debug_exceptions

[Richard Henderson]

On 5/31/22 05:07, Peter Maydell wrote:
> On Mon, 23 May 2022 at 22:07, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> We often have this value already handy in the caller.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> 
> True, but it makes the function clunkier to use. Does it really
> make a noticeable difference to performance ?

Probably not.  I'll drop this one.


r~