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

[Greg Bellows]

On 6 October 2014 10:53, Peter Maydell <address@hidden> wrote:

On 30 September 2014 22:49, 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.

"AA" (just do a search and replace through the whole patchset,
please.)

>
> 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>
>
> ----------
> v4 -> v5
> - Merge with v4 patch 10
> ---
>  target-arm/cpu.h | 116 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 106 insertions(+), 10 deletions(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index c000716..30f57fd 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -1226,11 +1226,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
> @@ -1245,19 +1242,118 @@ 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) */

"*/" goes on its own line.

 

Fixed in v6

 

>      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) {
> +                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;
> +            }
> +        }

This is all kind of confusing. What is the division of work
between this function and arm_phys_excp_target_el(),
conceptually? Why isn't "we're in EL3 but FIQs aren't going
to EL3" handled by the "cur_el > arm_excp_target_el()" check
above, for instance?

I inherited the code, so I can't speak entirely to the motivation.  I'm guessing that the code closely follows the ARM spec for masking.  Certainly the code could be divided differently, but functionally it works so I saw no need to change it.  I can certainly adjust the two functions if you would prefer to isolate the routing/target function.

thanks
-- PMM