Messages in this thread | | | Subject | Re: [PATCH v3] powerpc/traps: Enhance readability for trap types | From | Xiongwei Song <> | Date | Sat, 10 Apr 2021 16:31:25 +0800 |
| |
> 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); >> } >>
| |