diff mbox

[v1,1/1] mips: properly compute hflags and fcr0 on cpu reset

Message ID 1330722210-13488-1-git-send-email-meadori@codesourcery.com
State New
Headers show

Commit Message

Meador Inge March 2, 2012, 9:03 p.m. UTC 
Currently 'cpu_reset' doesn't fully compute all of the needed
HFLAGs and fails to setup fcr0 after clearing the CPU state.
This can cause instruction exceptions.  For example, using
'madd.d' on machines that should support it is kindly greeted
with:

qemu: uncaught target signal 4 (Illegal instruction) - core dumped
Illegal instruction (core dumped)

because fcr0 is bogus and MIPS_HFLAG_COP1X is not correcly set in hflags.

This is fixed by modifying 'cpu_reset' to use 'compute_hflags' and
initializing 'fcr0' from the current CPU model.

Signed-off-by: Maciej W. Rozycki <macro@codesourcery.com>
Signed-off-by: Nathan Froyd <froydnj@codesourcery.com>
Signed-off-by: Meador Inge <meadori@codesourcery.com>
---
 target-mips/cpu.h       |   49 +++++++++++++++++++++++++++++++++++++++++++++++
 target-mips/op_helper.c |   49 -----------------------------------------------
 target-mips/translate.c |   17 +++------------
 3 files changed, 53 insertions(+), 62 deletions(-)

Comments

Andreas Färber March 3, 2012, 4:45 p.m. UTC | #1 
Am 02.03.2012 22:03, schrieb Meador Inge:
> Currently 'cpu_reset' doesn't fully compute all of the needed
> HFLAGs and fails to setup fcr0 after clearing the CPU state.
> This can cause instruction exceptions.  For example, using
> 'madd.d' on machines that should support it is kindly greeted
> with:
> 
> qemu: uncaught target signal 4 (Illegal instruction) - core dumped
> Illegal instruction (core dumped)
> 
> because fcr0 is bogus and MIPS_HFLAG_COP1X is not correcly set in hflags.
> 
> This is fixed by modifying 'cpu_reset' to use 'compute_hflags' and
> initializing 'fcr0' from the current CPU model.

fcr0 issue has also been

Reported-by: Khansa Butt <khansa@kics.edu.pk>

e.g., http://patchwork.ozlabs.org/patch/133974/

Your use of compute_hflags() looks more future-proof.

> 
> Signed-off-by: Maciej W. Rozycki <macro@codesourcery.com>
> Signed-off-by: Nathan Froyd <froydnj@codesourcery.com>
> Signed-off-by: Meador Inge <meadori@codesourcery.com>
> ---
>  target-mips/cpu.h       |   49 +++++++++++++++++++++++++++++++++++++++++++++++
>  target-mips/op_helper.c |   49 -----------------------------------------------
>  target-mips/translate.c |   17 +++------------
>  3 files changed, 53 insertions(+), 62 deletions(-)
> 
> diff --git a/target-mips/cpu.h b/target-mips/cpu.h
> index 71cb4e8..fc65348 100644
> --- a/target-mips/cpu.h
> +++ b/target-mips/cpu.h
> @@ -737,4 +737,53 @@ static inline void cpu_pc_from_tb(CPUState *env, TranslationBlock *tb)
>      env->hflags |= tb->flags & MIPS_HFLAG_BMASK;
>  }
>  
> +static inline void compute_hflags(CPUState *env)
> +{

Moving helper functions like these to cpu.h has proven troublesome for
QOM'ification (when they need access to MIPSCPU[Class] in addition to
CPUMIPSState) but it'll do for now.

Reviewed-by: Andreas Färber <afaerber@suse.de>

Andreas
Meador Inge March 3, 2012, 5:26 p.m. UTC | #2 
On 03/03/2012 10:45 AM, Andreas Färber wrote:

> Am 02.03.2012 22:03, schrieb Meador Inge:
>> Currently 'cpu_reset' doesn't fully compute all of the needed
>> HFLAGs and fails to setup fcr0 after clearing the CPU state.
>> This can cause instruction exceptions.  For example, using
>> 'madd.d' on machines that should support it is kindly greeted
>> with:
>>
>> qemu: uncaught target signal 4 (Illegal instruction) - core dumped
>> Illegal instruction (core dumped)
>>
>> because fcr0 is bogus and MIPS_HFLAG_COP1X is not correcly set in hflags.
>>
>> This is fixed by modifying 'cpu_reset' to use 'compute_hflags' and
>> initializing 'fcr0' from the current CPU model.
> 
> fcr0 issue has also been
> 
> Reported-by: Khansa Butt <khansa@kics.edu.pk>
> 
> e.g., http://patchwork.ozlabs.org/patch/133974/

Ah, thanks.  The fcr0 fix had been sitting in our local tree for a while and I
just forgot to check upstream patch submissions.  I need to get in the habit of
looking at patchwork first.

