RE: [PATCH 34/39] target/hexagon: Add TLB, k0 {un,}lock

[<ltaylorsimpson@gmail.com>]

> -----Original Message-----
> From: Brian Cain <brian.cain@oss.qualcomm.com>
> Sent: Monday, March 3, 2025 10:24 AM
> To: qemu-devel@nongnu.org
> Cc: richard.henderson@linaro.org; philmd@linaro.org;
> quic_mathbern@quicinc.com; ale@rev.ng; anjo@rev.ng;
> quic_mliebel@quicinc.com; ltaylorsimpson@gmail.com;
> alex.bennee@linaro.org; quic_mburton@quicinc.com;
> sidneym@quicinc.com; Brian Cain <bcain@quicinc.com>
> Subject: Re: [PATCH 34/39] target/hexagon: Add TLB, k0 {un,}lock
> 
> 
> On 2/28/2025 11:28 PM, Brian Cain wrote:
> > From: Brian Cain <bcain@quicinc.com>
> >
> > Signed-off-by: Brian Cain <brian.cain@oss.qualcomm.com>
> > ---
> >   target/hexagon/sys_macros.h |   8 +--
> >   target/hexagon/op_helper.c  | 104
> ++++++++++++++++++++++++++++++++++++
> >   2 files changed, 108 insertions(+), 4 deletions(-)
> >
> > diff --git a/target/hexagon/sys_macros.h b/target/hexagon/sys_macros.h
> > index 3c4c3c7aa5..e5dc1ce0ab 100644
> > --- a/target/hexagon/sys_macros.h
> > +++ b/target/hexagon/sys_macros.h
> > @@ -143,11 +143,11 @@
> >   #define fDCINVIDX(REG)
> >   #define fDCINVA(REG) do { REG = REG; } while (0) /* Nothing to do in
> > qemu */
> >
> > -#define fSET_TLB_LOCK()       g_assert_not_reached()
> > -#define fCLEAR_TLB_LOCK()     g_assert_not_reached()
> > +#define fSET_TLB_LOCK()       hex_tlb_lock(env);
> > +#define fCLEAR_TLB_LOCK()     hex_tlb_unlock(env);
> >
> > -#define fSET_K0_LOCK()        g_assert_not_reached()
> > -#define fCLEAR_K0_LOCK()      g_assert_not_reached()
> > +#define fSET_K0_LOCK()        hex_k0_lock(env);
> > +#define fCLEAR_K0_LOCK()      hex_k0_unlock(env);
> >
> >   #define fTLB_IDXMASK(INDEX) \
> >       ((INDEX) & (fPOW2_ROUNDUP(fCAST4u(env_archcpu(env)-
> >num_tlbs)) -
> > 1)) diff --git a/target/hexagon/op_helper.c
> > b/target/hexagon/op_helper.c index 702c3dd3c6..f3b14fbf58 100644
> > --- a/target/hexagon/op_helper.c
> > +++ b/target/hexagon/op_helper.c
> > @@ -1184,6 +1184,110 @@ void HELPER(modify_ssr)(CPUHexagonState
> *env, uint32_t new, uint32_t old)
> >       BQL_LOCK_GUARD();
> >       hexagon_modify_ssr(env, new, old);
> >   }
> > +
> > +static void hex_k0_lock(CPUHexagonState *env) {
> > +    BQL_LOCK_GUARD();
> > +    g_assert((env->k0_lock_count == 0) || (env->k0_lock_count == 1));
> > +
> > +    uint32_t syscfg = arch_get_system_reg(env, HEX_SREG_SYSCFG);

Minor nit - registers should be target_ulong type.

> > +    if (GET_SYSCFG_FIELD(SYSCFG_K0LOCK, syscfg)) {
> > +        if (env->k0_lock_state == HEX_LOCK_QUEUED) {

Are HEX_LOCK_* defined in an earlier patch in this series.  Can we move the definitions here?

> > +            env->next_PC += 4;
> > +            env->k0_lock_count++;
> > +            env->k0_lock_state = HEX_LOCK_OWNER;
> > +            SET_SYSCFG_FIELD(env, SYSCFG_K0LOCK, 1);
> > +            return;
> > +        }
> > +        if (env->k0_lock_state == HEX_LOCK_OWNER) {
> > +            qemu_log_mask(LOG_GUEST_ERROR,
> > +                          "Double k0lock at PC: 0x%x, thread may hang\n",
> > +                          env->next_PC);
> > +            env->next_PC += 4;
> > +            CPUState *cs = env_cpu(env);
> > +            cpu_interrupt(cs, CPU_INTERRUPT_HALT);
> > +            return;
> > +        }
> > +        env->k0_lock_state = HEX_LOCK_WAITING;
> > +        CPUState *cs = env_cpu(env);
> > +        cpu_interrupt(cs, CPU_INTERRUPT_HALT);
> > +    } else {
> > +        env->next_PC += 4;
> > +        env->k0_lock_count++;
> > +        env->k0_lock_state = HEX_LOCK_OWNER;
> > +        SET_SYSCFG_FIELD(env, SYSCFG_K0LOCK, 1);
> > +    }
> > +
> > +}
> 
> This was discussed previously at
> https://lore.kernel.org/qemu-
> devel/CH3PR02MB102479550F96F09E0C9D50BA7B87B2@CH3PR02MB10247.n
> amprd02.prod.outlook.com/
> 
> We have taken some but not all of the suggestions from then.  One of our
> concerns is regarding an architectural requirement for "fairness" with regards
> to picking the hardware thread to be selected to pass the lock to.  If we
> unleash the thundering herd, does this just mean that the fairness is
> dependent on the host scheduler design / configuration?
> 
> Also - I note that we didn't take the suggestions regarding cpu_loop_exit /
> cpu_loop_exit_restore.  That was an oversight, the next revision will include
> that update.
> 
> > +
> > +static void hex_k0_unlock(CPUHexagonState *env) {
> > +    BQL_LOCK_GUARD();
> > +    g_assert((env->k0_lock_count == 0) || (env->k0_lock_count == 1));
> > +
> > +    /* Nothing to do if the k0 isn't locked by this thread */
> > +    uint32_t syscfg = arch_get_system_reg(env, HEX_SREG_SYSCFG);

target_ulong

> > +    if ((GET_SYSCFG_FIELD(SYSCFG_K0LOCK, syscfg) == 0) ||
> > +        (env->k0_lock_state != HEX_LOCK_OWNER)) {
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "thread %d attempted to unlock k0 without having the "
> > +                      "lock, k0_lock state = %d, syscfg:k0 = %d\n",
> > +                      env->threadId, env->k0_lock_state,
> > +                      GET_SYSCFG_FIELD(SYSCFG_K0LOCK, syscfg));
> > +        g_assert(env->k0_lock_state != HEX_LOCK_WAITING);
> > +        return;
> > +    }
> > +
> > +    env->k0_lock_count--;
> > +    env->k0_lock_state = HEX_LOCK_UNLOCKED;
> > +    SET_SYSCFG_FIELD(env, SYSCFG_K0LOCK, 0);
> > +
> > +    /* Look for a thread to unlock */
> > +    unsigned int this_threadId = env->threadId;
> > +    CPUHexagonState *unlock_thread = NULL;
> > +    CPUState *cs;
> > +    CPU_FOREACH(cs) {
> > +        CPUHexagonState *thread = cpu_env(cs);
> > +
> > +        /*
> > +         * The hardware implements round-robin fairness, so we look for
> threads
> > +         * starting at env->threadId + 1 and incrementing modulo the number
> of
> > +         * threads.
> > +         *
> > +         * To implement this, we check if thread is a earlier in the modulo
> > +         * sequence than unlock_thread.
> > +         *     if unlock thread is higher than this thread
> > +         *         thread must be between this thread and unlock_thread
> > +         *     else
> > +         *         thread higher than this thread is ahead of unlock_thread
> > +         *         thread must be lower then unlock thread
> > +         */
> > +        if (thread->k0_lock_state == HEX_LOCK_WAITING) {
> > +            if (!unlock_thread) {
> > +                unlock_thread = thread;
> > +            } else if (unlock_thread->threadId > this_threadId) {
> > +                if (this_threadId < thread->threadId &&
> > +                    thread->threadId < unlock_thread->threadId) {
> > +                    unlock_thread = thread;
> > +                }
> > +            } else {
> > +                if (thread->threadId > this_threadId) {
> > +                    unlock_thread = thread;
> > +                }
> > +                if (thread->threadId < unlock_thread->threadId) {
> > +                    unlock_thread = thread;
> > +                }
> > +            }
> > +        }
> > +    }
> > +    if (unlock_thread) {
> > +        cs = env_cpu(unlock_thread);
> > +        unlock_thread->k0_lock_state = HEX_LOCK_QUEUED;
> > +        SET_SYSCFG_FIELD(unlock_thread, SYSCFG_K0LOCK, 1);
> > +        cpu_interrupt(cs, CPU_INTERRUPT_K0_UNLOCK);
> > +    }
> > +
> > +}
> >   #endif
> >
> >