Messages in this thread | | | Date | Sat, 10 Apr 2021 10:35:23 +1000 | From | Nicholas Piggin <> | Subject | Re: [PATCH v4] powerpc/traps: Enhance readability for trap types |
| |
Thanks for working on this, I think it's a nice cleanup and helps non-powerpc people understand the code a bit better.
Excerpts from Xiongwei Song's message of April 10, 2021 12:28 am: > From: Xiongwei Song <sxwjean@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 <lkp@intel.com>
It now looks like lkp asked for this whole cleanup patch. I would put [kernel test robot <lkp@intel.com>] in your v3->4 changelog item.
> Signed-off-by: Xiongwei Song <sxwjean@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 <lkp@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.
That looks wrong for 4xx. 64e does put 0x000 there but I wonder if it should use 0x200 instead. 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.
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.
Thanks, Nick
| |