On 27 March 2015 at 19:10, Greg Bellows <greg.bell...@linaro.org> wrote:
> Add support for trapping WFI and WFE instructions to the proper EL when
> SCTLR/SCR/HCR settings apply.
>
> Signed-off-by: Greg Bellows <greg.bell...@linaro.org>
> ---
> target-arm/op_helper.c | 75
> +++++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 71 insertions(+), 4 deletions(-)
>
> diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
> index aa175b5..d7e734d 100644
> --- a/target-arm/op_helper.c
> +++ b/target-arm/op_helper.c
> @@ -209,23 +209,90 @@ uint32_t HELPER(usat16)(CPUARMState *env, uint32_t x,
> uint32_t shift)
> return res;
> }
>
> +static inline uint32_t check_wfx_trap(CPUARMState *env, bool is_wfe)
Why uint32_t rather than int?
A comment that gave the semantics of the return value would be nice.
> +{
> + int cur_el = arm_current_el(env), el;
> + uint32_t target_el = 0;
> + uint64_t mask;
> +
> + /* Check whether any EL controls above us trap WFx instructions */
> + for (el = cur_el + 1; el <= 3; el++) {
> + switch (el) {
> + case 1:
> + mask = (is_wfe) ? SCTLR_nTWE : SCTLR_nTWI;
I don't think the brackets round is_wfe are needed.
> + /* Secure EL1 is only valid in AArch64. If EL0 is AArch64 then
> so
> + * must be EL1.
> + */
I'm not clear how this comment relates to the code...
> + if (arm_el_is_aa64(env, 1)) {
> + if ((env->cp15.sctlr_el[1] & mask) == 0) {
> + target_el = el;
> + }
> + } else if (arm_feature(env, ARM_FEATURE_V8) &&
> + !arm_is_secure(env)) {
Why the !arm_is_secure() check? WFI/WFE at Secure-PL0 should trap
to Secure-PL1 if these bits are clear, I think.
> + /* SCTLR WFx SCTLR trap bits only exist in ARMv8 */
> + if ((A32_BANKED_CURRENT_REG_GET(env, sctlr) & mask) == 0) {
> + target_el = el;
> + }
> + }
> + break;
> + case 2:
> + if (arm_feature(env, ARM_FEATURE_EL2) && !arm_is_secure(env)) {
> + mask = (is_wfe) ? HCR_TWE : HCR_TWI;
> + if ((env->cp15.hcr_el2 & mask) == mask) {
Just "if (val & mask)" is fine.
> + target_el = el;
> + }
> + }
> + break;
> + case 3:
> + if (arm_feature(env, ARM_FEATURE_EL3) &&
> + arm_feature(env, ARM_FEATURE_V8)) {
> + mask = (is_wfe) ? SCR_TWE : SCR_TWI;
> + if ((env->cp15.scr_el3 & mask) == mask) {
> + target_el = el;
> + }
> + }
> + break;
The loop-and-switch structure here seems a bit odd; why
not just have three successive if() statements?
Also, I think you have the exception priority wrong -- taking
a trap to EL1 should take priority over trapping to EL2, which
in turn is prioritised over trapping to EL3. So you want
something like
if (el < 1) {
if (conditions for trap to EL1) {
return 1;
}
}
if (el <= 2 && conditions for trap to EL2) {
return 2;
}
if (el <= 3 && conditions for trap to EL3) {
return 3;
}
return 0;
> + }
> + }
> +
> + return target_el;
> +}
> +
> void HELPER(wfi)(CPUARMState *env)
> {
> CPUState *cs = CPU(arm_env_get_cpu(env));
> -
> - cs->exception_index = EXCP_HLT;
> - cs->halted = 1;
> + uint32_t target_el = 0;
No point zero-initializing this variable, we set it
in the following line anyway.
> +
> + target_el = check_wfx_trap(env, false);
> + if (target_el) {
> + cs->exception_index = EXCP_UDEF;
> + env->exception.syndrome = syn_wfx(1, 0xe, false);
The third argument here is an int, not a bool.
> + env->exception.target_el = target_el;
> + env->pc -= 4;
> + } else {
> + cs->exception_index = EXCP_HLT;
> + cs->halted = 1;
> + }
> cpu_loop_exit(cs);
> }
>
> void HELPER(wfe)(CPUARMState *env)
> {
> CPUState *cs = CPU(arm_env_get_cpu(env));
> + uint32_t target_el = 0;
Ditto.
>
> /* Don't actually halt the CPU, just yield back to top
> * level loop
> */
> - cs->exception_index = EXCP_YIELD;
> + target_el = check_wfx_trap(env, true);
> + if (target_el) {
> + cs->exception_index = EXCP_UDEF;
> + env->exception.syndrome = syn_wfx(1, 0xe, true);
> + env->exception.target_el = target_el;
> + env->pc -= 4;
> + } else {
> + cs->exception_index = EXCP_YIELD;
> + }
> cpu_loop_exit(cs);
> }
>
> --
> 1.8.3.2
>
thanks
-- PMM
