> On Apr 10, 2021, at 8:35 AM, Nicholas Piggin <npig...@gmail.com> 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 <sxwj...@gmail.com>
>>
>> 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 <l...@intel.com>
>
> It now looks like lkp asked for this whole cleanup patch. I would
> put [kernel test robot <l...@intel.com>] in your v3->4 changelog
> item.
>
Agree. I just forgot to delete this line in the patch.
>> Signed-off-by: Xiongwei Song <sxwj...@gmail.com>
>> ---
>>
>> 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 <l...@intel.com>" 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 <an...@samba.org>
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 <an...@samba.org>
Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
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
