To make sure I understand, you are proposing that we simply use the return from arm_current_pl of 3 to indicate isSecure?
Fabian's code closely follows the v8 spec pseudo code. I believe the case that would be omitted if we let a return of 3 mean "secure" is the case where we are in EL0/1 with SCR.NS=0. So, the to functions are not quite identical. In the case of arm_current_pl, it returns the PL regardless of the SCR.NS bit. In the case of arm_is_secure, the secure state takes into consideration both the PL and the SCR.NS setting. On 12 June 2014 12:26, Sergey Fedorov <serge.f...@gmail.com> wrote: > Hi Greg, > > I'm sorry, I wasn't thoughtful enough and missed that. > I would just suggest to combine that functions since they have a common > part, i.e.: > > if (arm_feature(env, ARM_FEATURE_EL3)) { > ... > > What do you think? > > // Sergey > > 12.06.2014 20:26, Greg Bellows пишет: > > Hi Sergey, > > I think I am missing your point. In patch 6 arm_current_pl calls > arm_is_secure. Can you elaborate? > > Greg > > > On 11 June 2014 07:17, Sergey Fedorov <serge.f...@gmail.com> wrote: > >> On 11.06.2014 03:54, Fabian Aggeler wrote: >> > arm_is_secure() function allows to determine CPU security state >> > if the CPU implements Security Extensions/EL3. >> > arm_is_secure_below_el3() returns true if CPU is in secure state >> > below EL3. >> > >> > Signed-off-by: Sergey Fedorov <s.fedo...@samsung.com> >> > Signed-off-by: Fabian Aggeler <aggel...@ethz.ch> >> > --- >> > target-arm/cpu.h | 38 ++++++++++++++++++++++++++++++++++++++ >> > 1 file changed, 38 insertions(+) >> > >> > diff --git a/target-arm/cpu.h b/target-arm/cpu.h >> > index 903aa01..cb0da6b 100644 >> > --- a/target-arm/cpu.h >> > +++ b/target-arm/cpu.h >> > @@ -710,6 +710,44 @@ static inline int arm_feature(CPUARMState *env, >> int feature) >> > return (env->features & (1ULL << feature)) != 0; >> > } >> > >> > + >> > +/* Return true if exception level below EL3 is in secure state */ >> > +static inline bool arm_is_secure_below_el3(CPUARMState *env) >> > +{ >> > +#if !defined(CONFIG_USER_ONLY) >> > + if (arm_feature(env, ARM_FEATURE_EL3)) { >> > + return !(env->cp15.scr_el3 & SCR_NS); >> > + } else if (arm_feature(env, ARM_FEATURE_EL2)) { >> > + return false; >> > + } else { >> > + /* IMPDEF: QEMU defaults to non-secure */ >> > + return false; >> > + } >> > +#else >> > + return false; >> > +#endif >> > +} >> > + >> > +/* Return true if the processor is in secure state */ >> > +static inline bool arm_is_secure(CPUARMState *env) >> > +{ >> > +#if !defined(CONFIG_USER_ONLY) >> > + if (arm_feature(env, ARM_FEATURE_EL3)) { >> > + if (env->aarch64 && extract32(env->pstate, 2, 2) == 3) { >> > + /* CPU currently in Aarch64 state and EL3 */ >> > + return true; >> > + } else if (!env->aarch64 && >> > + (env->uncached_cpsr & CPSR_M) == ARM_CPU_MODE_MON) { >> > + /* CPU currently in Aarch32 state and monitor mode */ >> > + return true; >> > + } >> >> Hi Fabian, >> >> Why don't use arm_current_pl() from patch 6 to determine EL here? >> >> Best regards, >> Sergey >> >> > + } >> > + return arm_is_secure_below_el3(env); >> > +#else >> > + return false; >> > +#endif >> > +} >> > + >> > /* Return true if the specified exception level is running in AArch64 >> state. */ >> > static inline bool arm_el_is_aa64(CPUARMState *env, int el) >> > { >> >> > >