Change in coreboot[4.11_branch]: lib: Fix FMAP cache on FSP1.0 platforms

[Andrey Petrov (Code Review)]

Andrey Petrov has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37214 )

Change subject: lib: Fix FMAP cache on FSP1.0 platforms
......................................................................

lib: Fix FMAP cache on FSP1.0 platforms

On FSP1.0 platforms CAR region is torn down by FspMemoryInit() on exit,
since there is no FspTempRamExit() API. Current FMAP cache code correctly
assumes CAR is around and usable after. However this assumption is only
correct for >= FSP1.0.

This change address the problem by using cache only if CAR region is
usable.

TEST=boot test on OCP monolake.

Change-Id: I9f64c26650fa4c397a0e4835e470ab41922e1290
---
M src/lib/fmap.c
1 file changed, 14 insertions(+), 7 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/37214/1

diff --git a/src/lib/fmap.c b/src/lib/fmap.c
index 48aab8f..b2dde6b 100644
--- a/src/lib/fmap.c
+++ b/src/lib/fmap.c
@@ -114,15 +114,22 @@
 {
    const struct region_device *boot;
    struct fmap *fmap;
-	struct mem_region_device *cache;
+	struct mem_region_device *cache = NULL;
    size_t offset = FMAP_OFFSET;

-	/* Try FMAP cache first */
-	cache = car_get_var_ptr(&fmap_cache);
-	if (!region_device_sz(&cache->rdev))
-		setup_preram_cache(cache);
-	if (region_device_sz(&cache->rdev))
-		return rdev_chain_full(fmrd, &cache->rdev);
+	/*
+	 * Try FMAP cache first.
+	 * On FSP 1.0 platforms make sure CAR is still around because
+	 * the contents are destroyed on FspMemoryInit() exit.
+	 */
+	if (!CONFIG(PLATFORM_USES_FSP1_0) ||
+	    (CONFIG(PLATFORM_USES_FSP1_0) && car_active())) {
+		cache = car_get_var_ptr(&fmap_cache);
+		if (!region_device_sz(&cache->rdev))
+			setup_preram_cache(cache);
+		if (region_device_sz(&cache->rdev))
+			return rdev_chain_full(fmrd, &cache->rdev);
+	}

boot_device_init();
    boot = boot_device_ro();

[Andrey Petrov (Code Review)]

Andrey Petrov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37214 )

Change subject: lib: Fix FMAP cache on FSP1.0 platforms
......................................................................

Patch Set 1:

while fixing this issue I learnt it has been already addressed

[Arthur Heymans (Code Review)]

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

Change subject: lib: Fix FMAP cache on FSP1.0 platforms
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/37214/1/src/lib/fmap.c 
File src/lib/fmap.c:

https://review.coreboot.org/c/coreboot/+/37214/1/src/lib/fmap.c@123 
PS1, Line 123: destroyed on FspMemoryInit() exit.
That is not true. They are relocated. You probably should check the return value of car_get_var_ptr() for NULL.

[Andrey Petrov (Code Review)]

Andrey Petrov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37214 )

Change subject: lib: Fix FMAP cache on FSP1.0 platforms
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/37214/1/src/lib/fmap.c 
File src/lib/fmap.c:

https://review.coreboot.org/c/coreboot/+/37214/1/src/lib/fmap.c@123 
PS1, Line 123: destroyed on FspMemoryInit() exit.
> That is not true. They are relocated. […]

perhaps wording is incorrect. yeah they are relocated, but CAR is torn down. We cache only mem_region_device, but the payload is located in CAR region that is not CAR variable on its own. So mem_region_device has an invalid wild pointer after fspmemoryinit

[Arthur Heymans (Code Review)]

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

Change subject: lib: Fix FMAP cache on FSP1.0 platforms
......................................................................

Patch Set 1: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/37214/1/src/lib/fmap.c 
File src/lib/fmap.c:

https://review.coreboot.org/c/coreboot/+/37214/1/src/lib/fmap.c@123 
PS1, Line 123: destroyed on FspMemoryInit() exit.
> perhaps wording is incorrect. yeah they are relocated, but CAR is torn down. We cache only mem_region_device, but the payload is located in CAR region that is not CAR variable on its own. So mem_region_device has an invalid wild pointer after fspmemoryinit

