Re: [Qemu-devel] [PATCH] target-mips: gdbstub: Clean up FPU register handling

[Leon Alrae]

On 20/11/2014 11:08, Maciej W. Rozycki wrote:
> Rewrite the FPU register access parts of `mips_cpu_gdb_read_register' 
> and `mips_cpu_gdb_write_register' for consistency between each other.
> 
> Signed-off-by: Maciej W. Rozycki <address@hidden>
> ---
> Hi,
> 
>  This is the FPU register handling cleanup previously promised.
> 
>  It was regression-tested by running the GDB test suite for the 
> mips-sde-elf target and a -EB (o32) multilib, on a 24Kf processor.  
> QEMU in the system emulation mode was used as the remote stub for GDB.  
> 
>  Apart from overall register accesses the test suite includes specific 
> tests that make manual function calls with FP arguments and/or FP 
> results and these rely on FP registers to be passed around correctly.  
> I also visually inspected test logs and verified that the values 
> reported for CP1.FIR and CP1.FCSR did not change.
> 
>  Please apply.
> 
>   Maciej
> 
> qemu-mips-gdbstub-cleanup.diff
> Index: qemu-git-trunk/target-mips/gdbstub.c
> ===================================================================
> --- qemu-git-trunk.orig/target-mips/gdbstub.c 2014-11-20 10:44:24.058944521 
> +0000
> +++ qemu-git-trunk/target-mips/gdbstub.c      2014-11-20 10:44:28.058940153 
> +0000
> @@ -29,8 +29,13 @@ int mips_cpu_gdb_read_register(CPUState 
>      if (n < 32) {
>          return gdb_get_regl(mem_buf, env->active_tc.gpr[n]);
>      }
> -    if (env->CP0_Config1 & (1 << CP0C1_FP)) {
> -        if (n >= 38 && n < 70) {
> +    if (env->CP0_Config1 & (1 << CP0C1_FP) && n >= 38 && n < 72) {
> +        switch (n) {
> +        case 70:
> +            return gdb_get_regl(mem_buf, (int32_t)env->active_fpu.fcr31);
> +        case 71:
> +            return gdb_get_regl(mem_buf, (int32_t)env->active_fpu.fcr0);
> +        default:
>              if (env->CP0_Status & (1 << CP0St_FR)) {
>                  return gdb_get_regl(mem_buf,
>                      env->active_fpu.fpr[n - 38].d);
> @@ -39,12 +44,6 @@ int mips_cpu_gdb_read_register(CPUState 
>                      env->active_fpu.fpr[n - 38].w[FP_ENDIAN_IDX]);
>              }
>          }
> -        switch (n) {
> -        case 70:
> -            return gdb_get_regl(mem_buf, (int32_t)env->active_fpu.fcr31);
> -        case 71:
> -            return gdb_get_regl(mem_buf, (int32_t)env->active_fpu.fcr0);
> -        }
>      }
>      switch (n) {
>      case 32:
> @@ -64,8 +63,10 @@ int mips_cpu_gdb_read_register(CPUState 
>          return gdb_get_regl(mem_buf, 0); /* fp */
>      case 89:
>          return gdb_get_regl(mem_buf, (int32_t)env->CP0_PRid);
> -    }
> -    if (n >= 73 && n <= 88) {
> +    default:
> +        if (n > 89) {
> +            return 0;
> +        }
>          /* 16 embedded regs.  */
>          return gdb_get_regl(mem_buf, 0);
>      }
> @@ -89,15 +90,7 @@ int mips_cpu_gdb_write_register(CPUState
>          env->active_tc.gpr[n] = tmp;
>          return sizeof(target_ulong);
>      }
> -    if (env->CP0_Config1 & (1 << CP0C1_FP)
> -            && n >= 38 && n < 72) {
> -        if (n < 70) {
> -            if (env->CP0_Status & (1 << CP0St_FR)) {
> -                env->active_fpu.fpr[n - 38].d = tmp;
> -            } else {
> -                env->active_fpu.fpr[n - 38].w[FP_ENDIAN_IDX] = tmp;
> -            }
> -        }
> +    if (env->CP0_Config1 & (1 << CP0C1_FP) && n >= 38 && n < 72) {
>          switch (n) {
>          case 70:
>              env->active_fpu.fcr31 = tmp & 0xFF83FFFF;
> @@ -107,6 +100,12 @@ int mips_cpu_gdb_write_register(CPUState
>          case 71:
>              /* FIR is read-only.  Ignore writes.  */
>              break;
> +        default:
> +            if (env->CP0_Status & (1 << CP0St_FR))
> +                env->active_fpu.fpr[n - 38].d = tmp;
> +            else
> +                env->active_fpu.fpr[n - 38].w[FP_ENDIAN_IDX] = tmp;

Braces are missing here.

> +            break;
>          }
>          return sizeof(target_ulong);
>      }
> 

Otherwise,

Reviewed-by: Leon Alrae <address@hidden>