x86: use enum instead of literals for trap values

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 <[email protected]>

---
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(-)

arch/x86/include/asm/traps.h +25 -1

index 0012d09..768afb2 100644
@@ -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
  */
 

Quoted 58 lines · Kees Cook

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 <[email protected]>

---
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(-)

arch/x86/include/asm/traps.h +25 -1

index 0012d09..768afb2 100644
@@ -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
   */
 

arch/x86/include/asm/traps.h void math_error(struct pt_regs *regs, int error_code, int trapnr) · line 540

Quoted 8 lines

arch/x86/include/asm/traps.h +3 -2

@@ -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.

Quoted 8 lines · Ingo Molnar

Quoted 5 lines · Kees Cook

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.

Quoted 9 lines · Kees Cook

Quoted 8 lines · Ingo Molnar

Quoted 5 lines · Kees Cook

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.

I have to admit personally to prefer something like X86_XCP_XX where XX is the two-letter code that the Intel documentation uses for that trap, i.e. #GP, #BR, #MC and so on.

Quoted 3 lines

I have to admit personally to prefer something like X86_XCP_XX where XX is the two-letter code that the Intel documentation uses for that trap, i.e. #GP, #BR, #MC and so on.

We need a single person to decide on this bike shed color. :) If the list of enum names can be agreed on, I'll be happy to do the search/replace for it.

Quoted 7 lines · Kees Cook

Quoted 3 lines

I have to admit personally to prefer something like X86_XCP_XX where XX is the two-letter code that the Intel documentation uses for that trap, i.e. #GP, #BR, #MC and so on.

We need a single person to decide on this bike shed color. :) If the list of enum names can be agreed on, I'll be happy to do the search/replace for it.

Well,

here are my 2¢: I agree with hpa because

a) it maps the CPU vendor documentation
b) it is nicely short

I agree too, except where does "XCP" come from? Is it short for "exception"?

Quoted 2 lines · Steven Rostedt

I agree too, except where does "XCP" come from? Is it short for "exception"?

Yes, short for "exception". I hate to admit it, but it comes from the constants we used in the Transmeta microcode ;)

Make it X86_TRAP_XX, it's both short and correct?

Quoted 4 lines · Steven Rostedt

Quoted 2 lines

I agree too, except where does "XCP" come from? Is it short for "exception"?

Quoted 2 lines

Yes, short for "exception".  I hate to admit it, but it comes from the constants we used in the Transmeta microcode ;)

Hm, were I to see 'X86_XCP_MC' for the first time I'd have no idea what it means. Really, lets not use random new abbreviations, lets use well-known patterns.

Make it X86_TRAP_XX, it's both short and correct?

Quoted 7 lines · Kees Cook

Quoted 3 lines

I have to admit personally to prefer something like X86_XCP_XX where XX is the two-letter code that the Intel documentation uses for that trap, i.e. #GP, #BR, #MC and so on.

We need a single person to decide on this bike shed color. :) If the list of enum names can be agreed on, I'll be happy to do the search/replace for it.

Well,

here are my 2¢: I agree with hpa because

a) it maps the CPU vendor documentation
b) it is nicely short

Quoted 9 lines · Borislav Petkov

Quoted 7 lines · Kees Cook

Quoted 3 lines

I have to admit personally to prefer something like X86_XCP_XX where XX is the two-letter code that the Intel documentation uses for that trap, i.e. #GP, #BR, #MC and so on.

We need a single person to decide on this bike shed color. :) If the list of enum names can be agreed on, I'll be happy to do the search/replace for it.

Quoted 6 lines

Well,

here are my 2¢: I agree with hpa because

a) it maps the CPU vendor documentation
b) it is nicely short

How about:

X86_XCP_DE = 0,         /* 0, Divide-by-zero */
X86_XCP_DB,             /* 1, Debug */
X86_XCP_NMI,            /* 2, Non-maskable Interrupt */
X86_XCP_BP,             /* 3, Breakpoint */
X86_XCP_OF,             /* 4, Overflow */
X86_XCP_BR,             /* 5, Bound Range Exceeded */
X86_XCP_UD,             /* 6, Invalid Opcode */
X86_XCP_NM,             /* 7, Device Not Available */
X86_XCP_DF,             /* 8, Double Fault */
X86_XCP_OLD_MF,         /* 9, Coprocessor Segment Overrun */
X86_XCP_TS,             /* 10, Invalid TSS */
X86_XCP_NP,             /* 11, Segment Not Present */
X86_XCP_SS,             /* 12, Stack-Segment Fault */
X86_XCP_GP,             /* 13, General Protection Fault  */
X86_XCP_PF,             /* 14, Page Fault */
X86_XCP_RES,            /* 15, Reserved */

X86_XCP_MF,             /* 16, x87 Floating-Point Exception */
X86_XCP_AC,             /* 17, Alignment Check */
X86_XCP_MC,             /* 18, Machine Check */
X86_XCP_XM,             /* 19, SIMD Floating-Point Exception */

X86_XCP_IRET = 32, /* 32, IRET Exception */

There is a name collision for "MF", there's no mnemonic for NMI,

IRET, or the reserved "spurious" interrupt.

