Change in ...coreboot[master]: util/ifdtools: Make EC region readable to BIOS/CPU

List overview All Threads
Download

newer

older

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

Bora Guvendik (Code Review)

12 Feb 2019 12 Feb '19

9:21 p.m.

Bora Guvendik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31377

Change subject: util/ifdtools: Make EC region readable to BIOS/CPU ......................................................................

util/ifdtools: Make EC region readable to BIOS/CPU

Allow EC region to be readable by BIOS/CPU so that flashrom can read it.

BUG=b:123199222 TEST=Build coreboot with CONFIG_LOCK_MANAGEMENT_ENGINE set, run firmware_LockedME test.

Change-Id: I306c74a0893355e57632a22a712b1f4fdaa19306 Signed-off-by: Bora Guvendik bora.guvendik@intel.com --- M util/ifdtool/ifdtool.c 1 file changed, 2 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/31377/1

diff --git a/util/ifdtool/ifdtool.c b/util/ifdtool/ifdtool.c index 85623fd..a97f352 100644 --- a/util/ifdtool/ifdtool.c +++ b/util/ifdtool/ifdtool.c @@ -969,8 +969,8 @@ case PLATFORM_CNL: case PLATFORM_ICL: case PLATFORM_SKLKBL: - /* CPU/BIOS can read descriptor, BIOS and GbE. */ - fmba->flmstr1 |= 0xb << rd_shift; + /* CPU/BIOS can read descriptor, BIOS, EC and GbE. */ + fmba->flmstr1 |= 0x10b << rd_shift; /* CPU/BIOS can write BIOS and Gbe. */ fmba->flmstr1 |= 0xa << wr_shift; /* ME can read descriptor, ME and GbE. */

Attachments:

attachment.htm (text/html — 2.8 KB)

Show replies by date

Furquan Shaikh (Code Review)

12 Feb 12 Feb

9:37 p.m.

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

Change subject: util/ifdtools: Make EC region readable to BIOS/CPU ......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/31377/1/util/ifdtool/ifdtool.c File util/ifdtool/ifdtool.c:

https://review.coreboot.org/#/c/31377/1/util/ifdtool/ifdtool.c@973 PS1, Line 973: 10 Wondering if this should be behind a Kconfig option. I remember Duncan had mentioned this doesn't matter if there is no EC region -- so probably might be okay to just do it.

-- To view, visit https://review.coreboot.org/c/coreboot/+/31377 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I306c74a0893355e57632a22a712b1f4fdaa19306 Gerrit-Change-Number: 31377 Gerrit-PatchSet: 1 Gerrit-Owner: Bora Guvendik bora.guvendik@intel.com Gerrit-Reviewer: Bora Guvendik bora.guvendik@intel.com Gerrit-Reviewer: Duncan Laurie dlaurie@chromium.org Gerrit-Reviewer: Lijian Zhao lijian.zhao@intel.com Gerrit-Reviewer: Stefan Reinauer stefan.reinauer@coreboot.org Gerrit-Reviewer: Subrata Banik subrata.banik@intel.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-Comment-Date: Tue, 12 Feb 2019 21:37:41 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Duncan Laurie (Code Review)

9:49 p.m.

Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31377 )

Change subject: util/ifdtools: Make EC region readable to BIOS/CPU ......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/31377/1/util/ifdtool/ifdtool.c File util/ifdtool/ifdtool.c:

https://review.coreboot.org/#/c/31377/1/util/ifdtool/ifdtool.c@973 PS1, Line 973: 10

...

Wondering if this should be behind a Kconfig option. […]

It does seem harmless if there is no EC region. In general it would be nice if this tool was more flexible with the permission settings it applies..

It so happens I need the EC region readable for software sync so this is useful, but I could see a scenario where someone would not want the EC to be readable. (not that security by obscurity is going to stop anything)

-- To view, visit https://review.coreboot.org/c/coreboot/+/31377 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I306c74a0893355e57632a22a712b1f4fdaa19306 Gerrit-Change-Number: 31377 Gerrit-PatchSet: 1 Gerrit-Owner: Bora Guvendik bora.guvendik@intel.com Gerrit-Reviewer: Bora Guvendik bora.guvendik@intel.com Gerrit-Reviewer: Duncan Laurie dlaurie@chromium.org Gerrit-Reviewer: Lijian Zhao lijian.zhao@intel.com Gerrit-Reviewer: Stefan Reinauer stefan.reinauer@coreboot.org Gerrit-Reviewer: Subrata Banik subrata.banik@intel.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-Comment-Date: Tue, 12 Feb 2019 21:49:58 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Furquan Shaikh furquan@google.com Gerrit-MessageType: comment

Duncan Laurie (Code Review)

13 Feb 13 Feb

3:46 a.m.

Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31377 )

Change subject: util/ifdtools: Make EC region readable to BIOS/CPU ......................................................................

Patch Set 1: Code-Review+2

-- To view, visit https://review.coreboot.org/c/coreboot/+/31377 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I306c74a0893355e57632a22a712b1f4fdaa19306 Gerrit-Change-Number: 31377 Gerrit-PatchSet: 1 Gerrit-Owner: Bora Guvendik bora.guvendik@intel.com Gerrit-Reviewer: Bora Guvendik bora.guvendik@intel.com Gerrit-Reviewer: Duncan Laurie dlaurie@chromium.org Gerrit-Reviewer: Lijian Zhao lijian.zhao@intel.com Gerrit-Reviewer: Stefan Reinauer stefan.reinauer@coreboot.org Gerrit-Reviewer: Subrata Banik subrata.banik@intel.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-Comment-Date: Wed, 13 Feb 2019 03:46:47 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Stefan Reinauer (Code Review)

