Aaron Durbin has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36939 )
Change subject: cbfs: switch to region_device for location APIs ......................................................................
cbfs: switch to region_device for location APIs
Drop struct cbfs_props and replace with struct region_device object. The goal of the cbfs locator APIs are to determine the correct region device to find the cbfs files. Therefore, start directly using struct region_device in the cbfs location paths. Update the users of the API and leverage the default boot region device implementation for apollolake.
Change-Id: I0158a095cc64c9900d8738f8ffd45ae4040575ea Signed-off-by: Aaron Durbin adurbin@chromium.org --- M src/include/cbfs.h M src/lib/cbfs.c M src/lib/coreboot_table.c M src/security/vboot/vboot_loader.c M src/soc/intel/apollolake/mmap_boot.c 5 files changed, 32 insertions(+), 55 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/36939/1
diff --git a/src/include/cbfs.h b/src/include/cbfs.h index 60129d3..7a984b8 100644 --- a/src/include/cbfs.h +++ b/src/include/cbfs.h @@ -73,18 +73,22 @@ size_t size; };
-/* Default CBFS locator .locate() callback that locates "COREBOOT" region. */ -int cbfs_default_props(struct cbfs_props *props); +/* Default CBFS locator .locate() callback that locates "COREBOOT" region. This + function is exposed to reduce code duplication in other parts of the code + base. To obtain the correct region device the selection process is required + by way of cbfs_boot_region_device(). */ +int cbfs_default_region_device(struct region_device *rdev);
-/* Return < 0 on error otherwise props are filled out accordingly. */ -int cbfs_boot_region_properties(struct cbfs_props *props); +/* Select the boot region device from the cbfs locators. + Return < 0 on error, 0 on success. */ +int cbfs_boot_region_device(struct region_device *rdev);
/* Object used to identify location of current cbfs to use for cbfs_boot_* * operations. It's used by cbfs_boot_region_properties(). */ struct cbfs_locator { const char *name; /* Returns 0 on successful fill of cbfs properties. */ - int (*locate)(struct cbfs_props *props); + int (*locate)(struct region_device *rdev); };
#endif diff --git a/src/lib/cbfs.c b/src/lib/cbfs.c index fbe6e43..636ff70 100644 --- a/src/lib/cbfs.c +++ b/src/lib/cbfs.c @@ -39,26 +39,9 @@ int cbfs_boot_locate(struct cbfsf *fh, const char *name, uint32_t *type) { struct region_device rdev; - const struct region_device *boot_dev; - struct cbfs_props props;
- if (cbfs_boot_region_properties(&props)) { - printk(BIOS_ALERT, "ERROR: Failed to locate boot region\n"); + if (cbfs_boot_region_device(&rdev)) return -1; - } - - /* All boot CBFS operations are performed using the RO device. */ - boot_dev = boot_device_ro(); - - if (boot_dev == NULL) { - printk(BIOS_ALERT, "ERROR: Failed to find boot device\n"); - return -1; - } - - if (rdev_chain(&rdev, boot_dev, props.offset, props.size)) { - printk(BIOS_ALERT, "ERROR: Failed to access boot region inside boot device\n"); - return -1; - }
int ret = cbfs_locate(fh, &rdev, name, type);
@@ -297,17 +280,13 @@ }
/* The default locator to find the CBFS in the "COREBOOT" FMAP region. */ -int cbfs_default_props(struct cbfs_props *props) +int cbfs_default_region_device(struct region_device *rdev) { - struct region region; - - if (fmap_locate_area("COREBOOT", ®ion)) + if (fmap_locate_area_as_rdev("COREBOOT", rdev)) return -1;
- props->offset = region_offset(®ion); - props->size = region_sz(®ion); - - printk(BIOS_SPEW, "CBFS @ %zx size %zx\n", props->offset, props->size); + printk(BIOS_SPEW, "CBFS @ %zx size %zx\n", + region_device_offset(rdev), region_device_sz(rdev));
return 0; } @@ -317,7 +296,7 @@ * devices. */ const struct cbfs_locator __weak cbfs_default_locator = { .name = "COREBOOT Locator", - .locate = cbfs_default_props, + .locate = cbfs_default_region_device, };
extern const struct cbfs_locator vboot_locator; @@ -334,7 +313,7 @@ &cbfs_default_locator, };
-int cbfs_boot_region_properties(struct cbfs_props *props) +int cbfs_boot_region_device(struct region_device *rdev) { int i;
@@ -348,11 +327,12 @@ if (ops->locate == NULL) continue;
- if (ops->locate(props)) + if (ops->locate(rdev)) continue;
LOG("'%s' located CBFS at [%zx:%zx)\n", - ops->name, props->offset, props->offset + props->size); + ops->name, region_device_offset(rdev), + region_device_end(rdev));
return 0; } diff --git a/src/lib/coreboot_table.c b/src/lib/coreboot_table.c index d3576e6..da2a933 100644 --- a/src/lib/coreboot_table.c +++ b/src/lib/coreboot_table.c @@ -254,12 +254,12 @@ static void lb_boot_media_params(struct lb_header *header) { struct lb_boot_media_params *bmp; - struct cbfs_props props; const struct region_device *boot_dev; + struct region_device cbfs_dev;
boot_device_init();
- if (cbfs_boot_region_properties(&props)) + if (cbfs_boot_region_device(&cbfs_dev)) return;
boot_dev = boot_device_ro(); @@ -270,8 +270,8 @@ bmp->tag = LB_TAG_BOOT_MEDIA_PARAMS; bmp->size = sizeof(*bmp);
- bmp->cbfs_offset = props.offset; - bmp->cbfs_size = props.size; + bmp->cbfs_offset = region_device_offset(&cbfs_dev); + bmp->cbfs_size = region_device_sz(&cbfs_dev); bmp->boot_media_size = region_device_sz(boot_dev);
bmp->fmap_offset = get_fmap_flash_offset(); diff --git a/src/security/vboot/vboot_loader.c b/src/security/vboot/vboot_loader.c index 2b7ba83..f6cf6b0 100644 --- a/src/security/vboot/vboot_loader.c +++ b/src/security/vboot/vboot_loader.c @@ -14,6 +14,7 @@ */
#include <arch/early_variables.h> +#include <boot_device.h> #include <cbfs.h> #include <console/console.h> #include <ec/google/chromeec/ec.h> @@ -71,7 +72,7 @@ } }
-static int vboot_locate(struct cbfs_props *props) +static int vboot_locate(struct region_device *rdev) { struct region selected_region;
@@ -82,10 +83,7 @@ if (vboot_get_selected_region(&selected_region)) return -1;
- props->offset = region_offset(&selected_region); - props->size = region_sz(&selected_region); - - return 0; + return boot_device_ro_subregion(&selected_region, rdev); }
const struct cbfs_locator vboot_locator = { diff --git a/src/soc/intel/apollolake/mmap_boot.c b/src/soc/intel/apollolake/mmap_boot.c index 631a834..96ddc05 100644 --- a/src/soc/intel/apollolake/mmap_boot.c +++ b/src/soc/intel/apollolake/mmap_boot.c @@ -99,28 +99,23 @@ return &real_dev.rdev; }
-static int iafw_boot_region_properties(struct cbfs_props *props) +static int iafw_boot_region_device(struct region_device *rdev) { struct region *real_dev_reg; - struct region regn;
- /* use fmap to locate CBFS area */ - if (fmap_locate_area("COREBOOT", ®n)) + if (cbfs_default_region_device(rdev)) return -1;
- props->offset = region_offset(®n); - props->size = region_sz(®n); - /* 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, ®n)) { + if (region_is_subregion(real_dev_reg, region_device_region(rdev))) { printk(BIOS_DEBUG, "CBFS @ %zx size %zx\n", - props->offset, props->size); + 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", - props->offset, props->size, + region_device_offset(rdev), region_device_sz(rdev), region_offset(real_dev_reg), region_sz(real_dev_reg)); }
@@ -133,5 +128,5 @@ */ const struct cbfs_locator cbfs_default_locator = { .name = "IAFW Locator", - .locate = iafw_boot_region_properties, + .locate = iafw_boot_region_device, };
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36939 )
Change subject: cbfs: switch to region_device for location APIs ......................................................................
Patch Set 1: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/36939/1/src/lib/cbfs.c File src/lib/cbfs.c:
https://review.coreboot.org/c/coreboot/+/36939/1/src/lib/cbfs.c@289 PS1, Line 289: region_device_offset(rdev), region_device_sz(rdev)); nit: This is pretty redundant anyway considering that cbfs_boot_region_device() prints it again, right?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36939 )
Change subject: cbfs: switch to region_device for location APIs ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36939/1/src/lib/cbfs.c File src/lib/cbfs.c:
https://review.coreboot.org/c/coreboot/+/36939/1/src/lib/cbfs.c@289 PS1, Line 289: region_device_offset(rdev), region_device_sz(rdev));
nit: This is pretty redundant anyway considering that cbfs_boot_region_device() prints it again, rig […]
Ya. I was noticing that as well. I wasn't trying to remove the prints in this patch. I wanted to do a straight change of the types.
Hello Patrick Rudolph, Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36939
to look at the new patch set (#2).
Change subject: cbfs: switch to region_device for location APIs ......................................................................
cbfs: switch to region_device for location APIs
Drop struct cbfs_props and replace with struct region_device object. The goal of the cbfs locator APIs are to determine the correct region device to find the cbfs files. Therefore, start directly using struct region_device in the cbfs location paths. Update the users of the API and leverage the default boot region device implementation for apollolake.
Change-Id: I0158a095cc64c9900d8738f8ffd45ae4040575ea Signed-off-by: Aaron Durbin adurbin@chromium.org --- M src/include/cbfs.h M src/lib/cbfs.c M src/lib/coreboot_table.c M src/security/vboot/vboot_loader.c M src/soc/intel/apollolake/mmap_boot.c 5 files changed, 32 insertions(+), 59 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/36939/2
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36939 )
Change subject: cbfs: switch to region_device for location APIs ......................................................................
Patch Set 2: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/36939 )
Change subject: cbfs: switch to region_device for location APIs ......................................................................
cbfs: switch to region_device for location APIs
Drop struct cbfs_props and replace with struct region_device object. The goal of the cbfs locator APIs are to determine the correct region device to find the cbfs files. Therefore, start directly using struct region_device in the cbfs location paths. Update the users of the API and leverage the default boot region device implementation for apollolake.
Change-Id: I0158a095cc64c9900d8738f8ffd45ae4040575ea Signed-off-by: Aaron Durbin adurbin@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/36939 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Julius Werner jwerner@chromium.org --- M src/include/cbfs.h M src/lib/cbfs.c M src/lib/coreboot_table.c M src/security/vboot/vboot_loader.c M src/soc/intel/apollolake/mmap_boot.c 5 files changed, 32 insertions(+), 59 deletions(-)
Approvals: build bot (Jenkins): Verified Julius Werner: Looks good to me, approved
diff --git a/src/include/cbfs.h b/src/include/cbfs.h index 60129d3..7a984b8 100644 --- a/src/include/cbfs.h +++ b/src/include/cbfs.h @@ -73,18 +73,22 @@ size_t size; };
-/* Default CBFS locator .locate() callback that locates "COREBOOT" region. */ -int cbfs_default_props(struct cbfs_props *props); +/* Default CBFS locator .locate() callback that locates "COREBOOT" region. This + function is exposed to reduce code duplication in other parts of the code + base. To obtain the correct region device the selection process is required + by way of cbfs_boot_region_device(). */ +int cbfs_default_region_device(struct region_device *rdev);
-/* Return < 0 on error otherwise props are filled out accordingly. */ -int cbfs_boot_region_properties(struct cbfs_props *props); +/* Select the boot region device from the cbfs locators. + Return < 0 on error, 0 on success. */ +int cbfs_boot_region_device(struct region_device *rdev);
/* Object used to identify location of current cbfs to use for cbfs_boot_* * operations. It's used by cbfs_boot_region_properties(). */ struct cbfs_locator { const char *name; /* Returns 0 on successful fill of cbfs properties. */ - int (*locate)(struct cbfs_props *props); + int (*locate)(struct region_device *rdev); };
#endif diff --git a/src/lib/cbfs.c b/src/lib/cbfs.c index fbe6e43..636ff70 100644 --- a/src/lib/cbfs.c +++ b/src/lib/cbfs.c @@ -39,26 +39,9 @@ int cbfs_boot_locate(struct cbfsf *fh, const char *name, uint32_t *type) { struct region_device rdev; - const struct region_device *boot_dev; - struct cbfs_props props;
- if (cbfs_boot_region_properties(&props)) { - printk(BIOS_ALERT, "ERROR: Failed to locate boot region\n"); + if (cbfs_boot_region_device(&rdev)) return -1; - } - - /* All boot CBFS operations are performed using the RO device. */ - boot_dev = boot_device_ro(); - - if (boot_dev == NULL) { - printk(BIOS_ALERT, "ERROR: Failed to find boot device\n"); - return -1; - } - - if (rdev_chain(&rdev, boot_dev, props.offset, props.size)) { - printk(BIOS_ALERT, "ERROR: Failed to access boot region inside boot device\n"); - return -1; - }
int ret = cbfs_locate(fh, &rdev, name, type);
@@ -297,17 +280,13 @@ }
/* The default locator to find the CBFS in the "COREBOOT" FMAP region. */ -int cbfs_default_props(struct cbfs_props *props) +int cbfs_default_region_device(struct region_device *rdev) { - struct region region; - - if (fmap_locate_area("COREBOOT", ®ion)) + if (fmap_locate_area_as_rdev("COREBOOT", rdev)) return -1;
- props->offset = region_offset(®ion); - props->size = region_sz(®ion); - - printk(BIOS_SPEW, "CBFS @ %zx size %zx\n", props->offset, props->size); + printk(BIOS_SPEW, "CBFS @ %zx size %zx\n", + region_device_offset(rdev), region_device_sz(rdev));
return 0; } @@ -317,7 +296,7 @@ * devices. */ const struct cbfs_locator __weak cbfs_default_locator = { .name = "COREBOOT Locator", - .locate = cbfs_default_props, + .locate = cbfs_default_region_device, };
extern const struct cbfs_locator vboot_locator; @@ -334,7 +313,7 @@ &cbfs_default_locator, };
-int cbfs_boot_region_properties(struct cbfs_props *props) +int cbfs_boot_region_device(struct region_device *rdev) { int i;
@@ -348,11 +327,12 @@ if (ops->locate == NULL) continue;
- if (ops->locate(props)) + if (ops->locate(rdev)) continue;
LOG("'%s' located CBFS at [%zx:%zx)\n", - ops->name, props->offset, props->offset + props->size); + ops->name, region_device_offset(rdev), + region_device_end(rdev));
return 0; } diff --git a/src/lib/coreboot_table.c b/src/lib/coreboot_table.c index 241d8e1..7245a63 100644 --- a/src/lib/coreboot_table.c +++ b/src/lib/coreboot_table.c @@ -261,12 +261,12 @@ static void lb_boot_media_params(struct lb_header *header) { struct lb_boot_media_params *bmp; - struct cbfs_props props; const struct region_device *boot_dev; + struct region_device cbfs_dev;
boot_device_init();
- if (cbfs_boot_region_properties(&props)) + if (cbfs_boot_region_device(&cbfs_dev)) return;
boot_dev = boot_device_ro(); @@ -277,8 +277,8 @@ bmp->tag = LB_TAG_BOOT_MEDIA_PARAMS; bmp->size = sizeof(*bmp);
- bmp->cbfs_offset = props.offset; - bmp->cbfs_size = props.size; + bmp->cbfs_offset = region_device_offset(&cbfs_dev); + bmp->cbfs_size = region_device_sz(&cbfs_dev); bmp->boot_media_size = region_device_sz(boot_dev);
bmp->fmap_offset = get_fmap_flash_offset(); diff --git a/src/security/vboot/vboot_loader.c b/src/security/vboot/vboot_loader.c index 3903f18..3e491a7 100644 --- a/src/security/vboot/vboot_loader.c +++ b/src/security/vboot/vboot_loader.c @@ -14,6 +14,7 @@ */
#include <arch/early_variables.h> +#include <boot_device.h> #include <cbfs.h> #include <console/console.h> #include <ec/google/chromeec/ec.h> @@ -71,10 +72,9 @@ } }
-static int vboot_locate(struct cbfs_props *props) +static int vboot_locate(struct region_device *rdev) { const struct vb2_context *ctx; - struct region_device fw_main;
/* Don't honor vboot results until the vboot logic has run. */ if (!vboot_logic_executed()) @@ -85,13 +85,7 @@ if (ctx->flags & VB2_CONTEXT_RECOVERY_MODE) return -1;
- if (vboot_locate_firmware(ctx, &fw_main)) - return -1; - - props->offset = region_device_offset(&fw_main); - props->size = region_device_sz(&fw_main); - - return 0; + return vboot_locate_firmware(ctx, rdev); }
const struct cbfs_locator vboot_locator = { diff --git a/src/soc/intel/apollolake/mmap_boot.c b/src/soc/intel/apollolake/mmap_boot.c index 631a834..96ddc05 100644 --- a/src/soc/intel/apollolake/mmap_boot.c +++ b/src/soc/intel/apollolake/mmap_boot.c @@ -99,28 +99,23 @@ return &real_dev.rdev; }
-static int iafw_boot_region_properties(struct cbfs_props *props) +static int iafw_boot_region_device(struct region_device *rdev) { struct region *real_dev_reg; - struct region regn;
- /* use fmap to locate CBFS area */ - if (fmap_locate_area("COREBOOT", ®n)) + if (cbfs_default_region_device(rdev)) return -1;
- props->offset = region_offset(®n); - props->size = region_sz(®n); - /* 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, ®n)) { + if (region_is_subregion(real_dev_reg, region_device_region(rdev))) { printk(BIOS_DEBUG, "CBFS @ %zx size %zx\n", - props->offset, props->size); + 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", - props->offset, props->size, + region_device_offset(rdev), region_device_sz(rdev), region_offset(real_dev_reg), region_sz(real_dev_reg)); }
@@ -133,5 +128,5 @@ */ const struct cbfs_locator cbfs_default_locator = { .name = "IAFW Locator", - .locate = iafw_boot_region_properties, + .locate = iafw_boot_region_device, };
-
Aaron Durbin (Code Review) -
Julius Werner (Code Review)
-
Patrick Georgi (Code Review)