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;