Re: [Qemu-devel] [PATCH v4 09/33] target-arm: extend Aarch32 async excp masking

["Edgar E. Iglesias" <edgar.iglesias@gmail.com>]

On Mon, Jun 30, 2014 at 06:09:09PM -0500, greg.bellows@linaro.org wrote:
> From: Fabian Aggeler <aggelerf@ethz.ch>
> 
> This patch extends arm_excp_unmasked() according to ARM ARMv7 and
> ARM ARMv8 (all EL running in Aarch32) and adds comments.
> 
> Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
> Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
> ---
>  target-arm/cpu.h | 77 ++++++++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 61 insertions(+), 16 deletions(-)
> 
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 44e0943..fbd7cad 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -597,6 +597,8 @@ static inline void xpsr_write(CPUARMState *env, uint32_t val, uint32_t mask)
>  #define SCR_IRQ       (1U << 1)
>  #define SCR_FIQ       (1U << 2)
>  #define SCR_EA        (1U << 3)
> +#define SCR_FW        (1U << 4)
> +#define SCR_AW        (1U << 5)
>  #define SCR_SMD       (1U << 7)
>  #define SCR_HCE       (1U << 8)
>  #define SCR_SIF       (1U << 9)
> @@ -1199,30 +1201,73 @@ static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx)
>  {
>      CPUARMState *env = cs->env_ptr;
>      unsigned int cur_el = arm_current_pl(env);
> -    unsigned int target_el = arm_excp_target_el(cs, excp_idx);
> -    /* FIXME: Use actual secure state.  */
> -    bool secure = false;
> -    /* Interrupts can only be hypervised and routed to
> -     * EL2 if we are in NS EL0/1.
> -     */
> -    bool irq_can_hyp = !secure && cur_el < 2 && target_el == 2;
> +    bool secure = arm_is_secure(env);
> +
>      bool irq_unmasked = ((IS_M(env) && env->regs[15] < 0xfffffff0)
>                            || !(env->daif & PSTATE_I));
>  
> -    /* Don't take exceptions if they target a lower EL.  */
> -    if (cur_el > target_el) {
> -        return false;
> -    }
> -

Hi,

I think you might have missed a few of comments on the previous review.
If we remove the check for cur_el > target_el, we need to replace it
with something similar for VIRQ/VFIQ. VIRQ and VFIQ target EL1 and
cannot be taken while in EL2 or 3.

Cheers,
Edgar


> +    /* 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 (!secure) {
> +            if (arm_feature(env, ARM_FEATURE_EL2) &&
> +                    (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
> +                 */
> +                if (cur_el < 2) {
> +                    return true;
> +                } else if (arm_feature(env, ARM_FEATURE_EL3) &&
> +                        (env->cp15.scr_el3 & SCR_FIQ)) {
> +                    return true;
> +                }
> +            }
> +            /* 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)) {
> +                    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 (!secure) {
> +            if (arm_feature(env, ARM_FEATURE_EL2) &&
> +                    (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
> +                 */
> +                if (cur_el < 2) {
> +                    return true;
> +                } else if (arm_feature(env, ARM_FEATURE_EL3) &&
> +                        (env->cp15.scr_el3 & SCR_IRQ)) {
> +                    return true;
> +                }
> +            }
>          }
>          return irq_unmasked;
>      case EXCP_VFIQ:
> -- 
> 1.8.3.2
>