From: Xiongwei Song <[email protected]>
Create a new header named traps.h, define macros to list ppc exception
types in traps.h, replace the reference of the real trap values with
these macros.
Signed-off-by: Xiongwei Song <[email protected]>
---
arch/powerpc/include/asm/interrupt.h | 7 ++++---
arch/powerpc/include/asm/ptrace.h | 3 ++-
arch/powerpc/include/asm/traps.h | 19 +++++++++++++++++++
arch/powerpc/kernel/interrupt.c | 2 +-
arch/powerpc/kernel/process.c | 3 ++-
arch/powerpc/kernel/traps.c | 5 +++--
arch/powerpc/kexec/crash.c | 3 ++-
arch/powerpc/kvm/book3s_hv_builtin.c | 5 +++--
arch/powerpc/mm/book3s64/hash_utils.c | 5 +++--
arch/powerpc/mm/fault.c | 17 +++++++++--------
arch/powerpc/perf/core-book3s.c | 5 +++--
arch/powerpc/xmon/xmon.c | 21 +++++++++++----------
12 files changed, 62 insertions(+), 33 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..d4c935ba8e16 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) != TRAP_PROG)
CT_WARN_ON(ct_state() != CONTEXT_KERNEL);
}
#endif
@@ -156,7 +157,7 @@ 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) != TRAP_DEC && TRAP(regs) != TRAP_PMI && TRAP(regs) != 0x260) {
state->ftrace_enabled = this_cpu_get_ftrace_enabled();
this_cpu_set_ftrace_enabled(0);
}
@@ -180,7 +181,7 @@ 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) != TRAP_DEC && TRAP(regs) != TRAP_PMI && TRAP(regs) != 0x260)
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..04726fb43a9d 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) == TRAP_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..a31b6122de23
--- /dev/null
+++ b/arch/powerpc/include/asm/traps.h
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_PPC_TRAPS_H
+#define _ASM_PPC_TRAPS_H
+
+#define TRAP_RESET 0x100 /* System reset */
+#define TRAP_MCE 0x200 /* Machine check */
+#define TRAP_DSI 0x300 /* Data storage */
+#define TRAP_DSEGI 0x380 /* Data segment */
+#define TRAP_ISI 0x400 /* Instruction storage */
+#define TRAP_ISEGI 0x480 /* Instruction segment */
+#define TRAP_ALIGN 0x600 /* Alignment */
+#define TRAP_PROG 0x700 /* Program */
+#define TRAP_DEC 0x900 /* Decrementer */
+#define TRAP_SYSCALL 0xc00 /* System call */
+#define TRAP_TRACEI 0xd00 /* Trace */
+#define TRAP_FPA 0xe00 /* Floating-point Assist */
+#define TRAP_PMI 0xf00 /* Performance monitor */
+
+#endif /* _ASM_PPC_TRAPS_H */
diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
index c4dd4b8f9cfa..fc9a40cbbcae 100644
--- a/arch/powerpc/kernel/interrupt.c
+++ b/arch/powerpc/kernel/interrupt.c
@@ -456,7 +456,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) != TRAP_PROG)
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..0d416744136f 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,7 @@ 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 == TRAP_MCE || trap == TRAP_DSI || trap == TRAP_ALIGN) {
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/kernel/traps.c b/arch/powerpc/kernel/traps.c
index 76d17492e0e5..c9fa10e20140 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -67,6 +67,7 @@
#include <asm/kprobes.h>
#include <asm/stacktrace.h>
#include <asm/nmi.h>
+#include <asm/traps.h>
#if defined(CONFIG_DEBUGGER) || defined(CONFIG_KEXEC_CORE)
int (*__debugger)(struct pt_regs *regs) __read_mostly;
@@ -221,7 +222,7 @@ static void oops_end(unsigned long flags, struct pt_regs *regs,
/*
* system_reset_excption handles debugger, crash dump, panic, for 0x100
*/
- if (TRAP(regs) == 0x100)
+ if (TRAP(regs) == TRAP_RESET)
return;
crash_fadump(regs, "die oops");
@@ -289,7 +290,7 @@ void die(const char *str, struct pt_regs *regs, long err)
/*
* system_reset_excption handles debugger, crash dump, panic, for 0x100
*/
- if (TRAP(regs) != 0x100) {
+ if (TRAP(regs) != TRAP_RESET) {
if (debugger(regs))
return;
}
diff --git a/arch/powerpc/kexec/crash.c b/arch/powerpc/kexec/crash.c
index c9a889880214..5246969e3f68 100644
--- a/arch/powerpc/kexec/crash.c
+++ b/arch/powerpc/kexec/crash.c
@@ -24,6 +24,7 @@
#include <asm/smp.h>
#include <asm/setjmp.h>
#include <asm/debug.h>
+#include <asm/traps.h>
/*
* The primary CPU waits a while for all secondary CPUs to enter. This is to
@@ -336,7 +337,7 @@ void default_machine_crash_shutdown(struct pt_regs *regs)
* If we came in via system reset, wait a while for the secondary
* CPUs to enter.
*/
- if (TRAP(regs) == 0x100)
+ if (TRAP(regs) == TRAP_RESET)
mdelay(PRIMARY_TIMEOUT);
crash_kexec_prepare_cpus(crashing_cpu);
diff --git a/arch/powerpc/kvm/book3s_hv_builtin.c b/arch/powerpc/kvm/book3s_hv_builtin.c
index 158d309b42a3..795d4a2bc8e3 100644
--- a/arch/powerpc/kvm/book3s_hv_builtin.c
+++ b/arch/powerpc/kvm/book3s_hv_builtin.c
@@ -28,6 +28,7 @@
#include <asm/io.h>
#include <asm/opal.h>
#include <asm/smp.h>
+#include <asm/traps.h>
#define KVM_CMA_CHUNK_ORDER 18
@@ -639,11 +640,11 @@ void kvmppc_bad_interrupt(struct pt_regs *regs)
* 100 could happen at any time, 200 can happen due to invalid real
* address access for example (or any time due to a hardware problem).
*/
- if (TRAP(regs) == 0x100) {
+ if (TRAP(regs) == TRAP_RESET) {
get_paca()->in_nmi++;
system_reset_exception(regs);
get_paca()->in_nmi--;
- } else if (TRAP(regs) == 0x200) {
+ } else if (TRAP(regs) == TRAP_MCE) {
machine_check_exception(regs);
} else {
die("Bad interrupt in KVM entry/exit code", regs, SIGABRT);
diff --git a/arch/powerpc/mm/book3s64/hash_utils.c b/arch/powerpc/mm/book3s64/hash_utils.c
index 7719995323c3..97ff82a1925f 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 == TRAP_ISI) {
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) == TRAP_ISI)
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..fae9c072e498 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) == TRAP_ISI;
/* 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) == TRAP_ISI;
int is_user = user_mode(regs);
int is_write = page_fault_is_write(error_code);
vm_fault_t fault, major = 0;
@@ -588,20 +589,20 @@ 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 TRAP_DSI:
+ case TRAP_DSEGI:
+ case TRAP_FPA:
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 TRAP_ISI:
+ case TRAP_ISEGI:
pr_alert("BUG: Unable to handle kernel instruction fetch%s",
regs->nip < PAGE_SIZE ? " (NULL pointer?)\n" : "\n");
break;
- case 0x600:
+ case TRAP_ALIGN:
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..15fa471d8205 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) == TRAP_PMI) && 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) != TRAP_PMI)
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..570277c4d471 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>
@@ -605,7 +606,7 @@ static int xmon_core(struct pt_regs *regs, int fromipi)
* debugger break (IPI). This is similar to
* crash_kexec_secondary().
*/
- if (TRAP(regs) != 0x100 || !wait_for_other_cpus(ncpus))
+ if (TRAP(regs) != TRAP_RESET || !wait_for_other_cpus(ncpus))
smp_send_debugger_break();
wait_for_other_cpus(ncpus);
@@ -615,7 +616,7 @@ static int xmon_core(struct pt_regs *regs, int fromipi)
if (!locked_down) {
/* for breakpoint or single step, print curr insn */
- if (bp || TRAP(regs) == 0xd00)
+ if (bp || TRAP(regs) == TRAP_TRACEI)
ppc_inst_dump(regs->nip, 1, 0);
printf("enter ? for help\n");
}
@@ -684,7 +685,7 @@ static int xmon_core(struct pt_regs *regs, int fromipi)
disable_surveillance();
if (!locked_down) {
/* for breakpoint or single step, print current insn */
- if (bp || TRAP(regs) == 0xd00)
+ if (bp || TRAP(regs) == TRAP_TRACEI)
ppc_inst_dump(regs->nip, 1, 0);
printf("enter ? for help\n");
}
@@ -1769,9 +1770,9 @@ 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 == TRAP_DSI || trap == TRAP_DSEGI || trap == TRAP_ALIGN || trap == TRAP_MCE) {
printf(" dar: %lx\n", fp->dar);
- if (trap != 0x380)
+ if (trap != TRAP_DSEGI)
printf(" dsisr: %lx\n", fp->dsisr);
}
@@ -1785,7 +1786,7 @@ static void excprint(struct pt_regs *fp)
current->pid, current->comm);
}
- if (trap == 0x700)
+ if (trap == TRAP_PROG)
print_bug_trap(fp);
printf(linux_banner);
@@ -1846,7 +1847,7 @@ 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 == TRAP_DSI || trap == TRAP_DSEGI || trap == TRAP_ALIGN)
printf("dar = "REG" dsisr = %.8lx\n", fp->dar, fp->dsisr);
}
@@ -2235,11 +2236,11 @@ static int handle_fault(struct pt_regs *regs)
{
fault_except = TRAP(regs);
switch (TRAP(regs)) {
- case 0x200:
+ case TRAP_MCE:
fault_type = 0;
break;
- case 0x300:
- case 0x380:
+ case TRAP_DSI:
+ case TRAP_DSEGI:
fault_type = 1;
break;
default:
--
2.17.1
Xiongwei Song <[email protected]> writes:
> From: Xiongwei Song <[email protected]>
>
> Create a new header named traps.h, define macros to list ppc exception
> types in traps.h, replace the reference of the real trap values with
> these macros.
Personally I find the hex values easier to recognise, but I realise
that's probably not true of other people :)
...
> diff --git a/arch/powerpc/include/asm/traps.h b/arch/powerpc/include/asm/traps.h
> new file mode 100644
> index 000000000000..a31b6122de23
> --- /dev/null
> +++ b/arch/powerpc/include/asm/traps.h
> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _ASM_PPC_TRAPS_H
> +#define _ASM_PPC_TRAPS_H
> +
> +#define TRAP_RESET 0x100 /* System reset */
> +#define TRAP_MCE 0x200 /* Machine check */
> +#define TRAP_DSI 0x300 /* Data storage */
> +#define TRAP_DSEGI 0x380 /* Data segment */
> +#define TRAP_ISI 0x400 /* Instruction storage */
> +#define TRAP_ISEGI 0x480 /* Instruction segment */
> +#define TRAP_ALIGN 0x600 /* Alignment */
> +#define TRAP_PROG 0x700 /* Program */
> +#define TRAP_DEC 0x900 /* Decrementer */
> +#define TRAP_SYSCALL 0xc00 /* System call */
> +#define TRAP_TRACEI 0xd00 /* Trace */
> +#define TRAP_FPA 0xe00 /* Floating-point Assist */
> +#define TRAP_PMI 0xf00 /* Performance monitor */
I know the macro is called TRAP and the field in pt_regs is called trap,
but the terminology in the architecture is "exception", and we already
have many uses of that. In particular we have a lot of uses of "exc" as
an abbreviation for "exception". So I think I'd rather we use that than
"TRAP".
I think we should probably use the names from the ISA, unless they are
really over long.
Which are:
0x100 System Reset
0x200 Machine Check
0x300 Data Storage
0x380 Data Segment
0x400 Instruction Storage
0x480 Instruction Segment
0x500 External
0x600 Alignment
0x700 Program
0x800 Floating-Point Unavailable
0x900 Decrementer
0x980 Hypervisor Decrementer
0xA00 Directed Privileged Doorbell
0xC00 System Call
0xD00 Trace
0xE00 Hypervisor Data Storage
0xE20 Hypervisor Instruction Storage
0xE40 Hypervisor Emulation Assistance
0xE60 Hypervisor Maintenance
0xE80 Directed Hypervisor Doorbell
0xEA0 Hypervisor Virtualization
0xF00 Performance Monitor
0xF20 Vector Unavailable
0xF40 VSX Unavailable
0xF60 Facility Unavailable
0xF80 Hypervisor Facility Unavailable
0xFA0 Directed Ultravisor Doorbell
So perhaps:
EXC_SYSTEM_RESET
EXC_MACHINE_CHECK
EXC_DATA_STORAGE
EXC_DATA_SEGMENT
EXC_INST_STORAGE
EXC_INST_SEGMENT
EXC_EXTERNAL_INTERRUPT
EXC_ALIGNMENT
EXC_PROGRAM_CHECK
EXC_FP_UNAVAILABLE
EXC_DECREMENTER
EXC_HV_DECREMENTER
EXC_SYSTEM_CALL
EXC_HV_DATA_STORAGE
EXC_PERF_MONITOR
cheers
On Wed, Mar 31, 2021 at 08:58:17PM +1100, Michael Ellerman wrote:
> So perhaps:
>
> EXC_SYSTEM_RESET
> EXC_MACHINE_CHECK
> EXC_DATA_STORAGE
> EXC_DATA_SEGMENT
> EXC_INST_STORAGE
> EXC_INST_SEGMENT
> EXC_EXTERNAL_INTERRUPT
> EXC_ALIGNMENT
> EXC_PROGRAM_CHECK
> EXC_FP_UNAVAILABLE
> EXC_DECREMENTER
> EXC_HV_DECREMENTER
> EXC_SYSTEM_CALL
> EXC_HV_DATA_STORAGE
> EXC_PERF_MONITOR
These are interrupt (vectors), not exceptions. It doesn't matter all
that much, but confusing things more isn't useful either! There can be
multiple exceptions that all can trigger the same interrupt.
Segher
Segher Boessenkool <[email protected]> writes:
> On Wed, Mar 31, 2021 at 08:58:17PM +1100, Michael Ellerman wrote:
>> So perhaps:
>>
>> EXC_SYSTEM_RESET
>> EXC_MACHINE_CHECK
>> EXC_DATA_STORAGE
>> EXC_DATA_SEGMENT
>> EXC_INST_STORAGE
>> EXC_INST_SEGMENT
>> EXC_EXTERNAL_INTERRUPT
>> EXC_ALIGNMENT
>> EXC_PROGRAM_CHECK
>> EXC_FP_UNAVAILABLE
>> EXC_DECREMENTER
>> EXC_HV_DECREMENTER
>> EXC_SYSTEM_CALL
>> EXC_HV_DATA_STORAGE
>> EXC_PERF_MONITOR
>
> These are interrupt (vectors), not exceptions. It doesn't matter all
> that much, but confusing things more isn't useful either! There can be
> multiple exceptions that all can trigger the same interrupt.
Yeah I know, but I think that ship has already sailed as far as the
naming we have in the kernel.
We have over 250 uses of "exc", and several files called "exception"
something.
Using "interrupt" can also be confusing because Linux uses that to mean
"external interrupt".
But I dunno, maybe INT or VEC is clearer? .. or TRAP :)
cheers
Excerpts from Michael Ellerman's message of April 1, 2021 12:39 pm:
> Segher Boessenkool <[email protected]> writes:
>> On Wed, Mar 31, 2021 at 08:58:17PM +1100, Michael Ellerman wrote:
>>> So perhaps:
>>>
>>> EXC_SYSTEM_RESET
>>> EXC_MACHINE_CHECK
>>> EXC_DATA_STORAGE
>>> EXC_DATA_SEGMENT
>>> EXC_INST_STORAGE
>>> EXC_INST_SEGMENT
>>> EXC_EXTERNAL_INTERRUPT
>>> EXC_ALIGNMENT
>>> EXC_PROGRAM_CHECK
>>> EXC_FP_UNAVAILABLE
>>> EXC_DECREMENTER
>>> EXC_HV_DECREMENTER
>>> EXC_SYSTEM_CALL
>>> EXC_HV_DATA_STORAGE
>>> EXC_PERF_MONITOR
>>
>> These are interrupt (vectors), not exceptions. It doesn't matter all
>> that much, but confusing things more isn't useful either! There can be
>> multiple exceptions that all can trigger the same interrupt.
>
> Yeah I know, but I think that ship has already sailed as far as the
> naming we have in the kernel.
It has, but there are also several other ships also sailing in different
directions. It could be worse though, at least they are not sideways in
the Suez.
> We have over 250 uses of "exc", and several files called "exception"
> something.
>
> Using "interrupt" can also be confusing because Linux uses that to mean
> "external interrupt".
>
> But I dunno, maybe INT or VEC is clearer? .. or TRAP :)
We actually already have defines that follow Segher's suggestion, it's
just that they're hidden away in a KVM header.
#define BOOK3S_INTERRUPT_SYSTEM_RESET 0x100
#define BOOK3S_INTERRUPT_MACHINE_CHECK 0x200
#define BOOK3S_INTERRUPT_DATA_STORAGE 0x300
#define BOOK3S_INTERRUPT_DATA_SEGMENT 0x380
#define BOOK3S_INTERRUPT_INST_STORAGE 0x400
#define BOOK3S_INTERRUPT_INST_SEGMENT 0x480
#define BOOK3S_INTERRUPT_EXTERNAL 0x500
#define BOOK3S_INTERRUPT_EXTERNAL_HV 0x502
#define BOOK3S_INTERRUPT_ALIGNMENT 0x600
It would take just a small amount of work to move these to general
powerpc header, add #ifdefs for Book E/S where the numbers differ,
and remove the BOOK3S_ prefix.
I don't mind INTERRUPT_ but INT_ would be okay too. VEC_ actually
doesn't match what Book E does (which is some weirdness to map some
of them to match Book S but not all, arguably we should clean that
up too and just use vector numbers consistently, but the INTERRUPT_
prefix would still be valid if we did that).
BookE KVM entry will still continue to use a different convention
there so I would leave all those KVM defines in place for now, we
might do another pass on them later.
Thanks,
Nick
On Thu, Apr 01, 2021 at 10:55:58AM +0800, Xiongwei Song wrote:
> Segher Boessenkool <[email protected]> 于2021年4月1日周四 上午6:15写道：
>
> > On Wed, Mar 31, 2021 at 08:58:17PM +1100, Michael Ellerman wrote:
> > > So perhaps:
> > >
> > > EXC_SYSTEM_RESET
> > > EXC_MACHINE_CHECK
> > > EXC_DATA_STORAGE
> > > EXC_DATA_SEGMENT
> > > EXC_INST_STORAGE
> > > EXC_INST_SEGMENT
> > > EXC_EXTERNAL_INTERRUPT
> > > EXC_ALIGNMENT
> > > EXC_PROGRAM_CHECK
> > > EXC_FP_UNAVAILABLE
> > > EXC_DECREMENTER
> > > EXC_HV_DECREMENTER
> > > EXC_SYSTEM_CALL
> > > EXC_HV_DATA_STORAGE
> > > EXC_PERF_MONITOR
> >
> > These are interrupt (vectors), not exceptions. It doesn't matter all
> > that much, but confusing things more isn't useful either! There can be
> > multiple exceptions that all can trigger the same interrupt.
> >
> > When looking at the reference manual of e500 and e600 from NXP
> official, they call them as interrupts.While looking at the "The
> Programming Environments"
> that is also from NXP, they call them exceptions. Looks like there is
> no explicit distinction between interrupts and exceptions.
The architecture documents have always called it interrupts. The PEM
says it calls them exceptions instead, but they are called interrupts in
the architecture (and the PEM says that, too).
> Here is the "The Programming Environments" link:
> https://www.nxp.com.cn/docs/en/user-guide/MPCFPE_AD_R1.pdf
That document is 24 years old. The architecture is still published,
new versions regularly.
> As far as I know, the values of interrupts or exceptions above are defined
> explicitly in reference manual or the programming environments.
They are defined in the architecture.
> Could
> you please provide more details about multiple exceptions with the same
> interrupts?
The simplest example is 700, program interrupt. There are many causes
for it, including all the exceptions in FPSCR: VX, ZX, OX, UX, XX, and
VX is actually divided into nine separate cases itself. There also are
the various causes of privileged instruction type program interrupts,
and the trap type program interrupt, but the FEX ones are most obvious
here.
Segher
On Thu, Apr 01, 2021 at 06:01:29PM +1000, Nicholas Piggin wrote:
> Excerpts from Michael Ellerman's message of April 1, 2021 12:39 pm:
> > Segher Boessenkool <[email protected]> writes:
> >> On Wed, Mar 31, 2021 at 08:58:17PM +1100, Michael Ellerman wrote:
> >>> So perhaps:
> >>>
> >>> EXC_SYSTEM_RESET
> >>> EXC_MACHINE_CHECK
> >>> EXC_DATA_STORAGE
> >>> EXC_DATA_SEGMENT
> >>> EXC_INST_STORAGE
> >>> EXC_INST_SEGMENT
> >>> EXC_EXTERNAL_INTERRUPT
> >>> EXC_ALIGNMENT
> >>> EXC_PROGRAM_CHECK
> >>> EXC_FP_UNAVAILABLE
> >>> EXC_DECREMENTER
> >>> EXC_HV_DECREMENTER
> >>> EXC_SYSTEM_CALL
> >>> EXC_HV_DATA_STORAGE
> >>> EXC_PERF_MONITOR
> >>
> >> These are interrupt (vectors), not exceptions. It doesn't matter all
> >> that much, but confusing things more isn't useful either! There can be
> >> multiple exceptions that all can trigger the same interrupt.
> >
> > Yeah I know, but I think that ship has already sailed as far as the
> > naming we have in the kernel.
>
> It has, but there are also several other ships also sailing in different
> directions. It could be worse though, at least they are not sideways in
> the Suez.
:-)
> > We have over 250 uses of "exc", and several files called "exception"
> > something.
> >
> > Using "interrupt" can also be confusing because Linux uses that to mean
> > "external interrupt".
> >
> > But I dunno, maybe INT or VEC is clearer? .. or TRAP :)
>
> We actually already have defines that follow Segher's suggestion, it's
> just that they're hidden away in a KVM header.
>
> #define BOOK3S_INTERRUPT_SYSTEM_RESET 0x100
> #define BOOK3S_INTERRUPT_MACHINE_CHECK 0x200
> #define BOOK3S_INTERRUPT_DATA_STORAGE 0x300
> #define BOOK3S_INTERRUPT_DATA_SEGMENT 0x380
> #define BOOK3S_INTERRUPT_INST_STORAGE 0x400
> #define BOOK3S_INTERRUPT_INST_SEGMENT 0x480
> #define BOOK3S_INTERRUPT_EXTERNAL 0x500
> #define BOOK3S_INTERRUPT_EXTERNAL_HV 0x502
> #define BOOK3S_INTERRUPT_ALIGNMENT 0x600
>
> It would take just a small amount of work to move these to general
> powerpc header, add #ifdefs for Book E/S where the numbers differ,
> and remove the BOOK3S_ prefix.
>
> I don't mind INTERRUPT_ but INT_ would be okay too. VEC_ actually
> doesn't match what Book E does (which is some weirdness to map some
> of them to match Book S but not all, arguably we should clean that
> up too and just use vector numbers consistently, but the INTERRUPT_
> prefix would still be valid if we did that).
VEC also is pretty incorrect: there is code at those addresses, not
vectors pointing to code (as similar things on some other architectures
have). Everyone understands what it means of course, except it is
confusing with a thing we *do* have on Power called VEC (the MSR bit) :-P
(And TRAP is just one cause of 700...)
Segher
Excerpts from Segher Boessenkool's message of April 2, 2021 2:11 am:
> On Thu, Apr 01, 2021 at 10:55:58AM +0800, Xiongwei Song wrote:
>> Segher Boessenkool <[email protected]> 于2021年4月1日周四 上午6:15写道：
>>
>> > On Wed, Mar 31, 2021 at 08:58:17PM +1100, Michael Ellerman wrote:
>> > > So perhaps:
>> > >
>> > > EXC_SYSTEM_RESET
>> > > EXC_MACHINE_CHECK
>> > > EXC_DATA_STORAGE
>> > > EXC_DATA_SEGMENT
>> > > EXC_INST_STORAGE
>> > > EXC_INST_SEGMENT
>> > > EXC_EXTERNAL_INTERRUPT
>> > > EXC_ALIGNMENT
>> > > EXC_PROGRAM_CHECK
>> > > EXC_FP_UNAVAILABLE
>> > > EXC_DECREMENTER
>> > > EXC_HV_DECREMENTER
>> > > EXC_SYSTEM_CALL
>> > > EXC_HV_DATA_STORAGE
>> > > EXC_PERF_MONITOR
>> >
>> > These are interrupt (vectors), not exceptions. It doesn't matter all
>> > that much, but confusing things more isn't useful either! There can be
>> > multiple exceptions that all can trigger the same interrupt.
>> >
>> > When looking at the reference manual of e500 and e600 from NXP
>> official, they call them as interrupts.While looking at the "The
>> Programming Environments"
>> that is also from NXP, they call them exceptions. Looks like there is
>> no explicit distinction between interrupts and exceptions.
>
> The architecture documents have always called it interrupts. The PEM
> says it calls them exceptions instead, but they are called interrupts in
> the architecture (and the PEM says that, too).
>
>> Here is the "The Programming Environments" link:
>> https://www.nxp.com.cn/docs/en/user-guide/MPCFPE_AD_R1.pdf
>
> That document is 24 years old. The architecture is still published,
> new versions regularly.
>
>> As far as I know, the values of interrupts or exceptions above are defined
>> explicitly in reference manual or the programming environments.
>
> They are defined in the architecture.
>
>> Could
>> you please provide more details about multiple exceptions with the same
>> interrupts?
>
> The simplest example is 700, program interrupt. There are many causes
> for it, including all the exceptions in FPSCR: VX, ZX, OX, UX, XX, and
> VX is actually divided into nine separate cases itself. There also are
> the various causes of privileged instruction type program interrupts,
> and the trap type program interrupt, but the FEX ones are most obvious
> here.
Also:
* Some interrupts have no corresponding exception (system call and
system call vectored). This is not just semantics or a bug in the ISA
because it is different from other synchronous interrupts: instructions
which cause exceptions (e.g., a page fault) do not complete before
taking the interrupt whereas sc does.
* It's quite usual for an exception to not cause an interrupt
immediately (MSR[EE]=0, HMEER) or never cause one and be cleared by
other means (msgclr, mtDEC, mtHMER, etc).
* It's possible for an exception to cause different interrupts!
A decrementer exception usually causes a decrementer interrupt, but it
can cause a system reset interrupt if the processor was in a power
saving mode. A data storage exception can cause a DSI or HDSI interrupt
depending on LPCR settings, and many other examples.
So I agree with Segher on this. We should use interrupt for interrupts,
reduce exception except where we really mean it, and move away from vec
and trap (I've got this wrong in the past too I admit). We don't have to
do it all immediately, but new code should go in this direction.
Thanks,
Nick
> On Apr 2, 2021, at 12:11 AM, Segher Boessenkool <[email protected]> wrote:
>
> On Thu, Apr 01, 2021 at 10:55:58AM +0800, Xiongwei Song wrote:
>> Segher Boessenkool <[email protected]> 于2021年4月1日周四 上午6:15写道：
>>
>>> On Wed, Mar 31, 2021 at 08:58:17PM +1100, Michael Ellerman wrote:
>>>> So perhaps:
>>>>
>>>> EXC_SYSTEM_RESET
>>>> EXC_MACHINE_CHECK
>>>> EXC_DATA_STORAGE
>>>> EXC_DATA_SEGMENT
>>>> EXC_INST_STORAGE
>>>> EXC_INST_SEGMENT
>>>> EXC_EXTERNAL_INTERRUPT
>>>> EXC_ALIGNMENT
>>>> EXC_PROGRAM_CHECK
>>>> EXC_FP_UNAVAILABLE
>>>> EXC_DECREMENTER
>>>> EXC_HV_DECREMENTER
>>>> EXC_SYSTEM_CALL
>>>> EXC_HV_DATA_STORAGE
>>>> EXC_PERF_MONITOR
>>>
>>> These are interrupt (vectors), not exceptions. It doesn't matter all
>>> that much, but confusing things more isn't useful either! There can be
>>> multiple exceptions that all can trigger the same interrupt.
>>>
>>> When looking at the reference manual of e500 and e600 from NXP
>> official, they call them as interrupts.While looking at the "The
>> Programming Environments"
>> that is also from NXP, they call them exceptions. Looks like there is
>> no explicit distinction between interrupts and exceptions.
>
> The architecture documents have always called it interrupts. The PEM
> says it calls them exceptions instead, but they are called interrupts in
> the architecture (and the PEM says that, too).
>
>> Here is the "The Programming Environments" link:
>> https://www.nxp.com.cn/docs/en/user-guide/MPCFPE_AD_R1.pdf
>
> That document is 24 years old. The architecture is still published,
> new versions regularly.
>
>> As far as I know, the values of interrupts or exceptions above are defined
>> explicitly in reference manual or the programming environments.
>
> They are defined in the architecture.
>
>> Could
>> you please provide more details about multiple exceptions with the same
>> interrupts?
>
> The simplest example is 700, program interrupt. There are many causes
> for it, including all the exceptions in FPSCR: VX, ZX, OX, UX, XX, and
> VX is actually divided into nine separate cases itself. There also are
> the various causes of privileged instruction type program interrupts,
> and the trap type program interrupt, but the FEX ones are most obvious
> here.
Thanks for the explanation.
Regards,
Xiongwei
>
>
> Segher
Regards,
Xiongwei
> On Apr 1, 2021, at 4:01 PM, Nicholas Piggin <[email protected]> wrote:
>
> Excerpts from Michael Ellerman's message of April 1, 2021 12:39 pm:
>> Segher Boessenkool <[email protected]> writes:
>>> On Wed, Mar 31, 2021 at 08:58:17PM +1100, Michael Ellerman wrote:
>>>> So perhaps:
>>>>
>>>> EXC_SYSTEM_RESET
>>>> EXC_MACHINE_CHECK
>>>> EXC_DATA_STORAGE
>>>> EXC_DATA_SEGMENT
>>>> EXC_INST_STORAGE
>>>> EXC_INST_SEGMENT
>>>> EXC_EXTERNAL_INTERRUPT
>>>> EXC_ALIGNMENT
>>>> EXC_PROGRAM_CHECK
>>>> EXC_FP_UNAVAILABLE
>>>> EXC_DECREMENTER
>>>> EXC_HV_DECREMENTER
>>>> EXC_SYSTEM_CALL
>>>> EXC_HV_DATA_STORAGE
>>>> EXC_PERF_MONITOR
>>>
>>> These are interrupt (vectors), not exceptions. It doesn't matter all
>>> that much, but confusing things more isn't useful either! There can be
>>> multiple exceptions that all can trigger the same interrupt.
>>
>> Yeah I know, but I think that ship has already sailed as far as the
>> naming we have in the kernel.
>
> It has, but there are also several other ships also sailing in different
> directions. It could be worse though, at least they are not sideways in
> the Suez.
>
>> We have over 250 uses of "exc", and several files called "exception"
>> something.
>>
>> Using "interrupt" can also be confusing because Linux uses that to mean
>> "external interrupt".
>>
>> But I dunno, maybe INT or VEC is clearer? .. or TRAP :)
>
> We actually already have defines that follow Segher's suggestion, it's
> just that they're hidden away in a KVM header.
>
> #define BOOK3S_INTERRUPT_SYSTEM_RESET 0x100
> #define BOOK3S_INTERRUPT_MACHINE_CHECK 0x200
> #define BOOK3S_INTERRUPT_DATA_STORAGE 0x300
> #define BOOK3S_INTERRUPT_DATA_SEGMENT 0x380
> #define BOOK3S_INTERRUPT_INST_STORAGE 0x400
> #define BOOK3S_INTERRUPT_INST_SEGMENT 0x480
> #define BOOK3S_INTERRUPT_EXTERNAL 0x500
> #define BOOK3S_INTERRUPT_EXTERNAL_HV 0x502
> #define BOOK3S_INTERRUPT_ALIGNMENT 0x600
>
> It would take just a small amount of work to move these to general
> powerpc header, add #ifdefs for Book E/S where the numbers differ,
> and remove the BOOK3S_ prefix.
>
> I don't mind INTERRUPT_ but INT_ would be okay too. VEC_ actually
> doesn't match what Book E does (which is some weirdness to map some
> of them to match Book S but not all, arguably we should clean that
> up too and just use vector numbers consistently, but the INTERRUPT_
> prefix would still be valid if we did that).
>
> BookE KVM entry will still continue to use a different convention
> there so I would leave all those KVM defines in place for now, we
> might do another pass on them later.
Thanks for the comments.
>
> Thanks,
> Nick
Regards,
Xiongwei
> On Apr 2, 2021, at 8:36 AM, Nicholas Piggin <[email protected]> wrote:
>
> Excerpts from Segher Boessenkool's message of April 2, 2021 2:11 am:
>> On Thu, Apr 01, 2021 at 10:55:58AM +0800, Xiongwei Song wrote:
>>> Segher Boessenkool <[email protected]> 于2021年4月1日周四 上午6:15写道：
>>>
>>>> On Wed, Mar 31, 2021 at 08:58:17PM +1100, Michael Ellerman wrote:
>>>>> So perhaps:
>>>>>
>>>>> EXC_SYSTEM_RESET
>>>>> EXC_MACHINE_CHECK
>>>>> EXC_DATA_STORAGE
>>>>> EXC_DATA_SEGMENT
>>>>> EXC_INST_STORAGE
>>>>> EXC_INST_SEGMENT
>>>>> EXC_EXTERNAL_INTERRUPT
>>>>> EXC_ALIGNMENT
>>>>> EXC_PROGRAM_CHECK
>>>>> EXC_FP_UNAVAILABLE
>>>>> EXC_DECREMENTER
>>>>> EXC_HV_DECREMENTER
>>>>> EXC_SYSTEM_CALL
>>>>> EXC_HV_DATA_STORAGE
>>>>> EXC_PERF_MONITOR
>>>>
>>>> These are interrupt (vectors), not exceptions. It doesn't matter all
>>>> that much, but confusing things more isn't useful either! There can be
>>>> multiple exceptions that all can trigger the same interrupt.
>>>>
>>>> When looking at the reference manual of e500 and e600 from NXP
>>> official, they call them as interrupts.While looking at the "The
>>> Programming Environments"
>>> that is also from NXP, they call them exceptions. Looks like there is
>>> no explicit distinction between interrupts and exceptions.
>>
>> The architecture documents have always called it interrupts. The PEM
>> says it calls them exceptions instead, but they are called interrupts in
>> the architecture (and the PEM says that, too).
>>
>>> Here is the "The Programming Environments" link:
>>> https://www.nxp.com.cn/docs/en/user-guide/MPCFPE_AD_R1.pdf
>>
>> That document is 24 years old. The architecture is still published,
>> new versions regularly.
>>
>>> As far as I know, the values of interrupts or exceptions above are defined
>>> explicitly in reference manual or the programming environments.
>>
>> They are defined in the architecture.
>>
>>> Could
>>> you please provide more details about multiple exceptions with the same
>>> interrupts?
>>
>> The simplest example is 700, program interrupt. There are many causes
>> for it, including all the exceptions in FPSCR: VX, ZX, OX, UX, XX, and
>> VX is actually divided into nine separate cases itself. There also are
>> the various causes of privileged instruction type program interrupts,
>> and the trap type program interrupt, but the FEX ones are most obvious
>> here.
>
> Also:
>
> * Some interrupts have no corresponding exception (system call and
> system call vectored). This is not just semantics or a bug in the ISA
> because it is different from other synchronous interrupts: instructions
> which cause exceptions (e.g., a page fault) do not complete before
> taking the interrupt whereas sc does.
>
> * It's quite usual for an exception to not cause an interrupt
> immediately (MSR[EE]=0, HMEER) or never cause one and be cleared by
> other means (msgclr, mtDEC, mtHMER, etc).
>
> * It's possible for an exception to cause different interrupts!
> A decrementer exception usually causes a decrementer interrupt, but it
> can cause a system reset interrupt if the processor was in a power
> saving mode. A data storage exception can cause a DSI or HDSI interrupt
> depending on LPCR settings, and many other examples.
>
> So I agree with Segher on this. We should use interrupt for interrupts,
> reduce exception except where we really mean it, and move away from vec
> and trap (I've got this wrong in the past too I admit). We don't have to
> do it all immediately, but new code should go in this direction.
Appreciate these details about exceptions and interrupts. Looks like interrupt
is the correct term here. I’m glad to create the v3 patch with INTERRUPT_
prefix.
Regards,
Xiongwei
>
> Thanks,
> Nick