Change in coreboot[master]: security/vboot/Makefile.inc: Update regions-for-file function

List overview All Threads
Download

newer

older

Change in coreboot[master]:...

Change in coreboot[master]: lib:...

Sridhar Siricilla (Code Review)

23 Jul 2020 23 Jul '20

7:54 p.m.

Sridhar Siricilla has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/43766 )

Change subject: security/vboot/Makefile.inc: Update regions-for-file function ......................................................................

security/vboot/Makefile.inc: Update regions-for-file function

This patch updates regions-for-file function in the security/vboot/Makefile.inc to support addition of a CBFS file into required FMAP REGIONs in a flexible manner. The file that needs to be added to specific REGIONs, those regions list should be specified in the regions-for-file-{CBFS_FILE_TO_BE_ADDED} variable. For example, if a file foo.bin needs to be added in FW_MAIN_B and COREBOOT, then below code needs to be added in a Makefile.inc. regions-for-file-foo := FW_MAIN_B,COREBOOT cbfs-file-y := foo foo-file := foo.bin foo-type := raw

TEST=Verified on hatch

Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com Change-Id: I1f5c22b3d9558ee3c5daa2781a115964f8d2d83b --- M src/security/vboot/Makefile.inc 1 file changed, 3 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/43766/1

diff --git a/src/security/vboot/Makefile.inc b/src/security/vboot/Makefile.inc index 90b2756..fe8e90b 100644 --- a/src/security/vboot/Makefile.inc +++ b/src/security/vboot/Makefile.inc @@ -171,6 +171,8 @@ # All other files will be installed into RO and RW regions # Use $(sort) to cut down on extra spaces that would be translated to commas regions-for-file = $(subst $(spc),$(comma),$(sort \ + $(if $(value regions-for-file-$(1)), \ + $(regions-for-file-$(1)), \ $(if $(filter \ $(if $(filter y,$(CONFIG_VBOOT_STARTS_IN_ROMSTAGE)), \ %/romstage,) \ @@ -194,7 +196,7 @@ $(if $(filter \ $(call strip_quotes,$(CONFIG_RW_REGION_ONLY)) \ ,$(1)), $(RW_PARTITIONS), $(VBOOT_PARTITIONS) ) \ - ))))) + ))))))

CONFIG_GBB_HWID := $(call strip_quotes,$(CONFIG_GBB_HWID)) CONFIG_GBB_BMPFV_FILE := $(call strip_quotes,$(CONFIG_GBB_BMPFV_FILE))

Attachments:

attachment.htm (text/html — 3.9 KB)

Show replies by date

Tim Wawrzynczak (Code Review)

23 Jul 23 Jul

8:55 p.m.

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43766 )

Change subject: security/vboot/Makefile.inc: Update regions-for-file function ......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/43766/1/src/security/vboot/Makefile... File src/security/vboot/Makefile.inc:

https://review.coreboot.org/c/coreboot/+/43766/1/src/security/vboot/Makefile... PS1, Line 176: $(if $(filter \ : $(if $(filter y,$(CONFIG_VBOOT_STARTS_IN_ROMSTAGE)), \ : %/romstage,) \ : mts \ : %/verstage \ : locales \ : locale_%.bin \ : font.bin \ : vbgfx.bin \ : rmu.bin \ : cmos_layout.bin \ : cmos.default \ : $(call strip_quotes,$(CONFIG_RO_REGION_ONLY)) \ : ,$(1)),COREBOOT,\ : $(if $(filter \ : $(call strip_quotes,$(CONFIG_RWA_REGION_ONLY)) \ : ,$(1)), FW_MAIN_A, \ : $(if $(filter \ : $(call strip_quotes,$(CONFIG_RWB_REGION_ONLY)) \ : ,$(1)), FW_MAIN_B, \ : $(if $(filter \ : $(call strip_quotes,$(CONFIG_RW_REGION_ONLY)) \ : ,$(1)), $(RW_PARTITIONS), $(VBOOT_PARTITIONS) ) \ sorry, my pseudopatch in the comment was cheating... these should all be indented another two levels, as this is the 'else' clause of line 174 (line 175 is the `then` clause)

