M profile cores can never trap on WFI or WFE instructions. Check for
M profile in check_wfx_trap() to ensure this.

The existing code will do the right thing for v7M cores because
the hcr_el2 and scr_el3 registers will be all-zeroes and so we
won't attempt to trap, but when we start setting ARM_FEATURE_V8
for v8M cores the v8A handling of SCTLR.nTWE and .nTWI will not
give the right results.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/op_helper.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
index 2a85666..5a94a5f 100644
--- a/target/arm/op_helper.c
+++ b/target/arm/op_helper.c
@@ -370,6 +370,11 @@ static inline int check_wfx_trap(CPUARMState *env, bool is_wfe)
     int cur_el = arm_current_el(env);
     uint64_t mask;
 
+    if (arm_feature(env, ARM_FEATURE_M)) {
+        /* M profile cores can never trap WFI/WFE. */
+        return 0;
+    }
+
     /* If we are currently in EL0 then we need to check if SCTLR is set up for
      * WFx instructions being trapped to EL1. These trap bits don't exist in v7.
      */
-- 
2.7.4

On Wed, Aug 02, 2017 at 05:43:48PM +0100, Peter Maydell wrote:
> M profile cores can never trap on WFI or WFE instructions. Check for
> M profile in check_wfx_trap() to ensure this.
> 
> The existing code will do the right thing for v7M cores because
> the hcr_el2 and scr_el3 registers will be all-zeroes and so we
> won't attempt to trap, but when we start setting ARM_FEATURE_V8
> for v8M cores the v8A handling of SCTLR.nTWE and .nTWI will not
> give the right results.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> ---
>  target/arm/op_helper.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
> index 2a85666..5a94a5f 100644
> --- a/target/arm/op_helper.c
> +++ b/target/arm/op_helper.c
> @@ -370,6 +370,11 @@ static inline int check_wfx_trap(CPUARMState *env, bool is_wfe)
>      int cur_el = arm_current_el(env);
>      uint64_t mask;
>  
> +    if (arm_feature(env, ARM_FEATURE_M)) {
> +        /* M profile cores can never trap WFI/WFE. */
> +        return 0;
> +    }
> +
>      /* If we are currently in EL0 then we need to check if SCTLR is set up for
>       * WFx instructions being trapped to EL1. These trap bits don't exist in v7.
>       */
> -- 
> 2.7.4
> 
> 

On 08/02/2017 09:43 AM, Peter Maydell wrote:
> M profile cores can never trap on WFI or WFE instructions. Check for
> M profile in check_wfx_trap() to ensure this.
> 
> The existing code will do the right thing for v7M cores because
> the hcr_el2 and scr_el3 registers will be all-zeroes and so we
> won't attempt to trap, but when we start setting ARM_FEATURE_V8
> for v8M cores the v8A handling of SCTLR.nTWE and .nTWI will not
> give the right results.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/op_helper.c | 5 +++++
>  1 file changed, 5 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

While looking at this, I think there's an error in helper_wfi.  The early exit
for cpu_has_work should happen after the exception check.


r~

On Thu, Aug 03, 2017 at 01:28:28PM -0700, Richard Henderson wrote:
> On 08/02/2017 09:43 AM, Peter Maydell wrote:
> > M profile cores can never trap on WFI or WFE instructions. Check for
> > M profile in check_wfx_trap() to ensure this.
> > 
> > The existing code will do the right thing for v7M cores because
> > the hcr_el2 and scr_el3 registers will be all-zeroes and so we
> > won't attempt to trap, but when we start setting ARM_FEATURE_V8
> > for v8M cores the v8A handling of SCTLR.nTWE and .nTWI will not
> > give the right results.
> > 
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >  target/arm/op_helper.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> 
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 
> While looking at this, I think there's an error in helper_wfi.  The early exit
> for cpu_has_work should happen after the exception check.

I don't have the spec at hand but IIRC the trap should only happen
if the processor would have entered the low-power state (i.e if
there's no work).

A comment in the code would probably be good...

Cheers,
Edgar 

On 08/03/2017 01:40 PM, Edgar E. Iglesias wrote:
> I don't have the spec at hand but IIRC the trap should only happen
> if the processor would have entered the low-power state (i.e if
> there's no work).

  when SystemHintOp_WFE
    if IsEventRegisterSet() then
      ClearEventRegister();
    else
      if PSTATE.EL == EL0 then
        AArch64.CheckForWFxTrap(EL1, TRUE);
      if HaveEL(EL2) && !IsSecure()
         && PSTATE.EL IN {EL0, EL1} && !IsInHost() then
        AArch64.CheckForWFxTrap(EL2, TRUE);
      if HaveEL(EL3) && PSTATE.EL != EL3 then
        AArch64.CheckForWFxTrap(EL3, TRUE);
      WaitForEvent();

Ah, I see what you mean, since WaitForEvent is also
described as checking EventRegister.

Thanks.


r~

On 3 August 2017 at 21:28, Richard Henderson <rth@twiddle.net> wrote:
> While looking at this, I think there's an error in helper_wfi.  The early exit
> for cpu_has_work should happen after the exception check.

No, that's deliberate; as Edgar says, the trap only
happens "if the instruction would otherwise have caused the
PE to enter a low-power state".
The rationale AIUI is that the traps to EL2 are there so
that when an EL guest does a WFI in its idle loop the EL2
hypervisor can gain control and give the CPU to
something else. This obviously imposes overhead, so if
the WFI wouldn't actually halt (because there's already
a condition that will cause it to wake up) it's more
efficient just to let the guest continue to execute.
(It also means that NOP is a valid WFI implementation,
though I think that's just a coincidental bonus.)

In fact the architecture gives even more flexibility
in that it only requires the trap to be taken "if the
instruction does not complete in finite time in the
absence of a Wakeup event", so you can do more complicated
things like "just pause for a short period of time to
see if an interrupt might come in and wake us up,
before giving up and taking the trap to EL2".

thanks
-- PMM