Attention is currently required from: Martin Roth, ron minnich, Paul Menzel, Stefan Reinauer, Julius Werner, Angel Pons, Keith Hui, Patrick Rudolph, Felix Held. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/55068 )
Change subject: Allow to build romstage sources inside the bootblock ......................................................................
Patch Set 11:
(13 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/55068/comment/5843e7b2_69cb697a PS1, Line 9: H
or when the romstage is too large because it doesn't call into an external blob ;-) […]
Done
https://review.coreboot.org/c/coreboot/+/55068/comment/af9e8c04_6f471b48 PS1, Line 22: TODO make this option only visible when it makes sense
Please mention, this is controlled by a Kconfig option.
Done
Patchset:
PS4:
Why are we architecturally going back to 2004 in order to save 10-20kb? With all due respect but that seems like a very questionable micro-optimization at best to put it mildly. Are you really suggesting creating another code path for a platform that only runs on at least 2mb worth of blobs is worth it for 10kb on a 32MB flash chip?
I wasn't around then but 2004 coreboot looks very different from 2021 coreboot. There were romcc compiled stages and a bootblock/romstage separation just made more sense then. Also the x86 bootblock as limited to 64K which is not the case anymore. Now coreboot already supports tons of different bootpaths: just take a look at the rules.h file! Coreboot supports statically linked code, XIP code, relocatable code, separate or not verstage either linked in bootblock or in romstage, decompressor stages in bootblock to make things smaller and faster, ... This just adds one extra bootpath. I don't get why it's so scary. Keeping the complexity increase limited for different use cases is a price to pay and however often it leads to better abstractions and APIs, therefore increasing the general quality of the codebase. See the previous patch that makes for a common romstage entry. Besides this patch barely changes 20 lines and could probably be even less! I think this is a solid argument that this bootpath won't be hard to maintain.
I don't get why the size of some big blobs required on some platform is a good argument for anything. It's just being pessimistic about any optimization one can make in coreboot.
Implementing this on x86 is just a POC. The concept is not x86 specific though and requires only small changes for other platforms. On x86 often makes more sense for a 'big' bootblock than on other platforms since there is a lot of care and complexity needed to keep CAR symbols in sync between stages. On AMD systems it's even worse as APs need to communicate across stages stages without having common CAR. FWIW I could just as easily turn your argument around and ask why bootblock/romstage separation is needed or good. For a lot of use cases bootblock/romstage is architecturally over engineered and a historical artifact. That solution had a lot of merits at the time for sure, but things are different now as there is no romcc anymore. It's currently useful for VBOOT and some other edge cases and that's it. Why should 'one' usecase determine how all other images should look and function?
If you want to save space, drop all the bloat that made it into the bootblock that shouldn't be in there in the first place (like console drivers)
Or the bloat that came with our so called generic drivers.
The code pulled in bootblock is very often the same code as the one in romstage and not just more bloated things like console drivers but 'essential' things required to continue booting, like fmap, cbfs, decompression. That code is present in all stages so less stages is often a good idea. https://review.coreboot.org/c/coreboot/+/52788/4 for instance links verstage inside the bootblock.
I want to also include ramstage inside the bootblock on AMD platforms so that those only have one stage which makes sense as the x86 boots into RAM.
PS4:
I still have to build it with seabios with a ps/2 init patch for boot testing - I do have issues w […]
Done
Patchset:
PS7:
Building for asus/p2b-ls breaks when selecting the option to build separate romstage with this patch series applied:
src/arch/x86/romstage.c:18:17: error: no previous prototype for 'car_stage_entry'
It could be because of cb:55067 in the train - Then again I have to edit that patch for it to apply, so please reproduce.
Otherwise, I got these results of free flash space testing for LTO and this: (No payload, build config included in cbfs per default)
With LTO, combined romstage: 148516 bytes free With LTO, without this train: 140004 bytes free Without LTO, combined romstage: 137316 bytes free Without LTO nor this train: 127652 bytes free
Seems to build.
File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/55068/comment/3749fbc4_de4475e3 PS4, Line 171: default y
Please add a help text explaining what it does and what trade-offs should be considered in setting i […]
Done
File src/drivers/amd/agesa/def_callouts.c:
https://review.coreboot.org/c/coreboot/+/55068/comment/bd5d5aa5_7abe35f4 PS1, Line 132: if (!(ENV_ROMSTAGE || (ENV_BOOTBLOCK && !CONFIG(SEPARATE_ROMSTAGE)))
Would defining both `ENV_BOOTBLOCK` and `ENV_ROMSTAGE` for the combined stage work? Or does existi […]
Done
File src/include/rules.h:
https://review.coreboot.org/c/coreboot/+/55068/comment/51592eb4_4f0a5efb PS4, Line 37: #define ENV_ROMSTAGE !CONFIG(SEPARATE_ROMSTAGE)
Also incompatible setups like separate verstage and !separate romstage should be guarded against.
Good point, you should just add a Kconfig dependency for that.
Done in later CL
File src/lib/prog_loaders.c:
https://review.coreboot.org/c/coreboot/+/55068/comment/a19cb373_6b6299ee PS1, Line 4: #include "commonlib/timestamp_serialized.h"
Shouldn't this #include use `<>` instead of double quotes?
Done
https://review.coreboot.org/c/coreboot/+/55068/comment/3f8687c2_b41f36d0 PS1, Line 19: #include <romstage_common.h>
x86-specific, apparently. […]
Done
File src/lib/prog_loaders.c:
https://review.coreboot.org/c/coreboot/+/55068/comment/b48ca65d_9a71daa0 PS4, Line 24: romstage_main();
This isn't marked __noreturn__ so you'll want to add a return after this to avoid linking all the re […]
Done
File src/southbridge/intel/bd82x6x/early_pch.c:
https://review.coreboot.org/c/coreboot/+/55068/comment/b3add2be_c885f901 PS1, Line 313: if (ENV_ROMSTAGE || (ENV_BOOTBLOCK && !CONFIG(SEPARATE_ROMSTAGE)))
This guard exists because this function is called twice: once in bootblock, once in romstage. I had some patches to clean it up, but they're probably stale by now...
src/southbridge/intel/bd82x6x/bootblock.c: early_pch_init(); src/northbridge/intel/sandybridge/romstage.c: early_pch_init();
Yeah that needs to be fixed. That was done for some weird compatibility reasons but there is no need for that anymore.
Not sure if cleaned up, but it should not block this.
File src/southbridge/intel/i82801ix/early_init.c:
https://review.coreboot.org/c/coreboot/+/55068/comment/ec408413_4859fe45 PS1, Line 50: if (ENV_ROMSTAGE || (ENV_BOOTBLOCK && !CONFIG(SEPARATE_ROMSTAGE)))
Same as bd82x6x, this function is called twice: once in bootblock, once in romstage. This is the case for both users of this southbridge (gm45 and qemu-q35).
src/southbridge/intel/i82801ix/bootblock.c: i82801ix_early_init(); src/northbridge/intel/gm45/romstage.c: i82801ix_early_init(); src/mainboard/emulation/qemu-q35/romstage.c: i82801ix_early_init();
Needs a cleanup.
-
Arthur Heymans (Code Review)