Martin Roth has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/43488 )
Change subject: lib: Remove corrupted fmap cache error for psp_verstage ......................................................................
lib: Remove corrupted fmap cache error for psp_verstage
The assumption was that the fmap cache would be initialized in bootblock, otherwise an error is shown. This error is showing up in psp_verstage when the fmap cache is initialized there, so add an exception.
BUG=None TEST=Boot, see that error message is gone from psp_verstage
Signed-off-by: Martin Roth martinroth@chromium.org Change-Id: I142f2092ade7b4327780d423d121728bfbdab247 --- M src/lib/fmap.c 1 file changed, 2 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/43488/1
diff --git a/src/lib/fmap.c b/src/lib/fmap.c index 671a962..3c06b1a 100644 --- a/src/lib/fmap.c +++ b/src/lib/fmap.c @@ -56,7 +56,8 @@ }
struct fmap *fmap = (struct fmap *)_fmap_cache; - if (!ENV_BOOTBLOCK) { + if (!(ENV_BOOTBLOCK || + (ENV_SEPARATE_VERSTAGE && CONFIG(VBOOT_STARTS_BEFORE_BOOTBLOCK)))) { /* NOTE: This assumes that for all platforms running this code, the bootblock is the first stage and the bootblock will make at least one FMAP access (usually from finding CBFS). */
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43488 )
Change subject: lib: Remove corrupted fmap cache error for psp_verstage ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43488/1/src/lib/fmap.c File src/lib/fmap.c:
https://review.coreboot.org/c/coreboot/+/43488/1/src/lib/fmap.c@60 PS1, Line 60: (ENV_SEPARATE_VERSTAGE && CONFIG(VBOOT_STARTS_BEFORE_BOOTBLOCK)))) { What this line is really trying to say is "if we're not in the first stage that runs", which is a concept we need in multiple places. That used to always be the bootblock, but if that's now not true anymore I think we should separate it out in its own macro. How about adding an ENV_FIRST_STAGE or something like that to <rules.h> to encapsulate this?
Also, it should really be
#define ENV_FIRST_STAGE (CONFIG(VBOOT_STARTS_BEFORE_BOOTBLOCK ? ENV_SEPARATE_VERSTAGE : ENV_BOOTBLOCK)
right? You don't want this to match ENV_BOOTBLOCK on Zork because otherwise you're regenerating the cache from flash again after DRAM init. You should have passed it in from the PSP already.
https://review.coreboot.org/c/coreboot/+/43488/1/src/lib/fmap.c@63 PS1, Line 63: at least one FMAP access (usually from finding CBFS). */ This comment needs updating now.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43488 )
Change subject: lib: Remove corrupted fmap cache error for psp_verstage ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43488/1/src/lib/fmap.c File src/lib/fmap.c:
https://review.coreboot.org/c/coreboot/+/43488/1/src/lib/fmap.c@60 PS1, Line 60: (ENV_SEPARATE_VERSTAGE && CONFIG(VBOOT_STARTS_BEFORE_BOOTBLOCK)))) {
What this line is really trying to say is "if we're not in the first stage that runs", which is a co […]
No, we do still want it to run in bootblock on zork. It gets generated in the PSP's memory by verstage, but that gets wiped out - we don't (currently) transfer the fmap cache, just the workbuf. If you think there's enough time savings to make it worthwhile to transfer the fmap cache from verstage to the x86, we could probably do it, but that wasn't previously on our list of things to transfer.
Even if we do add it to the list of buffers to transfer, we can't guarantee that it will get transferred. The only thing we're guaranteed enough space to transfer is the BKB resized workbuf. In practice, on the current generation of chips, we'll always have enough room unless we're working with the Sensor Fusion Hub processor (MP2) where we store the transfer buffer. On future chips that space probably won't be available.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43488 )
Change subject: lib: Remove corrupted fmap cache error for psp_verstage ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43488/1/src/lib/fmap.c File src/lib/fmap.c:
https://review.coreboot.org/c/coreboot/+/43488/1/src/lib/fmap.c@60 PS1, Line 60: (ENV_SEPARATE_VERSTAGE && CONFIG(VBOOT_STARTS_BEFORE_BOOTBLOCK)))) {
No, we do still want it to run in bootblock on zork. […]
It's definitely beneficial to use the cache if possible. I'm a bit confused by the "can't guarantee" part... shouldn't you know at build time whether it will all fit or not? How could that change at runtime?
My main concern here is to find a way to model these things in a simple and platform-agnostic way for the common code. We have this "cache that is initialized in the first stage and expected to exist by later ones" concept in several places (and my CBFS verification stuff is going to add more of those). I think we need a consistent way to model it in a way where Zork can fit in, so ENV_FIRST_STAGE was one idea to do that. Let me know if you have a better one.
If on later AMD platforms you can have an FMAP cache on the PSP but don't have the space to pass it along, that's an even more complicated situation to try to model in a platform-agnostic way. I'd say let's cross that bridge when we get there and maybe hope it doesn't come to that (the FMAP should only be 2K or so after all, it sounds unlikely that you couldn't fit that... we still have the options of reducing vboot workbuffer requirements that we discussed earlier but didn't end up needing).
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43488 )
Change subject: lib: Remove corrupted fmap cache error for psp_verstage ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43488/1/src/lib/fmap.c File src/lib/fmap.c:
https://review.coreboot.org/c/coreboot/+/43488/1/src/lib/fmap.c@60 PS1, Line 60: (ENV_SEPARATE_VERSTAGE && CONFIG(VBOOT_STARTS_BEFORE_BOOTBLOCK)))) {
It's definitely beneficial to use the cache if possible. […]
We will know at build time whether or not the MP2 is being used. If it is, we can only pass the 8k workbuf. I expect that will be the case on future platforms as the MP2 memory is already used for S0I3 resume so won't be available for verstage.
What I meant by saying that we "can't guarantee" that the 32k we currently have is available is that we can have one build where it is available, and another build where it isn't. We get the available size at runtime though, not at build time.
I'll add the code to pass the fmap cache if memory is available. I do think that we should consider the PSP flow to be outside the coreboot flow for many things since it has its own memory space. Things like this buffer may need to be looked at as having 2 'first' stages, one for each different processor that coreboot is running on.
I'll see what I can do about making an appropriate ENV_FIRST_STAGE.
Hello build bot (Jenkins), Raul Rangel, Furquan Shaikh, Eric Peers, Aaron Durbin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43488
to look at the new patch set (#2).
Change subject: lib: Update fmap cache error for psp_verstage ......................................................................
lib: Update fmap cache error for psp_verstage
The assumption was that the fmap cache would be initialized in bootblock, otherwise an error is shown. This error is showing up in psp_verstage when the fmap cache is initialized there, so create a new ENV value for ENV_FIRST_STAGE.
BUG=None TEST=Boot, see that error message is gone from psp_verstage
Signed-off-by: Martin Roth martinroth@chromium.org Change-Id: I142f2092ade7b4327780d423d121728bfbdab247 --- M src/include/rules.h M src/lib/fmap.c 2 files changed, 9 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/43488/2
Hello build bot (Jenkins), Raul Rangel, Furquan Shaikh, Eric Peers, Aaron Durbin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43488
to look at the new patch set (#3).
Change subject: lib: Update fmap cache error for psp_verstage ......................................................................
lib: Update fmap cache error for psp_verstage
The assumption was that the fmap cache would be initialized in bootblock, otherwise an error is shown. This error is showing up in psp_verstage when the fmap cache is initialized there, so create a new ENV value for ENV_INITIAL_STAGE.
BUG=None TEST=Boot, see that error message is gone from psp_verstage
Signed-off-by: Martin Roth martinroth@chromium.org Change-Id: I142f2092ade7b4327780d423d121728bfbdab247 --- M src/include/rules.h M src/lib/fmap.c 2 files changed, 9 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/43488/3
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43488 )
Change subject: lib: Update fmap cache error for psp_verstage ......................................................................
Patch Set 3: Code-Review+2
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43488 )
Change subject: lib: Update fmap cache error for psp_verstage ......................................................................
Patch Set 3: Code-Review+2
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43488 )
Change subject: lib: Update fmap cache error for psp_verstage ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43488/1/src/lib/fmap.c File src/lib/fmap.c:
https://review.coreboot.org/c/coreboot/+/43488/1/src/lib/fmap.c@60 PS1, Line 60: (ENV_SEPARATE_VERSTAGE && CONFIG(VBOOT_STARTS_BEFORE_BOOTBLOCK)))) {
We will know at build time whether or not the MP2 is being used. […]
Done
https://review.coreboot.org/c/coreboot/+/43488/1/src/lib/fmap.c@63 PS1, Line 63: at least one FMAP access (usually from finding CBFS). */
This comment needs updating now.
Done
Aaron Durbin has submitted this change. ( https://review.coreboot.org/c/coreboot/+/43488 )
Change subject: lib: Update fmap cache error for psp_verstage ......................................................................
lib: Update fmap cache error for psp_verstage
The assumption was that the fmap cache would be initialized in bootblock, otherwise an error is shown. This error is showing up in psp_verstage when the fmap cache is initialized there, so create a new ENV value for ENV_INITIAL_STAGE.
BUG=None TEST=Boot, see that error message is gone from psp_verstage
Signed-off-by: Martin Roth martinroth@chromium.org Change-Id: I142f2092ade7b4327780d423d121728bfbdab247 Reviewed-on: https://review.coreboot.org/c/coreboot/+/43488 Reviewed-by: Julius Werner jwerner@chromium.org Reviewed-by: Raul Rangel rrangel@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/include/rules.h M src/lib/fmap.c 2 files changed, 9 insertions(+), 3 deletions(-)
Approvals: build bot (Jenkins): Verified Julius Werner: Looks good to me, approved Raul Rangel: Looks good to me, approved
diff --git a/src/include/rules.h b/src/include/rules.h index be72e9e..39836b1 100644 --- a/src/include/rules.h +++ b/src/include/rules.h @@ -274,6 +274,13 @@ #define ENV_USER_SPACE 0 #endif
+/* Define the first stage to run */ +#if CONFIG(VBOOT_STARTS_BEFORE_BOOTBLOCK) +#define ENV_INITIAL_STAGE ENV_SEPARATE_VERSTAGE +#else +#define ENV_INITIAL_STAGE ENV_BOOTBLOCK +#endif + /** * For pre-DRAM stages and post-CAR always build with simple device model, ie. * PCI, PNP and CPU functions operate without use of devicetree. The reason diff --git a/src/lib/fmap.c b/src/lib/fmap.c index 671a962..e1e6a57 100644 --- a/src/lib/fmap.c +++ b/src/lib/fmap.c @@ -56,9 +56,8 @@ }
struct fmap *fmap = (struct fmap *)_fmap_cache; - if (!ENV_BOOTBLOCK) { - /* NOTE: This assumes that for all platforms running this code, - the bootblock is the first stage and the bootblock will make + if (!(ENV_INITIAL_STAGE)) { + /* NOTE: This assumes that the first stage will make at least one FMAP access (usually from finding CBFS). */ if (!check_signature(fmap)) goto register_cache;
-
Aaron Durbin (Code Review) -
Julius Werner (Code Review)
-
Martin Roth (Code Review)
-
Raul Rangel (Code Review)