Change in coreboot[master]: cbfs: Introduce cbfs_ro_map() and cbfs_ro_load()

[Julius Werner (Code Review)]

Hello Aaron Durbin,

I'd like you to do a code review. Please visit

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

to review the following change.

Change subject: cbfs: Introduce cbfs_ro_map() and cbfs_ro_load()
......................................................................

cbfs: Introduce cbfs_ro_map() and cbfs_ro_load()

This patch introduces two new CBFS API functions which are equivalent to
cbfs_map() and cbfs_load(), respectively, with the difference that they
always operate on the read-only CBFS region ("COREBOOT" FMAP section).
Use it to replace some of the simple cases that needed to use
cbfs_locate_file_in_region().

Signed-off-by: Julius Werner 
Change-Id: I9c55b022b6502a333a9805ab0e4891dd7b97ef7f
---
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/romstage/raminit.c
M src/soc/intel/quark/romstage/romstage.c
6 files changed, 60 insertions(+), 89 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/39306/1

diff --git a/src/drivers/pc80/rtc/option.c b/src/drivers/pc80/rtc/option.c
index 4789b15..1f9abb5 100644
--- a/src/drivers/pc80/rtc/option.c
+++ b/src/drivers/pc80/rtc/option.c
@@ -60,36 +60,28 @@
    return CB_SUCCESS;
 }

-static enum cb_err locate_cmos_layout(struct region_device *rdev)
+static struct cmos_option_table *get_cmos_layout(struct region_device *rdev)
 {
-	uint32_t cbfs_type = CBFS_COMPONENT_CMOS_LAYOUT;
-	MAYBE_STATIC_BSS struct cbfsf fh = {};
+	MAYBE_STATIC_BSS struct cmos_option_table ct = NULL;

/*
     * 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 'COREBOOT' region for now.
+	 * Support only one CMOS layout in the RO CBFS for now.
     */
-	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;
+	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;
 }

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;
@@ -97,16 +89,9 @@
    /* Figure out how long name is */
    namelen = strnlen(name, CMOS_MAX_NAME_LENGTH);

-	if (locate_cmos_layout(&rdev) != CB_SUCCESS) {
+	ct = get_cmos_layout();
+	if (!ct)
    	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);
@@ -119,19 +104,15 @@
    }
    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)) {
-		rdev_munmap(&rdev, ct);
+	if (!cmos_checksum_valid(LB_CKS_RANGE_START, LB_CKS_RANGE_END, LB_CKS_LOC))
    	return CB_CMOS_CHECKSUM_INVALID;
-	}
-	if (get_cmos_value(ce->bit, ce->length, dest) != CB_SUCCESS) {
-		rdev_munmap(&rdev, ct);
+}
+	if (get_cmos_value(ce->bit, ce->length, dest) != CB_SUCCESS)
    	return CB_CMOS_ACCESS_ERROR;
-	}
-	rdev_munmap(&rdev, ct);
+}
    return CB_SUCCESS;
 }

@@ -179,7 +160,6 @@
 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;
@@ -188,16 +168,9 @@
    /* Figure out how long name is */
    namelen = strnlen(name, CMOS_MAX_NAME_LENGTH);

-	if (locate_cmos_layout(&rdev) != CB_SUCCESS) {
+	ct = get_cmos_layout();
+	if (!ct)
    	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);
@@ -210,7 +183,6 @@
    }
    if (!found) {
    	printk(BIOS_DEBUG, "WARNING: No CMOS option '%s'.\n", name);
-		rdev_munmap(&rdev, ct);
    	return CB_CMOS_OPTION_NOT_FOUND;
    }

@@ -219,18 +191,13 @@
    	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) {
-			rdev_munmap(&rdev, ct);
+		    != CB_SUCCESS)
    		return CB_CMOS_ACCESS_ERROR;
-		}
    }

-	if (set_cmos_value(ce->bit, length, value) != CB_SUCCESS) {
-		rdev_munmap(&rdev, ct);
+	if (set_cmos_value(ce->bit, length, value) != CB_SUCCESS)
    	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 cc61b16..8cd5713 100644
--- a/src/include/cbfs.h
+++ b/src/include/cbfs.h
@@ -36,9 +36,12 @@
    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);
