diff mbox

target-arm: Fix "no 64-bit EL2" assumption in arm_excp_unmasked()

Message ID 1444327729-4120-1-git-send-email-peter.maydell@linaro.org
State Superseded
Headers show

Commit Message

Peter Maydell Oct. 8, 2015, 6:08 p.m. UTC
The code in arm_excp_unmasked() suppresses the ability of PSTATE.AIF
to mask exceptions from a lower EL targeting EL2 or EL3 if the
CPU is 64-bit. This is correct for a target of EL3, but not correct
for targeting EL2. Further, we go to some effort to calculate
scr and hcr values which are not used at all for the 64-bit CPU
case.

Rearrange the code to correctly implement the 64-bit CPU logic
and keep the hcr/scr calculations in the 32-bit CPU codepath.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
As promised, here's the patch that fixes the assumption about
64-bit implying no EL2 in arm_excp_unmasked(). This applies on
top of my current target-arm.next tree, since it has Sergey's
patch in it which touches the same code.



 target-arm/cpu.h | 82 +++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 52 insertions(+), 30 deletions(-)

Comments

Edgar E. Iglesias Oct. 14, 2015, 4:05 p.m. UTC | #1
On Thu, Oct 08, 2015 at 07:08:49PM +0100, Peter Maydell wrote:
> The code in arm_excp_unmasked() suppresses the ability of PSTATE.AIF
> to mask exceptions from a lower EL targeting EL2 or EL3 if the
> CPU is 64-bit. This is correct for a target of EL3, but not correct
> for targeting EL2. Further, we go to some effort to calculate
> scr and hcr values which are not used at all for the 64-bit CPU
> case.
> 
> Rearrange the code to correctly implement the 64-bit CPU logic
> and keep the hcr/scr calculations in the 32-bit CPU codepath.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Looks good to me! I gave it a test run together with
Sergey's eb4ac7e08b342ae20661a4feac885b8e2e966418 from
target-arm.next.

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Tested-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>



