qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v7 07/32] target-arm: extend async excp masking


From: Greg Bellows
Subject: Re: [Qemu-devel] [PATCH v7 07/32] target-arm: extend async excp masking
Date: Fri, 24 Oct 2014 17:43:47 -0500

Hi Peter,

Based on our discussion, I looked into a table lookup approach to replace the arm_phys_excp_target_el() as we discussed.  I have something working but still need to verify it is 100% correct.  Before I went much further, I thought I'd share my findings.

In order to do the table in a way that it does not blow-up with a bunch of duplicate data, we still need a function for accessing the table.  The function will still have a good part of what the existing arm_phys_excp_target_el() has at the beginning.  This is necessary as we need to decide which of the SCR and HCR bits need to be plugged into the table.  

In addition, we need the following: 
  • The table has to include special values to account for the cases where an interrupt is not taken so we will need logic on the other end to catch the special table value and return cur_el.
  • Either another dimension needs to be added to the table or a second table to handle the differences between 32/64-bit EL3. (Still needed)
  • Another dimension is needed to include HCR_TGE eventually.
Basically, I'm not sure that it buys us much other than a static table. Otherwise, we are swapping one bunch of logic for a different set.

Below are the changes (convert to a fixed font for better table format):

Greg

=============================

diff --git a/target-arm/helper.c b/target-arm/helper.c
index b4db1a5..fd3d637 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -4028,6 +4028,44 @@ void switch_mode(CPUARMState *env, int mode)
     env->spsr = env->banked_spsr[i];
 }
 
+const unsigned int aarch64_target_el[2][2][2][2][4] = {
+    /* Physical Interrupt Target EL Lookup Table
+     *
+     * [ From ARM ARM section D1.14.1 (Table D1-11) ]
+     *
+     * The below multi-dimensional table is used for looking up the target
+     * exception level given numerous condition criteria.  Specifically, the
+     * target EL is based on SCR and HCR routing controls as well as the
+     * currently executing EL and secure state.
+     *
+     *   The table values are as such:
+     *   0-3 = EL0-EL3
+     *     8 = Cannot occur
+     *     9 = Interrupt not taken
+     *
+     *      SCR     HCR
+     *       EA     AMO
+     *      IRQ     IMO             FROM
+     *      FIQ RW  FMO  NS    EL0 EL1 EL2 EL3
+     */
+    {{{{  /* 0   0   0   0  */  1,  1,  8,  9  },
+       {  /* 0   0   0   1  */  1,  1,  2,  8  },},
+      {{  /* 0   0   1   0  */  1,  1,  8,  9  },
+       {  /* 0   0   1   1  */  2,  2,  2,  8  },},},
+     {{{  /* 0   1   0   0  */  1,  1,  8,  9  },
+       {  /* 0   1   0   1  */  1,  1,  8,  8  },},
+      {{  /* 0   1   1   0  */  1,  1,  8,  9  },
+       {  /* 0   1   1   1  */  2,  2,  2,  8  },},},},
+    {{{{  /* 1   0   0   0  */  3,  3,  8,  3  },
+       {  /* 1   0   0   1  */  3,  3,  3,  8  },},
+      {{  /* 1   0   1   0  */  3,  3,  8,  3  },
+       {  /* 1   0   1   1  */  3,  3,  3,  8  },},},
+     {{{  /* 1   1   0   0  */  3,  3,  8,  3  },
+       {  /* 1   1   0   1  */  3,  3,  3,  8  },},
+      {{  /* 1   1   1   0  */  3,  3,  8,  3  },
+       {  /* 1   1   1   1  */  3,  3,  3,  8  },},},},
+};
+
 /*
  * Determine the target EL for physical exceptions
  */