-/* 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. */
+/* 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);
+/* 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. */
 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,
@@ -47,6 +50,9 @@
    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 c3660aa..ba50c34 100644
--- a/src/lib/cbfs.c
+++ b/src/lib/cbfs.c
@@ -79,9 +79,9 @@
    return 0;
 }

-void *cbfs_map(const char *name, size_t *size)
+static void *_cbfs_map(const char *name, size_t *size, bool force_ro)
 {
-	const struct cbfs_boot_device *cbd = cbfs_get_boot_device(false);
+	const struct cbfs_boot_device *cbd = cbfs_get_boot_device(force_ro);
    union cbfs_mdata mdata;
    size_t data_offset;

@@ -96,6 +96,16 @@
    return rdev_mmap(&cbd->rdev, data_offset, data_size);
 }

+void *cbfs_map(const char *name, size_t *size)
+{
+	return _cbfs_map(name, size, false);
+}
+
+void *cbfs_ro_map(const char *name, size_t *size)
+{
+	return _cbfs_map(name, size, true);
+}
+
 int cbfs_unmap(void *mapping)
 {
    /* This works because munmap() only works on the root rdev and never
@@ -215,7 +225,8 @@
    return prog_entry(&stage);
 }

-size_t cbfs_load(const char *name, void *buf, size_t buf_size)
+static size_t _cbfs_load(const char *name, void *buf, size_t buf_size,
+			 bool force_ro)
 {
    const struct cbfs_boot_device *cbd = cbfs_get_boot_device(false);
    union cbfs_mdata mdata;
@@ -238,6 +249,16 @@
    				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);
+}
+
 size_t cbfs_prog_stage_section(struct prog *pstage, uintptr_t *base)
 {
    struct cbfs_stage stage;
diff --git a/src/northbridge/intel/haswell/raminit.c b/src/northbridge/intel/haswell/raminit.c
index 8267833..00211bd 100644
--- a/src/northbridge/intel/haswell/raminit.c
+++ b/src/northbridge/intel/haswell/raminit.c
@@ -118,8 +118,6 @@
 void sdram_initialize(struct pei_data *pei_data)
 {
    unsigned long entry;
-	uint32_t type = CBFS_TYPE_MRC;
-	struct cbfsf f;

printk(BIOS_DEBUG, "Starting UEFI PEI System Agent\n");

@@ -144,12 +142,11 @@
    /*
     * 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
+	 * COREBOOT fmap region. We don't care about leaking the mapping.
     */
-	if (cbfs_locate_file_in_region(&f, "COREBOOT", "mrc.bin", &type) < 0)
+	entry = (unsigned long)cbfs_ro_map("mrc.bin", NULL);
+	if (!entry)
    	die("mrc.bin not found!");
-	/* We don't care about leaking the mapping */
-	entry = (unsigned long)rdev_mmap_full(&f.data);
    if (entry) {
    	int rv;
    	asm volatile (
diff --git a/src/soc/intel/broadwell/romstage/raminit.c b/src/soc/intel/broadwell/romstage/raminit.c
index 3759c1f..bcf7148 100644
--- a/src/soc/intel/broadwell/romstage/raminit.c
+++ b/src/soc/intel/broadwell/romstage/raminit.c
@@ -43,8 +43,6 @@
    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);

@@ -76,15 +74,10 @@
    	pei_data->saved_data_size = 0;
    }

-	/* 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;
-	}
+	entry = cbfs_ro_map("mrc.bin", NULL);
+	if (entry == NULL)
+		die("mrc.bin not found!");

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 4ac580e..3e08663 100644
--- a/src/soc/intel/quark/romstage/romstage.c
+++ b/src/soc/intel/quark/romstage/romstage.c
@@ -63,29 +63,16 @@

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 */
-	type = CBFS_TYPE_RAW;
-	if (cbfs_locate_file_in_region(&fh, "COREBOOT", "rmu.bin", &type))
+	rmu_data = cbfs_ro_map("rmu.bin", &fsize);
+	if (!rmu_data)
    	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;
 }

