[11/14] libpdbg: Remove enable_attn target command

enable_attn is only used by gdbserver and it can be implemented with
existing target register access. Move it to gdbserver.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 libpdbg/hwunit.h |  1 -
 libpdbg/p8chip.c | 31 -------------------------------
 src/pdbgproxy.c  | 38 +++++++++++++++++++++++++++++++++++---
 3 files changed, 35 insertions(+), 35 deletions(-)

On Mon, 14 Mar 2022 at 04:18, Nicholas Piggin <npiggin@gmail.com> wrote:
>
> enable_attn is only used by gdbserver and it can be implemented with
> existing target register access. Move it to gdbserver.

The downside of this is it moves chip-specific code into the proxy.
Would it make sense to keep all hardware specific code in libpdbg?



>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  libpdbg/hwunit.h |  1 -
>  libpdbg/p8chip.c | 31 -------------------------------
>  src/pdbgproxy.c  | 38 +++++++++++++++++++++++++++++++++++---
>  3 files changed, 35 insertions(+), 35 deletions(-)
>
> diff --git a/libpdbg/hwunit.h b/libpdbg/hwunit.h
> index 4662aec..2ce41db 100644
> --- a/libpdbg/hwunit.h
> +++ b/libpdbg/hwunit.h
> @@ -155,7 +155,6 @@ struct thread {
>         int (*ram_setup)(struct thread *);
>         int (*ram_instruction)(struct thread *, uint64_t opcode, uint64_t *scratch);
>         int (*ram_destroy)(struct thread *);
> -       int (*enable_attn)(struct thread *);
>
>         int (*getmem)(struct thread *, uint64_t, uint64_t *);
>         int (*getregs)(struct thread *, struct thread_regs *regs);
> diff --git a/libpdbg/p8chip.c b/libpdbg/p8chip.c
> index 92e18cc..5b2a90a 100644
> --- a/libpdbg/p8chip.c
> +++ b/libpdbg/p8chip.c
> @@ -611,36 +611,6 @@ static int p8_get_hid0(struct pdbg_target *chip, uint64_t *value)
>         return 0;
>  }
>
> -static int p8_put_hid0(struct pdbg_target *chip, uint64_t value)
> -{
> -       CHECK_ERR(pib_write(chip, HID0_REG, value));
> -       return 0;
> -}
> -
> -static int p8_enable_attn(struct thread *thread)
> -{
> -       struct pdbg_target *core;
> -       uint64_t hid0;
> -
> -       core = pdbg_target_require_parent("core", &thread->target);
> -
> -       /* Need to enable the attn instruction in HID0 */
> -       if (p8_get_hid0(core, &hid0)) {
> -               PR_ERROR("Unable to get HID0\n");
> -               return 1;
> -       }
> -       PR_INFO("HID0 was 0x%"PRIx64 " \n", hid0);
> -
> -       hid0 |= EN_ATTN;
> -
> -       PR_INFO("writing 0x%"PRIx64 " to HID0\n", hid0);
> -       if (p8_put_hid0(core, hid0)) {
> -               PR_ERROR("Unable to set HID0\n");
> -               return 1;
> -       }
> -       return 0;
> -}
> -
>  static struct thread p8_thread = {
>         .target = {
>                 .name = "POWER8 Thread",
> @@ -657,7 +627,6 @@ static struct thread p8_thread = {
>         .ram_setup = p8_ram_setup,
>         .ram_instruction = p8_ram_instruction,
>         .ram_destroy = p8_ram_destroy,
> -       .enable_attn = p8_enable_attn,
>         .getmem = ram_getmem,
>         .getregs = ram_getregs,
>         .getgpr = ram_getgpr,
> diff --git a/src/pdbgproxy.c b/src/pdbgproxy.c
> index ce52e9d..75e14b7 100644
> --- a/src/pdbgproxy.c
> +++ b/src/pdbgproxy.c
> @@ -24,6 +24,7 @@
>  #include "optcmd.h"
>  #include "debug.h"
>  #include "path.h"
> +#include "sprs.h"
>
>  #ifndef DISABLE_GDBSERVER
>
> @@ -101,6 +102,35 @@ static void detach(uint64_t *stack, void *priv)
>         send_response(fd, OK);
>  }
>
> +#define POWER8_HID_ENABLE_ATTN                 PPC_BIT(31)
> +
> +static int set_attn(bool enable)
> +{
> +       uint64_t hid;
> +
> +       if (thread_getspr(thread_target, SPR_HID, &hid))
> +               return -1;
> +
> +       if (pdbg_target_compatible(thread_target, "ibm,power8-thread")) {
> +               if (enable) {
> +                       if (hid & POWER8_HID_ENABLE_ATTN)
> +                               return 0;
> +                       hid |= POWER8_HID_ENABLE_ATTN;
> +               } else {
> +                       if (!(hid & POWER8_HID_ENABLE_ATTN))
> +                               return 0;
> +                       hid &= ~POWER8_HID_ENABLE_ATTN;
> +               }
> +       } else {
> +               return -1;
> +       }
> +
> +       if (thread_putspr(thread_target, SPR_HID, hid))
> +               return -1;
> +
> +       return 0;
> +}
> +
>  /* 32 registers represented as 16 char hex numbers with null-termination */
>  #define REG_DATA_SIZE (32*16+1)
>  static void get_gprs(uint64_t *stack, void *priv)
> @@ -254,7 +284,6 @@ static void put_mem(uint64_t *stack, void *priv)
>         uint8_t *data;
>         uint8_t attn_opcode[] = {0x00, 0x00, 0x02, 0x00};
>         int err = 0;
> -       struct thread *thread = target_to_thread(thread_target);
>
>         if (littleendian) {
>                 attn_opcode[1] = 0x02;
> @@ -285,10 +314,13 @@ static void put_mem(uint64_t *stack, void *priv)
>                 data = attn_opcode;
>
>                 /* Need to enable the attn instruction in HID0 */
> -               if (thread->enable_attn(thread))
> +               if (set_attn(true)) {
> +                       err = 2;
>                         goto out;
> -       } else
> +               }
> +       } else {
>                 stack[2] = __builtin_bswap64(stack[2]) >> 32;
> +       }
>
>         PR_INFO("put_mem 0x%016" PRIx64 " = 0x%016" PRIx64 "\n", addr, stack[2]);
>
> --
> 2.23.0
>
> --
> Pdbg mailing list
> Pdbg@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/pdbg

Excerpts from Joel Stanley's message of March 16, 2022 9:47 am:
> On Mon, 14 Mar 2022 at 04:18, Nicholas Piggin <npiggin@gmail.com> wrote:
>>
>> enable_attn is only used by gdbserver and it can be implemented with
>> existing target register access. Move it to gdbserver.
> 
> The downside of this is it moves chip-specific code into the proxy.
> Would it make sense to keep all hardware specific code in libpdbg?

Yeah, I'm not sure how best to split it up.

We almost need a middle-end for a given processor. The back-ends let
you do things like modify an SPR, but they don't necessarily know what
that SPR is for.

I tried the ->enable_attn route to start with, but then let's say
p9chip.c and p10chip.c will have copies, and then sbefifo will have
its own that will have to do differently depending on whether it's
a p9 or p10...

So I ended up just putting it in gdbserver for now.

I guess it's probably not _that_ complicated to do the right thing.
We'd have some helpers in the p*chip.c libs that do the SPR updates
via the backend accessors, and then call those. I just didn't get
that working yet.

Thanks,
Nick