Re: [PATCH v5 05/28] target/hexagon: Implement wait helper

On 8/14/2026 12:29 PM, Philippe Mathieu-Daudé wrote:
> Hi Brian, Taylor,
>
> (now committed as acd2b7a2f19ac04bd313e816e5f9e7f87adc9858)
>
> On 29/5/26 23:57, Brian Cain wrote:
>> From: Brian Cain <[email protected]>
>>
>> Reviewed-by: Taylor Simpson <[email protected]>
>> Signed-off-by: Brian Cain <[email protected]>
>> ---
>>   target/hexagon/gen_tcg_sys.h |  2 +-
>>   target/hexagon/op_helper.c   | 60 ++++++++++++++++++++++++++++++++++--
>>   2 files changed, 59 insertions(+), 3 deletions(-)
>>
>> diff --git a/target/hexagon/gen_tcg_sys.h b/target/hexagon/gen_tcg_sys.h
>> index 000fc415c74..98cf6216780 100644
>> --- a/target/hexagon/gen_tcg_sys.h
>> +++ b/target/hexagon/gen_tcg_sys.h
>> @@ -80,7 +80,7 @@
>>   #define fGEN_TCG_Y2_wait(SHORTCODE) \
>>       do { \
>>           RsV = RsV; \
>> -        gen_helper_wait(tcg_env, tcg_constant_tl(ctx->pkt->pc)); \
>> +        gen_helper_wait(tcg_env, tcg_constant_tl(ctx->pkt.pc)); \
>>       } while (0)
>>     #define fGEN_TCG_Y2_resume(SHORTCODE) \
>> diff --git a/target/hexagon/op_helper.c b/target/hexagon/op_helper.c
>> index 235c70f7370..d40061de06e 100644
>> --- a/target/hexagon/op_helper.c
>> +++ b/target/hexagon/op_helper.c
>> @@ -1486,9 +1486,65 @@ void HELPER(stop)(CPUHexagonState *env)
>>       hexagon_stop_thread(env);
>>   }
>>   -void HELPER(wait)(CPUHexagonState *env, target_ulong PC)
>> +static void set_wait_mode(CPUHexagonState *env)
>>   {
>> -    g_assert_not_reached();
>> +    HexagonCPU *cpu;
>> +    uint32_t modectl;
>> +    uint32_t thread_wait_mask;
>> +
>> +    g_assert(bql_locked());
>> +
>> +    cpu = env_archcpu(env);
>> +    if (!cpu->globalregs) {
>> +        return;
>> +    }
>> +    modectl =
>> +        hexagon_globalreg_read(cpu->globalregs, HEX_SREG_MODECTL,
>> +                               env->threadId);
>> +    thread_wait_mask = GET_FIELD(MODECTL_W, modectl);
>> +    thread_wait_mask |= 0x1 << env->threadId;
>> +    SET_SYSTEM_FIELD(env, HEX_SREG_MODECTL, MODECTL_W, 
>> thread_wait_mask);
>> +}
>> +
>> +static void hexagon_wait_thread(CPUHexagonState *env, uint32_t PC)
>> +{
>> +    CPUState *cs;
>> +
>> +    g_assert(bql_locked());
>> +
>> +    if (qemu_loglevel_mask(LOG_GUEST_ERROR) &&
>> +        (env->k0_lock_state != HEX_LOCK_UNLOCKED ||
>> +         env->tlb_lock_state != HEX_LOCK_UNLOCKED)) {
>> +        qemu_log("WARNING: executing wait() with acquired lock"
>> +                 "may lead to deadlock\n");
>> +    }
>> +    g_assert(get_exe_mode(env) != HEX_EXE_MODE_WAIT);
>> +
>> +    cs = env_cpu(env);
>> +    /*
>> +     * The addtion of cpu_has_work is borrowed from arm's wfi helper
>> +     * and is critical for our stability
>
> Could you explain this comment?

It's a note that was added to our downstream fork when fixing a bug 
attributed to hexagon_wait_thread() that was a bit challenging to 
uncover because none of our existing regression suite detected it.  It 
was only uncovered in a "virtual platform" environment modeling the 
various cores among the SoC.

We can follow up with a reword/clarification or deletion if you prefer.

>
>> +     */
>> +    if ((cs->exception_index != HEX_EVENT_NONE) ||
>> +        (cpu_has_work(cs))) {
>> +        qemu_log_mask(CPU_LOG_INT,
>> +            "%s: thread %" PRIu32 " skipping WAIT mode, have some 
>> work\n",
>> +            __func__, env->threadId);
>> +        return;
>> +    }
>> +    set_wait_mode(env);
>> +    env->wait_next_pc = PC + 4;
>> +
>> +    cpu_interrupt(cs, CPU_INTERRUPT_HALT);
>> +}
>> +
>> +void HELPER(wait)(CPUHexagonState *env, uint32_t PC)
>> +{
>> +    BQL_LOCK_GUARD();
>> +
>> +    if (!fIN_DEBUG_MODE(env->threadId)) {
>> +        hexagon_wait_thread(env, PC);
>> +    }
>>   }
>>     void HELPER(resume)(CPUHexagonState *env, uint32_t mask)
>