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

[Brian Cain <brian.cain@oss.qualcomm.com>]

On 3/19/2025 1:49 PM, ltaylorsimpson@gmail.com wrote:
>
>> -----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
>> Subject: [PATCH 39/39] target/hexagon: Add pcycle setting functionality
>>
>> Signed-off-by: Brian Cain <brian.cain@oss.qualcomm.com>
>> Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@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?
I will address the lack of locking here in v3, seems like an appropriate 
change.
>> +    *(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;
>
>
>