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)))