On 7 October 2014 04:16, Greg Bellows <greg.bell...@linaro.org> wrote:
>
>
> On 6 October 2014 10:53, Peter Maydell <peter.mayd...@linaro.org> wrote:
>> > 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.
My point is really that it's hard to review if it's not
clear how the functions fit together and why code is in
one place and not another. I'm not asking for changes to
the code so much as better explication of why it is the way
it is. I guess I need to sit down with the ARM ARM and work
through whether it's putting things in the right place, then.
-- PMM
