[coreboot-gerrit] [M] Change in coreboot[master]: Allow to build romstage sources inside the bootblock

[Julius Werner (Code Review)]

Attention is currently required from: Angel Pons, Arthur Heymans, Branden Waldner, Christian Walter, Felix Held, Jakub Czapiga, Keith Hui, Martin L Roth, Patrick Rudolph, Paul Menzel, Raul Rangel, Stefan Reinauer, ron minnich.

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/55068?usp=email )

Change subject: Allow to build romstage sources inside the bootblock
......................................................................

Patch Set 39:

(7 comments)

File src/cpu/x86/cache/cache.c:

https://review.coreboot.org/c/coreboot/+/55068/comment/feb979c2_38ff8eaa :
PS39, Line 36: ENV_CREATES_CBMEM
nit: same thing but maybe ENV_RAMINIT would fit a little better here?

File src/include/rules.h:

https://review.coreboot.org/c/coreboot/+/55068/comment/0053f931_18da3608 :
PS39, Line 306: #define ENV_HAS_CBMEM		(ENV_SEPARATE_ROMSTAGE || ENV_POSTCAR || ENV_RAMSTAGE || \
I think this is missing the VBOOT_STARTS_IN_ROMSTAGE case. If you want this to be a perfect replacement for cbmem_possibly_online() (which it should be), maybe just write it the same way, e.g.
```
#define ENV_HAS_CBMEM !(ENV_BOOTBLOCK || (ENV_SEPARATE_VERSTAGE && CONFIG(VBOOT_STARTS_IN_BOOTBLOCK))
```

File src/lib/cbfs.c:

https://review.coreboot.org/c/coreboot/+/55068/comment/5d5e6893_33bd7605 :
PS39, Line 135: {
Sorry, I may have forgotten what was going on here, but where did the FSP stuff go? I think we still need that as well? (If you just copied the suggestion I posted last year, I think that was meant to go after the FSP lines.)

File src/security/vboot/Kconfig:

https://review.coreboot.org/c/coreboot/+/55068/comment/e73bb704_7e67fb59 :
PS39, Line 93: 	select SEPARATE_ROMSTAGE
I feel like this would probably be better as a `depends on` than a `select`?

File src/security/vboot/common.c:

https://review.coreboot.org/c/coreboot/+/55068/comment/f5f9d74c_f810c1bd :
PS39, Line 82: 	 * been made yet, e.g. with !CONFIG(SEPARATE_ROMSTAGE).
This config combination is illegal now, so this code here isn't needed.

File src/security/vboot/misc.h:

https://review.coreboot.org/c/coreboot/+/55068/comment/46eceef0_64cae9b0 :
PS39, Line 51: ENV_CREATES_CBMEM
nit: also maybe ENV_RAMINIT?

File src/security/vboot/vboot_common.c:

https://review.coreboot.org/c/coreboot/+/55068/comment/845e2a9b_60c30f77 :
PS39, Line 31: 	if (!verification_should_run() && !(ENV_CREATES_CBMEM && CONFIG(VBOOT_EARLY_EC_SYNC))) {
nit: here too

(And really it could also be ENV_SEPARATE_ROMSTAGE because doing an early EC sync before vboot makes no sense. We should have make EARLY_EC_SYNC depend on VBOOT_STARTS_IN_BOOTBLOCK, but it looks like we forgot.)