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

[Edgar E. Iglesias]

On Wed, Jun 11, 2014 at 01:54:51AM +0200, Fabian Aggeler wrote:
> This patch extends arm_excp_unmasked() according to ARM ARMv7 and
> ARM ARMv8 (all EL running in Aarch32) and adds comments.

Hi Fabian,

I think this and the following patch generally look good. I haven't
checked all the details yet though. A few minor comments here.



> 
> Signed-off-by: Fabian Aggeler <address@hidden>
> ---
>  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 661bfbe..f8531aa 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -581,6 +581,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)
> @@ -1183,30 +1185,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;
> -    }

If you remove this, it needs to be replaced with something equivalent
for VFIQ/VIRQ.


> -
> +    /* 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)) {

I think some of this logic will be simpler to read if you can for positive
flag testing assume that hcr_el2 is zero when el2 is unavailable. Same
for scr_el3 positive flag testing.

> +                /* 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
>