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;