> Your use of compute_hflags() looks more future-proof.
> 
>>
>> Signed-off-by: Maciej W. Rozycki <macro@codesourcery.com>
>> Signed-off-by: Nathan Froyd <froydnj@codesourcery.com>
>> Signed-off-by: Meador Inge <meadori@codesourcery.com>
>> ---
>>  target-mips/cpu.h       |   49 +++++++++++++++++++++++++++++++++++++++++++++++
>>  target-mips/op_helper.c |   49 -----------------------------------------------
>>  target-mips/translate.c |   17 +++------------
>>  3 files changed, 53 insertions(+), 62 deletions(-)
>>
>> diff --git a/target-mips/cpu.h b/target-mips/cpu.h
>> index 71cb4e8..fc65348 100644
>> --- a/target-mips/cpu.h
>> +++ b/target-mips/cpu.h
>> @@ -737,4 +737,53 @@ static inline void cpu_pc_from_tb(CPUState *env, TranslationBlock *tb)
>>      env->hflags |= tb->flags & MIPS_HFLAG_BMASK;
>>  }
>>  
>> +static inline void compute_hflags(CPUState *env)
>> +{
> 
> Moving helper functions like these to cpu.h has proven troublesome for
> QOM'ification (when they need access to MIPSCPU[Class] in addition to
> CPUMIPSState) but it'll do for now.

Okay, I will keep that in mind for the future.  Thanks for the review.

> Reviewed-by: Andreas Färber <afaerber@suse.de>
> 
> Andreas
>
diff mbox

Patch

diff --git a/target-mips/cpu.h b/target-mips/cpu.h
index 71cb4e8..fc65348 100644
--- a/target-mips/cpu.h
+++ b/target-mips/cpu.h
@@ -737,4 +737,53 @@  static inline void cpu_pc_from_tb(CPUState *env, TranslationBlock *tb)
     env->hflags |= tb->flags & MIPS_HFLAG_BMASK;
 }
 
+static inline void compute_hflags(CPUState *env)
+{
+    env->hflags &= ~(MIPS_HFLAG_COP1X | MIPS_HFLAG_64 | MIPS_HFLAG_CP0 |
+                     MIPS_HFLAG_F64 | MIPS_HFLAG_FPU | MIPS_HFLAG_KSU |
+                     MIPS_HFLAG_UX);
+    if (!(env->CP0_Status & (1 << CP0St_EXL)) &&
+        !(env->CP0_Status & (1 << CP0St_ERL)) &&
+        !(env->hflags & MIPS_HFLAG_DM)) {
+        env->hflags |= (env->CP0_Status >> CP0St_KSU) & MIPS_HFLAG_KSU;
+    }
+#if defined(TARGET_MIPS64)
+    if (((env->hflags & MIPS_HFLAG_KSU) != MIPS_HFLAG_UM) ||
+        (env->CP0_Status & (1 << CP0St_PX)) ||
+        (env->CP0_Status & (1 << CP0St_UX))) {
+        env->hflags |= MIPS_HFLAG_64;
+    }
+    if (env->CP0_Status & (1 << CP0St_UX)) {
+        env->hflags |= MIPS_HFLAG_UX;
+    }
+#endif
+    if ((env->CP0_Status & (1 << CP0St_CU0)) ||
+        !(env->hflags & MIPS_HFLAG_KSU)) {
+        env->hflags |= MIPS_HFLAG_CP0;
+    }
+    if (env->CP0_Status & (1 << CP0St_CU1)) {
+        env->hflags |= MIPS_HFLAG_FPU;
+    }
+    if (env->CP0_Status & (1 << CP0St_FR)) {
+        env->hflags |= MIPS_HFLAG_F64;
+    }
+    if (env->insn_flags & ISA_MIPS32R2) {
+        if (env->active_fpu.fcr0 & (1 << FCR0_F64)) {
+            env->hflags |= MIPS_HFLAG_COP1X;
+        }
+    } else if (env->insn_flags & ISA_MIPS32) {
+        if (env->hflags & MIPS_HFLAG_64) {
+            env->hflags |= MIPS_HFLAG_COP1X;
+        }
+    } else if (env->insn_flags & ISA_MIPS4) {
+        /* All supported MIPS IV CPUs use the XX (CU3) to enable
+           and disable the MIPS IV extensions to the MIPS III ISA.
+           Some other MIPS IV CPUs ignore the bit, so the check here
+           would be too restrictive for them.  */
+        if (env->CP0_Status & (1 << CP0St_CU3)) {
+            env->hflags |= MIPS_HFLAG_COP1X;
+        }
+    }
+}
+
 #endif /* !defined (__MIPS_CPU_H__) */
diff --git a/target-mips/op_helper.c b/target-mips/op_helper.c
index c51b9cb..1f1c736 100644
--- a/target-mips/op_helper.c
+++ b/target-mips/op_helper.c
@@ -32,55 +32,6 @@ 
 static inline void cpu_mips_tlb_flush (CPUState *env, int flush_global);
 #endif
 
