Hello Hung-Te Lin,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/37465
to review the following change.
Change subject: arm64: Print a char to UART early in exception handler ......................................................................
arm64: Print a char to UART early in exception handler
Over time our printk() seems to acquire more and more features... which is nice, but it also makes it a little less robust when something goes wrong. If the wrong global is trampled by some buffer overflow, it suddenly doesn't print anymore. It would be nice to have at least some way to tell that we triggered a real exception in that case.
With this patch, arm64 exceptions will print a '!' straight to the UART before trying any of the more fancy printk() stuff. It's not much but it should tell the difference between an exception and a hang and hopefully help someone dig in the right direction sooner. This violates loglevels (which is part of the point), but presumably when you have a fatal exception you shouldn't care about that anymore.
Change-Id: I3b08ab86beaee55263786011caa5588d93bbc720 Signed-off-by: Julius Werner jwerner@chromium.org --- M src/arch/arm64/armv8/exception.c 1 file changed, 8 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/65/37465/1
diff --git a/src/arch/arm64/armv8/exception.c b/src/arch/arm64/armv8/exception.c index ec8fc05..c11ce57 100644 --- a/src/arch/arm64/armv8/exception.c +++ b/src/arch/arm64/armv8/exception.c @@ -34,6 +34,7 @@ #include <arch/exception.h> #include <arch/transition.h> #include <console/console.h> +#include <console/uart.h> #include <arch/lib_helpers.h>
uint8_t exception_stack[1*KiB] __attribute__((aligned(16))); @@ -131,8 +132,13 @@
static void print_exception_info(struct exc_state *state, uint64_t idx) { - if (idx < NUM_EXC_VIDS) - printk(BIOS_DEBUG, "exception %s\n", exception_names[idx]); + /* Poor man's sign of life in case printk() is shot. */ + __uart_tx_byte('\r'); + __uart_tx_byte('\n'); + __uart_tx_byte('!'); + + printk(BIOS_DEBUG, "\nexception %s\n", + idx < NUM_EXC_VIDS ? exception_names[idx] : "_unknown");
print_regs(state); /* Few words below SP in case we need state from a returned function. */
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37465 )
Change subject: arm64: Print a char to UART early in exception handler ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37465/1/src/arch/arm64/armv8/except... File src/arch/arm64/armv8/exception.c:
https://review.coreboot.org/c/coreboot/+/37465/1/src/arch/arm64/armv8/except... PS1, Line 136: __uart_tx_byte What if a board does not initialize UART, or even does not include UART drivers? Will we stuck there?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37465 )
Change subject: arm64: Print a char to UART early in exception handler ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37465/1/src/arch/arm64/armv8/except... File src/arch/arm64/armv8/exception.c:
https://review.coreboot.org/c/coreboot/+/37465/1/src/arch/arm64/armv8/except... PS1, Line 136: __uart_tx_byte
What if a board does not initialize UART, or even does not include UART drivers? […]
__uart_tx_byte() is defined to a no-op if serial drivers aren't configured in.
If the UART driver is configured in but hangs, then yeah, this will hang. But otherwise it would hang at the printk() below anyway.
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37465 )
Change subject: arm64: Print a char to UART early in exception handler ......................................................................
Patch Set 2: Code-Review+1
I think this is basically fine but you probably want to find more people to agree on this behavior change?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37465 )
Change subject: arm64: Print a char to UART early in exception handler ......................................................................
Patch Set 2:
I think this is basically fine but you probably want to find more people to agree on this behavior change?
I think the two of us are the main people developing coreboot on arm64 right now. ;)
In practice this is just like slightly changing a printk(), I don't see why it would be a huge deal.
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37465 )
Change subject: arm64: Print a char to UART early in exception handler ......................................................................
Patch Set 2: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/37465 )
Change subject: arm64: Print a char to UART early in exception handler ......................................................................
arm64: Print a char to UART early in exception handler
Over time our printk() seems to acquire more and more features... which is nice, but it also makes it a little less robust when something goes wrong. If the wrong global is trampled by some buffer overflow, it suddenly doesn't print anymore. It would be nice to have at least some way to tell that we triggered a real exception in that case.
With this patch, arm64 exceptions will print a '!' straight to the UART before trying any of the more fancy printk() stuff. It's not much but it should tell the difference between an exception and a hang and hopefully help someone dig in the right direction sooner. This violates loglevels (which is part of the point), but presumably when you have a fatal exception you shouldn't care about that anymore.
Change-Id: I3b08ab86beaee55263786011caa5588d93bbc720 Signed-off-by: Julius Werner jwerner@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/37465 Reviewed-by: Hung-Te Lin hungte@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/arch/arm64/armv8/exception.c 1 file changed, 8 insertions(+), 2 deletions(-)
Approvals: build bot (Jenkins): Verified Hung-Te Lin: Looks good to me, approved
diff --git a/src/arch/arm64/armv8/exception.c b/src/arch/arm64/armv8/exception.c index 13f456d..4d566aa 100644 --- a/src/arch/arm64/armv8/exception.c +++ b/src/arch/arm64/armv8/exception.c @@ -34,6 +34,7 @@ #include <arch/exception.h> #include <arch/transition.h> #include <console/console.h> +#include <console/uart.h> #include <arch/lib_helpers.h>
uint8_t exception_stack[2*KiB] __attribute__((aligned(16))); @@ -131,8 +132,13 @@
static void print_exception_info(struct exc_state *state, uint64_t idx) { - if (idx < NUM_EXC_VIDS) - printk(BIOS_DEBUG, "exception %s\n", exception_names[idx]); + /* Poor man's sign of life in case printk() is shot. */ + __uart_tx_byte('\r'); + __uart_tx_byte('\n'); + __uart_tx_byte('!'); + + printk(BIOS_DEBUG, "\nexception %s\n", + idx < NUM_EXC_VIDS ? exception_names[idx] : "_unknown");
print_regs(state); /* Few words below SP in case we need state from a returned function. */
-
Hung-Te Lin (Code Review) -
Julius Werner (Code Review)
-
Patrick Georgi (Code Review)