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

On Apr 10, 2021, at 8:35 AM, Nicholas Piggin [off-list ref] wrote:

Thanks for working on this, I think it's a nice cleanup and helps
non-powerpc people understand the code a bit better.

My pleasure.

Excerpts from Xiongwei Song's message of April 10, 2021 12:28 am:

quoted

From: Xiongwei Song <redacted>

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

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

Reported-by: kernel test robot <redacted>

It now looks like lkp asked for this whole cleanup patch. I would
put [kernel test robot [off-list ref]] in your v3->4 changelog
item.

Agree. I just forgot to delete this line in the patch.

quoted

Signed-off-by: Xiongwei Song <redacted>
---

v3-v4:
Fix compile issue:
arch/powerpc/kernel/process.c:1473:14: error: 'INTERRUPT_MACHINE_CHECK' undeclared (first use in this function); did you mean 'TAINT_MACHINE_CHECK'?
I didn't add "Reported-by: kernel test robot [off-list ref]" here,
because it's improper for this patch.

[...]

quoted

diff --git a/arch/powerpc/include/asm/traps.h b/arch/powerpc/include/asm/traps.h
new file mode 100644
index 000000000000..2e64e10afcef
--- /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

These could go in interrupt.h.

quoted

+#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
+#else
+#define INTERRUPT_SYSTEM_RESET    0x100
+#define INTERRUPT_MACHINE_CHECK   0x200

[...]

quoted

@@ -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

This is now a change in behaviour because previously BOOKE/4xx tested
0x200, but now it tests 0.

Yes. Previously BOOKE/4xx tested 0x200, but checked this line history, please see
the commit below:

commit c54006491dde7d1b8050c5542716b751be92ed80
Author: Anton Blanchard [off-list ref]
Date:   Fri Nov 15 15:41:19 2013 +1100

    powerpc: Print DAR and DSISR on machine check oopses
    
    Machine check exceptions set DAR and DSISR, so print them in our
    oops output.
    
    Signed-off-by: Anton Blanchard [off-list ref]
    Signed-off-by: Benjamin Herrenschmidt [off-list ref]

diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 75c2d1009985..37c4103a8cff 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c

@@ -864,7 +864,7 @@ void show_regs(struct pt_regs * regs)
        trap = TRAP(regs);
        if ((regs->trap != 0xc00) && cpu_has_feature(CPU_FTR_CFAR))
                printk("CFAR: "REG"\n", regs->orig_gpr3);
-       if (trap == 0x300 || trap == 0x600)
+       if (trap == 0x200 || trap == 0x300 || trap == 0x600)
 #if defined(CONFIG_4xx) || defined(CONFIG_BOOKE)
                printk("DEAR: "REG", ESR: "REG"\n", regs->dar, regs->dsisr);
 #else

0x200 aims to test Machine check, but for 64e, the regs->trap should be 0x000 here,
under the commit comments, I changed the code behavior. Sorry I didn’t add 
comments. 

That looks wrong for 4xx. 64e does put 0x000 there but I wonder if it 
should use 0x200 instead.

Ok. Thanks for pointing this out, let me learn about 4xx. 

Bit difficult to test this stuff, I do have
some MCE injection patches for QEMU for 64s, might be able to look at
porting them to 64e although I have no idea about booke machine checks.

Yes, would appreciate your sharing.

Anyway I don't think this patch should change generated code at all.
Either change the code first with smaller patches, or make sure you
keep the tests the same.

Agree.

Regards,
Xiongwei

Thanks,
Nick