Re: [Qemu-devel] [PATCH 1/2] target-arm: Split DISAS_YIELD from DISAS_WFE

[Peter Maydell]

On 27 June 2015 at 03:25, Peter Crosthwaite
<address@hidden> wrote:
> On Mon, Jun 15, 2015 at 11:49 AM, Peter Maydell
> <address@hidden> 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 <address@hidden>
>> ---
>>  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