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