Re: [Qemu-devel] [PATCH v10 01/26] target-arm: extend async excp masking

[Peter Maydell]

On 6 November 2014 15:50, Greg Bellows <address@hidden> wrote:
> This patch extends arm_excp_unmasked() to use lookup tables for determining
> whether IRQ and FIQ exceptions are masked.  The lookup tables are based on the
> ARMv8 and ARMv7 specification physical interrupt masking tables.
>
> 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: Greg Bellows <address@hidden>
>
> ---
>
> v8 -> v9
> - Undo the use of tables for exception masking and instead go with simplified
>   logic based on the target EL lookup.
> - Remove the masking tables
>
> v7 -> v8
> - Add IRQ and FIQ exeception masking lookup tables.
> - Rewrite patch to use lookup tables for determining whether an excpetion is
>   masked or not.
>
> v5 -> v6
> - Globally change Aarch# to AArch#
> - Fixed comment termination
>
> v4 -> v5
> - Merge with v4 patch 10
> ---
>  target-arm/cpu.h | 66 
> ++++++++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 52 insertions(+), 14 deletions(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 7f80090..cf30b2a 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -1247,27 +1247,51 @@ 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;
> -
> -    /* Don't take exceptions if they target a lower EL.  */
> +    bool secure = arm_is_secure(env);
> +    uint32_t scr;
> +    uint32_t hcr;
> +    bool pstate_unmasked;
> +    int8_t unmasked = 0;
> +    bool is_aa64 = arm_el_is_aa64(env, 3);

If you keep this variable, call it el3_is_aa64 (but see comments below).

> +
> +    /* Don't take exceptions if they target a lower EL.
> +     * This check should catch any exceptions that would not be taken but 
> left
> +     * pending.
> +     */
>      if (cur_el > target_el) {
>          return false;
>      }
>
>      switch (excp_idx) {
>      case EXCP_FIQ:
> -        if (irq_can_hyp && (env->cp15.hcr_el2 & HCR_FMO)) {
> -            return true;
> -        }
> -        return !(env->daif & PSTATE_F);
> +        /* 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 &= is_aa64 || !((env->cp15.scr_el3 & SCR_FW) && !hcr);
> +        pstate_unmasked = !(env->daif & PSTATE_F);
> +        break;
> +
>      case EXCP_IRQ:
> -        if (irq_can_hyp && (env->cp15.hcr_el2 & HCR_IMO)) {
> -            return true;
> -        }
> -        return !(env->daif & PSTATE_I);
> +        /* 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 = is_aa64 || (env->cp15.hcr_el2 & HCR_IMO);
> +        scr = false;
> +        pstate_unmasked = !(env->daif & PSTATE_I);
> +        break;
> +
>      case EXCP_VFIQ:
>          if (secure || !(env->cp15.hcr_el2 & HCR_FMO)) {
>              /* VFIQs are only taken when hypervized and non-secure.  */
> @@ -1283,6 +1307,20 @@ static inline bool arm_excp_unmasked(CPUState *cs, 
> unsigned int excp_idx)
>      default:
>          g_assert_not_reached();
>      }
> +
> +    /* Use the target EL, current execution state and SCR/HCR settings to
> +     * determine whether the corresponding CPSR bit is used to mask the
> +     * interrupt.
> +     */
> +    if ((target_el > cur_el) && (target_el != 1) && (scr || hcr) &&
> +        (is_aa64 || !secure)) {
> +        unmasked = 1;
> +    }

I think this logic is correct but I find it a little confusing.
In particular it is not obvious that most of this doesn't apply
for 64-bit EL3, because there is an "is_aa64 || " hidden in the
settings of scr and hcr, as well as in the check on !secure.

I would suggest pulling that out so:

    if ((target_el > cur_el) && (target_el != 1) {
        /* With 64-bit EL3 the logic is simple: we always ignore PSTATE masking
         * when taking an exception to a higher-and-not-1 exception level.
         * In a 32 bit EL3 there are some awkward special cases where the
         * SCR/HCR mask bits determine whether or not we should honour
         * PSTATE masking.
         */
        if ((arm_el_is_aa64(env, 3) || ((scr || hcr) && !secure)) {
            unmasked = 1;
        }
    }

(and dropping the "is_aa64 || " in the setting of scr/hcr.)

If you make that tweak you can add my reviewed-by tag.

thanks
-- PMM