On 8 February 2016 at 15:25, Sergey Fedorov <serge.f...@gmail.com> wrote: > On 05.02.2016 19:44, Peter Maydell wrote: >> Correct some corner cases we were getting wrong for >> CNTFRQ access rights: >> * should UNDEF from 32-bit Secure EL1 >> * only writable from the highest implemented exception level, >> which might not be EL1 now
>> + switch (arm_current_el(env)) { >> + case 0: >> + if (!extract32(env->cp15.c14_cntkctl, 0, 2)) { >> + return CP_ACCESS_TRAP; >> + } >> + /* EL0 reads are forbidden by the .access fields */ > > s/reads/writes/ ? Yes. >> + break; >> + case 1: >> + if (!isread && (arm_feature(env, ARM_FEATURE_EL2) >> + || arm_feature(env, ARM_FEATURE_EL3))) { >> + return CP_ACCESS_TRAP_UNCATEGORIZED; >> + } >> + if (!isread && ri->state == ARM_CP_STATE_AA32 && >> + arm_is_secure_below_el3(env)) { >> + /* Accesses from 32-bit Secure EL1 UNDEF (*not* trap to EL3!) */ >> + return CP_ACCESS_TRAP_UNCATEGORIZED; >> + } >> + break; >> + case 2: >> + if (!isread && arm_feature(env, ARM_FEATURE_EL3)) { >> + return CP_ACCESS_TRAP_UNCATEGORIZED; >> + } >> + break; >> + case 3: >> + break; >> } >> return CP_ACCESS_OK; >> } > > Maybe calculating "the highest implemented exception level" could > simplify reading of the code a bit? E.g.: > > int highest_el = arm_feature(env, ARM_FEATURE_EL3) ? 3 : > arm_feature(env, ARM_FEATURE_EL2) ? 2 : 1; > > We would probably want to have a dedicated static inline function for > this similar to HighestEL() from ARMv8 ARM pseudocode. Mmm, that might look neater. I'll have a play with the code. thanks -- PMM