Re: [Qemu-devel] [PATCH v2] target-i386: update fp status fix

[Paolo Bonzini <pbonzini@redhat.com>]

Il 17/09/2014 10:05, Pavel Dovgalyuk ha scritto:
> This patch introduces cpu_set_fpuc() function, which changes fpuc field
> of the CPU state and calls update_fp_status() function.
> These calls update status of softfloat library and prevent bugs caused
> by non-coherent rounding settings of the FPU and softfloat.
> 
> v2 changes:
>  * Added missed calls and intoduced setter function (as suggested by TeLeMan)
> 
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> ---
>  target-i386/cpu.c        |    2 +-
>  target-i386/cpu.h        |    2 ++
>  target-i386/fpu_helper.c |   21 +++++++++++++--------
>  target-i386/gdbstub.c    |    2 +-
>  target-i386/machine.c    |    2 +-
>  5 files changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 217500c..9f48b1c 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -2572,7 +2572,7 @@ static void x86_cpu_reset(CPUState *s)
>      for (i = 0; i < 8; i++) {
>          env->fptags[i] = 1;
>      }
> -    env->fpuc = 0x37f;
> +    cpu_set_fpuc(env, 0x37f);
>  
>      env->mxcsr = 0x1f80;
>      env->xstate_bv = XSTATE_FP | XSTATE_SSE;
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index e634d83..6ab8eca 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -1249,6 +1249,7 @@ void QEMU_NORETURN raise_interrupt(CPUX86State *nenv, int intno, int is_int,
>  /* cc_helper.c */
>  extern const uint8_t parity_table[256];
>  uint32_t cpu_cc_compute_all(CPUX86State *env1, int op);
> +void update_fp_status(CPUX86State *env);
>  
>  static inline uint32_t cpu_compute_eflags(CPUX86State *env)
>  {
> @@ -1284,6 +1285,7 @@ static inline void cpu_load_efer(CPUX86State *env, uint64_t val)
>  
>  /* fpu_helper.c */
>  void cpu_set_mxcsr(CPUX86State *env, uint32_t val);
> +void cpu_set_fpuc(CPUX86State *env, uint16_t val);
>  
>  /* svm_helper.c */
>  void cpu_svm_check_intercept_param(CPUX86State *env1, uint32_t type,
> diff --git a/target-i386/fpu_helper.c b/target-i386/fpu_helper.c
> index 1b2900d..1d4eee3 100644
> --- a/target-i386/fpu_helper.c
> +++ b/target-i386/fpu_helper.c
> @@ -537,7 +537,7 @@ uint32_t helper_fnstcw(CPUX86State *env)
>      return env->fpuc;
>  }
>  
> -static void update_fp_status(CPUX86State *env)
> +void update_fp_status(CPUX86State *env)
>  {
>      int rnd_type;
>  
> @@ -575,8 +575,7 @@ static void update_fp_status(CPUX86State *env)
>  
>  void helper_fldcw(CPUX86State *env, uint32_t val)
>  {
> -    env->fpuc = val;
> -    update_fp_status(env);
> +    cpu_set_fpuc(env, val);
>  }
>  
>  void helper_fclex(CPUX86State *env)
> @@ -595,7 +594,7 @@ void helper_fninit(CPUX86State *env)
>  {
>      env->fpus = 0;
>      env->fpstt = 0;
> -    env->fpuc = 0x37f;
> +    cpu_set_fpuc(env, 0x37f);
>      env->fptags[0] = 1;
>      env->fptags[1] = 1;
>      env->fptags[2] = 1;
> @@ -1013,11 +1012,11 @@ void helper_fldenv(CPUX86State *env, target_ulong ptr, int data32)
>      int i, fpus, fptag;
>  
>      if (data32) {
> -        env->fpuc = cpu_lduw_data(env, ptr);
> +        cpu_set_fpuc(env, cpu_lduw_data(env, ptr));
>          fpus = cpu_lduw_data(env, ptr + 4);
>          fptag = cpu_lduw_data(env, ptr + 8);
>      } else {
> -        env->fpuc = cpu_lduw_data(env, ptr);
> +        cpu_set_fpuc(env, cpu_lduw_data(env, ptr));
>          fpus = cpu_lduw_data(env, ptr + 2);
>          fptag = cpu_lduw_data(env, ptr + 4);
>      }
> @@ -1046,7 +1045,7 @@ void helper_fsave(CPUX86State *env, target_ulong ptr, int data32)
>      /* fninit */
>      env->fpus = 0;
>      env->fpstt = 0;
> -    env->fpuc = 0x37f;
> +    cpu_set_fpuc(env, 0x37f);
>      env->fptags[0] = 1;
>      env->fptags[1] = 1;
>      env->fptags[2] = 1;
> @@ -1157,7 +1156,7 @@ void helper_fxrstor(CPUX86State *env, target_ulong ptr, int data64)
>          raise_exception(env, EXCP0D_GPF);
>      }
>  
> -    env->fpuc = cpu_lduw_data(env, ptr);
> +    cpu_set_fpuc(env, cpu_lduw_data(env, ptr));
>      fpus = cpu_lduw_data(env, ptr + 2);
>      fptag = cpu_lduw_data(env, ptr + 4);
>      env->fpstt = (fpus >> 11) & 7;
> @@ -1257,6 +1256,12 @@ void cpu_set_mxcsr(CPUX86State *env, uint32_t mxcsr)
>      set_flush_to_zero((mxcsr & SSE_FZ) ? 1 : 0, &env->fp_status);
>  }
>  
> +void cpu_set_fpuc(CPUX86State *env, uint16_t val)
> +{
> +    env->fpuc = val;
> +    update_fp_status(env);
> +}
> +
>  void helper_ldmxcsr(CPUX86State *env, uint32_t val)
>  {
>      cpu_set_mxcsr(env, val);
> diff --git a/target-i386/gdbstub.c b/target-i386/gdbstub.c
> index 19fe9ad..ff99cfb 100644
> --- a/target-i386/gdbstub.c
> +++ b/target-i386/gdbstub.c
> @@ -203,7 +203,7 @@ int x86_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
>              return x86_cpu_gdb_load_seg(cpu, R_GS, mem_buf);
>  
>          case IDX_FP_REGS + 8:
> -            env->fpuc = ldl_p(mem_buf);
> +            cpu_set_fpuc(env, ldl_p(mem_buf));
>              return 4;
>          case IDX_FP_REGS + 9:
>              tmp = ldl_p(mem_buf);
> diff --git a/target-i386/machine.c b/target-i386/machine.c
> index 16d2f6a..2922c97 100644
> --- a/target-i386/machine.c
> +++ b/target-i386/machine.c
> @@ -315,13 +315,13 @@ static int cpu_post_load(void *opaque, int version_id)
>      env->hflags &= ~HF_CPL_MASK;
>      env->hflags |= (env->segs[R_SS].flags >> DESC_DPL_SHIFT) & HF_CPL_MASK;
>  
> -    /* XXX: restore FPU round state */
>      env->fpstt = (env->fpus_vmstate >> 11) & 7;
>      env->fpus = env->fpus_vmstate & ~0x3800;
>      env->fptag_vmstate ^= 0xff;
>      for(i = 0; i < 8; i++) {
>          env->fptags[i] = (env->fptag_vmstate >> i) & 1;
>      }
> +    update_fp_status(env);
>  
>      cpu_breakpoint_remove_all(cs, BP_CPU);
>      cpu_watchpoint_remove_all(cs, BP_CPU);
> 

Looks good.  Richard, are you going to pick it up or shall I?

Paolo