-- To view, visit https://review.coreboot.org/c/coreboot/+/43766 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I1f5c22b3d9558ee3c5daa2781a115964f8d2d83b Gerrit-Change-Number: 43766 Gerrit-PatchSet: 1 Gerrit-Owner: Sridhar Siricilla sridhar.siricilla@intel.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Comment-Date: Thu, 23 Jul 2020 20:55:57 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Sridhar Siricilla (Code Review)

24 Jul 24 Jul

4:04 a.m.

Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Aaron Durbin,

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

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

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

Change subject: security/vboot/Makefile.inc: Update regions-for-file function ......................................................................

security/vboot/Makefile.inc: Update regions-for-file function

TEST=Verified on hatch

Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com Change-Id: I1f5c22b3d9558ee3c5daa2781a115964f8d2d83b --- M src/security/vboot/Makefile.inc 1 file changed, 26 insertions(+), 24 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/43766/2

-- To view, visit https://review.coreboot.org/c/coreboot/+/43766 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I1f5c22b3d9558ee3c5daa2781a115964f8d2d83b Gerrit-Change-Number: 43766 Gerrit-PatchSet: 2 Gerrit-Owner: Sridhar Siricilla sridhar.siricilla@intel.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-CC: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-MessageType: newpatchset

Sridhar Siricilla (Code Review)

4:04 a.m.

Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43766 )

Change subject: security/vboot/Makefile.inc: Update regions-for-file function ......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/43766/1/src/security/vboot/Makefile... File src/security/vboot/Makefile.inc:

...

sorry, my pseudopatch in the comment was cheating... […]

Ack

-- To view, visit https://review.coreboot.org/c/coreboot/+/43766 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I1f5c22b3d9558ee3c5daa2781a115964f8d2d83b Gerrit-Change-Number: 43766 Gerrit-PatchSet: 2 Gerrit-Owner: Sridhar Siricilla sridhar.siricilla@intel.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-CC: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Comment-Date: Fri, 24 Jul 2020 04:04:58 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-MessageType: comment

Tim Wawrzynczak (Code Review)

8:55 p.m.

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43766 )

Change subject: security/vboot/Makefile.inc: Update regions-for-file function ......................................................................

Patch Set 3:

(2 comments)

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

https://review.coreboot.org/c/coreboot/+/43766/3//COMMIT_MSG@7 PS3, Line 7: /Makefile.inc you don't need to mention the filename, directory is good enough

https://review.coreboot.org/c/coreboot/+/43766/3/src/security/vboot/Makefile... File src/security/vboot/Makefile.inc:

https://review.coreboot.org/c/coreboot/+/43766/3/src/security/vboot/Makefile... PS3, Line 176: $(if $(filter \ : $(if $(filter y,$(CONFIG_VBOOT_STARTS_IN_ROMSTAGE)), \ : %/romstage,) \ : mts \ : %/verstage \ : locales \ : locale_%.bin \ : font.bin \ : vbgfx.bin \ : rmu.bin \ : cmos_layout.bin \ : cmos.default \ : $(call strip_quotes,$(CONFIG_RO_REGION_ONLY)) \ : ,$(1)),COREBOOT,\ : $(if $(filter \ : $(call strip_quotes,$(CONFIG_RWA_REGION_ONLY)) \ : ,$(1)), FW_MAIN_A, \ : $(if $(filter \ : $(call strip_quotes,$(CONFIG_RWB_REGION_ONLY)) \ : ,$(1)), FW_MAIN_B, \ : $(if $(filter \ : $(call strip_quotes,$(CONFIG_RW_REGION_ONLY)) \ : ,$(1)), $(RW_PARTITIONS), $(VBOOT_PARTITIONS) ) \ : )))))) I think this can be shifted left one tab, the `$(if $(filter ` should line up with `$(regions-for-file-$(1)), `

