| Message ID | 1412113785-21525-3-git-send-email-greg.bellows@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Tue, Sep 30, 2014 at 04:49:14PM -0500, Greg Bellows wrote: > From: Fabian Aggeler <aggelerf@ethz.ch> > > 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. Hi Greg, > > Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com> > Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch> > Signed-off-by: Greg Bellows <greg.bellows@linaro.org> > --- > target-arm/cpu.h | 38 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 38 insertions(+) > > diff --git a/target-arm/cpu.h b/target-arm/cpu.h > index 81fffd2..10afef0 100644 > --- a/target-arm/cpu.h > +++ b/target-arm/cpu.h > @@ -753,6 +753,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 > +} arm_is_secure_below_el3() is never called from CONFIG_USER_ONLY code so maybe we could ifdef around the entire function for readability? Or maybe even around both functions and provide a separate static inline bool arm_is_secure(CPUARMState *env) { return false; } for the user_only case. Cheers, Edgar > + > +/* 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; > + } > + } > + 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) > { > -- > 1.8.3.2 >
Yeah, that would be cleaner, I'll fix it in the next version. On 30 September 2014 17:50, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote: > On Tue, Sep 30, 2014 at 04:49:14PM -0500, Greg Bellows wrote: > > From: Fabian Aggeler <aggelerf@ethz.ch> > > > > 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. > > Hi Greg, > > > > > Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com> > > Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch> > > Signed-off-by: Greg Bellows <greg.bellows@linaro.org> > > --- > > target-arm/cpu.h | 38 ++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 38 insertions(+) > > > > diff --git a/target-arm/cpu.h b/target-arm/cpu.h > > index 81fffd2..10afef0 100644 > > --- a/target-arm/cpu.h > > +++ b/target-arm/cpu.h > > @@ -753,6 +753,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 > > +} > > arm_is_secure_below_el3() is never called from CONFIG_USER_ONLY > code so maybe we could ifdef around the entire function > for readability? > > Or maybe even around both functions and provide a separate > static inline bool arm_is_secure(CPUARMState *env) > { > return false; > } > for the user_only case. > > Cheers, > Edgar > > > > + > > +/* 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; > > + } > > + } > > + 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) > > { > > -- > > 1.8.3.2 > > >
On 30 September 2014 22:49, Greg Bellows <greg.bellows@linaro.org> wrote: > From: Fabian Aggeler <aggelerf@ethz.ch> > > 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.fedorov@samsung.com> > Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch> > Signed-off-by: Greg Bellows <greg.bellows@linaro.org> > --- > target-arm/cpu.h | 38 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 38 insertions(+) > > diff --git a/target-arm/cpu.h b/target-arm/cpu.h > index 81fffd2..10afef0 100644 > --- a/target-arm/cpu.h > +++ b/target-arm/cpu.h > @@ -753,6 +753,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; I would be happy to fold both these identical 'return false' cases together and have a comment that it's only IMPDEF if EL2 isn't implemented. > + } > +#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 */ Nit: "AArch64" with two capital 'A's (here and elsewhere). > + 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; > + } > + } > + return arm_is_secure_below_el3(env); > +#else > + return false; > +#endif > +} I checked your git tree and we don't actually use arm_is_secure_below_el3() anywhere except in arm_is_secure(), do we? That suggests to me we should just fold the two functions together. Can these functions live in internals.h rather than cpu.h? (The difference is that internals.h is restricted to only target-arm/ code whereas cpu.h is auto-included for a much wider set of files.) thanks -- PMM
On 06.10.2014 07:56, Peter Maydell wrote: > On 30 September 2014 22:49, Greg Bellows <greg.bellows@linaro.org> wrote: >> From: Fabian Aggeler <aggelerf@ethz.ch> >> >> 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.fedorov@samsung.com> >> Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch> >> Signed-off-by: Greg Bellows <greg.bellows@linaro.org> >> --- >> target-arm/cpu.h | 38 ++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 38 insertions(+) >> >> diff --git a/target-arm/cpu.h b/target-arm/cpu.h >> index 81fffd2..10afef0 100644 >> --- a/target-arm/cpu.h >> +++ b/target-arm/cpu.h >> @@ -753,6 +753,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; > I would be happy to fold both these identical 'return false' > cases together and have a comment that it's only IMPDEF > if EL2 isn't implemented. > >> + } >> +#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 */ > Nit: "AArch64" with two capital 'A's (here and elsewhere). > >> + 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; >> + } >> + } >> + return arm_is_secure_below_el3(env); >> +#else >> + return false; >> +#endif >> +} > I checked your git tree and we don't actually use > arm_is_secure_below_el3() anywhere except in > arm_is_secure(), do we? That suggests to me we should > just fold the two functions together. > > Can these functions live in internals.h rather than cpu.h? > (The difference is that internals.h is restricted to only > target-arm/ code whereas cpu.h is auto-included for a much > wider set of files.) Probably arm_is_secure() would be used by ARM GIC emulation until there is no better way to determine memory transaction NS tag. > > thanks > -- PMM
On 6 October 2014 18:57, Sergey Fedorov <serge.fdrv@gmail.com> wrote: > On 06.10.2014 07:56, Peter Maydell wrote: >> Can these functions live in internals.h rather than cpu.h? >> (The difference is that internals.h is restricted to only >> target-arm/ code whereas cpu.h is auto-included for a much >> wider set of files.) > > Probably arm_is_secure() would be used by ARM GIC emulation until there > is no better way to determine memory transaction NS tag. We could have the GIC code temporarily include internals.h, which would be a nice big red flag that it was doing things the wrong way :-) -- PMM
On 6 October 2014 09:56, Peter Maydell <peter.maydell@linaro.org> wrote: > On 30 September 2014 22:49, Greg Bellows <greg.bellows@linaro.org> wrote: > > From: Fabian Aggeler <aggelerf@ethz.ch> > > > > 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.fedorov@samsung.com> > > Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch> > > Signed-off-by: Greg Bellows <greg.bellows@linaro.org> > > --- > > target-arm/cpu.h | 38 ++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 38 insertions(+) > > > > diff --git a/target-arm/cpu.h b/target-arm/cpu.h > > index 81fffd2..10afef0 100644 > > --- a/target-arm/cpu.h > > +++ b/target-arm/cpu.h > > @@ -753,6 +753,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; > > I would be happy to fold both these identical 'return false' > cases together and have a comment that it's only IMPDEF > if EL2 isn't implemented. > Yes, this makes sense. Fixed in v6. > > > + } > > +#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 */ > > Nit: "AArch64" with two capital 'A's (here and elsewhere). > > > + 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; > > + } > > + } > > + return arm_is_secure_below_el3(env); > > +#else > > + return false; > > +#endif > > +} > > I checked your git tree and we don't actually use > arm_is_secure_below_el3() anywhere except in > arm_is_secure(), do we? That suggests to me we should > just fold the two functions together. > This is true and I contemplated this myself. The reason I did not fold them together is because they match what is defined in the ARM v8 ARM and the below_el3 pseudo-function is actually used elsewhere in the spec separate from isSecure(). Honestly, I can go whichever way, so given the above what is your preference? > > Can these functions live in internals.h rather than cpu.h? > (The difference is that internals.h is restricted to only > target-arm/ code whereas cpu.h is auto-included for a much > wider set of files.) > I can move the code, but how does it differ from the likes of arm_feature() or arm_el_is_aa64()? They seem to serve the same utility purpose. > > thanks > -- PMM >
On 6 October 2014 20:45, Greg Bellows <greg.bellows@linaro.org> wrote: > On 6 October 2014 09:56, Peter Maydell <peter.maydell@linaro.org> wrote: >> I checked your git tree and we don't actually use >> arm_is_secure_below_el3() anywhere except in >> arm_is_secure(), do we? That suggests to me we should >> just fold the two functions together. > > > This is true and I contemplated this myself. The reason I did not fold them > together is because they match what is defined in the ARM v8 ARM and the > below_el3 pseudo-function is actually used elsewhere in the spec separate > from isSecure(). Honestly, I can go whichever way, so given the above what > is your preference? Ah, my search through the ARM ARM didn't find the pseudocode function first time around. I was also a bit confused by the comment on the function, which you have as: /* Return true if exception level below EL3 is in secure state */ which implies that it's just "arm_is_secure() but it only works if you're not in EL3", whereas the ARM ARM says: // Return TRUE if an Exception level below EL3 is in Secure state // or would be following an exception return to that level. // Differs from IsSecure in that it ignores the current EL or Mode // in considering security state. which makes it clearer why it might be useful and why it's not the same as arm_is_secure(). So yes, we should retain the two separate functions, but we should improve the comment describing what arm_is_secure_below_el3() does. You should use is_a64() rather than directly looking at env->aarch64, incidentally. >> Can these functions live in internals.h rather than cpu.h? >> (The difference is that internals.h is restricted to only >> target-arm/ code whereas cpu.h is auto-included for a much >> wider set of files.) > > > I can move the code, but how does it differ from the likes of arm_feature() > or arm_el_is_aa64()? They seem to serve the same utility purpose. Many functions in cpu.h are there simply because they were written before we added internals.h (ie for legacy reasons). I'd prefer not to add to the set of functions in the wrong place, though. thanks -- PMM
On 6 October 2014 15:07, Peter Maydell <peter.maydell@linaro.org> wrote: > On 6 October 2014 20:45, Greg Bellows <greg.bellows@linaro.org> wrote: > > On 6 October 2014 09:56, Peter Maydell <peter.maydell@linaro.org> wrote: > >> I checked your git tree and we don't actually use > >> arm_is_secure_below_el3() anywhere except in > >> arm_is_secure(), do we? That suggests to me we should > >> just fold the two functions together. > > > > > > This is true and I contemplated this myself. The reason I did not fold > them > > together is because they match what is defined in the ARM v8 ARM and the > > below_el3 pseudo-function is actually used elsewhere in the spec separate > > from isSecure(). Honestly, I can go whichever way, so given the above > what > > is your preference? > > Ah, my search through the ARM ARM didn't find the pseudocode > function first time around. I was also a bit confused by > the comment on the function, which you have as: > /* Return true if exception level below EL3 is in secure state */ > > which implies that it's just "arm_is_secure() but it only > works if you're not in EL3", whereas the ARM ARM says: > > // Return TRUE if an Exception level below EL3 is in Secure state > // or would be following an exception return to that level. > // Differs from IsSecure in that it ignores the current EL or Mode > // in considering security state. > > which makes it clearer why it might be useful and why > it's not the same as arm_is_secure(). So yes, we should > retain the two separate functions, but we should improve > the comment describing what arm_is_secure_below_el3() does. > > Comments added in v6. > You should use is_a64() rather than directly looking at > env->aarch64, incidentally. > Since I am touching arm_current_el(), should I go ahead and fix it to use is_a64() as well? > > >> Can these functions live in internals.h rather than cpu.h? > >> (The difference is that internals.h is restricted to only > >> target-arm/ code whereas cpu.h is auto-included for a much > >> wider set of files.) > > > > > > I can move the code, but how does it differ from the likes of > arm_feature() > > or arm_el_is_aa64()? They seem to serve the same utility purpose. > > Many functions in cpu.h are there simply because they were > written before we added internals.h (ie for legacy reasons). > I'd prefer not to add to the set of functions in the wrong > place, though. > > I'll move in v6. Should I also go ahead and move arm_current_el() to internals.h as well since I am touching it? > thanks > -- PMM >
On 6 October 2014 21:47, Greg Bellows <greg.bellows@linaro.org> wrote: > > > On 6 October 2014 15:07, Peter Maydell <peter.maydell@linaro.org> wrote: >> You should use is_a64() rather than directly looking at >> env->aarch64, incidentally. > > > Since I am touching arm_current_el(), should I go ahead and fix it to use > is_a64() as well? Optional. > I'll move in v6. Should I also go ahead and move arm_current_el() to > internals.h as well since I am touching it? No. Let's try to keep the scope of this patchset under control. -- PMM
Unfortunately, the arm_is_secure*() functions cannot be moved either as they are used within cpu.h. Greg On 6 October 2014 16:07, Peter Maydell <peter.maydell@linaro.org> wrote: > On 6 October 2014 21:47, Greg Bellows <greg.bellows@linaro.org> wrote: > > > > > > On 6 October 2014 15:07, Peter Maydell <peter.maydell@linaro.org> wrote: > >> You should use is_a64() rather than directly looking at > >> env->aarch64, incidentally. > > > > > > Since I am touching arm_current_el(), should I go ahead and fix it to use > > is_a64() as well? > > Optional. > > > I'll move in v6. Should I also go ahead and move arm_current_el() to > > internals.h as well since I am touching it? > > No. Let's try to keep the scope of this patchset under control. > > -- PMM >
diff --git a/target-arm/cpu.h b/target-arm/cpu.h index 81fffd2..10afef0 100644 --- a/target-arm/cpu.h +++ b/target-arm/cpu.h @@ -753,6 +753,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; + } + } + 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) {