diff mbox series

[v2,24/25] target/arm: Rearrange Secure PL1 test in arm_debug_target_el

Message ID 20220607024734.541321-25-richard.henderson@linaro.org
State New
Headers show
Series target/arm: tidy exception routing | expand

Commit Message

Richard Henderson June 7, 2022, 2:47 a.m. UTC 
Not a bug, because arm_is_el2_enabled tests for secure,
and SCR_EL3.EEL2 cannot be set for AArch32, however the
ordering of the tests looks odd.  Mirror the structure
over in exception_target_el().

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

Comments

Peter Maydell June 9, 2022, 4:53 p.m. UTC | #1 
On Tue, 7 Jun 2022 at 04:06, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Not a bug, because arm_is_el2_enabled tests for secure,
> and SCR_EL3.EEL2 cannot be set for AArch32, however the
> ordering of the tests looks odd.  Mirror the structure
> over in exception_target_el().

I think the code is following the ordering in the
DebugTarget() and DebugTargetFrom() pseudocode (or else some other
equivalent function in an older version of the Arm ARM...)

> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/debug_helper.c | 30 ++++++++++++++++--------------
>  1 file changed, 16 insertions(+), 14 deletions(-)
>
> diff --git a/target/arm/debug_helper.c b/target/arm/debug_helper.c
> index b18a6bd3a2..59dfcb5d5c 100644
> --- a/target/arm/debug_helper.c
> +++ b/target/arm/debug_helper.c
> @@ -15,22 +15,24 @@
>  /* Return the Exception Level targeted by debug exceptions. */
>  static 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) {
> +    /*
> +     * No such thing as secure EL1 if EL3 is AArch32.
> +     * Remap Secure PL1 to EL3.
> +     */

I think you're also relying on there being no secure EL2
if EL3 is AArch32 (otherwise an exception from secure EL0
might need to be routed to secure EL2, not EL3).

> +    if (arm_is_secure(env) && !arm_el_is_aa64(env, 3)) {
>          return 3;
> -    } else {
> -        return 1;
>      }
> +
> +    /*
> +     * HCR.TGE redirects EL0 exceptions from EL1 to EL2.
> +     * MDCR.TDE redirects both EL0 and EL1 debug exceptions to EL2.
> +     */
> +    if (arm_is_el2_enabled(env) &&
> +        (env->cp15.hcr_el2 & HCR_TGE || env->cp15.mdcr_el2 & MDCR_TDE)) {
> +        return 2;
> +    }
> +
> +    return 1;
>  }

Anyway, if you prefer this way around
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

though I think there is usually some value in following
the pseudocode's arrangement.

thanks
-- PMM
Richard Henderson June 9, 2022, 7:49 p.m. UTC | #2 
On 6/9/22 09:53, Peter Maydell wrote:
> On Tue, 7 Jun 2022 at 04:06, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> Not a bug, because arm_is_el2_enabled tests for secure,
>> and SCR_EL3.EEL2 cannot be set for AArch32, however the
>> ordering of the tests looks odd.  Mirror the structure
>> over in exception_target_el().
> 
> I think the code is following the ordering in the
> DebugTarget() and DebugTargetFrom() pseudocode (or else some other
> equivalent function in an older version of the Arm ARM...)

Fair enough.

> I think you're also relying on there being no secure EL2
> if EL3 is AArch32 (otherwise an exception from secure EL0
> might need to be routed to secure EL2, not EL3).

Correct, though I don't imagine SEL2 will ever apply to A32.

I'll drop the patch though.


r~
diff mbox series

Patch

diff --git a/target/arm/debug_helper.c b/target/arm/debug_helper.c
index b18a6bd3a2..59dfcb5d5c 100644
--- a/target/arm/debug_helper.c
+++ b/target/arm/debug_helper.c
@@ -15,22 +15,24 @@ 
 /* Return the Exception Level targeted by debug exceptions. */
 static 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) {
+    /*
+     * No such thing as secure EL1 if EL3 is AArch32.
+     * Remap Secure PL1 to EL3.
+     */
+    if (arm_is_secure(env) && !arm_el_is_aa64(env, 3)) {
         return 3;
-    } else {
-        return 1;
     }
+
+    /*
+     * HCR.TGE redirects EL0 exceptions from EL1 to EL2.
+     * MDCR.TDE redirects both EL0 and EL1 debug exceptions to EL2.
+     */
+    if (arm_is_el2_enabled(env) &&
+        (env->cp15.hcr_el2 & HCR_TGE || env->cp15.mdcr_el2 & MDCR_TDE)) {
+        return 2;
+    }
+
+    return 1;
 }
 
 /*