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

[Peter Maydell]

On 24 October 2014 23:43, Greg Bellows <address@hidden> wrote:
> Based on our discussion, I looked into a table lookup approach to replace
> the arm_phys_excp_target_el() as we discussed.  I have something working but
> still need to verify it is 100% correct.  Before I went much further, I
> thought I'd share my findings.

Thanks for writing this up. Personally I like the table lookup,
because I find it much easier to review and confirm that it matches
the ARM ARM (which also uses tables to describe asynchronous interrupt
routing). The table's only 64 bytes if you make it an int8_t array.

> In order to do the table in a way that it does not blow-up with a bunch of
> duplicate data, we still need a function for accessing the table.  The
> function will still have a good part of what the existing
> arm_phys_excp_target_el() has at the beginning.  This is necessary as we
> need to decide which of the SCR and HCR bits need to be plugged into the
> table.
>
> In addition, we need the following:
>
> The table has to include special values to account for the cases where an
> interrupt is not taken so we will need logic on the other end to catch the
> special table value and return cur_el.

I think you really want to return "exception not taken" to the
caller directly, because if you return the current exception level
we may go ahead and (wrongly) take the exception. This is actually
the thing that allows you to make the calling code in arm_excp_unmasked()
much simpler, because it then doesn't have to effectively repeat
the routing calculations to figure out whether it's one of the
"exception not taken" cases, and the "is this exception masked"
check then boils down to:
 if target_el == "masked" || target_el < current_el
     return false
 if target_el > current_el && target_el > 1
     /* PSTATE mask bits don't apply */
     return true
 return !(env->daif & PSTATE_whatever)

which is vastly simpler than the code I originally objected to
in the patchset...

(If you look at the masking tables in the ARM ARM you'll see they're
really just copies of the routing tables with this straightforward
logic applied to identify when the PSTATE mask bits should be checked.)

In fact, since all of the "exception is not taken" cases are for
"we are in secure EL3 and the exception is not being routed to
secure EL3" you could just make all those entries read "1" and
rely on the "target_el < current_el" check. That does slightly
harm readability though.

> Either another dimension needs to be added to the table or a second table to
> handle the differences between 32/64-bit EL3. (Still needed)
> Another dimension is needed to include HCR_TGE eventually.
>
> Basically, I'm not sure that it buys us much other than a static table.
> Otherwise, we are swapping one bunch of logic for a different set.

I would handle HCR.TGE by just making the AMO/IMO/FMO bit used in the
lookup 1, because that's how the ARM ARM describes its effect.
Similarly you can just squash the routing bits to 0 for the "EL2
not implemented and "EL3 not implemented" cases. Fabian's code
actually already does both of these things in calculating the
hcr_routing and scr_routing flags, so you can just reinstate
and use that code.

I looked through the tables for the AArch32 routing, and I can't
see anywhere where they're different from the AArch64 routing
handling. The SCR_EL3.RW bit needs to be squashed to 0, but that
seems to be it. (We don't need to encode the target AArch32 mode
in the tables, I think; at least in our current design the 32 bit
do_interrupt() function can figure it out based on the exception
number and what exception level it's in now. It's a bit ugly
that we do that by calling arm_excp_target_el() again but never
mind for now.) Did I miss something?

