| Message ID | 1501692241-23310-3-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
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 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
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~
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. */
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(+)