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.