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.