Asami Doi has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34774 )
Change subject: lib: Register exception handlers for ARMv8 ......................................................................
lib: Register exception handlers for ARMv8
Register exception handlers to avoid a Synchronous External Abort that is raised when you try to access a non-memory address on ARMv8. An exception handler can jump over the faulting instruction.
Signed-off-by: Asami Doi d0iasm.pub@gmail.com Change-Id: I09a306ca307ba4027d9758c3debc2e7c844c66b8 --- M src/lib/ramdetect.c 1 file changed, 54 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/34774/1
diff --git a/src/lib/ramdetect.c b/src/lib/ramdetect.c index 5416a58..7f53b57 100644 --- a/src/lib/ramdetect.c +++ b/src/lib/ramdetect.c @@ -15,24 +15,62 @@ #include <symbols.h> #include <device/mmio.h> #include <ramdetect.h> +#include <arch/exception.h> #include <console/console.h>
#define OVERLAP(a, b, s, e) ((b) > (s) && (a) < (e))
+static enum { + ABORT_CHECKER_NOT_REGISTERED, + ABORT_CHECKER_DEACTIVATED, + ABORT_CHECKER_NOT_TRIGGERED, + ABORT_CHECKER_TRIGGERED, +} abort_state = ABORT_CHECKER_NOT_REGISTERED; + +static int abort_checker(struct exc_state *state, uint64_t vector_id) +{ + if (abort_state == ABORT_CHECKER_DEACTIVATED) + return EXC_RET_IGNORED; + + if (raw_read_esr_el3() >> 26 != 0x25) + return EXC_RET_IGNORED; /* Not a data abort. */ + + abort_state = ABORT_CHECKER_TRIGGERED; + state->elx.elr += sizeof(uint32_t); /* Jump over faulting instruction. */ + raw_write_elr_el3(state->elx.elr); + return EXC_RET_HANDLED; +} + +static void register_checker(void) +{ + struct exception_handler sync_elx; + struct exception_handler sync_el0; + + sync_elx.handler = &abort_checker; + sync_el0.handler = &abort_checker; + + exception_handler_register(EXC_VID_CUR_SP_ELX_SYNC, &sync_elx); + exception_handler_register(EXC_VID_CUR_SP_EL0_SYNC, &sync_el0); + + printk(BIOS_DEBUG, "ARM64: Abort checker handlers installed.\n"); +} + static int probe_mb(const uintptr_t dram_start, const uintptr_t size) { uintptr_t addr = dram_start + (size * MiB) - sizeof(uint32_t); - static const uint32_t patterns[] = { - 0x55aa55aa, - 0x12345678 - }; - void *ptr = (void *) addr; + static const uint32_t patterns[] = {0x55aa55aa, 0x12345678}; + void *ptr = (void *)addr; size_t i;
/* Don't accidentally clober oneself. */ - if (OVERLAP(addr, addr + sizeof(uint32_t), (uintptr_t)_program, (uintptr_t) _eprogram)) + if (OVERLAP(addr, addr + sizeof(uint32_t), (uintptr_t)_program, (uintptr_t)_eprogram)) return 1;
+ if (ENV_ARM64) { + read32((void *)addr); + return abort_state == ABORT_CHECKER_NOT_TRIGGERED; + } + uint32_t old = read32(ptr); for (i = 0; i < ARRAY_SIZE(patterns); i++) { write32(ptr, patterns[i]); @@ -50,6 +88,12 @@ /* Probe an area if it's read/writable. */ size_t probe_ramsize(const uintptr_t dram_start, const size_t probe_size) { + if (ENV_ARM64) { + if (abort_state == ABORT_CHECKER_NOT_REGISTERED) + register_checker(); + abort_state = ABORT_CHECKER_NOT_TRIGGERED; + } + ssize_t i; size_t msb = 0; size_t discovered = 0; @@ -74,5 +118,9 @@
saved_result = discovered; printk(BIOS_DEBUG, "RAMDETECT: Found %zu MiB RAM\n", discovered); + + if (ENV_ARM64) + abort_state = ABORT_CHECKER_DEACTIVATED; + return discovered; }
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34774 )
Change subject: lib: Register exception handlers for ARMv8 ......................................................................
Patch Set 1:
(8 comments)
(How did you manage to add comments without clicking the "Start Review" thing, Raul? I can't find that. ^^)
https://review.coreboot.org/c/coreboot/+/34774/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34774/1//COMMIT_MSG@7 PS1, Line 7: lib: Register exception handlers for ARMv8 nit: "ramdetect: " or "lib: ramdetect: " would make it clearer what this is for
https://review.coreboot.org/c/coreboot/+/34774/1//COMMIT_MSG@9 PS1, Line 9: Register exception handlers to avoid a Synchronous External Abort nit: maybe clarify that this is specifically for the RAM detection feature in emulators, and nothing else (because this sounds like we'd just be doing this for any abort on all platforms).
https://review.coreboot.org/c/coreboot/+/34774/1/src/lib/ramdetect.c File src/lib/ramdetect.c:
https://review.coreboot.org/c/coreboot/+/34774/1/src/lib/ramdetect.c@53 PS1, Line 53: exception_handler_register(EXC_VID_CUR_SP_EL0_SYNC, &sync_el0); Do you really need both? I'm pretty sure you should only need one of these (I think EL0).
https://review.coreboot.org/c/coreboot/+/34774/1/src/lib/ramdetect.c@55 PS1, Line 55: printk(BIOS_DEBUG, "ARM64: Abort checker handlers installed.\n"); nit: This probably adds more confusion than clarity since the whole probe_ramsize() thing doesn't print anything.
https://review.coreboot.org/c/coreboot/+/34774/1/src/lib/ramdetect.c@58 PS1, Line 58: probe_mb
Can you make this a __weak function. […]
If we do this (personally I'm ambivalent... this at least reuses the address calculation from this function, after all), the parts from probe_ramsize() should also be factored out into that file. But I'm not sure how to best do that... having an arch_probe_ramsize_entry() and arch_probe_ramsize_exit() seems really heavy-handed for what you're trying to do here... or maybe we should change the current probe_ramsize() to probe_ramsize_helper() that also takes a function pointer for probe_mb(), and then probe_ramsize() should be the weak function where the default implementation just calls probe_ramsize_helper() with the default probe_mb(), and the arch-specific version can do something else.
...but, honestly, I think that gets so complicated that just doing it like this may be better (at least as long as it's only one architecture that needs the special treatment).
https://review.coreboot.org/c/coreboot/+/34774/1/src/lib/ramdetect.c@69 PS1, Line 69: if (ENV_ARM64) { Since you're only reading it shouldn't matter if you overlap code, so can put this above the overlap check
https://review.coreboot.org/c/coreboot/+/34774/1/src/lib/ramdetect.c@70 PS1, Line 70: read32((void *)addr); This could just use 'ptr'.
https://review.coreboot.org/c/coreboot/+/34774/1/src/lib/ramdetect.c@71 PS1, Line 71: return abort_state == ABORT_CHECKER_NOT_TRIGGERED; Forgot one thing: this needs to reset the abort_state to NOT_TRIGGERED if it has been triggered. So should rather be
if (abort_state == ABORT_CHECKER_NOT_TRIGGERED) return 1; abort_state = ABORT_CHECKER_NOT_TRIGGERED; return 0;
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34774
to look at the new patch set (#2).
Change subject: lib: ramdetect: Register exception handlers for ARMv8 ......................................................................
lib: ramdetect: Register exception handlers for ARMv8
Register exception handlers to avoid a Synchronous External Abort that is raised when you try to access a non-memory address on ARMv8. An exception handler can jump over the faulting instruction. This is the feature only for QEMU/AArch64.
Signed-off-by: Asami Doi d0iasm.pub@gmail.com Change-Id: I09a306ca307ba4027d9758c3debc2e7c844c66b8 --- M src/lib/ramdetect.c 1 file changed, 47 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/34774/2
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34774
to look at the new patch set (#3).
Change subject: lib: ramdetect: Register exception handlers for ARMv8 ......................................................................
lib: ramdetect: Register exception handlers for ARMv8
Register exception handlers to avoid a Synchronous External Abort that is raised when you try to access a non-memory address on ARMv8. An exception handler can jump over the faulting instruction. This is the feature only for QEMU/AArch64.
Signed-off-by: Asami Doi d0iasm.pub@gmail.com Change-Id: I09a306ca307ba4027d9758c3debc2e7c844c66b8 --- M src/arch/arm64/Makefile.inc A src/arch/arm64/ramdetect.c M src/include/ramdetect.h M src/lib/ramdetect.c 4 files changed, 98 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/34774/3
Asami Doi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34774 )
Change subject: lib: ramdetect: Register exception handlers for ARMv8 ......................................................................
Patch Set 3:
(9 comments)
I have not completed implementing this CL yet. Now, I'm facing the problem "Couldn't load romstage." when it's executed.
I'm thinking of some reasons: - The address of "doit" in arch_prog_run() in src/arch/arm64/boot.c is wrong because it's the entry address for the ramstage but the stage should be the romstage. - The type of "struct prog" is wrong because the next stage should be the ramstage but the type of "struct prog" is the romstage.
The detail is here. $ qemu-system-aarch64 -bios ./build/coreboot.rom -M virt,secure=on,virtualization=on -cpu cortex-a53 -nographic -m 1024M
coreboot-4.10-342-g559fa0d1ce-dirty Tue Aug 20 07:44:56 UTC 2019 bootblock starting (log level: 7)... ARM64: Exception handlers installed. CBFS: 'Master Header Locator' located CBFS at [20100:1000000) CBFS: Locating 'fallback/romstage' CBFS: Found @ offset 80 size 2eab
coreboot-4.10-342-g559fa0d1ce-dirty Tue Aug 20 07:44:56 UTC 2019 romstage starting (log level: 7)... ARM64: Exception handlers installed. RAMDETECT: Found 1024 MiB RAM CBMEM: IMD: root @ 000000007ffff000 254 entries. IMD: root @ 000000007fffec00 62 entries. CBFS: 'Master Header Locator' located CBFS at [20100:1000000) CBFS: Locating 'fallback/ramstage' CBFS: Found @ offset 2f80 size 644a
coreboot-4.10-342-g559fa0d1ce-dirty Tue Aug 20 07:44:56 UTC 2019 ramstage starting (log level: 7)... Couldn't load romstage.
https://review.coreboot.org/c/coreboot/+/34774/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34774/1//COMMIT_MSG@7 PS1, Line 7: lib: Register exception handlers for ARMv8
nit: "ramdetect: " or "lib: ramdetect: " would make it clearer what this is for
Done
https://review.coreboot.org/c/coreboot/+/34774/1//COMMIT_MSG@9 PS1, Line 9: Register exception handlers to avoid a Synchronous External Abort
nit: maybe clarify that this is specifically for the RAM detection feature in emulators, and nothing […]
I added the comment. "This is the feature only for QEMU/AArch64."
https://review.coreboot.org/c/coreboot/+/34774/1/src/lib/ramdetect.c File src/lib/ramdetect.c:
https://review.coreboot.org/c/coreboot/+/34774/1/src/lib/ramdetect.c@53 PS1, Line 53: exception_handler_register(EXC_VID_CUR_SP_EL0_SYNC, &sync_el0);
Do you really need both? I'm pretty sure you should only need one of these (I think EL0).
No, I don't need both. Thank you!
https://review.coreboot.org/c/coreboot/+/34774/1/src/lib/ramdetect.c@55 PS1, Line 55: printk(BIOS_DEBUG, "ARM64: Abort checker handlers installed.\n");
nit: This probably adds more confusion than clarity since the whole probe_ramsize() thing doesn't pr […]
I see. I removed the line.
https://review.coreboot.org/c/coreboot/+/34774/1/src/lib/ramdetect.c@58 PS1, Line 58: probe_mb
If we do this (personally I'm ambivalent... […]
I created 2 versions: 1. Switch operations depending on ENV_ARM64 or not in the patchset 2 (https://review.coreboot.org/c/coreboot/+/34774/2). 2. Use __weak function and make a new file in /arch/arm64 in the patchset3 (https://review.coreboot.org/c/coreboot/+/34774/3).
When I use __weak function, probe_mb() becomes simpler but probe_ramsize() is almost the same so I feel it's verbose. I prefer to have one ramdetect file (patchset2).
https://review.coreboot.org/c/coreboot/+/34774/1/src/lib/ramdetect.c@62 PS1, Line 62: void *ptr = (void *)addr;
Can you do the clang format changes in their own CLs
Yes, I reverted it in this CL.
https://review.coreboot.org/c/coreboot/+/34774/1/src/lib/ramdetect.c@69 PS1, Line 69: if (ENV_ARM64) {
Since you're only reading it shouldn't matter if you overlap code, so can put this above the overlap […]
I moved this if-block to above the overlap check.
https://review.coreboot.org/c/coreboot/+/34774/1/src/lib/ramdetect.c@70 PS1, Line 70: read32((void *)addr);
This could just use 'ptr'.
Done
https://review.coreboot.org/c/coreboot/+/34774/1/src/lib/ramdetect.c@71 PS1, Line 71: return abort_state == ABORT_CHECKER_NOT_TRIGGERED;
Forgot one thing: this needs to reset the abort_state to NOT_TRIGGERED if it has been triggered. […]
Done
Asami Doi has removed Raul Rangel from this change. ( https://review.coreboot.org/c/coreboot/+/34774 )
Change subject: lib: ramdetect: Register exception handlers for ARMv8 ......................................................................
Removed reviewer Raul Rangel.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34774 )
Change subject: lib: ramdetect: Register exception handlers for ARMv8 ......................................................................
Patch Set 3:
(6 comments)
https://review.coreboot.org/c/coreboot/+/34774/3/src/arch/arm64/ramdetect.c File src/arch/arm64/ramdetect.c:
https://review.coreboot.org/c/coreboot/+/34774/3/src/arch/arm64/ramdetect.c@... PS3, Line 14: ABORT_CHECKER_NOT_REGISTERED, : ABORT_CHECKER_DEACTIVATED Remove these
https://review.coreboot.org/c/coreboot/+/34774/3/src/arch/arm64/ramdetect.c@... PS3, Line 36: exception_handler I think this needs to be static. Move it out of the function.
https://review.coreboot.org/c/coreboot/+/34774/3/src/arch/arm64/ramdetect.c@... PS3, Line 45: delete register_checker() and add the following: + abort_state == ABORT_CHECKER_NOT_TRIGGERED; + exception_handler_register(EXC_VID_CUR_SP_EL0_SYNC, &sync_el0);
https://review.coreboot.org/c/coreboot/+/34774/3/src/arch/arm64/ramdetect.c@... PS3, Line 47: if (abort_state == ABORT_CHECKER_NOT_TRIGGERED) : return 1; : abort_state = ABORT_CHECKER_NOT_TRIGGERED Replace with: + exception_handler_unregister(EXC_VID_CUR_SP_EL0_SYNC, &sync_el0); + return abort_state == ABORT_CHECKER_NOT_TRIGGERED;
https://review.coreboot.org/c/coreboot/+/34774/3/src/arch/arm64/ramdetect.c@... PS3, Line 53: probe_ramsize You can remove this with the above suggestion.
https://review.coreboot.org/c/coreboot/+/34774/3/src/lib/ramdetect.c File src/lib/ramdetect.c:
https://review.coreboot.org/c/coreboot/+/34774/3/src/lib/ramdetect.c@49 PS3, Line 49: __weak remove the __weak
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34774 )
Change subject: lib: ramdetect: Register exception handlers for ARMv8 ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/34774/1/src/lib/ramdetect.c File src/lib/ramdetect.c:
https://review.coreboot.org/c/coreboot/+/34774/1/src/lib/ramdetect.c@58 PS1, Line 58: probe_mb
I created 2 versions: […]
Well, now you essentially created a whole separate implementation for the whole thing in patch set 3, which isn't quite what I meant. But I agree that trying to do this with weak functions gets too confusion, and just having a few ENV_ARM64 checks in the main code is way simpler. So I'd also prefer the patch set 2 approach.
https://review.coreboot.org/c/coreboot/+/34774/2/src/lib/ramdetect.c File src/lib/ramdetect.c:
https://review.coreboot.org/c/coreboot/+/34774/2/src/lib/ramdetect.c@46 PS2, Line 46: struct exception_handler sync_el0; Like Raul said in the other patch set, this needs to be static or global. This is probably the reason for your very weird bug (you're using random garbage on the stack as your exception handler function pointer).
https://review.coreboot.org/c/coreboot/+/34774/2/src/lib/ramdetect.c@90 PS2, Line 90: if (ENV_ARM64) { This should be below the part with saved_result below. (It probably makes sense to put both of these right around the for loop.)
Hello Julius Werner, build bot (Jenkins), Raul Rangel, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34774
to look at the new patch set (#4).
Change subject: lib: ramdetect: Register exception handlers for ARMv8 ......................................................................
lib: ramdetect: Register exception handlers for ARMv8
Register exception handlers to avoid a Synchronous External Abort that is raised when you try to access a non-memory address on ARMv8. An exception handler can jump over the faulting instruction. This is the feature only for QEMU/AArch64.
Signed-off-by: Asami Doi d0iasm.pub@gmail.com Change-Id: I09a306ca307ba4027d9758c3debc2e7c844c66b8 --- M src/lib/ramdetect.c 1 file changed, 31 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/34774/4
Hello Julius Werner, build bot (Jenkins), Raul Rangel, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34774
to look at the new patch set (#5).
Change subject: lib: ramdetect: Register exception handlers for ARMv8 ......................................................................
lib: ramdetect: Register exception handlers for ARMv8
Register exception handlers to avoid a Synchronous External Abort that is raised when you try to access a non-memory address on ARMv8. An exception handler can jump over the faulting instruction. This is the feature only for QEMU/AArch64.
Signed-off-by: Asami Doi d0iasm.pub@gmail.com Change-Id: I09a306ca307ba4027d9758c3debc2e7c844c66b8 --- M src/lib/ramdetect.c 1 file changed, 32 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/34774/5
Asami Doi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34774 )
Change subject: lib: ramdetect: Register exception handlers for ARMv8 ......................................................................
Patch Set 5:
(9 comments)
Thank you for all of your suggestions, but it doesn't work well yet. An "exception _sync_sp_el0" happens during the ramstage. In the romstage, it seems to work well. I'm investigating why but please tell me if you find something I overlook.
https://review.coreboot.org/c/coreboot/+/34774/3/src/arch/arm64/ramdetect.c File src/arch/arm64/ramdetect.c:
https://review.coreboot.org/c/coreboot/+/34774/3/src/arch/arm64/ramdetect.c@... PS3, Line 14: ABORT_CHECKER_NOT_REGISTERED, : ABORT_CHECKER_DEACTIVATED
Remove these
Done
https://review.coreboot.org/c/coreboot/+/34774/3/src/arch/arm64/ramdetect.c@... PS3, Line 36: exception_handler
I think this needs to be static. Move it out of the function.
I defined it as a static variable. Thank you.
https://review.coreboot.org/c/coreboot/+/34774/3/src/arch/arm64/ramdetect.c@... PS3, Line 45:
delete register_checker() and add the following: […]
Done
https://review.coreboot.org/c/coreboot/+/34774/3/src/arch/arm64/ramdetect.c@... PS3, Line 47: if (abort_state == ABORT_CHECKER_NOT_TRIGGERED) : return 1; : abort_state = ABORT_CHECKER_NOT_TRIGGERED
Replace with: […]
Done
https://review.coreboot.org/c/coreboot/+/34774/3/src/arch/arm64/ramdetect.c@... PS3, Line 53: probe_ramsize
You can remove this with the above suggestion.
Your suggestion makes code simpler. Thank you!
https://review.coreboot.org/c/coreboot/+/34774/1/src/lib/ramdetect.c File src/lib/ramdetect.c:
https://review.coreboot.org/c/coreboot/+/34774/1/src/lib/ramdetect.c@58 PS1, Line 58: probe_mb
Well, now you essentially created a whole separate implementation for the whole thing in patch set 3 […]
Now I don't need to modify probe_ramsize() because of simplifying the type of abort_state. So I only need to have one "if (ENV_ARM64)" block in probe_mb(). I believe it's fine to keep it in one file.
https://review.coreboot.org/c/coreboot/+/34774/1/src/lib/ramdetect.c@69 PS1, Line 69: if (ENV_ARM64) {
I moved this if-block to above the overlap check.
Done
https://review.coreboot.org/c/coreboot/+/34774/2/src/lib/ramdetect.c File src/lib/ramdetect.c:
https://review.coreboot.org/c/coreboot/+/34774/2/src/lib/ramdetect.c@46 PS2, Line 46: struct exception_handler sync_el0;
Like Raul said in the other patch set, this needs to be static or global. […]
I defined it as a static variable. Thank you!
https://review.coreboot.org/c/coreboot/+/34774/2/src/lib/ramdetect.c@90 PS2, Line 90: if (ENV_ARM64) {
This should be below the part with saved_result below. […]
Now I don't use the if-statement in probe_ramsize() thanks to Raul's suggestion. So I removed this block.
Asami Doi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34774 )
Change subject: lib: ramdetect: Register exception handlers for ARMv8 ......................................................................
Patch Set 5:
I said it's better to have all operations in one file in the previous comment, but it might not work well. Because "struct exc_state" is defined at an ARM specific header file, so other architectures can't build it. I will use __weak function and divide 2 files to implement probe_mb() for other architectures if I can't find any solutions.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34774 )
Change subject: lib: ramdetect: Register exception handlers for ARMv8 ......................................................................
Patch Set 5:
(3 comments)
Patch Set 5:
I said it's better to have all operations in one file in the previous comment, but it might not work well. Because "struct exc_state" is defined at an ARM specific header file, so other architectures can't build it. I will use __weak function and divide 2 files to implement probe_mb() for other architectures if I can't find any solutions.
It should be fine if you wrap it in #ifdef.
Though I think the separate file is cleaner than having ifdefs sprinkled through out the code.
https://review.coreboot.org/c/coreboot/+/34774/5/src/lib/ramdetect.c File src/lib/ramdetect.c:
https://review.coreboot.org/c/coreboot/+/34774/5/src/lib/ramdetect.c@24 PS5, Line 24: You could wrap this all in #ifdef ENV_ARM64
https://review.coreboot.org/c/coreboot/+/34774/5/src/lib/ramdetect.c@25 PS5, Line 25: ; Can you initialize the struct here and avoid setting the handler below? = { .handler = &abort_checker, };
https://review.coreboot.org/c/coreboot/+/34774/5/src/lib/ramdetect.c@36 PS5, Line 36: = Do you need this check now that the handler is only registered for the single read? I would think it's an error if this got called for anything other than a data abort.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34774 )
Change subject: lib: ramdetect: Register exception handlers for ARMv8 ......................................................................
Patch Set 5:
(3 comments)
Thank you for all of your suggestions, but it doesn't work well yet. An "exception _sync_sp_el0" happens during the ramstage. In the romstage, it seems to work well. I'm investigating why but please tell me if you find something I overlook.
I think your ramstage problems stem from the fact that ramstage calls cbmem_initialize() (which ends up calling cbmem_top() and therefore probe_ramsize()) before it calls exception_init(). That seems pretty dumb (and looks like I was the one who put it there many years ago... oops), we should always initialize exception handling as soon as possible (i.e. right after console_init()) so as much code as possible can benefit from it.
Can you upload a separate patch that moves the exception_init() call in src/lib/hardwaremain.c#main further up (to right below console_init())?
https://review.coreboot.org/c/coreboot/+/34774/5/src/lib/ramdetect.c File src/lib/ramdetect.c:
https://review.coreboot.org/c/coreboot/+/34774/5/src/lib/ramdetect.c@24 PS5, Line 24:
You could wrap this all in #ifdef ENV_ARM64
It needs to be #if ENV_ARM64, not #ifdef... but yes, we could do that. Then again, with the new approach here we could just make probe_mb() weak and only override that, and that should be pretty clean as well.
https://review.coreboot.org/c/coreboot/+/34774/5/src/lib/ramdetect.c@36 PS5, Line 36: =
Do you need this check now that the handler is only registered for the single read? I would think it […]
Well, it's an error yes, and you'd want that to be reported by the normal crash handler rather than misinterpreted as an invalid access. So I think this still makes sense (although probably never gonna happen).
https://review.coreboot.org/c/coreboot/+/34774/5/src/lib/ramdetect.c@60 PS5, Line 60: exception_handler_unregister(EXC_VID_CUR_SP_EL0_SYNC, &sync_el0); Oh, funny, I didn't know we had this (although the current version looks a bit broken to me, but not in a way that should affect you).
Hello Julius Werner, build bot (Jenkins), Raul Rangel, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34774
to look at the new patch set (#6).
Change subject: lib: ramdetect: Register exception handlers for ARMv8 ......................................................................
lib: ramdetect: Register exception handlers for ARMv8
Register exception handlers to avoid a Synchronous External Abort that is raised when you try to access a non-memory address on ARMv8. An exception handler can jump over the faulting instruction. This is the feature only for QEMU/AArch64.
Signed-off-by: Asami Doi d0iasm.pub@gmail.com Change-Id: I09a306ca307ba4027d9758c3debc2e7c844c66b8 --- M src/lib/ramdetect.c 1 file changed, 31 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/34774/6
Hello Julius Werner, build bot (Jenkins), Raul Rangel, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34774
to look at the new patch set (#7).
Change subject: lib: ramdetect: Register exception handlers for ARMv8 ......................................................................
lib: ramdetect: Register exception handlers for ARMv8
Register exception handlers to avoid a Synchronous External Abort that is raised when you try to access a non-memory address on ARMv8. An exception handler can jump over the faulting instruction. This is the feature only for QEMU/AArch64.
Signed-off-by: Asami Doi d0iasm.pub@gmail.com Change-Id: I09a306ca307ba4027d9758c3debc2e7c844c66b8 --- M src/arch/arm64/Makefile.inc A src/arch/arm64/ramdetect.c M src/include/ramdetect.h M src/lib/ramdetect.c 4 files changed, 46 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/34774/7
Hello Julius Werner, build bot (Jenkins), Raul Rangel, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34774
to look at the new patch set (#8).
Change subject: lib: ramdetect: Register exception handlers for ARMv8 ......................................................................
lib: ramdetect: Register exception handlers for ARMv8
Register exception handlers to avoid a Synchronous External Abort that is raised when you try to access a non-memory address on ARMv8. An exception handler can jump over the faulting instruction. This is the feature only for QEMU/AArch64.
Signed-off-by: Asami Doi d0iasm.pub@gmail.com Change-Id: I09a306ca307ba4027d9758c3debc2e7c844c66b8 --- M src/arch/arm64/Makefile.inc A src/arch/arm64/ramdetect.c M src/include/ramdetect.h M src/lib/ramdetect.c 4 files changed, 45 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/34774/8
Asami Doi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34774 )
Change subject: lib: ramdetect: Register exception handlers for ARMv8 ......................................................................
Patch Set 8:
(4 comments)
I think your ramstage problems stem from the fact that ramstage calls cbmem_initialize() (which ends up calling cbmem_top() and therefore probe_ramsize()) before it calls exception_init(). That seems pretty dumb (and looks like I was the one who put it there many years ago... oops), we should always initialize exception handling as soon as possible (i.e. right after console_init()) so as much code as possible can benefit from it.
Can you upload a separate patch that moves the exception_init() call in src/lib/hardwaremain.c#main further up (to right below console_init())?
That was exactly the place it made the problem. Thank you so much! I made a new CL to move exception_init() to before cbmem_initialize(). Could you take a look at it? https://review.coreboot.org/c/coreboot/+/35022
https://review.coreboot.org/c/coreboot/+/34774/5/src/lib/ramdetect.c File src/lib/ramdetect.c:
https://review.coreboot.org/c/coreboot/+/34774/5/src/lib/ramdetect.c@24 PS5, Line 24:
It needs to be #if ENV_ARM64, not #ifdef... but yes, we could do that. […]
I created 2 versions: 1. Use #if ENV_ARM64 (https://review.coreboot.org/c/coreboot/+/34774/6) 2. Override probe_mb() (https://review.coreboot.org/c/coreboot/+/34774/8) Both are cleaner than the previous implementation. They are acceptable to me.
https://review.coreboot.org/c/coreboot/+/34774/5/src/lib/ramdetect.c@25 PS5, Line 25: ;
Can you initialize the struct here and avoid setting the handler below? […]
Done
https://review.coreboot.org/c/coreboot/+/34774/5/src/lib/ramdetect.c@36 PS5, Line 36: =
Well, it's an error yes, and you'd want that to be reported by the normal crash handler rather than […]
I believe this if-statement is similar to assert(raw_read_esr_el3() >> 26 == 0x25). It would help for other developers to understand that the handler expects a data abort. However, I think it's ok to remove it.
https://review.coreboot.org/c/coreboot/+/34774/5/src/lib/ramdetect.c@60 PS5, Line 60: exception_handler_unregister(EXC_VID_CUR_SP_EL0_SYNC, &sync_el0);
Oh, funny, I didn't know we had this (although the current version looks a bit broken to me, but not […]
Sorry, do you mean that unregistering a handler might affect other parts?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34774 )
Change subject: lib: ramdetect: Register exception handlers for ARMv8 ......................................................................
Patch Set 8: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/34774/5/src/lib/ramdetect.c File src/lib/ramdetect.c:
https://review.coreboot.org/c/coreboot/+/34774/5/src/lib/ramdetect.c@60 PS5, Line 60: exception_handler_unregister(EXC_VID_CUR_SP_EL0_SYNC, &sync_el0);
Sorry, do you mean that unregistering a handler might affect other parts?
No, I mean the implementation of exception_handler_unregister() has a bug. If you look at it, it walks a list but never updates the 'prev' pointer, so when it unlinks a handler it will unintentionally unlink every handler before that as well.
I doubt that anyone ever tested that function with more than one handler registered...
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34774 )
Change subject: lib: ramdetect: Register exception handlers for ARMv8 ......................................................................
Patch Set 9: Code-Review+1
(2 comments)
Thanks for making the changes. It's nice and easy to read now.
https://review.coreboot.org/c/coreboot/+/34774/9/src/arch/arm64/ramdetect.c File src/arch/arm64/ramdetect.c:
https://review.coreboot.org/c/coreboot/+/34774/9/src/arch/arm64/ramdetect.c@... PS9, Line 38: void Don't think you need the cast anymore.
https://review.coreboot.org/c/coreboot/+/34774/9/src/include/ramdetect.h File src/include/ramdetect.h:
https://review.coreboot.org/c/coreboot/+/34774/9/src/include/ramdetect.h@22 PS9, Line 22: probe_mb The doc above is for probe_ramsize, not probe_mb. Add some documentation to probe_mb as well.
Hello Julius Werner, build bot (Jenkins), Raul Rangel, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34774
to look at the new patch set (#10).
Change subject: lib: ramdetect: Register exception handlers for ARMv8 ......................................................................
lib: ramdetect: Register exception handlers for ARMv8
Register exception handlers to avoid a Synchronous External Abort that is raised when you try to access a non-memory address on ARMv8. An exception handler can jump over the faulting instruction. This is the feature only for QEMU/AArch64.
Signed-off-by: Asami Doi d0iasm.pub@gmail.com Change-Id: I09a306ca307ba4027d9758c3debc2e7c844c66b8 --- M src/arch/arm64/Makefile.inc A src/arch/arm64/ramdetect.c M src/include/ramdetect.h M src/lib/ramdetect.c 4 files changed, 50 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/34774/10
Asami Doi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34774 )
Change subject: lib: ramdetect: Register exception handlers for ARMv8 ......................................................................
Patch Set 9:
(6 comments)
https://review.coreboot.org/c/coreboot/+/34774/9/src/arch/arm64/ramdetect.c File src/arch/arm64/ramdetect.c:
https://review.coreboot.org/c/coreboot/+/34774/9/src/arch/arm64/ramdetect.c@... PS9, Line 38: void
Don't think you need the cast anymore.
Done
https://review.coreboot.org/c/coreboot/+/34774/9/src/include/ramdetect.h File src/include/ramdetect.h:
https://review.coreboot.org/c/coreboot/+/34774/9/src/include/ramdetect.h@22 PS9, Line 22: probe_mb
The doc above is for probe_ramsize, not probe_mb. Add some documentation to probe_mb as well.
Done
https://review.coreboot.org/c/coreboot/+/34774/1/src/lib/ramdetect.c File src/lib/ramdetect.c:
https://review.coreboot.org/c/coreboot/+/34774/1/src/lib/ramdetect.c@58 PS1, Line 58: probe_mb
Now I don't need to modify probe_ramsize() because of simplifying the type of abort_state. […]
Done
https://review.coreboot.org/c/coreboot/+/34774/3/src/lib/ramdetect.c File src/lib/ramdetect.c:
https://review.coreboot.org/c/coreboot/+/34774/3/src/lib/ramdetect.c@49 PS3, Line 49: __weak
remove the __weak
Done
https://review.coreboot.org/c/coreboot/+/34774/5/src/lib/ramdetect.c File src/lib/ramdetect.c:
https://review.coreboot.org/c/coreboot/+/34774/5/src/lib/ramdetect.c@24 PS5, Line 24:
I created 2 versions: […]
I'm using the second solution now.
https://review.coreboot.org/c/coreboot/+/34774/5/src/lib/ramdetect.c@36 PS5, Line 36: =
I believe this if-statement is similar to assert(raw_read_esr_el3() >> 26 == 0x25). […]
Done
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34774 )
Change subject: lib: ramdetect: Register exception handlers for ARMv8 ......................................................................
Patch Set 10: Code-Review+2
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34774 )
Change subject: lib: ramdetect: Register exception handlers for ARMv8 ......................................................................
lib: ramdetect: Register exception handlers for ARMv8
Register exception handlers to avoid a Synchronous External Abort that is raised when you try to access a non-memory address on ARMv8. An exception handler can jump over the faulting instruction. This is the feature only for QEMU/AArch64.
Signed-off-by: Asami Doi d0iasm.pub@gmail.com Change-Id: I09a306ca307ba4027d9758c3debc2e7c844c66b8 Reviewed-on: https://review.coreboot.org/c/coreboot/+/34774 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Raul Rangel rrangel@chromium.org --- M src/arch/arm64/Makefile.inc A src/arch/arm64/ramdetect.c M src/include/ramdetect.h M src/lib/ramdetect.c 4 files changed, 50 insertions(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Raul Rangel: Looks good to me, approved
diff --git a/src/arch/arm64/Makefile.inc b/src/arch/arm64/Makefile.inc index 6bb7196..89cc878 100644 --- a/src/arch/arm64/Makefile.inc +++ b/src/arch/arm64/Makefile.inc @@ -107,6 +107,7 @@ romstage-y += memset.S romstage-y += memcpy.S romstage-y += memmove.S +romstage-y += ramdetect.c romstage-y += romstage.c romstage-y += transition.c transition_asm.S
@@ -131,6 +132,7 @@ ramstage-y += eabi_compat.c ramstage-y += boot.c ramstage-y += tables.c +ramstage-y += ramdetect.c ramstage-$(CONFIG_ARM64_USE_ARCH_TIMER) += arch_timer.c ramstage-y += memset.S ramstage-y += memcpy.S diff --git a/src/arch/arm64/ramdetect.c b/src/arch/arm64/ramdetect.c new file mode 100644 index 0000000..bc034c3 --- /dev/null +++ b/src/arch/arm64/ramdetect.c @@ -0,0 +1,41 @@ +/* + * This file is part of the coreboot project. + * + * SPDX-License-Identifier: GPL-2.0-or-later + */ + +#include <types.h> +#include <device/mmio.h> +#include <ramdetect.h> +#include <arch/exception.h> +#include <arch/transition.h> + +static enum { + ABORT_CHECKER_NOT_TRIGGERED, + ABORT_CHECKER_TRIGGERED, +} abort_state = ABORT_CHECKER_NOT_TRIGGERED; + +static int abort_checker(struct exc_state *state, uint64_t vector_id) +{ + if (raw_read_esr_el3() >> 26 != 0x25) + return EXC_RET_IGNORED; /* Not a data abort. */ + + abort_state = ABORT_CHECKER_TRIGGERED; + state->elx.elr += sizeof(uint32_t); /* Jump over faulting instruction. */ + raw_write_elr_el3(state->elx.elr); + return EXC_RET_HANDLED; +} + +static struct exception_handler sync_el0 = {.handler = &abort_checker}; + +int probe_mb(const uintptr_t dram_start, const uintptr_t size) +{ + uintptr_t addr = dram_start + (size * MiB) - sizeof(uint32_t); + void *ptr = (void *)addr; + + abort_state = ABORT_CHECKER_NOT_TRIGGERED; + exception_handler_register(EXC_VID_CUR_SP_EL0_SYNC, &sync_el0); + read32(ptr); + exception_handler_unregister(EXC_VID_CUR_SP_EL0_SYNC, &sync_el0); + return abort_state == ABORT_CHECKER_NOT_TRIGGERED; +} diff --git a/src/include/ramdetect.h b/src/include/ramdetect.h index b63cdf1..e2a7ece 100644 --- a/src/include/ramdetect.h +++ b/src/include/ramdetect.h @@ -11,6 +11,12 @@ * GNU General Public License for more details. */
+ +/* + * Used in probe_ramsize(). This is a weak function and it's overridden for + * ARMv8. Return 1 when DRAM exists at the address, otherwise return 0. + */ +int probe_mb(const uintptr_t dram_start, const uintptr_t size); /* * Probe an area if it's read/writable. * Primary use case is the detection of DRAM amount on emulators. diff --git a/src/lib/ramdetect.c b/src/lib/ramdetect.c index 5416a58..2c83092 100644 --- a/src/lib/ramdetect.c +++ b/src/lib/ramdetect.c @@ -19,7 +19,7 @@
#define OVERLAP(a, b, s, e) ((b) > (s) && (a) < (e))
-static int probe_mb(const uintptr_t dram_start, const uintptr_t size) +int __weak probe_mb(const uintptr_t dram_start, const uintptr_t size) { uintptr_t addr = dram_start + (size * MiB) - sizeof(uint32_t); static const uint32_t patterns[] = {
-
Asami Doi (Code Review) -
Julius Werner (Code Review)
-
Patrick Georgi (Code Review)
-
Raul Rangel (Code Review)