[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/2] target-arm: Split DISAS_YIELD from DISAS_WF
From: |
Peter Maydell |
Subject: |
Re: [Qemu-devel] [PATCH 1/2] target-arm: Split DISAS_YIELD from DISAS_WFE |
Date: |
Sun, 28 Jun 2015 22:53:09 +0100 |
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