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

[Xiongwei Song]

> On Apr 10, 2021, at 8:35 AM, Nicholas Piggin <npiggin@xxxxxxxxx> 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:
>> From: Xiongwei Song <sxwjean@xxxxxxxxx>
>> 
>> 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 <lkp@xxxxxxxxx>
> 
> It now looks like lkp asked for this whole cleanup patch. I would
> put [kernel test robot <lkp@xxxxxxxxx>] in your v3->4 changelog
> item.
> 

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

>> Signed-off-by: Xiongwei Song <sxwjean@xxxxxxxxx>
>> ---
>> 
>> 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 <lkp@xxxxxxxxx>" here,
>> because it's improper for this patch.
> 
> [...]
> 
>> 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.
> 
>> +#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
> 
> [...]
> 
>> @@ -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 <anton@xxxxxxxxx>
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 <anton@xxxxxxxxx>
    Signed-off-by: Benjamin Herrenschmidt <benh@xxxxxxxxxxxxxxxxxxx>

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