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

[Peter Maydell]

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