| Message ID | CAOgzsHU3PMYLqrivRYtwkv4p-=cfx8ysEiaHv6359Hpj=syUqg@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On 24 October 2014 23:43, Greg Bellows <greg.bellows@linaro.org> 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
On 26 October 2014 22:30, Peter Maydell <peter.maydell@linaro.org> wrote: > 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. Thinking further about this I actually prefer it -- it completely separates routing from masking. So you should make those entries read '1' and then just use -1 for "not possible" (and assert that the table lookup never gives you -1). > I looked through the tables for the AArch32 routing, and I can't > see anywhere where they're different from the AArch64 routing > handling. [...] Did I miss something? I did! If EL3 is AArch32 and the SCR.FIQ etc bits are clear then the FIQ/IRQ in the secure world target EL3, not EL1 (since the latter doesn't exist). We can handle that by using the AArch64 table anyway and just having a bit at the end that says "if we're secure and target_el is 1 then set it to 3", or if you prefer with a second table. -- PMM
On 27 October 2014 06:57, Peter Maydell <peter.maydell@linaro.org> wrote: > On 26 October 2014 22:30, Peter Maydell <peter.maydell@linaro.org> wrote: > > 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. > > Thinking further about this I actually prefer it -- it completely > separates routing from masking. So you should make those entries > read '1' and then just use -1 for "not possible" (and assert > that the table lookup never gives you -1). > > > I looked through the tables for the AArch32 routing, and I can't > > see anywhere where they're different from the AArch64 routing > > handling. [...] Did I miss something? > > I did! If EL3 is AArch32 and the SCR.FIQ etc bits are clear then > the FIQ/IRQ in the secure world target EL3, not EL1 (since the > latter doesn't exist). We can handle that by using the AArch64 table > anyway and just having a bit at the end that says "if we're secure > and target_el is 1 then set it to 3", or if you prefer with a second > table. > > Right, this is what I was saying about either needing another column or or table. If we are going with the table approach I think we should go all the way and not add a "conditional". Besides, the tables are small anyway. I extended the existing able to take 32/64 bit identifier. The changes discussed above will be made in v8. > -- PMM >
============================= 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 },}, + {{ /* 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
Hi Peter, 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. 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. - 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. Below are the changes (convert to a fixed font for better table format): Greg */ @@ -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; } On 24 October 2014 11:25, Peter Maydell <peter.maydell@linaro.org> wrote: > On 21 October 2014 17:55, Greg Bellows <greg.bellows@linaro.org> wrote: > > From: Fabian Aggeler <aggelerf@ethz.ch> > > > > This patch extends arm_excp_unmasked() according to ARM ARMv7 and > > ARM ARMv8 (all EL running in AArch32) and adds comments. > > > > 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 <aggelerf@ethz.ch> > > Signed-off-by: Greg Bellows <greg.bellows@linaro.org> > > > > ========== > > > > v5 -> v6 > > - Globally change Aarch# to AArch# > > - Fixed comment termination > > > > v4 -> v5 > > - Merge with v4 patch 10 > > > > Signed-off-by: Greg Bellows <greg.bellows@linaro.org> > > --- > > target-arm/cpu.h | 117 > ++++++++++++++++++++++++++++++++++++++++++++++++++----- > > 1 file changed, 107 insertions(+), 10 deletions(-) > > > > diff --git a/target-arm/cpu.h b/target-arm/cpu.h > > index cb6ec5c..1a564b9 100644 > > --- a/target-arm/cpu.h > > +++ b/target-arm/cpu.h > > @@ -1246,11 +1246,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 > > @@ -1265,19 +1262,119 @@ 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) > > + */ > > 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) { > > Why are we recalculating whether the target level is EL3 by looking > at SCR_EL3.SCR_FIQ, rather than using the target_el which > arm_excp_target_el() returns? > > > + 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; > > Isn't this unreachable? If SCR_FIQ is clear then arm_excp_target_el() > will have returned either 1 or 2, and so we'll have returned false > due to the check on cur_el earlier. > > > + } > > + } > > + if (!secure) { > > + if (arm_feature(env, ARM_FEATURE_EL2)) { > > + if (env->cp15.hcr_el2 & HCR_FMO) { > > + /* 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 > > + * (EL3 in AArch32) > > + */ > > + if (cur_el < 2) { > > + return true; > > + } else if (arm_feature(env, ARM_FEATURE_EL3) && > > + (env->cp15.scr_el3 & SCR_FIQ) && > > + !arm_el_is_aa64(env, 3)) { > > + return true; > > + } > > + } else if (arm_el_is_aa64(env, 3) && > > + (env->cp15.scr_el3 & SCR_RW) && > > + cur_el == 2) { > > + /* FIQs not routed to EL2 but currently in EL2 > (A64). > > + * Exception is not taken but remains pending. */ > > + return false; > > + } > > + } > > + /* 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)) { > > Something odd here -- in ARMv8 SCR_EL3 bit 4 is RES1, so this test > should never pass -- either this check is wrong or the check on > ARM_FEATURE_V8 in the outer if() is wrong, presumably. > > > + 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 (arm_feature(env, ARM_FEATURE_EL3) && arm_el_is_aa64(env, > 3)) { > > + /* If EL3 is using AArch64 and IRQs are routed to EL3 > masking is > > + * ignored in all exception levels except EL3. > > + */ > > + if ((env->cp15.scr_el3 & SCR_IRQ) && cur_el < 3) { > > + return true; > > + } > > + /* If we are in EL3 but IRQ s are not routed to EL3 the > exception > > + * is not taken but remains pending. > > + */ > > + if (!(env->cp15.scr_el3 & SCR_IRQ) && cur_el == 3) { > > + return false; > > + } > > + } > > + if (!secure) { > > + if (arm_feature(env, ARM_FEATURE_EL2)) { > > + if (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 > > + * (EL3 in AArch32) > > + */ > > + if (cur_el < 2) { > > + return true; > > + } else if (arm_feature(env, ARM_FEATURE_EL3) && > > + (env->cp15.scr_el3 & SCR_IRQ) && > > + !arm_el_is_aa64(env, 3)) { > > + return true; > > + } > > + } else if (arm_el_is_aa64(env, 3) && > > + (env->cp15.scr_el3 & SCR_RW) && > > + cur_el == 2) { > > + /* IRQs not routed to EL2 but currently in EL2 > (A64). > > + * Exception is not taken but remains pending. */ > > + return false; > > + } > > + } > > } > > return irq_unmasked; > > case EXCP_VFIQ: > > -- > > 1.8.3.2 > > I have to say I find this set of nested conditionals pretty confusing, > and hard to relate to the tables in the ARM ARM. Maybe it would be better > if we actually had a set of data tables in our implementation which > we used to look up whether the exception should be always taken, > remain pending, or honour the PSTATE mask flag ? > > I think it would also be good if our implementation tried to keep > the same separation of routing [ie "which exception level is this > exception going to go to?"] and masking [ie "do we take this exception > at this time?"] which the ARM ARM has. At the moment we sort of > have that in the arm_excp_target_el() and this function, but a lot > of the code here seems to be repeating bits of the routing calculation. > > thanks > -- PMM >