Mike Banon would like build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Julius Werner, Angel Pons, Aaron Durbin and Patrick Rudolph to review this change.

View Change

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 change 48324. To unsubscribe, or for help writing mail filters, visit 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