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

[Nicholas Piggin]

Excerpts from Joel Stanley's message of May 3, 2022 5:28 pm:
> 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.

Yeah good point actually, we could probably try tighten that up a
bit more.

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

Not actually very much in skiboot, I think only some FSP crash path.

skiboot does come in with attn enabled initially though I think,
and uses that for some very early asserts before skiboot can get
errors out to console.

Thanks,
Nick

> 
> 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
>