Attention is currently required from: Raul Rangel, Patrick Georgi, Tim Wawrzynczak. Hello Raul Rangel, Patrick Georgi, Tim Wawrzynczak,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/59877
to review the following change.
Change subject: cbfstool: Fix offset calculation for aligned files ......................................................................
cbfstool: Fix offset calculation for aligned files
The placement calculation logic in cbfs_add_component() has become quite a mess, and this patch can only fix that to a limited degree. The interaction between all the different pathways of how the `offset` variable can be set and at what point exactly the final placement offset is decided can get quite convoluted. In particular, one existing problem is that the offset for a file added with the --align flag is decided before the convert() function is called, which may change the form (and thereby the size) of the file again after its location was found -- resulting in a location that ends up being too small, or being unable to find a location for a file that should fit. This used to be okay under the assumption that forced alignment should really only be necessary for use cases like XIP where the file is directly "used" straight from its location on flash in some way, and those cases can never be compressed -- however, recent AMD platforms have started using the --align flag to meet the requirements of their SPI DMA controller and broken this assumption.
This patch fixes that particular problem and hopefully eliminates a bit of the convolution by moving the offset decision point in the --align case after the convert() step. This is safe when the steps in-between (add_topswap_bootblock() and convert() itself) do not rely on the location having already been decided by --align before that point. For the topswap case this is easy, because in practice we always call it with --base-address (and as far as I can tell that's the only way it was ever meant to work?) -- so codify that assumption in the function. For convert() this mostly means that the implementations that do touch the offset variable (mkstage and FSP) need to ensure they take care of the alignment themselves. The FSP case is particularly complex so I tried to rewrite the code in a slightly more straight-forward way and clearly document the supported cases, which should hopefully make it easier to see that the offset variable is handled correctly in all of them. For mkstage the best solution seems to be to only have it touch the offset variable in the XIP case (where we know compression must be disabled, so we can rely on it not changing the file size later), and have the extra space for the stage header directly taken care of by do_cbfs_locate() so that can happen after convert().
NOTE: This is changing the behavior of `cbfstool add -t fsp` when neither --base-address nor --xip are passed (e.g. FSP-S). Previously, cbfstool would implicitly force an alignment of 4K. As far as I can tell from the comments, this is unnecessary because this binary is loaded into RAM and CBFS placement does not matter, so I assume this is an oversight caused by accidentally reusing code that was only meant for the XIP case.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: Ia49a585988f7a74944a6630b77b3ebd79b3a9897 --- M util/cbfstool/cbfstool.c 1 file changed, 82 insertions(+), 67 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/59877/1
diff --git a/util/cbfstool/cbfstool.c b/util/cbfstool/cbfstool.c index c9db45a..27a6952 100644 --- a/util/cbfstool/cbfstool.c +++ b/util/cbfstool/cbfstool.c @@ -507,9 +507,10 @@ return 0; }
-static int do_cbfs_locate(uint32_t *cbfs_addr, size_t metadata_size, - size_t data_size) +static int do_cbfs_locate(uint32_t *cbfs_addr, size_t data_size) { + uint32_t metadata_size = 0; + if (!param.filename) { ERROR("You need to specify -f/--filename.\n"); return 1; @@ -559,6 +560,8 @@ } if (param.precompression || param.compression != CBFS_COMPRESS_NONE) metadata_size += sizeof(struct cbfs_file_attr_compression); + if (param.type == CBFS_TYPE_STAGE) + metadata_size += sizeof(struct cbfs_file_attr_stageheader);
/* Take care of the hash attribute if it is used */ if (param.hash != VB2_HASH_INVALID) @@ -776,6 +779,11 @@ { size_t bb_buf_size = buffer_size(buffer);
+ if (!param.baseaddress_assigned) { + ERROR("--topswap-size must also have --base-address\n"); + return 1; + } + if (bb_buf_size > param.topswap_size) { ERROR("Bootblock bigger than the topswap boundary\n"); ERROR("size = %zd, ts = %d\n", bb_buf_size, @@ -812,18 +820,37 @@
static int cbfs_add_component(const char *filename, const char *name, - uint32_t type, uint32_t headeroffset, convert_buffer_t convert) { - size_t len_align = 0; + /* + * The steps used to determine the final placement offset in CBFS, in order: + * + * 1. If --base-address was passed, that value is used. + * + * 2. The convert() function may write a location back to |offset|, usually by calling + * do_cbfs_locate(). In this case, it needs to ensure that the location found can fit + * the CBFS file in its final form (after any compression and conversion). + * + * 3. If --align was passed and the offset is still undecided at this point, + * do_cbfs_locate() is called to find an appropriately aligned location. + * + * 4. If |offset| is still 0 at the end, cbfs_add_entry() will find the first available + * location that fits. + */ uint32_t offset = param.baseaddress_assigned ? param.baseaddress : 0; + size_t len_align = 0;
if (param.alignment && param.baseaddress_assigned) { ERROR("Cannot specify both alignment and base address\n"); return 1; }
+ if (param.stage_xip && param.compression != CBFS_COMPRESS_NONE) { + ERROR("Cannot specify compression for XIP.\n"); + return 1; + } + if (!filename) { ERROR("You need to specify -f/--filename.\n"); return 1; @@ -834,7 +861,7 @@ return 1; }
- if (type == 0) { + if (param.type == 0) { ERROR("You need to specify a valid -t/--type.\n"); return 1; } @@ -855,12 +882,12 @@ }
struct cbfs_file *header = - cbfs_create_file_header(type, buffer.size, name); + cbfs_create_file_header(param.type, buffer.size, name);
/* 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. */ - if (type != CBFS_TYPE_BOOTBLOCK && type != CBFS_TYPE_CBFSHEADER) { + if (param.type != CBFS_TYPE_BOOTBLOCK && param.type != CBFS_TYPE_CBFSHEADER) { enum vb2_hash_algorithm mh_algo = get_mh_cache()->cbfs_hash.algo; if (mh_algo != VB2_HASH_INVALID && param.hash != mh_algo) { if (param.hash == VB2_HASH_INVALID) { @@ -874,16 +901,11 @@ } }
- /* 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 (param.type == CBFS_TYPE_BOOTBLOCK && param.topswap_size) if (add_topswap_bootblock(&buffer, &offset)) goto error;
@@ -892,7 +914,12 @@ goto error; }
- /* This needs to run after convert(). */ + /* This needs to run after convert() to take compression into account. */ + if (!offset && param.alignment) + if (do_cbfs_locate(&offset, 0)) + goto error; + + /* This needs to run after convert() to hash the actual final file data. */ if (param.hash != VB2_HASH_INVALID && cbfs_add_file_hash(header, &buffer, param.hash) == -1) { ERROR("couldn't add hash for '%s'\n", name); @@ -1028,45 +1055,44 @@ { uint32_t address; struct buffer fsp; - int do_relocation = 1; - - address = *offset;
/* - * If the FSP component is xip, then ensure that the address is a memory - * mapped one. - * If the FSP component is not xip, then use param.baseaddress that is - * passed in by the caller. + * There are 4 different cases here: + * + * 1. --xip and --base-address: we need to place the binary at the given base address + * in the CBFS image and relocate it to that address. *offset was already filled in. + * + * 2. --xip but no --base-address: we implicitly force a 4K minimum alignment so that + * relocation can occur. Call do_cbfs_locate() here to find an appropriate *offset. + * + * 3. No --xip but a --base-address: special case where --base-address does not have its + * normal meaning, instead we use it as the relocation target address. We explicitly + * reset *offset to 0 so that the file will be placed wherever it fits in CBFS. + * + * 4. No --xip and no --base-address: this means that the FSP was pre-linked and should + * not be relocated. Just chain directly to convert_raw() for compression. */ + if (param.stage_xip) { - if (!IS_HOST_SPACE_ADDRESS(address)) - address = convert_addr_space(param.image_region, address); + if (!param.baseaddress_assigned) { + param.alignment = 4*1024; + if (do_cbfs_locate(offset, 0)) + return -1; + } + if (!IS_HOST_SPACE_ADDRESS(*offset)) + address = convert_addr_space(param.image_region, *offset); + else + address = *offset; } else { if (param.baseaddress_assigned == 0) { - INFO("Honoring pre-linked FSP module.\n"); - do_relocation = 0; + INFO("Honoring pre-linked FSP module, no relocation.\n"); + return cbfstool_convert_raw(buffer, offset, header); } else { address = param.baseaddress; - /* - * *offset should either be 0 or the value returned by - * do_cbfs_locate. do_cbfs_locate is called only when param.baseaddress - * is not provided by user. Thus, set *offset to 0 if user provides - * a baseaddress i.e. params.baseaddress_assigned is set. The only - * requirement in this case is that the binary should be relocated to - * the base address that is requested. There is no requirement on where - * the file ends up in the cbfs. - */ *offset = 0; } }
- /* - * Nothing left to do if relocation is not being attempted. Just add - * the file. - */ - if (!do_relocation) - return cbfstool_convert_raw(buffer, offset, header); - /* Create a copy of the buffer to attempt relocation. */ if (buffer_create(&fsp, buffer_size(buffer), "fsp")) return -1; @@ -1100,15 +1126,12 @@ }
/* - * 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. + * We need a final location for XIP parsing, so we need to call do_cbfs_locate() early + * here. That is okay because XIP stages may not be compressed, so their size cannot + * change anymore at a later point. */ - if ((param.alignment || param.stage_xip) && - do_cbfs_locate(offset, sizeof(struct cbfs_file_attr_stageheader), - data_size)) { + if (param.stage_xip && + do_cbfs_locate(offset, data_size)) { ERROR("Could not find location for stage.\n"); return 1; } @@ -1240,50 +1263,42 @@ { 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. */ if (param.type == CBFS_TYPE_FSP) { if (!param.baseaddress_assigned) - param.alignment = 4*1024; convert = cbfstool_convert_fsp; + } else if (param.type == CBFS_TYPE_STAGE) { + ERROR("stages can only be added with cbfstool add-stage\n"); + return 1; } else if (param.stage_xip) { - ERROR("cbfs add supports xip only for FSP component type\n"); + ERROR("cbfstool add supports xip only for FSP component type\n"); return 1; }
return cbfs_add_component(param.filename, param.name, - param.type, param.headeroffset, convert); }
static int cbfs_add_stage(void) { - if (param.stage_xip) { - if (param.baseaddress_assigned) { - ERROR("Cannot specify base address for XIP.\n"); - return 1; - } - - if (param.compression != CBFS_COMPRESS_NONE) { - ERROR("Cannot specify compression for XIP.\n"); - return 1; - } + if (param.stage_xip && param.baseaddress_assigned) { + ERROR("Cannot specify base address for XIP.\n"); + return 1; } + param.type = CBFS_TYPE_STAGE;
return cbfs_add_component(param.filename, param.name, - CBFS_TYPE_STAGE, param.headeroffset, cbfstool_convert_mkstage); }
static int cbfs_add_payload(void) { + param.type = CBFS_TYPE_SELF; return cbfs_add_component(param.filename, param.name, - CBFS_TYPE_SELF, param.headeroffset, cbfstool_convert_mkpayload); } @@ -1300,9 +1315,9 @@ "-e/--entry-point.\n"); return 1; } + param.type = CBFS_TYPE_SELF; return cbfs_add_component(param.filename, param.name, - CBFS_TYPE_SELF, param.headeroffset, cbfstool_convert_mkflatpayload); }
-
Julius Werner (Code Review)