Change in coreboot[master]: arch/x86: Add support for catching null dereferences through debug regs
Attention is currently required from: Raul Rangel, Nico Huber, Tim Wawrzynczak, Paul Menzel, Angel Pons, Arthur Heymans, Kyösti Mälkki, Karthik Ramasubramanian. Robert Zieba has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63657 )
Change subject: arch/x86: Add support for catching null dereferences through debug regs ......................................................................
Patch Set 11:
(19 comments)
File src/arch/x86/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/63657/comment/be953b12_5dba3e9f PS7, Line 237: ramstage
Why only ramstage?
Done
File src/arch/x86/breakpoint.c:
https://review.coreboot.org/c/coreboot/+/63657/comment/38e93e77_24ca6688 PS7, Line 41: = {0, 0, 0, 0}
If you still want to keep the initializer, you can simply use `{ 0 }` regardless of how many element […]
Done
https://review.coreboot.org/c/coreboot/+/63657/comment/08541e9e_6576ed51 PS7, Line 41: DEBUG_REGISTER_COUNT
Instead of parallel arrays how about a struct? […]
Done
https://review.coreboot.org/c/coreboot/+/63657/comment/2b48509f_0841bc1c PS7, Line 42: = {NULL, NULL, NULL, NULL};
Same
Done
https://review.coreboot.org/c/coreboot/+/63657/comment/f83b07dc_5fb95ca5 PS7, Line 46: uintptr_t ret = ~0x0;
Instead of returning a magic value, can you define an `*out` parameter and return true or false on s […]
This function isn't be used anywhere so I ended up just removing it.
https://review.coreboot.org/c/coreboot/+/63657/comment/a192bac4_acb65e6f PS7, Line 72: void
Same
Done
https://review.coreboot.org/c/coreboot/+/63657/comment/f1b21701_c7f7be7d PS7, Line 193: enum breakpoint_type type = write_only ? BREAKPOINT_TYPE_DATA_WRITE : BREAKPOINT_TYPE_DATA_RW;
clang-format
Done
https://review.coreboot.org/c/coreboot/+/63657/comment/93a64453_67ec4ad2 PS7, Line 258: DEBUG_STATUS_GET_BP_HIT
Can multiple BPs be hit at the same time?
The documentation isn't clear on this. I want to say that multiples can't be hit at once but I've changed this to handle multiple ones just to be safe.
https://review.coreboot.org/c/coreboot/+/63657/comment/7274333b_eda00195 PS7, Line 257: for (int i = 0; i < DEBUG_REGISTER_COUNT; i++) { : if (DEBUG_STATUS_GET_BP_HIT(i, status)) { : bp = i; : break; : } : } : : if (bp == -1) : return 0;
Can we move this to a helper that returns true/false if hit.
Done
https://review.coreboot.org/c/coreboot/+/63657/comment/dd22a6f7_a5c570da PS7, Line 279: fails
Why is this going to fail? We can store the type in the struct above can't we?
Done
File src/arch/x86/include/arch/breakpoint.h:
https://review.coreboot.org/c/coreboot/+/63657/comment/1b887634_34c20eff PS7, Line 22: BREAKPOINT_TYPE_DATA_RW
What's the difference beteween this and BREAKPOINT_TYPE_DATA_WRITE?
A write breakpoint only gets hit on write, RW gets hit on read and write.
https://review.coreboot.org/c/coreboot/+/63657/comment/ab3c360d_a4b157a6 PS7, Line 26: out_handle
What's an out_handle?
Done
https://review.coreboot.org/c/coreboot/+/63657/comment/9220ae21_ac1f6461 PS7, Line 28: out_handle
Same?
Done
https://review.coreboot.org/c/coreboot/+/63657/comment/52e3af43_31acfdba PS7, Line 31: handle
Oh this is a handle to control the breakpoint? It would make it clearer if you defined a `struct bre […]
Done
https://review.coreboot.org/c/coreboot/+/63657/comment/4564674b_7af1c309 PS7, Line 44: ) { return 0; /* Not implemented */ }
clang-format
Done
File src/arch/x86/null_breakpoint.c:
https://review.coreboot.org/c/coreboot/+/63657/comment/8836e006_cdb2d7f9 PS7, Line 10: handle
What does handle refer to in this case?
Done
https://review.coreboot.org/c/coreboot/+/63657/comment/1ff0b276_8e2652a4 PS7, Line 14: else
Should we have two exception handlers instead?
Done
https://review.coreboot.org/c/coreboot/+/63657/comment/d4e316e6_4df84d99 PS7, Line 16: printk
Can you instead call x86_exception. it prints out more information […]
Ack
https://review.coreboot.org/c/coreboot/+/63657/comment/3d5a54c0_57476f0b PS7, Line 40: null_exec_bp
Do we need a NULL instruction handler? Isn't that an invalid opcode? The existing exception handler […]
This breakpoint will get hit when there's an instruction fetch from address 0. I've updated the message to make it clearer.
-
Robert Zieba (Code Review)