Change in coreboot[master]: lib/cbfs.c: Update the ro bootdevice in CBMEM init in romstage

[Arthur Heymans (Code Review)]

Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/48283 )

Change subject: lib/cbfs.c: Update the ro bootdevice in CBMEM init in romstage
......................................................................

lib/cbfs.c: Update the ro bootdevice in CBMEM init in romstage

Use the cbmem mcache in romstage when cbmem is online.

Change-Id: I8d05c38ad0a1dc4b275efc71b0db8b8a7e7c27ec
Signed-off-by: Arthur Heymans 
---
M src/lib/cbfs.c
1 file changed, 6 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/48283/1

diff --git a/src/lib/cbfs.c b/src/lib/cbfs.c
index 5df1d8b..646f912 100644
--- a/src/lib/cbfs.c
+++ b/src/lib/cbfs.c
@@ -439,9 +439,10 @@
    return err;
 }

+static struct cbfs_boot_device ro;
+
 const struct cbfs_boot_device *cbfs_get_boot_device(bool force_ro)
 {
-	static struct cbfs_boot_device ro;

/* Ensure we always init RO mcache, even if first file is from RW.
       Otherwise it may not be available when needed in later stages. */
@@ -485,12 +486,16 @@

size_t real_size = cbfs_mcache_real_size(cbd->mcache, cbd->mcache_size);
    void *cbmem_mcache = cbmem_add(cbmem_id, real_size);
+	printk(BIOS_DEBUG, "CBMEM_MCACHE 0x%08lx\n", (uintptr_t)cbmem_mcache);
    if (!cbmem_mcache) {
    	printk(BIOS_ERR, "ERROR: Cannot allocate CBMEM mcache %#x (%#zx bytes)!\n",
    	       cbmem_id, real_size);
    	return;
    }
    memcpy(cbmem_mcache, cbd->mcache, real_size);
+
+	ro.mcache = cbmem_mcache;
+	ro.mcache_size = real_size;
 }

static void cbfs_mcache_migrate(int unused)

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48283 )

Change subject: lib/cbfs.c: Update the ro bootdevice in CBMEM init in romstage
......................................................................

Patch Set 1: Code-Review-1

Actually for performance reasons, this is likely not a good idea since caching is possible not set up for cbmem in romstage.

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48283 )

Change subject: lib/cbfs.c: Update the ro bootdevice in CBMEM init in romstage
......................................................................

Patch Set 1:

(2 comments)

Does this solve any specific problem? I thought CAR stays valid until the end of romstage on all platforms these days? Or do we still use the CAR migration code in romstage somewhere (then this would probably need to be integrated in there somehow)?

https://review.coreboot.org/c/coreboot/+/48283/1/src/lib/cbfs.c 
File src/lib/cbfs.c:

https://review.coreboot.org/c/coreboot/+/48283/1/src/lib/cbfs.c@489 
PS1, Line 489: 	printk(BIOS_DEBUG, "CBMEM_MCACHE 0x%08lx\n", (uintptr_t)cbmem_mcache);
Does it make sense to land this line permanently? The mcache regions already get printed as part of the CBMEM table at the end of ramstage.

https://review.coreboot.org/c/coreboot/+/48283/1/src/lib/cbfs.c@498 
PS1, Line 498: 	ro.mcache_size = real_size;
With vboot this function is called for both RO and RW mcache so this should probably go in cbfs_mcache_migrate() instead, or it needs to be guarded with a cbmem_id check.

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48283 )

Change subject: lib/cbfs.c: Update the ro bootdevice in CBMEM init in romstage
......................................................................

Patch Set 1:
> Patch Set 1:
>
> (2 comments)
>
> Does this solve any specific problem? I thought CAR stays valid until the end of romstage on all platforms these days? Or do we still use the CAR migration code in romstage somewhere (then this would probably need to be integrated in there somehow)?

No we don't. I tried this when debugging what goes wrong on the xeon_sp/cpx. FSP-M is known to trash some of the CAR region, which could have been an issue.

I'll drop it.

[Arthur Heymans (Code Review)]

Arthur Heymans has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/48283 )

Change subject: lib/cbfs.c: Update the ro bootdevice in CBMEM init in romstage
......................................................................

Abandoned