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: Friday, February 28, 2025 11:29 PM
> To: qemu-devel@nongnu.org
> Cc: brian.cain@oss.qualcomm.com; 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: [PATCH 34/39] target/hexagon: Add TLB, k0 {un,}lock
> 
> 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);

Move these to the patch that implements TLB lock/unlock.

> 
> -#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);
> +    if (GET_SYSCFG_FIELD(SYSCFG_K0LOCK, syscfg)) {
> +        if (env->k0_lock_state == HEX_LOCK_QUEUED) {
> +            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);

QEMU coding standards prefer to put declarations at the beginning of the code block (just after the open curly brace).

> +            cpu_interrupt(cs, CPU_INTERRUPT_HALT);
> +            return;
> +        }
> +        env->k0_lock_state = HEX_LOCK_WAITING;
> +        CPUState *cs = env_cpu(env);

Ditto

> +        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);
> +    }
> +
> +}
> +
> +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);
> +    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;

Ditto

Otherwise
Reviewed-by: Taylor Simpson <ltaylorsimpson@gmail.com>