-- To view, visit https://review.coreboot.org/c/coreboot/+/43766 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I1f5c22b3d9558ee3c5daa2781a115964f8d2d83b Gerrit-Change-Number: 43766 Gerrit-PatchSet: 3 Gerrit-Owner: Sridhar Siricilla sridhar.siricilla@intel.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-CC: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Comment-Date: Fri, 24 Jul 2020 20:55:24 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Sridhar Siricilla (Code Review)

25 Jul 25 Jul

8:38 a.m.

Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Aaron Durbin,

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

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

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

Change subject: security/vboot/Makefile.inc: Update regions-for-file function ......................................................................

security/vboot/Makefile.inc: Update regions-for-file function

TEST=Verified on hatch

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/43766/4

-- To view, visit https://review.coreboot.org/c/coreboot/+/43766 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I1f5c22b3d9558ee3c5daa2781a115964f8d2d83b Gerrit-Change-Number: 43766 Gerrit-PatchSet: 4 Gerrit-Owner: Sridhar Siricilla sridhar.siricilla@intel.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-CC: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-MessageType: newpatchset

Sridhar Siricilla (Code Review)

8:39 a.m.

Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43766 )

Change subject: security/vboot/Makefile.inc: Update regions-for-file function ......................................................................

Patch Set 3:

(2 comments)

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

https://review.coreboot.org/c/coreboot/+/43766/3//COMMIT_MSG@7 PS3, Line 7: /Makefile.inc

...

you don't need to mention the filename, directory is good enough

Having filename, indicates wherein the change is added. In this case, I'm ok for this.

https://review.coreboot.org/c/coreboot/+/43766/3/src/security/vboot/Makefile... File src/security/vboot/Makefile.inc:

...

