On 6 November 2014 15:50, Greg Bellows <greg.bell...@linaro.org> 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 <greg.bell...@linaro.org>
>
> ---
>
> 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
