On 27 June 2015 at 03:25, Peter Crosthwaite <peter.crosthwa...@xilinx.com> wrote: > On Mon, Jun 15, 2015 at 11:49 AM, Peter Maydell > <peter.mayd...@linaro.org> wrote: >> Currently we use DISAS_WFE for both WFE and YIELD instructions. >> This is functionally correct because at the moment both of them >> are implemented as "yield this CPU back to the top level loop so >> another CPU has a chance to run". However it's rather confusing >> that YIELD ends up calling HELPER(wfe), and if we ever want to >> implement real behaviour for WFE and SEV it's likely to trip us up. >> >> Split out the yield codepath to use DISAS_YIELD and a new >> HELPER(yield) function. >> >> Signed-off-by: Peter Maydell <peter.mayd...@linaro.org> >> --- >> target-arm/helper.h | 1 + >> target-arm/op_helper.c | 12 ++++++++++++ >> target-arm/translate-a64.c | 6 ++++++ >> target-arm/translate.h | 1 + >> 4 files changed, 20 insertions(+) >> >> diff --git a/target-arm/helper.h b/target-arm/helper.h >> index fc885de..827b33d 100644 >> --- a/target-arm/helper.h >> +++ b/target-arm/helper.h >> @@ -50,6 +50,7 @@ DEF_HELPER_2(exception_internal, void, env, i32) >> DEF_HELPER_4(exception_with_syndrome, void, env, i32, i32, i32) >> DEF_HELPER_1(wfi, void, env) >> DEF_HELPER_1(wfe, void, env) >> +DEF_HELPER_1(yield, void, env) >> DEF_HELPER_1(pre_hvc, void, env) >> DEF_HELPER_2(pre_smc, void, env, i32) >> >> diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c >> index 7fa32c4..5f06ca0 100644 >> --- a/target-arm/op_helper.c >> +++ b/target-arm/op_helper.c >> @@ -334,6 +334,18 @@ void HELPER(wfe)(CPUARMState *env) >> cpu_loop_exit(cs); >> } >> >> +void HELPER(yield)(CPUARMState *env) >> +{ >> + CPUState *cs = CPU(arm_env_get_cpu(env)); >> + >> + /* This is a non-trappable hint instruction, so semantically >> + * different from WFE even though we currently implement it >> + * identically. Yield control back to the top level loop. >> + */ > > Comment referencing out of scope functionality is a trap for > developers, anyone patching WFE and not thinking about yield needs to > be aware of comment staleness over here. > >> + cs->exception_index = EXCP_YIELD; >> + cpu_loop_exit(cs); >> +} >> + > > I think the real problem here is the inaccuracy of WFE and not yield, > so that is where such an explanatory comment should go. You can also > make it more self documenting by maying wfe call yield: > > HELPER(wfe)(CPUARMState *env) > { > /* This is a hint instruction semantically different from YIELD > even though we currently > * implement it identically. Yield control back to the top level loop ... > */ > HELPER(yield)(env); > }
Yeah, I agree. I'll respin. -- PMM