Ah I see. The mem rdev region is just not valid anymore. Thx for explaining.
What do you think about the following wording: "On FSP1.0 platforms the CAR content is destroyed and relocated. Instead of updating the memory region device holding the cache, work around this by reading FMAP from flash after FspMemoryInit()."

[Andrey Petrov (Code Review)]

Hello Julius Werner, Arthur Heymans, David Hendricks, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/37214

to look at the new patch set (#2).

Change subject: lib: Fix FMAP cache on FSP1.0 platforms
......................................................................

lib: Fix FMAP cache on FSP1.0 platforms

On FSP1.0 platforms CAR region is torn down by FspMemoryInit() on exit,
since there is no FspTempRamExit() API. Current FMAP cache code correctly
assumes CAR is around and usable after. However this assumption is only
correct for >= FSP1.0.

This change address the problem by using cache only if CAR region is
usable.

TEST=boot test on OCP monolake.

Change-Id: I9f64c26650fa4c397a0e4835e470ab41922e1290
Signed-off-by: Andrey Petrov 
---
M src/lib/fmap.c
1 file changed, 14 insertions(+), 7 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/37214/2

[Andrey Petrov (Code Review)]

Andrey Petrov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37214 )

Change subject: lib: Fix FMAP cache on FSP1.0 platforms
......................................................................

Patch Set 2:

(1 comment)

it would be great if @Julius could take a look at this because there may be some code paths that I am not aware of

https://review.coreboot.org/c/coreboot/+/37214/1/src/lib/fmap.c 
File src/lib/fmap.c:

https://review.coreboot.org/c/coreboot/+/37214/1/src/lib/fmap.c@123 
PS1, Line 123: destroyed on FspMemoryInit() exit.
> > perhaps wording is incorrect. yeah they are relocated, but CAR is torn down. […]

thanks, I updated the comment.

[Julius Werner (Code Review)]

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

Change subject: lib: Fix FMAP cache on FSP1.0 platforms
......................................................................

Patch Set 2:
> it would be great if @Julius could take a look at this because there may be some code paths that I am not aware of

Sorry, I'm a bit confused here... doesn't CB:36937 already fix this? Like you mentioned above, I thought the issue was already addressed so why do we need another patch?

[Andrey Petrov (Code Review)]

Andrey Petrov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37214 )

Change subject: lib: Fix FMAP cache on FSP1.0 platforms
......................................................................

Patch Set 2:
> Sorry, I'm a bit confused here... doesn't CB:36937 already fix this? Like you mentioned above, I thought the issue was already addressed so why do we need another patch?

yes it does. I just thought if somebody wants it back in, we could use this patch. Do you see any value? if you think it does not worth the trouble I am ok dropping this

[Julius Werner (Code Review)]

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

Change subject: lib: Fix FMAP cache on FSP1.0 platforms
......................................................................

Patch Set 2:
> yes it does. I just thought if somebody wants it back in, we could use this patch. Do you see any value? if you think it does not worth the trouble I am ok dropping this

You mean you want to use the FMAP cache on FSP1.0 (instead of disabling it)? You won't get that like this... you're checking car_active() which will always be true at the time where you'd need the pre-RAM cache. After car_active() is false, you're already using the other cache in CBMEM anyway.

If you really wanted to make it usable instead you could check stage macros to enable it only during ENV_BOOTBLOCK and ENV_VERSTAGE, but I don't think that's worth it. (Truth be told, I would expect the feature doesn't add much on Intel platforms in general since the SPI memory mapping hardware is probably already caching the area internally anyway. I mostly wrote it for Arm and as a pre-requisite that we'll eventually need for TOCTOU-safe verification.)

[Andrey Petrov (Code Review)]

Andrey Petrov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37214 )

Change subject: lib: Fix FMAP cache on FSP1.0 platforms
......................................................................

Patch Set 2:
> If you really wanted to make it usable instead you could check stage macros to enable it only during ENV_BOOTBLOCK and ENV_VERSTAGE, but I don't think that's worth it. (Truth be told, I would expect the feature doesn't add much on Intel platforms in general since the SPI memory mapping hardware is probably already caching the area internally anyway. I mostly wrote it for Arm and as a pre-requisite that we'll eventually need for TOCTOU-safe verification.)

yeah looks like car_active does not even work properly on fsp1.0. ok, killing this, thanks

[Andrey Petrov (Code Review)]

Andrey Petrov has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/37214 )

Change subject: lib: Fix FMAP cache on FSP1.0 platforms
......................................................................

Abandoned