Hello Hung-Te Lin,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/37464
to review the following change.
Change subject: arm64: Bump exception stack size to 2KB ......................................................................
arm64: Bump exception stack size to 2KB
To avoid trampling over interesting exception artifacts on the real stack, our arm64 systems switch to a separate exception stack when entering an exception handler. We don't want that to use up too much SRAM so we just set it to 512 bytes. I mean it just prints a bunch of registers, how much stack could it need, right?
Quite a bit it turns out. The whole vtxprintf() call stack goes pretty deep, and aarch64 generally seems to be very generous with stack space. Just the varargs handling seems to require 128 bytes for some reason, and the other stuff adds up too. In the end the current implementation takes 1008 bytes, so bump the exception stack size to 2K to make sure it fits.
Change-Id: I910be4c5f6b29fae35eb53929c733a1bd4585377 Signed-off-by: Julius Werner jwerner@chromium.org --- M src/arch/arm64/armv8/exception.c 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/37464/1
diff --git a/src/arch/arm64/armv8/exception.c b/src/arch/arm64/armv8/exception.c index 579e104..ec8fc05 100644 --- a/src/arch/arm64/armv8/exception.c +++ b/src/arch/arm64/armv8/exception.c @@ -36,7 +36,7 @@ #include <console/console.h> #include <arch/lib_helpers.h>
-uint8_t exception_stack[0x200] __attribute__((aligned(16))); +uint8_t exception_stack[1*KiB] __attribute__((aligned(16)));
static const char *exception_names[NUM_EXC_VIDS] = { [EXC_VID_CUR_SP_EL0_SYNC] = "_sync_sp_el0",
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37464 )
Change subject: arm64: Bump exception stack size to 2KB ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37464/1/src/arch/arm64/armv8/except... File src/arch/arm64/armv8/exception.c:
https://review.coreboot.org/c/coreboot/+/37464/1/src/arch/arm64/armv8/except... PS1, Line 39: 1*KiB commit message says 2KB?
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37464 )
Change subject: arm64: Bump exception stack size to 2KB ......................................................................
Patch Set 1: Code-Review+1
Everything looks good except your implementation actually holds only 1K.
Hello Hung-Te Lin, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37464
to look at the new patch set (#2).
Change subject: arm64: Bump exception stack size to 2KB ......................................................................
arm64: Bump exception stack size to 2KB
To avoid trampling over interesting exception artifacts on the real stack, our arm64 systems switch to a separate exception stack when entering an exception handler. We don't want that to use up too much SRAM so we just set it to 512 bytes. I mean it just prints a bunch of registers, how much stack could it need, right?
Quite a bit it turns out. The whole vtxprintf() call stack goes pretty deep, and aarch64 generally seems to be very generous with stack space. Just the varargs handling seems to require 128 bytes for some reason, and the other stuff adds up too. In the end the current implementation takes 1008 bytes, so bump the exception stack size to 2K to make sure it fits.
Change-Id: I910be4c5f6b29fae35eb53929c733a1bd4585377 Signed-off-by: Julius Werner jwerner@chromium.org --- M payloads/libpayload/arch/arm64/exception.c M src/arch/arm64/armv8/exception.c 2 files changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/37464/2
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37464 )
Change subject: arm64: Bump exception stack size to 2KB ......................................................................
Patch Set 2:
(1 comment)
Also remembered libpayload this time.
https://review.coreboot.org/c/coreboot/+/37464/1/src/arch/arm64/armv8/except... File src/arch/arm64/armv8/exception.c:
https://review.coreboot.org/c/coreboot/+/37464/1/src/arch/arm64/armv8/except... PS1, Line 39: 1*KiB
commit message says 2KB?
Whoops, yeah, forgot to update this to what I actually decided on. Done.
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37464 )
Change subject: arm64: Bump exception stack size to 2KB ......................................................................
Patch Set 2: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/37464 )
Change subject: arm64: Bump exception stack size to 2KB ......................................................................
arm64: Bump exception stack size to 2KB
To avoid trampling over interesting exception artifacts on the real stack, our arm64 systems switch to a separate exception stack when entering an exception handler. We don't want that to use up too much SRAM so we just set it to 512 bytes. I mean it just prints a bunch of registers, how much stack could it need, right?
Quite a bit it turns out. The whole vtxprintf() call stack goes pretty deep, and aarch64 generally seems to be very generous with stack space. Just the varargs handling seems to require 128 bytes for some reason, and the other stuff adds up too. In the end the current implementation takes 1008 bytes, so bump the exception stack size to 2K to make sure it fits.
Change-Id: I910be4c5f6b29fae35eb53929c733a1bd4585377 Signed-off-by: Julius Werner jwerner@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/37464 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Hung-Te Lin hungte@chromium.org --- M payloads/libpayload/arch/arm64/exception.c M src/arch/arm64/armv8/exception.c 2 files changed, 2 insertions(+), 2 deletions(-)
Approvals: build bot (Jenkins): Verified Hung-Te Lin: Looks good to me, approved
diff --git a/payloads/libpayload/arch/arm64/exception.c b/payloads/libpayload/arch/arm64/exception.c index a5e5516..d9bd3e6 100644 --- a/payloads/libpayload/arch/arm64/exception.c +++ b/payloads/libpayload/arch/arm64/exception.c @@ -31,7 +31,7 @@ #include <libpayload.h> #include <stdint.h>
-u64 exception_stack[0x200] __attribute__((aligned(16))); +u64 exception_stack[2*KiB] __attribute__((aligned(16))); u64 *exception_stack_end = exception_stack + ARRAY_SIZE(exception_stack); extern unsigned int test_exc;
diff --git a/src/arch/arm64/armv8/exception.c b/src/arch/arm64/armv8/exception.c index 579e104..13f456d 100644 --- a/src/arch/arm64/armv8/exception.c +++ b/src/arch/arm64/armv8/exception.c @@ -36,7 +36,7 @@ #include <console/console.h> #include <arch/lib_helpers.h>
-uint8_t exception_stack[0x200] __attribute__((aligned(16))); +uint8_t exception_stack[2*KiB] __attribute__((aligned(16)));
static const char *exception_names[NUM_EXC_VIDS] = { [EXC_VID_CUR_SP_EL0_SYNC] = "_sync_sp_el0",
-
Arthur Heymans (Code Review) -
Hung-Te Lin (Code Review)
-
Julius Werner (Code Review)
-
Patrick Georgi (Code Review)