Change in ...coreboot[master]: ifdtool: Enable GbE/PDR region access if they exist

List overview All Threads
Download

newer

older

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

Duncan Laurie (Code Review)

6 Jun 2019 6 Jun '19

10:51 p.m.

Duncan Laurie has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33273

Change subject: ifdtool: Enable GbE/PDR region access if they exist ......................................................................

ifdtool: Enable GbE/PDR region access if they exist

Instead of assuming GbE region is used and PDR region is not, check if there is a valid region defined in the descriptor and set the region access permissions based on that.

This enables the use of the PDR region on the sarien platform, which also uses the GbE region.

This results in the following example changes:

mb/google/sarien (GbE and PDR) . DESC BIOS ME GbE PDR EC -BIOS r rw rw r +BIOS r rw rw rw r

mb/google/eve: (no GbE, no PDR) . DESC BIOS ME GbE PDR EC -BIOS r rw rw +BIOS r rw

Change-Id: I7aeffc8f8194638c6012340b43aea8f8460d268a Signed-off-by: Duncan Laurie dlaurie@google.com --- M util/ifdtool/ifdtool.c M util/ifdtool/ifdtool.h 2 files changed, 40 insertions(+), 8 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/73/33273/1

diff --git a/util/ifdtool/ifdtool.c b/util/ifdtool/ifdtool.c index 6c5e784..0204a71 100644 --- a/util/ifdtool/ifdtool.c +++ b/util/ifdtool/ifdtool.c @@ -925,15 +925,28 @@ write_image(filename, image, size); }

