Re: [PATCH] target/ppc: Update gdbstub to read SPR's CFAR, DEC, HDEC, TB-L/U

["Nicholas Piggin" <npiggin@gmail.com>]

On Mon Sep 18, 2023 at 7:26 PM AEST, Saif Abrar wrote:
> SPR's CFAR, DEC, HDEC, TB-L/U are not implemented as part of CPUPPCState.
> Hence, gdbstub is not able to access them using (CPUPPCState *)env->spr[] array.
> Update gdb_get_spr_reg() method to handle these SPR's specifically.
>
> Signed-off-by: Saif Abrar <saif.abrar@linux.vnet.ibm.com>

Thanks for this, I have been annoyed by missing those regs when using
gdb at times :)


> ---
>  target/ppc/gdbstub.c | 40 ++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 38 insertions(+), 2 deletions(-)
>
> diff --git a/target/ppc/gdbstub.c b/target/ppc/gdbstub.c
> index 2ad11510bf..eb086c0168 100644
> --- a/target/ppc/gdbstub.c
> +++ b/target/ppc/gdbstub.c
> @@ -412,7 +412,32 @@ static int gdb_get_spr_reg(CPUPPCState *env, GByteArray *buf, int n)
>      }
>  
>      len = TARGET_LONG_SIZE;
> -    gdb_get_regl(buf, env->spr[reg]);
> +
> +    /* Handle those SPRs that are not part of the env->spr[] array */
> +    target_ulong val;

Could you move this to the of the block with other declarations?

> +    switch (reg) {
> +#if defined(TARGET_PPC64)
> +    case SPR_CFAR:
> +        val = env->cfar;
> +        break;
> +#endif
> +    case SPR_HDEC:
> +        val = cpu_ppc_load_hdecr(env);
> +        break;
> +    case SPR_TBL:
> +        val = cpu_ppc_load_tbl(env);
> +        break;
> +    case SPR_TBU:
> +        val = cpu_ppc_load_tbu(env);
> +        break;
> +    case SPR_DECR:
> +        val = cpu_ppc_load_decr(env);
> +        break;
> +    default:
> +        val = env->spr[reg];
> +    }
> +    gdb_get_regl(buf, val);
> +
>      ppc_maybe_bswap_register(env, gdb_get_reg_ptr(buf, len), len);
>      return len;
>  }
> @@ -429,7 +454,18 @@ static int gdb_set_spr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
>  
>      len = TARGET_LONG_SIZE;
>      ppc_maybe_bswap_register(env, mem_buf, len);
> -    env->spr[reg] = ldn_p(mem_buf, len);
> +
> +    /* Handle those SPRs that are not part of the env->spr[] array */
> +    target_ulong val = ldn_p(mem_buf, len);
> +    switch (reg) {
> +#if defined(TARGET_PPC64)
> +    case SPR_CFAR:
> +        env->cfar = val;
> +        break;
> +#endif

I suppose we could store some time regs here too. (h)decr I have found
useful to change at times to interrupts when debugging Linux timer code.
TB could be similar.

We have a bit of weirdness with our timebase registers though, I'm going
to send out some patches for them. Maybe hold off changing this until
we agree on those.

I'll take this for next release, sorry didn't get to it earlier.

Thanks,
Nick

> +    default:
> +        env->spr[reg] = val;
> +    }
>  
>      return len;
>  }