Can use "VEC" instead "XCP", as Steven suggests.

Quoted 1 line

Can use "VEC" instead "XCP", as Steven suggests.

Yeah, because those actually are fixed interrupt vectors, as they're called in the AMD docs. Makes sense.

Quoted 19 lines · Kees Cook

Quoted 9 lines · Borislav Petkov

Quoted 7 lines · Kees Cook

Quoted 3 lines

I have to admit personally to prefer something like X86_XCP_XX where XX is the two-letter code that the Intel documentation uses for that trap, i.e. #GP, #BR, #MC and so on.

We need a single person to decide on this bike shed color. :) If the list of enum names can be agreed on, I'll be happy to do the search/replace for it.

Quoted 6 lines

Well,

here are my 2¢: I agree with hpa because

a) it maps the CPU vendor documentation
b) it is nicely short

How about:

Quoted 16 lines

X86_XCP_DE = 0,         /* 0, Divide-by-zero */ X86_XCP_DB,             /* 1, Debug */ X86_XCP_NMI,            /* 2, Non-maskable Interrupt */ X86_XCP_BP,             /* 3, Breakpoint */ X86_XCP_OF,             /* 4, Overflow */ X86_XCP_BR,             /* 5, Bound Range Exceeded */ X86_XCP_UD,             /* 6, Invalid Opcode */ X86_XCP_NM,             /* 7, Device Not Available */ X86_XCP_DF,             /* 8, Double Fault */ X86_XCP_OLD_MF,         /* 9, Coprocessor Segment Overrun */ X86_XCP_TS,             /* 10, Invalid TSS */ X86_XCP_NP,             /* 11, Segment Not Present */ X86_XCP_SS,             /* 12, Stack-Segment Fault */ X86_XCP_GP,             /* 13, General Protection Fault  */ X86_XCP_PF,             /* 14, Page Fault */ X86_XCP_RES,            /* 15, Reserved */

So is this reserved or are we using it for Spurious IRQs? If we use it, then 'RES' is a bad name. Maybe we define our own like

X86_VEC_SP

and then do

X86_VEC_IR for IRET

in the manner we assumed for the rest?

Quoted 4 lines

X86_XCP_MF,             /* 16, x87 Floating-Point Exception */ X86_XCP_AC,             /* 17, Alignment Check */ X86_XCP_MC,             /* 18, Machine Check */ X86_XCP_XM,             /* 19, SIMD Floating-Point Exception */

Shouln't this be #XF actually? At least it is so in the AMD docs.

Quoted 3 lines

X86_XCP_IRET = 32,      /* 32, IRET Exception */

There is a name collision for "MF", there's no mnemonic for NMI,

Well, in the AMD docs we actually do have the '#NMI' mnemonic in use.

Quoted 3 lines

IRET, or the reserved "spurious" interrupt.

Can use "VEC" instead "XCP", as Steven suggests.

Yeah, because those actually are fixed interrupt vectors, as they're called in the AMD docs. Makes sense.

So is this reserved or are we using it for Spurious IRQs? If we use it, then 'RES' is a bad name. Maybe we define our own like

X86_VEC_SP

and then do

X86_VEC_IR for IRET

in the manner we assumed for the rest?

Quoted 8 lines · Borislav Petkov

So is this reserved or are we using it for Spurious IRQs? If we use it, then 'RES' is a bad name. Maybe we define our own like

X86_VEC_SP

and then do

Quoted 4 lines

X86_VEC_IR for IRET

in the manner we assumed for the rest?

Actually I prefer if we didn't use two-letter versions for the ones that aren't defined in the documentation; especially IRET which is a pure Linux invention.

For the Linux made up ones, a more descriptive name is better. Then there wont be confusion when people look at "IR" and can't find it online or in the document, and scratch their head thinking they are not smart enough to do kernel development and we lose another kernel developer ;-)

Quoted 5 lines · Steven Rostedt

For the Linux made up ones, a more descriptive name is better. Then there wont be confusion when people look at "IR" and can't find it online or in the document, and scratch their head thinking they are not smart enough to do kernel development and we lose another kernel developer ;-)

Like that ever happens... Kernel developers have thick heads, in most cases thicker than processor manuals.

Quoted 7 lines · Borislav Petkov

Quoted 5 lines · Steven Rostedt

For the Linux made up ones, a more descriptive name is better. Then there wont be confusion when people look at "IR" and can't find it online or in the document, and scratch their head thinking they are not smart enough to do kernel development and we lose another kernel developer ;-)

Quoted 2 lines

Like that ever happens... Kernel developers have thick heads, in most cases thicker than processor manuals.

I'm talking about new kernel developers, not the current ones ;-)

Quoted 15 lines

Quoted 15 lines

+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 */

Quoted 8 lines

Quoted 8 lines

+     INTR_PAGE_FAULT,        /* 14 */
+     INTR_SPURIOUS,          /* 15 */
+     INTR_COPROCESSOR,       /* 16 */
+     INTR_ALIGNMENT,         /* 17 */
+     INTR_MCE,               /* 18 */
+     INTR_SIMD_COPROCESSOR,  /* 19 */
+     INTR_IRET = 32,         /* 32 */
+};