[Pdbg] [PATCH v2 27/39] gdbserver: track attn enablement by breakpoints

[Joel Stanley]

On Wed, 20 Apr 2022 at 06:51, Nicholas Piggin <npiggin at gmail.com> wrote:
>
> attn-enabled is a per-core property that host software (e.g.,
> skiboot) may change. gdbserver should not disable attn if it
> was found to be enabled.
>
> There are still unavoidable races where the host could change
> between gdbserver wanting it enabled and disabled.
>
> This also moves set_attn to iterate all targets in preparation
> for multi-threaded debugging.

Should this also set gdb_thread->attn_set to false when it's disabled it?

I was picturing the case where skiboot had turned it on while a long
running debugging session was happening. Perhaps that's a corner case
we don't care for.

(I don't know the circumstances that skiboot would enable attn).

Reviewed-by: Joel Stanley <joel at jms.id.au>

>
> Signed-off-by: Nicholas Piggin <npiggin at gmail.com>
> ---
>  src/pdbgproxy.c | 40 ++++++++++++++++++++++++++++++++++------
>  1 file changed, 34 insertions(+), 6 deletions(-)
>
> diff --git a/src/pdbgproxy.c b/src/pdbgproxy.c
> index b8ee2a06..d0da1f37 100644
> --- a/src/pdbgproxy.c
> +++ b/src/pdbgproxy.c
> @@ -54,6 +54,7 @@ static enum client_state state = IDLE;
>  /* Attached to thread->gdbserver_priv */
>  struct gdb_thread {
>         uint64_t pir;
> +       bool attn_set;
>  };
>
>  static void destroy_client(int dead_fd);
> @@ -116,39 +117,49 @@ static void detach(uint64_t *stack, void *priv)
>  #define POWER10_HID_ENABLE_ATTN                        PPC_BIT(3)
>  #define POWER10_HID_FLUSH_ICACHE               PPC_BIT(2)
>
> -static int set_attn(bool enable)
> +static int thread_set_attn(struct pdbg_target *target, bool enable)
>  {
> +       struct thread *thread = target_to_thread(target);
> +       struct gdb_thread *gdb_thread = thread->gdbserver_priv;
>         uint64_t hid;
>
> -       if (thread_getspr(thread_target, SPR_HID, &hid))
> +       if (!enable && !gdb_thread->attn_set) {
> +               /* Don't clear attn if we didn't enable it */
> +               return 0;
> +       }
> +
> +       if (thread_getspr(target, SPR_HID, &hid))
>                 return -1;
>
> -       if (pdbg_target_compatible(thread_target, "ibm,power8-thread")) {
> +       if (pdbg_target_compatible(target, "ibm,power8-thread")) {
>                 if (enable) {
>                         if (hid & POWER8_HID_ENABLE_ATTN)
>                                 return 0;
>                         hid |= POWER8_HID_ENABLE_ATTN;
> +                       gdb_thread->attn_set = true;
>                 } else {
>                         if (!(hid & POWER8_HID_ENABLE_ATTN))
>                                 return 0;
>                         hid &= ~POWER8_HID_ENABLE_ATTN;
>                 }
> -       } else if (pdbg_target_compatible(thread_target, "ibm,power9-thread")) {
> +       } else if (pdbg_target_compatible(target, "ibm,power9-thread")) {
>                 if (enable) {
>                         if (hid & POWER9_HID_ENABLE_ATTN)
>                                 return 0;
>                         hid |= POWER9_HID_ENABLE_ATTN;
> +                       gdb_thread->attn_set = true;
>                 } else {
>                         if (!(hid & POWER9_HID_ENABLE_ATTN))
>                                 return 0;
>                         hid &= ~POWER9_HID_ENABLE_ATTN;
>                 }
>                 hid |= POWER9_HID_FLUSH_ICACHE;
> -       } else if (pdbg_target_compatible(thread_target, "ibm,power10-thread")) {
> +       } else if (pdbg_target_compatible(target, "ibm,power10-thread")) {
>                 if (enable) {
>                         if (hid & POWER10_HID_ENABLE_ATTN)
>                                 return 0;
>                         hid |= POWER10_HID_ENABLE_ATTN;
> +                       gdb_thread->attn_set = true;
>                 } else {
>                         if (!(hid & POWER10_HID_ENABLE_ATTN))
>                                 return 0;
> @@ -159,12 +170,29 @@ static int set_attn(bool enable)
>                 return -1;
>         }
>
> -       if (thread_putspr(thread_target, SPR_HID, hid))
> +       if (thread_putspr(target, SPR_HID, hid))
>                 return -1;
>
>         return 0;
>  }
>
> +static int set_attn(bool enable)
> +{
> +       struct pdbg_target *target;
> +       int err = 0;
> +
> +       for_each_path_target_class("thread", target) {
> +               if (pdbg_target_status(target) != PDBG_TARGET_ENABLED)
> +                       continue;
> +
> +               err = thread_set_attn(target, enable);
> +               if (err)
> +                       break;
> +       }
> +
> +       return err;
> +}
> +
>  /* 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)
> --
> 2.35.1
>
> --
> Pdbg mailing list
> Pdbg at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/pdbg