Re: [Qemu-devel] [[PATCH] 7/7] target-arm: Add WFx instruction trap support

[Greg Bellows]

On Thu, Apr 16, 2015 at 1:22 PM, Peter Maydell <address@hidden> wrote:

On 27 March 2015 at 19:10, Greg Bellows <address@hidden> wrote:
> Add support for trapping WFI and WFE instructions to the proper EL when
> SCTLR/SCR/HCR settings apply.
>
> Signed-off-by: Greg Bellows <address@hidden>
> ---
>  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?

​EL can't be negative so this made sense.​

 

A comment that gave the semantics of the return value would be nice.

​Yeah, may have helped with the above?

​

 

> +{
> +    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.

​I'll remove

​

 

> +            /* 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...

​Me either :-)  Probably made sense when I wrote it.​

 

> +            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.

It must have been left over from when I simplified but you are right it should not be there.  Removed in v2

 

> +                /* 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.

​Fixed

​

 

> +                    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?

Yeah, the loop came out of me simplifying the spec pseudo code, but it is excessive.  Will fix in v2.

 

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.

​Changed function to take 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