[Qemu-devel] [PATCH v2] target-mips: Fix RDHWR on CP0.Count

[Qemu-devel] [PATCH v2] target-mips: Fix RDHWR on CP0.Count

[Alex Smith]

For RDHWR on the CP0.Count register, env->CP0_Count was being returned.
This value is a delta against the QEMU_CLOCK_VIRTUAL clock, not the
correct current value of CP0.Count. Use cpu_mips_get_count() instead.

Signed-off-by: Alex Smith <alex.smith@imgtec.com>
Cc: Aurelien Jarno <aurelien@aurel32.net>
Cc: Leon Alrae <leon.alrae@imgtec.com>
---
Changes in v2:
 - Fix build breakage for user builds.
 - Correct existing code to follow QEMU coding style.
---
 target-mips/op_helper.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/target-mips/op_helper.c b/target-mips/op_helper.c
index 809a061e296b..99574322a29c 100644
--- a/target-mips/op_helper.c
+++ b/target-mips/op_helper.c
@@ -2184,10 +2184,15 @@ target_ulong helper_rdhwr_synci_step(CPUMIPSState *env)
 target_ulong helper_rdhwr_cc(CPUMIPSState *env)
 {
     if ((env->hflags & MIPS_HFLAG_CP0) ||
-        (env->CP0_HWREna & (1 << 2)))
+        (env->CP0_HWREna & (1 << 2))) {
+#ifdef CONFIG_USER_ONLY
         return env->CP0_Count;
-    else
+#else
+        return (int32_t)cpu_mips_get_count(env);
+#endif
+    } else {
         helper_raise_exception(env, EXCP_RI);
+    }
 
     return 0;
 }
-- 
2.5.0

Re: [Qemu-devel] [PATCH v2] target-mips: Fix RDHWR on CP0.Count

[Leon Alrae]

On 08/09/15 11:34, Alex Smith wrote:
> For RDHWR on the CP0.Count register, env->CP0_Count was being returned.
> This value is a delta against the QEMU_CLOCK_VIRTUAL clock, not the
> correct current value of CP0.Count. Use cpu_mips_get_count() instead.
> 
> Signed-off-by: Alex Smith <alex.smith@imgtec.com>
> Cc: Aurelien Jarno <aurelien@aurel32.net>
> Cc: Leon Alrae <leon.alrae@imgtec.com>
> ---
> Changes in v2:
>  - Fix build breakage for user builds.
>  - Correct existing code to follow QEMU coding style.
> ---
>  target-mips/op_helper.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)

Reviewed-by: Leon Alrae <leon.alrae@imgtec.com>

Re: [Qemu-devel] [PATCH v2] target-mips: Fix RDHWR on CP0.Count

[Aurelien Jarno]

On 2015-09-08 11:34, Alex Smith wrote:
> For RDHWR on the CP0.Count register, env->CP0_Count was being returned.
> This value is a delta against the QEMU_CLOCK_VIRTUAL clock, not the
> correct current value of CP0.Count. Use cpu_mips_get_count() instead.
> 
> Signed-off-by: Alex Smith <alex.smith@imgtec.com>
> Cc: Aurelien Jarno <aurelien@aurel32.net>
> Cc: Leon Alrae <leon.alrae@imgtec.com>
> ---
> Changes in v2:
>  - Fix build breakage for user builds.
>  - Correct existing code to follow QEMU coding style.
> ---
>  target-mips/op_helper.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/target-mips/op_helper.c b/target-mips/op_helper.c
> index 809a061e296b..99574322a29c 100644
> --- a/target-mips/op_helper.c
> +++ b/target-mips/op_helper.c
> @@ -2184,10 +2184,15 @@ target_ulong helper_rdhwr_synci_step(CPUMIPSState *env)
>  target_ulong helper_rdhwr_cc(CPUMIPSState *env)
>  {
>      if ((env->hflags & MIPS_HFLAG_CP0) ||
> -        (env->CP0_HWREna & (1 << 2)))
> +        (env->CP0_HWREna & (1 << 2))) {
> +#ifdef CONFIG_USER_ONLY
>          return env->CP0_Count;
> -    else
> +#else
> +        return (int32_t)cpu_mips_get_count(env);
> +#endif
> +    } else {
>          helper_raise_exception(env, EXCP_RI);
> +    }
>  
>      return 0;
>  }

Reviewed-by: Aurelien Jarno <aurelien@aurel32.net>

Independently of your patch, I do wonder if we shouldn't change the
return type of cpu_mips_get_count to int32_t. With your patch, there
are now 2 calls to this functions, and both cast the value to int32_t.

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH v2] target-mips: Fix RDHWR on CP0.Count

[Leon Alrae]

On 16/09/15 06:27, Aurelien Jarno wrote:
> Independently of your patch, I do wonder if we shouldn't change the
> return type of cpu_mips_get_count to int32_t. With your patch, there
> are now 2 calls to this functions, and both cast the value to int32_t.

Yes, I think that will make more sense if the return type of
cpu_mips_get_count() matches CP0_Count.

Leon