Change in coreboot[master]: arch/x86: Add support for catching null deferences through debug regs

[Karthik Ramasubramanian (Code Review)]

Attention is currently required from: Robert Zieba, Nico Huber, Raul Rangel, Angel Pons, Arthur Heymans, Tim Wawrzynczak, Kyösti Mälkki.
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63657 )

Change subject: arch/x86: Add support for catching null deferences through debug regs
......................................................................

Patch Set 5:

(10 comments)

File src/arch/x86/Kconfig:

https://review.coreboot.org/c/coreboot/+/63657/comment/2fe9ee80_f528e604 
PS5, Line 327: 	
Nit: Replace 2nd tab with 2 spaces so that it is consistent with other config items.

File src/arch/x86/breakpoint.c:

https://review.coreboot.org/c/coreboot/+/63657/comment/389c3a07_9815cedf 
PS5, Line 35: #define DEBUG_CTRL_INSTR 0x0
            : #define DEBUG_CTRL_WRITE 0x1
            : #define DEBUG_CTRL_IO_RW 0x2
            : #define DEBUG_CTRL_RW 0x3
enum seems like a good candidate

https://review.coreboot.org/c/coreboot/+/63657/comment/255cc93e_7b56e99a 
PS5, Line 50:  {
Nit: For functions, recommend to keep opening braces in a separate line.

https://review.coreboot.org/c/coreboot/+/63657/comment/6382279b_ab8a2361 
PS5, Line 51:     uintptr_t ret = ~0x0;
Nit: Always leave an empty line between a declaration block and function body. Here and in other places in this CL.

https://review.coreboot.org/c/coreboot/+/63657/comment/73041323_794f9f8e 
PS5, Line 173:         case 8:
Nit:
```
case 8:
        if (!ENV_X86_64)
                return BREAKPOINT_RES_INVALID_LENGTH;
        len_value = DEBUG_CTRL_LEN_8;
        break;
```

File src/arch/x86/exception.c:

https://review.coreboot.org/c/coreboot/+/63657/comment/266aa05d_01f3732b 
PS5, Line 374: #include <arch/registers.h>
If this source file has been using anything from registers.h, then it is always better to include this file. Probably better to move it to the top along with other includes.

https://review.coreboot.org/c/coreboot/+/63657/comment/128a36ca_4ba7a31a 
PS5, Line 491: #if ENV_RAMSTAGE && CONFIG(DEBUG_HW_BREAKPOINTS)
If you have a static inline stubbed out implementation of breakpoint_dispatch_handler in breakpoint.h then you dont need this.

https://review.coreboot.org/c/coreboot/+/63657/comment/9a950766_a7fc02fc 
PS5, Line 492: 1
What is this magic number 1?

File src/arch/x86/include/arch/breakpoint.h:

https://review.coreboot.org/c/coreboot/+/63657/comment/ef10b369_dd48b865 
PS5, Line 5: CONFIG(DEBUG_HW_BREAKPOINTS)
I dont think this is required since it is included only in breakpoint.c and null_breakpoint.c which are built only if the concerned configs are enabled.

File src/arch/x86/include/arch/null_breakpoint.h:

https://review.coreboot.org/c/coreboot/+/63657/comment/5744004d_bb015101 
PS5, Line 5: #if CONFIG(DEBUG_NULL_DEREF_BREAKPOINTS)
I dont think this is required. This is included only in null_breakpoint.c which is compiled only when CONFIG(DEBUG_NULL_DEREF_BREAKPOINTS) is enabled.