Re: [Qemu-devel] [PATCH v5 02/33] target-arm: add arm_is_secure() function

[Peter Maydell]

On 6 October 2014 20:45, Greg Bellows <address@hidden> wrote:
> On 6 October 2014 09:56, Peter Maydell <address@hidden> 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