Change in coreboot[master]: Revert "cbfs: Introduce cbfs_ro_map() and cbfs_ro_load()" - coreboot-gerrit

12 Dec 2020

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;
 }
-- 
To view, visit https://review.coreboot.org/c/coreboot/+/48324
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I0e092c966da60deae21d19eb4053628541595bbb
Gerrit-Change-Number: 48324
Gerrit-PatchSet: 1
Gerrit-Owner: Mike Banon mikebdp2@gmail.com
Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org
Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com
Gerrit-Reviewer: Furquan Shaikh furquan@google.com
Gerrit-Reviewer: Julius Werner jwerner@chromium.org
Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com
Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org
Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org
Gerrit-CC: 9elements QA hardwaretestrobot@gmail.com
Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net
Gerrit-MessageType: newchange



    