Re: [Qemu-devel] [PATCH v8 02/27] target-arm: add async excp target_el function

[Peter Maydell]

On 30 October 2014 21:28, Greg Bellows <address@hidden> wrote:
> Adds a dedicated function and a lookup table for determining the target
> exception level of IRQ and FIQ exceptions.  The lookup table is taken from the
> ARMv7 and ARMv8 specification exception routing tables.
>
> Signed-off-by: Greg Bellows <address@hidden>
>
> ---
>
> v7 -> v8
> - Added target EL lookup table
> - Rework arm_phys_excp_target_el to use an EL lookup table rather than
>   conditionals.
>
> v5 -> v6
> - Removed unneeded arm_phys_excp_target_el() function prototype.
> - Removed unneeded arm_phys_excp_target_el() USER_ONLY function.
> - Fixed up arm_phys_excp_target_el() function definition to be static.
> - Globally replace Aarch# with AArch#
>
> v4 -> v5
> - Simplify target EL function including removal of mode which was unused
> - Merged with patch that plugs in the use of the function
>
> v3 -> v4
> - Fixed arm_phys_excp_target_el() 0/0/0 case to return excp_mode when EL<2
>   rather than ABORT.
> ---
>  target-arm/helper.c | 111 
> ++++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 91 insertions(+), 20 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index c47487a..e610466 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -3761,6 +3761,94 @@ void switch_mode(CPUARMState *env, int mode)
>      env->spsr = env->banked_spsr[i];
>  }
>
> +/* Physical Interrupt Target EL Lookup Table
> + *
> + * [ From ARM ARM section G1.13.4 (Table G1-15) ]
> + *
> + * 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.
> + *
> + *    Dimensions:
> + *    target_el_table[2][2][2][2][2][4]
> + *                    |  |  |  |  |  +--- Current EL
> + *                    |  |  |  |  +------ Non-secure(0)/Secure(1)
> + *                    |  |  |  +--------- HCR mask override
> + *                    |  |  +------------ SCR exec state control
> + *                    |  +--------------- SCR mask override
> + *                    +------------------ 32-bit(0)/64-bit(1) EL3
> + *
> + *    The table values are as such:
> + *    0-3 = EL0-EL3
> + *     -1 = Cannot occur
> + *
> + * In the case of exceptions not being taken, EL1 is returned.  These cases
> + * will be caught by the checks for target being >= current.

This could use rephrasing to make it clearer why returning 1 is ok:

"The ARM ARM tables include some entries indicating 'exception not taken'.
These are for the cases where we are currently at EL3 and the
exception is not routed to EL3 by the SCR, or where we are currently
at EL2 and the exception is not routed to EL3 or EL2. We can therefore
put '1' in those entries in our array, and rely on the check for
"target EL >= current EL" in the caller to ensure that the exception
is not taken."

> + *
> + *            SCR     HCR
> + *         64  EA     AMO                 From
> + *        BIT IRQ     IMO      Non-secure         Secure
> + *        EL3 FIQ  RW FMO   EL0 EL1 EL2 EL3   EL0 EL1 EL2 EL3
> + */
> +const int8_t target_el_table[2][2][2][2][2][4] = {
> +    {{{{/* 0   0   0   0 */{ 1,  1,  2, -1 },{ 3,  3, -1,  3 },},
> +       {/* 0   0   0   1 */{ 2,  2,  2, -1 },{ 3,  3, -1,  3 },},},
> +      {{/* 0   0   1   0 */{ 1,  1,  2, -1 },{ 3,  3, -1,  3 },},
> +       {/* 0   0   1   1 */{ 2,  2,  2, -1 },{ 3,  3, -1,  3 },},},},
> +     {{{/* 0   1   0   0 */{ 3,  3,  3, -1 },{ 3,  3, -1,  3 },},
> +       {/* 0   1   0   1 */{ 3,  3,  3, -1 },{ 3,  3, -1,  3 },},},
> +      {{/* 0   1   1   0 */{ 3,  3,  3, -1 },{ 3,  3, -1,  3 },},
> +       {/* 0   1   1   1 */{ 3,  3,  3, -1 },{ 3,  3, -1,  3 },},},},},

Your entries for 32-bit EL3, exception from Secure EL1 should all
read "-1", not "3" -- it's not possible to have current_el be 1
in this case (this is why table G1-15 has a "-" in those entries).

> +    {{{{/* 1   0   0   0 */{ 1,  1,  2, -1 },{ 1,  1, -1,  1 },},
> +       {/* 1   0   0   1 */{ 2,  2,  2, -1 },{ 1,  1, -1,  1 },},},
> +      {{/* 1   0   1   0 */{ 1,  1,  1, -1 },{ 1,  1, -1,  1 },},
> +       {/* 1   0   1   1 */{ 2,  2,  2, -1 },{ 1,  1, -1,  1 },},},},
> +     {{{/* 1   1   0   0 */{ 3,  3,  3, -1 },{ 3,  3, -1,  3 },},
> +       {/* 1   1   0   1 */{ 3,  3,  3, -1 },{ 3,  3, -1,  3 },},},
> +      {{/* 1   1   1   0 */{ 3,  3,  3, -1 },{ 3,  3, -1,  3 },},
> +       {/* 1   1   1   1 */{ 3,  3,  3, -1 },{ 3,  3, -1,  3 },},},},},
> +};
> +
> +/*
> + * Determine the target EL for physical exceptions
> + */
> +static inline uint32_t arm_phys_excp_target_el(CPUState *cs, uint32_t 
> excp_idx,
> +                                        uint32_t cur_el, bool secure)
> +{
> +    CPUARMState *env = cs->env_ptr;
> +    uint32_t rw = ((env->cp15.scr_el3 & SCR_RW) == SCR_RW);
> +    uint32_t scr;
> +    uint32_t hcr;
> +    uint32_t target_el;
> +    uint32_t is64 = arm_el_is_aa64(env, 3);

You can just make these all "int".

> +
> +    switch (excp_idx) {
> +    case EXCP_IRQ:
> +        scr = ((env->cp15.scr_el3 & SCR_IRQ) == SCR_IRQ);
> +        hcr = ((env->cp15.hcr_el2 & HCR_IMO) == HCR_IMO);
> +        break;
> +    case EXCP_FIQ:
> +        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;
> +    };
> +
> +    /* If HCR.TGE is set then HCR is treated as being 1 */
> +    hcr |= ((env->cp15.hcr_el2 & HCR_TGE) == HCR_TGE);
> +
> +    /* Perform a table-lookup for the target EL given the current state */
> +    target_el = target_el_table[is64][scr][rw][hcr][secure][cur_el];
> +
> +    assert(target_el > 0);
> +
> +    return target_el;
> +}
> +
>  /*
>   * Determine the target EL for a given exception type.
>   */
> @@ -3769,14 +3857,8 @@ unsigned int arm_excp_target_el(CPUState *cs, unsigned 
> int excp_idx)
>      ARMCPU *cpu = ARM_CPU(cs);
>      CPUARMState *env = &cpu->env;
>      unsigned int cur_el = arm_current_el(env);
> -    unsigned int target_el;
> -    /* FIXME: Use actual secure state.  */
> -    bool secure = false;
> -
> -    if (!env->aarch64) {
> -        /* TODO: Add EL2 and 3 exception handling for AArch32.  */
> -        return 1;
> -    }
> +    unsigned int target_el = 1;

