Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Julius Werner, Angel Pons, Aaron Durbin, Patrick Rudolph,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/48324
to review the following change.
Change subject: Revert "cbfs: Introduce cbfs_ro_map() and cbfs_ro_load()" ......................................................................
Revert "cbfs: Introduce cbfs_ro_map() and cbfs_ro_load()"
This reverts commit 9d0cc2aea918eced42dc3825c1ac94d0d4fbc380.
Reason for revert: This commit breaks ASUS AM1I-A board (AMD fam16h) - confirmed by commit dichotomy. Now, while booting, this board gets a "CBFS ERROR: CBFS mcache overflow!" - which happens regardless of CBFS_MCACHE_SIZE. Luckily it doesn't break Lenovo G505S or ASUS A88XM-E, which are AMD fam15h. Until we come up with a workaround, this commit should be reverted, as well as the others based on it if available.
Change-Id: I0e092c966da60deae21d19eb4053628541595bbb --- M src/drivers/pc80/rtc/option.c M src/include/cbfs.h M src/lib/cbfs.c M src/northbridge/intel/haswell/raminit.c M src/soc/intel/broadwell/raminit.c M src/soc/intel/quark/romstage/romstage.c 6 files changed, 93 insertions(+), 61 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/24/48324/1
diff --git a/src/drivers/pc80/rtc/option.c b/src/drivers/pc80/rtc/option.c index 409f0ef..bb9d29a 100644 --- a/src/drivers/pc80/rtc/option.c +++ b/src/drivers/pc80/rtc/option.c @@ -49,28 +49,36 @@ return CB_SUCCESS; }
-static struct cmos_option_table *get_cmos_layout(void) +static enum cb_err locate_cmos_layout(struct region_device *rdev) { - static struct cmos_option_table *ct = NULL; + uint32_t cbfs_type = CBFS_COMPONENT_CMOS_LAYOUT; + static struct cbfsf fh;
/* * In case VBOOT is enabled and this function is called from SMM, * we have multiple CMOS layout files and to locate them we'd need to * include VBOOT into SMM... * - * Support only one CMOS layout in the RO CBFS for now. + * Support only one CMOS layout in the 'COREBOOT' region for now. */ - if (!ct) - ct = cbfs_ro_map("cmos_layout.bin", NULL); - if (!ct) - printk(BIOS_ERR, "RTC: cmos_layout.bin could not be found. " - "Options are disabled\n"); - return ct; + if (!region_device_sz(&(fh.data))) { + if (cbfs_locate_file_in_region(&fh, "COREBOOT", "cmos_layout.bin", + &cbfs_type)) { + printk(BIOS_ERR, "RTC: cmos_layout.bin could not be found. " + "Options are disabled\n"); + return CB_CMOS_LAYOUT_NOT_FOUND; + } + } + + cbfs_file_data(rdev, &fh); + + return CB_SUCCESS; }
enum cb_err cmos_get_option(void *dest, const char *name) { struct cmos_option_table *ct; + struct region_device rdev; struct cmos_entries *ce; size_t namelen; int found = 0; @@ -78,9 +86,16 @@ /* Figure out how long name is */ namelen = strnlen(name, CMOS_MAX_NAME_LENGTH);
- ct = get_cmos_layout(); - if (!ct) + if (locate_cmos_layout(&rdev) != CB_SUCCESS) { return CB_CMOS_LAYOUT_NOT_FOUND; + } + ct = rdev_mmap_full(&rdev); + if (!ct) { + printk(BIOS_ERR, "RTC: cmos_layout.bin could not be mapped. " + "Options are disabled\n"); + + return CB_CMOS_LAYOUT_NOT_FOUND; + }
/* find the requested entry record */ ce = (struct cmos_entries *)((unsigned char *)ct + ct->header_length); @@ -93,15 +108,19 @@ } if (!found) { printk(BIOS_DEBUG, "No CMOS option '%s'.\n", name); + rdev_munmap(&rdev, ct); return CB_CMOS_OPTION_NOT_FOUND; }
- if (!cmos_checksum_valid(LB_CKS_RANGE_START, LB_CKS_RANGE_END, LB_CKS_LOC)) + if (!cmos_checksum_valid(LB_CKS_RANGE_START, LB_CKS_RANGE_END, LB_CKS_LOC)) { + rdev_munmap(&rdev, ct); return CB_CMOS_CHECKSUM_INVALID; - - if (get_cmos_value(ce->bit, ce->length, dest) != CB_SUCCESS) + } + if (get_cmos_value(ce->bit, ce->length, dest) != CB_SUCCESS) { + rdev_munmap(&rdev, ct); return CB_CMOS_ACCESS_ERROR; - + } + rdev_munmap(&rdev, ct); return CB_SUCCESS; }
@@ -149,6 +168,7 @@ enum cb_err cmos_set_option(const char *name, void *value) { struct cmos_option_table *ct; + struct region_device rdev; struct cmos_entries *ce; unsigned long length; size_t namelen; @@ -157,9 +177,16 @@ /* Figure out how long name is */ namelen = strnlen(name, CMOS_MAX_NAME_LENGTH);
- ct = get_cmos_layout(); - if (!ct) + if (locate_cmos_layout(&rdev) != CB_SUCCESS) { return CB_CMOS_LAYOUT_NOT_FOUND; + } + ct = rdev_mmap_full(&rdev); + if (!ct) { + printk(BIOS_ERR, "RTC: cmos_layout.bin could not be mapped. " + "Options are disabled\n"); + + return CB_CMOS_LAYOUT_NOT_FOUND; + }
/* find the requested entry record */ ce = (struct cmos_entries *)((unsigned char *)ct + ct->header_length); @@ -172,6 +199,7 @@ } if (!found) { printk(BIOS_DEBUG, "WARNING: No CMOS option '%s'.\n", name); + rdev_munmap(&rdev, ct); return CB_CMOS_OPTION_NOT_FOUND; }
@@ -180,13 +208,18 @@ length = MAX(strlen((const char *)value) * 8, ce->length - 8); /* make sure the string is null terminated */ if (set_cmos_value(ce->bit + ce->length - 8, 8, &(u8[]){0}) - != CB_SUCCESS) + != CB_SUCCESS) { + rdev_munmap(&rdev, ct); return CB_CMOS_ACCESS_ERROR; + } }
- if (set_cmos_value(ce->bit, length, value) != CB_SUCCESS) + if (set_cmos_value(ce->bit, length, value) != CB_SUCCESS) { + rdev_munmap(&rdev, ct); return CB_CMOS_ACCESS_ERROR; + }
+ rdev_munmap(&rdev, ct); return CB_SUCCESS; }
diff --git a/src/include/cbfs.h b/src/include/cbfs.h index 8d4c220..992b658 100644 --- a/src/include/cbfs.h +++ b/src/include/cbfs.h @@ -23,12 +23,9 @@ NOTE: Since this may return a direct pointer to memory-mapped hardware, compressed files are NOT transparently decompressed (unlike cbfs_load()). */ void *cbfs_map(const char *name, size_t *size_out); -/* Like cbfs_map(), except that it will always read from the read-only CBFS - ("COREBOOT" FMAP region), even when CONFIG(VBOOT) is enabled. */ -void *cbfs_ro_map(const char *name, size_t *size_out); -/* Removes a previously allocated CBFS mapping. Should try to unmap mappings in - strict LIFO order where possible, since mapping backends often don't support - more complicated cases. */ +/* Removes a mapping previously allocated with cbfs_map(). Should try to unmap + mappings in strict LIFO order where possible, since mapping backends often + don't support more complicated cases. */ int cbfs_unmap(void *mapping); /* Locate file in a specific region of fmap. Return 0 on success. < 0 on error*/ int cbfs_locate_file_in_region(struct cbfsf *fh, const char *region_name, @@ -37,9 +34,6 @@ success or 0 on error. File will get decompressed as necessary. Same decompression requirements as cbfs_load_and_decompress(). */ size_t cbfs_load(const char *name, void *buf, size_t buf_size); -/* Like cbfs_load(), except that it will always read from the read-only CBFS - ("COREBOOT" FMAP region), even when CONFIG(VBOOT) is enabled. */ -size_t cbfs_ro_load(const char *name, void *buf, size_t buf_size); /* Load |in_size| bytes from |rdev| at |offset| to the |buffer_size| bytes * large |buffer|, decompressing it according to |compression| in the process. * Returns the decompressed file size, or 0 on error. diff --git a/src/lib/cbfs.c b/src/lib/cbfs.c index beab74e..8d868a6 100644 --- a/src/lib/cbfs.c +++ b/src/lib/cbfs.c @@ -70,12 +70,12 @@ return 0; }
-static void *_cbfs_map(const char *name, size_t *size_out, bool force_ro) +void *cbfs_map(const char *name, size_t *size_out) { struct region_device rdev; union cbfs_mdata mdata;
- if (cbfs_boot_lookup(name, force_ro, &mdata, &rdev)) + if (cbfs_boot_lookup(name, false, &mdata, &rdev)) return NULL;
if (size_out != NULL) @@ -84,16 +84,6 @@ return rdev_mmap_full(&rdev); }
-void *cbfs_map(const char *name, size_t *size_out) -{ - return _cbfs_map(name, size_out, false); -} - -void *cbfs_ro_map(const char *name, size_t *size_out) -{ - return _cbfs_map(name, size_out, true); -} - int cbfs_unmap(void *mapping) { /* This works because munmap() only works on the root rdev and never @@ -291,13 +281,12 @@ return cbfs_map(name, NULL); }
-static size_t _cbfs_load(const char *name, void *buf, size_t buf_size, - bool force_ro) +size_t cbfs_load(const char *name, void *buf, size_t buf_size) { struct region_device rdev; union cbfs_mdata mdata;
- if (cbfs_boot_lookup(name, force_ro, &mdata, &rdev)) + if (cbfs_boot_lookup(name, false, &mdata, &rdev)) return 0;
uint32_t compression = CBFS_COMPRESS_NONE; @@ -313,16 +302,6 @@ buf, buf_size, compression); }
-size_t cbfs_load(const char *name, void *buf, size_t buf_size) -{ - return _cbfs_load(name, buf, buf_size, false); -} - -size_t cbfs_ro_load(const char *name, void *buf, size_t buf_size) -{ - return _cbfs_load(name, buf, buf_size, true); -} - int cbfs_prog_stage_load(struct prog *pstage) { struct cbfs_stage stage; diff --git a/src/northbridge/intel/haswell/raminit.c b/src/northbridge/intel/haswell/raminit.c index 9617ffb..290e402 100644 --- a/src/northbridge/intel/haswell/raminit.c +++ b/src/northbridge/intel/haswell/raminit.c @@ -107,6 +107,9 @@ { int (*entry)(struct pei_data *pei_data) __attribute__((regparm(1)));
+ uint32_t type = CBFS_TYPE_MRC; + struct cbfsf f; + printk(BIOS_DEBUG, "Starting UEFI PEI System Agent\n");
/* @@ -127,10 +130,13 @@
/* * Locate and call UEFI System Agent binary. The binary needs to be at a fixed offset - * in the flash and can therefore only reside in the COREBOOT fmap region. We don't care - * about leaking the mapping. + * in the flash and can therefore only reside in the COREBOOT fmap region. */ - entry = cbfs_ro_map("mrc.bin", NULL); + if (cbfs_locate_file_in_region(&f, "COREBOOT", "mrc.bin", &type) < 0) + die("mrc.bin not found!"); + + /* We don't care about leaking the mapping */ + entry = rdev_mmap_full(&f.data); if (entry) { int rv = entry(pei_data);
diff --git a/src/soc/intel/broadwell/raminit.c b/src/soc/intel/broadwell/raminit.c index 44a8937..e51b4f7 100644 --- a/src/soc/intel/broadwell/raminit.c +++ b/src/soc/intel/broadwell/raminit.c @@ -80,6 +80,8 @@ struct memory_info *mem_info; pei_wrapper_entry_t entry; int ret; + struct cbfsf f; + uint32_t type = CBFS_TYPE_MRC;
broadwell_fill_pei_data(pei_data);
@@ -112,10 +114,15 @@ pei_data->saved_data_size = 0; }
- /* We don't care about leaking the mapping */ - entry = cbfs_ro_map("mrc.bin", NULL); - if (entry == NULL) + /* Determine if mrc.bin is in the cbfs. */ + if (cbfs_locate_file_in_region(&f, "COREBOOT", "mrc.bin", &type) < 0) die("mrc.bin not found!"); + /* We don't care about leaking the mapping */ + entry = (pei_wrapper_entry_t)rdev_mmap_full(&f.data); + if (entry == NULL) { + printk(BIOS_DEBUG, "Couldn't find mrc.bin\n"); + return; + }
printk(BIOS_DEBUG, "Starting Memory Reference Code\n");
diff --git a/src/soc/intel/quark/romstage/romstage.c b/src/soc/intel/quark/romstage/romstage.c index 23a551e..9800aa4 100644 --- a/src/soc/intel/quark/romstage/romstage.c +++ b/src/soc/intel/quark/romstage/romstage.c @@ -49,16 +49,29 @@
void *locate_rmu_file(size_t *rmu_file_len) { + struct cbfsf fh; size_t fsize; void *rmu_data; + uint32_t type;
/* Locate the rmu.bin file in the read-only region of the flash */ - rmu_data = cbfs_ro_map("rmu.bin", &fsize); - if (!rmu_data) + type = CBFS_TYPE_RAW; + if (cbfs_locate_file_in_region(&fh, "COREBOOT", "rmu.bin", &type)) return NULL;
+ /* Get the file size */ + fsize = region_device_sz(&fh.data); if (rmu_file_len != NULL) *rmu_file_len = fsize;
+ /* Get the data address */ + rmu_data = rdev_mmap(&fh.data, 0, fsize); + + /* Since the SPI flash is directly mapped into memory, we do not need + * the mapping provided by the rdev service. Unmap the file to prevent + * a memory leak. Return/leak the SPI flash address for the rmu.bin + * file data which will be directly accessed by FSP MemoryInit. + */ + rdev_munmap(&fh.data, rmu_data); return rmu_data; }