I think this can be shifted left one tab, the `$(if $(filter ` should line up with `$(regions-for-f […]

Ack

-- To view, visit https://review.coreboot.org/c/coreboot/+/43766 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I1f5c22b3d9558ee3c5daa2781a115964f8d2d83b Gerrit-Change-Number: 43766 Gerrit-PatchSet: 3 Gerrit-Owner: Sridhar Siricilla sridhar.siricilla@intel.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-CC: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Comment-Date: Sat, 25 Jul 2020 08:39:34 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-MessageType: comment

Paul Menzel (Code Review)

5:26 p.m.

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43766 )

Change subject: security/vboot/Makefile.inc: Update regions-for-file function ......................................................................

Patch Set 4:

(2 comments)

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

https://review.coreboot.org/c/coreboot/+/43766/4//COMMIT_MSG@10 PS4, Line 10: to support addition of a CBFS file

...

… to support adding a CBFS file …

https://review.coreboot.org/c/coreboot/+/43766/4//COMMIT_MSG@14 PS4, Line 14: For example, if a file foo.bin needs to be added in FW_MAIN_B and COREBOOT, Please add a blank line above.

-- To view, visit https://review.coreboot.org/c/coreboot/+/43766 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I1f5c22b3d9558ee3c5daa2781a115964f8d2d83b Gerrit-Change-Number: 43766 Gerrit-PatchSet: 4 Gerrit-Owner: Sridhar Siricilla sridhar.siricilla@intel.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-CC: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Comment-Date: Sat, 25 Jul 2020 17:26:41 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Sridhar Siricilla (Code Review)

27 Jul 27 Jul

5:01 a.m.

Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Aaron Durbin,

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

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

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

Change subject: security/vboot/Makefile.inc: Update regions-for-file function ......................................................................

security/vboot/Makefile.inc: Update regions-for-file function

This patch updates regions-for-file function in the security/vboot/Makefile.inc to support adding a CBFS file into required FMAP REGIONs in a flexible manner. The file that needs to be added to specific REGIONs, those regions list should be specified in the regions-for-file-{CBFS_FILE_TO_BE_ADDED} variable.

For example, if a file foo.bin needs to be added in FW_MAIN_B and COREBOOT, then below code needs to be added in a Makefile.inc. regions-for-file-foo := FW_MAIN_B,COREBOOT cbfs-file-y := foo foo-file := foo.bin foo-type := raw

TEST=Verified on hatch

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/43766/5

-- To view, visit https://review.coreboot.org/c/coreboot/+/43766 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I1f5c22b3d9558ee3c5daa2781a115964f8d2d83b Gerrit-Change-Number: 43766 Gerrit-PatchSet: 5 Gerrit-Owner: Sridhar Siricilla sridhar.siricilla@intel.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-CC: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-MessageType: newpatchset

Sridhar Siricilla (Code Review)

5:09 a.m.

Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43766 )

Change subject: security/vboot/Makefile.inc: Update regions-for-file function ......................................................................

Patch Set 5:

(2 comments)

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

https://review.coreboot.org/c/coreboot/+/43766/4//COMMIT_MSG@10 PS4, Line 10: to support addition of a CBFS file

...

...
… to support adding a CBFS file …

Ack

https://review.coreboot.org/c/coreboot/+/43766/4//COMMIT_MSG@14 PS4, Line 14: For example, if a file foo.bin needs to be added in FW_MAIN_B and COREBOOT,

...

Please add a blank line above.

Ack

-- To view, visit https://review.coreboot.org/c/coreboot/+/43766 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I1f5c22b3d9558ee3c5daa2781a115964f8d2d83b Gerrit-Change-Number: 43766 Gerrit-PatchSet: 5 Gerrit-Owner: Sridhar Siricilla sridhar.siricilla@intel.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-CC: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Comment-Date: Mon, 27 Jul 2020 05:09:28 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: comment

Tim Wawrzynczak (Code Review)

7:21 p.m.

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43766 )

Change subject: security/vboot/Makefile.inc: Update regions-for-file function ......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/43766/5/src/security/vboot/Makefile... File src/security/vboot/Makefile.inc:

https://review.coreboot.org/c/coreboot/+/43766/5/src/security/vboot/Makefile... PS5, Line 174: $(if $(value regions-for-file-$(1)), \ Here's how I think the indentation should look here:

```C regions-for-file = $(subst $(spc),$(comma),$(sort \ $(if $(value regions-for-file-$(1)), \ $(regions-for-file-$(1)), $(if $(filter y,$(CONFIG_VBOOT_STARTS_IN_ROMSTAGE)), \ %/romstage,) \ mts \ %/verstage \ ... ```

The `$(regions-for-file-$(1)), ` is the "then" arm of the if, and the `$(if $(filter y...` is the 'else' arm of the if statement.

-- To view, visit https://review.coreboot.org/c/coreboot/+/43766 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I1f5c22b3d9558ee3c5daa2781a115964f8d2d83b Gerrit-Change-Number: 43766 Gerrit-PatchSet: 5 Gerrit-Owner: Sridhar Siricilla sridhar.siricilla@intel.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-CC: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Comment-Date: Mon, 27 Jul 2020 19:21:36 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Sridhar Siricilla (Code Review)

28 Jul 28 Jul

4:45 p.m.

Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43766 )

Change subject: security/vboot/Makefile.inc: Update regions-for-file function ......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/43766/5/src/security/vboot/Makefile... File src/security/vboot/Makefile.inc:

https://review.coreboot.org/c/coreboot/+/43766/5/src/security/vboot/Makefile... PS5, Line 174: $(if $(value regions-for-file-$(1)), \

...

Here's how I think the indentation should look here: […]

Do you mean as below? ........................................ regions-for-file = $(subst $(spc),$(comma),$(sort \ $(if $(value regions-for-file-$(1)), \ $(regions-for-file-$(1)), \ $(if $(filter $(if $(filter y,$(CONFIG_VBOOT_STARTS_IN_ROMSTAGE)), \ %/romstage,) \ mts \ %/verstage \ locales \ locale_%.bin \ font.bin \ vbgfx.bin \ rmu.bin \ ....................................

-- To view, visit https://review.coreboot.org/c/coreboot/+/43766 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I1f5c22b3d9558ee3c5daa2781a115964f8d2d83b Gerrit-Change-Number: 43766 Gerrit-PatchSet: 5 Gerrit-Owner: Sridhar Siricilla sridhar.siricilla@intel.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-CC: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Comment-Date: Tue, 28 Jul 2020 16:45:29 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-MessageType: comment

Sridhar Siricilla (Code Review)

5:47 p.m.

Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Aaron Durbin,

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

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

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

Change subject: security/vboot/Makefile.inc: Update regions-for-file function ......................................................................

security/vboot/Makefile.inc: Update regions-for-file function

TEST=Verified on hatch

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/43766/6

-- To view, visit https://review.coreboot.org/c/coreboot/+/43766 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I1f5c22b3d9558ee3c5daa2781a115964f8d2d83b Gerrit-Change-Number: 43766 Gerrit-PatchSet: 6 Gerrit-Owner: Sridhar Siricilla sridhar.siricilla@intel.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-CC: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-MessageType: newpatchset

Tim Wawrzynczak (Code Review)

10:06 p.m.

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43766 )

Change subject: security/vboot/Makefile.inc: Update regions-for-file function ......................................................................

Patch Set 6: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/43766/5/src/security/vboot/Makefile... File src/security/vboot/Makefile.inc:

https://review.coreboot.org/c/coreboot/+/43766/5/src/security/vboot/Makefile... PS5, Line 174: $(if $(value regions-for-file-$(1)), \

...

Do you mean as below? […]

This is what the logic looks like in pseudo-C:

``` if (regions-for-file-$(1)) { regions-for-file-$(1) } else { if (filter (if (filter y, $(CONFIG_VBOOT...), \ $/romstage,) \ }

```

which is why I think lines 175 and 176 should be indented the same amount. but I think the indentation that was already here is incorrect anyway.

-- To view, visit https://review.coreboot.org/c/coreboot/+/43766 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I1f5c22b3d9558ee3c5daa2781a115964f8d2d83b Gerrit-Change-Number: 43766 Gerrit-PatchSet: 6 Gerrit-Owner: Sridhar Siricilla sridhar.siricilla@intel.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Tue, 28 Jul 2020 22:06:25 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Tim Wawrzynczak twawrzynczak@chromium.org Comment-In-Reply-To: Sridhar Siricilla sridhar.siricilla@intel.com Gerrit-MessageType: comment

Sridhar Siricilla (Code Review)

11 Aug 11 Aug

9:06 a.m.

Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Tim Wawrzynczak, Aaron Durbin,

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

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

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

Change subject: security/vboot/Makefile.inc: Update regions-for-file function ......................................................................

security/vboot/Makefile.inc: Update regions-for-file function

TEST=Verified on hatch

Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com Change-Id: I1f5c22b3d9558ee3c5daa2781a115964f8d2d83b --- M src/security/vboot/Makefile.inc 1 file changed, 6 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/43766/10

-- To view, visit https://review.coreboot.org/c/coreboot/+/43766 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I1f5c22b3d9558ee3c5daa2781a115964f8d2d83b Gerrit-Change-Number: 43766 Gerrit-PatchSet: 10 Gerrit-Owner: Sridhar Siricilla sridhar.siricilla@intel.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: newpatchset

Sridhar Siricilla (Code Review)

8:48 p.m.

Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43766 )

Change subject: security/vboot/Makefile.inc: Update regions-for-file function ......................................................................

Patch Set 10:

(1 comment)

https://review.coreboot.org/c/coreboot/+/43766/5/src/security/vboot/Makefile... File src/security/vboot/Makefile.inc:

https://review.coreboot.org/c/coreboot/+/43766/5/src/security/vboot/Makefile... PS5, Line 174: $(if $(value regions-for-file-$(1)), \

...

This is what the logic looks like in pseudo-C: […]

done

-- To view, visit https://review.coreboot.org/c/coreboot/+/43766 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I1f5c22b3d9558ee3c5daa2781a115964f8d2d83b Gerrit-Change-Number: 43766 Gerrit-PatchSet: 10 Gerrit-Owner: Sridhar Siricilla sridhar.siricilla@intel.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Tue, 11 Aug 2020 20:48:20 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Tim Wawrzynczak twawrzynczak@chromium.org Comment-In-Reply-To: Sridhar Siricilla sridhar.siricilla@intel.com Gerrit-MessageType: comment

Tim Wawrzynczak (Code Review)

10:40 p.m.

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43766 )

Change subject: security/vboot/Makefile.inc: Update regions-for-file function ......................................................................

Patch Set 10: Code-Review+2

-- To view, visit https://review.coreboot.org/c/coreboot/+/43766 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I1f5c22b3d9558ee3c5daa2781a115964f8d2d83b Gerrit-Change-Number: 43766 Gerrit-PatchSet: 10 Gerrit-Owner: Sridhar Siricilla sridhar.siricilla@intel.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Tue, 11 Aug 2020 22:40:14 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Subrata Banik (Code Review)

13 Aug 13 Aug

5:43 a.m.

Subrata Banik has submitted this change. ( https://review.coreboot.org/c/coreboot/+/43766 )

Change subject: security/vboot/Makefile.inc: Update regions-for-file function ......................................................................

security/vboot/Makefile.inc: Update regions-for-file function

TEST=Verified on hatch

Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com Change-Id: I1f5c22b3d9558ee3c5daa2781a115964f8d2d83b Reviewed-on: https://review.coreboot.org/c/coreboot/+/43766 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/security/vboot/Makefile.inc 1 file changed, 6 insertions(+), 5 deletions(-)

Approvals: build bot (Jenkins): Verified Tim Wawrzynczak: Looks good to me, approved

diff --git a/src/security/vboot/Makefile.inc b/src/security/vboot/Makefile.inc index 90b2756..e92396d 100644 --- a/src/security/vboot/Makefile.inc +++ b/src/security/vboot/Makefile.inc @@ -171,8 +171,9 @@ # All other files will be installed into RO and RW regions # Use $(sort) to cut down on extra spaces that would be translated to commas regions-for-file = $(subst $(spc),$(comma),$(sort \ - $(if $(filter \ - $(if $(filter y,$(CONFIG_VBOOT_STARTS_IN_ROMSTAGE)), \ + $(if $(value regions-for-file-$(1)), \ + $(regions-for-file-$(1)), \ + $(if $(filter $(if $(filter y,$(CONFIG_VBOOT_STARTS_IN_ROMSTAGE)), \ %/romstage,) \ mts \ %/verstage \ @@ -186,15 +187,15 @@ $(call strip_quotes,$(CONFIG_RO_REGION_ONLY)) \ ,$(1)),COREBOOT,\ $(if $(filter \ - $(call strip_quotes,$(CONFIG_RWA_REGION_ONLY)) \ - ,$(1)), FW_MAIN_A, \ + $(call strip_quotes,$(CONFIG_RWA_REGION_ONLY)) \ + ,$(1)), FW_MAIN_A, \ $(if $(filter \ $(call strip_quotes,$(CONFIG_RWB_REGION_ONLY)) \ ,$(1)), FW_MAIN_B, \ $(if $(filter \ $(call strip_quotes,$(CONFIG_RW_REGION_ONLY)) \ ,$(1)), $(RW_PARTITIONS), $(VBOOT_PARTITIONS) ) \ - ))))) + ))))))

CONFIG_GBB_HWID := $(call strip_quotes,$(CONFIG_GBB_HWID)) CONFIG_GBB_BMPFV_FILE := $(call strip_quotes,$(CONFIG_GBB_BMPFV_FILE))

-- To view, visit https://review.coreboot.org/c/coreboot/+/43766 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I1f5c22b3d9558ee3c5daa2781a115964f8d2d83b Gerrit-Change-Number: 43766 Gerrit-PatchSet: 11 Gerrit-Owner: Sridhar Siricilla sridhar.siricilla@intel.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: Subrata Banik subrata.banik@intel.com Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: merged

1593

days inactive

1614

days old

coreboot-gerrit@coreboot.org

17 comments

4 participants

tags (0)

participants (4)

Paul Menzel (Code Review)
Sridhar Siricilla (Code Review)
Subrata Banik (Code Review)
Tim Wawrzynczak (Code Review)