[Julius Werner (Code Review)]

Hello build bot (Jenkins), Patrick Rudolph, Aaron Durbin,

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

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

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

Change subject: cbfs: Introduce cbfs_ro_map() and cbfs_ro_load()
......................................................................

cbfs: Introduce cbfs_ro_map() and cbfs_ro_load()

This patch introduces two new CBFS API functions which are equivalent to
cbfs_map() and cbfs_load(), respectively, with the difference that they
always operate on the read-only CBFS region ("COREBOOT" FMAP section).
Use it to replace some of the simple cases that needed to use
cbfs_locate_file_in_region().

Signed-off-by: Julius Werner 
Change-Id: I9c55b022b6502a333a9805ab0e4891dd7b97ef7f
---
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/romstage/raminit.c
M src/soc/intel/quark/romstage/romstage.c
6 files changed, 61 insertions(+), 90 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/39306/2

[Julius Werner (Code Review)]

Hello build bot (Jenkins), Patrick Rudolph, Aaron Durbin,

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

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

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

Change subject: cbfs: Introduce cbfs_ro_map() and cbfs_ro_load()
......................................................................

cbfs: Introduce cbfs_ro_map() and cbfs_ro_load()

This patch introduces two new CBFS API functions which are equivalent to
cbfs_map() and cbfs_load(), respectively, with the difference that they
always operate on the read-only CBFS region ("COREBOOT" FMAP section).
Use it to replace some of the simple cases that needed to use
cbfs_locate_file_in_region().

Signed-off-by: Julius Werner 
Change-Id: I9c55b022b6502a333a9805ab0e4891dd7b97ef7f
---
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/romstage/raminit.c
M src/soc/intel/quark/romstage/romstage.c
6 files changed, 61 insertions(+), 90 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/39306/3

[Julius Werner (Code Review)]

Hello build bot (Jenkins), Patrick Georgi, Patrick Rudolph, Aaron Durbin,

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

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

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

Change subject: cbfs: Introduce cbfs_ro_map() and cbfs_ro_load()
......................................................................

cbfs: Introduce cbfs_ro_map() and cbfs_ro_load()

This patch introduces two new CBFS API functions which are equivalent to
cbfs_map() and cbfs_load(), respectively, with the difference that they
always operate on the read-only CBFS region ("COREBOOT" FMAP section).
Use it to replace some of the simple cases that needed to use
cbfs_locate_file_in_region().

Signed-off-by: Julius Werner 
Change-Id: I9c55b022b6502a333a9805ab0e4891dd7b97ef7f
---
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/romstage/raminit.c
M src/soc/intel/quark/romstage/romstage.c
6 files changed, 62 insertions(+), 91 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/39306/4

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39306 )

Change subject: cbfs: Introduce cbfs_ro_map() and cbfs_ro_load()
......................................................................

Patch Set 4:

(2 comments)

https://review.coreboot.org/c/coreboot/+/39306/4/src/drivers/pc80/rtc/option... 
File src/drivers/pc80/rtc/option.c:

https://review.coreboot.org/c/coreboot/+/39306/4/src/drivers/pc80/rtc/option... 
PS4, Line 53: truct region_device *rdev
What's this parameter for now?

https://review.coreboot.org/c/coreboot/+/39306/4/src/drivers/pc80/rtc/option... 
PS4, Line 55: ct
This should be a pointer?

[Julius Werner (Code Review)]

Hello build bot (Jenkins), Patrick Georgi, Aaron Durbin, Patrick Rudolph,

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

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

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

Change subject: cbfs: Introduce cbfs_ro_map() and cbfs_ro_load()
......................................................................

cbfs: Introduce cbfs_ro_map() and cbfs_ro_load()

This patch introduces two new CBFS API functions which are equivalent to
cbfs_map() and cbfs_load(), respectively, with the difference that they
always operate on the read-only CBFS region ("COREBOOT" FMAP section).
Use it to replace some of the simple cases that needed to use
cbfs_locate_file_in_region().

