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

2022-06-09 Thread Richard Henderson 

On 6/9/22 09:53, Peter Maydell wrote:

On Tue, 7 Jun 2022 at 04:06, Richard Henderson
 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~

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

2022-06-09 Thread Peter Maydell 
On Tue, 7 Jun 2022 at 04:06, Richard Henderson
 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 
> ---
>  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 

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

thanks
-- PMM

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

2022-06-06 Thread Richard Henderson 
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 
---
 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.
+ */
+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;
 }
 
 /*
-- 
2.34.1