+static int check_region(const frba_t *frba, unsigned int region_type) +{ + region_t region; + + if (!frba) + return 0; + + region = get_region(frba, region_type); + return !!((region.base < region.limit) && (region.size > 0)); +} + static void lock_descriptor(const char *filename, char *image, int size) { - int wr_shift, rd_shift; + int wr_shift, rd_shift, rw_pdr, rw_gbe; fmba_t *fmba = find_fmba(image, size); + const frba_t *frba = find_frba(image, size); if (!fmba) exit(EXIT_FAILURE); - /* TODO: Dynamically take Platform Data Region and GbE Region - * into regard. - */ + + /* Check for valid PDR and GBE region */ + rw_pdr = check_region(frba, REGION_PDR); + rw_gbe = check_region(frba, REGION_GBE);

if (ifd_version >= IFD_VERSION_2) { wr_shift = FLMSTR_WR_SHIFT_V2; @@ -969,10 +982,20 @@ case PLATFORM_CNL: case PLATFORM_ICL: case PLATFORM_SKLKBL: - /* 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; + /* CPU/BIOS can read descriptor, BIOS and EC */ + fmba->flmstr1 |= 0x103 << rd_shift; + /* CPU/BIOS can write BIOS. */ + fmba->flmstr1 |= 0x2 << wr_shift; + if (rw_gbe) { + /* Enable GBE RW if region is defined */ + fmba->flmstr1 |= (1 << REGION_GBE) << rd_shift; + fmba->flmstr1 |= (1 << REGION_GBE) << wr_shift; + } + if (rw_pdr) { + /* Enable PDR RW if region is defined */ + fmba->flmstr1 |= (1 << REGION_PDR) << rd_shift; + fmba->flmstr1 |= (1 << REGION_PDR) << wr_shift; + } /* ME can read descriptor, ME and GbE. */ fmba->flmstr2 |= 0xd << rd_shift; /* ME can write ME. */ diff --git a/util/ifdtool/ifdtool.h b/util/ifdtool/ifdtool.h index 49463b9..6617319 100644 --- a/util/ifdtool/ifdtool.h +++ b/util/ifdtool/ifdtool.h @@ -94,6 +94,15 @@ #define MAX_REGIONS 9 #define MAX_REGIONS_OLD 5

+enum flash_regions { + REGION_DESCRIPTOR, + REGION_BIOS, + REGION_ME, + REGION_GBE, + REGION_PDR, + REGION_EC = 8, +}; + typedef struct { uint32_t flreg[MAX_REGIONS]; } __attribute__((packed)) frba_t;

Attachments:

attachment.htm (text/html — 7.6 KB)

Show replies by date

Duncan Laurie (Code Review)

7 Jun 7 Jun

1:01 a.m.

Hello build bot (Jenkins), Lijian Zhao, Furquan Shaikh,

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

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

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

Change subject: ifdtool: Enable GbE/PDR region access if they exist ......................................................................

ifdtool: Enable GbE/PDR region access if they exist

Instead of assuming GbE region is used and PDR region is not, check if there is a valid region defined in the descriptor and set the region access permissions based on that.

This enables the use of the PDR region on the sarien platform, which also uses the GbE region.

This results in the following example changes:

mb/google/sarien (GbE and PDR) . DESC BIOS ME GbE PDR EC -BIOS r rw rw r +BIOS r rw rw rw r

mb/google/eve: (no GbE, no PDR) . DESC BIOS ME GbE PDR EC -BIOS r rw rw +BIOS r rw

BUG=b:134703987

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/73/33273/2

-- To view, visit https://review.coreboot.org/c/coreboot/+/33273 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I7aeffc8f8194638c6012340b43aea8f8460d268a Gerrit-Change-Number: 33273 Gerrit-PatchSet: 2 Gerrit-Owner: Duncan Laurie dlaurie@chromium.org Gerrit-Reviewer: Duncan Laurie dlaurie@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Lijian Zhao lijian.zhao@intel.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-MessageType: newpatchset

Furquan Shaikh (Code Review)

1:36 a.m.

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

Change subject: ifdtool: Enable GbE/PDR region access if they exist ......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/33273/2/util/ifdtool/ifdtool.c File util/ifdtool/ifdtool.c:

https://review.coreboot.org/#/c/33273/2/util/ifdtool/ifdtool.c@989 PS2, Line 989: rw_gbe Should we have this check for all masters? Also, is this check required for EC region too?

Duncan Laurie (Code Review)

1:50 a.m.

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

Change subject: ifdtool: Enable GbE/PDR region access if they exist ......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/33273/2/util/ifdtool/ifdtool.c File util/ifdtool/ifdtool.c:

https://review.coreboot.org/#/c/33273/2/util/ifdtool/ifdtool.c@989 PS2, Line 989: rw_gbe

...

Should we have this check for all masters? Also, is this check required for EC region too?

Most of the masters are not very flexible like the BIOS one is.

I could do it for EC region as well, I intended to do PDR only until I saw the comment about GbE at the top of the function. But probably the EC region wasn't in here when that was written or it would have said EC as well...

Duncan Laurie (Code Review)

6:36 p.m.

Hello build bot (Jenkins), Lijian Zhao, Furquan Shaikh,

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

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

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

Change subject: ifdtool: Enable GbE/PDR region access if they exist ......................................................................

ifdtool: Enable GbE/PDR region access if they exist

Instead of assuming GbE/PDR/EC regions may exist or not, check if there is a valid region defined in the descriptor and set the region access permissions based on that.

The net effect change is to enable the use of the PDR region on the sarien platform, which also uses the GbE and EC regions.

This results in the following example changes:

mb/google/sarien (GbE, PDR, EC) . DESC BIOS ME GbE PDR EC -BIOS r rw rw r ------------------------------- +BIOS r rw rw rw r

mb/google/eve: (no GbE, no PDR, no EC) . DESC BIOS ME GbE PDR EC -BIOS r rw rw r -ME r rw r -GbE r rw -EC r rw ------------------------------- +BIOS r rw +ME r rw +GbE +EC

BUG=b:134703987

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/73/33273/3

-- To view, visit https://review.coreboot.org/c/coreboot/+/33273 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I7aeffc8f8194638c6012340b43aea8f8460d268a Gerrit-Change-Number: 33273 Gerrit-PatchSet: 3 Gerrit-Owner: Duncan Laurie dlaurie@chromium.org Gerrit-Reviewer: Duncan Laurie dlaurie@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Lijian Zhao lijian.zhao@intel.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-MessageType: newpatchset

Duncan Laurie (Code Review)

6:42 p.m.

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

Change subject: ifdtool: Enable GbE/PDR region access if they exist ......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/#/c/33273/3/util/ifdtool/ifdtool.c File util/ifdtool/ifdtool.c:

https://review.coreboot.org/#/c/33273/3/util/ifdtool/ifdtool.c@999 PS3, Line 999: fmba->flmstr3 |= (1 << REGION_GBE) << rd_shift; : fmba->flmstr3 |= (1 << REGION_GBE) << wr_shift; I'm not sure these settings have much effect since the SPI programming guide says that GbE master always has RW access to GbE region, etc, but there are bits for it so we might as well program them as we expect them to be used.

-- To view, visit https://review.coreboot.org/c/coreboot/+/33273 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I7aeffc8f8194638c6012340b43aea8f8460d268a Gerrit-Change-Number: 33273 Gerrit-PatchSet: 3 Gerrit-Owner: Duncan Laurie dlaurie@chromium.org Gerrit-Reviewer: Duncan Laurie dlaurie@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Lijian Zhao lijian.zhao@intel.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Fri, 07 Jun 2019 16:42:10 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Duncan Laurie (Code Review)

9:11 p.m.

New subject: Change in ...coreboot[master]: ifdtool: Enable GbE/PDR/EC region access only if they exist

Hello build bot (Jenkins), Lijian Zhao, Furquan Shaikh,

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

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

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

Change subject: ifdtool: Enable GbE/PDR/EC region access only if they exist ......................................................................

ifdtool: Enable GbE/PDR/EC region access only if they exist

Instead of assuming GbE/PDR/EC regions may exist or not, check if there is a valid region defined in the descriptor and set the region access permissions based on that.

The net effect change is to enable the use of the PDR region on the sarien platform, which also uses the GbE and EC regions.

This results in the following example changes:

mb/google/sarien (GbE, PDR, EC) . DESC BIOS ME GbE PDR EC -BIOS r rw rw r ------------------------------- +BIOS r rw rw rw r

mb/google/eve: (no GbE, no PDR, no EC) . DESC BIOS ME GbE PDR EC -BIOS r rw rw r -ME r rw r -GbE r rw -EC r rw ------------------------------- +BIOS r rw +ME r rw +GbE +EC

BUG=b:134703987

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/73/33273/4

-- To view, visit https://review.coreboot.org/c/coreboot/+/33273 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I7aeffc8f8194638c6012340b43aea8f8460d268a Gerrit-Change-Number: 33273 Gerrit-PatchSet: 4 Gerrit-Owner: Duncan Laurie dlaurie@chromium.org Gerrit-Reviewer: Duncan Laurie dlaurie@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Lijian Zhao lijian.zhao@intel.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-MessageType: newpatchset

Furquan Shaikh (Code Review)

10:32 p.m.

New subject: Change in ...coreboot[master]: ifdtool: Enable GbE/PDR/EC region access only if they exist

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

Change subject: ifdtool: Enable GbE/PDR/EC region access only if they exist ......................................................................

Patch Set 4: Code-Review+2

-- To view, visit https://review.coreboot.org/c/coreboot/+/33273 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I7aeffc8f8194638c6012340b43aea8f8460d268a Gerrit-Change-Number: 33273 Gerrit-PatchSet: 4 Gerrit-Owner: Duncan Laurie dlaurie@chromium.org Gerrit-Reviewer: Duncan Laurie dlaurie@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Lijian Zhao lijian.zhao@intel.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Fri, 07 Jun 2019 20:32:10 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Duncan Laurie (Code Review)

10:51 p.m.

New subject: Change in ...coreboot[master]: ifdtool: Enable GbE/PDR/EC region access only if they exist

Duncan Laurie has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33273 )

Change subject: ifdtool: Enable GbE/PDR/EC region access only if they exist ......................................................................

ifdtool: Enable GbE/PDR/EC region access only if they exist

Instead of assuming GbE/PDR/EC regions may exist or not, check if there is a valid region defined in the descriptor and set the region access permissions based on that.

The net effect change is to enable the use of the PDR region on the sarien platform, which also uses the GbE and EC regions.

This results in the following example changes:

mb/google/sarien (GbE, PDR, EC) . DESC BIOS ME GbE PDR EC -BIOS r rw rw r ------------------------------- +BIOS r rw rw rw r

mb/google/eve: (no GbE, no PDR, no EC) . DESC BIOS ME GbE PDR EC -BIOS r rw rw r -ME r rw r -GbE r rw -EC r rw ------------------------------- +BIOS r rw +ME r rw +GbE +EC

BUG=b:134703987

Change-Id: I7aeffc8f8194638c6012340b43aea8f8460d268a Signed-off-by: Duncan Laurie dlaurie@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/33273 Reviewed-by: Furquan Shaikh furquan@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M util/ifdtool/ifdtool.c M util/ifdtool/ifdtool.h 2 files changed, 78 insertions(+), 30 deletions(-)

Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved

diff --git a/util/ifdtool/ifdtool.c b/util/ifdtool/ifdtool.c index 6c5e784..a1a327f 100644 --- a/util/ifdtool/ifdtool.c +++ b/util/ifdtool/ifdtool.c @@ -925,15 +925,24 @@ write_image(filename, image, size); }

