| Message ID | 1583225220-26137-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:00 PST (-0800), vincent.chen@sifive.com wrote: > The break instruction in RISC-V does not have an immediate value field, so > the kernel cannot identify the purpose of each trap exception through the > opcode. This makes the existing identification schemes in other > architecture unsuitable for the RISC-V kernel. To solve this problem, this > patch adds kgdb_has_hit_break(), which can help RISC-V kernel identify > the KGDB trap exception. > > Signed-off-by: Vincent Chen <vincent.chen@sifive.com> > --- > kernel/debug/debug_core.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c > index 2b7c9b67931d..01bc3eea3d4d 100644 > --- a/kernel/debug/debug_core.c > +++ b/kernel/debug/debug_core.c > @@ -417,6 +417,18 @@ int kgdb_isremovedbreak(unsigned long addr) > return 0; > } > > +int kgdb_has_hit_break(unsigned long addr) > +{ > + int i; > + > + for (i = 0; i < KGDB_MAX_BREAKPOINTS; i++) { > + if (kgdb_break[i].state == BP_ACTIVE && > + kgdb_break[i].bpt_addr == addr) > + return 1; > + } > + return 0; > +} > + > int dbg_remove_all_break(void) > { > int error; Reviewed-by: Palmer Dabbelt <palmerdabbelt@google.com>
On Wed, Mar 18, 2020 at 11:03:25AM -0700, Palmer Dabbelt wrote: > On Tue, 03 Mar 2020 00:47:00 PST (-0800), vincent.chen@sifive.com wrote: > > The break instruction in RISC-V does not have an immediate value field, so > > the kernel cannot identify the purpose of each trap exception through the > > opcode. This makes the existing identification schemes in other > > architecture unsuitable for the RISC-V kernel. To solve this problem, this > > patch adds kgdb_has_hit_break(), which can help RISC-V kernel identify > > the KGDB trap exception. > > > > Signed-off-by: Vincent Chen <vincent.chen@sifive.com> > > --- > > kernel/debug/debug_core.c | 12 ++++++++++++ > > 1 file changed, 12 insertions(+) > > > > diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c > > index 2b7c9b67931d..01bc3eea3d4d 100644 > > --- a/kernel/debug/debug_core.c > > +++ b/kernel/debug/debug_core.c > > @@ -417,6 +417,18 @@ int kgdb_isremovedbreak(unsigned long addr) > > return 0; > > } > > > > +int kgdb_has_hit_break(unsigned long addr) > > +{ > > + int i; > > + > > + for (i = 0; i < KGDB_MAX_BREAKPOINTS; i++) { > > + if (kgdb_break[i].state == BP_ACTIVE && > > + kgdb_break[i].bpt_addr == addr) > > + return 1; > > + } > > + return 0; > > +} > > + > > int dbg_remove_all_break(void) > > { > > int error; > > Reviewed-by: Palmer Dabbelt <palmerdabbelt@google.com> I've been slow to review this because I wanted to take a closer look at whether this issue is unique to RV or whether one of the other architectures solved it a different way so, out of interest, did you do any investigations in this direction? Daniel.
On Thu, Mar 19, 2020 at 7:40 PM Daniel Thompson <daniel.thompson@linaro.org> wrote: > > On Wed, Mar 18, 2020 at 11:03:25AM -0700, Palmer Dabbelt wrote: > > On Tue, 03 Mar 2020 00:47:00 PST (-0800), vincent.chen@sifive.com wrote: > > > The break instruction in RISC-V does not have an immediate value field, so > > > the kernel cannot identify the purpose of each trap exception through the > > > opcode. This makes the existing identification schemes in other > > > architecture unsuitable for the RISC-V kernel. To solve this problem, this > > > patch adds kgdb_has_hit_break(), which can help RISC-V kernel identify > > > the KGDB trap exception. > > > > > > Signed-off-by: Vincent Chen <vincent.chen@sifive.com> > > > --- > > > kernel/debug/debug_core.c | 12 ++++++++++++ > > > 1 file changed, 12 insertions(+) > > > > > > diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c > > > index 2b7c9b67931d..01bc3eea3d4d 100644 > > > --- a/kernel/debug/debug_core.c > > > +++ b/kernel/debug/debug_core.c > > > @@ -417,6 +417,18 @@ int kgdb_isremovedbreak(unsigned long addr) > > > return 0; > > > } > > > > > > +int kgdb_has_hit_break(unsigned long addr) > > > +{ > > > + int i; > > > + > > > + for (i = 0; i < KGDB_MAX_BREAKPOINTS; i++) { > > > + if (kgdb_break[i].state == BP_ACTIVE && > > > + kgdb_break[i].bpt_addr == addr) > > > + return 1; > > > + } > > > + return 0; > > > +} > > > + > > > int dbg_remove_all_break(void) > > > { > > > int error; > > > > Reviewed-by: Palmer Dabbelt <palmerdabbelt@google.com> > > I've been slow to review this because I wanted to take a closer > look at whether this issue is unique to RV or whether one of the > other architectures solved it a different way so, out of interest, > did you do any investigations in this direction? Hi Daniel: Yes, I tried to find references from other architecture such as ARM, ARM64, MIPS, and x86. However, I found ARM, ARM64, MIPS uses a specific trap number to distinguish the purpose. X86 does not embed a specific number into the trap instruction, but X86 uses the undefined instruction, UD, to implement BUG() or WARN(). Therefore, X86 just needs to distinguish the debug trap between debug tools. Unlike X86, RISC-V only can use "ebreak" instruction to implement the BUG() related functions and debug features. Therefore, I decided to create the kgdb_has_hit_break function for RISC-V port to distinguish the purpose of a trap. If I have any misunderstanding or you have a better idea, please feel free to let me know. Thank you
On Thu, 19 Mar 2020 04:40:32 PDT (-0700), daniel.thompson@linaro.org wrote: > On Wed, Mar 18, 2020 at 11:03:25AM -0700, Palmer Dabbelt wrote: >> On Tue, 03 Mar 2020 00:47:00 PST (-0800), vincent.chen@sifive.com wrote: >> > The break instruction in RISC-V does not have an immediate value field, so >> > the kernel cannot identify the purpose of each trap exception through the >> > opcode. This makes the existing identification schemes in other >> > architecture unsuitable for the RISC-V kernel. To solve this problem, this >> > patch adds kgdb_has_hit_break(), which can help RISC-V kernel identify >> > the KGDB trap exception. >> > >> > Signed-off-by: Vincent Chen <vincent.chen@sifive.com> >> > --- >> > kernel/debug/debug_core.c | 12 ++++++++++++ >> > 1 file changed, 12 insertions(+) >> > >> > diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c >> > index 2b7c9b67931d..01bc3eea3d4d 100644 >> > --- a/kernel/debug/debug_core.c >> > +++ b/kernel/debug/debug_core.c >> > @@ -417,6 +417,18 @@ int kgdb_isremovedbreak(unsigned long addr) >> > return 0; >> > } >> > >> > +int kgdb_has_hit_break(unsigned long addr) >> > +{ >> > + int i; >> > + >> > + for (i = 0; i < KGDB_MAX_BREAKPOINTS; i++) { >> > + if (kgdb_break[i].state == BP_ACTIVE && >> > + kgdb_break[i].bpt_addr == addr) >> > + return 1; >> > + } >> > + return 0; >> > +} >> > + >> > int dbg_remove_all_break(void) >> > { >> > int error; >> >> Reviewed-by: Palmer Dabbelt <palmerdabbelt@google.com> > > I've been slow to review this because I wanted to take a closer > look at whether this issue is unique to RV or whether one of the > other architectures solved it a different way so, out of interest, > did you do any investigations in this direction? Not specifically in the context of KGDB implementations, but we've been chasing these issues around in the rest of the debug stack for years now (I also maintain the RISC-V GDB port, though much less actively than I used to). For many projects we've solved this by adding an ABI restriction that defines a two-instruction pairing for certain types of breakpoints that looks something like addi x0, x0, MAGIC ebreak I generally prefer the approach here, as it doesn't impose an ABI restriction, but we've had to shoehorn it into so many projects I just kind of pattern matched KGDB into the same bin. Looking through "find -name "*kgdb*" | grep arch", I see that most architectures that have KGDB support also have immediates in their breakpoints. I'm just going to assume they're all differentiating between their types of breakpoints based on the immediate: * Microblaze has a "brki" instruction, presumably the immediate indicates it's a KGDB breakpoint. I think they spin on BUG(). * arm64 has a "brk" insturction with an immediate. They're using 0 for BUG(), 0x401 for KDB. * PPC uses "twge r2, r2". I can't find it in the ISA manual, but they have other traps with immediates and there seems to be more than sufficient information to differentiate between the types of traps. They use "twi 31, 0, 0" for BUG. * ARC uses "trap_s 0x4", and they appear to use different immediates for different purposes (0 for a user breakpoint). They use __builtin_trap() for BUG(), which I'm assuming is the same as the user breakpoint. * H8 uses "trapa #2", and they're using "trapa #3" for userspace breakpoints. As far as I can tell they're spinning on BUG(). * SPARC64 uses "ta 0x72". That matches too many things for me to grep, though :). They're using __builtin_trap() for BUG. * PA-RISC doesn't name the instruciton, but they have two that appear to differ by an immediate and use different ones for KGDB and BUG. * NIOS uses "trap 30", which goes directly to a KGDB handler. I think they spin on BUG(). * SH uses "trapa #0x3c" for KGDB and other immediates for other breakpoints. Looks like they use 0x3C for BUG(), and while there's some tables being built I don't think those are to differentiate between KGDB and BUG. * ARM uses an undefined instruction, which needs to be hooked into the undefined instruction trap handler. I think it's a unique undefined instruction for kgdb, though. The only architecture that doesn't work this way is Hexagon. Hexagon uses "trap0(#0xDB)" for both, just switching on whether the trap came from user mode or kernel mode. Breakpoint traps from user mode cause SIGBREAK, those from kernel mode go to KGDB. They don't have a bug.h, which I think means they're just spinning on BUG(). We use ebreak for BUG(), so we need something to differentiate between BUG() and KGDB. I guess we could build up the tables in BUG(), but given that KGDB already knows the breakpoints this seems simpler -- I'd prefer to avoid having BUG rely on the sanity of the kernel image :). Presumably this mechanism could be used for H8, microblaze, and NIOS. > > > Daniel.
On Thu, Mar 19, 2020 at 10:34:33AM -0700, Palmer Dabbelt wrote: > On Thu, 19 Mar 2020 04:40:32 PDT (-0700), daniel.thompson@linaro.org wrote: > > On Wed, Mar 18, 2020 at 11:03:25AM -0700, Palmer Dabbelt wrote: > > > On Tue, 03 Mar 2020 00:47:00 PST (-0800), vincent.chen@sifive.com wrote: > > > > The break instruction in RISC-V does not have an immediate value field, so > > > > the kernel cannot identify the purpose of each trap exception through the > > > > opcode. This makes the existing identification schemes in other > > > > architecture unsuitable for the RISC-V kernel. To solve this problem, this > > > > patch adds kgdb_has_hit_break(), which can help RISC-V kernel identify > > > > the KGDB trap exception. > > > > > > > > Signed-off-by: Vincent Chen <vincent.chen@sifive.com> > > > > --- > > > > kernel/debug/debug_core.c | 12 ++++++++++++ > > > > 1 file changed, 12 insertions(+) > > > > > > > > diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c > > > > index 2b7c9b67931d..01bc3eea3d4d 100644 > > > > --- a/kernel/debug/debug_core.c > > > > +++ b/kernel/debug/debug_core.c > > > > @@ -417,6 +417,18 @@ int kgdb_isremovedbreak(unsigned long addr) > > > > return 0; > > > > } > > > > > > > > +int kgdb_has_hit_break(unsigned long addr) > > > > +{ > > > > + int i; > > > > + > > > > + for (i = 0; i < KGDB_MAX_BREAKPOINTS; i++) { > > > > + if (kgdb_break[i].state == BP_ACTIVE && > > > > + kgdb_break[i].bpt_addr == addr) > > > > + return 1; > > > > + } > > > > + return 0; > > > > +} > > > > + > > > > int dbg_remove_all_break(void) > > > > { > > > > int error; > > > > > > Reviewed-by: Palmer Dabbelt <palmerdabbelt@google.com> > > > > I've been slow to review this because I wanted to take a closer > > look at whether this issue is unique to RV or whether one of the > > other architectures solved it a different way so, out of interest, > > did you do any investigations in this direction? > > Not specifically in the context of KGDB implementations, but we've been chasing > these issues around in the rest of the debug stack for years now (I also > maintain the RISC-V GDB port, though much less actively than I used to). For > many projects we've solved this by adding an ABI restriction that defines a > two-instruction pairing for certain types of breakpoints that looks something > like > > addi x0, x0, MAGIC > ebreak > > I generally prefer the approach here, as it doesn't impose an ABI restriction, > but we've had to shoehorn it into so many projects I just kind of pattern > matched KGDB into the same bin. > > Looking through "find -name "*kgdb*" | grep arch", I see that most > architectures that have KGDB support also have immediates in their breakpoints. > I'm just going to assume they're all differentiating between their types of > breakpoints based on the immediate: Many thanks for the summary here. The implementation looked fine... I just wanted to be sure that there wasn't a second solution to the same problem lurking in one of the less common architectures! Acked-by: Daniel Thompson <daniel.thompson@linaro.org> Daniel. > > * Microblaze has a "brki" instruction, presumably the immediate indicates it's > a KGDB breakpoint. I think they spin on BUG(). > * arm64 has a "brk" insturction with an immediate. They're using 0 for BUG(), > 0x401 for KDB. > * PPC uses "twge r2, r2". I can't find it in the ISA manual, but they have > other traps with immediates and there seems to be more than sufficient > information to differentiate between the types of traps. They use "twi 31, > 0, 0" for BUG. > * ARC uses "trap_s 0x4", and they appear to use different immediates for > different purposes (0 for a user breakpoint). They use __builtin_trap() for > BUG(), which I'm assuming is the same as the user breakpoint. > * H8 uses "trapa #2", and they're using "trapa #3" for userspace breakpoints. > As far as I can tell they're spinning on BUG(). > * SPARC64 uses "ta 0x72". That matches too many things for me to grep, though > :). They're using __builtin_trap() for BUG. > * PA-RISC doesn't name the instruciton, but they have two that appear to differ > by an immediate and use different ones for KGDB and BUG. > * NIOS uses "trap 30", which goes directly to a KGDB handler. I think they > spin on BUG(). > * SH uses "trapa #0x3c" for KGDB and other immediates for other breakpoints. > Looks like they use 0x3C for BUG(), and while there's some tables being built > I don't think those are to differentiate between KGDB and BUG. > * ARM uses an undefined instruction, which needs to be hooked into the > undefined instruction trap handler. I think it's a unique undefined > instruction for kgdb, though. > > The only architecture that doesn't work this way is Hexagon. Hexagon uses > "trap0(#0xDB)" for both, just switching on whether the trap came from user mode > or kernel mode. Breakpoint traps from user mode cause SIGBREAK, those from > kernel mode go to KGDB. They don't have a bug.h, which I think means they're > just spinning on BUG(). We use ebreak for BUG(), so we need something to > differentiate between BUG() and KGDB. I guess we could build up the tables in > BUG(), but given that KGDB already knows the breakpoints this seems simpler -- > I'd prefer to avoid having BUG rely on the sanity of the kernel image :). > > Presumably this mechanism could be used for H8, microblaze, and NIOS. > > > > > > > Daniel.
diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c index 2b7c9b67931d..01bc3eea3d4d 100644 --- a/kernel/debug/debug_core.c +++ b/kernel/debug/debug_core.c @@ -417,6 +417,18 @@ int kgdb_isremovedbreak(unsigned long addr) return 0; } +int kgdb_has_hit_break(unsigned long addr) +{ + int i; + + for (i = 0; i < KGDB_MAX_BREAKPOINTS; i++) { + if (kgdb_break[i].state == BP_ACTIVE && + kgdb_break[i].bpt_addr == addr) + return 1; + } + return 0; +} + int dbg_remove_all_break(void) { int error;
The break instruction in RISC-V does not have an immediate value field, so the kernel cannot identify the purpose of each trap exception through the opcode. This makes the existing identification schemes in other architecture unsuitable for the RISC-V kernel. To solve this problem, this patch adds kgdb_has_hit_break(), which can help RISC-V kernel identify the KGDB trap exception. Signed-off-by: Vincent Chen <vincent.chen@sifive.com> --- kernel/debug/debug_core.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)