All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3] powerpc/traps: Enhance readability for trap types
@ 2021-04-08 14:07 ` Xiongwei Song
  0 siblings, 0 replies; 17+ messages in thread
From: Xiongwei Song @ 2021-04-08 14:07 UTC (permalink / raw)
  To: mpe, benh, paulus, oleg, npiggin, christophe.leroy, aneesh.kumar,
	ravi.bangoria, mikey, haren, akpm, rppt, jniethe5, atrajeev,
	maddy, peterz, kjain, kan.liang, aik, alistair, pmladek,
	john.ogness
  Cc: linuxppc-dev, linux-kernel, Xiongwei Song

From: Xiongwei Song <sxwjean@gmail.com>

Create a new header named traps.h, define macros to list ppc interrupt
types in traps.h, replace the reference of the trap hex values with these
macros.

Referred the hex number in arch/powerpc/kernel/exceptions-64e.S,
arch/powerpc/kernel/exceptions-64s.S and
arch/powerpc/include/asm/kvm_asm.h.

v2-v3:
Correct the prefix of trap macros with INTERRUPT_, the previous prefix
is TRAP_, which is not precise. This is suggested by Segher Boessenkool
and Nicholas Piggin.

v1-v2:
Define more trap macros to replace more trap hexs in code, not just for
the __show_regs function. This is suggested by Christophe Leroy.

Signed-off-by: Xiongwei Song <sxwjean@gmail.com>
---
 arch/powerpc/include/asm/interrupt.h  |  9 +++++---
 arch/powerpc/include/asm/ptrace.h     |  3 ++-
 arch/powerpc/include/asm/traps.h      | 32 +++++++++++++++++++++++++++
 arch/powerpc/kernel/interrupt.c       |  3 ++-
 arch/powerpc/kernel/process.c         |  5 ++++-
 arch/powerpc/mm/book3s64/hash_utils.c |  5 +++--
 arch/powerpc/mm/fault.c               | 21 +++++++++++-------
 arch/powerpc/perf/core-book3s.c       |  5 +++--
 arch/powerpc/xmon/xmon.c              | 16 +++++++++++---
 9 files changed, 78 insertions(+), 21 deletions(-)
 create mode 100644 arch/powerpc/include/asm/traps.h

diff --git a/arch/powerpc/include/asm/interrupt.h b/arch/powerpc/include/asm/interrupt.h
index 7c633896d758..5ce9898bc9a6 100644
--- a/arch/powerpc/include/asm/interrupt.h
+++ b/arch/powerpc/include/asm/interrupt.h
@@ -8,6 +8,7 @@
 #include <asm/ftrace.h>
 #include <asm/kprobes.h>
 #include <asm/runlatch.h>
+#include <asm/traps.h>
 
 struct interrupt_state {
 #ifdef CONFIG_PPC_BOOK3E_64
@@ -59,7 +60,7 @@ static inline void interrupt_enter_prepare(struct pt_regs *regs, struct interrup
 		 * CT_WARN_ON comes here via program_check_exception,
 		 * so avoid recursion.
 		 */
-		if (TRAP(regs) != 0x700)
+		if (TRAP(regs) != INTERRUPT_PROGRAM)
 			CT_WARN_ON(ct_state() != CONTEXT_KERNEL);
 	}
 #endif
@@ -156,7 +157,8 @@ static inline void interrupt_nmi_enter_prepare(struct pt_regs *regs, struct inte
 	/* Don't do any per-CPU operations until interrupt state is fixed */
 #endif
 	/* Allow DEC and PMI to be traced when they are soft-NMI */
-	if (TRAP(regs) != 0x900 && TRAP(regs) != 0xf00 && TRAP(regs) != 0x260) {
+	if (TRAP(regs) != INTERRUPT_DECREMENTER &&
+	    TRAP(regs) != INTERRUPT_PERFMON) {
 		state->ftrace_enabled = this_cpu_get_ftrace_enabled();
 		this_cpu_set_ftrace_enabled(0);
 	}
@@ -180,7 +182,8 @@ static inline void interrupt_nmi_exit_prepare(struct pt_regs *regs, struct inter
 		nmi_exit();
 
 #ifdef CONFIG_PPC64
-	if (TRAP(regs) != 0x900 && TRAP(regs) != 0xf00 && TRAP(regs) != 0x260)
+	if (TRAP(regs) != INTERRUPT_DECREMENTER &&
+	    TRAP(regs) != INTERRUPT_PERFMON)
 		this_cpu_set_ftrace_enabled(state->ftrace_enabled);
 
 #ifdef CONFIG_PPC_BOOK3S_64
diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
index f10498e1b3f6..7a17e0365d43 100644
--- a/arch/powerpc/include/asm/ptrace.h
+++ b/arch/powerpc/include/asm/ptrace.h
@@ -21,6 +21,7 @@
 
 #include <uapi/asm/ptrace.h>
 #include <asm/asm-const.h>
+#include <asm/traps.h>
 
 #ifndef __ASSEMBLY__
 struct pt_regs
@@ -237,7 +238,7 @@ static inline bool trap_is_unsupported_scv(struct pt_regs *regs)
 
 static inline bool trap_is_syscall(struct pt_regs *regs)
 {
-	return (trap_is_scv(regs) || TRAP(regs) == 0xc00);
+	return (trap_is_scv(regs) || TRAP(regs) == INTERRUPT_SYSCALL);
 }
 
 static inline bool trap_norestart(struct pt_regs *regs)
diff --git a/arch/powerpc/include/asm/traps.h b/arch/powerpc/include/asm/traps.h
new file mode 100644
index 000000000000..cb416a17097c
--- /dev/null
+++ b/arch/powerpc/include/asm/traps.h
@@ -0,0 +1,32 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_PPC_TRAPS_H
+#define _ASM_PPC_TRAPS_H
+
+#if defined(CONFIG_BOOKE) || defined(CONFIG_4xx)
+#define INTERRUPT_MACHINE_CHECK   0x000
+#define INTERRUPT_CRITICAL_INPUT  0x100
+#define INTERRUPT_ALTIVEC_UNAVAIL 0x200
+#define INTERRUPT_PERFMON         0x260
+#define INTERRUPT_DOORBELL        0x280
+#define INTERRUPT_DEBUG           0xd00
+#elif defined(CONFIG_PPC_BOOK3S)
+#define INTERRUPT_SYSTEM_RESET    0x100
+#define INTERRUPT_MACHINE_CHECK   0x200
+#define INTERRUPT_DATA_SEGMENT    0x380
+#define INTERRUPT_INST_SEGMENT    0x480
+#define INTERRUPT_DOORBELL        0xa00
+#define INTERRUPT_TRACE           0xd00
+#define INTERRUPT_H_DATA_STORAGE  0xe00
+#define INTERRUPT_PERFMON         0xf00
+#define INTERRUPT_H_FAC_UNAVAIL   0xf80
+#endif
+
+#define INTERRUPT_DATA_STORAGE    0x300
+#define INTERRUPT_INST_STORAGE    0x400
+#define INTERRUPT_ALIGNMENT       0x600
+#define INTERRUPT_PROGRAM         0x700
+#define INTERRUPT_FP_UNAVAIL      0x800
+#define INTERRUPT_DECREMENTER     0x900
+#define INTERRUPT_SYSCALL         0xc00
+
+#endif /* _ASM_PPC_TRAPS_H */
diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
index c4dd4b8f9cfa..72689f7ca7c8 100644
--- a/arch/powerpc/kernel/interrupt.c
+++ b/arch/powerpc/kernel/interrupt.c
@@ -19,6 +19,7 @@
 #include <asm/syscall.h>
 #include <asm/time.h>
 #include <asm/unistd.h>
+#include <asm/traps.h>
 
 #if defined(CONFIG_PPC_ADV_DEBUG_REGS) && defined(CONFIG_PPC32)
 unsigned long global_dbcr0[NR_CPUS];
@@ -456,7 +457,7 @@ notrace unsigned long interrupt_exit_kernel_prepare(struct pt_regs *regs, unsign
 	 * CT_WARN_ON comes here via program_check_exception,
 	 * so avoid recursion.
 	 */
-	if (TRAP(regs) != 0x700)
+	if (TRAP(regs) != INTERRUPT_PROGRAM)
 		CT_WARN_ON(ct_state() == CONTEXT_USER);
 
 	kuap = kuap_get_and_assert_locked();
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index b966c8e0cead..92cd49427b2f 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -64,6 +64,7 @@
 #include <asm/asm-prototypes.h>
 #include <asm/stacktrace.h>
 #include <asm/hw_breakpoint.h>
+#include <asm/traps.h>
 
 #include <linux/kprobes.h>
 #include <linux/kdebug.h>
@@ -1469,7 +1470,9 @@ static void __show_regs(struct pt_regs *regs)
 	trap = TRAP(regs);
 	if (!trap_is_syscall(regs) && cpu_has_feature(CPU_FTR_CFAR))
 		pr_cont("CFAR: "REG" ", regs->orig_gpr3);
-	if (trap == 0x200 || trap == 0x300 || trap == 0x600) {
+	if (trap == INTERRUPT_MACHINE_CHECK ||
+	    trap == INTERRUPT_DATA_STORAGE ||
+	    trap == INTERRUPT_ALIGNMENT) {
 		if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
 			pr_cont("DEAR: "REG" ESR: "REG" ", regs->dar, regs->dsisr);
 		else
diff --git a/arch/powerpc/mm/book3s64/hash_utils.c b/arch/powerpc/mm/book3s64/hash_utils.c
index 7719995323c3..2bf06e01b309 100644
--- a/arch/powerpc/mm/book3s64/hash_utils.c
+++ b/arch/powerpc/mm/book3s64/hash_utils.c
@@ -64,6 +64,7 @@
 #include <asm/pte-walk.h>
 #include <asm/asm-prototypes.h>
 #include <asm/ultravisor.h>
+#include <asm/traps.h>
 
 #include <mm/mmu_decl.h>
 
@@ -1145,7 +1146,7 @@ unsigned int hash_page_do_lazy_icache(unsigned int pp, pte_t pte, int trap)
 
 	/* page is dirty */
 	if (!test_bit(PG_dcache_clean, &page->flags) && !PageReserved(page)) {
-		if (trap == 0x400) {
+		if (trap == INTERRUPT_INST_STORAGE) {
 			flush_dcache_icache_page(page);
 			set_bit(PG_dcache_clean, &page->flags);
 		} else
@@ -1545,7 +1546,7 @@ DEFINE_INTERRUPT_HANDLER_RET(__do_hash_fault)
 	if (user_mode(regs) || (region_id == USER_REGION_ID))
 		access &= ~_PAGE_PRIVILEGED;
 
-	if (TRAP(regs) == 0x400)
+	if (TRAP(regs) == INTERRUPT_INST_STORAGE)
 		access |= _PAGE_EXEC;
 
 	err = hash_page_mm(mm, ea, access, TRAP(regs), flags);
diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index 0c0b1c2cfb49..641b3feef7ee 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -44,6 +44,7 @@
 #include <asm/debug.h>
 #include <asm/kup.h>
 #include <asm/inst.h>
+#include <asm/traps.h>
 
 
 /*
@@ -197,7 +198,7 @@ static int mm_fault_error(struct pt_regs *regs, unsigned long addr,
 static bool bad_kernel_fault(struct pt_regs *regs, unsigned long error_code,
 			     unsigned long address, bool is_write)
 {
-	int is_exec = TRAP(regs) == 0x400;
+	int is_exec = TRAP(regs) == INTERRUPT_INST_STORAGE;
 
 	/* NX faults set DSISR_PROTFAULT on the 8xx, DSISR_NOEXEC_OR_G on others */
 	if (is_exec && (error_code & (DSISR_NOEXEC_OR_G | DSISR_KEYFAULT |
@@ -391,7 +392,7 @@ static int ___do_page_fault(struct pt_regs *regs, unsigned long address,
 	struct vm_area_struct * vma;
 	struct mm_struct *mm = current->mm;
 	unsigned int flags = FAULT_FLAG_DEFAULT;
- 	int is_exec = TRAP(regs) == 0x400;
+	int is_exec = TRAP(regs) == INTERRUPT_INST_STORAGE;
 	int is_user = user_mode(regs);
 	int is_write = page_fault_is_write(error_code);
 	vm_fault_t fault, major = 0;
@@ -588,20 +589,24 @@ void __bad_page_fault(struct pt_regs *regs, int sig)
 	/* kernel has accessed a bad area */
 
 	switch (TRAP(regs)) {
-	case 0x300:
-	case 0x380:
-	case 0xe00:
+	case INTERRUPT_DATA_STORAGE:
+#ifdef CONFIG_PPC_BOOK3S
+	case INTERRUPT_DATA_SEGMENT:
+	case INTERRUPT_H_DATA_STORAGE:
+#endif
 		pr_alert("BUG: %s on %s at 0x%08lx\n",
 			 regs->dar < PAGE_SIZE ? "Kernel NULL pointer dereference" :
 			 "Unable to handle kernel data access",
 			 is_write ? "write" : "read", regs->dar);
 		break;
-	case 0x400:
-	case 0x480:
+	case INTERRUPT_INST_STORAGE:
+#ifdef CONFIG_PPC_BOOK3S
+	case INTERRUPT_INST_SEGMENT:
+#endif
 		pr_alert("BUG: Unable to handle kernel instruction fetch%s",
 			 regs->nip < PAGE_SIZE ? " (NULL pointer?)\n" : "\n");
 		break;
-	case 0x600:
+	case INTERRUPT_ALIGNMENT:
 		pr_alert("BUG: Unable to handle kernel unaligned access at 0x%08lx\n",
 			 regs->dar);
 		break;
diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
index 766f064f00fb..6e34f5bba232 100644
--- a/arch/powerpc/perf/core-book3s.c
+++ b/arch/powerpc/perf/core-book3s.c
@@ -17,6 +17,7 @@
 #include <asm/firmware.h>
 #include <asm/ptrace.h>
 #include <asm/code-patching.h>
+#include <asm/traps.h>
 
 #ifdef CONFIG_PPC64
 #include "internal.h"
@@ -168,7 +169,7 @@ static bool regs_use_siar(struct pt_regs *regs)
 	 * they have not been setup using perf_read_regs() and so regs->result
 	 * is something random.
 	 */
-	return ((TRAP(regs) == 0xf00) && regs->result);
+	return ((TRAP(regs) == INTERRUPT_PERFMON) && regs->result);
 }
 
 /*
@@ -347,7 +348,7 @@ static inline void perf_read_regs(struct pt_regs *regs)
 	 * hypervisor samples as well as samples in the kernel with
 	 * interrupts off hence the userspace check.
 	 */
-	if (TRAP(regs) != 0xf00)
+	if (TRAP(regs) != INTERRUPT_PERFMON)
 		use_siar = 0;
 	else if ((ppmu->flags & PPMU_NO_SIAR))
 		use_siar = 0;
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index bf7d69625a2e..2a4f99e64bf3 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -54,6 +54,7 @@
 #include <asm/code-patching.h>
 #include <asm/sections.h>
 #include <asm/inst.h>
+#include <asm/traps.h>
 
 #ifdef CONFIG_PPC64
 #include <asm/hvcall.h>
@@ -1769,7 +1770,12 @@ static void excprint(struct pt_regs *fp)
 	printf("    sp: %lx\n", fp->gpr[1]);
 	printf("   msr: %lx\n", fp->msr);
 
-	if (trap == 0x300 || trap == 0x380 || trap == 0x600 || trap == 0x200) {
+	if (trap == INTERRUPT_DATA_STORAGE ||
+#ifdef CONFIG_PPC_BOOK3S
+	    trap == INTERRUPT_DATA_SEGMENT ||
+#endif
+	    trap == INTERRUPT_ALIGNMENT ||
+	    trap == INTERRUPT_MACHINE_CHECK) {
 		printf("   dar: %lx\n", fp->dar);
 		if (trap != 0x380)
 			printf(" dsisr: %lx\n", fp->dsisr);
@@ -1785,7 +1791,7 @@ static void excprint(struct pt_regs *fp)
 		       current->pid, current->comm);
 	}
 
-	if (trap == 0x700)
+	if (trap == INTERRUPT_PROGRAM)
 		print_bug_trap(fp);
 
 	printf(linux_banner);
@@ -1846,7 +1852,11 @@ static void prregs(struct pt_regs *fp)
 	printf("ctr = "REG"   xer = "REG"   trap = %4lx\n",
 	       fp->ctr, fp->xer, fp->trap);
 	trap = TRAP(fp);
-	if (trap == 0x300 || trap == 0x380 || trap == 0x600)
+	if (trap == INTERRUPT_DATA_STORAGE ||
+#ifdef CONFIG_PPC_BOOK3S
+	    trap == INTERRUPT_DATA_SEGMENT ||
+#endif
+	    trap == INTERRUPT_ALIGNMENT)
 		printf("dar = "REG"   dsisr = %.8lx\n", fp->dar, fp->dsisr);
 }
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v3] powerpc/traps: Enhance readability for trap types
@ 2021-04-08 14:07 ` Xiongwei Song
  0 siblings, 0 replies; 17+ messages in thread
