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

[Xiongwei Song]

> On Apr 10, 2021, at 8:04 AM, Michael Ellerman <mpe@xxxxxxxxxxxxxx> wrote:
> 
> Christophe Leroy <christophe.leroy@xxxxxxxxxx> writes:
>> Le 08/04/2021 à 16:07, Xiongwei Song a écrit :
>>> From: Xiongwei Song <sxwjean@xxxxxxxxx>
>>> 
>>> Create a new header named traps.h, define macros to list ppc interrupt
>>> types in traps.h, replace the reference of the trap hex values with these
>>> macros.
> ...
>>> diff --git a/arch/powerpc/include/asm/interrupt.h b/arch/powerpc/include/asm/interrupt.h
>>> index 7c633896d758..5ce9898bc9a6 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) != INTERRUPT_PROGRAM)
>>>  			CT_WARN_ON(ct_state() != CONTEXT_KERNEL);
>>>  	}
>>>  #endif
>>> @@ -156,7 +157,8 @@ 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) != INTERRUPT_DECREMENTER &&
>>> +	    TRAP(regs) != INTERRUPT_PERFMON) {
>> 
>> I think too long names hinder readability, see later for suggestions.
> 
> I asked for the longer names :)
> 
> I think they make it easier for people who are less familiar with the
> architecture than us to make sense of the names.
> 
> And there's only a couple of cases where it requires splitting a line,
> and they could be converted to use switch if we think it's a problem.
> 
>>> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
>>> index 0c0b1c2cfb49..641b3feef7ee 100644
>>> --- a/arch/powerpc/mm/fault.c
>>> +++ b/arch/powerpc/mm/fault.c
>>> @@ -588,20 +589,24 @@ 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 INTERRUPT_DATA_STORAGE:
>>> +#ifdef CONFIG_PPC_BOOK3S
>>> +	case INTERRUPT_DATA_SEGMENT:
>>> +	case INTERRUPT_H_DATA_STORAGE:
>>> +#endif
>> 
>> It would be better to avoid #ifdefs when none where necessary before.
> 
> Yes I agree.
> 
> I think these can all be avoided by defining most of the values
> regardless of what platform we're building for. Only the values that
> overlap need to be kept behind an ifdef.

Ok. 

Regards,
Xiongwei

> 
> cheers