Change in ...coreboot[master]: Revert "vboot: Fix linking error with USE_OPTION_TABLE enabled" - coreboot-gerrit

14 Jan 2019

Hello Aaron Durbin, Patrick Rudolph, Arthur Heymans, Patrick Rudolph, Paul Menzel, Philipp Deppenwiese, build bot (Jenkins), Patrick Georgi,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/30904
to review the following change.
Change subject: Revert "vboot: Fix linking error with USE_OPTION_TABLE enabled"
......................................................................
Revert "vboot: Fix linking error with USE_OPTION_TABLE enabled"
This reverts commit 9554b26f9fd608cc613fc3ad869db33ef0edfe5c.
Reason for revert: This breaks USB Always On, and no one cared to
investigate the issue further.
Change-Id: I5a13be56d47dc8373d9c6d556505bb2e3674d4a8
---
M src/drivers/pc80/rtc/mc146818rtc.c
M src/lib/cbfs.c
M src/security/vboot/Makefile.inc
3 files changed, 18 insertions(+), 71 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/30904/1

diff --git a/src/drivers/pc80/rtc/mc146818rtc.c b/src/drivers/pc80/rtc/mc146818rtc.c
index 34db4c3..928b403 100644
--- a/src/drivers/pc80/rtc/mc146818rtc.c
+++ b/src/drivers/pc80/rtc/mc146818rtc.c
@@ -240,34 +240,9 @@
    return CB_SUCCESS;
 }
-static enum cb_err locate_cmos_layout(struct region_device *rdev)
-{
-	uint32_t cbfs_type = CBFS_COMPONENT_CMOS_LAYOUT;
-	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 'COREBOOT' region for now.
-	 */
-	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 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;
@@ -280,20 +255,16 @@
    /* Figure out how long name is */
    namelen = strnlen(name, CMOS_MAX_NAME_LENGTH);
-	if (locate_cmos_layout(&rdev) != CB_SUCCESS) {
-		UNLOCK_NVRAM_CBFS_SPINLOCK();
-		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");
-
-		UNLOCK_NVRAM_CBFS_SPINLOCK();
-		return CB_CMOS_LAYOUT_NOT_FOUND;
-	}
-
    /* find the requested entry record */
+	ct = cbfs_boot_map_with_leak("cmos_layout.bin",
+					CBFS_COMPONENT_CMOS_LAYOUT, NULL);
+	if (!ct) {
+		printk(BIOS_ERR, "RTC: cmos_layout.bin could not be found. "
+						"Options are disabled\n");
+
+		UNLOCK_NVRAM_CBFS_SPINLOCK();
+		return CB_CMOS_LAYOUT_NOT_FOUND;
+	}
    ce = (struct cmos_entries *)((unsigned char *)ct + ct->header_length);
    for (; ce->tag == LB_TAG_OPTION;
    	ce = (struct cmos_entries *)((unsigned char *)ce + ce->size)) {
@@ -304,22 +275,18 @@
    }
    if (!found) {
    	printk(BIOS_DEBUG, "WARNING: No CMOS option '%s'.\n", name);
-		rdev_munmap(&rdev, ct);
    	UNLOCK_NVRAM_CBFS_SPINLOCK();
    	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);
    	UNLOCK_NVRAM_CBFS_SPINLOCK();
    	return CB_CMOS_CHECKSUM_INVALID;
    }
    if (get_cmos_value(ce->bit, ce->length, dest) != CB_SUCCESS) {
-		rdev_munmap(&rdev, ct);
    	UNLOCK_NVRAM_CBFS_SPINLOCK();
    	return CB_CMOS_ACCESS_ERROR;
    }
-	rdev_munmap(&rdev, ct);
    UNLOCK_NVRAM_CBFS_SPINLOCK();
    return CB_SUCCESS;
 }
@@ -380,7 +347,6 @@
 enum cb_err 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;
@@ -392,20 +358,14 @@
    /* Figure out how long name is */
    namelen = strnlen(name, CMOS_MAX_NAME_LENGTH);
-	if (locate_cmos_layout(&rdev) != CB_SUCCESS) {
-		UNLOCK_NVRAM_CBFS_SPINLOCK();
-		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");
-
-		UNLOCK_NVRAM_CBFS_SPINLOCK();
-		return CB_CMOS_LAYOUT_NOT_FOUND;
-	}
-
    /* find the requested entry record */
+	ct = cbfs_boot_map_with_leak("cmos_layout.bin",
+					CBFS_COMPONENT_CMOS_LAYOUT, NULL);
+	if (!ct) {
+		printk(BIOS_ERR, "cmos_layout.bin could not be found. "
+				 "Options are disabled\n");
+		return CB_CMOS_LAYOUT_NOT_FOUND;
+	}
    ce = (struct cmos_entries *)((unsigned char *)ct + ct->header_length);
    for (; ce->tag == LB_TAG_OPTION;
    	ce = (struct cmos_entries *)((unsigned char *)ce + ce->size)) {
@@ -416,7 +376,6 @@
    }
    if (!found) {
    	printk(BIOS_DEBUG, "WARNING: No CMOS option '%s'.\n", name);
-		rdev_munmap(&rdev, ct);
    	return CB_CMOS_OPTION_NOT_FOUND;
    }
@@ -425,18 +384,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/lib/cbfs.c b/src/lib/cbfs.c
index ca1fc84..9938e6a 100644
--- a/src/lib/cbfs.c
+++ b/src/lib/cbfs.c
@@ -321,11 +321,6 @@
static const struct cbfs_locator *locators[] = {
 #if IS_ENABLED(CONFIG_VBOOT)
-	/*
-	 * NOTE: Does not link in SMM, as the vboot_locator isn't compiled.
-	 * ATM there's no need for VBOOT functionality in SMM and it's not
-	 * a problem.
-	 */
    &vboot_locator,
 #endif
    &cbfs_master_header_locator,
diff --git a/src/security/vboot/Makefile.inc b/src/security/vboot/Makefile.inc
index 75c2a9e..6f18a35 100644
--- a/src/security/vboot/Makefile.inc
+++ b/src/security/vboot/Makefile.inc
@@ -154,8 +154,6 @@
    	font.bin \
    	vbgfx.bin \
    	rmu.bin \
-		cmos_layout.bin \
-		cmos.default \
    	$(call strip_quotes,$(CONFIG_RO_REGION_ONLY)) \
    	,$(1)),COREBOOT,COREBOOT FW_MAIN_A FW_MAIN_B)))
-- 
To view, visit https://review.coreboot.org/c/coreboot/+/30904
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I5a13be56d47dc8373d9c6d556505bb2e3674d4a8
Gerrit-Change-Number: 30904
Gerrit-PatchSet: 1
Gerrit-Owner: Angel Pons th3fanbus@gmail.com
Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org
Gerrit-Reviewer: Arthur Heymans arthur@aheymans.xyz
Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com
Gerrit-Reviewer: Patrick Rudolph patrick.rudolph@9elements.com
Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org
Gerrit-Reviewer: Paul Menzel paulepanter@users.sourceforge.net
Gerrit-Reviewer: Philipp Deppenwiese zaolin.daisuki@gmail.com
Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org
Gerrit-CC: Patrick Rudolph
Gerrit-MessageType: newchange



    