On 6 October 2014 10:53, Peter Maydell <peter.mayd...@linaro.org> wrote:
> On 30 September 2014 22:49, Greg Bellows <greg.bell...@linaro.org> wrote:
> > From: Fabian Aggeler <aggel...@ethz.ch>
> >
> > This patch extends arm_excp_unmasked() according to ARM ARMv7 and
> > ARM ARMv8 (all EL running in Aarch32) and adds comments.
>
> "AA" (just do a search and replace through the whole patchset,
> please.)
>
> >
> > 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: Fabian Aggeler <aggel...@ethz.ch>
> > Signed-off-by: Greg Bellows <greg.bell...@linaro.org>
> >
> > ----------
> > v4 -> v5
> > - Merge with v4 patch 10
> > ---
> > target-arm/cpu.h | 116
> ++++++++++++++++++++++++++++++++++++++++++++++++++-----
> > 1 file changed, 106 insertions(+), 10 deletions(-)
> >
> > diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> > index c000716..30f57fd 100644
> > --- a/target-arm/cpu.h
> > +++ b/target-arm/cpu.h
> > @@ -1226,11 +1226,8 @@ 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;
> > + bool secure = arm_is_secure(env);
> > +
> > /* ARMv7-M interrupt return works by loading a magic value
> > * into the PC. On real hardware the load causes the
> > * return to occur. The qemu implementation performs the
> > @@ -1245,19 +1242,118 @@ static inline bool arm_excp_unmasked(CPUState
> *cs, unsigned int excp_idx)
> > && (!IS_M(env) || env->regs[15] < 0xfffffff0);
> >
> > /* Don't take exceptions if they target a lower EL. */
> > - if (cur_el > target_el) {
> > + if (cur_el > arm_excp_target_el(cs, excp_idx)) {
> > return false;
> > }
> >
> > + /* 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) */
>
> "*/" goes on its own line.
>
>
Fixed in v6
> > switch (excp_idx) {
> > case EXCP_FIQ:
> > - if (irq_can_hyp && (env->cp15.hcr_el2 & HCR_FMO)) {
> > - return true;
> > + if (arm_feature(env, ARM_FEATURE_EL3) && arm_el_is_aa64(env,
> 3)) {
> > + /* If EL3 is using Aarch64 and FIQs are routed to EL3
> masking is
> > + * ignored in all exception levels except EL3.
> > + */
> > + if ((env->cp15.scr_el3 & SCR_FIQ) && cur_el < 3) {
> > + return true;
> > + }
> > + /* If we are in EL3 but FIQs are not routed to EL3 the
> exception
> > + * is not taken but remains pending.
> > + */
> > + if (!(env->cp15.scr_el3 & SCR_FIQ) && cur_el == 3) {
> > + return false;
> > + }
> > + }
>
> This is all kind of confusing. What is the division of work
> between this function and arm_phys_excp_target_el(),
> conceptually? Why isn't "we're in EL3 but FIQs aren't going
> to EL3" handled by the "cur_el > arm_excp_target_el()" check
> above, for instance?
>
I inherited the code, so I can't speak entirely to the motivation. I'm
guessing that the code closely follows the ARM spec for masking. Certainly
the code could be divided differently, but functionally it works so I saw
no need to change it. I can certainly adjust the two functions if you
would prefer to isolate the routing/target function.
> thanks
> -- PMM
>
