Hello Aaron Durbin,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/38418
to review the following change.
Change subject: intel/apollolake: Remove CBFS locator override ......................................................................
intel/apollolake: Remove CBFS locator override
This patch removes the CBFS locator override for the Apollolake SoC and instead integrates the extra sanity check it was used for straight in the boot device initializer.
Change-Id: Iccdb885be233bb027a6a1f2cc79054582cbdf3fc Signed-off-by: Julius Werner jwerner@chromium.org --- M src/soc/intel/apollolake/mmap_boot.c 1 file changed, 10 insertions(+), 32 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/38418/1
diff --git a/src/soc/intel/apollolake/mmap_boot.c b/src/soc/intel/apollolake/mmap_boot.c index 96ddc05..611b0a7 100644 --- a/src/soc/intel/apollolake/mmap_boot.c +++ b/src/soc/intel/apollolake/mmap_boot.c @@ -90,6 +90,16 @@ CONFIG_ROM_SIZE);
bios_size = size; + + /* Check that the CBFS lies within the memory mapped area. It's too + easy to forget the SRAM mapping when crafting an FMAP file. */ + struct region cbfs_region; + if (!fmap_locate_area("COREBOOT", &cbfs_region) && + !region_is_subregion(&real_dev.sub_region, &cbfs_region)) + printk(BIOS_CRIT, + "ERROR: CBFS @ %zx size %zx exceeds mem-mapped area @ %zx size %zx\n", + region_offset(&cbfs_region), region_sz(&cbfs_region), + start, bios_mapped_size); }
const struct region_device *boot_device_ro(void) @@ -98,35 +108,3 @@
return &real_dev.rdev; } - -static int iafw_boot_region_device(struct region_device *rdev) -{ - struct region *real_dev_reg; - - if (cbfs_default_region_device(rdev)) - return -1; - - /* Check that we are within the memory mapped area. It's too - easy to forget the SRAM mapping when crafting an FMAP file. */ - real_dev_reg = &real_dev.sub_region; - if (region_is_subregion(real_dev_reg, region_device_region(rdev))) { - printk(BIOS_DEBUG, "CBFS @ %zx size %zx\n", - region_device_offset(rdev), region_device_sz(rdev)); - } else { - printk(BIOS_CRIT, - "ERROR: CBFS @ %zx size %zx exceeds mem-mapped area @ %zx size %zx\n", - region_device_offset(rdev), region_device_sz(rdev), - region_offset(real_dev_reg), region_sz(real_dev_reg)); - } - - return 0; -} - -/* - * Named cbfs_default_locator so that it overrides the default, but incompatible - * locator in cbfs.c - */ -const struct cbfs_locator cbfs_default_locator = { - .name = "IAFW Locator", - .locate = iafw_boot_region_device, -};
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38418 )
Change subject: intel/apollolake: Remove CBFS locator override ......................................................................
Patch Set 1: Code-Review+2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38418 )
Change subject: intel/apollolake: Remove CBFS locator override ......................................................................
Patch Set 1: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38418 )
Change subject: intel/apollolake: Remove CBFS locator override ......................................................................
intel/apollolake: Remove CBFS locator override
This patch removes the CBFS locator override for the Apollolake SoC and instead integrates the extra sanity check it was used for straight in the boot device initializer.
Change-Id: Iccdb885be233bb027a6a1f2cc79054582cbdf3fc Signed-off-by: Julius Werner jwerner@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/38418 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Nico Huber nico.h@gmx.de --- M src/soc/intel/apollolake/mmap_boot.c 1 file changed, 10 insertions(+), 32 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Angel Pons: Looks good to me, approved
diff --git a/src/soc/intel/apollolake/mmap_boot.c b/src/soc/intel/apollolake/mmap_boot.c index 96ddc05..611b0a7 100644 --- a/src/soc/intel/apollolake/mmap_boot.c +++ b/src/soc/intel/apollolake/mmap_boot.c @@ -90,6 +90,16 @@ CONFIG_ROM_SIZE);
bios_size = size; + + /* Check that the CBFS lies within the memory mapped area. It's too + easy to forget the SRAM mapping when crafting an FMAP file. */ + struct region cbfs_region; + if (!fmap_locate_area("COREBOOT", &cbfs_region) && + !region_is_subregion(&real_dev.sub_region, &cbfs_region)) + printk(BIOS_CRIT, + "ERROR: CBFS @ %zx size %zx exceeds mem-mapped area @ %zx size %zx\n", + region_offset(&cbfs_region), region_sz(&cbfs_region), + start, bios_mapped_size); }
const struct region_device *boot_device_ro(void) @@ -98,35 +108,3 @@
return &real_dev.rdev; } - -static int iafw_boot_region_device(struct region_device *rdev) -{ - struct region *real_dev_reg; - - if (cbfs_default_region_device(rdev)) - return -1; - - /* Check that we are within the memory mapped area. It's too - easy to forget the SRAM mapping when crafting an FMAP file. */ - real_dev_reg = &real_dev.sub_region; - if (region_is_subregion(real_dev_reg, region_device_region(rdev))) { - printk(BIOS_DEBUG, "CBFS @ %zx size %zx\n", - region_device_offset(rdev), region_device_sz(rdev)); - } else { - printk(BIOS_CRIT, - "ERROR: CBFS @ %zx size %zx exceeds mem-mapped area @ %zx size %zx\n", - region_device_offset(rdev), region_device_sz(rdev), - region_offset(real_dev_reg), region_sz(real_dev_reg)); - } - - return 0; -} - -/* - * Named cbfs_default_locator so that it overrides the default, but incompatible - * locator in cbfs.c - */ -const struct cbfs_locator cbfs_default_locator = { - .name = "IAFW Locator", - .locate = iafw_boot_region_device, -};
-
Angel Pons (Code Review) -
Julius Werner (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)