target-mips: gdbstub: Clean up FPU register handling

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 <macro@codesourcery.com>
---
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

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 <macro@codesourcery.com>
> ---
> 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 <leon.alrae@imgtec.com>

On Fri, 5 Dec 2014, Leon Alrae wrote:

> > 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
[...]
> > @@ -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.

 Fixed in v2, thanks for your review.

  Maciej