On 6 October 2014 15:07, Peter Maydell <peter.mayd...@linaro.org> wrote:
> On 6 October 2014 20:45, Greg Bellows <greg.bell...@linaro.org> wrote: > > On 6 October 2014 09:56, Peter Maydell <peter.mayd...@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 >