-static inline void compute_hflags(CPUState *env)
-{
-    env->hflags &= ~(MIPS_HFLAG_COP1X | MIPS_HFLAG_64 | MIPS_HFLAG_CP0 |
-                     MIPS_HFLAG_F64 | MIPS_HFLAG_FPU | MIPS_HFLAG_KSU |
-                     MIPS_HFLAG_UX);
-    if (!(env->CP0_Status & (1 << CP0St_EXL)) &&
-        !(env->CP0_Status & (1 << CP0St_ERL)) &&
-        !(env->hflags & MIPS_HFLAG_DM)) {
-        env->hflags |= (env->CP0_Status >> CP0St_KSU) & MIPS_HFLAG_KSU;
-    }
-#if defined(TARGET_MIPS64)
-    if (((env->hflags & MIPS_HFLAG_KSU) != MIPS_HFLAG_UM) ||
-        (env->CP0_Status & (1 << CP0St_PX)) ||
-        (env->CP0_Status & (1 << CP0St_UX))) {
-        env->hflags |= MIPS_HFLAG_64;
-    }
-    if (env->CP0_Status & (1 << CP0St_UX)) {
-        env->hflags |= MIPS_HFLAG_UX;
-    }
-#endif
-    if ((env->CP0_Status & (1 << CP0St_CU0)) ||
-        !(env->hflags & MIPS_HFLAG_KSU)) {
-        env->hflags |= MIPS_HFLAG_CP0;
-    }
-    if (env->CP0_Status & (1 << CP0St_CU1)) {
-        env->hflags |= MIPS_HFLAG_FPU;
-    }
-    if (env->CP0_Status & (1 << CP0St_FR)) {
-        env->hflags |= MIPS_HFLAG_F64;
-    }
-    if (env->insn_flags & ISA_MIPS32R2) {
-        if (env->active_fpu.fcr0 & (1 << FCR0_F64)) {
-            env->hflags |= MIPS_HFLAG_COP1X;
-        }
-    } else if (env->insn_flags & ISA_MIPS32) {
-        if (env->hflags & MIPS_HFLAG_64) {
-            env->hflags |= MIPS_HFLAG_COP1X;
-        }
-    } else if (env->insn_flags & ISA_MIPS4) {
-        /* All supported MIPS IV CPUs use the XX (CU3) to enable
-           and disable the MIPS IV extensions to the MIPS III ISA.
-           Some other MIPS IV CPUs ignore the bit, so the check here
-           would be too restrictive for them.  */
-        if (env->CP0_Status & (1 << CP0St_CU3)) {
-            env->hflags |= MIPS_HFLAG_COP1X;
-        }
-    }
-}
-
 /*****************************************************************************/
 /* Exceptions processing helpers */
 
diff --git a/target-mips/translate.c b/target-mips/translate.c
index d5b1c76..cace3a3 100644
--- a/target-mips/translate.c
+++ b/target-mips/translate.c
@@ -12769,20 +12769,16 @@  void cpu_reset (CPUMIPSState *env)
     env->CP0_SRSConf3 = env->cpu_model->CP0_SRSConf3;
     env->CP0_SRSConf4_rw_bitmask = env->cpu_model->CP0_SRSConf4_rw_bitmask;
     env->CP0_SRSConf4 = env->cpu_model->CP0_SRSConf4;
+    env->active_fpu.fcr0 = env->cpu_model->CP1_fcr0;
     env->insn_flags = env->cpu_model->insn_flags;
 
 #if defined(CONFIG_USER_ONLY)
-    env->hflags = MIPS_HFLAG_UM;
+    env->CP0_Status = (MIPS_HFLAG_UM << CP0St_KSU);
     /* Enable access to the SYNCI_Step register.  */
     env->CP0_HWREna |= (1 << 1);
     if (env->CP0_Config1 & (1 << CP0C1_FP)) {
-        env->hflags |= MIPS_HFLAG_FPU;
+        env->CP0_Status |= (1 << CP0St_CU1);
     }
-#ifdef TARGET_MIPS64
-    if (env->active_fpu.fcr0 & (1 << FCR0_F64)) {
-        env->hflags |= MIPS_HFLAG_F64;
-    }
-#endif
 #else
     if (env->hflags & MIPS_HFLAG_BMASK) {
         /* If the exception was raised from a delay slot,
@@ -12812,7 +12808,6 @@  void cpu_reset (CPUMIPSState *env)
     }
     /* Count register increments in debug mode, EJTAG version 1 */
     env->CP0_Debug = (1 << CP0DB_CNT) | (0x1 << CP0DB_VER);
-    env->hflags = MIPS_HFLAG_CP0;
 
     if (env->CP0_Config3 & (1 << CP0C3_MT)) {
         int i;
@@ -12840,11 +12835,7 @@  void cpu_reset (CPUMIPSState *env)
         }
     }
 #endif
-#if defined(TARGET_MIPS64)
-    if (env->cpu_model->insn_flags & ISA_MIPS3) {
-        env->hflags |= MIPS_HFLAG_64;
-    }
-#endif
+    compute_hflags(env);
     env->exception_index = EXCP_NONE;
 }