diff mbox

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

Message ID 1427483446-31900-8-git-send-email-greg.bellows@linaro.org
State New
Headers show

Commit Message

Greg Bellows March 27, 2015, 7:10 p.m. UTC 
Add support for trapping WFI and WFE instructions to the proper EL when
SCTLR/SCR/HCR settings apply.

Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
---
 target-arm/op_helper.c | 75 +++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 71 insertions(+), 4 deletions(-)

Comments

Peter Maydell April 16, 2015, 6:22 p.m. UTC | #1 
On 27 March 2015 at 19:10, Greg Bellows <greg.bellows@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.bellows@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
Greg Bellows April 17, 2015, 3:47 p.m. UTC | #2 
On Thu, Apr 16, 2015 at 1:22 PM, Peter Maydell <peter.maydell@linaro.org>
wrote:

> On 27 March 2015 at 19:10, Greg Bellows <greg.bellows@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.bellows@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?
>

​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
>
Peter Maydell April 17, 2015, 3:50 p.m. UTC | #3 
On 17 April 2015 at 16:47, Greg Bellows <greg.bellows@linaro.org> wrote:
>
>
> On Thu, Apr 16, 2015 at 1:22 PM, Peter Maydell <peter.maydell@linaro.org>
> wrote:
>>
>> On 27 March 2015 at 19:10, Greg Bellows <greg.bellows@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.bellows@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?
>
>
> EL can't be negative so this made sense.

Mostly our existing functions that return an EL use 'int'
(eg arm_current_el()), so my thinking was that 'int' would be
more in line with those.

-- PMM
diff mbox

Patch

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)
+{
+    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;
+            /* Secure EL1 is only valid in AArch64.  If EL0 is AArch64 then so
+             * must be EL1.
+             */
+            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)) {
+                /* 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) {
+                    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;
+        }
+    }
+
+    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;
+
+    target_el = check_wfx_trap(env, false);
+    if (target_el) {
+        cs->exception_index = EXCP_UDEF;
+        env->exception.syndrome = syn_wfx(1, 0xe, false);
+        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;
 
     /* 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);
 }