Change in coreboot[master]: cbfstool: Move alignment/baseaddress handling into cbfs_add_component()
Julius Werner has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47823 )
Change subject: cbfstool: Move alignment/baseaddress handling into cbfs_add_component() ......................................................................
cbfstool: Move alignment/baseaddress handling into cbfs_add_component()
The --alignment flag is currently only handled by cbfstool add, but there seems little reason to not handle it for all file-adding commands (the help text actually mentions it for add-stage as well but it doesn't currently work there). This patch moves the related code (and the related baseaddress handling) into cbfs_add_component(). As a nice side effect this allows us to rearrange cbfs_add_component() such that we can conclusively determine whether we need a hash attribute before trying to align the file, allowing that code to correctly infer the final header size even when a hash attribute was implicitly added (for an image built with CBFS verification enabled).
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: Idc6d68b2c7f30e5d136433adb3aec5a87053f992 --- M util/cbfstool/cbfstool.c 1 file changed, 37 insertions(+), 47 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/47823/1
diff --git a/util/cbfstool/cbfstool.c b/util/cbfstool/cbfstool.c index 878db22..68c2c22 100644 --- a/util/cbfstool/cbfstool.c +++ b/util/cbfstool/cbfstool.c @@ -301,7 +301,7 @@ return convert_to_from_absolute_top_aligned(region, offset); }
-static int do_cbfs_locate(int32_t *cbfs_addr, size_t metadata_size, +static int do_cbfs_locate(uint32_t *cbfs_addr, size_t metadata_size, size_t data_size) { if (!param.filename) { @@ -363,7 +363,7 @@ int32_t address = cbfs_locate_entry(&image, data_size, param.pagesize, param.alignment, metadata_size);
- if (address == -1) { + if (address < 0) { ERROR("'%s' can't fit in CBFS for page-size %#x, align %#x.\n", param.name, param.pagesize, param.alignment); return 1; @@ -616,11 +616,16 @@ static int cbfs_add_component(const char *filename, const char *name, uint32_t type, - uint32_t offset, uint32_t headeroffset, convert_buffer_t convert) { size_t len_align = 0; + uint32_t offset = param.baseaddress_assigned ? param.baseaddress : 0; + + if (param.alignment && param.baseaddress_assigned) { + ERROR("Cannot specify both alignment and base address\n"); + return 1; + }
if (!filename) { ERROR("You need to specify -f/--filename.\n"); @@ -652,22 +657,9 @@ return 1; }
- /* - * Check if Intel CPU topswap is specified this will require a - * second bootblock to be added. - */ - if (type == CBFS_TYPE_BOOTBLOCK && param.topswap_size) - if (add_topswap_bootblock(&buffer, &offset)) - return 1; - struct cbfs_file *header = cbfs_create_file_header(type, buffer.size, name);
- if (convert && convert(&buffer, &offset, header) != 0) { - ERROR("Failed to parse file '%s'.\n", filename); - goto error; - } - /* Bootblock and CBFS header should never have file hashes. When adding the bootblock it is important that we *don't* look up the metadata hash yet (before it is added) or we'll cache an outdated result. */ @@ -683,12 +675,31 @@ goto error; } } + }
- if (param.hash != VB2_HASH_INVALID && - cbfs_add_file_hash(header, &buffer, param.hash) == -1) { - ERROR("couldn't add hash for '%s'\n", name); + /* This needs to run after potentially updating param.hash above. */ + if (param.alignment) + if (do_cbfs_locate(&offset, 0, 0)) goto error; - } + + /* + * Check if Intel CPU topswap is specified this will require a + * second bootblock to be added. + */ + if (type == CBFS_TYPE_BOOTBLOCK && param.topswap_size) + if (add_topswap_bootblock(&buffer, &offset)) + goto error; + + if (convert && convert(&buffer, &offset, header) != 0) { + ERROR("Failed to parse file '%s'.\n", filename); + goto error; + } + + /* This needs to run after convert(). */ + if (param.hash != VB2_HASH_INVALID && + cbfs_add_file_hash(header, &buffer, param.hash) == -1) { + ERROR("couldn't add hash for '%s'\n", name); + goto error; }
if (param.autogen_attr) { @@ -700,7 +711,7 @@ CBFS_FILE_ATTR_TAG_POSITION, sizeof(struct cbfs_file_attr_position)); if (attrs == NULL) - return -1; + goto error; attrs->position = htonl(offset); } /* Add alignment attribute if used */ @@ -711,7 +722,7 @@ CBFS_FILE_ATTR_TAG_ALIGNMENT, sizeof(struct cbfs_file_attr_align)); if (attrs == NULL) - return -1; + goto error; attrs->alignment = htonl(param.alignment); } } @@ -722,7 +733,7 @@ CBFS_FILE_ATTR_TAG_IBB, sizeof(struct cbfs_file_attribute)); if (attrs == NULL) - return -1; + goto error; /* For Intel TXT minimum align is 16 */ len_align = 16; } @@ -736,7 +747,7 @@ header, CBFS_FILE_ATTR_TAG_PADDING, size); if (attr == NULL) - return -1; + goto error; }
if (IS_TOP_ALIGNED_ADDRESS(offset)) @@ -892,7 +903,7 @@ return -1;
if (param.stage_xip) { - int32_t address; + uint32_t address; size_t data_size;
if (elf_program_file_size(buffer, &data_size) < 0) { @@ -1027,16 +1038,7 @@
static int cbfs_add(void) { - int32_t address; - convert_buffer_t convert; - uint32_t local_baseaddress = param.baseaddress; - - if (param.alignment && param.baseaddress) { - ERROR("Cannot specify both alignment and base address\n"); - return 1; - } - - convert = cbfstool_convert_raw; + convert_buffer_t convert = cbfstool_convert_raw;
/* Set the alignment to 4KiB minimum for FSP blobs when no base address * is provided so that relocation can occur. */ @@ -1049,18 +1051,9 @@ return 1; }
- if (param.alignment) { - /* CBFS compression file attribute is unconditionally added. */ - size_t metadata_sz = sizeof(struct cbfs_file_attr_compression); - if (do_cbfs_locate(&address, metadata_sz, 0)) - return 1; - local_baseaddress = address; - } - return cbfs_add_component(param.filename, param.name, param.type, - local_baseaddress, param.headeroffset, convert); } @@ -1082,7 +1075,6 @@ return cbfs_add_component(param.filename, param.name, CBFS_TYPE_STAGE, - param.baseaddress, param.headeroffset, cbfstool_convert_mkstage); } @@ -1092,7 +1084,6 @@ return cbfs_add_component(param.filename, param.name, CBFS_TYPE_SELF, - param.baseaddress, param.headeroffset, cbfstool_convert_mkpayload); } @@ -1112,7 +1103,6 @@ return cbfs_add_component(param.filename, param.name, CBFS_TYPE_SELF, - param.baseaddress, param.headeroffset, cbfstool_convert_mkflatpayload); }
Hello Patrick Georgi, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47823
to look at the new patch set (#2).
Change subject: cbfstool: Move alignment/baseaddress handling into cbfs_add_component() ......................................................................
cbfstool: Move alignment/baseaddress handling into cbfs_add_component()
The --alignment flag is currently only handled by cbfstool add, but there seems little reason to not handle it for all file-adding commands (the help text actually mentions it for add-stage as well but it doesn't currently work there). This patch moves the related code (and the related baseaddress handling) into cbfs_add_component(). As a nice side effect this allows us to rearrange cbfs_add_component() such that we can conclusively determine whether we need a hash attribute before trying to align the file, allowing that code to correctly infer the final header size even when a hash attribute was implicitly added (for an image built with CBFS verification enabled).
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: Idc6d68b2c7f30e5d136433adb3aec5a87053f992 --- M util/cbfstool/cbfstool.c 1 file changed, 37 insertions(+), 47 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/47823/2
Attention is currently required from: Furquan Shaikh. Hello build bot (Jenkins), Patrick Georgi, Furquan Shaikh, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47823
to look at the new patch set (#6).
Change subject: cbfstool: Move alignment/baseaddress handling into cbfs_add_component() ......................................................................
cbfstool: Move alignment/baseaddress handling into cbfs_add_component()
The --alignment flag is currently only handled by cbfstool add, but there seems little reason to not handle it for all file-adding commands (the help text actually mentions it for add-stage as well but it doesn't currently work there). This patch moves the related code (and the related baseaddress handling) into cbfs_add_component(). As a nice side effect this allows us to rearrange cbfs_add_component() such that we can conclusively determine whether we need a hash attribute before trying to align the file, allowing that code to correctly infer the final header size even when a hash attribute was implicitly added (for an image built with CBFS verification enabled).
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: Idc6d68b2c7f30e5d136433adb3aec5a87053f992 --- M util/cbfstool/cbfstool.c 1 file changed, 37 insertions(+), 47 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/47823/6
Attention is currently required from: Furquan Shaikh, Julius Werner. Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47823
to look at the new patch set (#7).
Change subject: cbfstool: Move alignment/baseaddress handling into cbfs_add_component() ......................................................................
cbfstool: Move alignment/baseaddress handling into cbfs_add_component()
The --alignment flag is currently only handled by cbfstool add, but there seems little reason to not handle it for all file-adding commands (the help text actually mentions it for add-stage as well but it doesn't currently work there). This patch moves the related code (and the related baseaddress handling) into cbfs_add_component(). As a nice side effect this allows us to rearrange cbfs_add_component() such that we can conclusively determine whether we need a hash attribute before trying to align the file, allowing that code to correctly infer the final header size even when a hash attribute was implicitly added (for an image built with CBFS verification enabled).
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: Idc6d68b2c7f30e5d136433adb3aec5a87053f992 --- M util/cbfstool/cbfstool.c 1 file changed, 56 insertions(+), 61 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/47823/7
Attention is currently required from: Furquan Shaikh, Julius Werner. Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47823
to look at the new patch set (#8).
Change subject: cbfstool: Move alignment/baseaddress handling into cbfs_add_component() ......................................................................
cbfstool: Move alignment/baseaddress handling into cbfs_add_component()
The --alignment flag is currently only handled by cbfstool add, but there seems little reason to not handle it for all file-adding commands (the help text actually mentions it for add-stage as well but it doesn't currently work there). This patch moves the related code (and the related baseaddress handling) into cbfs_add_component(). As a nice side effect this allows us to rearrange cbfs_add_component() such that we can conclusively determine whether we need a hash attribute before trying to align the file, allowing that code to correctly infer the final header size even when a hash attribute was implicitly added (for an image built with CBFS verification enabled).
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: Idc6d68b2c7f30e5d136433adb3aec5a87053f992 --- M util/cbfstool/cbfstool.c 1 file changed, 59 insertions(+), 61 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/47823/8
Attention is currently required from: Furquan Shaikh, Julius Werner. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47823 )
Change subject: cbfstool: Move alignment/baseaddress handling into cbfs_add_component() ......................................................................
Patch Set 8:
(1 comment)
File util/cbfstool/cbfstool.c:
https://review.coreboot.org/c/coreboot/+/47823/comment/671d5dfe_19ee8824 PS8, Line 561: if (param.precompression || param.compression != CBFS_COMPRESS_NONE) { braces {} are not necessary for single statement blocks
Attention is currently required from: Furquan Shaikh, Julius Werner. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47823 )
Change subject: cbfstool: Move alignment/baseaddress handling into cbfs_add_component() ......................................................................
Patch Set 9:
(1 comment)
File util/cbfstool/cbfstool.c:
https://review.coreboot.org/c/coreboot/+/47823/comment/5823ce3a_20c90c10 PS9, Line 561: if (param.precompression || param.compression != CBFS_COMPRESS_NONE) { braces {} are not necessary for single statement blocks
Attention is currently required from: Furquan Shaikh, Julius Werner. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47823 )
Change subject: cbfstool: Move alignment/baseaddress handling into cbfs_add_component() ......................................................................
Patch Set 10:
(1 comment)
File util/cbfstool/cbfstool.c:
https://review.coreboot.org/c/coreboot/+/47823/comment/295cbccc_3ada89ea PS10, Line 561: if (param.precompression || param.compression != CBFS_COMPRESS_NONE) { braces {} are not necessary for single statement blocks
Attention is currently required from: Furquan Shaikh, Julius Werner. Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47823
to look at the new patch set (#11).
Change subject: cbfstool: Move alignment/baseaddress handling into cbfs_add_component() ......................................................................
cbfstool: Move alignment/baseaddress handling into cbfs_add_component()
The --alignment flag is currently only handled by cbfstool add, but there seems little reason to not handle it for all file-adding commands (the help text actually mentions it for add-stage as well but it doesn't currently work there). This patch moves the related code (and the related baseaddress handling) into cbfs_add_component(). As a nice side effect this allows us to rearrange cbfs_add_component() such that we can conclusively determine whether we need a hash attribute before trying to align the file, allowing that code to correctly infer the final header size even when a hash attribute was implicitly added (for an image built with CBFS verification enabled).
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: Idc6d68b2c7f30e5d136433adb3aec5a87053f992 --- M util/cbfstool/cbfstool.c 1 file changed, 58 insertions(+), 61 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/47823/11
Attention is currently required from: Furquan Shaikh, Julius Werner. Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47823 )
Change subject: cbfstool: Move alignment/baseaddress handling into cbfs_add_component() ......................................................................
Patch Set 20: Code-Review+2
Julius Werner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/47823 )
Change subject: cbfstool: Move alignment/baseaddress handling into cbfs_add_component() ......................................................................
cbfstool: Move alignment/baseaddress handling into cbfs_add_component()
The --alignment flag is currently only handled by cbfstool add, but there seems little reason to not handle it for all file-adding commands (the help text actually mentions it for add-stage as well but it doesn't currently work there). This patch moves the related code (and the related baseaddress handling) into cbfs_add_component(). As a nice side effect this allows us to rearrange cbfs_add_component() such that we can conclusively determine whether we need a hash attribute before trying to align the file, allowing that code to correctly infer the final header size even when a hash attribute was implicitly added (for an image built with CBFS verification enabled).
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: Idc6d68b2c7f30e5d136433adb3aec5a87053f992 Reviewed-on: https://review.coreboot.org/c/coreboot/+/47823 Reviewed-by: Patrick Georgi pgeorgi@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M util/cbfstool/cbfstool.c 1 file changed, 58 insertions(+), 61 deletions(-)
Approvals: build bot (Jenkins): Verified Patrick Georgi: Looks good to me, approved
diff --git a/util/cbfstool/cbfstool.c b/util/cbfstool/cbfstool.c index 16654d9..3e80712 100644 --- a/util/cbfstool/cbfstool.c +++ b/util/cbfstool/cbfstool.c @@ -505,7 +505,7 @@ return 0; }
-static int do_cbfs_locate(int32_t *cbfs_addr, size_t metadata_size, +static int do_cbfs_locate(uint32_t *cbfs_addr, size_t metadata_size, size_t data_size) { if (!param.filename) { @@ -559,6 +559,8 @@ if (param.baseaddress_assigned || param.stage_xip) metadata_size += sizeof(struct cbfs_file_attr_position); } + if (param.precompression || param.compression != CBFS_COMPRESS_NONE) + metadata_size += sizeof(struct cbfs_file_attr_compression);
/* Take care of the hash attribute if it is used */ if (param.hash != VB2_HASH_INVALID) @@ -567,7 +569,7 @@ int32_t address = cbfs_locate_entry(&image, data_size, param.pagesize, param.alignment, metadata_size);
- if (address == -1) { + if (address < 0) { ERROR("'%s' can't fit in CBFS for page-size %#x, align %#x.\n", param.name, param.pagesize, param.alignment); return 1; @@ -813,11 +815,16 @@ static int cbfs_add_component(const char *filename, const char *name, uint32_t type, - uint32_t offset, uint32_t headeroffset, convert_buffer_t convert) { size_t len_align = 0; + uint32_t offset = param.baseaddress_assigned ? param.baseaddress : 0; + + if (param.alignment && param.baseaddress_assigned) { + ERROR("Cannot specify both alignment and base address\n"); + return 1; + }
if (!filename) { ERROR("You need to specify -f/--filename.\n"); @@ -849,22 +856,9 @@ return 1; }
- /* - * Check if Intel CPU topswap is specified this will require a - * second bootblock to be added. - */ - if (type == CBFS_TYPE_BOOTBLOCK && param.topswap_size) - if (add_topswap_bootblock(&buffer, &offset)) - return 1; - struct cbfs_file *header = cbfs_create_file_header(type, buffer.size, name);
- if (convert && convert(&buffer, &offset, header) != 0) { - ERROR("Failed to parse file '%s'.\n", filename); - goto error; - } - /* Bootblock and CBFS header should never have file hashes. When adding the bootblock it is important that we *don't* look up the metadata hash yet (before it is added) or we'll cache an outdated result. */ @@ -880,12 +874,31 @@ goto error; } } + }
- if (param.hash != VB2_HASH_INVALID && - cbfs_add_file_hash(header, &buffer, param.hash) == -1) { - ERROR("couldn't add hash for '%s'\n", name); + /* This needs to run after potentially updating param.hash above. */ + if (param.alignment) + if (do_cbfs_locate(&offset, 0, 0)) goto error; - } + + /* + * Check if Intel CPU topswap is specified this will require a + * second bootblock to be added. + */ + if (type == CBFS_TYPE_BOOTBLOCK && param.topswap_size) + if (add_topswap_bootblock(&buffer, &offset)) + goto error; + + if (convert && convert(&buffer, &offset, header) != 0) { + ERROR("Failed to parse file '%s'.\n", filename); + goto error; + } + + /* This needs to run after convert(). */ + if (param.hash != VB2_HASH_INVALID && + cbfs_add_file_hash(header, &buffer, param.hash) == -1) { + ERROR("couldn't add hash for '%s'\n", name); + goto error; }
if (param.autogen_attr) { @@ -897,7 +910,7 @@ CBFS_FILE_ATTR_TAG_POSITION, sizeof(struct cbfs_file_attr_position)); if (attrs == NULL) - return -1; + goto error; /* If we add a stage or a payload, we need to take */ /* care about the additional metadata that is added */ /* to the cbfs file and therefore set the position */ @@ -917,7 +930,7 @@ CBFS_FILE_ATTR_TAG_ALIGNMENT, sizeof(struct cbfs_file_attr_align)); if (attrs == NULL) - return -1; + goto error; attrs->alignment = htonl(param.alignment); } } @@ -928,7 +941,7 @@ CBFS_FILE_ATTR_TAG_IBB, sizeof(struct cbfs_file_attribute)); if (attrs == NULL) - return -1; + goto error; /* For Intel TXT minimum align is 16 */ len_align = 16; } @@ -943,7 +956,7 @@ header, CBFS_FILE_ATTR_TAG_PADDING, size); if (attr == NULL) - return -1; + goto error; }
if (IS_HOST_SPACE_ADDRESS(offset)) @@ -1089,29 +1102,34 @@ struct cbfs_file *header) { struct buffer output; + size_t data_size; int ret;
+ if (elf_program_file_size(buffer, &data_size) < 0) { + ERROR("Could not obtain ELF size\n"); + return 1; + } + + /* + * If we already did a locate for alignment we need to locate again to + * take the stage header into account. XIP stage parsing also needs the + * location. But don't locate in other cases, because it will ignore + * compression (not applied yet) and thus may cause us to refuse adding + * stages that would actually fit once compressed. + */ + if ((param.alignment || param.stage_xip) && + do_cbfs_locate(offset, sizeof(struct cbfs_stage), data_size)) { + ERROR("Could not find location for stage.\n"); + return 1; + } + if (param.stage_xip) { - int32_t address; - size_t data_size; - - if (elf_program_file_size(buffer, &data_size) < 0) { - ERROR("Could not obtain ELF size\n"); - return 1; - } - - if (do_cbfs_locate(&address, sizeof(struct cbfs_stage), - data_size)) { - ERROR("Could not find location for XIP stage.\n"); - return 1; - } - /* * Ensure the address is a memory mapped one. This assumes * x86 semantics about the boot media being directly mapped * below 4GiB in the CPU address space. **/ - *offset = convert_addr_space(param.image_region, address); + *offset = convert_addr_space(param.image_region, *offset);
ret = parse_elf_to_xip_stage(buffer, &output, offset, param.ignore_section); @@ -1186,16 +1204,7 @@
static int cbfs_add(void) { - int32_t address; - convert_buffer_t convert; - uint32_t local_baseaddress = param.baseaddress; - - if (param.alignment && param.baseaddress) { - ERROR("Cannot specify both alignment and base address\n"); - return 1; - } - - convert = cbfstool_convert_raw; + convert_buffer_t convert = cbfstool_convert_raw;
/* Set the alignment to 4KiB minimum for FSP blobs when no base address * is provided so that relocation can occur. */ @@ -1208,18 +1217,9 @@ return 1; }
- if (param.alignment) { - /* CBFS compression file attribute is unconditionally added. */ - size_t metadata_sz = sizeof(struct cbfs_file_attr_compression); - if (do_cbfs_locate(&address, metadata_sz, 0)) - return 1; - local_baseaddress = address; - } - return cbfs_add_component(param.filename, param.name, param.type, - local_baseaddress, param.headeroffset, convert); } @@ -1241,7 +1241,6 @@ return cbfs_add_component(param.filename, param.name, CBFS_TYPE_STAGE, - param.baseaddress, param.headeroffset, cbfstool_convert_mkstage); } @@ -1251,7 +1250,6 @@ return cbfs_add_component(param.filename, param.name, CBFS_TYPE_SELF, - param.baseaddress, param.headeroffset, cbfstool_convert_mkpayload); } @@ -1271,7 +1269,6 @@ return cbfs_add_component(param.filename, param.name, CBFS_TYPE_SELF, - param.baseaddress, param.headeroffset, cbfstool_convert_mkflatpayload); }
-
build bot (Jenkins) (Code Review) -
Julius Werner (Code Review)
-
Patrick Georgi (Code Review)