2019-02-28 12:13:45

by Vincent Chen

[permalink] [raw]
Subject: [PATCH 0/3] riscv: Fix debug instruction check and support trap-based WARN()

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




2019-02-28 12:13:51

by Vincent Chen

[permalink] [raw]
Subject: [PATCH 1/3] riscv: Add the support for c.ebreak check in is_valid_bugaddr()

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 <[email protected]>
---
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


2019-02-28 12:14:08

by Vincent Chen

[permalink] [raw]
Subject: [PATCH 3/3] riscv: Make WARN() related functions able to trigger a trap exception

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 <[email protected]>
---
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


2019-02-28 12:14:21

by Vincent Chen

[permalink] [raw]
Subject: [PATCH 2/3] riscv: Support BUG() in kernel module

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 <[email protected]>
---
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


2019-03-04 20:36:14

by Palmer Dabbelt

[permalink] [raw]
Subject: Re: [PATCH 0/3] riscv: Fix debug instruction check and support trap-based WARN()

On Thu, 28 Feb 2019 02:31:28 PST (-0800), [email protected] 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?

2019-03-05 00:23:45

by Vincent Chen

[permalink] [raw]
Subject: Re: [PATCH 0/3] riscv: Fix debug instruction check and support trap-based WARN()

On Tue, Mar 05, 2019 at 04:35:08AM +0800, Palmer Dabbelt wrote:
> On Thu, 28 Feb 2019 02:31:28 PST (-0800), [email protected] 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.