* [PATCH 1/5] kgdb: Add kgdb_has_hit_break function
@ 2020-03-03 8:47 Vincent Chen
2020-03-18 18:03 ` Palmer Dabbelt
0 siblings, 1 reply; 6+ messages in thread
From: Vincent Chen @ 2020-03-03 8:47 UTC (permalink / raw)
To: paul.walmsley, palmer, jason.wessel, daniel.thompson, dianders
Cc: Vincent Chen, linux-riscv
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;
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/5] kgdb: Add kgdb_has_hit_break function
2020-03-03 8:47 [PATCH 1/5] kgdb: Add kgdb_has_hit_break function Vincent Chen
@ 2020-03-18 18:03 ` Palmer Dabbelt
2020-03-19 11:40 ` Daniel Thompson
0 siblings, 1 reply; 6+ messages in thread
From: Palmer Dabbelt @ 2020-03-18 18:03 UTC (permalink / raw)
To: vincent.chen
Cc: daniel.thompson, Paul Walmsley, dianders, vincent.chen,
jason.wessel, linux-riscv
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>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/5] kgdb: Add kgdb_has_hit_break function
2020-03-18 18:03 ` Palmer Dabbelt
@ 2020-03-19 11:40 ` Daniel Thompson
2020-03-19 17:02 ` Vincent Chen
2020-03-19 17:34 ` Palmer Dabbelt
0 siblings, 2 replies; 6+ messages in thread
From: Daniel Thompson @ 2020-03-19 11:40 UTC (permalink / raw)
To: Palmer Dabbelt
Cc: vincent.chen, linux-riscv, jason.wessel, dianders, Paul Walmsley
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/5] kgdb: Add kgdb_has_hit_break function
2020-03-19 11:40 ` Daniel Thompson
@ 2020-03-19 17:02 ` Vincent Chen
2020-03-19 17:34 ` Palmer Dabbelt
1 sibling, 0 replies; 6+ messages in thread
From: Vincent Chen @ 2020-03-19 17:02 UTC (permalink / raw)
To: Daniel Thompson
Cc: linux-riscv, jason.wessel, Palmer Dabbelt, Douglas Anderson,
Paul Walmsley
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/5] kgdb: Add kgdb_has_hit_break function
2020-03-19 11:40 ` Daniel Thompson
2020-03-19 17:02 ` Vincent Chen
@ 2020-03-19 17:34 ` Palmer Dabbelt
2020-03-20 12:23 ` Daniel Thompson
1 sibling, 1 reply; 6+ messages in thread
From: Palmer Dabbelt @ 2020-03-19 17:34 UTC (permalink / raw)
To: daniel.thompson
Cc: vincent.chen, linux-riscv, jason.wessel, dianders, Paul Walmsley
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/5] kgdb: Add kgdb_has_hit_break function
2020-03-19 17:34 ` Palmer Dabbelt
@ 2020-03-20 12:23 ` Daniel Thompson
0 siblings, 0 replies; 6+ messages in thread
From: Daniel Thompson @ 2020-03-20 12:23 UTC (permalink / raw)
To: Palmer Dabbelt
Cc: vincent.chen, linux-riscv, jason.wessel, dianders, Paul Walmsley
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-03-20 12:24 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-03 8:47 [PATCH 1/5] kgdb: Add kgdb_has_hit_break function Vincent Chen
2020-03-18 18:03 ` Palmer Dabbelt
2020-03-19 11:40 ` Daniel Thompson
2020-03-19 17:02 ` Vincent Chen
2020-03-19 17:34 ` Palmer Dabbelt
2020-03-20 12:23 ` Daniel Thompson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).