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

[Greg Bellows]

On 6 October 2014 15:07, Peter Maydell <address@hidden> wrote:

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.

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