> Below are the changes (convert to a fixed font for better table format):
>
> Greg
>
> =============================
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index b4db1a5..fd3d637 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -4028,6 +4028,44 @@ void switch_mode(CPUARMState *env, int mode)
>      env->spsr = env->banked_spsr[i];
>  }
>
> +const unsigned int aarch64_target_el[2][2][2][2][4] = {
> +    /* Physical Interrupt Target EL Lookup Table
> +     *
> +     * [ From ARM ARM section D1.14.1 (Table D1-11) ]
> +     *
> +     * 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.
> +     *
> +     *   The table values are as such:
> +     *   0-3 = EL0-EL3
> +     *     8 = Cannot occur
> +     *     9 = Interrupt not taken
> +     *
> +     *      SCR     HCR
> +     *       EA     AMO
> +     *      IRQ     IMO             FROM
> +     *      FIQ RW  FMO  NS    EL0 EL1 EL2 EL3
> +     */
> +    {{{{  /* 0   0   0   0  */  1,  1,  8,  9  },
> +       {  /* 0   0   0   1  */  1,  1,  2,  8  },},

If you make the index for S/NS be the "secure" flag rather than
"!secure" then you can have each line here be
          { 1, 1, 2, 8 }, { 1, 1, 8, 9 }

which matches the order of the columns in the ARM ARM.
(Some shortish symbolic names for the "can't happen" and
"don't take" cases would help readability too. And/or use
negative numbers.)

> +      {{  /* 0   0   1   0  */  1,  1,  8,  9  },
> +       {  /* 0   0   1   1  */  2,  2,  2,  8  },},},
> +     {{{  /* 0   1   0   0  */  1,  1,  8,  9  },
> +       {  /* 0   1   0   1  */  1,  1,  8,  8  },},
> +      {{  /* 0   1   1   0  */  1,  1,  8,  9  },
> +       {  /* 0   1   1   1  */  2,  2,  2,  8  },},},},
> +    {{{{  /* 1   0   0   0  */  3,  3,  8,  3  },
> +       {  /* 1   0   0   1  */  3,  3,  3,  8  },},
> +      {{  /* 1   0   1   0  */  3,  3,  8,  3  },
> +       {  /* 1   0   1   1  */  3,  3,  3,  8  },},},
> +     {{{  /* 1   1   0   0  */  3,  3,  8,  3  },
> +       {  /* 1   1   0   1  */  3,  3,  3,  8  },},
> +      {{  /* 1   1   1   0  */  3,  3,  8,  3  },
> +       {  /* 1   1   1   1  */  3,  3,  3,  8  },},},},
> +};
> +
>  /*
>   * Determine the target EL for physical exceptions
>   */
> @@ -4035,69 +4073,28 @@ static inline uint32_t
> arm_phys_excp_target_el(CPUState *cs, ui
>                                          uint32_t cur_el, bool secure)
>  {
>      CPUARMState *env = cs->env_ptr;
> -    uint32_t target_el = 1;
> -
> -    /* There is no SCR or HCR routing unless the respective EL3 and EL2
> -     * extensions are supported.  This initial setting affects whether any
> -     * other conditions matter.
> -     */
> -    bool scr_routing = arm_feature(env, ARM_FEATURE_EL3); /* IRQ, FIQ, EA
> */
> -    bool hcr_routing = arm_feature(env, ARM_FEATURE_EL2); /* IMO, FMO, AMO
> */
> -
> -    /* Fast-path if EL2 and EL3 are not enabled */
> -    if (!scr_routing && !hcr_routing) {
> -        return target_el;
> -    }
> +    uint32_t rw = ((env->cp15.scr_el3 & SCR_RW) == SCR_RW);
> +    uint32_t scr;
> +    uint32_t hcr;
> +    uint32_t target_el;
>
>      switch (excp_idx) {
>      case EXCP_IRQ:
> -        scr_routing &= ((env->cp15.scr_el3 & SCR_IRQ) == SCR_IRQ);
> -        hcr_routing &= ((env->cp15.hcr_el2 & HCR_IMO) == HCR_IMO);
> +        scr = ((env->cp15.scr_el3 & SCR_IRQ) == SCR_IRQ);
> +        hcr = ((env->cp15.hcr_el2 & HCR_IMO) == HCR_IMO);
>          break;
>      case EXCP_FIQ:
> -        scr_routing &= ((env->cp15.scr_el3 & SCR_FIQ) == SCR_FIQ);
> -        hcr_routing &= ((env->cp15.hcr_el2 & HCR_FMO) == HCR_FMO);
> -    }
> -
> -    /* If SCR routing is enabled we always go to EL3 regardless of EL3
> -     * execution state
> -     */
> -    if (scr_routing) {
> -        /* IRQ|FIQ|EA == 1 */
> -        return 3;
> -    }
> -
> -    /* If HCR.TGE is set all exceptions that would be routed to EL1 are
> -     * routed to EL2 (in non-secure world).
> -     */
> -    hcr_routing &= (env->cp15.hcr_el2 & HCR_TGE) == HCR_TGE;
> +        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;
> +    };
>
> -    /* Determine target EL according to ARM ARMv8 tables G1-15 and G1-16 */
> -    if (arm_el_is_aa64(env, 3)) {
> -        /* EL3 in AArch64 */
> -        if (!secure) {
> -            /* If non-secure, we may route to EL2 depending on other state.
> -             * If we are coming from the secure world then we always route
> to
> -             * EL1.
> -             */
> -            if (hcr_routing ||
> -                (cur_el == 2 && !(env->cp15.scr_el3 & SCR_RW))) {
> -                /* If HCR.FMO/IMO is set or we already in EL2 and it is not
> -                 * configured to be AArch64 then route to EL2.
> -                 */
> -                target_el = 2;
> -            }
> -        }
> -    } else {
> -        /* EL3 in AArch32 */
> -        if (secure) {
> -            /* If coming from secure always route to EL3 */
> -            target_el = 3;
> -        } else if (hcr_routing || cur_el == 2) {
> -            /* If HCR.FMO/IMO is set or we are already EL2 then route to
> EL2 */
> -            target_el = 2;
> -        }
> -    }
> +    target_el = aarch64_target_el[scr][rw][hcr][!secure][cur_el];
> +    target_el = (target_el > 3) ? cur_el : target_el;
>
>      return target_el;
>  }
>

thanks
-- PMM