Re: [PATCH v3] powerpc/traps: Enhance readability for trap types

From: Xiongwei Song
Date: Sat Apr 10 2021 - 04:34:15 EST

Next message: Greg KH: "Re: [PATCH 0/6] staging: rtl8192e: cleanup patchset for style issues"
Previous message: Julia Lawall: "Re: [Outreachy kernel] [Patch 0/3]"
In reply to: Segher Boessenkool: "Re: [PATCH v3] powerpc/traps: Enhance readability for trap types"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

> On Apr 10, 2021, at 12:14 AM, Christophe Leroy <christophe.leroy@xxxxxxxxxx> wrote:
>
>
>
> Le 08/04/2021 à 16:07, Xiongwei Song a écrit :
>> From: Xiongwei Song <sxwjean@xxxxxxxxx>
>> 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@xxxxxxxxx>
>> ---
>> 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);
>> }
>>

Next message: Greg KH: "Re: [PATCH 0/6] staging: rtl8192e: cleanup patchset for style issues"
Previous message: Julia Lawall: "Re: [Outreachy kernel] [Patch 0/3]"
In reply to: Segher Boessenkool: "Re: [PATCH v3] powerpc/traps: Enhance readability for trap types"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]