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