+static int check_region(const frba_t *frba, unsigned int region_type) +{ + region_t region; + + if (!frba) + return 0; + + region = get_region(frba, region_type); + return !!((region.base < region.limit) && (region.size > 0)); +} + static void lock_descriptor(const char *filename, char *image, int size) { int wr_shift, rd_shift; fmba_t *fmba = find_fmba(image, size); + const frba_t *frba = find_frba(image, size); if (!fmba) exit(EXIT_FAILURE); - /* TODO: Dynamically take Platform Data Region and GbE Region - * into regard. - */

if (ifd_version >= IFD_VERSION_2) { wr_shift = FLMSTR_WR_SHIFT_V2; @@ -969,36 +978,66 @@ case PLATFORM_CNL: case PLATFORM_ICL: case PLATFORM_SKLKBL: - /* 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. */ - fmba->flmstr2 |= 0xd << rd_shift; + /* CPU/BIOS can read descriptor and BIOS. */ + fmba->flmstr1 |= (1 << REGION_DESC) << rd_shift; + fmba->flmstr1 |= (1 << REGION_BIOS) << rd_shift; + /* CPU/BIOS can write BIOS. */ + fmba->flmstr1 |= (1 << REGION_BIOS) << wr_shift; + /* ME can read descriptor and ME. */ + fmba->flmstr2 |= (1 << REGION_DESC) << rd_shift; + fmba->flmstr2 |= (1 << REGION_ME) << rd_shift; /* ME can write ME. */ - fmba->flmstr2 |= 0x4 << wr_shift; - /* GbE can read GbE and descriptor. */ - fmba->flmstr3 |= 0x9 << rd_shift; - /* GbE can write GbE. */ - fmba->flmstr3 |= 0x8 << wr_shift; - /* EC can read EC and descriptor. */ - fmba->flmstr5 |= 0x101 << rd_shift; - /* EC can write EC region. */ - fmba->flmstr5 |= 0x100 << wr_shift; + fmba->flmstr2 |= (1 << REGION_ME) << wr_shift; + if (check_region(frba, REGION_GBE)) { + /* BIOS can read/write GbE. */ + fmba->flmstr1 |= (1 << REGION_GBE) << rd_shift; + fmba->flmstr1 |= (1 << REGION_GBE) << wr_shift; + /* ME can read GbE. */ + fmba->flmstr2 |= (1 << REGION_GBE) << rd_shift; + /* GbE can read descriptor and read/write GbE.. */ + fmba->flmstr3 |= (1 << REGION_DESC) << rd_shift; + fmba->flmstr3 |= (1 << REGION_GBE) << rd_shift; + fmba->flmstr3 |= (1 << REGION_GBE) << wr_shift; + } + if (check_region(frba, REGION_PDR)) { + /* BIOS can read/write PDR. */ + fmba->flmstr1 |= (1 << REGION_PDR) << rd_shift; + fmba->flmstr1 |= (1 << REGION_PDR) << wr_shift; + } + if (check_region(frba, REGION_EC)) { + /* BIOS can read EC. */ + fmba->flmstr1 |= (1 << REGION_EC) << rd_shift; + /* EC can read descriptor and read/write EC. */ + fmba->flmstr5 |= (1 << REGION_DESC) << rd_shift; + fmba->flmstr5 |= (1 << REGION_EC) << rd_shift; + fmba->flmstr5 |= (1 << REGION_EC) << wr_shift; + } break; default: - /* CPU/BIOS can read descriptor, BIOS, and GbE. */ - fmba->flmstr1 |= 0xb << rd_shift; - /* CPU/BIOS can write BIOS and GbE. */ - fmba->flmstr1 |= 0xa << wr_shift; - /* ME can read descriptor, ME, and GbE. */ - fmba->flmstr2 |= 0xd << rd_shift; - /* ME can write ME and GbE. */ - fmba->flmstr2 |= 0xc << wr_shift; - /* GbE can write only GbE. */ - fmba->flmstr3 |= 0x8 << rd_shift; - /* GbE can read only GbE. */ - fmba->flmstr3 |= 0x8 << wr_shift; + /* CPU/BIOS can read descriptor and BIOS. */ + fmba->flmstr1 |= (1 << REGION_DESC) << rd_shift; + fmba->flmstr1 |= (1 << REGION_BIOS) << rd_shift; + /* CPU/BIOS can write BIOS. */ + fmba->flmstr1 |= (1 << REGION_BIOS) << wr_shift; + /* ME can read descriptor and ME. */ + fmba->flmstr2 |= (1 << REGION_DESC) << rd_shift; + fmba->flmstr2 |= (1 << REGION_ME) << rd_shift; + /* ME can write ME. */ + fmba->flmstr2 |= (1 << REGION_ME) << wr_shift; + if (check_region(frba, REGION_GBE)) { + /* BIOS can read GbE. */ + fmba->flmstr1 |= (1 << REGION_GBE) << rd_shift; + /* BIOS can write GbE. */ + fmba->flmstr1 |= (1 << REGION_GBE) << wr_shift; + /* ME can read GbE. */ + fmba->flmstr2 |= (1 << REGION_GBE) << rd_shift; + /* ME can write GbE. */ + fmba->flmstr2 |= (1 << REGION_GBE) << wr_shift; + /* GbE can write GbE. */ + fmba->flmstr3 |= (1 << REGION_GBE) << rd_shift; + /* GbE can read GbE. */ + fmba->flmstr3 |= (1 << REGION_GBE) << wr_shift; + } break; }

diff --git a/util/ifdtool/ifdtool.h b/util/ifdtool/ifdtool.h index 49463b9..f3b9a53 100644 --- a/util/ifdtool/ifdtool.h +++ b/util/ifdtool/ifdtool.h @@ -94,6 +94,15 @@ #define MAX_REGIONS 9 #define MAX_REGIONS_OLD 5

+enum flash_regions { + REGION_DESC, + REGION_BIOS, + REGION_ME, + REGION_GBE, + REGION_PDR, + REGION_EC = 8, +}; + typedef struct { uint32_t flreg[MAX_REGIONS]; } __attribute__((packed)) frba_t;

-- To view, visit https://review.coreboot.org/c/coreboot/+/33273 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I7aeffc8f8194638c6012340b43aea8f8460d268a Gerrit-Change-Number: 33273 Gerrit-PatchSet: 5 Gerrit-Owner: Duncan Laurie dlaurie@chromium.org Gerrit-Reviewer: Duncan Laurie dlaurie@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Lijian Zhao lijian.zhao@intel.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-MessageType: merged

2153

days inactive

2154

days old

coreboot-gerrit@coreboot.org

8 comments

2 participants

tags (0)

participants (2)

Duncan Laurie (Code Review)
Furquan Shaikh (Code Review)