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).