From: Xiongwei Song @ 2021-04-08 14:07 UTC (permalink / raw)
  To: mpe, benh, paulus, oleg, npiggin, christophe.leroy, aneesh.kumar,
	ravi.bangoria, mikey, haren, akpm, rppt, jniethe5, atrajeev,
	maddy, peterz, kjain, kan.liang, aik, alistair, pmladek,
	john.ogness
  Cc: Xiongwei Song, linuxppc-dev, linux-kernel

From: Xiongwei Song <sxwjean@gmail.com>

Create a new header named traps.h, define macros to list ppc interrupt
types in traps.h, replace the reference of the trap hex values with these
macros.

Referred the hex number in arch/powerpc/kernel/exceptions-64e.S,
arch/powerpc/kernel/exceptions-64s.S and
arch/powerpc/include/asm/kvm_asm.h.

v2-v3:
Correct the prefix of trap macros with INTERRUPT_, the previous prefix
is TRAP_, which is not precise. This is suggested by Segher Boessenkool
and Nicholas Piggin.

v1-v2:
Define more trap macros to replace more trap hexs in code, not just for
the __show_regs function. This is suggested by Christophe Leroy.

Signed-off-by: Xiongwei Song <sxwjean@gmail.com>
---
 arch/powerpc/include/asm/interrupt.h  |  9 +++++---
 arch/powerpc/include/asm/ptrace.h     |  3 ++-
 arch/powerpc/include/asm/traps.h      | 32 +++++++++++++++++++++++++++
 arch/powerpc/kernel/interrupt.c       |  3 ++-
 arch/powerpc/kernel/process.c         |  5 ++++-
 arch/powerpc/mm/book3s64/hash_utils.c |  5 +++--
 arch/powerpc/mm/fault.c               | 21 +++++++++++-------
 arch/powerpc/perf/core-book3s.c       |  5 +++--
 arch/powerpc/xmon/xmon.c              | 16 +++++++++++---
 9 files changed, 78 insertions(+), 21 deletions(-)
 create mode 100644 arch/powerpc/include/asm/traps.h

diff --git a/arch/powerpc/include/asm/interrupt.h b/arch/powerpc/include/asm/interrupt.h
index 7c633896d758..5ce9898bc9a6 100644
--- a/arch/powerpc/include/asm/interrupt.h
+++ b/arch/powerpc/include/asm/interrupt.h
@@ -8,6 +8,7 @@
 #include <asm/ftrace.h>
 #include <asm/kprobes.h>
 #include <asm/runlatch.h>
