Re: [PATCH V2 3/4] target/microblaze: gdb: Fix incorrect SReg reporting

[Edgar E. Iglesias]

On Wed, May 13, 2020 at 11:08:47AM -0700, Joe Komlodi wrote:
> SRegs used to be reported to GDB by iterating over the SRegs array,
> however we do not store them in an order that allows them to be
> reported to GDB in that way.
> 
> To fix this, a simple map is used to map the register GDB wants to its
> location in the SRegs array.

Reviewed-by: Edgar E. Iglesias <address@hidden>


> 
> Signed-off-by: Joe Komlodi <address@hidden>
> ---
>  target/microblaze/gdbstub.c | 59 
> +++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 49 insertions(+), 10 deletions(-)
> 
> diff --git a/target/microblaze/gdbstub.c b/target/microblaze/gdbstub.c
> index af29f00..485b717 100644
> --- a/target/microblaze/gdbstub.c
> +++ b/target/microblaze/gdbstub.c
> @@ -25,6 +25,21 @@ int mb_cpu_gdb_read_register(CPUState *cs, GByteArray 
> *mem_buf, int n)
>  {
>      MicroBlazeCPU *cpu = MICROBLAZE_CPU(cs);
>      CPUMBState *env = &cpu->env;
> +    /*
> +     * GDB expects SREGs in the following order:
> +     * PC, MSR, EAR, ESR, FSR, BTR, EDR, PID, ZPR, TLBX, TLBSX, TLBLO, TLBHI.
> +     * They aren't stored in this order, so make a map.
> +     * PID, ZPR, TLBx, TLBsx, TLBLO, and TLBHI aren't modeled, so we don't
> +     * map them to anything and return a value of 0 instead.
> +     */
> +    static const uint8_t sreg_map[6] = {
> +        SR_PC,
> +        SR_MSR,
> +        SR_EAR,
> +        SR_ESR,
> +        SR_FSR,
> +        SR_BTR
> +    };
>  
>      /*
>       * GDB expects registers to be reported in this order:
> @@ -40,15 +55,16 @@ int mb_cpu_gdb_read_register(CPUState *cs, GByteArray 
> *mem_buf, int n)
>          n -= 32;
>          switch (n) {
>          case 0 ... 5:
> -            return gdb_get_reg32(mem_buf, env->sregs[n]);
> +            return gdb_get_reg32(mem_buf, env->sregs[sreg_map[n]]);
>          /* PVR12 is intentionally skipped */
>          case 6 ... 17:
>              n -= 6;
>              return gdb_get_reg32(mem_buf, env->pvr.regs[n]);
> -        case 18 ... 24:
> -            /* Add an offset of 6 to resume where we left off with SRegs */
> -            n = n - 18 + 6;
> -            return gdb_get_reg32(mem_buf, env->sregs[n]);
> +        case 18:
> +            return gdb_get_reg32(mem_buf, env->sregs[SR_EDR]);
> +        /* Other SRegs aren't modeled, so report a value of 0 */
> +        case 19 ... 24:
> +            return gdb_get_reg32(mem_buf, 0);
>          case 25:
>              return gdb_get_reg32(mem_buf, env->slr);
>          case 26:
> @@ -66,29 +82,52 @@ int mb_cpu_gdb_write_register(CPUState *cs, uint8_t 
> *mem_buf, int n)
>      CPUMBState *env = &cpu->env;
>      uint32_t tmp;
>  
> +    /*
> +     * GDB expects SREGs in the following order:
> +     * PC, MSR, EAR, ESR, FSR, BTR, EDR, PID, ZPR, TLBX, TLBSX, TLBLO, TLBHI.
> +     * They aren't stored in this order, so make a map.
> +     * PID, ZPR, TLBx, TLBsx, TLBLO, and TLBHI aren't modeled, so we don't
> +     * map them to anything.
> +     */
> +    static const uint8_t sreg_map[6] = {
> +        SR_PC,
> +        SR_MSR,
> +        SR_EAR,
> +        SR_ESR,
> +        SR_FSR,
> +        SR_BTR
> +    };
> +
>      if (n > cc->gdb_num_core_regs) {
>          return 0;
>      }
>  
>      tmp = ldl_p(mem_buf);
>  
> +    /*
> +     * GDB expects registers to be reported in this order:
> +     * R0-R31
> +     * PC-BTR
> +     * PVR0-PVR11
> +     * EDR-TLBHI
> +     * SLR-SHR
> +     */
>      if (n < 32) {
>          env->regs[n] = tmp;
>      } else {
>          n -= 32;
>          switch (n) {
>          case 0 ... 5:
> -            env->sregs[n] = tmp;
> +            env->sregs[sreg_map[n]] = tmp;
>              break;
>          /* PVR12 is intentionally skipped */
>          case 6 ... 17:
>              n -= 6;
>              env->pvr.regs[n] = tmp;
>              break;
> -        case 18 ... 24:
> -            /* Add an offset of 6 to resume where we left off with SRegs */
> -            n = n - 18 + 6;
> -            env->sregs[n] = tmp;
> +        /* Only EDR is modeled in these indeces, so ignore the rest */
> +        case 18:
> +            env->sregs[SR_EDR] = tmp;
>              break;
>          case 25:
>              env->slr = tmp;
> -- 
> 2.7.4
>