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

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

[Vincent Chen]

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



_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Vincent Chen]

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

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

[Vincent Chen]

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


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Vincent Chen]

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

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

[Vincent Chen]

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


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Vincent Chen]

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

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

[Vincent Chen]

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


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Vincent Chen]

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

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

[Palmer Dabbelt]

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?

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Palmer Dabbelt]

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?

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

[Vincent Chen]

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.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Vincent Chen]

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.