| Message ID | 1383641146-27274-4-git-send-email-vijay.kilari@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Nov 05, 2013 at 08:45:45AM +0000, vijay.kilari@gmail.com wrote: > From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com> > > Add KGDB software step debugging support for EL1 debug > in AArch64 mode. > > KGDB registers step debug handler with debug monitor. > On receiving 'step' command from GDB tool, target enables > software step debugging and step address is updated. > If no step address is received from GDB tool, target assumes > next step address is current PC. > > Software Step debugging is disabled when 'continue' command > is received > > Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com> > --- > arch/arm64/kernel/kgdb.c | 67 +++++++++++++++++++++++++++++++++++++++------- > 1 file changed, 58 insertions(+), 9 deletions(-) > > diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c > index ec624bc..f10f2ba 100644 > --- a/arch/arm64/kernel/kgdb.c > +++ b/arch/arm64/kernel/kgdb.c > @@ -137,13 +137,26 @@ void kgdb_arch_set_pc(struct pt_regs *regs, unsigned long pc) > > static int compiled_break; > > +static void kgdb_arch_update_addr(struct pt_regs *regs, > + char *remcom_in_buffer) > +{ > + unsigned long addr; > + char *ptr; > + > + ptr = &remcom_in_buffer[1]; > + if (kgdb_hex2long(&ptr, &addr)) > + kgdb_arch_set_pc(regs, addr); > + else if (compiled_break == 1) > + kgdb_arch_set_pc(regs, regs->pc + 4); > + > + compiled_break = 0; > +} > + > int kgdb_arch_handle_exception(int exception_vector, int signo, > int err_code, char *remcom_in_buffer, > char *remcom_out_buffer, > struct pt_regs *linux_regs) > { > - unsigned long addr; > - char *ptr; > int err; > > switch (remcom_in_buffer[0]) { > @@ -153,6 +166,8 @@ int kgdb_arch_handle_exception(int exception_vector, int signo, > * Packet D (Detach), k (kill). No special handling > * is required here > */ > + atomic_set(&kgdb_cpu_doing_single_step, -1); > + kgdb_single_step = 0; This looks really weird: you have two variables, which you attempt to keep in sync, only one of them is an atomic access and you don't have any locking or memory barriers between them, so it must be ok if they're not in-sync. In which case, why do we have two variables? Will
On Fri, Nov 8, 2013 at 7:48 PM, Will Deacon <will.deacon@arm.com> wrote: > On Tue, Nov 05, 2013 at 08:45:45AM +0000, vijay.kilari@gmail.com wrote: >> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com> >> >> Add KGDB software step debugging support for EL1 debug >> in AArch64 mode. >> >> KGDB registers step debug handler with debug monitor. >> On receiving 'step' command from GDB tool, target enables >> software step debugging and step address is updated. >> If no step address is received from GDB tool, target assumes >> next step address is current PC. >> >> Software Step debugging is disabled when 'continue' command >> is received >> >> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com> >> --- >> arch/arm64/kernel/kgdb.c | 67 +++++++++++++++++++++++++++++++++++++++------- >> 1 file changed, 58 insertions(+), 9 deletions(-) >> >> diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c >> index ec624bc..f10f2ba 100644 >> --- a/arch/arm64/kernel/kgdb.c >> +++ b/arch/arm64/kernel/kgdb.c >> @@ -137,13 +137,26 @@ void kgdb_arch_set_pc(struct pt_regs *regs, unsigned long pc) >> >> static int compiled_break; >> >> +static void kgdb_arch_update_addr(struct pt_regs *regs, >> + char *remcom_in_buffer) >> +{ >> + unsigned long addr; >> + char *ptr; >> + >> + ptr = &remcom_in_buffer[1]; >> + if (kgdb_hex2long(&ptr, &addr)) >> + kgdb_arch_set_pc(regs, addr); >> + else if (compiled_break == 1) >> + kgdb_arch_set_pc(regs, regs->pc + 4); >> + >> + compiled_break = 0; >> +} >> + >> int kgdb_arch_handle_exception(int exception_vector, int signo, >> int err_code, char *remcom_in_buffer, >> char *remcom_out_buffer, >> struct pt_regs *linux_regs) >> { >> - unsigned long addr; >> - char *ptr; >> int err; >> >> switch (remcom_in_buffer[0]) { >> @@ -153,6 +166,8 @@ int kgdb_arch_handle_exception(int exception_vector, int signo, >> * Packet D (Detach), k (kill). No special handling >> * is required here >> */ >> + atomic_set(&kgdb_cpu_doing_single_step, -1); >> + kgdb_single_step = 0; > > This looks really weird: you have two variables, which you attempt to keep > in sync, only one of them is an atomic access and you don't have any > locking or memory barriers between them, so it must be ok if they're not > in-sync. In which case, why do we have two variables? > IMHO, two variables are being used by kgdb framework The variables 'kgdb_cpu_doing_single_step' holds the cpu on which step debugging is happening. With this, when during step debugging, it ensures that on next step debug exception only this CPU aquires master lock and step debugging is performed. The variable 'kgdb_single_step' is used to know if step debugging is ongoing or not. If so, the non-primary cpu's are not released untill the continue command is received So, with this the cpu's are not released and acquired for every step command > Will
On Mon, Nov 11, 2013 at 11:09:46AM +0000, Vijay Kilari wrote: > On Fri, Nov 8, 2013 at 7:48 PM, Will Deacon <will.deacon@arm.com> wrote: > > On Tue, Nov 05, 2013 at 08:45:45AM +0000, vijay.kilari@gmail.com wrote: > >> @@ -153,6 +166,8 @@ int kgdb_arch_handle_exception(int exception_vector, int signo, > >> * Packet D (Detach), k (kill). No special handling > >> * is required here > >> */ > >> + atomic_set(&kgdb_cpu_doing_single_step, -1); > >> + kgdb_single_step = 0; > > > > This looks really weird: you have two variables, which you attempt to keep > > in sync, only one of them is an atomic access and you don't have any > > locking or memory barriers between them, so it must be ok if they're not > > in-sync. In which case, why do we have two variables? > > > IMHO, two variables are being used by kgdb framework > The variables 'kgdb_cpu_doing_single_step' holds the cpu on which > step debugging > is happening. With this, when during step debugging, it ensures that on next > step debug exception only this CPU aquires master lock and step > debugging is performed. > > The variable 'kgdb_single_step' is used to know if step debugging is > ongoing or not. > If so, the non-primary cpu's are not released untill the continue > command is received > So, with this the cpu's are not released and acquired for every step command Ok, but in what situation would you have both kgdb_cpu_doing_single_step == -1 and kgdb_single_step == 1 (ignoring races between setting the two variables)? In other words, can we reduce this to a single variable? Will
On Mon, Nov 11, 2013 at 8:30 PM, Will Deacon <will.deacon@arm.com> wrote: > On Mon, Nov 11, 2013 at 11:09:46AM +0000, Vijay Kilari wrote: >> On Fri, Nov 8, 2013 at 7:48 PM, Will Deacon <will.deacon@arm.com> wrote: >> > On Tue, Nov 05, 2013 at 08:45:45AM +0000, vijay.kilari@gmail.com wrote: >> >> @@ -153,6 +166,8 @@ int kgdb_arch_handle_exception(int exception_vector, int signo, >> >> * Packet D (Detach), k (kill). No special handling >> >> * is required here >> >> */ >> >> + atomic_set(&kgdb_cpu_doing_single_step, -1); >> >> + kgdb_single_step = 0; >> > >> > This looks really weird: you have two variables, which you attempt to keep >> > in sync, only one of them is an atomic access and you don't have any >> > locking or memory barriers between them, so it must be ok if they're not >> > in-sync. In which case, why do we have two variables? >> > >> IMHO, two variables are being used by kgdb framework >> The variables 'kgdb_cpu_doing_single_step' holds the cpu on which >> step debugging >> is happening. With this, when during step debugging, it ensures that on next >> step debug exception only this CPU aquires master lock and step >> debugging is performed. >> >> The variable 'kgdb_single_step' is used to know if step debugging is >> ongoing or not. >> If so, the non-primary cpu's are not released untill the continue >> command is received >> So, with this the cpu's are not released and acquired for every step command > > Ok, but in what situation would you have both kgdb_cpu_doing_single_step == -1 > and kgdb_single_step == 1 (ignoring races between setting the two variables)? > > In other words, can we reduce this to a single variable? > More elaborately, The kgdb_cpu_doing_single_step holds the cpu number which is used to store the task information that is currently being step debugged in the corresponding cpu structure. so we cannot get rid of this variable. Also this variable tells which cpu should take the exception for step debugging so it waits for this paricular cpu to get the chance to take exception in case if exception has been taken by other cpu. The kgdb_single_step variable, it only tells if cpu's that don't get debug exception should be released & acquired for every step debug or not So, both variables are used for different purpose. we can make an attempt to make to use one variable, but I think, the flexibility is lost. Moreover, I see that these variables are declared in generic kgdb code and different architectures use this based on their requirement. The race will not happen because, only one cpu will be executing this function at any point of the time and all other cpu's will be spinning. > Will
diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c index ec624bc..f10f2ba 100644 --- a/arch/arm64/kernel/kgdb.c +++ b/arch/arm64/kernel/kgdb.c @@ -137,13 +137,26 @@ void kgdb_arch_set_pc(struct pt_regs *regs, unsigned long pc) static int compiled_break; +static void kgdb_arch_update_addr(struct pt_regs *regs, + char *remcom_in_buffer) +{ + unsigned long addr; + char *ptr; + + ptr = &remcom_in_buffer[1]; + if (kgdb_hex2long(&ptr, &addr)) + kgdb_arch_set_pc(regs, addr); + else if (compiled_break == 1) + kgdb_arch_set_pc(regs, regs->pc + 4); + + compiled_break = 0; +} + int kgdb_arch_handle_exception(int exception_vector, int signo, int err_code, char *remcom_in_buffer, char *remcom_out_buffer, struct pt_regs *linux_regs) { - unsigned long addr; - char *ptr; int err; switch (remcom_in_buffer[0]) { @@ -153,6 +166,8 @@ int kgdb_arch_handle_exception(int exception_vector, int signo, * Packet D (Detach), k (kill). No special handling * is required here */ + atomic_set(&kgdb_cpu_doing_single_step, -1); + kgdb_single_step = 0; err = 0; break; case 'c': @@ -164,16 +179,39 @@ int kgdb_arch_handle_exception(int exception_vector, int signo, * to the next instruction else we will just breakpoint * over and over again. */ - ptr = &remcom_in_buffer[1]; - if (kgdb_hex2long(&ptr, &addr)) - kgdb_arch_set_pc(linux_regs, addr); - else if (compiled_break == 1) - kgdb_arch_set_pc(linux_regs, linux_regs->pc + 4); + kgdb_arch_update_addr(linux_regs, remcom_in_buffer); + atomic_set(&kgdb_cpu_doing_single_step, -1); + kgdb_single_step = 0; - compiled_break = 0; + /* + * Received continue command, disable single step + */ + if (kernel_active_single_step()) + kernel_disable_single_step(); err = 0; break; + case 's': + /* + * Update step address value with address passed + * with step packet. + * On debug exception return PC is copied to ELR + * So just update PC. + * If no step address is passed, resume from the address + * pointed by PC. Do not update PC + */ + kgdb_arch_update_addr(linux_regs, remcom_in_buffer); + atomic_set(&kgdb_cpu_doing_single_step, raw_smp_processor_id()); + kgdb_single_step = 1; + + /* + * Enable single step handling + */ + if (!kernel_active_single_step()) + kernel_enable_single_step(linux_regs); + err = 0; + break; + default: err = -1; } @@ -194,6 +232,12 @@ static int kgdb_compiled_brk_fn(struct pt_regs *regs, unsigned int esr) return 0; } +static int kgdb_step_brk_fn(struct pt_regs *regs, unsigned int esr) +{ + kgdb_handle_exception(1, SIGTRAP, 0, regs); + return 0; +} + static struct break_hook kgdb_brkpt_hook = { .esr_mask = 0xffffffff, .esr_val = DBG_ESR_VAL_BRK(KGDB_DYN_DGB_BRK_IMM), @@ -206,6 +250,10 @@ static struct break_hook kgdb_compiled_brkpt_hook = { .fn = kgdb_compiled_brk_fn }; +static struct step_hook kgdb_step_hook = { + .fn = kgdb_step_brk_fn +}; + static void kgdb_call_nmi_hook(void *ignored) { kgdb_nmicallback(raw_smp_processor_id(), get_irq_regs()); @@ -259,7 +307,7 @@ int kgdb_arch_init(void) register_break_hook(&kgdb_brkpt_hook); register_break_hook(&kgdb_compiled_brkpt_hook); - + register_step_hook(&kgdb_step_hook); return 0; } @@ -272,6 +320,7 @@ void kgdb_arch_exit(void) { unregister_break_hook(&kgdb_brkpt_hook); unregister_break_hook(&kgdb_compiled_brkpt_hook); + unregister_step_hook(&kgdb_step_hook); unregister_die_notifier(&kgdb_notifier); }