hexagon system emu, part 2/3

[PATCH 10/39] target/hexagon: Implement arch_{s, g}et_{thread, system}_reg()

Posted by Brian Cain via 11 months, 2 weeks ago

From: Brian Cain <bcain@quicinc.com>

Signed-off-by: Brian Cain <brian.cain@oss.qualcomm.com>
---
 target/hexagon/cpu_helper.h | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/target/hexagon/cpu_helper.h b/target/hexagon/cpu_helper.h
index e8d89d8526..1cdf4f8dd0 100644
--- a/target/hexagon/cpu_helper.h
+++ b/target/hexagon/cpu_helper.h
@@ -26,20 +26,27 @@ static inline void arch_set_thread_reg(CPUHexagonState *env, uint32_t reg,
                                        uint32_t val)
 {
     g_assert(reg < TOTAL_PER_THREAD_REGS);
-    g_assert_not_reached();
+    env->gpr[reg] = val;
 }
 
 static inline uint32_t arch_get_thread_reg(CPUHexagonState *env, uint32_t reg)
 {
     g_assert(reg < TOTAL_PER_THREAD_REGS);
-    g_assert_not_reached();
+    return env->gpr[reg];
 }
 
+#ifndef CONFIG_USER_ONLY
 static inline void arch_set_system_reg(CPUHexagonState *env, uint32_t reg,
                                        uint32_t val)
 {
-    g_assert_not_reached();
+    g_assert(reg < NUM_SREGS);
+    if (reg < HEX_SREG_GLB_START) {
+        env->t_sreg[reg] = val;
+    } else {
+        env->g_sreg[reg] = val;
+    }
 }
+#endif
 
 uint32_t arch_get_system_reg(CPUHexagonState *env, uint32_t reg);
 
-- 
2.34.1

RE: [PATCH 10/39] target/hexagon: Implement arch_{s, g}et_{thread, system}_reg()

Posted by ltaylorsimpson@gmail.com 10 months, 4 weeks ago


> -----Original Message-----
> From: Brian Cain <brian.cain@oss.qualcomm.com>
> Sent: Friday, February 28, 2025 11:28 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 10/39] target/hexagon: Implement
> arch_{s,g}et_{thread,system}_reg()
> 
> From: Brian Cain <bcain@quicinc.com>
> 
> Signed-off-by: Brian Cain <brian.cain@oss.qualcomm.com>
> ---
>  target/hexagon/cpu_helper.h | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/target/hexagon/cpu_helper.h b/target/hexagon/cpu_helper.h
> index e8d89d8526..1cdf4f8dd0 100644
> --- a/target/hexagon/cpu_helper.h
> +++ b/target/hexagon/cpu_helper.h
> @@ -26,20 +26,27 @@ static inline void
> arch_set_thread_reg(CPUHexagonState *env, uint32_t reg,
>                                         uint32_t val)  {
>      g_assert(reg < TOTAL_PER_THREAD_REGS);
> -    g_assert_not_reached();
> +    env->gpr[reg] = val;

Gotta be careful here.  Not all registers can be assigned directly.  Look at gen_write_ctrl_reg in genptr.c.

>  }
> 
>  static inline uint32_t arch_get_thread_reg(CPUHexagonState *env, uint32_t
> reg)  {
>      g_assert(reg < TOTAL_PER_THREAD_REGS);
> -    g_assert_not_reached();
> +    return env->gpr[reg];

Ditto - look at gen_read_ctrl_reg in genptr.c

>  }
> 
> +#ifndef CONFIG_USER_ONLY
>  static inline void arch_set_system_reg(CPUHexagonState *env, uint32_t
> reg,
>                                         uint32_t val)  {
> -    g_assert_not_reached();
> +    g_assert(reg < NUM_SREGS);
> +    if (reg < HEX_SREG_GLB_START) {
> +        env->t_sreg[reg] = val;
> +    } else {
> +        env->g_sreg[reg] = val;
> +    }

Be careful here too.  Look at gen_log_sreg_write.

>  }
> +#endif
> 
>  uint32_t arch_get_system_reg(CPUHexagonState *env, uint32_t reg);

Honestly, consider getting rid of these.  All the uses are a specific register number, so there's no benefit of things like g_assert(reg < TOTAL_PER_THREAD_REGS) or reg < HEX_SREG_GLB_START.  Also, keeping them runs the risk of not having all the proper checks inside.

Re: [PATCH 10/39] target/hexagon: Implement arch_{s,g}et_{thread,system}_reg()

Posted by Brian Cain 5 months, 1 week ago

On 3/17/2025 2:24 PM, ltaylorsimpson@gmail.com wrote:
>
>> -----Original Message-----
>> From: Brian Cain <brian.cain@oss.qualcomm.com>
>> Sent: Friday, February 28, 2025 11:28 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 10/39] target/hexagon: Implement
>> arch_{s,g}et_{thread,system}_reg()
>>
>> From: Brian Cain <bcain@quicinc.com>
>>
>> Signed-off-by: Brian Cain <brian.cain@oss.qualcomm.com>
>> ---
>>   target/hexagon/cpu_helper.h | 13 ++++++++++---
>>   1 file changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/target/hexagon/cpu_helper.h b/target/hexagon/cpu_helper.h
>> index e8d89d8526..1cdf4f8dd0 100644
>> --- a/target/hexagon/cpu_helper.h
>> +++ b/target/hexagon/cpu_helper.h
>> @@ -26,20 +26,27 @@ static inline void
>> arch_set_thread_reg(CPUHexagonState *env, uint32_t reg,
>>                                          uint32_t val)  {
>>       g_assert(reg < TOTAL_PER_THREAD_REGS);
>> -    g_assert_not_reached();
>> +    env->gpr[reg] = val;
> Gotta be careful here.  Not all registers can be assigned directly.  Look at gen_write_ctrl_reg in genptr.c.
>
>>   }
>>
>>   static inline uint32_t arch_get_thread_reg(CPUHexagonState *env, uint32_t
>> reg)  {
>>       g_assert(reg < TOTAL_PER_THREAD_REGS);
>> -    g_assert_not_reached();
>> +    return env->gpr[reg];
> Ditto - look at gen_read_ctrl_reg in genptr.c
>
>>   }
>>
>> +#ifndef CONFIG_USER_ONLY
>>   static inline void arch_set_system_reg(CPUHexagonState *env, uint32_t
>> reg,
>>                                          uint32_t val)  {
>> -    g_assert_not_reached();
>> +    g_assert(reg < NUM_SREGS);
>> +    if (reg < HEX_SREG_GLB_START) {
>> +        env->t_sreg[reg] = val;
>> +    } else {
>> +        env->g_sreg[reg] = val;
>> +    }
> Be careful here too.  Look at gen_log_sreg_write.
>
>>   }
>> +#endif
>>
>>   uint32_t arch_get_system_reg(CPUHexagonState *env, uint32_t reg);
> Honestly, consider getting rid of these.  All the uses are a specific register number, so there's no benefit of things like g_assert(reg < TOTAL_PER_THREAD_REGS) or reg < HEX_SREG_GLB_START.  Also, keeping them runs the risk of not having all the proper checks inside.


I see your point.  We will take a look at this for v3.

>