Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36657 )
Change subject: lib/fmap: Add optional pre-RAM cache ......................................................................
Patch Set 3:
(8 comments)
I wonder, is it necessary to manually set the size of the CAR/SRAM FMAP cache via a config option? Or could we just set it to the exact size of the known FMAP and just have the config as a on/off? (And if there isn't enough room, it'll still know at link time and fail.)
Okay, did that for x86. For Arm, see inline comment for why I'd like to stick to explicit sizes.
I'm starting to wonder if it's not getting easier to define a section for persistent preram objects that you use objcopy to make sure they are the same between early stages or just use the romstage one for all early stages.
Not really sure I get the objcopy part. I agree that we're accumulating quite a lot of these pre-RAM cache areas and it might be nice to combine them somehow... maybe we could just have a big PRERAM_CACHE() section and then FMAP, timestamps, console, CBFS and others will automatically grab their required amount of space from that. One tricky issue would be to make sure one cannot starve the others, though... e.g. you don't want to fail to boot because you added so many timestamps you can't fit a critical file into CBFS cache anymore.
Anyway, one patch at a time.
https://review.coreboot.org/c/coreboot/+/36657/3/src/arch/x86/car.ld File src/arch/x86/car.ld:
https://review.coreboot.org/c/coreboot/+/36657/3/src/arch/x86/car.ld@36 PS3, Line 36: FMAP_CACHE(., CONFIG_FMAP_CACHE_SIZE) Decided to change this to just use FMAP_SIZE. I could add a boolean Kconfig to disable it, but I doubt any of our x86 platforms are so tight on CAR that that would be necessary?
https://review.coreboot.org/c/coreboot/+/36657/3/src/include/memlayout.h File src/include/memlayout.h:
https://review.coreboot.org/c/coreboot/+/36657/3/src/include/memlayout.h@35 PS3, Line 35: #define STR(x) XSTR(x) : #define XSTR(x) #x
I don't doubt that this is necessary... […]
It makes it so the 'FMAP_SIZE' macro in the error message below will actually be expanded. It's a common trick (in fact we actually have another copy of STR() in src/arch/riscv/include/bits.h which already does this), I just forgot to add it the first time I wrote this file.
https://review.coreboot.org/c/coreboot/+/36657/3/src/include/memlayout.h@79 PS3, Line 79: FMAP_CACHE(addr, sz)
Any need to specify the size and not use the one specified by fmap_config. […]
This is sort of a philosophical choice... I prefer memlayout to always explicitly contain address and size for every section so that the file can serve as a reference as well as a definition. It's true that we could automatically determine it in this case (and the linker would catch overflows), but you don't really gain anything from that either -- you still have to decide how far behind this you place the next section. I think defining the available space explicitly rather than implicitly through the next member is more clear and more consistent with the rest of memlayout.
(I could however change the x86 version to just use FMAP_SIZE if you want. It would make more sense there because the addresses are dynamic anyway.)
https://review.coreboot.org/c/coreboot/+/36657/3/src/lib/fmap.c File src/lib/fmap.c:
https://review.coreboot.org/c/coreboot/+/36657/3/src/lib/fmap.c@35 PS3, Line 35: print_once
Not sure I like this name... it sounds like the function should take care of setting fmap_print_once itself. On the other hand, I can't think of anything better at the moment.
I mean, it does *effectively* print once because all this code will be walked through exactly once before report() is called to disable it. I don't really see a better name either (print_first() or something like that is less clear, I think).
Yes, it will print once per stage (but if you use the cache, all lines using this only execute in the bootblock anyway).
https://review.coreboot.org/c/coreboot/+/36657/3/src/lib/fmap.c@64 PS3, Line 64: print_once(BIOS_WARNING, : "WARNING: Post-RAM FMAP access too early for cache!\n");
I guess it could be helpful to add a comment that this code is reached before cbmem_init?
Yeah, that was what I was trying to express without making the message too long. Will add a comment as well.
https://review.coreboot.org/c/coreboot/+/36657/3/src/lib/fmap.c@70 PS3, Line 70: BIOS_WARNING
BIOS_NOTICE since running without pre-RAM cache is also possible?
Okay. I just want to make sure it doesn't become common and too easy to overlook (really, the only reason we need to support that is for that damn RK3288 platform with 100K SRAM).
https://review.coreboot.org/c/coreboot/+/36657/3/src/lib/fmap.c@305 PS3, Line 305: const ssize_t ret = rdev_readat(&fmrd, fmap, 0, s);
So is this re-reading FMAP when CBMEM comes up, rather than copying it from CAR/SRAM if it is availa […]
No, in the cached case fmrd is chained from fmap_cache here, which is the pre-RAM memory buffer. So even though this is "reading from an rdev", it's really just a memcpy() from CAR/SRAM into CBMEM.
https://review.coreboot.org/c/coreboot/+/36657/3/src/soc/mediatek/mt8173/inc... File src/soc/mediatek/mt8173/include/soc/memlayout.ld:
https://review.coreboot.org/c/coreboot/+/36657/3/src/soc/mediatek/mt8173/inc... PS3, Line 44: 12K
Should probably note this reduction in the commit message.
Done