@@ -4035,69 +4073,28 @@ static inline uint32_t arm_phys_excp_target_el(CPUState *cs, ui
                                         uint32_t cur_el, bool secure)
 {
     CPUARMState *env = cs->env_ptr;
-    uint32_t target_el = 1;
-
-    /* There is no SCR or HCR routing unless the respective EL3 and EL2
-     * extensions are supported.  This initial setting affects whether any
-     * other conditions matter.
-     */
-    bool scr_routing = arm_feature(env, ARM_FEATURE_EL3); /* IRQ, FIQ, EA */
-    bool hcr_routing = arm_feature(env, ARM_FEATURE_EL2); /* IMO, FMO, AMO */
-
-    /* Fast-path if EL2 and EL3 are not enabled */
-    if (!scr_routing && !hcr_routing) {
-        return target_el;
-    }
+    uint32_t rw = ((env->cp15.scr_el3 & SCR_RW) == SCR_RW);
+    uint32_t scr;
+    uint32_t hcr;
+    uint32_t target_el;
 
     switch (excp_idx) {
     case EXCP_IRQ:
-        scr_routing &= ((env->cp15.scr_el3 & SCR_IRQ) == SCR_IRQ);
-        hcr_routing &= ((env->cp15.hcr_el2 & HCR_IMO) == HCR_IMO);
+        scr = ((env->cp15.scr_el3 & SCR_IRQ) == SCR_IRQ);
+        hcr = ((env->cp15.hcr_el2 & HCR_IMO) == HCR_IMO);
         break;
     case EXCP_FIQ:
-        scr_routing &= ((env->cp15.scr_el3 & SCR_FIQ) == SCR_FIQ);
-        hcr_routing &= ((env->cp15.hcr_el2 & HCR_FMO) == HCR_FMO);
-    }
-
-    /* If SCR routing is enabled we always go to EL3 regardless of EL3
-     * execution state
-     */
-    if (scr_routing) {
-        /* IRQ|FIQ|EA == 1 */
-        return 3;
-    }
-
-    /* If HCR.TGE is set all exceptions that would be routed to EL1 are
-     * routed to EL2 (in non-secure world).
-     */
-    hcr_routing &= (env->cp15.hcr_el2 & HCR_TGE) == HCR_TGE;
+        scr = ((env->cp15.scr_el3 & SCR_FIQ) == SCR_FIQ);
+        hcr = ((env->cp15.hcr_el2 & HCR_FMO) == HCR_FMO);
+        break;
+    default:
+        scr = ((env->cp15.scr_el3 & SCR_EA) == SCR_EA);
+        hcr = ((env->cp15.hcr_el2 & HCR_AMO) == HCR_AMO);
+        break;
+    };
 
-    /* Determine target EL according to ARM ARMv8 tables G1-15 and G1-16 */
-    if (arm_el_is_aa64(env, 3)) {
-        /* EL3 in AArch64 */
-        if (!secure) {
-            /* If non-secure, we may route to EL2 depending on other state.
-             * If we are coming from the secure world then we always route to
-             * EL1.
-             */
-            if (hcr_routing ||
-                (cur_el == 2 && !(env->cp15.scr_el3 & SCR_RW))) {
-                /* If HCR.FMO/IMO is set or we already in EL2 and it is not
-                 * configured to be AArch64 then route to EL2.
-                 */
-                target_el = 2;
-            }
-        }
-    } else {
-        /* EL3 in AArch32 */
-        if (secure) {
-            /* If coming from secure always route to EL3 */
-            target_el = 3;
-        } else if (hcr_routing || cur_el == 2) {
-            /* If HCR.FMO/IMO is set or we are already EL2 then route to EL2 */
-            target_el = 2;
-        }
-    }
+    target_el = aarch64_target_el[scr][rw][hcr][!secure][cur_el];
+    target_el = (target_el > 3) ? cur_el : target_el;
 
     return target_el;
 }



