Attention is currently required from: Dinesh Gehlot.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/76393?usp=email )
Change subject: src/{include,cpu,lib}: Implement framework for PRERAM buffer ......................................................................
Patch Set 1:
(4 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/76393/comment/74bcb3f4_29b8bf65 : PS1, Line 17: BUG=b:273661726 I think you mean 285405031? This looks unrelated.
Patchset:
PS1: This is an interesting change, but a pretty big one (conceptually). Is there a design doc or something somewhere that outlines your larger plans with this? I think if we want to do something like this, then it needs to be done all the way, on both Arm and x86, for all the sections that get allocated elsewhere in pre-RAM stages and later migrated to CBMEM (timestamps, vboot workbuffer, console, CBFS mcache, etc.), not just as a one-off for one specific thing which then leaves the rest of coreboot in an inconsistent state between some code using the "old way" and other code this new approach to persistent pre-RAM data. And we should also carefully evaluate potential drawbacks (e.g. whether there are systems that have constraints on how we can shuffle around memory areas in the pre-RAM environment that would make it problematic to move all the CBMEM-related stuff into one big, contiguous block, like the MediaTek split SRAM systems).
Again, not saying I'm against this, I'm just saying this is something we shouldn't do half-baked.
File src/cpu/intel/car/romstage.c:
https://review.coreboot.org/c/coreboot/+/76393/comment/ef4de93a_421b9756 : PS1, Line 42: CBMEM_EARLY_BACKUP I don't think it really makes sense to call this "backup" when it's not really "backing up" anything? I'd just call it pre-RAM CBMEM.
File src/lib/preram_cbmem.c:
PS1: Why are you implementing a completely new memory allocator from scratch rather than just using the same IMD/CBMEM allocator code on a memory area in CAR? Wouldn't that make more sense (and then you could later just memcpy() the used memory area and then update the availabe size in the root node, rather than having to individually re-add each entry manually). Is there a specific reason the IMD allocator would be unsuitable for pre-RAM use?