Re: [Qemu-devel] [PATCH] MIPS/user: Fix reset CPU state initialization

[Aurelien Jarno <aurelien@aurel32.net>]

On Fri, Jun 08, 2012 at 02:04:40AM +0100, Maciej W. Rozycki wrote:
> 
>  This change updates the CPU reset sequence to use a common piece of code 
> that figures out CPU state flags, fixing the problem with MIPS_HFLAG_COP1X 
> not being set where applicable that causes floating-point MADD family 
> instructions (and other instructions from the MIPS IV FP subset) to trap.
> 
>  As compute_hflags is now shared between op_helper.c and translate.c, the 
> function is now moved to a common header.  There are no changes to this 
> function.
> 
>  The problem was seen with the 24Kf MIPS32r2 processor in user emulation.  
> The new approach prevents system and user emulation from diverging -- all 
> the hflags state is initialized in one place now.
> 
> Signed-off-by: Maciej W. Rozycki <macro@codesourcery.com>
> ---
> 
>  This is effectively a follow-up to Nathan's FCR0 fix -- please apply.
> 
>   Maciej

Thanks, applied.

> qemu-mips-hflags.patch
> Index: qemu-git-trunk/target-mips/cpu.h
> ===================================================================
> --- qemu-git-trunk.orig/target-mips/cpu.h	2012-06-07 03:15:53.645461055 +0100
> +++ qemu-git-trunk/target-mips/cpu.h	2012-06-07 03:18:48.345427587 +0100
> @@ -753,4 +753,53 @@ static inline void cpu_pc_from_tb(CPUMIP
>      env->hflags |= tb->flags & MIPS_HFLAG_BMASK;
>  }
>  
> +static inline void compute_hflags(CPUMIPSState *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__) */
> Index: qemu-git-trunk/target-mips/op_helper.c
> ===================================================================
> --- qemu-git-trunk.orig/target-mips/op_helper.c	2012-06-07 03:15:53.645461055 +0100
> +++ qemu-git-trunk/target-mips/op_helper.c	2012-06-07 03:18:48.345427587 +0100
> @@ -32,55 +32,6 @@
>  static inline void cpu_mips_tlb_flush (CPUMIPSState *env, int flush_global);
>  #endif
>  
> -static inline void compute_hflags(CPUMIPSState *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 */
>  
> Index: qemu-git-trunk/target-mips/translate.c
> ===================================================================
> --- qemu-git-trunk.orig/target-mips/translate.c	2012-06-07 03:18:46.275645763 +0100
> +++ qemu-git-trunk/target-mips/translate.c	2012-06-07 03:18:48.345427587 +0100
> @@ -12780,17 +12780,12 @@ void cpu_state_reset(CPUMIPSState *env)
>      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;
> -    }
> -#ifdef TARGET_MIPS64
> -    if (env->active_fpu.fcr0 & (1 << FCR0_F64)) {
> -        env->hflags |= MIPS_HFLAG_F64;
> +        env->CP0_Status |= (1 << CP0St_CU1);
>      }
> -#endif
>  #else
>      if (env->hflags & MIPS_HFLAG_BMASK) {
>          /* If the exception was raised from a delay slot,
> @@ -12820,7 +12815,6 @@ void cpu_state_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;
> @@ -12848,11 +12842,7 @@ void cpu_state_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;
>  }
>  
> 
> 

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net