Change in coreboot[master]: lib/fmap.c: Properly handle cache for !C_ENVIRONMENT_BOOTBLOCK
Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36908 )
Change subject: lib/fmap.c: Properly handle cache for !C_ENVIRONMENT_BOOTBLOCK ......................................................................
lib/fmap.c: Properly handle cache for !C_ENVIRONMENT_BOOTBLOCK
On platforms without C_ENVIRONMENT_BOOTBLOCK the first stage accessing FMAP is romstage. As on these platforms only romstage is expected to call setup_preram_cache() no further handling is needed.
Change-Id: I7d70585b0c076707e73e20c2ed3f11e4c9ffdf37 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/lib/fmap.c 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/36908/1
diff --git a/src/lib/fmap.c b/src/lib/fmap.c index 4b4179c..f0fae55 100644 --- a/src/lib/fmap.c +++ b/src/lib/fmap.c @@ -78,7 +78,7 @@ }
struct fmap *fmap = (struct fmap *)_fmap_cache; - if (!ENV_BOOTBLOCK) { + if (!ENV_BOOTBLOCK && CONFIG(C_ENVIRONMENT_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). */
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36908 )
Change subject: lib/fmap.c: Properly handle cache for !C_ENVIRONMENT_BOOTBLOCK ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36908/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36908/1//COMMIT_MSG@9 PS1, Line 9: first stage : accessing FMAP is romstage Why wouldn't it be accessed from bootblock? I don't understand the reasoning for this assertion.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36908 )
Change subject: lib/fmap.c: Properly handle cache for !C_ENVIRONMENT_BOOTBLOCK ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/36908/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36908/1//COMMIT_MSG@9 PS1, Line 9: first stage : accessing FMAP is romstage
Why wouldn't it be accessed from bootblock? I don't understand the reasoning for this assertion.
I didn't read 'without' properly in the first part of the sentence.
https://review.coreboot.org/c/coreboot/+/36908/1/src/lib/fmap.c File src/lib/fmap.c:
https://review.coreboot.org/c/coreboot/+/36908/1/src/lib/fmap.c@81 PS1, Line 81: if (!ENV_BOOTBLOCK && CONFIG(C_ENVIRONMENT_BOOTBLOCK)) { Looks like this will result in no signature checking for the !c_env_bootblock platforms post romstage. Do we need to expand this check to deal with both cases?
((CONFIG(C_ENVIRONMENT_BOOTBLOCK) && !ENV_BOOTBLOCK) || (!CONFIG(C_ENVIRONMENT_BOOTBLOCK) && !ENV_ROMSTAGE))
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36908 )
Change subject: lib/fmap.c: Properly handle cache for !C_ENVIRONMENT_BOOTBLOCK ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36908/1/src/lib/fmap.c File src/lib/fmap.c:
https://review.coreboot.org/c/coreboot/+/36908/1/src/lib/fmap.c@81 PS1, Line 81: if (!ENV_BOOTBLOCK && CONFIG(C_ENVIRONMENT_BOOTBLOCK)) {
Looks like this will result in no signature checking for the !c_env_bootblock platforms post romstage. Do we need to expand this check to deal with both cases?
((CONFIG(C_ENVIRONMENT_BOOTBLOCK) && !ENV_BOOTBLOCK) || (!CONFIG(C_ENVIRONMENT_BOOTBLOCK) && !ENV_ROMSTAGE))
This code returns on !ENV_ROMSTAGE_OR_BEFORE. Is there a use case I'm missing or do you prefer to make it more explicit here?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36908 )
Change subject: lib/fmap.c: Properly handle cache for !C_ENVIRONMENT_BOOTBLOCK ......................................................................
Patch Set 1:
(1 comment)
Thanks! There are so many different ways coreboot can run it's hard to keep track of them all.
https://review.coreboot.org/c/coreboot/+/36908/1/src/lib/fmap.c File src/lib/fmap.c:
https://review.coreboot.org/c/coreboot/+/36908/1/src/lib/fmap.c@81 PS1, Line 81: if (!ENV_BOOTBLOCK && CONFIG(C_ENVIRONMENT_BOOTBLOCK)) {
Looks like this will result in no signature checking for the !c_env_bootblock platforms post romstag […]
Yeah, I agree with Aaron, this needs to be written differently. Could be simplified to
if (!ENV_BOOTBLOCK && !(CONFIG(C_ENVIRONMENT_BOOTBLOCK) && ENV_ROMSTAGE))
Maybe we should also consider adding a new helper rule like ENV_FIRST_STAGE for this? (I guess the romstage isn't literally the first stage for those devices, but it's the first stage common code will see.)
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36908 )
Change subject: lib/fmap.c: Properly handle cache for !C_ENVIRONMENT_BOOTBLOCK ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36908/1/src/lib/fmap.c File src/lib/fmap.c:
https://review.coreboot.org/c/coreboot/+/36908/1/src/lib/fmap.c@81 PS1, Line 81: if (!ENV_BOOTBLOCK && CONFIG(C_ENVIRONMENT_BOOTBLOCK)) {
This code returns on !ENV_ROMSTAGE_OR_BEFORE. Is there a use case I'm missing or do you prefer to make it more explicit here?
I guess technically all the cases would be covered, but I think the other version looks cleaner. Essentially this code is asking "if (!(this is the first stage we're executing))".
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36908 )
Change subject: lib/fmap.c: Properly handle cache for !C_ENVIRONMENT_BOOTBLOCK ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36908/1/src/lib/fmap.c File src/lib/fmap.c:
https://review.coreboot.org/c/coreboot/+/36908/1/src/lib/fmap.c@81 PS1, Line 81: if (!ENV_BOOTBLOCK && CONFIG(C_ENVIRONMENT_BOOTBLOCK)) {
This code returns on !ENV_ROMSTAGE_OR_BEFORE. […]
I missed that this function was bailing out on !ENV_ROMSTAGE_OR_BEFORE. I think the original one will work and handle the SEPARATE_VERSTAGE case as well. I do think that a comment will be helpful providing more clarity in explaining the situation it's trying to handle.
Mike Banon has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36908 )
Change subject: lib/fmap.c: Properly handle cache for !C_ENVIRONMENT_BOOTBLOCK ......................................................................
Patch Set 1: Code-Review+1
I am one inch from giving this +2, but perhaps the comments below need to be addressed.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36908 )
Change subject: lib/fmap.c: Properly handle cache for !C_ENVIRONMENT_BOOTBLOCK ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36908/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36908/1//COMMIT_MSG@9 PS1, Line 9: first stage : accessing FMAP is romstage
I didn't read 'without' properly in the first part of the sentence.
A comma after C_ENVIRONMENT_BOOTBLOCK would have helped, I guess.
Hello Aaron Durbin, Julius Werner, Mike Banon, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36908
to look at the new patch set (#2).
Change subject: lib/fmap.c: Properly handle cache for !C_ENVIRONMENT_BOOTBLOCK ......................................................................
lib/fmap.c: Properly handle cache for !C_ENVIRONMENT_BOOTBLOCK
On platforms without C_ENVIRONMENT_BOOTBLOCK the first stage accessing FMAP is romstage.
Change-Id: I7d70585b0c076707e73e20c2ed3f11e4c9ffdf37 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/lib/fmap.c 1 file changed, 10 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/36908/2
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36908 )
Change subject: lib/fmap.c: Properly handle cache for !C_ENVIRONMENT_BOOTBLOCK ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36908/2/src/lib/fmap.c File src/lib/fmap.c:
https://review.coreboot.org/c/coreboot/+/36908/2/src/lib/fmap.c@65 PS2, Line 65: #endif No, sorry, this shouldn't be in here. If we want to do this, it should be in rules.h. (I'd also consider calling it ENV_FIRST_C_STAGE instead.)
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36908 )
Change subject: lib/fmap.c: Properly handle cache for !C_ENVIRONMENT_BOOTBLOCK ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36908/2/src/lib/fmap.c File src/lib/fmap.c:
https://review.coreboot.org/c/coreboot/+/36908/2/src/lib/fmap.c@65 PS2, Line 65: #endif
No, sorry, this shouldn't be in here. If we want to do this, it should be in rules.h. (I'd also consider calling it ENV_FIRST_C_STAGE instead.)
You are right. I just didn't want to add code there that's will likely be removed again in a week or two when romcc bootblocks are killed off.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36908 )
Change subject: lib/fmap.c: Properly handle cache for !C_ENVIRONMENT_BOOTBLOCK ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36908/2/src/lib/fmap.c File src/lib/fmap.c:
https://review.coreboot.org/c/coreboot/+/36908/2/src/lib/fmap.c@65 PS2, Line 65: #endif
No, sorry, this shouldn't be in here. If we want to do this, it should be in rules.h. […]
Okay, I mean... if this is going to become irrelevant in two weeks, do we still need this patch at all?
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36908 )
Change subject: lib/fmap.c: Properly handle cache for !C_ENVIRONMENT_BOOTBLOCK ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36908/2/src/lib/fmap.c File src/lib/fmap.c:
https://review.coreboot.org/c/coreboot/+/36908/2/src/lib/fmap.c@65 PS2, Line 65: #endif
Okay, I mean... if this is going to become irrelevant in two weeks, do we still need this patch at all?
Nope. I wasn't sure what the timeframe in which ROMCC will be deprecated, but it seems to be pretty fast.
Mike Banon has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36908 )
Change subject: lib/fmap.c: Properly handle cache for !C_ENVIRONMENT_BOOTBLOCK ......................................................................
Patch Set 2: Code-Review+2
I think it's good. Merge this if you would like.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36908 )
Change subject: lib/fmap.c: Properly handle cache for !C_ENVIRONMENT_BOOTBLOCK ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36908/2/src/lib/fmap.c File src/lib/fmap.c:
https://review.coreboot.org/c/coreboot/+/36908/2/src/lib/fmap.c@65 PS2, Line 65: #endif
Okay, I mean... […]
Ack, as this patch didn't make it into 4.11, it's more interesting for other potential branches. And even there, it depends on how maintainers want to handle things (e.g. if it will be a c-env-bootblock-free branch, it won't matter either).
Mike Banon has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36908 )
Change subject: lib/fmap.c: Properly handle cache for !C_ENVIRONMENT_BOOTBLOCK ......................................................................
Patch Set 2:
Bump
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36908 )
Change subject: lib/fmap.c: Properly handle cache for !C_ENVIRONMENT_BOOTBLOCK ......................................................................
Patch Set 2:
Bump
C_ENVIRONMENT_BOOTBLOCK has been deprecated so this patch should be abandoned, right?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36908 )
Change subject: lib/fmap.c: Properly handle cache for !C_ENVIRONMENT_BOOTBLOCK ......................................................................
Patch Set 2:
Patch Set 2:
Bump
C_ENVIRONMENT_BOOTBLOCK has been deprecated so this patch should be abandoned, right?
It was made the default, rather. So yes, this isn't needed anymore.
Arthur Heymans has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/36908 )
Change subject: lib/fmap.c: Properly handle cache for !C_ENVIRONMENT_BOOTBLOCK ......................................................................
Abandoned
-
Aaron Durbin (Code Review) -
Angel Pons (Code Review)
-
Arthur Heymans (Code Review)
-
Julius Werner (Code Review)
-
Mike Banon (Code Review)
-
Nico Huber (Code Review)