| Message ID | 1583225263-26245-1-git-send-email-vincent.chen@sifive.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | riscv: Add KGDB and KDB support | expand |
On Tue, 03 Mar 2020 00:47:43 PST (-0800), vincent.chen@sifive.com wrote: > KGDB only supports parts of GDB query packets. Add > kgdb_arch_cmd_query() hook function to enable arch to handle > more query packets such as the qSupported packet. > > Signed-off-by: Vincent Chen <vincent.chen@sifive.com> > --- > include/linux/kgdb.h | 10 ++++++++++ > kernel/debug/gdbstub.c | 6 ++++++ > 2 files changed, 16 insertions(+) > > diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h > index b072aeb1fd78..bbb35557f76d 100644 > --- a/include/linux/kgdb.h > +++ b/include/linux/kgdb.h > @@ -177,6 +177,16 @@ kgdb_arch_handle_exception(int vector, int signo, int err_code, > struct pt_regs *regs); > > /** > + * kgdb_arch_handle_exception - Handle architecture specific query packet. > + * @remcom_in_buffer: The buffer of the packet we have read. > + * @remcom_out_buffer: The buffer of %BUFMAX bytes to write a packet into. > + */ > + > +extern void > +kgdb_arch_cmd_query(char *remcom_in_buffer, > + char *remcom_out_buffer); > + > +/** > * kgdb_call_nmi_hook - Call kgdb_nmicallback() on the current CPU > * @ignored: This parameter is only here to match the prototype. > * > diff --git a/kernel/debug/gdbstub.c b/kernel/debug/gdbstub.c > index 4b280fc7dd67..2b2e7b99edcc 100644 > --- a/kernel/debug/gdbstub.c > +++ b/kernel/debug/gdbstub.c > @@ -694,6 +694,9 @@ static int gdb_cmd_reboot(struct kgdb_state *ks) > return 0; > } > > +void __weak kgdb_arch_cmd_query(char *remcom_in_buffer, > + char *remcom_out_buffer); > + > /* Handle the 'q' query packets */ > static void gdb_cmd_query(struct kgdb_state *ks) > { > @@ -792,6 +795,9 @@ static void gdb_cmd_query(struct kgdb_state *ks) > } > break; > #endif > + default: > + kgdb_arch_cmd_query(remcom_in_buffer, remcom_out_buffer); > + > } > } Won't this blow up on architectures that don't implement kgdb_arch_cmd_query()? IIRC undefined weak functions have the absolute address 0. It's probably better to avoid the __weak entirely and introduce something along the lines of CONFIG_ARCH_HAS_CMD_QUERY. One better: I don't think qSupported is a good example of an arch-specific packet -- it's probably cleaner to have the arch indicate which extra features it supports (presumably via Kconfig) rather than just punting on the whole packet. The XML stuff is a good example: rather than putting all that in the RISC-V port, we could just have something like CONFIG_ACRH_SUPPORTS_GDB_XML which indicates XML descriptions are supported and expects a function to fill out the XML response.
On Thu, Mar 19, 2020 at 6:32 AM Palmer Dabbelt <palmer@dabbelt.com> wrote: > > On Tue, 03 Mar 2020 00:47:43 PST (-0800), vincent.chen@sifive.com wrote: > > KGDB only supports parts of GDB query packets. Add > > kgdb_arch_cmd_query() hook function to enable arch to handle > > more query packets such as the qSupported packet. > > > > Signed-off-by: Vincent Chen <vincent.chen@sifive.com> > > --- > > include/linux/kgdb.h | 10 ++++++++++ > > kernel/debug/gdbstub.c | 6 ++++++ > > 2 files changed, 16 insertions(+) > > > > diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h > > index b072aeb1fd78..bbb35557f76d 100644 > > --- a/include/linux/kgdb.h > > +++ b/include/linux/kgdb.h > > @@ -177,6 +177,16 @@ kgdb_arch_handle_exception(int vector, int signo, int err_code, > > struct pt_regs *regs); > > > > /** > > + * kgdb_arch_handle_exception - Handle architecture specific query packet. > > + * @remcom_in_buffer: The buffer of the packet we have read. > > + * @remcom_out_buffer: The buffer of %BUFMAX bytes to write a packet into. > > + */ > > + > > +extern void > > +kgdb_arch_cmd_query(char *remcom_in_buffer, > > + char *remcom_out_buffer); > > + > > +/** > > * kgdb_call_nmi_hook - Call kgdb_nmicallback() on the current CPU > > * @ignored: This parameter is only here to match the prototype. > > * > > diff --git a/kernel/debug/gdbstub.c b/kernel/debug/gdbstub.c > > index 4b280fc7dd67..2b2e7b99edcc 100644 > > --- a/kernel/debug/gdbstub.c > > +++ b/kernel/debug/gdbstub.c > > @@ -694,6 +694,9 @@ static int gdb_cmd_reboot(struct kgdb_state *ks) > > return 0; > > } > > > > +void __weak kgdb_arch_cmd_query(char *remcom_in_buffer, > > + char *remcom_out_buffer); > > + > > /* Handle the 'q' query packets */ > > static void gdb_cmd_query(struct kgdb_state *ks) > > { > > @@ -792,6 +795,9 @@ static void gdb_cmd_query(struct kgdb_state *ks) > > } > > break; > > #endif > > + default: > > + kgdb_arch_cmd_query(remcom_in_buffer, remcom_out_buffer); > > + > > } > > } > > Won't this blow up on architectures that don't implement kgdb_arch_cmd_query()? > IIRC undefined weak functions have the absolute address 0. It's probably Yes, you are right. I planed to define an empty week function but I forgot to add braces after the function prototype. This is my mistake. > better to avoid the __weak entirely and introduce something along the lines of > CONFIG_ARCH_HAS_CMD_QUERY. > > One better: I don't think qSupported is a good example of an arch-specific > packet -- it's probably cleaner to have the arch indicate which extra features > it supports (presumably via Kconfig) rather than just punting on the whole > packet. The XML stuff is a good example: rather than putting all that in the > RISC-V port, we could just have something like CONFIG_ACRH_SUPPORTS_GDB_XML > which indicates XML descriptions are supported and expects a function to fill > out the XML response. I agree with you. I will follow your suggestion to modify it. Thank you.
diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h index b072aeb1fd78..bbb35557f76d 100644 --- a/include/linux/kgdb.h +++ b/include/linux/kgdb.h @@ -177,6 +177,16 @@ kgdb_arch_handle_exception(int vector, int signo, int err_code, struct pt_regs *regs); /** + * kgdb_arch_handle_exception - Handle architecture specific query packet. + * @remcom_in_buffer: The buffer of the packet we have read. + * @remcom_out_buffer: The buffer of %BUFMAX bytes to write a packet into. + */ + +extern void +kgdb_arch_cmd_query(char *remcom_in_buffer, + char *remcom_out_buffer); + +/** * kgdb_call_nmi_hook - Call kgdb_nmicallback() on the current CPU * @ignored: This parameter is only here to match the prototype. * diff --git a/kernel/debug/gdbstub.c b/kernel/debug/gdbstub.c index 4b280fc7dd67..2b2e7b99edcc 100644 --- a/kernel/debug/gdbstub.c +++ b/kernel/debug/gdbstub.c @@ -694,6 +694,9 @@ static int gdb_cmd_reboot(struct kgdb_state *ks) return 0; } +void __weak kgdb_arch_cmd_query(char *remcom_in_buffer, + char *remcom_out_buffer); + /* Handle the 'q' query packets */ static void gdb_cmd_query(struct kgdb_state *ks) { @@ -792,6 +795,9 @@ static void gdb_cmd_query(struct kgdb_state *ks) } break; #endif + default: + kgdb_arch_cmd_query(remcom_in_buffer, remcom_out_buffer); + } }
KGDB only supports parts of GDB query packets. Add kgdb_arch_cmd_query() hook function to enable arch to handle more query packets such as the qSupported packet. Signed-off-by: Vincent Chen <vincent.chen@sifive.com> --- include/linux/kgdb.h | 10 ++++++++++ kernel/debug/gdbstub.c | 6 ++++++ 2 files changed, 16 insertions(+)