| Message ID | 20220420065013.222816-28-npiggin@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | gdbserver multi-threaded debugging and POWER9/10 support | expand |
On Wed, 20 Apr 2022 at 06:51, Nicholas Piggin <npiggin@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@jms.id.au> > > Signed-off-by: Nicholas Piggin <npiggin@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@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/pdbg
Excerpts from Joel Stanley's message of May 3, 2022 5:28 pm: > On Wed, 20 Apr 2022 at 06:51, Nicholas Piggin <npiggin@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@jms.id.au> > >> >> Signed-off-by: Nicholas Piggin <npiggin@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@lists.ozlabs.org >> https://lists.ozlabs.org/listinfo/pdbg >
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)
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. Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- src/pdbgproxy.c | 40 ++++++++++++++++++++++++++++++++++------ 1 file changed, 34 insertions(+), 6 deletions(-)