RE: [PATCH 39/39] target/hexagon: Add pcycle setting functionality

[ltaylorsimpson]

> -----Original Message-----
> From: Brian Cain <brian.c...@oss.qualcomm.com>
> Sent: Friday, February 28, 2025 11:29 PM
> To: qemu-devel@nongnu.org
> Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org;
> phi...@linaro.org; quic_mathb...@quicinc.com; a...@rev.ng; a...@rev.ng;
> quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com;
> alex.ben...@linaro.org; quic_mbur...@quicinc.com;
> sidn...@quicinc.com
> Subject: [PATCH 39/39] target/hexagon: Add pcycle setting functionality
> 
> Signed-off-by: Brian Cain <brian.c...@oss.qualcomm.com>
> Signed-off-by: Matheus Tavares Bernardino <quic_mathb...@quicinc.com>
> ---
>  target/hexagon/cpu.c        | 10 +++++++---
>  target/hexagon/cpu_helper.c | 17 ++++++++++++++---
>  2 files changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/target/hexagon/cpu.c b/target/hexagon/cpu.c index
> 80f5e23794..4ca6add834 100644
> --- a/target/hexagon/cpu.c
> +++ b/target/hexagon/cpu.c
> @@ -440,19 +440,23 @@ static void hexagon_cpu_realize(DeviceState *dev,
> Error **errp)  #endif
> 
>      qemu_init_vcpu(cs);
> -#ifndef CONFIG_USER_ONLY
>      CPUHexagonState *env = cpu_env(cs);
> +#ifndef CONFIG_USER_ONLY
>      hex_mmu_realize(env);
>      if (cs->cpu_index == 0) {
>          env->g_sreg = g_new0(target_ulong, NUM_SREGS);
> -        env->g_pcycle_base = g_malloc0(sizeof(*env->g_pcycle_base));
>      } else {
>          CPUState *cpu0 = qemu_get_cpu(0);
>          CPUHexagonState *env0 = cpu_env(cpu0);
>          env->g_sreg = env0->g_sreg;
> -        env->g_pcycle_base = env0->g_pcycle_base;
>      }
>  #endif
> +    if (cs->cpu_index == 0) {
> +        env->g_pcycle_base = g_malloc0(sizeof(*env->g_pcycle_base));

Another shared resource ...

> +    } else {
> +        CPUState *cpu0 = qemu_get_cpu(0);
> +        env->g_pcycle_base = cpu_env(cpu0)->g_pcycle_base;
> +    }
> 
>      mcc->parent_realize(dev, errp);
>  }
> diff --git a/target/hexagon/cpu_helper.c b/target/hexagon/cpu_helper.c
> index 9c44cb7950..08c749e9fa 100644
> --- a/target/hexagon/cpu_helper.c
> +++ b/target/hexagon/cpu_helper.c
> @@ -70,18 +70,29 @@ uint32_t
> hexagon_get_sys_pcycle_count_low(CPUHexagonState *env)  void
> hexagon_set_sys_pcycle_count_high(CPUHexagonState *env,
>          uint32_t cycles_hi)
>  {
> -    g_assert_not_reached();
> +    uint64_t cur_cycles = hexagon_get_sys_pcycle_count(env);
> +    uint64_t cycles =
> +        ((uint64_t)cycles_hi << 32) | extract64(cur_cycles, 0, 32);
> +    hexagon_set_sys_pcycle_count(env, cycles);
>  }
> 
>  void hexagon_set_sys_pcycle_count_low(CPUHexagonState *env,
>          uint32_t cycles_lo)
>  {
> -    g_assert_not_reached();
> +    uint64_t cur_cycles = hexagon_get_sys_pcycle_count(env);
> +    uint64_t cycles = extract64(cur_cycles, 32, 32) | cycles_lo;
> +    hexagon_set_sys_pcycle_count(env, cycles);
>  }
> 
>  void hexagon_set_sys_pcycle_count(CPUHexagonState *env, uint64_t
> cycles)  {
> -    g_assert_not_reached();

Do we need a lock here?

> +    *(env->g_pcycle_base) = cycles;
> +
> +    CPUState *cs;
> +    CPU_FOREACH(cs) {
> +        CPUHexagonState *env_ = cpu_env(cs);

This underscore is easy to miss.  Just
    cpu_env(cs)->t_cycle_count = 0;