* [PATCH 0/3] riscv: Fix debug instruction check and support trap-based WARN()
@ 2019-02-28 10:31 Vincent Chen
2019-02-28 10:31 ` [PATCH 1/3] riscv: Add the support for c.ebreak check in is_valid_bugaddr() Vincent Chen
` (3 more replies)
0 siblings, 4 replies; 6+ messages in thread
From: Vincent Chen @ 2019-02-28 10:31 UTC (permalink / raw)
To: palmer, aou, ebiederm, jimw, arnd, linux-riscv, linux-kernel
Cc: deanbo422, vincentc
The handler for the debug exception will call is_valid_bugaddr(bugaddr) to
check if the instruction in bugaddr is a real debug instruction. However,
the expected instruction, ebreak, is possibly translated to c.ebreak by
assmebler when C extension is supported. This patchset will add c.ebreak
into the check mechanism. In addition, BUG() is currently unable to work in
the kernel module due to an inappropriated condition in is_valid_bugaddr().
This issue will be fixed in this patchset. Finally, this patchset enables
WARN() related functions to trap the code to help developers debug it.
Vincent Chen (3):
riscv: Add the support for c.ebreak check in is_valid_bugaddr()
riscv: Support BUG() in kernel module
riscv: Make WARN() related functions able to trigger a trap exception
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/3] riscv: Add the support for c.ebreak check in is_valid_bugaddr()
2019-02-28 10:31 [PATCH 0/3] riscv: Fix debug instruction check and support trap-based WARN() Vincent Chen
@ 2019-02-28 10:31 ` Vincent Chen
2019-02-28 10:31 ` [PATCH 2/3] riscv: Support BUG() in kernel module Vincent Chen
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Vincent Chen @ 2019-02-28 10:31 UTC (permalink / raw)
To: palmer, aou, ebiederm, jimw, arnd, linux-riscv, linux-kernel
Cc: deanbo422, vincentc
The is_valid_bugaddr() function compares the instruction pointed to by
$sepc with macro __BUG_INSN to check whether the current trap exception
is caused by an "ebreak" instruction. Hence the macro __BUG_INSN is
defined as "ebreak". However, this check flow is possibly erroneous
because the expected trap instruction possibly be translated to "c.ebreak"
by assembler if C extension is supported. Therefore, it requires
a mechanism to distinguish the length of the instruction in $spec and
compare it to the correct __BUG_INSN.
By the way, I make the kernel go to die() like BUG() when the trap
type is BUG_TRAP_TYPE_WARN because currently the WARN() does not trigger
any trap exception.
Signed-off-by: Vincent Chen <vincentc@andestech.com>
---
arch/riscv/include/asm/bug.h | 7 ++++++-
arch/riscv/kernel/traps.c | 8 +++++---
2 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/arch/riscv/include/asm/bug.h b/arch/riscv/include/asm/bug.h
index bfc7f09..1cab7f4 100644
--- a/arch/riscv/include/asm/bug.h
+++ b/arch/riscv/include/asm/bug.h
@@ -21,7 +21,12 @@
#include <asm/asm.h>
#ifdef CONFIG_GENERIC_BUG
-#define __BUG_INSN _AC(0x00100073, UL) /* ebreak */
+#define __INSN_LENGTH_MASK _UL(0x3)
+#define __INSN_LENGTH_32 _UL(0x3)
+#define __COMPRESSED_INSN_MASK _UL(0xffff)
+
+#define __BUG_INSN_32 _UL(0x00100073) /* ebreak */
+#define __BUG_INSN_16 _UL(0x9002) /* c.ebreak */
#ifndef __ASSEMBLY__
typedef u32 bug_insn_t;
diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
index 24a9333..deae0e5 100644
--- a/arch/riscv/kernel/traps.c
+++ b/arch/riscv/kernel/traps.c
@@ -129,8 +129,7 @@ asmlinkage void do_trap_break(struct pt_regs *regs)
case BUG_TRAP_TYPE_NONE:
break;
case BUG_TRAP_TYPE_WARN:
- regs->sepc += sizeof(bug_insn_t);
- return;
+ die(regs, "Kernel BUG. Kernel got an unexpected WARN trapped by ebreak");
case BUG_TRAP_TYPE_BUG:
die(regs, "Kernel BUG");
}
@@ -149,7 +148,10 @@ int is_valid_bugaddr(unsigned long pc)
return 0;
if (probe_kernel_address((bug_insn_t *)pc, insn))
return 0;
- return (insn == __BUG_INSN);
+ if ((insn & __INSN_LENGTH_MASK) == __INSN_LENGTH_32)
+ return insn == __BUG_INSN_32;
+ else
+ return (insn & __COMPRESSED_INSN_MASK) == __BUG_INSN_16;
}
#endif /* CONFIG_GENERIC_BUG */
--
1.7.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/3] riscv: Support BUG() in kernel module
2019-02-28 10:31 [PATCH 0/3] riscv: Fix debug instruction check and support trap-based WARN() Vincent Chen
2019-02-28 10:31 ` [PATCH 1/3] riscv: Add the support for c.ebreak check in is_valid_bugaddr() Vincent Chen
@ 2019-02-28 10:31 ` Vincent Chen
2019-02-28 10:31 ` [PATCH 3/3] riscv: Make WARN() related functions able to trigger a trap exception Vincent Chen
2019-03-04 20:35 ` [PATCH 0/3] riscv: Fix debug instruction check and support trap-based WARN() Palmer Dabbelt
3 siblings, 0 replies; 6+ messages in thread
From: Vincent Chen @ 2019-02-28 10:31 UTC (permalink / raw)
To: palmer, aou, ebiederm, jimw, arnd, linux-riscv, linux-kernel
Cc: deanbo422, vincentc
The kernel module is loaded into vmalloc region which is located below
to the PAGE_OFFSET. Hence the condition, pc < PAGE_OFFSET, in the
is_valid_bugaddr() will filter out all trap exceptions triggered
by kernel module. To support BUG() in kernel module, the condition is
changed to pc < VMALLOC_START.
Signed-off-by: Vincent Chen <vincentc@andestech.com>
---
arch/riscv/kernel/traps.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
index deae0e5..dee0e5e 100644
--- a/arch/riscv/kernel/traps.c
+++ b/arch/riscv/kernel/traps.c
@@ -144,7 +144,7 @@ int is_valid_bugaddr(unsigned long pc)
{
bug_insn_t insn;
- if (pc < PAGE_OFFSET)
+ if (pc < VMALLOC_START)
return 0;
if (probe_kernel_address((bug_insn_t *)pc, insn))
return 0;
--
1.7.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 3/3] riscv: Make WARN() related functions able to trigger a trap exception
2019-02-28 10:31 [PATCH 0/3] riscv: Fix debug instruction check and support trap-based WARN() Vincent Chen
2019-02-28 10:31 ` [PATCH 1/3] riscv: Add the support for c.ebreak check in is_valid_bugaddr() Vincent Chen
2019-02-28 10:31 ` [PATCH 2/3] riscv: Support BUG() in kernel module Vincent Chen
@ 2019-02-28 10:31 ` Vincent Chen
2019-03-04 20:35 ` [PATCH 0/3] riscv: Fix debug instruction check and support trap-based WARN() Palmer Dabbelt
3 siblings, 0 replies; 6+ messages in thread
From: Vincent Chen @ 2019-02-28 10:31 UTC (permalink / raw)
To: palmer, aou, ebiederm, jimw, arnd, linux-riscv, linux-kernel
Cc: deanbo422, vincentc
This can help developers to analyze the cause of WARN() because the
control will be transferred to debugging environment if the debugger is
connected.
Signed-off-by: Vincent Chen <vincentc@andestech.com>
---
arch/riscv/include/asm/bug.h | 27 ++++++++++++++++++---------
arch/riscv/kernel/traps.c | 19 ++++++++++++++++---
2 files changed, 34 insertions(+), 12 deletions(-)
diff --git a/arch/riscv/include/asm/bug.h b/arch/riscv/include/asm/bug.h
index 1cab7f4..b2aa88f 100644
--- a/arch/riscv/include/asm/bug.h
+++ b/arch/riscv/include/asm/bug.h
@@ -43,38 +43,47 @@
#define __BUG_ENTRY \
__BUG_ENTRY_ADDR "\n\t" \
__BUG_ENTRY_FILE "\n\t" \
- RISCV_SHORT " %1"
+ RISCV_SHORT " %1 \n\t" \
+ RISCV_SHORT " %2"
#else
#define __BUG_ENTRY \
- __BUG_ENTRY_ADDR
+ __BUG_ENTRY_ADDR "\n\t" \
+ RISCV_SHORT " %2"
#endif
-#define BUG() \
+#define __BUG_FLAGS(flags) \
do { \
__asm__ __volatile__ ( \
"1:\n\t" \
"ebreak\n" \
- ".pushsection __bug_table,\"a\"\n\t" \
+ ".pushsection __bug_table,\"aw\"\n\t" \
"2:\n\t" \
__BUG_ENTRY "\n\t" \
- ".org 2b + %2\n\t" \
+ ".org 2b + %3\n\t" \
".popsection" \
: \
: "i" (__FILE__), "i" (__LINE__), \
- "i" (sizeof(struct bug_entry))); \
- unreachable(); \
+ "i" (flags), \
+ "i" (sizeof(struct bug_entry))); \
} while (0)
+
#endif /* !__ASSEMBLY__ */
#else /* CONFIG_GENERIC_BUG */
#ifndef __ASSEMBLY__
-#define BUG() \
+#define __BUG_FLAGS(flags) \
do { \
__asm__ __volatile__ ("ebreak\n"); \
- unreachable(); \
} while (0)
#endif /* !__ASSEMBLY__ */
#endif /* CONFIG_GENERIC_BUG */
+#define BUG() do { \
+ __BUG_FLAGS(0); \
+ unreachable(); \
+} while (0)
+
+#define __WARN_FLAGS(flags) __BUG_FLAGS(BUGFLAG_WARNING|(flags))
+
#define HAVE_ARCH_BUG
#include <asm-generic/bug.h>
diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
index dee0e5e..023208b 100644
--- a/arch/riscv/kernel/traps.c
+++ b/arch/riscv/kernel/traps.c
@@ -118,6 +118,17 @@ asmlinkage void name(struct pt_regs *regs) \
DO_ERROR_INFO(do_trap_ecall_m,
SIGILL, ILL_ILLTRP, "environment call from M-mode");
+#ifdef CONFIG_GENERIC_BUG
+static inline unsigned long get_break_insn_length(unsigned long pc)
+{
+ bug_insn_t insn;
+
+ if (probe_kernel_address((bug_insn_t *)pc, insn))
+ return 0;
+ return ((insn & __INSN_LENGTH_MASK) == __INSN_LENGTH_32) ? 4UL : 2UL;
+}
+#endif /* CONFIG_GENERIC_BUG */
+
asmlinkage void do_trap_break(struct pt_regs *regs)
{
#ifdef CONFIG_GENERIC_BUG
@@ -129,7 +140,8 @@ asmlinkage void do_trap_break(struct pt_regs *regs)
case BUG_TRAP_TYPE_NONE:
break;
case BUG_TRAP_TYPE_WARN:
- die(regs, "Kernel BUG. Kernel got an unexpected WARN trapped by ebreak");
+ regs->sepc += get_break_insn_length(regs->sepc);
+ break;
case BUG_TRAP_TYPE_BUG:
die(regs, "Kernel BUG");
}
@@ -149,12 +161,13 @@ int is_valid_bugaddr(unsigned long pc)
if (probe_kernel_address((bug_insn_t *)pc, insn))
return 0;
if ((insn & __INSN_LENGTH_MASK) == __INSN_LENGTH_32)
- return insn == __BUG_INSN_32;
+ return (insn == __BUG_INSN_32);
else
- return (insn & __COMPRESSED_INSN_MASK) == __BUG_INSN_16;
+ return ((insn & __COMPRESSED_INSN_MASK) == __BUG_INSN_16);
}
#endif /* CONFIG_GENERIC_BUG */
+
void __init trap_init(void)
{
/*
--
1.7.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 0/3] riscv: Fix debug instruction check and support trap-based WARN()
2019-02-28 10:31 [PATCH 0/3] riscv: Fix debug instruction check and support trap-based WARN() Vincent Chen
` (2 preceding siblings ...)
2019-02-28 10:31 ` [PATCH 3/3] riscv: Make WARN() related functions able to trigger a trap exception Vincent Chen
@ 2019-03-04 20:35 ` Palmer Dabbelt
2019-03-05 0:21 ` Vincent Chen
3 siblings, 1 reply; 6+ messages in thread
From: Palmer Dabbelt @ 2019-03-04 20:35 UTC (permalink / raw)
To: vincentc
Cc: aou, ebiederm, Jim Wilson, Arnd Bergmann, linux-riscv,
linux-kernel, deanbo422, vincentc
On Thu, 28 Feb 2019 02:31:28 PST (-0800), vincentc@andestech.com wrote:
> The handler for the debug exception will call is_valid_bugaddr(bugaddr) to
> check if the instruction in bugaddr is a real debug instruction. However,
> the expected instruction, ebreak, is possibly translated to c.ebreak by
> assmebler when C extension is supported. This patchset will add c.ebreak
> into the check mechanism. In addition, BUG() is currently unable to work in
> the kernel module due to an inappropriated condition in is_valid_bugaddr().
> This issue will be fixed in this patchset. Finally, this patchset enables
> WARN() related functions to trap the code to help developers debug it.
>
>
>
>
>
> Vincent Chen (3):
> riscv: Add the support for c.ebreak check in is_valid_bugaddr()
> riscv: Support BUG() in kernel module
> riscv: Make WARN() related functions able to trigger a trap exception
I'm finding this patch set a bit hard to follow, and I think it has more diff
than is necessary. For example, the first patch introduces a new die() only to
have it removed by the third patch. There's also some unnecessary
non-functional diff, like
@@ -149,12 +161,13 @@ int is_valid_bugaddr(unsigned long pc)
if (probe_kernel_address((bug_insn_t *)pc, insn))
return 0;
if ((insn & __INSN_LENGTH_MASK) == __INSN_LENGTH_32)
- return insn == __BUG_INSN_32;
+ return (insn == __BUG_INSN_32);
else
- return (insn & __COMPRESSED_INSN_MASK) == __BUG_INSN_16;
+ return ((insn & __COMPRESSED_INSN_MASK) == __BUG_INSN_16);
}
#endif /* CONFIG_GENERIC_BUG */
+
void __init trap_init(void)
{
/*
I like the idea of the patch set, though. Do you have time to clean it up and
submit a v2?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/3] riscv: Fix debug instruction check and support trap-based WARN()
2019-03-04 20:35 ` [PATCH 0/3] riscv: Fix debug instruction check and support trap-based WARN() Palmer Dabbelt
@ 2019-03-05 0:21 ` Vincent Chen
0 siblings, 0 replies; 6+ messages in thread
From: Vincent Chen @ 2019-03-05 0:21 UTC (permalink / raw)
To: Palmer Dabbelt
Cc: aou, ebiederm, Jim Wilson, Arnd Bergmann, linux-riscv,
linux-kernel, deanbo422
On Tue, Mar 05, 2019 at 04:35:08AM +0800, Palmer Dabbelt wrote:
> On Thu, 28 Feb 2019 02:31:28 PST (-0800), vincentc@andestech.com wrote:
> > The handler for the debug exception will call is_valid_bugaddr(bugaddr) to
> > check if the instruction in bugaddr is a real debug instruction. However,
> > the expected instruction, ebreak, is possibly translated to c.ebreak by
> > assmebler when C extension is supported. This patchset will add c.ebreak
> > into the check mechanism. In addition, BUG() is currently unable to work in
> > the kernel module due to an inappropriated condition in is_valid_bugaddr().
> > This issue will be fixed in this patchset. Finally, this patchset enables
> > WARN() related functions to trap the code to help developers debug it.
> >
> >
> >
> >
> >
> > Vincent Chen (3):
> > riscv: Add the support for c.ebreak check in is_valid_bugaddr()
> > riscv: Support BUG() in kernel module
> > riscv: Make WARN() related functions able to trigger a trap exception
>
> I'm finding this patch set a bit hard to follow, and I think it has more diff
> than is necessary. For example, the first patch introduces a new die() only to
> have it removed by the third patch. There's also some unnecessary
> non-functional diff, like
>
> @@ -149,12 +161,13 @@ int is_valid_bugaddr(unsigned long pc)
> if (probe_kernel_address((bug_insn_t *)pc, insn))
> return 0;
> if ((insn & __INSN_LENGTH_MASK) == __INSN_LENGTH_32)
> - return insn == __BUG_INSN_32;
> + return (insn == __BUG_INSN_32);
> else
> - return (insn & __COMPRESSED_INSN_MASK) == __BUG_INSN_16;
> + return ((insn & __COMPRESSED_INSN_MASK) == __BUG_INSN_16);
> }
> #endif /* CONFIG_GENERIC_BUG */
>
> +
> void __init trap_init(void)
> {
> /*
>
> I like the idea of the patch set, though. Do you have time to clean it up and
> submit a v2?
OK, I will improve it and submit a v2 patch.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-03-05 0:22 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-28 10:31 [PATCH 0/3] riscv: Fix debug instruction check and support trap-based WARN() Vincent Chen
2019-02-28 10:31 ` [PATCH 1/3] riscv: Add the support for c.ebreak check in is_valid_bugaddr() Vincent Chen
2019-02-28 10:31 ` [PATCH 2/3] riscv: Support BUG() in kernel module Vincent Chen
2019-02-28 10:31 ` [PATCH 3/3] riscv: Make WARN() related functions able to trigger a trap exception Vincent Chen
2019-03-04 20:35 ` [PATCH 0/3] riscv: Fix debug instruction check and support trap-based WARN() Palmer Dabbelt
2019-03-05 0:21 ` Vincent Chen
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).