Change in coreboot[master]: lib: add ENV_SMM check to setup_preram_cache

List overview All Threads
Download

newer

older

Change in coreboot[master]:...

Josie Nordrum (Code Review)

10 Sep 2020 10 Sep '20

11:56 a.m.

Josie Nordrum has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/45274 )

Change subject: lib: add ENV_SMM check to setup_preram_cache ......................................................................

lib: add ENV_SMM check to setup_preram_cache

Add check in setup_preram_cache to return if ENV_SMM is true. This avoids false warning that post-RAM FMAP is accessed too early caused by ENV_ROMSTAGE_OR_BEFORE evaluation in SMI handler.

BUG=b:167321319 BRANCH=None TEST=None

Signed-Off-By: Josie Nordrum josienordrum@google.com Change-Id: I3a4c199c42ee556187d6c4277e8793a36e4d493b --- M src/lib/fmap.c 1 file changed, 3 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/45274/1

diff --git a/src/lib/fmap.c b/src/lib/fmap.c index e1e6a57..fc51122 100644 --- a/src/lib/fmap.c +++ b/src/lib/fmap.c @@ -46,6 +46,9 @@ if (CONFIG(NO_FMAP_CACHE)) return;

+ if (ENV_SMM) + return; + if (!ENV_ROMSTAGE_OR_BEFORE) { /* We get here if ramstage makes an FMAP access before calling cbmem_initialize(). We should avoid letting it come to that,

Attachments:

attachment.htm (text/html — 2.6 KB)

Show replies by date

Aaron Durbin (Code Review)

10 Sep 10 Sep

11:57 a.m.

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45274 )

Change subject: lib: add ENV_SMM check to setup_preram_cache ......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45274/1//COMMIT_MSG Commit Message:

https://review.coreboot.org/c/coreboot/+/45274/1//COMMIT_MSG@6 PS1, Line 6: : lib lib/fmap:

Josie Nordrum (Code Review)

11:58 a.m.

Josie Nordrum has uploaded a new patch set (#2). ( https://review.coreboot.org/c/coreboot/+/45274 )

Change subject: lib: add ENV_SMM check to setup_preram_cache ......................................................................

lib: add ENV_SMM check to setup_preram_cache

Add check in setup_preram_cache to return if ENV_SMM is true. This avoids false warning that post-RAM FMAP is accessed too early caused by ENV_ROMSTAGE_OR_BEFORE evaluation in SMI handler.

BUG=b:167321319 BRANCH=None TEST=None

Signed-Off-By: Josie Nordrum josienordrum@google.com Change-Id: I3a4c199c42ee556187d6c4277e8793a36e4d493b --- M src/lib/fmap.c 1 file changed, 3 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/45274/2

Josie Nordrum (Code Review)

11:59 a.m.

Josie Nordrum has uploaded a new patch set (#3). ( https://review.coreboot.org/c/coreboot/+/45274 )

Change subject: lib: add ENV_SMM check to setup_preram_cache ......................................................................

lib: add ENV_SMM check to setup_preram_cache

Add check in setup_preram_cache to return if ENV_SMM is true. This avoids false warning that post-RAM FMAP is accessed too early caused by ENV_ROMSTAGE_OR_BEFORE evaluation in SMI handler.

BUG=b:167321319 BRANCH=None TEST=None

Signed-Off-By: Josie Nordrum josienordrum@google.com Change-Id: I3a4c199c42ee556187d6c4277e8793a36e4d493b --- M src/lib/fmap.c 1 file changed, 4 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/45274/3

Furquan Shaikh (Code Review)

12:07 p.m.

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45274 )

Change subject: lib: add ENV_SMM check to setup_preram_cache ......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45274/3//COMMIT_MSG Commit Message:

https://review.coreboot.org/c/coreboot/+/45274/3//COMMIT_MSG@17 PS3, Line 17: Signed-Off-By Signed-off-by

-- To view, visit https://review.coreboot.org/c/coreboot/+/45274 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3a4c199c42ee556187d6c4277e8793a36e4d493b Gerrit-Change-Number: 45274 Gerrit-PatchSet: 3 Gerrit-Owner: Josie Nordrum josienordrum@google.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-Comment-Date: Thu, 10 Sep 2020 19:07:00 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Josie Nordrum (Code Review)

1:47 p.m.

New subject: Change in coreboot[master]: lib/fmap: add ENV_SMM check to setup_preram_cache

Hello build bot (Jenkins), Furquan Shaikh,

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

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

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

Change subject: lib/fmap: add ENV_SMM check to setup_preram_cache ......................................................................

lib/fmap: add ENV_SMM check to setup_preram_cache

Add check in setup_preram_cache to return if ENV_SMM is true. This avoids false warning that post-RAM FMAP is accessed too early caused by ENV_ROMSTAGE_OR_BEFORE evaluation in SMI handler.

BUG=b:167321319 BRANCH=None TEST=None

Signed-off-By: Josie Nordrum josienordrum@google.com Change-Id: I3a4c199c42ee556187d6c4277e8793a36e4d493b --- M src/lib/fmap.c 1 file changed, 4 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/45274/4

Josie Nordrum (Code Review)

1:48 p.m.

New subject: Change in coreboot[master]: lib/fmap: add ENV_SMM check to setup_preram_cache

Josie Nordrum has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45274 )

Change subject: lib/fmap: add ENV_SMM check to setup_preram_cache ......................................................................

Patch Set 4:

(2 comments)

https://review.coreboot.org/c/coreboot/+/45274/1//COMMIT_MSG Commit Message:

https://review.coreboot.org/c/coreboot/+/45274/1//COMMIT_MSG@6 PS1, Line 6: : lib

...

lib/fmap:

Done

https://review.coreboot.org/c/coreboot/+/45274/3//COMMIT_MSG Commit Message:

https://review.coreboot.org/c/coreboot/+/45274/3//COMMIT_MSG@17 PS3, Line 17: Signed-Off-By

...

Signed-off-by

Done

-- To view, visit https://review.coreboot.org/c/coreboot/+/45274 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3a4c199c42ee556187d6c4277e8793a36e4d493b Gerrit-Change-Number: 45274 Gerrit-PatchSet: 4 Gerrit-Owner: Josie Nordrum josienordrum@google.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-Comment-Date: Thu, 10 Sep 2020 20:48:06 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Furquan Shaikh furquan@google.com Comment-In-Reply-To: Aaron Durbin adurbin@chromium.org Gerrit-MessageType: comment

Aaron Durbin (Code Review)

2:18 p.m.

New subject: Change in coreboot[master]: lib/fmap: add ENV_SMM check to setup_preram_cache

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45274 )

Change subject: lib/fmap: add ENV_SMM check to setup_preram_cache ......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45274/4//COMMIT_MSG Commit Message:

https://review.coreboot.org/c/coreboot/+/45274/4//COMMIT_MSG@17 PS4, Line 17: B "No Signed-off-by line in commit message" -> lowercase 'b'

-- To view, visit https://review.coreboot.org/c/coreboot/+/45274 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3a4c199c42ee556187d6c4277e8793a36e4d493b Gerrit-Change-Number: 45274 Gerrit-PatchSet: 4 Gerrit-Owner: Josie Nordrum josienordrum@google.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-Comment-Date: Thu, 10 Sep 2020 21:18:00 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Josie Nordrum (Code Review)

11 Sep 11 Sep

4:17 p.m.

New subject: Change in coreboot[master]: lib/fmap: add ENV_SMM check to setup_preram_cache

Hello build bot (Jenkins), Furquan Shaikh,

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

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

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

Change subject: lib/fmap: add ENV_SMM check to setup_preram_cache ......................................................................

lib/fmap: add ENV_SMM check to setup_preram_cache

Add check in setup_preram_cache to return if ENV_SMM is true. This avoids false warning that post-RAM FMAP is accessed too early caused by ENV_ROMSTAGE_OR_BEFORE evaluation in SMI handler.

BUG=b:167321319 BRANCH=None TEST=None

Signed-off-by: Josie Nordrum josienordrum@google.com Change-Id: I3a4c199c42ee556187d6c4277e8793a36e4d493b --- M src/lib/fmap.c 1 file changed, 4 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/45274/5

Josie Nordrum (Code Review)

4:17 p.m.

New subject: Change in coreboot[master]: lib/fmap: add ENV_SMM check to setup_preram_cache

Josie Nordrum has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45274 )

Change subject: lib/fmap: add ENV_SMM check to setup_preram_cache ......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45274/4//COMMIT_MSG Commit Message:

https://review.coreboot.org/c/coreboot/+/45274/4//COMMIT_MSG@17 PS4, Line 17: B

...

"No Signed-off-by line in commit message" -> lowercase 'b'

Done

-- To view, visit https://review.coreboot.org/c/coreboot/+/45274 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3a4c199c42ee556187d6c4277e8793a36e4d493b Gerrit-Change-Number: 45274 Gerrit-PatchSet: 5 Gerrit-Owner: Josie Nordrum josienordrum@google.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-Comment-Date: Fri, 11 Sep 2020 23:17:50 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Aaron Durbin adurbin@chromium.org Gerrit-MessageType: comment

Arthur Heymans (Code Review)

12 Sep 12 Sep

2:57 a.m.

New subject: Change in coreboot[master]: lib/fmap: add ENV_SMM check to setup_preram_cache

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

Change subject: lib/fmap: add ENV_SMM check to setup_preram_cache ......................................................................

Patch Set 5: Code-Review+1

(1 comment)

Would it not be better if the SMM handler only used assets located in TSEG instead of things in flash (which may or may not be RO)?

https://review.coreboot.org/c/coreboot/+/45274/5/src/lib/fmap.c File src/lib/fmap.c:

https://review.coreboot.org/c/coreboot/+/45274/5/src/lib/fmap.c@50 PS5, Line 50: ENV_SMM Would defining a new symbol ENV_STAGE_NO_FMAP_CACHE be a good idea?

-- To view, visit https://review.coreboot.org/c/coreboot/+/45274 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3a4c199c42ee556187d6c4277e8793a36e4d493b Gerrit-Change-Number: 45274 Gerrit-PatchSet: 5 Gerrit-Owner: Josie Nordrum josienordrum@google.com Gerrit-Reviewer: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Sat, 12 Sep 2020 09:57:22 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Aaron Durbin (Code Review)

14 Sep 14 Sep

9:01 a.m.

New subject: Change in coreboot[master]: lib/fmap: add ENV_SMM check to setup_preram_cache

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45274 )

Change subject: lib/fmap: add ENV_SMM check to setup_preram_cache ......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45274/5/src/lib/fmap.c File src/lib/fmap.c:

https://review.coreboot.org/c/coreboot/+/45274/5/src/lib/fmap.c@50 PS5, Line 50: ENV_SMM

...

Would defining a new symbol ENV_STAGE_NO_FMAP_CACHE be a good idea?

Potentially, but it would only be for this case. The ROMSTAGE_OR_BEFORE check wants to spit out a warning. Let's see if such a thing becomes a necessity?

-- To view, visit https://review.coreboot.org/c/coreboot/+/45274 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3a4c199c42ee556187d6c4277e8793a36e4d493b Gerrit-Change-Number: 45274 Gerrit-PatchSet: 5 Gerrit-Owner: Josie Nordrum josienordrum@google.com Gerrit-Reviewer: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Mon, 14 Sep 2020 16:01:30 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Arthur Heymans arthur@aheymans.xyz Gerrit-MessageType: comment

Aaron Durbin (Code Review)

9:01 a.m.

New subject: Change in coreboot[master]: lib/fmap: add ENV_SMM check to setup_preram_cache

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45274 )

Change subject: lib/fmap: add ENV_SMM check to setup_preram_cache ......................................................................

Patch Set 5: Code-Review+2

-- To view, visit https://review.coreboot.org/c/coreboot/+/45274 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3a4c199c42ee556187d6c4277e8793a36e4d493b Gerrit-Change-Number: 45274 Gerrit-PatchSet: 5 Gerrit-Owner: Josie Nordrum josienordrum@google.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Mon, 14 Sep 2020 16:01:34 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Aaron Durbin (Code Review)

9:01 a.m.

New subject: Change in coreboot[master]: lib/fmap: add ENV_SMM check to setup_preram_cache

Aaron Durbin has submitted this change. ( https://review.coreboot.org/c/coreboot/+/45274 )

Change subject: lib/fmap: add ENV_SMM check to setup_preram_cache ......................................................................

lib/fmap: add ENV_SMM check to setup_preram_cache

Add check in setup_preram_cache to return if ENV_SMM is true. This avoids false warning that post-RAM FMAP is accessed too early caused by ENV_ROMSTAGE_OR_BEFORE evaluation in SMI handler.

BUG=b:167321319 BRANCH=None TEST=None

Signed-off-by: Josie Nordrum josienordrum@google.com Change-Id: I3a4c199c42ee556187d6c4277e8793a36e4d493b Reviewed-on: https://review.coreboot.org/c/coreboot/+/45274 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Arthur Heymans arthur@aheymans.xyz Reviewed-by: Aaron Durbin adurbin@chromium.org --- M src/lib/fmap.c 1 file changed, 4 insertions(+), 0 deletions(-)

Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved Arthur Heymans: Looks good to me, but someone else must approve

diff --git a/src/lib/fmap.c b/src/lib/fmap.c index e1e6a57..377123a 100644 --- a/src/lib/fmap.c +++ b/src/lib/fmap.c @@ -46,6 +46,10 @@ if (CONFIG(NO_FMAP_CACHE)) return;

+ /* No need to use FMAP cache in SMM */ + if (ENV_SMM) + return; + if (!ENV_ROMSTAGE_OR_BEFORE) { /* We get here if ramstage makes an FMAP access before calling cbmem_initialize(). We should avoid letting it come to that,

-- To view, visit https://review.coreboot.org/c/coreboot/+/45274 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3a4c199c42ee556187d6c4277e8793a36e4d493b Gerrit-Change-Number: 45274 Gerrit-PatchSet: 6 Gerrit-Owner: Josie Nordrum josienordrum@google.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: merged

9elements QA (Code Review)

10:23 a.m.

New subject: Change in coreboot[master]: lib/fmap: add ENV_SMM check to setup_preram_cache

9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45274 )

Change subject: lib/fmap: add ENV_SMM check to setup_preram_cache ......................................................................

Patch Set 6:

Automatic boot test returned (PASS/FAIL/TOTAL): 8/1/9 "QEMU x86 q35/ich9" (x86_32) using payload TianoCore : SUCCESS : https://lava.9esec.io/r/19281 "QEMU x86 q35/ich9" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/19280 "QEMU x86 i440fx/piix4" (x86_64) using payload SeaBIOS : FAIL : https://lava.9esec.io/r/19279 "QEMU x86 i440fx/piix4" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/19278 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/19277 "HP Z220 SFF Workstation" (x86_32) using payload LinuxBoot_BusyBox_kexec : SUCCESS : https://lava.9esec.io/r/19285 "HP Z220 SFF Workstation" (x86_32) using payload LinuxBoot_BusyBox_kexec : SUCCESS : https://lava.9esec.io/r/19284 "HP Compaq 8200 Elite SFF PC" (x86_32) using payload TianoCore : SUCCESS : https://lava.9esec.io/r/19283 "HP Compaq 8200 Elite SFF PC" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/19282

Please note: This test is under development and might not be accurate at all!

-- To view, visit https://review.coreboot.org/c/coreboot/+/45274 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3a4c199c42ee556187d6c4277e8793a36e4d493b Gerrit-Change-Number: 45274 Gerrit-PatchSet: 6 Gerrit-Owner: Josie Nordrum josienordrum@google.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: 9elements QA hardwaretestrobot@gmail.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Mon, 14 Sep 2020 17:23:24 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

1525

days inactive

1529

days old

coreboot-gerrit@coreboot.org

14 comments

5 participants

tags (0)

participants (5)

9elements QA (Code Review)
Aaron Durbin (Code Review)
Arthur Heymans (Code Review)
Furquan Shaikh (Code Review)
Josie Nordrum (Code Review)