Change-Id: I9c55b022b6502a333a9805ab0e4891dd7b97ef7f
Signed-off-by: Julius Werner 
---
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/romstage/raminit.c
M src/soc/intel/quark/romstage/romstage.c
6 files changed, 62 insertions(+), 91 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/39306/5

[Julius Werner (Code Review)]

Hello build bot (Jenkins), Patrick Georgi, Angel Pons, Aaron Durbin, Patrick Rudolph,

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

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

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

Change subject: cbfs: Introduce cbfs_ro_map() and cbfs_ro_load()
......................................................................

cbfs: Introduce cbfs_ro_map() and cbfs_ro_load()

This patch introduces two new CBFS API functions which are equivalent to
cbfs_map() and cbfs_load(), respectively, with the difference that they
always operate on the read-only CBFS region ("COREBOOT" FMAP section).
Use it to replace some of the simple cases that needed to use
cbfs_locate_file_in_region().

Change-Id: I9c55b022b6502a333a9805ab0e4891dd7b97ef7f
Signed-off-by: Julius Werner 
---
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/romstage/raminit.c
M src/soc/intel/quark/romstage/romstage.c
6 files changed, 62 insertions(+), 91 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/39306/6

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39306 )

Change subject: cbfs: Introduce cbfs_ro_map() and cbfs_ro_load()
......................................................................

Patch Set 6:

(2 comments)

https://review.coreboot.org/c/coreboot/+/39306/4/src/drivers/pc80/rtc/option... 
File src/drivers/pc80/rtc/option.c:

https://review.coreboot.org/c/coreboot/+/39306/4/src/drivers/pc80/rtc/option... 
PS4, Line 53: truct region_device *rdev
> What's this parameter for now?

Whoops, yes, looks this doesn't get built by default and I missed some stuff. Fixed.

https://review.coreboot.org/c/coreboot/+/39306/4/src/drivers/pc80/rtc/option... 
PS4, Line 55: ct
> This should be a pointer?

Done

[Julius Werner (Code Review)]

Hello build bot (Jenkins), Patrick Georgi, Angel Pons, Aaron Durbin, Patrick Rudolph,

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

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

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

Change subject: cbfs: Introduce cbfs_ro_map() and cbfs_ro_load()
......................................................................

cbfs: Introduce cbfs_ro_map() and cbfs_ro_load()

This patch introduces two new CBFS API functions which are equivalent to
cbfs_map() and cbfs_load(), respectively, with the difference that they
always operate on the read-only CBFS region ("COREBOOT" FMAP section).
Use it to replace some of the simple cases that needed to use
cbfs_locate_file_in_region().

Change-Id: I9c55b022b6502a333a9805ab0e4891dd7b97ef7f
Signed-off-by: Julius Werner 
---
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/romstage/raminit.c
M src/soc/intel/quark/romstage/romstage.c
6 files changed, 62 insertions(+), 91 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/39306/7

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39306 )

Change subject: cbfs: Introduce cbfs_ro_map() and cbfs_ro_load()
......................................................................

Patch Set 8: Code-Review+2

[Julius Werner (Code Review)]

Hello build bot (Jenkins), Patrick Georgi, Angel Pons, Aaron Durbin, Patrick Rudolph,

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

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

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

Change subject: cbfs: Introduce cbfs_ro_map() and cbfs_ro_load()
......................................................................

cbfs: Introduce cbfs_ro_map() and cbfs_ro_load()

This patch introduces two new CBFS API functions which are equivalent to
cbfs_map() and cbfs_load(), respectively, with the difference that they
always operate on the read-only CBFS region ("COREBOOT" FMAP section).
Use it to replace some of the simple cases that needed to use
cbfs_locate_file_in_region().

Change-Id: I9c55b022b6502a333a9805ab0e4891dd7b97ef7f
Signed-off-by: Julius Werner 
---
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/romstage/raminit.c
M src/soc/intel/quark/romstage/romstage.c
6 files changed, 61 insertions(+), 93 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/39306/13

[Julius Werner (Code Review)]

Hello build bot (Jenkins), Patrick Georgi, Angel Pons, Aaron Durbin, Patrick Rudolph,

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

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

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

Change subject: cbfs: Introduce cbfs_ro_map() and cbfs_ro_load()
......................................................................

