Re: [PATCH] x86: use enum instead of literals for trap values

[Ingo Molnar <mingo@elte.hu>]

* Kees Cook <keescook@chromium.org> wrote:

> The traps are referred to by their numbers and it can be difficult to
> understand them while reading the code without context. This patch adds
> enumeration of the trap numbers and replaces the numbers with the correct
> enum for x86.
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>
> 
> ---
> I've updated Aditya Kali's earlier patch:
> https://lkml.org/lkml/2011/4/22/328
> ---
>  arch/x86/include/asm/traps.h |   25 +++++++++
>  arch/x86/kernel/irqinit.c    |    2 +-
>  arch/x86/kernel/traps.c      |  117 ++++++++++++++++++++++--------------------
>  3 files changed, 88 insertions(+), 56 deletions(-)
> 
> diff --git a/arch/x86/include/asm/traps.h b/arch/x86/include/asm/traps.h
> index 0012d09..768afb2 100644
> --- a/arch/x86/include/asm/traps.h
> +++ b/arch/x86/include/asm/traps.h
> @@ -89,4 +89,29 @@ asmlinkage void smp_thermal_interrupt(void);
>  asmlinkage void mce_threshold_interrupt(void);
>  #endif
>  
> +/* Interrupts/Exceptions */
> +enum {
> +	INTR_DIV_BY_ZERO = 0,	/*  0 */
> +	INTR_DEBUG,		/*  1 */
> +	INTR_NMI,		/*  2 */
> +	INTR_BREAKPOINT,	/*  3 */
> +	INTR_OVERFLOW,		/*  4 */
> +	INTR_BOUNDS_CHECK,	/*  5 */
> +	INTR_INVALID_OP,	/*  6 */
> +	INTR_NO_DEV,		/*  7 */
> +	INTR_DBL_FAULT,		/*  8 */
> +	INTR_SEG_OVERRUN,	/*  9 */
> +	INTR_INVALID_TSS,	/* 10 */
> +	INTR_NO_SEG,		/* 11 */
> +	INTR_STACK_FAULT,	/* 12 */
> +	INTR_GPF,		/* 13 */
> +	INTR_PAGE_FAULT,	/* 14 */
> +	INTR_SPURIOUS,		/* 15 */
> +	INTR_COPROCESSOR,	/* 16 */
> +	INTR_ALIGNMENT,		/* 17 */
> +	INTR_MCE,		/* 18 */
> +	INTR_SIMD_COPROCESSOR,	/* 19 */
> +	INTR_IRET = 32,		/* 32 */
> +};

> @@ -453,14 +458,15 @@ dotraplinkage void __kprobes do_debug(struct pt_regs *regs, long error_code)
>  /*
>   * Note that we play around with the 'TS' bit in an attempt to get
>   * the correct behaviour even in the presence of the asynchronous
> - * IRQ13 behaviour
> + * INTR_GPF behaviour
>   */

> @@ -529,8 +535,9 @@ void math_error(struct pt_regs *regs, int error_code, int trapnr)
>  		info.si_code = FPE_FLTRES;
>  	} else {
>  		/*
> -		 * If we're using IRQ 13, or supposedly even some trap 16
> -		 * implementations, it's possible we get a spurious trap...
> +		 * If we're using INTR_GPF, or supposedly even some trap
> +		 * INTR_COPROCESSOR implementations, it's possible we get a
> +		 * spurious trap...

There's confusion in this patch between legacy IRQ #13 [vector 
0x20 + 13 ] and #GPF general protection fault [vector 13] - they 
are not the same.

Furthermore, the INTR_ naming is not ideal either for (most of) 
these entries: for example we don't think of a page fault as an 
asynchronous interrupt entity - we think of it as a more or less 
synchronous fault/exception.

Thus a X86_*_FAULT_VEC naming pattern might be better:

	X86_PAGE_FAULT_VEC
	X86_DOUBLE_FAULT_VEC

(With X86_*_EXCEPTION_VEC applied where appropriate.)

I don't disagree with the general principle of the cleanup 
otherwise, the numeric literals are often ambiguous and 
confusing - as the trap 13 - irq 13 mixup above shows.

Thanks,

	Ingo