On 24 October 2014 11:25, Peter Maydell <address@hidden> wrote:
On 21 October 2014 17:55, Greg Bellows <address@hidden> wrote:
> From: Fabian Aggeler <address@hidden>
>
> This patch extends arm_excp_unmasked() according to ARM ARMv7 and
> ARM ARMv8 (all EL running in AArch32) and adds comments.
>
> If EL3 is using AArch64 IRQ/FIQ masking is ignored in
> all exception levels other than EL3 if SCR.{FIQ|IRQ} is
> set to 1 (routed to EL3).
>
> Signed-off-by: Fabian Aggeler <address@hidden>
> Signed-off-by: Greg Bellows <address@hidden>
>
> ==========
>
> v5 -> v6
> - Globally change Aarch# to AArch#
> - Fixed comment termination
>
> v4 -> v5
> - Merge with v4 patch 10
>
> Signed-off-by: Greg Bellows <address@hidden>
> ---
>  target-arm/cpu.h | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 107 insertions(+), 10 deletions(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index cb6ec5c..1a564b9 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -1246,11 +1246,8 @@ 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);
> -    unsigned int target_el = arm_excp_target_el(cs, excp_idx);
> -    /* FIXME: Use actual secure state.  */
> -    bool secure = false;
> -    /* If in EL1/0, Physical IRQ routing to EL2 only happens from NS state.  */
> -    bool irq_can_hyp = !secure && cur_el < 2 && target_el == 2;
> +    bool secure = arm_is_secure(env);
> +
>      /* ARMv7-M interrupt return works by loading a magic value
>       * into the PC.  On real hardware the load causes the
>       * return to occur.  The qemu implementation performs the
> @@ -1265,19 +1262,119 @@ static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx)
>                          && (!IS_M(env) || env->regs[15] < 0xfffffff0);
>
>      /* Don't take exceptions if they target a lower EL.  */
> -    if (cur_el > target_el) {
> +    if (cur_el > arm_excp_target_el(cs, excp_idx)) {
>          return false;
>      }
>
> +    /* ARM ARMv7 B1.8.6  Asynchronous exception masking (table B1-12/B1-13)
> +     * ARM ARMv8 G1.11.3 Asynchronous exception masking controls
> +     * (table G1-18/G1-19)
> +     */
>      switch (excp_idx) {
>      case EXCP_FIQ:
> -        if (irq_can_hyp && (env->cp15.hcr_el2 & HCR_FMO)) {
> -            return true;
> +        if (arm_feature(env, ARM_FEATURE_EL3) && arm_el_is_aa64(env, 3)) {
> +            /* If EL3 is using AArch64 and FIQs are routed to EL3 masking is
> +             * ignored in all exception levels except EL3.
> +             */
> +            if ((env->cp15.scr_el3 & SCR_FIQ) && cur_el < 3) {

Why are we recalculating whether the target level is EL3 by looking
at SCR_EL3.SCR_FIQ, rather than using the target_el which
arm_excp_target_el() returns?

> +                return true;
> +            }
> +            /* If we are in EL3 but FIQs are not routed to EL3 the exception
> +             * is not taken but remains pending.
> +             */
> +            if (!(env->cp15.scr_el3 & SCR_FIQ) && cur_el == 3) {
> +                return false;

Isn't this unreachable? If SCR_FIQ is clear then arm_excp_target_el()
will have returned either 1 or 2, and so we'll have returned false
due to the check on cur_el earlier.

> +            }
> +        }
> +        if (!secure) {
> +            if (arm_feature(env, ARM_FEATURE_EL2)) {
> +                if (env->cp15.hcr_el2 & HCR_FMO) {
> +                    /* CPSR.F/PSTATE.F ignored if
> +                     *  - exception is taken from Non-secure state
> +                     *  - HCR.FMO == 1
> +                     *  - either:  - not in Hyp mode
> +                     *             - SCR.FIQ routes exception to monitor mode
> +                     *               (EL3 in AArch32)
> +                     */
> +                    if (cur_el < 2) {
> +                        return true;
> +                    } else if (arm_feature(env, ARM_FEATURE_EL3) &&
> +                            (env->cp15.scr_el3 & SCR_FIQ) &&
> +                            !arm_el_is_aa64(env, 3)) {
> +                        return true;
> +                    }
> +                } else if (arm_el_is_aa64(env, 3) &&
> +                          (env->cp15.scr_el3 & SCR_RW) &&
> +                          cur_el == 2) {
> +                    /* FIQs not routed to EL2 but currently in EL2 (A64).
> +                     * Exception is not taken but remains pending. */
> +                    return false;
> +                }
> +            }
> +            /* In ARMv7 only applies if both Security Extensions (EL3) and
> +             * Hypervirtualization Extensions (EL2) implemented, while
> +             * for ARMv8 it applies also if only EL3 implemented.
> +             */
> +            if (arm_feature(env, ARM_FEATURE_EL3) &&
> +                    (arm_feature(env, ARM_FEATURE_EL2) ||
> +                            arm_feature(env, ARM_FEATURE_V8))) {
> +                /* CPSR.F/PSTATE.F ignored if
> +                 * - exception is taken from Non-secure state
> +                 * - SCR.FIQ routes exception to monitor mode
> +                 * - SCR.FW bit is set to 0
> +                 * - HCR.FMO == 0 (if EL2 implemented)
> +                 */
> +                if ((env->cp15.scr_el3 & SCR_FIQ) &&
> +                        !(env->cp15.scr_el3 & SCR_FW)) {

Something odd here -- in ARMv8 SCR_EL3 bit 4 is RES1, so this test
should never pass -- either this check is wrong or the check on
ARM_FEATURE_V8 in the outer if() is wrong, presumably.

> +                    if (!arm_feature(env, ARM_FEATURE_EL2)) {
> +                        return true;
> +                    } else if (!(env->cp15.hcr_el2 & HCR_FMO)) {
> +                        return true;
> +                    }
> +                }
> +            }
>          }
>          return !(env->daif & PSTATE_F);
>      case EXCP_IRQ:
> -        if (irq_can_hyp && (env->cp15.hcr_el2 & HCR_IMO)) {
> -            return true;
> +        if (arm_feature(env, ARM_FEATURE_EL3) && arm_el_is_aa64(env, 3)) {
> +            /* If EL3 is using AArch64 and IRQs are routed to EL3 masking is
> +             * ignored in all exception levels except EL3.
> +             */
> +            if ((env->cp15.scr_el3 & SCR_IRQ) && cur_el < 3) {
> +                return true;
> +            }
> +            /* If we are in EL3 but IRQ s are not routed to EL3 the exception
> +             * is not taken but remains pending.
> +             */
> +            if (!(env->cp15.scr_el3 & SCR_IRQ) && cur_el == 3) {
> +                return false;
> +            }
> +        }
> +        if (!secure) {
> +            if (arm_feature(env, ARM_FEATURE_EL2)) {
> +                if (env->cp15.hcr_el2 & HCR_IMO) {
> +                    /* CPSR.I/PSTATE.I ignored if
> +                     *  - exception is taken from Non-secure state
> +                     *  - HCR.IMO == 1
> +                     *  - either:  - not in Hyp mode
> +                     *             - SCR.IRQ routes exception to monitor mode
> +                     *                (EL3 in AArch32)
> +                     */
> +                    if (cur_el < 2) {
> +                        return true;
> +                    } else if (arm_feature(env, ARM_FEATURE_EL3) &&
> +                            (env->cp15.scr_el3 & SCR_IRQ) &&
> +                            !arm_el_is_aa64(env, 3)) {
> +                        return true;
> +                    }
> +                } else if (arm_el_is_aa64(env, 3) &&
> +                          (env->cp15.scr_el3 & SCR_RW) &&
> +                          cur_el == 2) {
> +                    /* IRQs not routed to EL2 but currently in EL2 (A64).
> +                     * Exception is not taken but remains pending. */
> +                    return false;
> +                }
> +            }
>          }
>          return irq_unmasked;
>      case EXCP_VFIQ:
> --
> 1.8.3.2

I have to say I find this set of nested conditionals pretty confusing,
and hard to relate to the tables in the ARM ARM. Maybe it would be better
if we actually had a set of data tables in our implementation which
we used to look up whether the exception should be always taken,
remain pending, or honour the PSTATE mask flag ?

I think it would also be good if our implementation tried to keep
the same separation of routing [ie "which exception level is this
exception going to go to?"] and masking [ie "do we take this exception
at this time?"] which the ARM ARM has. At the moment we sort of
have that in the arm_excp_target_el() and this function, but a lot
of the code here seems to be repeating bits of the routing calculation.

thanks
-- PMM


reply via email to

[Prev in Thread] Current Thread [Next in Thread]