cbfs: Introduce cbfs_ro_map() and cbfs_ro_load()

This patch introduces two new CBFS API functions which are equivalent to
cbfs_map() and cbfs_load(), respectively, with the difference that they
always operate on the read-only CBFS region ("COREBOOT" FMAP section).
Use it to replace some of the simple cases that needed to use
cbfs_locate_file_in_region().

Change-Id: I9c55b022b6502a333a9805ab0e4891dd7b97ef7f
Signed-off-by: Julius Werner 
---
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, 61 insertions(+), 93 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/39306/14

[Patrick Georgi (Code Review)]

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39306 )

Change subject: cbfs: Introduce cbfs_ro_map() and cbfs_ro_load()
......................................................................

Patch Set 16: Code-Review+2

[Furquan Shaikh (Code Review)]

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

Change subject: cbfs: Introduce cbfs_ro_map() and cbfs_ro_load()
......................................................................

Patch Set 17: Code-Review+2

[Julius Werner (Code Review)]

Julius Werner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/39306 )

Change subject: cbfs: Introduce cbfs_ro_map() and cbfs_ro_load()
......................................................................

cbfs: Introduce cbfs_ro_map() and cbfs_ro_load()

This patch introduces two new CBFS API functions which are equivalent to
cbfs_map() and cbfs_load(), respectively, with the difference that they
always operate on the read-only CBFS region ("COREBOOT" FMAP section).
Use it to replace some of the simple cases that needed to use
cbfs_locate_file_in_region().

Change-Id: I9c55b022b6502a333a9805ab0e4891dd7b97ef7f
Signed-off-by: Julius Werner 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/39306
Reviewed-by: Furquan Shaikh 
Reviewed-by: Patrick Georgi 
Tested-by: build bot (Jenkins) 
---
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, 61 insertions(+), 93 deletions(-)

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

diff --git a/src/drivers/pc80/rtc/option.c b/src/drivers/pc80/rtc/option.c
index bb9d29a..409f0ef 100644
--- a/src/drivers/pc80/rtc/option.c
+++ b/src/drivers/pc80/rtc/option.c
@@ -49,36 +49,28 @@
    return CB_SUCCESS;
 }

-static enum cb_err locate_cmos_layout(struct region_device *rdev)
+static struct cmos_option_table *get_cmos_layout(void)
 {
-	uint32_t cbfs_type = CBFS_COMPONENT_CMOS_LAYOUT;
-	static struct cbfsf fh;
+	static struct cmos_option_table *ct = NULL;

/*
     * 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 'COREBOOT' region for now.
+	 * Support only one CMOS layout in the RO CBFS for now.
     */
-	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;
+	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;
 }

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;
@@ -86,16 +78,9 @@
    /* Figure out how long name is */
    namelen = strnlen(name, CMOS_MAX_NAME_LENGTH);

-	if (locate_cmos_layout(&rdev) != CB_SUCCESS) {
+	ct = get_cmos_layout();
+	if (!ct)
    	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);
@@ -108,19 +93,15 @@
    }
    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)) {
-		rdev_munmap(&rdev, ct);
+	if (!cmos_checksum_valid(LB_CKS_RANGE_START, LB_CKS_RANGE_END, LB_CKS_LOC))
    	return CB_CMOS_CHECKSUM_INVALID;
-	}
-	if (get_cmos_value(ce->bit, ce->length, dest) != CB_SUCCESS) {
-		rdev_munmap(&rdev, ct);
+
+	if (get_cmos_value(ce->bit, ce->length, dest) != CB_SUCCESS)
    	return CB_CMOS_ACCESS_ERROR;
-	}
-	rdev_munmap(&rdev, ct);
+
    return CB_SUCCESS;
 }

@@ -168,7 +149,6 @@
 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;
@@ -177,16 +157,9 @@
    /* Figure out how long name is */
    namelen = strnlen(name, CMOS_MAX_NAME_LENGTH);

-	if (locate_cmos_layout(&rdev) != CB_SUCCESS) {
+	ct = get_cmos_layout();
+	if (!ct)
    	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);