> ---
> As promised, here's the patch that fixes the assumption about
> 64-bit implying no EL2 in arm_excp_unmasked(). This applies on
> top of my current target-arm.next tree, since it has Sergey's
> patch in it which touches the same code.
> 
> 
> 
>  target-arm/cpu.h | 82 +++++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 52 insertions(+), 30 deletions(-)
> 
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 99b34c4..6b6e4e0 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -1524,8 +1524,6 @@ static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx,
>      CPUARMState *env = cs->env_ptr;
>      unsigned int cur_el = arm_current_el(env);
>      bool secure = arm_is_secure(env);
> -    bool scr;
> -    bool hcr;
>      bool pstate_unmasked;
>      int8_t unmasked = 0;
>  
> @@ -1539,31 +1537,10 @@ static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx,
>  
>      switch (excp_idx) {
>      case EXCP_FIQ:
> -        /* If FIQs are routed to EL3 or EL2 then there are cases where we
> -         * override the CPSR.F in determining if the exception is masked or
> -         * not.  If neither of these are set then we fall back to the CPSR.F
> -         * setting otherwise we further assess the state below.
> -         */
> -        hcr = (env->cp15.hcr_el2 & HCR_FMO);
> -        scr = (env->cp15.scr_el3 & SCR_FIQ);
> -
> -        /* When EL3 is 32-bit, the SCR.FW bit controls whether the CPSR.F bit
> -         * masks FIQ interrupts when taken in non-secure state.  If SCR.FW is
> -         * set then FIQs can be masked by CPSR.F when non-secure but only
> -         * when FIQs are only routed to EL3.
> -         */
> -        scr = scr && !((env->cp15.scr_el3 & SCR_FW) && !hcr);
>          pstate_unmasked = !(env->daif & PSTATE_F);
>          break;
>  
>      case EXCP_IRQ:
> -        /* When EL3 execution state is 32-bit, if HCR.IMO is set then we may
> -         * override the CPSR.I masking when in non-secure state.  The SCR.IRQ
> -         * setting has already been taken into consideration when setting the
> -         * target EL, so it does not have a further affect here.
> -         */
> -        hcr = (env->cp15.hcr_el2 & HCR_IMO);
> -        scr = false;
>          pstate_unmasked = !(env->daif & PSTATE_I);
>          break;
>  
> @@ -1588,13 +1565,58 @@ static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx,
>       * interrupt.
>       */
>      if ((target_el > cur_el) && (target_el != 1)) {
> -        /* ARM_FEATURE_AARCH64 enabled means the highest EL is AArch64.
> -         * This code currently assumes that EL2 is not implemented
> -         * (and so that highest EL will be 3 and the target_el also 3).
> -         */
> -        if (arm_feature(env, ARM_FEATURE_AARCH64) ||
> -            ((scr || hcr) && (!secure))) {
> -            unmasked = 1;
> +        /* Exceptions targeting a higher EL may not be maskable */
> +        if (arm_feature(env, ARM_FEATURE_AARCH64)) {
> +            /* 64-bit masking rules are simple: exceptions to EL3
> +             * can't be masked, and exceptions to EL2 can only be
> +             * masked from Secure state. The HCR and SCR settings
> +             * don't affect the masking logic, only the interrupt routing.
> +             */
> +            if (target_el == 3 || !secure) {
> +                unmasked = 1;
> +            }
> +        } else {
> +            /* The old 32-bit-only environment has a more complicated
> +             * masking setup. HCR and SCR bits not only affect interrupt
> +             * routing but also change the behaviour of masking.
> +             */
> +            bool hcr, scr;
> +
> +            switch (excp_idx) {
> +            case EXCP_FIQ:
> +                /* If FIQs are routed to EL3 or EL2 then there are cases where
> +                 * we override the CPSR.F in determining if the exception is
> +                 * masked or not. If neither of these are set then we fall back
> +                 * to the CPSR.F setting otherwise we further assess the state
> +                 * below.
> +                 */
> +                hcr = (env->cp15.hcr_el2 & HCR_FMO);
> +                scr = (env->cp15.scr_el3 & SCR_FIQ);
> +
> +                /* When EL3 is 32-bit, the SCR.FW bit controls whether the
> +                 * CPSR.F bit masks FIQ interrupts when taken in non-secure
> +                 * state. If SCR.FW is set then FIQs can be masked by CPSR.F
> +                 * when non-secure but only when FIQs are only routed to EL3.
> +                 */
> +                scr = scr && !((env->cp15.scr_el3 & SCR_FW) && !hcr);
> +                break;
> +            case EXCP_IRQ:
> +                /* When EL3 execution state is 32-bit, if HCR.IMO is set then
> +                 * we may override the CPSR.I masking when in non-secure state.
> +                 * The SCR.IRQ setting has already been taken into consideration
> +                 * when setting the target EL, so it does not have a further
> +                 * affect here.
> +                 */
> +                hcr = (env->cp15.hcr_el2 & HCR_IMO);
> +                scr = false;
> +                break;
> +            default:
> +                g_assert_not_reached();
> +            }
> +
> +            if ((scr || hcr) && !secure) {
> +                unmasked = 1;
> +            }
>          }
>      }
>  
> -- 
> 1.9.1
>
diff mbox

Patch

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 99b34c4..6b6e4e0 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -1524,8 +1524,6 @@  static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx,
     CPUARMState *env = cs->env_ptr;
     unsigned int cur_el = arm_current_el(env);
     bool secure = arm_is_secure(env);
-    bool scr;
-    bool hcr;
     bool pstate_unmasked;
     int8_t unmasked = 0;
 
@@ -1539,31 +1537,10 @@  static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx,
 
     switch (excp_idx) {
     case EXCP_FIQ:
-        /* If FIQs are routed to EL3 or EL2 then there are cases where we
-         * override the CPSR.F in determining if the exception is masked or
-         * not.  If neither of these are set then we fall back to the CPSR.F
-         * setting otherwise we further assess the state below.
-         */
-        hcr = (env->cp15.hcr_el2 & HCR_FMO);
-        scr = (env->cp15.scr_el3 & SCR_FIQ);
-
-        /* When EL3 is 32-bit, the SCR.FW bit controls whether the CPSR.F bit
-         * masks FIQ interrupts when taken in non-secure state.  If SCR.FW is
-         * set then FIQs can be masked by CPSR.F when non-secure but only
-         * when FIQs are only routed to EL3.
-         */
-        scr = scr && !((env->cp15.scr_el3 & SCR_FW) && !hcr);
         pstate_unmasked = !(env->daif & PSTATE_F);
         break;
 
     case EXCP_IRQ:
-        /* When EL3 execution state is 32-bit, if HCR.IMO is set then we may
-         * override the CPSR.I masking when in non-secure state.  The SCR.IRQ
-         * setting has already been taken into consideration when setting the
-         * target EL, so it does not have a further affect here.
-         */
-        hcr = (env->cp15.hcr_el2 & HCR_IMO);
-        scr = false;
         pstate_unmasked = !(env->daif & PSTATE_I);
         break;
 
@@ -1588,13 +1565,58 @@  static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx,
      * interrupt.
      */
     if ((target_el > cur_el) && (target_el != 1)) {
-        /* ARM_FEATURE_AARCH64 enabled means the highest EL is AArch64.
-         * This code currently assumes that EL2 is not implemented
-         * (and so that highest EL will be 3 and the target_el also 3).
-         */
-        if (arm_feature(env, ARM_FEATURE_AARCH64) ||
-            ((scr || hcr) && (!secure))) {
-            unmasked = 1;
+        /* Exceptions targeting a higher EL may not be maskable */
+        if (arm_feature(env, ARM_FEATURE_AARCH64)) {
+            /* 64-bit masking rules are simple: exceptions to EL3
+             * can't be masked, and exceptions to EL2 can only be
+             * masked from Secure state. The HCR and SCR settings
+             * don't affect the masking logic, only the interrupt routing.
+             */
+            if (target_el == 3 || !secure) {
+                unmasked = 1;
+            }
+        } else {
+            /* The old 32-bit-only environment has a more complicated
+             * masking setup. HCR and SCR bits not only affect interrupt
+             * routing but also change the behaviour of masking.
+             */
+            bool hcr, scr;
+
+            switch (excp_idx) {
+            case EXCP_FIQ:
+                /* If FIQs are routed to EL3 or EL2 then there are cases where
+                 * we override the CPSR.F in determining if the exception is
+                 * masked or not. If neither of these are set then we fall back
+                 * to the CPSR.F setting otherwise we further assess the state
+                 * below.
+                 */
+                hcr = (env->cp15.hcr_el2 & HCR_FMO);
+                scr = (env->cp15.scr_el3 & SCR_FIQ);
+
+                /* When EL3 is 32-bit, the SCR.FW bit controls whether the
+                 * CPSR.F bit masks FIQ interrupts when taken in non-secure
+                 * state. If SCR.FW is set then FIQs can be masked by CPSR.F
+                 * when non-secure but only when FIQs are only routed to EL3.
+                 */
+                scr = scr && !((env->cp15.scr_el3 & SCR_FW) && !hcr);
+                break;
+            case EXCP_IRQ:
+                /* When EL3 execution state is 32-bit, if HCR.IMO is set then
+                 * we may override the CPSR.I masking when in non-secure state.
+                 * The SCR.IRQ setting has already been taken into consideration
+                 * when setting the target EL, so it does not have a further
+                 * affect here.
+                 */
+                hcr = (env->cp15.hcr_el2 & HCR_IMO);
+                scr = false;
+                break;
+            default:
+                g_assert_not_reached();
+            }
+
+            if ((scr || hcr) && !secure) {
+                unmasked = 1;
+            }
         }
     }