On Thu, Apr 16, 2015 at 1:22 PM, Peter Maydell <peter.mayd...@linaro.org>
wrote:
> 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?
>
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
>