@@ -199,7 +172,6 @@
    }
    if (!found) {
    	printk(BIOS_DEBUG, "WARNING: No CMOS option '%s'.\n", name);
-		rdev_munmap(&rdev, ct);
    	return CB_CMOS_OPTION_NOT_FOUND;
    }

@@ -208,18 +180,13 @@
    	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) {
-			rdev_munmap(&rdev, ct);
+		    != CB_SUCCESS)
    		return CB_CMOS_ACCESS_ERROR;
-		}
    }

-	if (set_cmos_value(ce->bit, length, value) != CB_SUCCESS) {
-		rdev_munmap(&rdev, ct);
+	if (set_cmos_value(ce->bit, length, value) != CB_SUCCESS)
    	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 992b658..8d4c220 100644
--- a/src/include/cbfs.h
+++ b/src/include/cbfs.h
@@ -23,9 +23,12 @@
    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);
-/* 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. */
+/* 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. */
 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,
@@ -34,6 +37,9 @@
    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 8d868a6..beab74e 100644
--- a/src/lib/cbfs.c
+++ b/src/lib/cbfs.c
@@ -70,12 +70,12 @@
    return 0;
 }

-void *cbfs_map(const char *name, size_t *size_out)
+static void *_cbfs_map(const char *name, size_t *size_out, bool force_ro)
 {
    struct region_device rdev;
    union cbfs_mdata mdata;

-	if (cbfs_boot_lookup(name, false, &mdata, &rdev))
+	if (cbfs_boot_lookup(name, force_ro, &mdata, &rdev))
    	return NULL;

if (size_out != NULL)
@@ -84,6 +84,16 @@
    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
@@ -281,12 +291,13 @@
    return cbfs_map(name, NULL);
 }

-size_t cbfs_load(const char *name, void *buf, size_t buf_size)
+static size_t _cbfs_load(const char *name, void *buf, size_t buf_size,
+			 bool force_ro)
 {
    struct region_device rdev;
    union cbfs_mdata mdata;

-	if (cbfs_boot_lookup(name, false, &mdata, &rdev))
+	if (cbfs_boot_lookup(name, force_ro, &mdata, &rdev))
    	return 0;

uint32_t compression = CBFS_COMPRESS_NONE;
@@ -302,6 +313,16 @@
    				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 290e402..9617ffb 100644
--- a/src/northbridge/intel/haswell/raminit.c
+++ b/src/northbridge/intel/haswell/raminit.c
@@ -107,9 +107,6 @@
 {
    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");

/*
@@ -130,13 +127,10 @@

/*
     * 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.
+	 * in the flash and can therefore only reside in the COREBOOT fmap region. We don't care
+	 * about leaking the mapping.
     */
-	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);
+	entry = cbfs_ro_map("mrc.bin", NULL);
    if (entry) {
    	int rv = entry(pei_data);

diff --git a/src/soc/intel/broadwell/raminit.c b/src/soc/intel/broadwell/raminit.c
index e51b4f7..44a8937 100644
--- a/src/soc/intel/broadwell/raminit.c
+++ b/src/soc/intel/broadwell/raminit.c
@@ -80,8 +80,6 @@
    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);

@@ -114,15 +112,10 @@
    	pei_data->saved_data_size = 0;
    }

-	/* 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;
-	}
+	entry = cbfs_ro_map("mrc.bin", NULL);
+	if (entry == NULL)
+		die("mrc.bin not found!");

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 9800aa4..23a551e 100644
--- a/src/soc/intel/quark/romstage/romstage.c
+++ b/src/soc/intel/quark/romstage/romstage.c
@@ -49,29 +49,16 @@

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 */
-	type = CBFS_TYPE_RAW;
-	if (cbfs_locate_file_in_region(&fh, "COREBOOT", "rmu.bin", &type))
+	rmu_data = cbfs_ro_map("rmu.bin", &fsize);
+	if (!rmu_data)
    	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;
 }

[Mike Banon (Code Review)]

Mike Banon has created a revert of this change. ( https://review.coreboot.org/c/coreboot/+/39306 )

Change subject: cbfs: Introduce cbfs_ro_map() and cbfs_ro_load()
......................................................................