Hello Patrick Georgi, Furquan Shaikh,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/48429
to review the following change.
Change subject: cbfs: Skip mcache in post-RAM stages before CBMEM is online ......................................................................
cbfs: Skip mcache in post-RAM stages before CBMEM is online
There have been a few issues with the new CBFS mcache code in stages after romstage, where the mcache resides in CBMEM. In a few special cases the stage may be doing a CBFS lookup before calling cbmem_initialize(). To avoid breaking those cases, this patch makes the CBFS code fall back to a lookup from flash if CBMEM hasn't been reinitialized yet in those stages.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: Icf6d1a1288cb243d0c4c893cc58251687e2873b0 --- M src/lib/cbfs.c 1 file changed, 2 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/48429/1
diff --git a/src/lib/cbfs.c b/src/lib/cbfs.c index d275505..5e6fce4 100644 --- a/src/lib/cbfs.c +++ b/src/lib/cbfs.c @@ -27,7 +27,8 @@
size_t data_offset; cb_err_t err = CB_CBFS_CACHE_FULL; - if (!CONFIG(NO_CBFS_MCACHE) && !ENV_SMM) + if (!CONFIG(NO_CBFS_MCACHE) && !ENV_SMM && + (ENV_ROMSTAGE_OR_BEFORE || cbmem_online())) err = cbfs_mcache_lookup(cbd->mcache, cbd->mcache_size, name, mdata, &data_offset); if (err == CB_CBFS_CACHE_FULL) {
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48429 )
Change subject: cbfs: Skip mcache in post-RAM stages before CBMEM is online ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48429/1/src/lib/cbfs.c File src/lib/cbfs.c:
https://review.coreboot.org/c/coreboot/+/48429/1/src/lib/cbfs.c@31 PS1, Line 31: ENV_ROMSTAGE_OR_BEFORE Wouldn't this cause romstage to always skip the lookup? Isn't cbmem_online() check sufficient here?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48429 )
Change subject: cbfs: Skip mcache in post-RAM stages before CBMEM is online ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48429/1/src/lib/cbfs.c File src/lib/cbfs.c:
https://review.coreboot.org/c/coreboot/+/48429/1/src/lib/cbfs.c@31 PS1, Line 31: ENV_ROMSTAGE_OR_BEFORE
Wouldn't this cause romstage to always skip the lookup? Isn't cbmem_online() check sufficient here?
...no? Or did I confuse myself with the logic? (My romstage still says "Found ... in mcache", so I think it's WAI.) We're looking up files in the mcache if:
1. mcache is enabled, AND 2. we're not in SMM, AND 3. (at least one is true of) 3.1 this is a pre-RAM stage where mcache is in CAR/SRAM, OR 3.2 CBMEM is online
So romstage should be fine and use the mcache because 1, 2 and 3.1 are true.
Just checking cbmem_online() isn't sufficient because CBMEM is never online in early stages (like verstage), yet we still want to use the mcache which is still in its pre-RAM location at that point.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48429 )
Change subject: cbfs: Skip mcache in post-RAM stages before CBMEM is online ......................................................................
Patch Set 1: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/48429/1/src/lib/cbfs.c File src/lib/cbfs.c:
https://review.coreboot.org/c/coreboot/+/48429/1/src/lib/cbfs.c@31 PS1, Line 31: ENV_ROMSTAGE_OR_BEFORE
...no? Or did I confuse myself with the logic? (My romstage still says "Found ... […]
Sorry, I had confused myself. What you said ^^ makes sense.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48429 )
Change subject: cbfs: Skip mcache in post-RAM stages before CBMEM is online ......................................................................
Patch Set 1: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48429 )
Change subject: cbfs: Skip mcache in post-RAM stages before CBMEM is online ......................................................................
Patch Set 1: Code-Review+2
Julius Werner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/48429 )
Change subject: cbfs: Skip mcache in post-RAM stages before CBMEM is online ......................................................................
cbfs: Skip mcache in post-RAM stages before CBMEM is online
There have been a few issues with the new CBFS mcache code in stages after romstage, where the mcache resides in CBMEM. In a few special cases the stage may be doing a CBFS lookup before calling cbmem_initialize(). To avoid breaking those cases, this patch makes the CBFS code fall back to a lookup from flash if CBMEM hasn't been reinitialized yet in those stages.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: Icf6d1a1288cb243d0c4c893cc58251687e2873b0 Reviewed-on: https://review.coreboot.org/c/coreboot/+/48429 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Furquan Shaikh furquan@google.com Reviewed-by: Arthur Heymans arthur@aheymans.xyz Reviewed-by: Angel Pons th3fanbus@gmail.com --- M src/lib/cbfs.c 1 file changed, 2 insertions(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved Arthur Heymans: Looks good to me, approved Angel Pons: Looks good to me, approved
diff --git a/src/lib/cbfs.c b/src/lib/cbfs.c index d275505..5e6fce4 100644 --- a/src/lib/cbfs.c +++ b/src/lib/cbfs.c @@ -27,7 +27,8 @@
size_t data_offset; cb_err_t err = CB_CBFS_CACHE_FULL; - if (!CONFIG(NO_CBFS_MCACHE) && !ENV_SMM) + if (!CONFIG(NO_CBFS_MCACHE) && !ENV_SMM && + (ENV_ROMSTAGE_OR_BEFORE || cbmem_online())) err = cbfs_mcache_lookup(cbd->mcache, cbd->mcache_size, name, mdata, &data_offset); if (err == CB_CBFS_CACHE_FULL) {
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48429 )
Change subject: cbfs: Skip mcache in post-RAM stages before CBMEM is online ......................................................................
Patch Set 1:
(1 comment)
Something is still not right.
https://review.coreboot.org/c/coreboot/+/48429/1/src/lib/cbfs.c File src/lib/cbfs.c:
https://review.coreboot.org/c/coreboot/+/48429/1/src/lib/cbfs.c@454 PS1, Line 454: static struct cbfs_boot_device ro; If cbfs is accessed before mcache/cbmem is available, this function will always point real ro COREBOOT cbfs_boot_device instead of the mcache in cbmem.
-
Angel Pons (Code Review) -
Arthur Heymans (Code Review)
-
Furquan Shaikh (Code Review)
-
Julius Werner (Code Review)