7:02 a.m.

Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31377 )

Change subject: util/ifdtools: Make EC region readable to BIOS/CPU ......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/31377/1/util/ifdtool/ifdtool.c File util/ifdtool/ifdtool.c:

https://review.coreboot.org/#/c/31377/1/util/ifdtool/ifdtool.c@973 PS1, Line 973: 10

...

It does seem harmless if there is no EC region. […]

Please don't add Kconfig conditional compilation to the tools needed to build coreboot. It's better to add a command line option that then will conditionally be called from coreboot (if we can't always enable this, as suggested)

-- To view, visit https://review.coreboot.org/c/coreboot/+/31377 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I306c74a0893355e57632a22a712b1f4fdaa19306 Gerrit-Change-Number: 31377 Gerrit-PatchSet: 1 Gerrit-Owner: Bora Guvendik bora.guvendik@intel.com Gerrit-Reviewer: Bora Guvendik bora.guvendik@intel.com Gerrit-Reviewer: Duncan Laurie dlaurie@chromium.org Gerrit-Reviewer: Lijian Zhao lijian.zhao@intel.com Gerrit-Reviewer: Stefan Reinauer stefan.reinauer@coreboot.org Gerrit-Reviewer: Subrata Banik subrata.banik@intel.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-Comment-Date: Wed, 13 Feb 2019 07:02:45 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Duncan Laurie dlaurie@chromium.org Comment-In-Reply-To: Furquan Shaikh furquan@google.com Gerrit-MessageType: comment

Furquan Shaikh (Code Review)

7:06 a.m.

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

Change subject: util/ifdtools: Make EC region readable to BIOS/CPU ......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/31377/1/util/ifdtool/ifdtool.c File util/ifdtool/ifdtool.c:

https://review.coreboot.org/#/c/31377/1/util/ifdtool/ifdtool.c@973 PS1, Line 973: 10

...

Please don't add Kconfig conditional compilation to the tools needed to build coreboot. […]

Yes, that was the idea behind Kconfig i.e. allow Kconfig to provide certain command line options to the tool and the Kconfig can be selected by mainboard as per its requirements.

-- To view, visit https://review.coreboot.org/c/coreboot/+/31377 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I306c74a0893355e57632a22a712b1f4fdaa19306 Gerrit-Change-Number: 31377 Gerrit-PatchSet: 1 Gerrit-Owner: Bora Guvendik bora.guvendik@intel.com Gerrit-Reviewer: Bora Guvendik bora.guvendik@intel.com Gerrit-Reviewer: Duncan Laurie dlaurie@chromium.org Gerrit-Reviewer: Lijian Zhao lijian.zhao@intel.com Gerrit-Reviewer: Stefan Reinauer stefan.reinauer@coreboot.org Gerrit-Reviewer: Subrata Banik subrata.banik@intel.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-Comment-Date: Wed, 13 Feb 2019 07:06:40 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Stefan Reinauer stefan.reinauer@coreboot.org Comment-In-Reply-To: Duncan Laurie dlaurie@chromium.org Comment-In-Reply-To: Furquan Shaikh furquan@google.com Gerrit-MessageType: comment

Patrick Georgi (Code Review)

14 Feb 14 Feb

7:53 a.m.

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31377 )

Change subject: util/ifdtools: Make EC region readable to BIOS/CPU ......................................................................

util/ifdtools: Make EC region readable to BIOS/CPU

Allow EC region to be readable by BIOS/CPU so that flashrom can read it.

BUG=b:123199222 TEST=Build coreboot with CONFIG_LOCK_MANAGEMENT_ENGINE set, run firmware_LockedME test.

Change-Id: I306c74a0893355e57632a22a712b1f4fdaa19306 Signed-off-by: Bora Guvendik bora.guvendik@intel.com Reviewed-on: https://review.coreboot.org/c/31377 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Duncan Laurie dlaurie@chromium.org --- M util/ifdtool/ifdtool.c 1 file changed, 2 insertions(+), 2 deletions(-)

Approvals: build bot (Jenkins): Verified Duncan Laurie: Looks good to me, approved

-- To view, visit https://review.coreboot.org/c/coreboot/+/31377 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I306c74a0893355e57632a22a712b1f4fdaa19306 Gerrit-Change-Number: 31377 Gerrit-PatchSet: 2 Gerrit-Owner: Bora Guvendik bora.guvendik@intel.com Gerrit-Reviewer: Bora Guvendik bora.guvendik@intel.com Gerrit-Reviewer: Duncan Laurie dlaurie@chromium.org Gerrit-Reviewer: Lijian Zhao lijian.zhao@intel.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: Stefan Reinauer stefan.reinauer@coreboot.org Gerrit-Reviewer: Subrata Banik subrata.banik@intel.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-MessageType: merged

2113

days inactive

2115

days old

coreboot-gerrit@coreboot.org

6 comments

5 participants

tags (0)

participants (5)

Bora Guvendik (Code Review)
Duncan Laurie (Code Review)
Furquan Shaikh (Code Review)
Patrick Georgi (Code Review)
Stefan Reinauer (Code Review)