+#include <asm/traps.h>
 
 struct interrupt_state {
 #ifdef CONFIG_PPC_BOOK3E_64
@@ -59,7 +60,7 @@ static inline void interrupt_enter_prepare(struct pt_regs *regs, struct interrup
 		 * CT_WARN_ON comes here via program_check_exception,
 		 * so avoid recursion.
 		 */
-		if (TRAP(regs) != 0x700)
+		if (TRAP(regs) != INTERRUPT_PROGRAM)
 			CT_WARN_ON(ct_state() != CONTEXT_KERNEL);
 	}
 #endif
@@ -156,7 +157,8 @@ static inline void interrupt_nmi_enter_prepare(struct pt_regs *regs, struct inte
 	/* Don't do any per-CPU operations until interrupt state is fixed */
 #endif
 	/* Allow DEC and PMI to be traced when they are soft-NMI */
-	if (TRAP(regs) != 0x900 && TRAP(regs) != 0xf00 && TRAP(regs) != 0x260) {
+	if (TRAP(regs) != INTERRUPT_DECREMENTER &&
+	    TRAP(regs) != INTERRUPT_PERFMON) {
 		state->ftrace_enabled = this_cpu_get_ftrace_enabled();
 		this_cpu_set_ftrace_enabled(0);
 	}
@@ -180,7 +182,8 @@ static inline void interrupt_nmi_exit_prepare(struct pt_regs *regs, struct inter
 		nmi_exit();
 
 #ifdef CONFIG_PPC64
-	if (TRAP(regs) != 0x900 && TRAP(regs) != 0xf00 && TRAP(regs) != 0x260)
+	if (TRAP(regs) != INTERRUPT_DECREMENTER &&
+	    TRAP(regs) != INTERRUPT_PERFMON)
 		this_cpu_set_ftrace_enabled(state->ftrace_enabled);
 
 #ifdef CONFIG_PPC_BOOK3S_64
diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
index f10498e1b3f6..7a17e0365d43 100644
--- a/arch/powerpc/include/asm/ptrace.h
+++ b/arch/powerpc/include/asm/ptrace.h
@@ -21,6 +21,7 @@
 
 #include <uapi/asm/ptrace.h>
 #include <asm/asm-const.h>
+#include <asm/traps.h>
 
 #ifndef __ASSEMBLY__
 struct pt_regs
@@ -237,7 +238,7 @@ static inline bool trap_is_unsupported_scv(struct pt_regs *regs)
 
 static inline bool trap_is_syscall(struct pt_regs *regs)
 {
-	return (trap_is_scv(regs) || TRAP(regs) == 0xc00);
+	return (trap_is_scv(regs) || TRAP(regs) == INTERRUPT_SYSCALL);
 }
 
 static inline bool trap_norestart(struct pt_regs *regs)
diff --git a/arch/powerpc/include/asm/traps.h b/arch/powerpc/include/asm/traps.h
new file mode 100644
index 000000000000..cb416a17097c
--- /dev/null
+++ b/arch/powerpc/include/asm/traps.h
@@ -0,0 +1,32 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_PPC_TRAPS_H
+#define _ASM_PPC_TRAPS_H
+
+#if defined(CONFIG_BOOKE) || defined(CONFIG_4xx)
+#define INTERRUPT_MACHINE_CHECK   0x000
+#define INTERRUPT_CRITICAL_INPUT  0x100
+#define INTERRUPT_ALTIVEC_UNAVAIL 0x200
+#define INTERRUPT_PERFMON         0x260
+#define INTERRUPT_DOORBELL        0x280
+#define INTERRUPT_DEBUG           0xd00
+#elif defined(CONFIG_PPC_BOOK3S)
+#define INTERRUPT_SYSTEM_RESET    0x100
+#define INTERRUPT_MACHINE_CHECK   0x200
+#define INTERRUPT_DATA_SEGMENT    0x380
+#define INTERRUPT_INST_SEGMENT    0x480
+#define INTERRUPT_DOORBELL        0xa00
+#define INTERRUPT_TRACE           0xd00
+#define INTERRUPT_H_DATA_STORAGE  0xe00
+#define INTERRUPT_PERFMON         0xf00
+#define INTERRUPT_H_FAC_UNAVAIL   0xf80
+#endif
+
+#define INTERRUPT_DATA_STORAGE    0x300
+#define INTERRUPT_INST_STORAGE    0x400
+#define INTERRUPT_ALIGNMENT       0x600
+#define INTERRUPT_PROGRAM         0x700
+#define INTERRUPT_FP_UNAVAIL      0x800
+#define INTERRUPT_DECREMENTER     0x900
+#define INTERRUPT_SYSCALL         0xc00
+
+#endif /* _ASM_PPC_TRAPS_H */
diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
index c4dd4b8f9cfa..72689f7ca7c8 100644
--- a/arch/powerpc/kernel/interrupt.c
+++ b/arch/powerpc/kernel/interrupt.c
@@ -19,6 +19,7 @@
 #include <asm/syscall.h>
 #include <asm/time.h>
 #include <asm/unistd.h>
+#include <asm/traps.h>
 
 #if defined(CONFIG_PPC_ADV_DEBUG_REGS) && defined(CONFIG_PPC32)
 unsigned long global_dbcr0[NR_CPUS];
@@ -456,7 +457,7 @@ notrace unsigned long interrupt_exit_kernel_prepare(struct pt_regs *regs, unsign
 	 * CT_WARN_ON comes here via program_check_exception,
 	 * so avoid recursion.
 	 */
-	if (TRAP(regs) != 0x700)
+	if (TRAP(regs) != INTERRUPT_PROGRAM)
 		CT_WARN_ON(ct_state() == CONTEXT_USER);
 
 	kuap = kuap_get_and_assert_locked();
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index b966c8e0cead..92cd49427b2f 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -64,6 +64,7 @@
 #include <asm/asm-prototypes.h>
 #include <asm/stacktrace.h>
 #include <asm/hw_breakpoint.h>
+#include <asm/traps.h>
 
 #include <linux/kprobes.h>
 #include <linux/kdebug.h>
@@ -1469,7 +1470,9 @@ static void __show_regs(struct pt_regs *regs)
 	trap = TRAP(regs);
 	if (!trap_is_syscall(regs) && cpu_has_feature(CPU_FTR_CFAR))
 		pr_cont("CFAR: "REG" ", regs->orig_gpr3);
-	if (trap == 0x200 || trap == 0x300 || trap == 0x600) {
+	if (trap == INTERRUPT_MACHINE_CHECK ||
+	    trap == INTERRUPT_DATA_STORAGE ||
+	    trap == INTERRUPT_ALIGNMENT) {
 		if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
 			pr_cont("DEAR: "REG" ESR: "REG" ", regs->dar, regs->dsisr);
 		else
diff --git a/arch/powerpc/mm/book3s64/hash_utils.c b/arch/powerpc/mm/book3s64/hash_utils.c
index 7719995323c3..2bf06e01b309 100644
--- a/arch/powerpc/mm/book3s64/hash_utils.c
+++ b/arch/powerpc/mm/book3s64/hash_utils.c
@@ -64,6 +64,7 @@
 #include <asm/pte-walk.h>
 #include <asm/asm-prototypes.h>
 #include <asm/ultravisor.h>
+#include <asm/traps.h>
 
 #include <mm/mmu_decl.h>
 
@@ -1145,7 +1146,7 @@ unsigned int hash_page_do_lazy_icache(unsigned int pp, pte_t pte, int trap)
 
 	/* page is dirty */
 	if (!test_bit(PG_dcache_clean, &page->flags) && !PageReserved(page)) {
-		if (trap == 0x400) {
+		if (trap == INTERRUPT_INST_STORAGE) {
 			flush_dcache_icache_page(page);
 			set_bit(PG_dcache_clean, &page->flags);
 		} else
@@ -1545,7 +1546,7 @@ DEFINE_INTERRUPT_HANDLER_RET(__do_hash_fault)
 	if (user_mode(regs) || (region_id == USER_REGION_ID))
 		access &= ~_PAGE_PRIVILEGED;
 
-	if (TRAP(regs) == 0x400)
+	if (TRAP(regs) == INTERRUPT_INST_STORAGE)
 		access |= _PAGE_EXEC;
 
 	err = hash_page_mm(mm, ea, access, TRAP(regs), flags);
diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index 0c0b1c2cfb49..641b3feef7ee 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -44,6 +44,7 @@
 #include <asm/debug.h>
 #include <asm/kup.h>
 #include <asm/inst.h>
+#include <asm/traps.h>
 
 
 /*
@@ -197,7 +198,7 @@ static int mm_fault_error(struct pt_regs *regs, unsigned long addr,
 static bool bad_kernel_fault(struct pt_regs *regs, unsigned long error_code,
 			     unsigned long address, bool is_write)
 {
-	int is_exec = TRAP(regs) == 0x400;
+	int is_exec = TRAP(regs) == INTERRUPT_INST_STORAGE;
 
 	/* NX faults set DSISR_PROTFAULT on the 8xx, DSISR_NOEXEC_OR_G on others */
 	if (is_exec && (error_code & (DSISR_NOEXEC_OR_G | DSISR_KEYFAULT |
@@ -391,7 +392,7 @@ static int ___do_page_fault(struct pt_regs *regs, unsigned long address,
 	struct vm_area_struct * vma;
 	struct mm_struct *mm = current->mm;
 	unsigned int flags = FAULT_FLAG_DEFAULT;
- 	int is_exec = TRAP(regs) == 0x400;
+	int is_exec = TRAP(regs) == INTERRUPT_INST_STORAGE;
 	int is_user = user_mode(regs);
 	int is_write = page_fault_is_write(error_code);
 	vm_fault_t fault, major = 0;
@@ -588,20 +589,24 @@ void __bad_page_fault(struct pt_regs *regs, int sig)
 	/* kernel has accessed a bad area */
 
 	switch (TRAP(regs)) {
-	case 0x300:
-	case 0x380:
-	case 0xe00:
+	case INTERRUPT_DATA_STORAGE:
+#ifdef CONFIG_PPC_BOOK3S
+	case INTERRUPT_DATA_SEGMENT:
+	case INTERRUPT_H_DATA_STORAGE:
+#endif
 		pr_alert("BUG: %s on %s at 0x%08lx\n",
 			 regs->dar < PAGE_SIZE ? "Kernel NULL pointer dereference" :
 			 "Unable to handle kernel data access",
 			 is_write ? "write" : "read", regs->dar);
 		break;
-	case 0x400:
-	case 0x480:
+	case INTERRUPT_INST_STORAGE:
+#ifdef CONFIG_PPC_BOOK3S
+	case INTERRUPT_INST_SEGMENT:
+#endif
 		pr_alert("BUG: Unable to handle kernel instruction fetch%s",
 			 regs->nip < PAGE_SIZE ? " (NULL pointer?)\n" : "\n");
 		break;
-	case 0x600:
+	case INTERRUPT_ALIGNMENT:
 		pr_alert("BUG: Unable to handle kernel unaligned access at 0x%08lx\n",
 			 regs->dar);
 		break;
diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
index 766f064f00fb..6e34f5bba232 100644
--- a/arch/powerpc/perf/core-book3s.c
+++ b/arch/powerpc/perf/core-book3s.c
@@ -17,6 +17,7 @@
 #include <asm/firmware.h>
 #include <asm/ptrace.h>
 #include <asm/code-patching.h>
+#include <asm/traps.h>
 
 #ifdef CONFIG_PPC64
 #include "internal.h"
@@ -168,7 +169,7 @@ static bool regs_use_siar(struct pt_regs *regs)
 	 * they have not been setup using perf_read_regs() and so regs->result
 	 * is something random.
 	 */
-	return ((TRAP(regs) == 0xf00) && regs->result);
+	return ((TRAP(regs) == INTERRUPT_PERFMON) && regs->result);
 }
 
 /*
@@ -347,7 +348,7 @@ static inline void perf_read_regs(struct pt_regs *regs)
 	 * hypervisor samples as well as samples in the kernel with
 	 * interrupts off hence the userspace check.
 	 */
-	if (TRAP(regs) != 0xf00)
+	if (TRAP(regs) != INTERRUPT_PERFMON)
 		use_siar = 0;
 	else if ((ppmu->flags & PPMU_NO_SIAR))
 		use_siar = 0;
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index bf7d69625a2e..2a4f99e64bf3 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -54,6 +54,7 @@
 #include <asm/code-patching.h>
 #include <asm/sections.h>
 #include <asm/inst.h>
+#include <asm/traps.h>
 
 #ifdef CONFIG_PPC64
 #include <asm/hvcall.h>
@@ -1769,7 +1770,12 @@ static void excprint(struct pt_regs *fp)
 	printf("    sp: %lx\n", fp->gpr[1]);
 	printf("   msr: %lx\n", fp->msr);
 
-	if (trap == 0x300 || trap == 0x380 || trap == 0x600 || trap == 0x200) {
+	if (trap == INTERRUPT_DATA_STORAGE ||
+#ifdef CONFIG_PPC_BOOK3S
+	    trap == INTERRUPT_DATA_SEGMENT ||
+#endif
+	    trap == INTERRUPT_ALIGNMENT ||
+	    trap == INTERRUPT_MACHINE_CHECK) {
 		printf("   dar: %lx\n", fp->dar);
 		if (trap != 0x380)
 			printf(" dsisr: %lx\n", fp->dsisr);
@@ -1785,7 +1791,7 @@ static void excprint(struct pt_regs *fp)
 		       current->pid, current->comm);
 	}
 
-	if (trap == 0x700)
+	if (trap == INTERRUPT_PROGRAM)
 		print_bug_trap(fp);
 
 	printf(linux_banner);
@@ -1846,7 +1852,11 @@ static void prregs(struct pt_regs *fp)
 	printf("ctr = "REG"   xer = "REG"   trap = %4lx\n",
 	       fp->ctr, fp->xer, fp->trap);
 	trap = TRAP(fp);
-	if (trap == 0x300 || trap == 0x380 || trap == 0x600)
+	if (trap == INTERRUPT_DATA_STORAGE ||
+#ifdef CONFIG_PPC_BOOK3S
+	    trap == INTERRUPT_DATA_SEGMENT ||
+#endif
+	    trap == INTERRUPT_ALIGNMENT)
 		printf("dar = "REG"   dsisr = %.8lx\n", fp->dar, fp->dsisr);
 }
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] powerpc/traps: Enhance readability for trap types
  2021-04-08 14:07 ` Xiongwei Song
  (?)
@ 2021-04-08 16:54 ` kernel test robot
  -1 siblings, 0 replies; 17+ messages in thread
From: kernel test robot @ 2021-04-08 16:54 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 5804 bytes --]

Hi Xiongwei,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on powerpc/next]
[also build test ERROR on next-20210408]
[cannot apply to hnaz-linux-mm/master v5.12-rc6]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Xiongwei-Song/powerpc-traps-Enhance-readability-for-trap-types/20210408-221152
base:   https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc64-randconfig-r004-20210408 (attached as .config)
compiler: powerpc-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/d9dd965937ed76338a90b73b94190499a98cb5d1
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Xiongwei-Song/powerpc-traps-Enhance-readability-for-trap-types/20210408-221152
        git checkout d9dd965937ed76338a90b73b94190499a98cb5d1
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=powerpc64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from include/linux/kernel.h:10,
                    from include/linux/list.h:9,
                    from include/linux/rculist.h:10,
                    from include/linux/pid.h:5,
                    from include/linux/sched.h:14,
                    from arch/powerpc/kernel/process.c:14:
   arch/powerpc/kernel/process.c: In function '__show_regs':
>> arch/powerpc/kernel/process.c:1473:14: error: 'INTERRUPT_MACHINE_CHECK' undeclared (first use in this function); did you mean 'TAINT_MACHINE_CHECK'?
    1473 |  if (trap == INTERRUPT_MACHINE_CHECK ||
         |              ^~~~~~~~~~~~~~~~~~~~~~~
   include/linux/compiler.h:58:52: note: in definition of macro '__trace_if_var'
      58 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
         |                                                    ^~~~
   arch/powerpc/kernel/process.c:1473:2: note: in expansion of macro 'if'
    1473 |  if (trap == INTERRUPT_MACHINE_CHECK ||
         |  ^~
   arch/powerpc/kernel/process.c:1473:14: note: each undeclared identifier is reported only once for each function it appears in
    1473 |  if (trap == INTERRUPT_MACHINE_CHECK ||
         |              ^~~~~~~~~~~~~~~~~~~~~~~
   include/linux/compiler.h:58:52: note: in definition of macro '__trace_if_var'
      58 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
         |                                                    ^~~~
   arch/powerpc/kernel/process.c:1473:2: note: in expansion of macro 'if'
    1473 |  if (trap == INTERRUPT_MACHINE_CHECK ||
         |  ^~
--
   In file included from include/linux/kernel.h:10,
                    from arch/powerpc/xmon/xmon.c:10:
   arch/powerpc/xmon/xmon.c: In function 'excprint':
>> arch/powerpc/xmon/xmon.c:1778:14: error: 'INTERRUPT_MACHINE_CHECK' undeclared (first use in this function); did you mean 'TAINT_MACHINE_CHECK'?
    1778 |      trap == INTERRUPT_MACHINE_CHECK) {
         |              ^~~~~~~~~~~~~~~~~~~~~~~
   include/linux/compiler.h:58:52: note: in definition of macro '__trace_if_var'
      58 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
         |                                                    ^~~~
   arch/powerpc/xmon/xmon.c:1773:2: note: in expansion of macro 'if'
    1773 |  if (trap == INTERRUPT_DATA_STORAGE ||
         |  ^~
   arch/powerpc/xmon/xmon.c:1778:14: note: each undeclared identifier is reported only once for each function it appears in
    1778 |      trap == INTERRUPT_MACHINE_CHECK) {
         |              ^~~~~~~~~~~~~~~~~~~~~~~
   include/linux/compiler.h:58:52: note: in definition of macro '__trace_if_var'
      58 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
         |                                                    ^~~~
   arch/powerpc/xmon/xmon.c:1773:2: note: in expansion of macro 'if'
    1773 |  if (trap == INTERRUPT_DATA_STORAGE ||
         |  ^~


vim +1473 arch/powerpc/kernel/process.c

  1458	
  1459	static void __show_regs(struct pt_regs *regs)
  1460	{
  1461		int i, trap;
  1462	
  1463		printk("NIP:  "REG" LR: "REG" CTR: "REG"\n",
  1464		       regs->nip, regs->link, regs->ctr);
  1465		printk("REGS: %px TRAP: %04lx   %s  (%s)\n",
  1466		       regs, regs->trap, print_tainted(), init_utsname()->release);
  1467		printk("MSR:  "REG" ", regs->msr);
  1468		print_msr_bits(regs->msr);
  1469		pr_cont("  CR: %08lx  XER: %08lx\n", regs->ccr, regs->xer);
  1470		trap = TRAP(regs);
  1471		if (!trap_is_syscall(regs) && cpu_has_feature(CPU_FTR_CFAR))
  1472			pr_cont("CFAR: "REG" ", regs->orig_gpr3);
> 1473		if (trap == INTERRUPT_MACHINE_CHECK ||
  1474		    trap == INTERRUPT_DATA_STORAGE ||
  1475		    trap == INTERRUPT_ALIGNMENT) {
  1476			if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
  1477				pr_cont("DEAR: "REG" ESR: "REG" ", regs->dar, regs->dsisr);
  1478			else
  1479				pr_cont("DAR: "REG" DSISR: %08lx ", regs->dar, regs->dsisr);
  1480		}
  1481	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 27902 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] powerpc/traps: Enhance readability for trap types
  2021-04-08 14:07 ` Xiongwei Song
@ 2021-04-09 16:14   ` Christophe Leroy
  -1 siblings, 0 replies; 17+ messages in thread
From: Christophe Leroy @ 2021-04-09 16:14 UTC (permalink / raw)
  To: Xiongwei Song, mpe, benh, paulus, oleg, npiggin, aneesh.kumar,
	ravi.bangoria, mikey, haren, akpm, rppt, jniethe5, atrajeev,
	maddy, peterz, kjain, kan.liang, aik, alistair, pmladek,
	john.ogness
  Cc: linuxppc-dev, linux-kernel, Xiongwei Song



Le 08/04/2021 à 16:07, Xiongwei Song a écrit :
> From: Xiongwei Song <sxwjean@gmail.com>
> 
> Create a new header named traps.h, define macros to list ppc interrupt
> types in traps.h, replace the reference of the trap hex values with these
> macros.
> 
> Referred the hex number in arch/powerpc/kernel/exceptions-64e.S,
> arch/powerpc/kernel/exceptions-64s.S and
> arch/powerpc/include/asm/kvm_asm.h.
> 
> v2-v3:
> Correct the prefix of trap macros with INTERRUPT_, the previous prefix
> is TRAP_, which is not precise. This is suggested by Segher Boessenkool
> and Nicholas Piggin.
> 
> v1-v2:
> Define more trap macros to replace more trap hexs in code, not just for
> the __show_regs function. This is suggested by Christophe Leroy.
> 
> Signed-off-by: Xiongwei Song <sxwjean@gmail.com>
> ---
>   arch/powerpc/include/asm/interrupt.h  |  9 +++++---
>   arch/powerpc/include/asm/ptrace.h     |  3 ++-
>   arch/powerpc/include/asm/traps.h      | 32 +++++++++++++++++++++++++++
>   arch/powerpc/kernel/interrupt.c       |  3 ++-
>   arch/powerpc/kernel/process.c         |  5 ++++-
>   arch/powerpc/mm/book3s64/hash_utils.c |  5 +++--
>   arch/powerpc/mm/fault.c               | 21 +++++++++++-------
>   arch/powerpc/perf/core-book3s.c       |  5 +++--
>   arch/powerpc/xmon/xmon.c              | 16 +++++++++++---
>   9 files changed, 78 insertions(+), 21 deletions(-)
>   create mode 100644 arch/powerpc/include/asm/traps.h
> 
> diff --git a/arch/powerpc/include/asm/interrupt.h b/arch/powerpc/include/asm/interrupt.h
> index 7c633896d758..5ce9898bc9a6 100644
> --- a/arch/powerpc/include/asm/interrupt.h
> +++ b/arch/powerpc/include/asm/interrupt.h
> @@ -8,6 +8,7 @@
>   #include <asm/ftrace.h>
>   #include <asm/kprobes.h>
>   #include <asm/runlatch.h>
> +#include <asm/traps.h>
>   
>   struct interrupt_state {
>   #ifdef CONFIG_PPC_BOOK3E_64
> @@ -59,7 +60,7 @@ static inline void interrupt_enter_prepare(struct pt_regs *regs, struct interrup
>   		 * CT_WARN_ON comes here via program_check_exception,
>   		 * so avoid recursion.
>   		 */
> -		if (TRAP(regs) != 0x700)
> +		if (TRAP(regs) != INTERRUPT_PROGRAM)
>   			CT_WARN_ON(ct_state() != CONTEXT_KERNEL);
>   	}
>   #endif
> @@ -156,7 +157,8 @@ static inline void interrupt_nmi_enter_prepare(struct pt_regs *regs, struct inte
>   	/* Don't do any per-CPU operations until interrupt state is fixed */
>   #endif
>   	/* Allow DEC and PMI to be traced when they are soft-NMI */
> -	if (TRAP(regs) != 0x900 && TRAP(regs) != 0xf00 && TRAP(regs) != 0x260) {
> +	if (TRAP(regs) != INTERRUPT_DECREMENTER &&
> +	    TRAP(regs) != INTERRUPT_PERFMON) {

I think too long names hinder readability, see later for suggestions.

>   		state->ftrace_enabled = this_cpu_get_ftrace_enabled();
>   		this_cpu_set_ftrace_enabled(0);
>   	}
> @@ -180,7 +182,8 @@ static inline void interrupt_nmi_exit_prepare(struct pt_regs *regs, struct inter
>   		nmi_exit();
>   
>   #ifdef CONFIG_PPC64
> -	if (TRAP(regs) != 0x900 && TRAP(regs) != 0xf00 && TRAP(regs) != 0x260)
> +	if (TRAP(regs) != INTERRUPT_DECREMENTER &&
> +	    TRAP(regs) != INTERRUPT_PERFMON)
>   		this_cpu_set_ftrace_enabled(state->ftrace_enabled);
>   
>   #ifdef CONFIG_PPC_BOOK3S_64
> diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
> index f10498e1b3f6..7a17e0365d43 100644
> --- a/arch/powerpc/include/asm/ptrace.h
> +++ b/arch/powerpc/include/asm/ptrace.h
> @@ -21,6 +21,7 @@
>   
>   #include <uapi/asm/ptrace.h>
>   #include <asm/asm-const.h>
> +#include <asm/traps.h>
>   
>   #ifndef __ASSEMBLY__
>   struct pt_regs
> @@ -237,7 +238,7 @@ static inline bool trap_is_unsupported_scv(struct pt_regs *regs)
>   
>   static inline bool trap_is_syscall(struct pt_regs *regs)
>   {
> -	return (trap_is_scv(regs) || TRAP(regs) == 0xc00);
> +	return (trap_is_scv(regs) || TRAP(regs) == INTERRUPT_SYSCALL);
>   }
>   
>   static inline bool trap_norestart(struct pt_regs *regs)
> diff --git a/arch/powerpc/include/asm/traps.h b/arch/powerpc/include/asm/traps.h
> new file mode 100644
> index 000000000000..cb416a17097c
> --- /dev/null
> +++ b/arch/powerpc/include/asm/traps.h
> @@ -0,0 +1,32 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _ASM_PPC_TRAPS_H
> +#define _ASM_PPC_TRAPS_H
> +
> +#if defined(CONFIG_BOOKE) || defined(CONFIG_4xx)
> +#define INTERRUPT_MACHINE_CHECK   0x000

I'd prefer shorted names in order to not be obliged to split lines.
Here are some suggestions:

INT_MCE

> +#define INTERRUPT_CRITICAL_INPUT  0x100

INT_CRIT

> +#define INTERRUPT_ALTIVEC_UNAVAIL 0x200
> +#define INTERRUPT_PERFMON         0x260

INT_PERF

> +#define INTERRUPT_DOORBELL        0x280
> +#define INTERRUPT_DEBUG           0xd00
> +#elif defined(CONFIG_PPC_BOOK3S)
> +#define INTERRUPT_SYSTEM_RESET    0x100

INT_SRESET

> +#define INTERRUPT_MACHINE_CHECK   0x200

INT_MCE

> +#define INTERRUPT_DATA_SEGMENT    0x380

INT_DSEG

> +#define INTERRUPT_INST_SEGMENT    0x480

INT_ISEG

> +#define INTERRUPT_DOORBELL        0xa00

INT_DBELL

> +#define INTERRUPT_TRACE           0xd00

INT_TRACE

> +#define INTERRUPT_H_DATA_STORAGE  0xe00
> +#define INTERRUPT_PERFMON         0xf00

INT_PERF

> +#define INTERRUPT_H_FAC_UNAVAIL   0xf80
> +#endif
> +
> +#define INTERRUPT_DATA_STORAGE    0x300

INT_DSI

> +#define INTERRUPT_INST_STORAGE    0x400

INT_ISI

> +#define INTERRUPT_ALIGNMENT       0x600

INT_ALIGN

> +#define INTERRUPT_PROGRAM         0x700

INT_PROG

> +#define INTERRUPT_FP_UNAVAIL      0x800

INT_FP_UNAVAIL

> +#define INTERRUPT_DECREMENTER     0x900

INT_DEC

> +#define INTERRUPT_SYSCALL         0xc00

INT_SYSCALL


> +
> +#endif /* _ASM_PPC_TRAPS_H */

...

> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index 0c0b1c2cfb49..641b3feef7ee 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -44,6 +44,7 @@
>   #include <asm/debug.h>
>   #include <asm/kup.h>
>   #include <asm/inst.h>
> +#include <asm/traps.h>
>   
>   
>   /*
> @@ -197,7 +198,7 @@ static int mm_fault_error(struct pt_regs *regs, unsigned long addr,
>   static bool bad_kernel_fault(struct pt_regs *regs, unsigned long error_code,
>   			     unsigned long address, bool is_write)
>   {
> -	int is_exec = TRAP(regs) == 0x400;
> +	int is_exec = TRAP(regs) == INTERRUPT_INST_STORAGE;
>   
>   	/* NX faults set DSISR_PROTFAULT on the 8xx, DSISR_NOEXEC_OR_G on others */
>   	if (is_exec && (error_code & (DSISR_NOEXEC_OR_G | DSISR_KEYFAULT |
> @@ -391,7 +392,7 @@ static int ___do_page_fault(struct pt_regs *regs, unsigned long address,
>   	struct vm_area_struct * vma;
>   	struct mm_struct *mm = current->mm;
>   	unsigned int flags = FAULT_FLAG_DEFAULT;
> - 	int is_exec = TRAP(regs) == 0x400;
> +	int is_exec = TRAP(regs) == INTERRUPT_INST_STORAGE;
>   	int is_user = user_mode(regs);
>   	int is_write = page_fault_is_write(error_code);
>   	vm_fault_t fault, major = 0;
> @@ -588,20 +589,24 @@ void __bad_page_fault(struct pt_regs *regs, int sig)
>   	/* kernel has accessed a bad area */
>   
>   	switch (TRAP(regs)) {
> -	case 0x300:
> -	case 0x380:
> -	case 0xe00:
> +	case INTERRUPT_DATA_STORAGE:
> +#ifdef CONFIG_PPC_BOOK3S
> +	case INTERRUPT_DATA_SEGMENT:
> +	case INTERRUPT_H_DATA_STORAGE:
> +#endif

It would be better to avoid #ifdefs when none where necessary before.


>   		pr_alert("BUG: %s on %s at 0x%08lx\n",
>   			 regs->dar < PAGE_SIZE ? "Kernel NULL pointer dereference" :
>   			 "Unable to handle kernel data access",
>   			 is_write ? "write" : "read", regs->dar);
>   		break;
> -	case 0x400:
> -	case 0x480:
> +	case INTERRUPT_INST_STORAGE:
> +#ifdef CONFIG_PPC_BOOK3S
> +	case INTERRUPT_INST_SEGMENT:
> +#endif

It would be better to avoid #ifdefs when none where necessary before.



>   		pr_alert("BUG: Unable to handle kernel instruction fetch%s",
>   			 regs->nip < PAGE_SIZE ? " (NULL pointer?)\n" : "\n");
>   		break;
> -	case 0x600:
> +	case INTERRUPT_ALIGNMENT:
>   		pr_alert("BUG: Unable to handle kernel unaligned access at 0x%08lx\n",
>   			 regs->dar);
>   		break;
> diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
> index 766f064f00fb..6e34f5bba232 100644
> --- a/arch/powerpc/perf/core-book3s.c
> +++ b/arch/powerpc/perf/core-book3s.c
> @@ -17,6 +17,7 @@
>   #include <asm/firmware.h>
>   #include <asm/ptrace.h>
>   #include <asm/code-patching.h>
> +#include <asm/traps.h>
>   
>   #ifdef CONFIG_PPC64
>   #include "internal.h"
> @@ -168,7 +169,7 @@ static bool regs_use_siar(struct pt_regs *regs)
>   	 * they have not been setup using perf_read_regs() and so regs->result
>   	 * is something random.
>   	 */
> -	return ((TRAP(regs) == 0xf00) && regs->result);
> +	return ((TRAP(regs) == INTERRUPT_PERFMON) && regs->result);
>   }
>   
>   /*
> @@ -347,7 +348,7 @@ static inline void perf_read_regs(struct pt_regs *regs)
>   	 * hypervisor samples as well as samples in the kernel with
>   	 * interrupts off hence the userspace check.
>   	 */
> -	if (TRAP(regs) != 0xf00)
> +	if (TRAP(regs) != INTERRUPT_PERFMON)
>   		use_siar = 0;
>   	else if ((ppmu->flags & PPMU_NO_SIAR))
>   		use_siar = 0;
> diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
> index bf7d69625a2e..2a4f99e64bf3 100644
> --- a/arch/powerpc/xmon/xmon.c
> +++ b/arch/powerpc/xmon/xmon.c
> @@ -54,6 +54,7 @@
>   #include <asm/code-patching.h>
>   #include <asm/sections.h>
>   #include <asm/inst.h>
> +#include <asm/traps.h>
>   
>   #ifdef CONFIG_PPC64
>   #include <asm/hvcall.h>
> @@ -1769,7 +1770,12 @@ static void excprint(struct pt_regs *fp)
>   	printf("    sp: %lx\n", fp->gpr[1]);
>   	printf("   msr: %lx\n", fp->msr);
>   
> -	if (trap == 0x300 || trap == 0x380 || trap == 0x600 || trap == 0x200) {
> +	if (trap == INTERRUPT_DATA_STORAGE ||
> +#ifdef CONFIG_PPC_BOOK3S
> +	    trap == INTERRUPT_DATA_SEGMENT ||
> +#endif
It would be better to avoid #ifdefs when none where necessary before.

And an #ifdef in the middle of a code line is awful for readability and maintainability.

> +	    trap == INTERRUPT_ALIGNMENT ||
> +	    trap == INTERRUPT_MACHINE_CHECK) {
>   		printf("   dar: %lx\n", fp->dar);
>   		if (trap != 0x380)
>   			printf(" dsisr: %lx\n", fp->dsisr);
> @@ -1785,7 +1791,7 @@ static void excprint(struct pt_regs *fp)
>   		       current->pid, current->comm);
>   	}
>   
> -	if (trap == 0x700)
> +	if (trap == INTERRUPT_PROGRAM)
>   		print_bug_trap(fp);
>   
>   	printf(linux_banner);
> @@ -1846,7 +1852,11 @@ static void prregs(struct pt_regs *fp)
>   	printf("ctr = "REG"   xer = "REG"   trap = %4lx\n",
>   	       fp->ctr, fp->xer, fp->trap);
>   	trap = TRAP(fp);
> -	if (trap == 0x300 || trap == 0x380 || trap == 0x600)
> +	if (trap == INTERRUPT_DATA_STORAGE ||
> +#ifdef CONFIG_PPC_BOOK3S
> +	    trap == INTERRUPT_DATA_SEGMENT ||
> +#endif
> +	    trap == INTERRUPT_ALIGNMENT)

It would be better to avoid #ifdefs when none where necessary before.

And an #ifdef in the middle of a code line is awful for readability and maintainability.


>   		printf("dar = "REG"   dsisr = %.8lx\n", fp->dar, fp->dsisr);
>   }
>   
> 

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] powerpc/traps: Enhance readability for trap types
@ 2021-04-09 16:14   ` Christophe Leroy
  0 siblings, 0 replies; 17+ messages in thread
From: Christophe Leroy @ 2021-04-09 16:14 UTC (permalink / raw)
  To: Xiongwei Song, mpe, benh, paulus, oleg, npiggin, aneesh.kumar,
	ravi.bangoria, mikey, haren, akpm, rppt, jniethe5, atrajeev,
	maddy, peterz, kjain, kan.liang, aik, alistair, pmladek,
	john.ogness
  Cc: Xiongwei Song, linuxppc-dev, linux-kernel



Le 08/04/2021 à 16:07, Xiongwei Song a écrit :
> From: Xiongwei Song <sxwjean@gmail.com>
> 
> Create a new header named traps.h, define macros to list ppc interrupt
> types in traps.h, replace the reference of the trap hex values with these
> macros.
> 
> Referred the hex number in arch/powerpc/kernel/exceptions-64e.S,
> arch/powerpc/kernel/exceptions-64s.S and
> arch/powerpc/include/asm/kvm_asm.h.
> 
> v2-v3:
> Correct the prefix of trap macros with INTERRUPT_, the previous prefix
> is TRAP_, which is not precise. This is suggested by Segher Boessenkool
> and Nicholas Piggin.
> 
> v1-v2:
> Define more trap macros to replace more trap hexs in code, not just for
> the __show_regs function. This is suggested by Christophe Leroy.
> 
> Signed-off-by: Xiongwei Song <sxwjean@gmail.com>
> ---
>   arch/powerpc/include/asm/interrupt.h  |  9 +++++---
>   arch/powerpc/include/asm/ptrace.h     |  3 ++-
>   arch/powerpc/include/asm/traps.h      | 32 +++++++++++++++++++++++++++
>   arch/powerpc/kernel/interrupt.c       |  3 ++-
>   arch/powerpc/kernel/process.c         |  5 ++++-
>   arch/powerpc/mm/book3s64/hash_utils.c |  5 +++--
>   arch/powerpc/mm/fault.c               | 21 +++++++++++-------
>   arch/powerpc/perf/core-book3s.c       |  5 +++--
>   arch/powerpc/xmon/xmon.c              | 16 +++++++++++---
>   9 files changed, 78 insertions(+), 21 deletions(-)
>   create mode 100644 arch/powerpc/include/asm/traps.h
> 
> diff --git a/arch/powerpc/include/asm/interrupt.h b/arch/powerpc/include/asm/interrupt.h
> index 7c633896d758..5ce9898bc9a6 100644
> --- a/arch/powerpc/include/asm/interrupt.h
> +++ b/arch/powerpc/include/asm/interrupt.h
> @@ -8,6 +8,7 @@
>   #include <asm/ftrace.h>
>   #include <asm/kprobes.h>
>   #include <asm/runlatch.h>
> +#include <asm/traps.h>
>   
>   struct interrupt_state {
>   #ifdef CONFIG_PPC_BOOK3E_64
> @@ -59,7 +60,7 @@ static inline void interrupt_enter_prepare(struct pt_regs *regs, struct interrup
>   		 * CT_WARN_ON comes here via program_check_exception,
>   		 * so avoid recursion.
>   		 */
> -		if (TRAP(regs) != 0x700)
> +		if (TRAP(regs) != INTERRUPT_PROGRAM)
>   			CT_WARN_ON(ct_state() != CONTEXT_KERNEL);
>   	}
>   #endif
> @@ -156,7 +157,8 @@ static inline void interrupt_nmi_enter_prepare(struct pt_regs *regs, struct inte
>   	/* Don't do any per-CPU operations until interrupt state is fixed */
>   #endif
>   	/* Allow DEC and PMI to be traced when they are soft-NMI */
> -	if (TRAP(regs) != 0x900 && TRAP(regs) != 0xf00 && TRAP(regs) != 0x260) {
> +	if (TRAP(regs) != INTERRUPT_DECREMENTER &&
> +	    TRAP(regs) != INTERRUPT_PERFMON) {

I think too long names hinder readability, see later for suggestions.

>   		state->ftrace_enabled = this_cpu_get_ftrace_enabled();
>   		this_cpu_set_ftrace_enabled(0);
>   	}
> @@ -180,7 +182,8 @@ static inline void interrupt_nmi_exit_prepare(struct pt_regs *regs, struct inter
>   		nmi_exit();
>   
>   #ifdef CONFIG_PPC64
> -	if (TRAP(regs) != 0x900 && TRAP(regs) != 0xf00 && TRAP(regs) != 0x260)
> +	if (TRAP(regs) != INTERRUPT_DECREMENTER &&
> +	    TRAP(regs) != INTERRUPT_PERFMON)
>   		this_cpu_set_ftrace_enabled(state->ftrace_enabled);
>   
>   #ifdef CONFIG_PPC_BOOK3S_64
> diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
> index f10498e1b3f6..7a17e0365d43 100644
> --- a/arch/powerpc/include/asm/ptrace.h
> +++ b/arch/powerpc/include/asm/ptrace.h
> @@ -21,6 +21,7 @@
>   
>   #include <uapi/asm/ptrace.h>
>   #include <asm/asm-const.h>
> +#include <asm/traps.h>
>   
>   #ifndef __ASSEMBLY__
>   struct pt_regs
> @@ -237,7 +238,7 @@ static inline bool trap_is_unsupported_scv(struct pt_regs *regs)
>   
>   static inline bool trap_is_syscall(struct pt_regs *regs)
>   {
> -	return (trap_is_scv(regs) || TRAP(regs) == 0xc00);
> +	return (trap_is_scv(regs) || TRAP(regs) == INTERRUPT_SYSCALL);
>   }
>   
>   static inline bool trap_norestart(struct pt_regs *regs)
> diff --git a/arch/powerpc/include/asm/traps.h b/arch/powerpc/include/asm/traps.h
> new file mode 100644
> index 000000000000..cb416a17097c
> --- /dev/null
> +++ b/arch/powerpc/include/asm/traps.h
> @@ -0,0 +1,32 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _ASM_PPC_TRAPS_H
> +#define _ASM_PPC_TRAPS_H
> +
> +#if defined(CONFIG_BOOKE) || defined(CONFIG_4xx)
> +#define INTERRUPT_MACHINE_CHECK   0x000

I'd prefer shorted names in order to not be obliged to split lines.
Here are some suggestions:

INT_MCE

> +#define INTERRUPT_CRITICAL_INPUT  0x100

INT_CRIT

> +#define INTERRUPT_ALTIVEC_UNAVAIL 0x200
> +#define INTERRUPT_PERFMON         0x260

INT_PERF

> +#define INTERRUPT_DOORBELL        0x280
> +#define INTERRUPT_DEBUG           0xd00
> +#elif defined(CONFIG_PPC_BOOK3S)
> +#define INTERRUPT_SYSTEM_RESET    0x100

INT_SRESET

> +#define INTERRUPT_MACHINE_CHECK   0x200

INT_MCE

> +#define INTERRUPT_DATA_SEGMENT    0x380

INT_DSEG

> +#define INTERRUPT_INST_SEGMENT    0x480

INT_ISEG

> +#define INTERRUPT_DOORBELL        0xa00

INT_DBELL

> +#define INTERRUPT_TRACE           0xd00

INT_TRACE

> +#define INTERRUPT_H_DATA_STORAGE  0xe00
> +#define INTERRUPT_PERFMON         0xf00

INT_PERF

> +#define INTERRUPT_H_FAC_UNAVAIL   0xf80
> +#endif
> +
> +#define INTERRUPT_DATA_STORAGE    0x300

INT_DSI

> +#define INTERRUPT_INST_STORAGE    0x400

INT_ISI

> +#define INTERRUPT_ALIGNMENT       0x600

INT_ALIGN

> +#define INTERRUPT_PROGRAM         0x700

INT_PROG

> +#define INTERRUPT_FP_UNAVAIL      0x800

INT_FP_UNAVAIL

> +#define INTERRUPT_DECREMENTER     0x900

INT_DEC

> +#define INTERRUPT_SYSCALL         0xc00

INT_SYSCALL


> +
> +#endif /* _ASM_PPC_TRAPS_H */

...

> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index 0c0b1c2cfb49..641b3feef7ee 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -44,6 +44,7 @@
>   #include <asm/debug.h>
>   #include <asm/kup.h>
>   #include <asm/inst.h>
> +#include <asm/traps.h>
>   
>   
>   /*
> @@ -197,7 +198,7 @@ static int mm_fault_error(struct pt_regs *regs, unsigned long addr,
>   static bool bad_kernel_fault(struct pt_regs *regs, unsigned long error_code,
>   			     unsigned long address, bool is_write)
>   {
> -	int is_exec = TRAP(regs) == 0x400;
> +	int is_exec = TRAP(regs) == INTERRUPT_INST_STORAGE;
>   
>   	/* NX faults set DSISR_PROTFAULT on the 8xx, DSISR_NOEXEC_OR_G on others */
>   	if (is_exec && (error_code & (DSISR_NOEXEC_OR_G | DSISR_KEYFAULT |
> @@ -391,7 +392,7 @@ static int ___do_page_fault(struct pt_regs *regs, unsigned long address,
>   	struct vm_area_struct * vma;
>   	struct mm_struct *mm = current->mm;
>   	unsigned int flags = FAULT_FLAG_DEFAULT;
> - 	int is_exec = TRAP(regs) == 0x400;
> +	int is_exec = TRAP(regs) == INTERRUPT_INST_STORAGE;
>   	int is_user = user_mode(regs);
>   	int is_write = page_fault_is_write(error_code);
>   	vm_fault_t fault, major = 0;
> @@ -588,20 +589,24 @@ void __bad_page_fault(struct pt_regs *regs, int sig)
>   	/* kernel has accessed a bad area */
>   
>   	switch (TRAP(regs)) {
> -	case 0x300:
> -	case 0x380:
> -	case 0xe00:
> +	case INTERRUPT_DATA_STORAGE:
> +#ifdef CONFIG_PPC_BOOK3S
> +	case INTERRUPT_DATA_SEGMENT:
> +	case INTERRUPT_H_DATA_STORAGE:
> +#endif

It would be better to avoid #ifdefs when none where necessary before.


>   		pr_alert("BUG: %s on %s at 0x%08lx\n",
>   			 regs->dar < PAGE_SIZE ? "Kernel NULL pointer dereference" :
>   			 "Unable to handle kernel data access",
>   			 is_write ? "write" : "read", regs->dar);
>   		break;
> -	case 0x400:
> -	case 0x480:
> +	case INTERRUPT_INST_STORAGE:
> +#ifdef CONFIG_PPC_BOOK3S
> +	case INTERRUPT_INST_SEGMENT:
> +#endif

It would be better to avoid #ifdefs when none where necessary before.



>   		pr_alert("BUG: Unable to handle kernel instruction fetch%s",
>   			 regs->nip < PAGE_SIZE ? " (NULL pointer?)\n" : "\n");
>   		break;
> -	case 0x600:
> +	case INTERRUPT_ALIGNMENT:
>   		pr_alert("BUG: Unable to handle kernel unaligned access at 0x%08lx\n",
>   			 regs->dar);
>   		break;
> diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
> index 766f064f00fb..6e34f5bba232 100644
> --- a/arch/powerpc/perf/core-book3s.c
> +++ b/arch/powerpc/perf/core-book3s.c
> @@ -17,6 +17,7 @@
>   #include <asm/firmware.h>
>   #include <asm/ptrace.h>
>   #include <asm/code-patching.h>
> +#include <asm/traps.h>
>   
>   #ifdef CONFIG_PPC64
>   #include "internal.h"
> @@ -168,7 +169,7 @@ static bool regs_use_siar(struct pt_regs *regs)
>   	 * they have not been setup using perf_read_regs() and so regs->result
>   	 * is something random.
>   	 */
> -	return ((TRAP(regs) == 0xf00) && regs->result);
> +	return ((TRAP(regs) == INTERRUPT_PERFMON) && regs->result);
>   }
>   
>   /*
> @@ -347,7 +348,7 @@ static inline void perf_read_regs(struct pt_regs *regs)
>   	 * hypervisor samples as well as samples in the kernel with
>   	 * interrupts off hence the userspace check.
>   	 */
> -	if (TRAP(regs) != 0xf00)
> +	if (TRAP(regs) != INTERRUPT_PERFMON)
>   		use_siar = 0;
>   	else if ((ppmu->flags & PPMU_NO_SIAR))
>   		use_siar = 0;
> diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
> index bf7d69625a2e..2a4f99e64bf3 100644
> --- a/arch/powerpc/xmon/xmon.c
> +++ b/arch/powerpc/xmon/xmon.c
> @@ -54,6 +54,7 @@
>   #include <asm/code-patching.h>
>   #include <asm/sections.h>
>   #include <asm/inst.h>
> +#include <asm/traps.h>
>   
>   #ifdef CONFIG_PPC64
>   #include <asm/hvcall.h>
> @@ -1769,7 +1770,12 @@ static void excprint(struct pt_regs *fp)
>   	printf("    sp: %lx\n", fp->gpr[1]);
>   	printf("   msr: %lx\n", fp->msr);
>   
> -	if (trap == 0x300 || trap == 0x380 || trap == 0x600 || trap == 0x200) {
> +	if (trap == INTERRUPT_DATA_STORAGE ||
> +#ifdef CONFIG_PPC_BOOK3S
> +	    trap == INTERRUPT_DATA_SEGMENT ||
> +#endif
It would be better to avoid #ifdefs when none where necessary before.

And an #ifdef in the middle of a code line is awful for readability and maintainability.

> +	    trap == INTERRUPT_ALIGNMENT ||
> +	    trap == INTERRUPT_MACHINE_CHECK) {
>   		printf("   dar: %lx\n", fp->dar);
>   		if (trap != 0x380)
>   			printf(" dsisr: %lx\n", fp->dsisr);
> @@ -1785,7 +1791,7 @@ static void excprint(struct pt_regs *fp)
>   		       current->pid, current->comm);
>   	}
>   
> -	if (trap == 0x700)
> +	if (trap == INTERRUPT_PROGRAM)
>   		print_bug_trap(fp);
>   
>   	printf(linux_banner);
> @@ -1846,7 +1852,11 @@ static void prregs(struct pt_regs *fp)
>   	printf("ctr = "REG"   xer = "REG"   trap = %4lx\n",
>   	       fp->ctr, fp->xer, fp->trap);
>   	trap = TRAP(fp);
> -	if (trap == 0x300 || trap == 0x380 || trap == 0x600)
> +	if (trap == INTERRUPT_DATA_STORAGE ||
> +#ifdef CONFIG_PPC_BOOK3S
> +	    trap == INTERRUPT_DATA_SEGMENT ||
> +#endif
> +	    trap == INTERRUPT_ALIGNMENT)

It would be better to avoid #ifdefs when none where necessary before.

And an #ifdef in the middle of a code line is awful for readability and maintainability.


>   		printf("dar = "REG"   dsisr = %.8lx\n", fp->dar, fp->dsisr);
>   }
>   
> 

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] powerpc/traps: Enhance readability for trap types
  2021-04-09 16:14   ` Christophe Leroy
@ 2021-04-09 17:45     ` Segher Boessenkool
  -1 siblings, 0 replies; 17+ messages in thread
From: Segher Boessenkool @ 2021-04-09 17:45 UTC (permalink / raw)
  To: Christophe Leroy
  Cc: Xiongwei Song, mpe, benh, paulus, oleg, npiggin, aneesh.kumar,
	ravi.bangoria, mikey, haren, akpm, rppt, jniethe5, atrajeev,
	maddy, peterz, kjain, kan.liang, aik, alistair, pmladek,
	john.ogness, Xiongwei Song, linuxppc-dev, linux-kernel

On Fri, Apr 09, 2021 at 06:14:19PM +0200, Christophe Leroy wrote:
> >+#define INTERRUPT_SYSTEM_RESET    0x100
> 
> INT_SRESET

SRESET exists on many PowerPC, it means "soft reset".  Not the same
thing at all.

I think "INT" is not a great prefix fwiw, there are many things you can
abbr to "INT".

> >+#define INTERRUPT_DATA_SEGMENT    0x380
> 
> INT_DSEG

exceptions-64s.S calls this "DSLB" (I remember "DSSI" though -- but neither
is a very official name).  It probably is a good idea to look at that
existing code, not make up even more new names :-)

> >+#define INTERRUPT_DOORBELL        0xa00
> 
> INT_DBELL

That saves three characters and makes it very not understandable.


Segher

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] powerpc/traps: Enhance readability for trap types
@ 2021-04-09 17:45     ` Segher Boessenkool
  0 siblings, 0 replies; 17+ messages in thread
From: Segher Boessenkool @ 2021-04-09 17:45 UTC (permalink / raw)
  To: Christophe Leroy
  Cc: pmladek, peterz, linux-kernel, paulus, kan.liang, mikey, maddy,
	haren, aik, kjain, ravi.bangoria, john.ogness, alistair, npiggin,
	jniethe5, atrajeev, Xiongwei Song, Xiongwei Song, oleg,
	aneesh.kumar, akpm, linuxppc-dev, rppt

On Fri, Apr 09, 2021 at 06:14:19PM +0200, Christophe Leroy wrote:
> >+#define INTERRUPT_SYSTEM_RESET    0x100
> 
> INT_SRESET

SRESET exists on many PowerPC, it means "soft reset".  Not the same
thing at all.

I think "INT" is not a great prefix fwiw, there are many things you can
abbr to "INT".

> >+#define INTERRUPT_DATA_SEGMENT    0x380
> 
> INT_DSEG

exceptions-64s.S calls this "DSLB" (I remember "DSSI" though -- but neither
is a very official name).  It probably is a good idea to look at that
existing code, not make up even more new names :-)

> >+#define INTERRUPT_DOORBELL        0xa00
> 
> INT_DBELL

That saves three characters and makes it very not understandable.


Segher

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] powerpc/traps: Enhance readability for trap types
  2021-04-09 16:14   ` Christophe Leroy
@ 2021-04-10  0:04     ` Michael Ellerman
  -1 siblings, 0 replies; 17+ messages in thread
From: Michael Ellerman @ 2021-04-10  0:04 UTC (permalink / raw)
  To: Christophe Leroy, Xiongwei Song, benh, paulus, oleg, npiggin,
	aneesh.kumar, ravi.bangoria, mikey, haren, akpm, rppt, jniethe5,
	atrajeev, maddy, peterz, kjain, kan.liang, aik, alistair,
	pmladek, john.ogness
  Cc: linuxppc-dev, linux-kernel, Xiongwei Song

Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> Le 08/04/2021 à 16:07, Xiongwei Song a écrit :
>> From: Xiongwei Song <sxwjean@gmail.com>
>> 
>> Create a new header named traps.h, define macros to list ppc interrupt
>> types in traps.h, replace the reference of the trap hex values with these
>> macros.
...
>> diff --git a/arch/powerpc/include/asm/interrupt.h b/arch/powerpc/include/asm/interrupt.h
>> index 7c633896d758..5ce9898bc9a6 100644
>> --- a/arch/powerpc/include/asm/interrupt.h
>> +++ b/arch/powerpc/include/asm/interrupt.h
>> @@ -8,6 +8,7 @@
>>   #include <asm/ftrace.h>
>>   #include <asm/kprobes.h>
>>   #include <asm/runlatch.h>
>> +#include <asm/traps.h>
>>   
>>   struct interrupt_state {
>>   #ifdef CONFIG_PPC_BOOK3E_64
>> @@ -59,7 +60,7 @@ static inline void interrupt_enter_prepare(struct pt_regs *regs, struct interrup
>>   		 * CT_WARN_ON comes here via program_check_exception,
>>   		 * so avoid recursion.
>>   		 */
>> -		if (TRAP(regs) != 0x700)
>> +		if (TRAP(regs) != INTERRUPT_PROGRAM)
>>   			CT_WARN_ON(ct_state() != CONTEXT_KERNEL);
>>   	}
>>   #endif
>> @@ -156,7 +157,8 @@ static inline void interrupt_nmi_enter_prepare(struct pt_regs *regs, struct inte
>>   	/* Don't do any per-CPU operations until interrupt state is fixed */
>>   #endif
>>   	/* Allow DEC and PMI to be traced when they are soft-NMI */
>> -	if (TRAP(regs) != 0x900 && TRAP(regs) != 0xf00 && TRAP(regs) != 0x260) {
>> +	if (TRAP(regs) != INTERRUPT_DECREMENTER &&
>> +	    TRAP(regs) != INTERRUPT_PERFMON) {
>
> I think too long names hinder readability, see later for suggestions.

I asked for the longer names :)

I think they make it easier for people who are less familiar with the
architecture than us to make sense of the names.

And there's only a couple of cases where it requires splitting a line,
and they could be converted to use switch if we think it's a problem.

>> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
>> index 0c0b1c2cfb49..641b3feef7ee 100644
>> --- a/arch/powerpc/mm/fault.c
>> +++ b/arch/powerpc/mm/fault.c
>> @@ -588,20 +589,24 @@ void __bad_page_fault(struct pt_regs *regs, int sig)
>>   	/* kernel has accessed a bad area */
>>   
>>   	switch (TRAP(regs)) {
>> -	case 0x300:
>> -	case 0x380:
>> -	case 0xe00:
>> +	case INTERRUPT_DATA_STORAGE:
>> +#ifdef CONFIG_PPC_BOOK3S
>> +	case INTERRUPT_DATA_SEGMENT:
>> +	case INTERRUPT_H_DATA_STORAGE:
>> +#endif
>
> It would be better to avoid #ifdefs when none where necessary before.

Yes I agree.

I think these can all be avoided by defining most of the values
regardless of what platform we're building for. Only the values that
overlap need to be kept behind an ifdef.

cheers

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] powerpc/traps: Enhance readability for trap types
@ 2021-04-10  0:04     ` Michael Ellerman
  0 siblings, 0 replies; 17+ messages in thread
From: Michael Ellerman @ 2021-04-10  0:04 UTC (permalink / raw)
  To: Christophe Leroy, Xiongwei Song, benh, paulus, oleg, npiggin,
	aneesh.kumar, ravi.bangoria, mikey, haren, akpm, rppt, jniethe5,
	atrajeev, maddy, peterz, kjain, kan.liang, aik, alistair,
	pmladek, john.ogness
  Cc: Xiongwei Song, linuxppc-dev, linux-kernel

Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> Le 08/04/2021 à 16:07, Xiongwei Song a écrit :
>> From: Xiongwei Song <sxwjean@gmail.com>
>> 
>> Create a new header named traps.h, define macros to list ppc interrupt
>> types in traps.h, replace the reference of the trap hex values with these
>> macros.
...
>> diff --git a/arch/powerpc/include/asm/interrupt.h b/arch/powerpc/include/asm/interrupt.h
>> index 7c633896d758..5ce9898bc9a6 100644
>> --- a/arch/powerpc/include/asm/interrupt.h
>> +++ b/arch/powerpc/include/asm/interrupt.h
>> @@ -8,6 +8,7 @@
>>   #include <asm/ftrace.h>
>>   #include <asm/kprobes.h>
>>   #include <asm/runlatch.h>
>> +#include <asm/traps.h>
>>   
>>   struct interrupt_state {
>>   #ifdef CONFIG_PPC_BOOK3E_64
>> @@ -59,7 +60,7 @@ static inline void interrupt_enter_prepare(struct pt_regs *regs, struct interrup
>>   		 * CT_WARN_ON comes here via program_check_exception,
>>   		 * so avoid recursion.
>>   		 */
>> -		if (TRAP(regs) != 0x700)
>> +		if (TRAP(regs) != INTERRUPT_PROGRAM)
>>   			CT_WARN_ON(ct_state() != CONTEXT_KERNEL);
>>   	}
>>   #endif
>> @@ -156,7 +157,8 @@ static inline void interrupt_nmi_enter_prepare(struct pt_regs *regs, struct inte
>>   	/* Don't do any per-CPU operations until interrupt state is fixed */
>>   #endif
>>   	/* Allow DEC and PMI to be traced when they are soft-NMI */
>> -	if (TRAP(regs) != 0x900 && TRAP(regs) != 0xf00 && TRAP(regs) != 0x260) {
>> +	if (TRAP(regs) != INTERRUPT_DECREMENTER &&
>> +	    TRAP(regs) != INTERRUPT_PERFMON) {
>
> I think too long names hinder readability, see later for suggestions.

I asked for the longer names :)

I think they make it easier for people who are less familiar with the
architecture than us to make sense of the names.

And there's only a couple of cases where it requires splitting a line,
and they could be converted to use switch if we think it's a problem.

>> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
>> index 0c0b1c2cfb49..641b3feef7ee 100644
>> --- a/arch/powerpc/mm/fault.c
>> +++ b/arch/powerpc/mm/fault.c
>> @@ -588,20 +589,24 @@ void __bad_page_fault(struct pt_regs *regs, int sig)
>>   	/* kernel has accessed a bad area */
>>   
>>   	switch (TRAP(regs)) {
>> -	case 0x300:
>> -	case 0x380:
>> -	case 0xe00:
>> +	case INTERRUPT_DATA_STORAGE:
>> +#ifdef CONFIG_PPC_BOOK3S
>> +	case INTERRUPT_DATA_SEGMENT:
>> +	case INTERRUPT_H_DATA_STORAGE:
>> +#endif
>
> It would be better to avoid #ifdefs when none where necessary before.

Yes I agree.

I think these can all be avoided by defining most of the values
regardless of what platform we're building for. Only the values that
overlap need to be kept behind an ifdef.

cheers

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] powerpc/traps: Enhance readability for trap types
  2021-04-09 16:14   ` Christophe Leroy
@ 2021-04-10  8:31     ` Xiongwei Song
  -1 siblings, 0 replies; 17+ messages in thread
From: Xiongwei Song @ 2021-04-10  8:31 UTC (permalink / raw)
  To: Christophe Leroy
  Cc: Michael Ellerman, benh, paulus, oleg, npiggin, aneesh.kumar,
	ravi.bangoria, mikey, haren, akpm, rppt, jniethe5, atrajeev,
	maddy, peterz, kjain, kan.liang, aik, alistair, pmladek,
	john.ogness, linuxppc-dev, linux-kernel, Xiongwei Song


> On Apr 10, 2021, at 12:14 AM, Christophe Leroy <christophe.leroy@csgroup.eu> wrote:
> 
> 
> 
> Le 08/04/2021 à 16:07, Xiongwei Song a écrit :
>> From: Xiongwei Song <sxwjean@gmail.com>
>> Create a new header named traps.h, define macros to list ppc interrupt
>> types in traps.h, replace the reference of the trap hex values with these
>> macros.
>> Referred the hex number in arch/powerpc/kernel/exceptions-64e.S,
>> arch/powerpc/kernel/exceptions-64s.S and
>> arch/powerpc/include/asm/kvm_asm.h.
>> v2-v3:
>> Correct the prefix of trap macros with INTERRUPT_, the previous prefix
>> is TRAP_, which is not precise. This is suggested by Segher Boessenkool
>> and Nicholas Piggin.
>> v1-v2:
>> Define more trap macros to replace more trap hexs in code, not just for
>> the __show_regs function. This is suggested by Christophe Leroy.
>> Signed-off-by: Xiongwei Song <sxwjean@gmail.com>
>> ---
>>  arch/powerpc/include/asm/interrupt.h  |  9 +++++---
>>  arch/powerpc/include/asm/ptrace.h     |  3 ++-
>>  arch/powerpc/include/asm/traps.h      | 32 +++++++++++++++++++++++++++
>>  arch/powerpc/kernel/interrupt.c       |  3 ++-
>>  arch/powerpc/kernel/process.c         |  5 ++++-
>>  arch/powerpc/mm/book3s64/hash_utils.c |  5 +++--
>>  arch/powerpc/mm/fault.c               | 21 +++++++++++-------
>>  arch/powerpc/perf/core-book3s.c       |  5 +++--
>>  arch/powerpc/xmon/xmon.c              | 16 +++++++++++---
>>  9 files changed, 78 insertions(+), 21 deletions(-)
>>  create mode 100644 arch/powerpc/include/asm/traps.h
>> diff --git a/arch/powerpc/include/asm/interrupt.h b/arch/powerpc/include/asm/interrupt.h
>> index 7c633896d758..5ce9898bc9a6 100644
>> --- a/arch/powerpc/include/asm/interrupt.h
>> +++ b/arch/powerpc/include/asm/interrupt.h
>> @@ -8,6 +8,7 @@
>>  #include <asm/ftrace.h>
>>  #include <asm/kprobes.h>
>>  #include <asm/runlatch.h>
>> +#include <asm/traps.h>
>>    struct interrupt_state {
>>  #ifdef CONFIG_PPC_BOOK3E_64
>> @@ -59,7 +60,7 @@ static inline void interrupt_enter_prepare(struct pt_regs *regs, struct interrup
>>  		 * CT_WARN_ON comes here via program_check_exception,
>>  		 * so avoid recursion.
>>  		 */
>> -		if (TRAP(regs) != 0x700)
>> +		if (TRAP(regs) != INTERRUPT_PROGRAM)
>>  			CT_WARN_ON(ct_state() != CONTEXT_KERNEL);
>>  	}
>>  #endif
>> @@ -156,7 +157,8 @@ static inline void interrupt_nmi_enter_prepare(struct pt_regs *regs, struct inte
>>  	/* Don't do any per-CPU operations until interrupt state is fixed */
>>  #endif
>>  	/* Allow DEC and PMI to be traced when they are soft-NMI */
>> -	if (TRAP(regs) != 0x900 && TRAP(regs) != 0xf00 && TRAP(regs) != 0x260) {
>> +	if (TRAP(regs) != INTERRUPT_DECREMENTER &&
>> +	    TRAP(regs) != INTERRUPT_PERFMON) {
> 
> I think too long names hinder readability, see later for suggestions.
> 
>>  		state->ftrace_enabled = this_cpu_get_ftrace_enabled();
>>  		this_cpu_set_ftrace_enabled(0);
>>  	}
>> @@ -180,7 +182,8 @@ static inline void interrupt_nmi_exit_prepare(struct pt_regs *regs, struct inter
>>  		nmi_exit();
>>    #ifdef CONFIG_PPC64
>> -	if (TRAP(regs) != 0x900 && TRAP(regs) != 0xf00 && TRAP(regs) != 0x260)
>> +	if (TRAP(regs) != INTERRUPT_DECREMENTER &&
>> +	    TRAP(regs) != INTERRUPT_PERFMON)
>>  		this_cpu_set_ftrace_enabled(state->ftrace_enabled);
>>    #ifdef CONFIG_PPC_BOOK3S_64
>> diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
>> index f10498e1b3f6..7a17e0365d43 100644
>> --- a/arch/powerpc/include/asm/ptrace.h
>> +++ b/arch/powerpc/include/asm/ptrace.h
>> @@ -21,6 +21,7 @@
>>    #include <uapi/asm/ptrace.h>
>>  #include <asm/asm-const.h>
>> +#include <asm/traps.h>
>>    #ifndef __ASSEMBLY__
>>  struct pt_regs
>> @@ -237,7 +238,7 @@ static inline bool trap_is_unsupported_scv(struct pt_regs *regs)
>>    static inline bool trap_is_syscall(struct pt_regs *regs)
>>  {
>> -	return (trap_is_scv(regs) || TRAP(regs) == 0xc00);
>> +	return (trap_is_scv(regs) || TRAP(regs) == INTERRUPT_SYSCALL);
>>  }
>>    static inline bool trap_norestart(struct pt_regs *regs)
>> diff --git a/arch/powerpc/include/asm/traps.h b/arch/powerpc/include/asm/traps.h
>> new file mode 100644
>> index 000000000000..cb416a17097c
>> --- /dev/null
>> +++ b/arch/powerpc/include/asm/traps.h
>> @@ -0,0 +1,32 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef _ASM_PPC_TRAPS_H
>> +#define _ASM_PPC_TRAPS_H
>> +
>> +#if defined(CONFIG_BOOKE) || defined(CONFIG_4xx)
>> +#define INTERRUPT_MACHINE_CHECK   0x000
> 
> I'd prefer shorted names in order to not be obliged to split lines.
> Here are some suggestions:
> 
> INT_MCE
> 
>> +#define INTERRUPT_CRITICAL_INPUT  0x100
> 
> INT_CRIT
> 
>> +#define INTERRUPT_ALTIVEC_UNAVAIL 0x200
>> +#define INTERRUPT_PERFMON         0x260
> 
> INT_PERF
> 
>> +#define INTERRUPT_DOORBELL        0x280
>> +#define INTERRUPT_DEBUG           0xd00
>> +#elif defined(CONFIG_PPC_BOOK3S)
>> +#define INTERRUPT_SYSTEM_RESET    0x100
> 
> INT_SRESET
> 
>> +#define INTERRUPT_MACHINE_CHECK   0x200
> 
> INT_MCE
> 
>> +#define INTERRUPT_DATA_SEGMENT    0x380
> 
> INT_DSEG
> 
>> +#define INTERRUPT_INST_SEGMENT    0x480
> 
> INT_ISEG
> 
>> +#define INTERRUPT_DOORBELL        0xa00
> 
> INT_DBELL
> 
>> +#define INTERRUPT_TRACE           0xd00
> 
> INT_TRACE
> 
>> +#define INTERRUPT_H_DATA_STORAGE  0xe00
>> +#define INTERRUPT_PERFMON         0xf00
> 
> INT_PERF
> 
>> +#define INTERRUPT_H_FAC_UNAVAIL   0xf80
>> +#endif
>> +
>> +#define INTERRUPT_DATA_STORAGE    0x300
> 
> INT_DSI
> 
>> +#define INTERRUPT_INST_STORAGE    0x400
> 
> INT_ISI
> 
>> +#define INTERRUPT_ALIGNMENT       0x600
> 
> INT_ALIGN
> 
>> +#define INTERRUPT_PROGRAM         0x700
> 
> INT_PROG
> 
>> +#define INTERRUPT_FP_UNAVAIL      0x800
> 
> INT_FP_UNAVAIL
> 
>> +#define INTERRUPT_DECREMENTER     0x900
> 
> INT_DEC
> 
>> +#define INTERRUPT_SYSCALL         0xc00
> 
> INT_SYSCALL
> 
> 
>> +
>> +#endif /* _ASM_PPC_TRAPS_H */
> 
> ...
> 
>> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
>> index 0c0b1c2cfb49..641b3feef7ee 100644
>> --- a/arch/powerpc/mm/fault.c
>> +++ b/arch/powerpc/mm/fault.c
>> @@ -44,6 +44,7 @@
>>  #include <asm/debug.h>
>>  #include <asm/kup.h>
>>  #include <asm/inst.h>
>> +#include <asm/traps.h>
>>      /*
>> @@ -197,7 +198,7 @@ static int mm_fault_error(struct pt_regs *regs, unsigned long addr,
>>  static bool bad_kernel_fault(struct pt_regs *regs, unsigned long error_code,
>>  			     unsigned long address, bool is_write)
>>  {
>> -	int is_exec = TRAP(regs) == 0x400;
>> +	int is_exec = TRAP(regs) == INTERRUPT_INST_STORAGE;
>>    	/* NX faults set DSISR_PROTFAULT on the 8xx, DSISR_NOEXEC_OR_G on others */
>>  	if (is_exec && (error_code & (DSISR_NOEXEC_OR_G | DSISR_KEYFAULT |
>> @@ -391,7 +392,7 @@ static int ___do_page_fault(struct pt_regs *regs, unsigned long address,
>>  	struct vm_area_struct * vma;
>>  	struct mm_struct *mm = current->mm;
>>  	unsigned int flags = FAULT_FLAG_DEFAULT;
>> - 	int is_exec = TRAP(regs) == 0x400;
>> +	int is_exec = TRAP(regs) == INTERRUPT_INST_STORAGE;
>>  	int is_user = user_mode(regs);
>>  	int is_write = page_fault_is_write(error_code);
>>  	vm_fault_t fault, major = 0;
>> @@ -588,20 +589,24 @@ void __bad_page_fault(struct pt_regs *regs, int sig)
>>  	/* kernel has accessed a bad area */
>>    	switch (TRAP(regs)) {
>> -	case 0x300:
>> -	case 0x380:
>> -	case 0xe00:
>> +	case INTERRUPT_DATA_STORAGE:
>> +#ifdef CONFIG_PPC_BOOK3S
>> +	case INTERRUPT_DATA_SEGMENT:
>> +	case INTERRUPT_H_DATA_STORAGE:
>> +#endif
> 
> It would be better to avoid #ifdefs when none where necessary before.
> 
> 
>>  		pr_alert("BUG: %s on %s at 0x%08lx\n",
>>  			 regs->dar < PAGE_SIZE ? "Kernel NULL pointer dereference" :
>>  			 "Unable to handle kernel data access",
>>  			 is_write ? "write" : "read", regs->dar);
>>  		break;
>> -	case 0x400:
>> -	case 0x480:
>> +	case INTERRUPT_INST_STORAGE:
>> +#ifdef CONFIG_PPC_BOOK3S
>> +	case INTERRUPT_INST_SEGMENT:
>> +#endif
> 
> It would be better to avoid #ifdefs when none where necessary before.
> 
Good point. Will delete them.

Regards,
Xiongwei

> 
> 
>>  		pr_alert("BUG: Unable to handle kernel instruction fetch%s",
>>  			 regs->nip < PAGE_SIZE ? " (NULL pointer?)\n" : "\n");
>>  		break;
>> -	case 0x600:
>> +	case INTERRUPT_ALIGNMENT:
>>  		pr_alert("BUG: Unable to handle kernel unaligned access at 0x%08lx\n",
>>  			 regs->dar);
>>  		break;
>> diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
>> index 766f064f00fb..6e34f5bba232 100644
>> --- a/arch/powerpc/perf/core-book3s.c
>> +++ b/arch/powerpc/perf/core-book3s.c
>> @@ -17,6 +17,7 @@
>>  #include <asm/firmware.h>
>>  #include <asm/ptrace.h>
>>  #include <asm/code-patching.h>
>> +#include <asm/traps.h>
>>    #ifdef CONFIG_PPC64
>>  #include "internal.h"
>> @@ -168,7 +169,7 @@ static bool regs_use_siar(struct pt_regs *regs)
>>  	 * they have not been setup using perf_read_regs() and so regs->result
>>  	 * is something random.
>>  	 */
>> -	return ((TRAP(regs) == 0xf00) && regs->result);
>> +	return ((TRAP(regs) == INTERRUPT_PERFMON) && regs->result);
>>  }
>>    /*
>> @@ -347,7 +348,7 @@ static inline void perf_read_regs(struct pt_regs *regs)
>>  	 * hypervisor samples as well as samples in the kernel with
>>  	 * interrupts off hence the userspace check.
>>  	 */
>> -	if (TRAP(regs) != 0xf00)
>> +	if (TRAP(regs) != INTERRUPT_PERFMON)
>>  		use_siar = 0;
>>  	else if ((ppmu->flags & PPMU_NO_SIAR))
>>  		use_siar = 0;
>> diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
>> index bf7d69625a2e..2a4f99e64bf3 100644
>> --- a/arch/powerpc/xmon/xmon.c
>> +++ b/arch/powerpc/xmon/xmon.c
>> @@ -54,6 +54,7 @@
>>  #include <asm/code-patching.h>
>>  #include <asm/sections.h>
>>  #include <asm/inst.h>
>> +#include <asm/traps.h>
>>    #ifdef CONFIG_PPC64
>>  #include <asm/hvcall.h>
>> @@ -1769,7 +1770,12 @@ static void excprint(struct pt_regs *fp)
>>  	printf("    sp: %lx\n", fp->gpr[1]);
>>  	printf("   msr: %lx\n", fp->msr);
>>  -	if (trap == 0x300 || trap == 0x380 || trap == 0x600 || trap == 0x200) {
>> +	if (trap == INTERRUPT_DATA_STORAGE ||
>> +#ifdef CONFIG_PPC_BOOK3S
>> +	    trap == INTERRUPT_DATA_SEGMENT ||
>> +#endif
> It would be better to avoid #ifdefs when none where necessary before.
> 
> And an #ifdef in the middle of a code line is awful for readability and maintainability.
> 
>> +	    trap == INTERRUPT_ALIGNMENT ||
>> +	    trap == INTERRUPT_MACHINE_CHECK) {
>>  		printf("   dar: %lx\n", fp->dar);
>>  		if (trap != 0x380)
>>  			printf(" dsisr: %lx\n", fp->dsisr);
>> @@ -1785,7 +1791,7 @@ static void excprint(struct pt_regs *fp)
>>  		       current->pid, current->comm);
>>  	}
>>  -	if (trap == 0x700)
>> +	if (trap == INTERRUPT_PROGRAM)
>>  		print_bug_trap(fp);
>>    	printf(linux_banner);
>> @@ -1846,7 +1852,11 @@ static void prregs(struct pt_regs *fp)
>>  	printf("ctr = "REG"   xer = "REG"   trap = %4lx\n",
>>  	       fp->ctr, fp->xer, fp->trap);
>>  	trap = TRAP(fp);
>> -	if (trap == 0x300 || trap == 0x380 || trap == 0x600)
>> +	if (trap == INTERRUPT_DATA_STORAGE ||
>> +#ifdef CONFIG_PPC_BOOK3S
>> +	    trap == INTERRUPT_DATA_SEGMENT ||
>> +#endif
>> +	    trap == INTERRUPT_ALIGNMENT)
> 
> It would be better to avoid #ifdefs when none where necessary before.
> 
> And an #ifdef in the middle of a code line is awful for readability and maintainability.
> 
> 
>>  		printf("dar = "REG"   dsisr = %.8lx\n", fp->dar, fp->dsisr);
>>  }
>>  


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] powerpc/traps: Enhance readability for trap types
@ 2021-04-10  8:31     ` Xiongwei Song
  0 siblings, 0 replies; 17+ messages in thread
From: Xiongwei Song @ 2021-04-10  8:31 UTC (permalink / raw)
  To: Christophe Leroy
  Cc: pmladek, peterz, linux-kernel, paulus, kan.liang, mikey, maddy,
	haren, aik, kjain, ravi.bangoria, john.ogness, alistair, npiggin,
	jniethe5, atrajeev, Xiongwei Song, oleg, aneesh.kumar, akpm,
	linuxppc-dev, rppt


> On Apr 10, 2021, at 12:14 AM, Christophe Leroy <christophe.leroy@csgroup.eu> wrote:
> 
> 
> 
> Le 08/04/2021 à 16:07, Xiongwei Song a écrit :
>> From: Xiongwei Song <sxwjean@gmail.com>
>> Create a new header named traps.h, define macros to list ppc interrupt
>> types in traps.h, replace the reference of the trap hex values with these
>> macros.
>> Referred the hex number in arch/powerpc/kernel/exceptions-64e.S,
>> arch/powerpc/kernel/exceptions-64s.S and
>> arch/powerpc/include/asm/kvm_asm.h.
>> v2-v3:
>> Correct the prefix of trap macros with INTERRUPT_, the previous prefix
>> is TRAP_, which is not precise. This is suggested by Segher Boessenkool
>> and Nicholas Piggin.
>> v1-v2:
>> Define more trap macros to replace more trap hexs in code, not just for
>> the __show_regs function. This is suggested by Christophe Leroy.
>> Signed-off-by: Xiongwei Song <sxwjean@gmail.com>
>> ---
>>  arch/powerpc/include/asm/interrupt.h  |  9 +++++---
>>  arch/powerpc/include/asm/ptrace.h     |  3 ++-
>>  arch/powerpc/include/asm/traps.h      | 32 +++++++++++++++++++++++++++
>>  arch/powerpc/kernel/interrupt.c       |  3 ++-
>>  arch/powerpc/kernel/process.c         |  5 ++++-
>>  arch/powerpc/mm/book3s64/hash_utils.c |  5 +++--
>>  arch/powerpc/mm/fault.c               | 21 +++++++++++-------
>>  arch/powerpc/perf/core-book3s.c       |  5 +++--
>>  arch/powerpc/xmon/xmon.c              | 16 +++++++++++---
>>  9 files changed, 78 insertions(+), 21 deletions(-)
>>  create mode 100644 arch/powerpc/include/asm/traps.h
>> diff --git a/arch/powerpc/include/asm/interrupt.h b/arch/powerpc/include/asm/interrupt.h
>> index 7c633896d758..5ce9898bc9a6 100644
>> --- a/arch/powerpc/include/asm/interrupt.h
>> +++ b/arch/powerpc/include/asm/interrupt.h
>> @@ -8,6 +8,7 @@
>>  #include <asm/ftrace.h>
>>  #include <asm/kprobes.h>
>>  #include <asm/runlatch.h>
>> +#include <asm/traps.h>
>>    struct interrupt_state {
>>  #ifdef CONFIG_PPC_BOOK3E_64
>> @@ -59,7 +60,7 @@ static inline void interrupt_enter_prepare(struct pt_regs *regs, struct interrup
>>  		 * CT_WARN_ON comes here via program_check_exception,
>>  		 * so avoid recursion.
>>  		 */
>> -		if (TRAP(regs) != 0x700)
>> +		if (TRAP(regs) != INTERRUPT_PROGRAM)
>>  			CT_WARN_ON(ct_state() != CONTEXT_KERNEL);
>>  	}
>>  #endif
>> @@ -156,7 +157,8 @@ static inline void interrupt_nmi_enter_prepare(struct pt_regs *regs, struct inte
>>  	/* Don't do any per-CPU operations until interrupt state is fixed */
>>  #endif
>>  	/* Allow DEC and PMI to be traced when they are soft-NMI */
>> -	if (TRAP(regs) != 0x900 && TRAP(regs) != 0xf00 && TRAP(regs) != 0x260) {
>> +	if (TRAP(regs) != INTERRUPT_DECREMENTER &&
>> +	    TRAP(regs) != INTERRUPT_PERFMON) {
> 
> I think too long names hinder readability, see later for suggestions.
> 
>>  		state->ftrace_enabled = this_cpu_get_ftrace_enabled();
>>  		this_cpu_set_ftrace_enabled(0);
>>  	}
>> @@ -180,7 +182,8 @@ static inline void interrupt_nmi_exit_prepare(struct pt_regs *regs, struct inter
>>  		nmi_exit();
>>    #ifdef CONFIG_PPC64
>> -	if (TRAP(regs) != 0x900 && TRAP(regs) != 0xf00 && TRAP(regs) != 0x260)
>> +	if (TRAP(regs) != INTERRUPT_DECREMENTER &&
>> +	    TRAP(regs) != INTERRUPT_PERFMON)
>>  		this_cpu_set_ftrace_enabled(state->ftrace_enabled);
>>    #ifdef CONFIG_PPC_BOOK3S_64
>> diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
>> index f10498e1b3f6..7a17e0365d43 100644
>> --- a/arch/powerpc/include/asm/ptrace.h
>> +++ b/arch/powerpc/include/asm/ptrace.h
>> @@ -21,6 +21,7 @@
>>    #include <uapi/asm/ptrace.h>
>>  #include <asm/asm-const.h>
>> +#include <asm/traps.h>
>>    #ifndef __ASSEMBLY__
>>  struct pt_regs
>> @@ -237,7 +238,7 @@ static inline bool trap_is_unsupported_scv(struct pt_regs *regs)
>>    static inline bool trap_is_syscall(struct pt_regs *regs)
>>  {
>> -	return (trap_is_scv(regs) || TRAP(regs) == 0xc00);
>> +	return (trap_is_scv(regs) || TRAP(regs) == INTERRUPT_SYSCALL);
>>  }
>>    static inline bool trap_norestart(struct pt_regs *regs)
>> diff --git a/arch/powerpc/include/asm/traps.h b/arch/powerpc/include/asm/traps.h
>> new file mode 100644
>> index 000000000000..cb416a17097c
>> --- /dev/null
>> +++ b/arch/powerpc/include/asm/traps.h
>> @@ -0,0 +1,32 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef _ASM_PPC_TRAPS_H
>> +#define _ASM_PPC_TRAPS_H
>> +
>> +#if defined(CONFIG_BOOKE) || defined(CONFIG_4xx)
>> +#define INTERRUPT_MACHINE_CHECK   0x000
> 
> I'd prefer shorted names in order to not be obliged to split lines.
> Here are some suggestions:
> 
> INT_MCE
> 
>> +#define INTERRUPT_CRITICAL_INPUT  0x100
> 
> INT_CRIT
> 
>> +#define INTERRUPT_ALTIVEC_UNAVAIL 0x200
>> +#define INTERRUPT_PERFMON         0x260
> 
> INT_PERF
> 
>> +#define INTERRUPT_DOORBELL        0x280
>> +#define INTERRUPT_DEBUG           0xd00
>> +#elif defined(CONFIG_PPC_BOOK3S)
>> +#define INTERRUPT_SYSTEM_RESET    0x100
> 
> INT_SRESET
> 
>> +#define INTERRUPT_MACHINE_CHECK   0x200
> 
> INT_MCE
> 
>> +#define INTERRUPT_DATA_SEGMENT    0x380
> 
> INT_DSEG
> 
>> +#define INTERRUPT_INST_SEGMENT    0x480
> 
> INT_ISEG
> 
>> +#define INTERRUPT_DOORBELL        0xa00
> 
> INT_DBELL
> 
>> +#define INTERRUPT_TRACE           0xd00
> 
> INT_TRACE
> 
>> +#define INTERRUPT_H_DATA_STORAGE  0xe00
>> +#define INTERRUPT_PERFMON         0xf00
> 
> INT_PERF
> 
>> +#define INTERRUPT_H_FAC_UNAVAIL   0xf80
>> +#endif
>> +
>> +#define INTERRUPT_DATA_STORAGE    0x300
> 
> INT_DSI
> 
>> +#define INTERRUPT_INST_STORAGE    0x400
> 
> INT_ISI
> 
>> +#define INTERRUPT_ALIGNMENT       0x600
> 
> INT_ALIGN
> 
>> +#define INTERRUPT_PROGRAM         0x700
> 
> INT_PROG
> 
>> +#define INTERRUPT_FP_UNAVAIL      0x800
> 
> INT_FP_UNAVAIL
> 
>> +#define INTERRUPT_DECREMENTER     0x900
> 
> INT_DEC
> 
>> +#define INTERRUPT_SYSCALL         0xc00
> 
> INT_SYSCALL
> 
> 
>> +
>> +#endif /* _ASM_PPC_TRAPS_H */
> 
> ...
> 
>> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
>> index 0c0b1c2cfb49..641b3feef7ee 100644
>> --- a/arch/powerpc/mm/fault.c
>> +++ b/arch/powerpc/mm/fault.c
>> @@ -44,6 +44,7 @@
>>  #include <asm/debug.h>
>>  #include <asm/kup.h>
>>  #include <asm/inst.h>
>> +#include <asm/traps.h>
>>      /*
>> @@ -197,7 +198,7 @@ static int mm_fault_error(struct pt_regs *regs, unsigned long addr,
>>  static bool bad_kernel_fault(struct pt_regs *regs, unsigned long error_code,
>>  			     unsigned long address, bool is_write)
>>  {
>> -	int is_exec = TRAP(regs) == 0x400;
>> +	int is_exec = TRAP(regs) == INTERRUPT_INST_STORAGE;
>>    	/* NX faults set DSISR_PROTFAULT on the 8xx, DSISR_NOEXEC_OR_G on others */
>>  	if (is_exec && (error_code & (DSISR_NOEXEC_OR_G | DSISR_KEYFAULT |
>> @@ -391,7 +392,7 @@ static int ___do_page_fault(struct pt_regs *regs, unsigned long address,
>>  	struct vm_area_struct * vma;
>>  	struct mm_struct *mm = current->mm;
>>  	unsigned int flags = FAULT_FLAG_DEFAULT;
>> - 	int is_exec = TRAP(regs) == 0x400;
>> +	int is_exec = TRAP(regs) == INTERRUPT_INST_STORAGE;
>>  	int is_user = user_mode(regs);
>>  	int is_write = page_fault_is_write(error_code);
>>  	vm_fault_t fault, major = 0;
>> @@ -588,20 +589,24 @@ void __bad_page_fault(struct pt_regs *regs, int sig)
>>  	/* kernel has accessed a bad area */
>>    	switch (TRAP(regs)) {
>> -	case 0x300:
>> -	case 0x380:
>> -	case 0xe00:
>> +	case INTERRUPT_DATA_STORAGE:
>> +#ifdef CONFIG_PPC_BOOK3S
>> +	case INTERRUPT_DATA_SEGMENT:
>> +	case INTERRUPT_H_DATA_STORAGE:
>> +#endif
> 
> It would be better to avoid #ifdefs when none where necessary before.
> 
> 
>>  		pr_alert("BUG: %s on %s at 0x%08lx\n",
>>  			 regs->dar < PAGE_SIZE ? "Kernel NULL pointer dereference" :
>>  			 "Unable to handle kernel data access",
>>  			 is_write ? "write" : "read", regs->dar);
>>  		break;
>> -	case 0x400:
>> -	case 0x480:
>> +	case INTERRUPT_INST_STORAGE:
>> +#ifdef CONFIG_PPC_BOOK3S
>> +	case INTERRUPT_INST_SEGMENT:
>> +#endif
> 
> It would be better to avoid #ifdefs when none where necessary before.
> 
Good point. Will delete them.

Regards,
Xiongwei

> 
> 
>>  		pr_alert("BUG: Unable to handle kernel instruction fetch%s",
>>  			 regs->nip < PAGE_SIZE ? " (NULL pointer?)\n" : "\n");
>>  		break;
>> -	case 0x600:
>> +	case INTERRUPT_ALIGNMENT:
>>  		pr_alert("BUG: Unable to handle kernel unaligned access at 0x%08lx\n",
>>  			 regs->dar);
>>  		break;
>> diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
>> index 766f064f00fb..6e34f5bba232 100644
>> --- a/arch/powerpc/perf/core-book3s.c
>> +++ b/arch/powerpc/perf/core-book3s.c
>> @@ -17,6 +17,7 @@
>>  #include <asm/firmware.h>
>>  #include <asm/ptrace.h>
>>  #include <asm/code-patching.h>
>> +#include <asm/traps.h>
>>    #ifdef CONFIG_PPC64
>>  #include "internal.h"
>> @@ -168,7 +169,7 @@ static bool regs_use_siar(struct pt_regs *regs)
>>  	 * they have not been setup using perf_read_regs() and so regs->result
>>  	 * is something random.
>>  	 */
>> -	return ((TRAP(regs) == 0xf00) && regs->result);
>> +	return ((TRAP(regs) == INTERRUPT_PERFMON) && regs->result);
>>  }
>>    /*
>> @@ -347,7 +348,7 @@ static inline void perf_read_regs(struct pt_regs *regs)
>>  	 * hypervisor samples as well as samples in the kernel with
>>  	 * interrupts off hence the userspace check.
>>  	 */
>> -	if (TRAP(regs) != 0xf00)
>> +	if (TRAP(regs) != INTERRUPT_PERFMON)
>>  		use_siar = 0;
>>  	else if ((ppmu->flags & PPMU_NO_SIAR))
>>  		use_siar = 0;
>> diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
>> index bf7d69625a2e..2a4f99e64bf3 100644
>> --- a/arch/powerpc/xmon/xmon.c
>> +++ b/arch/powerpc/xmon/xmon.c
>> @@ -54,6 +54,7 @@
>>  #include <asm/code-patching.h>
>>  #include <asm/sections.h>
>>  #include <asm/inst.h>
>> +#include <asm/traps.h>
>>    #ifdef CONFIG_PPC64
>>  #include <asm/hvcall.h>
>> @@ -1769,7 +1770,12 @@ static void excprint(struct pt_regs *fp)
>>  	printf("    sp: %lx\n", fp->gpr[1]);
>>  	printf("   msr: %lx\n", fp->msr);
>>  -	if (trap == 0x300 || trap == 0x380 || trap == 0x600 || trap == 0x200) {
>> +	if (trap == INTERRUPT_DATA_STORAGE ||
>> +#ifdef CONFIG_PPC_BOOK3S
>> +	    trap == INTERRUPT_DATA_SEGMENT ||
>> +#endif
> It would be better to avoid #ifdefs when none where necessary before.
> 
> And an #ifdef in the middle of a code line is awful for readability and maintainability.
> 
>> +	    trap == INTERRUPT_ALIGNMENT ||
>> +	    trap == INTERRUPT_MACHINE_CHECK) {
>>  		printf("   dar: %lx\n", fp->dar);
>>  		if (trap != 0x380)
>>  			printf(" dsisr: %lx\n", fp->dsisr);
>> @@ -1785,7 +1791,7 @@ static void excprint(struct pt_regs *fp)
>>  		       current->pid, current->comm);
>>  	}
>>  -	if (trap == 0x700)
>> +	if (trap == INTERRUPT_PROGRAM)
>>  		print_bug_trap(fp);
>>    	printf(linux_banner);
>> @@ -1846,7 +1852,11 @@ static void prregs(struct pt_regs *fp)
>>  	printf("ctr = "REG"   xer = "REG"   trap = %4lx\n",
>>  	       fp->ctr, fp->xer, fp->trap);
>>  	trap = TRAP(fp);
>> -	if (trap == 0x300 || trap == 0x380 || trap == 0x600)
>> +	if (trap == INTERRUPT_DATA_STORAGE ||
>> +#ifdef CONFIG_PPC_BOOK3S
>> +	    trap == INTERRUPT_DATA_SEGMENT ||
>> +#endif
>> +	    trap == INTERRUPT_ALIGNMENT)
> 
> It would be better to avoid #ifdefs when none where necessary before.
> 
> And an #ifdef in the middle of a code line is awful for readability and maintainability.
> 
> 
>>  		printf("dar = "REG"   dsisr = %.8lx\n", fp->dar, fp->dsisr);
>>  }
>>  


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] powerpc/traps: Enhance readability for trap types
  2021-04-10  0:04     ` Michael Ellerman
@ 2021-04-10  8:33       ` Xiongwei Song
  -1 siblings, 0 replies; 17+ messages in thread
From: Xiongwei Song @ 2021-04-10  8:33 UTC (permalink / raw)
  To: Michael Ellerman
  Cc: Christophe Leroy, benh, paulus, oleg, npiggin, aneesh.kumar,
	ravi.bangoria, mikey, haren, akpm, rppt, jniethe5, atrajeev,
	maddy, peterz, kjain, kan.liang, aik, alistair, pmladek,
	john.ogness, linuxppc-dev, linux-kernel, Xiongwei Song


> On Apr 10, 2021, at 8:04 AM, Michael Ellerman <mpe@ellerman.id.au> wrote:
> 
> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>> Le 08/04/2021 à 16:07, Xiongwei Song a écrit :
>>> From: Xiongwei Song <sxwjean@gmail.com>
>>> 
>>> Create a new header named traps.h, define macros to list ppc interrupt
>>> types in traps.h, replace the reference of the trap hex values with these
>>> macros.
> ...
>>> diff --git a/arch/powerpc/include/asm/interrupt.h b/arch/powerpc/include/asm/interrupt.h
>>> index 7c633896d758..5ce9898bc9a6 100644
>>> --- a/arch/powerpc/include/asm/interrupt.h
>>> +++ b/arch/powerpc/include/asm/interrupt.h
>>> @@ -8,6 +8,7 @@
>>>  #include <asm/ftrace.h>
>>>  #include <asm/kprobes.h>
>>>  #include <asm/runlatch.h>
>>> +#include <asm/traps.h>
>>> 
>>>  struct interrupt_state {
>>>  #ifdef CONFIG_PPC_BOOK3E_64
>>> @@ -59,7 +60,7 @@ static inline void interrupt_enter_prepare(struct pt_regs *regs, struct interrup
>>>  		 * CT_WARN_ON comes here via program_check_exception,
>>>  		 * so avoid recursion.
>>>  		 */
>>> -		if (TRAP(regs) != 0x700)
>>> +		if (TRAP(regs) != INTERRUPT_PROGRAM)
>>>  			CT_WARN_ON(ct_state() != CONTEXT_KERNEL);
>>>  	}
>>>  #endif
>>> @@ -156,7 +157,8 @@ static inline void interrupt_nmi_enter_prepare(struct pt_regs *regs, struct inte
>>>  	/* Don't do any per-CPU operations until interrupt state is fixed */
>>>  #endif
>>>  	/* Allow DEC and PMI to be traced when they are soft-NMI */
>>> -	if (TRAP(regs) != 0x900 && TRAP(regs) != 0xf00 && TRAP(regs) != 0x260) {
>>> +	if (TRAP(regs) != INTERRUPT_DECREMENTER &&
>>> +	    TRAP(regs) != INTERRUPT_PERFMON) {
>> 
>> I think too long names hinder readability, see later for suggestions.
> 
> I asked for the longer names :)
> 
> I think they make it easier for people who are less familiar with the
> architecture than us to make sense of the names.
> 
> And there's only a couple of cases where it requires splitting a line,
> and they could be converted to use switch if we think it's a problem.
> 
>>> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
>>> index 0c0b1c2cfb49..641b3feef7ee 100644
>>> --- a/arch/powerpc/mm/fault.c
>>> +++ b/arch/powerpc/mm/fault.c
>>> @@ -588,20 +589,24 @@ void __bad_page_fault(struct pt_regs *regs, int sig)
>>>  	/* kernel has accessed a bad area */
>>> 
>>>  	switch (TRAP(regs)) {
>>> -	case 0x300:
>>> -	case 0x380:
>>> -	case 0xe00:
>>> +	case INTERRUPT_DATA_STORAGE:
>>> +#ifdef CONFIG_PPC_BOOK3S
>>> +	case INTERRUPT_DATA_SEGMENT:
>>> +	case INTERRUPT_H_DATA_STORAGE:
>>> +#endif
>> 
>> It would be better to avoid #ifdefs when none where necessary before.
> 
> Yes I agree.
> 
> I think these can all be avoided by defining most of the values
> regardless of what platform we're building for. Only the values that
> overlap need to be kept behind an ifdef.

Ok. 

Regards,
Xiongwei

> 
> cheers


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] powerpc/traps: Enhance readability for trap types
@ 2021-04-10  8:33       ` Xiongwei Song
  0 siblings, 0 replies; 17+ messages in thread
From: Xiongwei Song @ 2021-04-10  8:33 UTC (permalink / raw)
  To: Michael Ellerman
  Cc: pmladek, peterz, linux-kernel, paulus, kan.liang, mikey, maddy,
	aneesh.kumar, haren, aik, kjain, ravi.bangoria, john.ogness,
	alistair, npiggin, jniethe5, atrajeev, Xiongwei Song, oleg, akpm,
	linuxppc-dev, rppt


> On Apr 10, 2021, at 8:04 AM, Michael Ellerman <mpe@ellerman.id.au> wrote:
> 
> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>> Le 08/04/2021 à 16:07, Xiongwei Song a écrit :
>>> From: Xiongwei Song <sxwjean@gmail.com>
>>> 
>>> Create a new header named traps.h, define macros to list ppc interrupt
>>> types in traps.h, replace the reference of the trap hex values with these
>>> macros.
> ...
>>> diff --git a/arch/powerpc/include/asm/interrupt.h b/arch/powerpc/include/asm/interrupt.h
>>> index 7c633896d758..5ce9898bc9a6 100644
>>> --- a/arch/powerpc/include/asm/interrupt.h
>>> +++ b/arch/powerpc/include/asm/interrupt.h
>>> @@ -8,6 +8,7 @@
>>>  #include <asm/ftrace.h>
>>>  #include <asm/kprobes.h>
>>>  #include <asm/runlatch.h>
>>> +#include <asm/traps.h>
>>> 
>>>  struct interrupt_state {
>>>  #ifdef CONFIG_PPC_BOOK3E_64
>>> @@ -59,7 +60,7 @@ static inline void interrupt_enter_prepare(struct pt_regs *regs, struct interrup
>>>  		 * CT_WARN_ON comes here via program_check_exception,
>>>  		 * so avoid recursion.
>>>  		 */
>>> -		if (TRAP(regs) != 0x700)
>>> +		if (TRAP(regs) != INTERRUPT_PROGRAM)
>>>  			CT_WARN_ON(ct_state() != CONTEXT_KERNEL);
>>>  	}
>>>  #endif
>>> @@ -156,7 +157,8 @@ static inline void interrupt_nmi_enter_prepare(struct pt_regs *regs, struct inte
>>>  	/* Don't do any per-CPU operations until interrupt state is fixed */
>>>  #endif
>>>  	/* Allow DEC and PMI to be traced when they are soft-NMI */
>>> -	if (TRAP(regs) != 0x900 && TRAP(regs) != 0xf00 && TRAP(regs) != 0x260) {
>>> +	if (TRAP(regs) != INTERRUPT_DECREMENTER &&
>>> +	    TRAP(regs) != INTERRUPT_PERFMON) {
>> 
>> I think too long names hinder readability, see later for suggestions.
> 
> I asked for the longer names :)
> 
> I think they make it easier for people who are less familiar with the
> architecture than us to make sense of the names.
> 
> And there's only a couple of cases where it requires splitting a line,
> and they could be converted to use switch if we think it's a problem.
> 
>>> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
>>> index 0c0b1c2cfb49..641b3feef7ee 100644
>>> --- a/arch/powerpc/mm/fault.c
>>> +++ b/arch/powerpc/mm/fault.c
>>> @@ -588,20 +589,24 @@ void __bad_page_fault(struct pt_regs *regs, int sig)
>>>  	/* kernel has accessed a bad area */
>>> 
>>>  	switch (TRAP(regs)) {
>>> -	case 0x300:
>>> -	case 0x380:
>>> -	case 0xe00:
>>> +	case INTERRUPT_DATA_STORAGE:
>>> +#ifdef CONFIG_PPC_BOOK3S
>>> +	case INTERRUPT_DATA_SEGMENT:
>>> +	case INTERRUPT_H_DATA_STORAGE:
>>> +#endif
>> 
>> It would be better to avoid #ifdefs when none where necessary before.
> 
> Yes I agree.
> 
> I think these can all be avoided by defining most of the values
> regardless of what platform we're building for. Only the values that
> overlap need to be kept behind an ifdef.

Ok. 

Regards,
Xiongwei

> 
> cheers


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] powerpc/traps: Enhance readability for trap types
  2021-04-10  0:04     ` Michael Ellerman
@ 2021-04-10  9:42       ` Christophe Leroy
  -1 siblings, 0 replies; 17+ messages in thread
From: Christophe Leroy @ 2021-04-10  9:42 UTC (permalink / raw)
  To: Michael Ellerman, Xiongwei Song, benh, paulus, oleg, npiggin,
	aneesh.kumar, ravi.bangoria, mikey, haren, akpm, rppt, jniethe5,
	atrajeev, maddy, peterz, kjain, kan.liang, aik, alistair,
	pmladek, john.ogness
  Cc: linuxppc-dev, linux-kernel, Xiongwei Song



Le 10/04/2021 à 02:04, Michael Ellerman a écrit :
> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>> Le 08/04/2021 à 16:07, Xiongwei Song a écrit :
>>> From: Xiongwei Song <sxwjean@gmail.com>
>>>
>>> Create a new header named traps.h, define macros to list ppc interrupt
>>> types in traps.h, replace the reference of the trap hex values with these
>>> macros.
> ...
>>> diff --git a/arch/powerpc/include/asm/interrupt.h b/arch/powerpc/include/asm/interrupt.h
>>> index 7c633896d758..5ce9898bc9a6 100644
>>> --- a/arch/powerpc/include/asm/interrupt.h
>>> +++ b/arch/powerpc/include/asm/interrupt.h
>>> @@ -8,6 +8,7 @@
>>>    #include <asm/ftrace.h>
>>>    #include <asm/kprobes.h>
>>>    #include <asm/runlatch.h>
>>> +#include <asm/traps.h>
>>>    
>>>    struct interrupt_state {
>>>    #ifdef CONFIG_PPC_BOOK3E_64
>>> @@ -59,7 +60,7 @@ static inline void interrupt_enter_prepare(struct pt_regs *regs, struct interrup
>>>    		 * CT_WARN_ON comes here via program_check_exception,
>>>    		 * so avoid recursion.
>>>    		 */
>>> -		if (TRAP(regs) != 0x700)
>>> +		if (TRAP(regs) != INTERRUPT_PROGRAM)
>>>    			CT_WARN_ON(ct_state() != CONTEXT_KERNEL);
>>>    	}
>>>    #endif
>>> @@ -156,7 +157,8 @@ static inline void interrupt_nmi_enter_prepare(struct pt_regs *regs, struct inte
>>>    	/* Don't do any per-CPU operations until interrupt state is fixed */
>>>    #endif
>>>    	/* Allow DEC and PMI to be traced when they are soft-NMI */
>>> -	if (TRAP(regs) != 0x900 && TRAP(regs) != 0xf00 && TRAP(regs) != 0x260) {
>>> +	if (TRAP(regs) != INTERRUPT_DECREMENTER &&
>>> +	    TRAP(regs) != INTERRUPT_PERFMON) {
>>
>> I think too long names hinder readability, see later for suggestions.
> 
> I asked for the longer names :)
> 
> I think they make it easier for people who are less familiar with the
> architecture than us to make sense of the names.

Ok

> 
> And there's only a couple of cases where it requires splitting a line,
> and they could be converted to use switch if we think it's a problem.

> 
>>> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
>>> index 0c0b1c2cfb49..641b3feef7ee 100644
>>> --- a/arch/powerpc/mm/fault.c
>>> +++ b/arch/powerpc/mm/fault.c
>>> @@ -588,20 +589,24 @@ void __bad_page_fault(struct pt_regs *regs, int sig)
>>>    	/* kernel has accessed a bad area */
>>>    
>>>    	switch (TRAP(regs)) {
>>> -	case 0x300:
>>> -	case 0x380:
>>> -	case 0xe00:
>>> +	case INTERRUPT_DATA_STORAGE:
>>> +#ifdef CONFIG_PPC_BOOK3S
>>> +	case INTERRUPT_DATA_SEGMENT:
>>> +	case INTERRUPT_H_DATA_STORAGE:
>>> +#endif
>>
>> It would be better to avoid #ifdefs when none where necessary before.
> 
> Yes I agree.
> 
> I think these can all be avoided by defining most of the values
> regardless of what platform we're building for. Only the values that
> overlap need to be kept behind an ifdef.

Even if values overlap we can keep multiple definitions for the same value.

It is only when the same name has different values that we need to keep them behind ifdef. Is there 
any ?

Christophe

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] powerpc/traps: Enhance readability for trap types
@ 2021-04-10  9:42       ` Christophe Leroy
  0 siblings, 0 replies; 17+ messages in thread
From: Christophe Leroy @ 2021-04-10  9:42 UTC (permalink / raw)
  To: Michael Ellerman, Xiongwei Song, benh, paulus, oleg, npiggin,
	aneesh.kumar, ravi.bangoria, mikey, haren, akpm, rppt, jniethe5,
	atrajeev, maddy, peterz, kjain, kan.liang, aik, alistair,
	pmladek, john.ogness
  Cc: Xiongwei Song, linuxppc-dev, linux-kernel



Le 10/04/2021 à 02:04, Michael Ellerman a écrit :
> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>> Le 08/04/2021 à 16:07, Xiongwei Song a écrit :
>>> From: Xiongwei Song <sxwjean@gmail.com>
>>>
>>> Create a new header named traps.h, define macros to list ppc interrupt
>>> types in traps.h, replace the reference of the trap hex values with these
>>> macros.
> ...
>>> diff --git a/arch/powerpc/include/asm/interrupt.h b/arch/powerpc/include/asm/interrupt.h
>>> index 7c633896d758..5ce9898bc9a6 100644
>>> --- a/arch/powerpc/include/asm/interrupt.h
>>> +++ b/arch/powerpc/include/asm/interrupt.h
>>> @@ -8,6 +8,7 @@
>>>    #include <asm/ftrace.h>
>>>    #include <asm/kprobes.h>
>>>    #include <asm/runlatch.h>
>>> +#include <asm/traps.h>
>>>    
>>>    struct interrupt_state {
>>>    #ifdef CONFIG_PPC_BOOK3E_64
>>> @@ -59,7 +60,7 @@ static inline void interrupt_enter_prepare(struct pt_regs *regs, struct interrup
>>>    		 * CT_WARN_ON comes here via program_check_exception,
>>>    		 * so avoid recursion.
>>>    		 */
>>> -		if (TRAP(regs) != 0x700)
>>> +		if (TRAP(regs) != INTERRUPT_PROGRAM)
>>>    			CT_WARN_ON(ct_state() != CONTEXT_KERNEL);
>>>    	}
>>>    #endif
>>> @@ -156,7 +157,8 @@ static inline void interrupt_nmi_enter_prepare(struct pt_regs *regs, struct inte
>>>    	/* Don't do any per-CPU operations until interrupt state is fixed */
>>>    #endif
>>>    	/* Allow DEC and PMI to be traced when they are soft-NMI */
>>> -	if (TRAP(regs) != 0x900 && TRAP(regs) != 0xf00 && TRAP(regs) != 0x260) {
>>> +	if (TRAP(regs) != INTERRUPT_DECREMENTER &&
>>> +	    TRAP(regs) != INTERRUPT_PERFMON) {
>>
>> I think too long names hinder readability, see later for suggestions.
> 
> I asked for the longer names :)
> 
> I think they make it easier for people who are less familiar with the
> architecture than us to make sense of the names.

Ok

> 
> And there's only a couple of cases where it requires splitting a line,
> and they could be converted to use switch if we think it's a problem.

> 
>>> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
>>> index 0c0b1c2cfb49..641b3feef7ee 100644
>>> --- a/arch/powerpc/mm/fault.c
>>> +++ b/arch/powerpc/mm/fault.c
>>> @@ -588,20 +589,24 @@ void __bad_page_fault(struct pt_regs *regs, int sig)
>>>    	/* kernel has accessed a bad area */
>>>    
>>>    	switch (TRAP(regs)) {
>>> -	case 0x300:
>>> -	case 0x380:
>>> -	case 0xe00:
>>> +	case INTERRUPT_DATA_STORAGE:
>>> +#ifdef CONFIG_PPC_BOOK3S
>>> +	case INTERRUPT_DATA_SEGMENT:
>>> +	case INTERRUPT_H_DATA_STORAGE:
>>> +#endif
>>
>> It would be better to avoid #ifdefs when none where necessary before.
> 
> Yes I agree.
> 
> I think these can all be avoided by defining most of the values
> regardless of what platform we're building for. Only the values that
> overlap need to be kept behind an ifdef.

Even if values overlap we can keep multiple definitions for the same value.

It is only when the same name has different values that we need to keep them behind ifdef. Is there 
any ?

Christophe

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] powerpc/traps: Enhance readability for trap types
  2021-04-10  9:42       ` Christophe Leroy
@ 2021-04-10 16:46         ` Segher Boessenkool
  -1 siblings, 0 replies; 17+ messages in thread
From: Segher Boessenkool @ 2021-04-10 16:46 UTC (permalink / raw)
  To: Christophe Leroy
  Cc: Michael Ellerman, Xiongwei Song, benh, paulus, oleg, npiggin,
	aneesh.kumar, ravi.bangoria, mikey, haren, akpm, rppt, jniethe5,
	atrajeev, maddy, peterz, kjain, kan.liang, aik, alistair,
	pmladek, john.ogness, Xiongwei Song, linuxppc-dev, linux-kernel

On Sat, Apr 10, 2021 at 11:42:41AM +0200, Christophe Leroy wrote:
> Le 10/04/2021 à 02:04, Michael Ellerman a écrit :
> >I think these can all be avoided by defining most of the values
> >regardless of what platform we're building for. Only the values that
> >overlap need to be kept behind an ifdef.
> 
> Even if values overlap we can keep multiple definitions for the same value.

That works, but it can lead to puzzling bugs.  Of course we all *like*
working on more challenging bugs, but :-)


Segher

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] powerpc/traps: Enhance readability for trap types
@ 2021-04-10 16:46         ` Segher Boessenkool
  0 siblings, 0 replies; 17+ messages in thread
From: Segher Boessenkool @ 2021-04-10 16:46 UTC (permalink / raw)
  To: Christophe Leroy
  Cc: pmladek, peterz, linux-kernel, paulus, kan.liang, mikey, maddy,
	haren, aik, kjain, ravi.bangoria, john.ogness, alistair, npiggin,
	jniethe5, atrajeev, Xiongwei Song, Xiongwei Song, oleg,
	aneesh.kumar, akpm, linuxppc-dev, rppt

On Sat, Apr 10, 2021 at 11:42:41AM +0200, Christophe Leroy wrote:
> Le 10/04/2021 à 02:04, Michael Ellerman a écrit :
> >I think these can all be avoided by defining most of the values
> >regardless of what platform we're building for. Only the values that
> >overlap need to be kept behind an ifdef.
> 
> Even if values overlap we can keep multiple definitions for the same value.

That works, but it can lead to puzzling bugs.  Of course we all *like*
working on more challenging bugs, but :-)


Segher

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2021-04-10 17:03 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-08 14:07 [PATCH v3] powerpc/traps: Enhance readability for trap types Xiongwei Song
2021-04-08 14:07 ` Xiongwei Song
2021-04-08 16:54 ` kernel test robot
2021-04-09 16:14 ` Christophe Leroy
2021-04-09 16:14   ` Christophe Leroy
2021-04-09 17:45   ` Segher Boessenkool
2021-04-09 17:45     ` Segher Boessenkool
2021-04-10  0:04   ` Michael Ellerman
2021-04-10  0:04     ` Michael Ellerman
2021-04-10  8:33     ` Xiongwei Song
2021-04-10  8:33       ` Xiongwei Song
2021-04-10  9:42     ` Christophe Leroy
2021-04-10  9:42       ` Christophe Leroy
2021-04-10 16:46       ` Segher Boessenkool
2021-04-10 16:46         ` Segher Boessenkool
2021-04-10  8:31   ` Xiongwei Song
2021-04-10  8:31     ` Xiongwei Song

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.