Do you actually need to initialize this?

> +    bool secure = arm_is_secure(env);
>
>      switch (excp_idx) {
>      case EXCP_HVC:
> @@ -3788,19 +3870,8 @@ unsigned int arm_excp_target_el(CPUState *cs, unsigned 
> int excp_idx)
>          break;
>      case EXCP_FIQ:
>      case EXCP_IRQ:
> -    {
> -        const uint64_t hcr_mask = excp_idx == EXCP_FIQ ? HCR_FMO : HCR_IMO;
> -        const uint32_t scr_mask = excp_idx == EXCP_FIQ ? SCR_FIQ : SCR_IRQ;
> -
> -        target_el = 1;
> -        if (!secure && (env->cp15.hcr_el2 & hcr_mask)) {
> -            target_el = 2;
> -        }
> -        if (env->cp15.scr_el3 & scr_mask) {
> -            target_el = 3;
> -        }
> +        target_el = arm_phys_excp_target_el(cs, excp_idx, cur_el, secure);
>          break;
> -    }
>      case EXCP_VIRQ:
>      case EXCP_VFIQ:
>          target_el = 1;
> --
> 1.8.3.2
>

Thanks for adding the table-driven code: I found this much easier
to review against the ARM ARM. If you make those minor changes I
mention above then you can add my reviewed-by tag.

-- PMM