From efc4ad0bc763f135f3437f51df693e83941a9928 Mon Sep 17 00:00:00 2001 From: Alexandru Gagniuc Date: Wed, 20 Jan 2021 10:46:49 -0600 Subject: [PATCH 1/8] spl: fit: Drop 'length' argument to board_spl_fit_post_load() The size is derived from the FIT image itself. Any alignment requirements are machine-specific and known by the board code. Thus the total length can be derived from the FIT image and knowledge of the platform. The 'length' argument is redundant. Remove it. Signed-off-by: Alexandru Gagniuc Reviewed-by: Peng Fan Reviewed-by: Simon Glass CC: Matt Porter --- arch/arm/mach-imx/spl.c | 7 ++++--- common/spl/spl_fit.c | 4 ++-- include/spl.h | 4 ++-- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/arch/arm/mach-imx/spl.c b/arch/arm/mach-imx/spl.c index be4da0f702..36033d611c 100644 --- a/arch/arm/mach-imx/spl.c +++ b/arch/arm/mach-imx/spl.c @@ -19,6 +19,7 @@ #include #include #include +#include DECLARE_GLOBAL_DATA_PTR; @@ -321,11 +322,11 @@ ulong board_spl_fit_size_align(ulong size) return size; } -void board_spl_fit_post_load(ulong load_addr, size_t length) +void board_spl_fit_post_load(const void *fit) { - u32 offset = length - CONFIG_CSF_SIZE; + u32 offset = ALIGN(fdt_totalsize(fit), 0x1000); - if (imx_hab_authenticate_image(load_addr, + if (imx_hab_authenticate_image((uintptr_t)fit, offset + IVT_SIZE + CSF_PAD_SIZE, offset)) { panic("spl: ERROR: image authentication unsuccessful\n"); diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c index a2612b45a5..bf3731a627 100644 --- a/common/spl/spl_fit.c +++ b/common/spl/spl_fit.c @@ -27,7 +27,7 @@ DECLARE_GLOBAL_DATA_PTR; #define CONFIG_SYS_BOOTM_LEN (64 << 20) #endif -__weak void board_spl_fit_post_load(ulong load_addr, size_t length) +__weak void board_spl_fit_post_load(const void *fit) { } @@ -726,7 +726,7 @@ int spl_load_simple_fit(struct spl_image_info *spl_image, spl_image->flags |= SPL_FIT_FOUND; #ifdef CONFIG_IMX_HAB - board_spl_fit_post_load((ulong)fit, size); + board_spl_fit_post_load(fit); #endif return 0; diff --git a/include/spl.h b/include/spl.h index e172500b5f..0d134587de 100644 --- a/include/spl.h +++ b/include/spl.h @@ -701,9 +701,9 @@ int board_return_to_bootrom(struct spl_image_info *spl_image, /** * board_spl_fit_post_load - allow process images after loading finished - * + * @fit: Pointer to a valid Flattened Image Tree blob */ -void board_spl_fit_post_load(ulong load_addr, size_t length); +void board_spl_fit_post_load(const void *fit); /** * board_spl_fit_size_align - specific size align before processing payload From 917fa369f6fbdc2011e500fbeea0e07ba70af9c3 Mon Sep 17 00:00:00 2001 From: Alexandru Gagniuc Date: Wed, 20 Jan 2021 10:46:50 -0600 Subject: [PATCH 2/8] spl: fit: Factor out FIT parsing and use a context struct The logical steps in spl_load_simple_fit() are difficult to follow. I think the long comments, ifdefs, and ungodly number of variables seriously affect the readability. In particular, it violates section 6 of the coding style, paragraphs (3), and (4). The purpose of this patch is to improve the situation by - Factoring out initialization and parsing to separate functions - Reduce the number of variables by using a context structure This change introduces no functional changes. Signed-off-by: Alexandru Gagniuc Reviewed-by: Simon Glass --- common/spl/spl_fit.c | 90 +++++++++++++++++++++++++++++--------------- 1 file changed, 60 insertions(+), 30 deletions(-) diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c index bf3731a627..91091b651b 100644 --- a/common/spl/spl_fit.c +++ b/common/spl/spl_fit.c @@ -27,6 +27,12 @@ DECLARE_GLOBAL_DATA_PTR; #define CONFIG_SYS_BOOTM_LEN (64 << 20) #endif +struct spl_fit_info { + const void *fit; /* Pointer to a valid FIT blob */ + size_t ext_data_offset; /* Offset to FIT external data (end of FIT) */ + int images_node; /* FDT offset to "/images" node */ +}; + __weak void board_spl_fit_post_load(const void *fit) { } @@ -522,28 +528,22 @@ __weak bool spl_load_simple_fit_skip_processing(void) return false; } -int spl_load_simple_fit(struct spl_image_info *spl_image, - struct spl_load_info *info, ulong sector, void *fit) +static int spl_simple_fit_read(struct spl_fit_info *ctx, + struct spl_load_info *info, ulong sector, + const void *fit_header) { + unsigned long count, size; int sectors; - ulong size, hsize; - unsigned long count; - struct spl_image_info image_info; - int node = -1; - int images, ret; - int base_offset; - int index = 0; - int firmware_node; + void *buf; /* * For FIT with external data, figure out where the external images * start. This is the base for the data-offset properties in each * image. */ - size = fdt_totalsize(fit); - size = (size + 3) & ~3; + size = ALIGN(fdt_totalsize(fit_header), 4); size = board_spl_fit_size_align(size); - base_offset = (size + 3) & ~3; + ctx->ext_data_offset = ALIGN(size, 4); /* * So far we only have one block of data from the FIT. Read the entire @@ -553,36 +553,66 @@ int spl_load_simple_fit(struct spl_image_info *spl_image, * For FIT with external data, data is not loaded in this step. */ sectors = get_aligned_image_size(info, size, 0); - hsize = sectors * info->bl_len; - fit = spl_get_fit_load_buffer(hsize); - count = info->read(info, sector, sectors, fit); + buf = spl_get_fit_load_buffer(sectors * info->bl_len); + + count = info->read(info, sector, sectors, buf); + ctx->fit = buf; debug("fit read sector %lx, sectors=%d, dst=%p, count=%lu, size=0x%lx\n", - sector, sectors, fit, count, size); + sector, sectors, buf, count, size); - if (count == 0) - return -EIO; - - /* skip further processing if requested to enable load-only use cases */ - if (spl_load_simple_fit_skip_processing()) - return 0; + return (count == 0) ? -EIO : 0; +} +static int spl_simple_fit_parse(struct spl_fit_info *ctx) +{ if (IS_ENABLED(CONFIG_SPL_FIT_SIGNATURE)) { - int conf_offset = fit_find_config_node(fit); + int conf_offset = fit_find_config_node(ctx->fit); printf("## Checking hash(es) for config %s ... ", - fit_get_name(fit, conf_offset, NULL)); - if (fit_config_verify(fit, conf_offset)) + fit_get_name(ctx->fit, conf_offset, NULL)); + if (fit_config_verify(ctx->fit, conf_offset)) return -EPERM; puts("OK\n"); } /* find the node holding the images information */ - images = fdt_path_offset(fit, FIT_IMAGES_PATH); - if (images < 0) { - debug("%s: Cannot find /images node: %d\n", __func__, images); - return -1; + ctx->images_node = fdt_path_offset(ctx->fit, FIT_IMAGES_PATH); + if (ctx->images_node < 0) { + debug("%s: Cannot find /images node: %d\n", __func__, + ctx->images_node); + return -EINVAL; } + return 0; +} + +int spl_load_simple_fit(struct spl_image_info *spl_image, + struct spl_load_info *info, ulong sector, void *fit) +{ + struct spl_image_info image_info; + struct spl_fit_info ctx; + int node = -1; + int images, ret; + int base_offset; + int index = 0; + int firmware_node; + + ret = spl_simple_fit_read(&ctx, info, sector, fit); + if (ret < 0) + return ret; + + /* skip further processing if requested to enable load-only use cases */ + if (spl_load_simple_fit_skip_processing()) + return 0; + + ret = spl_simple_fit_parse(&ctx); + if (ret < 0) + return ret; + + images = ctx.images_node; + fit = (void *)ctx.fit; + base_offset = ctx.ext_data_offset; + #ifdef CONFIG_SPL_FPGA node = spl_fit_get_image_node(fit, images, "fpga", 0); if (node >= 0) { From 3dc2079733bba47ad1b72041010edab328bdcef6 Mon Sep 17 00:00:00 2001 From: Alexandru Gagniuc Date: Wed, 20 Jan 2021 10:46:51 -0600 Subject: [PATCH 3/8] spl: fit: Pass FIT context via a structure pointer Several loose arguments describe the FIT image. They are thus related, and it makes sense to pass them together, in a structure. Examples include the FIT blob pointer, offset to FDT nodes, and the offset to external data. Use a spl_fit_info structure to group these parameters. Signed-off-by: Alexandru Gagniuc Reviewed-by: Simon Glass --- common/spl/spl_fit.c | 101 ++++++++++++++++++------------------------- 1 file changed, 43 insertions(+), 58 deletions(-) diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c index 91091b651b..6fad7361bd 100644 --- a/common/spl/spl_fit.c +++ b/common/spl/spl_fit.c @@ -77,7 +77,7 @@ static int find_node_from_desc(const void *fit, int node, const char *str) * * Return: 0 on success, or a negative error number */ -static int spl_fit_get_image_name(const void *fit, int images, +static int spl_fit_get_image_name(const struct spl_fit_info *ctx, const char *type, int index, const char **outname) { @@ -88,21 +88,21 @@ static int spl_fit_get_image_name(const void *fit, int images, int len, i; bool found = true; - conf_node = fit_find_config_node(fit); + conf_node = fit_find_config_node(ctx->fit); if (conf_node < 0) { #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT printf("No matching DT out of these options:\n"); - for (node = fdt_first_subnode(fit, conf_node); + for (node = fdt_first_subnode(ctx->fit, conf_node); node >= 0; - node = fdt_next_subnode(fit, node)) { - name = fdt_getprop(fit, node, "description", &len); + node = fdt_next_subnode(ctx->fit, node)) { + name = fdt_getprop(ctx->fit, node, "description", &len); printf(" %s\n", name); } #endif return conf_node; } - name = fdt_getprop(fit, conf_node, type, &len); + name = fdt_getprop(ctx->fit, conf_node, type, &len); if (!name) { debug("cannot find property '%s': %d\n", type, len); return -EINVAL; @@ -136,11 +136,11 @@ static int spl_fit_get_image_name(const void *fit, int images, * node name. */ int node; - int images = fdt_path_offset(fit, FIT_IMAGES_PATH); + int images = fdt_path_offset(ctx->fit, FIT_IMAGES_PATH); - node = find_node_from_desc(fit, images, str); + node = find_node_from_desc(ctx->fit, images, str); if (node > 0) - str = fdt_get_name(fit, node, NULL); + str = fdt_get_name(ctx->fit, node, NULL); found = true; } @@ -167,20 +167,20 @@ static int spl_fit_get_image_name(const void *fit, int images, * Return: the node offset of the respective image node or a negative * error number. */ -static int spl_fit_get_image_node(const void *fit, int images, +static int spl_fit_get_image_node(const struct spl_fit_info *ctx, const char *type, int index) { const char *str; int err; int node; - err = spl_fit_get_image_name(fit, images, type, index, &str); + err = spl_fit_get_image_name(ctx, type, index, &str); if (err) return err; debug("%s: '%s'\n", type, str); - node = fdt_subnode_offset(fit, images, str); + node = fdt_subnode_offset(ctx->fit, ctx->images_node, str); if (node < 0) { pr_err("cannot find image node '%s': %d\n", str, node); return -EINVAL; @@ -231,10 +231,7 @@ static int get_aligned_image_size(struct spl_load_info *info, int data_size, * spl_load_fit_image(): load the image described in a certain FIT node * @info: points to information about the device to load data from * @sector: the start sector of the FIT image on the device - * @fit: points to the flattened device tree blob describing the FIT - * image - * @base_offset: the beginning of the data area containing the actual - * image data, relative to the beginning of the FIT + * @ctx: points to the FIT context structure * @node: offset of the DT node describing the image to load (relative * to @fit) * @image_info: will be filled with information about the loaded image @@ -245,7 +242,7 @@ static int get_aligned_image_size(struct spl_load_info *info, int data_size, * Return: 0 on success or a negative error number. */ static int spl_load_fit_image(struct spl_load_info *info, ulong sector, - void *fit, ulong base_offset, int node, + const struct spl_fit_info *ctx, int node, struct spl_image_info *image_info) { int offset; @@ -259,6 +256,7 @@ static int spl_load_fit_image(struct spl_load_info *info, ulong sector, int align_len = ARCH_DMA_MINALIGN - 1; uint8_t image_comp = -1, type = -1; const void *data; + const void *fit = ctx->fit; bool external_data = false; if (IS_ENABLED(CONFIG_SPL_FPGA) || @@ -280,7 +278,7 @@ static int spl_load_fit_image(struct spl_load_info *info, ulong sector, if (!fit_image_get_data_position(fit, node, &offset)) { external_data = true; } else if (!fit_image_get_data_offset(fit, node, &offset)) { - offset += base_offset; + offset += ctx->ext_data_offset; external_data = true; } @@ -356,7 +354,7 @@ static int spl_load_fit_image(struct spl_load_info *info, ulong sector, static int spl_fit_append_fdt(struct spl_image_info *spl_image, struct spl_load_info *info, ulong sector, - void *fit, int images, ulong base_offset) + const struct spl_fit_info *ctx) { struct spl_image_info image_info; int node, ret = 0, index = 0; @@ -368,7 +366,7 @@ static int spl_fit_append_fdt(struct spl_image_info *spl_image, image_info.load_addr = spl_image->load_addr + spl_image->size; /* Figure out which device tree the board wants to use */ - node = spl_fit_get_image_node(fit, images, FIT_FDT_PROP, index++); + node = spl_fit_get_image_node(ctx, FIT_FDT_PROP, index++); if (node < 0) { debug("%s: cannot find FDT node\n", __func__); @@ -382,7 +380,7 @@ static int spl_fit_append_fdt(struct spl_image_info *spl_image, else return node; } else { - ret = spl_load_fit_image(info, sector, fit, base_offset, node, + ret = spl_load_fit_image(info, sector, ctx, node, &image_info); if (ret < 0) return ret; @@ -395,8 +393,7 @@ static int spl_fit_append_fdt(struct spl_image_info *spl_image, void *tmpbuffer = NULL; for (; ; index++) { - node = spl_fit_get_image_node(fit, images, FIT_FDT_PROP, - index); + node = spl_fit_get_image_node(ctx, FIT_FDT_PROP, index); if (node == -E2BIG) { debug("%s: No additional FDT node\n", __func__); break; @@ -419,7 +416,7 @@ static int spl_fit_append_fdt(struct spl_image_info *spl_image, __func__); } image_info.load_addr = (ulong)tmpbuffer; - ret = spl_load_fit_image(info, sector, fit, base_offset, + ret = spl_load_fit_image(info, sector, ctx, node, &image_info); if (ret < 0) break; @@ -434,12 +431,12 @@ static int spl_fit_append_fdt(struct spl_image_info *spl_image, (void *)image_info.load_addr); if (ret) { pr_err("failed to apply DT overlay %s\n", - fit_get_name(fit, node, NULL)); + fit_get_name(ctx->fit, node, NULL)); break; } debug("%s: DT overlay %s applied\n", __func__, - fit_get_name(fit, node, NULL)); + fit_get_name(ctx->fit, node, NULL)); } free(tmpbuffer); if (ret) @@ -454,7 +451,7 @@ static int spl_fit_append_fdt(struct spl_image_info *spl_image, return ret; } -static int spl_fit_record_loadable(const void *fit, int images, int index, +static int spl_fit_record_loadable(const struct spl_fit_info *ctx, int index, void *blob, struct spl_image_info *image) { int ret = 0; @@ -462,17 +459,16 @@ static int spl_fit_record_loadable(const void *fit, int images, int index, const char *name; int node; - ret = spl_fit_get_image_name(fit, images, "loadables", - index, &name); + ret = spl_fit_get_image_name(ctx, "loadables", index, &name); if (ret < 0) return ret; - node = spl_fit_get_image_node(fit, images, "loadables", index); + node = spl_fit_get_image_node(ctx, "loadables", index); ret = fdt_record_loadable(blob, index, name, image->load_addr, image->size, image->entry_point, - fdt_getprop(fit, node, "type", NULL), - fdt_getprop(fit, node, "os", NULL)); + fdt_getprop(ctx->fit, node, "type", NULL), + fdt_getprop(ctx->fit, node, "os", NULL)); #endif return ret; } @@ -592,8 +588,7 @@ int spl_load_simple_fit(struct spl_image_info *spl_image, struct spl_image_info image_info; struct spl_fit_info ctx; int node = -1; - int images, ret; - int base_offset; + int ret; int index = 0; int firmware_node; @@ -609,16 +604,11 @@ int spl_load_simple_fit(struct spl_image_info *spl_image, if (ret < 0) return ret; - images = ctx.images_node; - fit = (void *)ctx.fit; - base_offset = ctx.ext_data_offset; - #ifdef CONFIG_SPL_FPGA - node = spl_fit_get_image_node(fit, images, "fpga", 0); + node = spl_fit_get_image_node(&ctx, "fpga", 0); if (node >= 0) { /* Load the image and set up the spl_image structure */ - ret = spl_load_fit_image(info, sector, fit, base_offset, node, - spl_image); + ret = spl_load_fit_image(info, sector, &ctx, node, spl_image); if (ret) { printf("%s: Cannot load the FPGA: %i\n", __func__, ret); return ret; @@ -647,15 +637,14 @@ int spl_load_simple_fit(struct spl_image_info *spl_image, * - fall back to using the first 'loadables' entry */ if (node < 0) - node = spl_fit_get_image_node(fit, images, FIT_FIRMWARE_PROP, - 0); + node = spl_fit_get_image_node(&ctx, FIT_FIRMWARE_PROP, 0); #ifdef CONFIG_SPL_OS_BOOT if (node < 0) - node = spl_fit_get_image_node(fit, images, FIT_KERNEL_PROP, 0); + node = spl_fit_get_image_node(&ctx, FIT_KERNEL_PROP, 0); #endif if (node < 0) { debug("could not find firmware image, trying loadables...\n"); - node = spl_fit_get_image_node(fit, images, "loadables", 0); + node = spl_fit_get_image_node(&ctx, "loadables", 0); /* * If we pick the U-Boot image from "loadables", start at * the second image when later loading additional images. @@ -669,8 +658,7 @@ int spl_load_simple_fit(struct spl_image_info *spl_image, } /* Load the image and set up the spl_image structure */ - ret = spl_load_fit_image(info, sector, fit, base_offset, node, - spl_image); + ret = spl_load_fit_image(info, sector, &ctx, node, spl_image); if (ret) return ret; @@ -678,7 +666,7 @@ int spl_load_simple_fit(struct spl_image_info *spl_image, * For backward compatibility, we treat the first node that is * as a U-Boot image, if no OS-type has been declared. */ - if (!spl_fit_image_get_os(fit, node, &spl_image->os)) + if (!spl_fit_image_get_os(ctx.fit, node, &spl_image->os)) debug("Image OS is %s\n", genimg_get_os_name(spl_image->os)); #if !defined(CONFIG_SPL_OS_BOOT) else @@ -690,8 +678,7 @@ int spl_load_simple_fit(struct spl_image_info *spl_image, * We allow this to fail, as the U-Boot image might embed its FDT. */ if (spl_image->os == IH_OS_U_BOOT) { - ret = spl_fit_append_fdt(spl_image, info, sector, fit, - images, base_offset); + ret = spl_fit_append_fdt(spl_image, info, sector, &ctx); if (!IS_ENABLED(CONFIG_OF_EMBED) && ret < 0) return ret; } @@ -701,7 +688,7 @@ int spl_load_simple_fit(struct spl_image_info *spl_image, for (; ; index++) { uint8_t os_type = IH_OS_INVALID; - node = spl_fit_get_image_node(fit, images, "loadables", index); + node = spl_fit_get_image_node(&ctx, "loadables", index); if (node < 0) break; @@ -713,20 +700,18 @@ int spl_load_simple_fit(struct spl_image_info *spl_image, if (firmware_node == node) continue; - ret = spl_load_fit_image(info, sector, fit, base_offset, node, - &image_info); + ret = spl_load_fit_image(info, sector, &ctx, node, &image_info); if (ret < 0) { printf("%s: can't load image loadables index %d (ret = %d)\n", __func__, index, ret); return ret; } - if (!spl_fit_image_get_os(fit, node, &os_type)) + if (!spl_fit_image_get_os(ctx.fit, node, &os_type)) debug("Loadable is %s\n", genimg_get_os_name(os_type)); if (os_type == IH_OS_U_BOOT) { - spl_fit_append_fdt(&image_info, info, sector, - fit, images, base_offset); + spl_fit_append_fdt(&image_info, info, sector, &ctx); spl_image->fdt_addr = image_info.fdt_addr; } @@ -740,7 +725,7 @@ int spl_load_simple_fit(struct spl_image_info *spl_image, /* Record our loadables into the FDT */ if (spl_image->fdt_addr) - spl_fit_record_loadable(fit, images, index, + spl_fit_record_loadable(&ctx, index, spl_image->fdt_addr, &image_info); } @@ -756,7 +741,7 @@ int spl_load_simple_fit(struct spl_image_info *spl_image, spl_image->flags |= SPL_FIT_FOUND; #ifdef CONFIG_IMX_HAB - board_spl_fit_post_load(fit); + board_spl_fit_post_load(ctx.fit); #endif return 0; From e4928270a4ab758ecfe54c9b296a00470fdee335 Mon Sep 17 00:00:00 2001 From: Alexandru Gagniuc Date: Wed, 20 Jan 2021 10:46:52 -0600 Subject: [PATCH 4/8] spl: fit: Remove useless loop in spl_fit_get_image_name() When a desired configuration is not found, conf_node will have a negative value. Thus the for loop will start at the root "/" node of the image, print the "/description" property, and stop. It appears the intent of the loop was to print the names of the subnodes under "/configurations". We would need the offset to the "/configurations" node, which is abstracted by fit_find_config_node(). This change agrees that abstracting the node offset is the correct design, and we shouldn't be parsing the configurations manually. Thus the loop in spl_fit_get_image_name() is useless. Remove it. Signed-off-by: Alexandru Gagniuc Reviewed-by: Simon Glass --- common/spl/spl_fit.c | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c index 6fad7361bd..78d25e160e 100644 --- a/common/spl/spl_fit.c +++ b/common/spl/spl_fit.c @@ -89,18 +89,8 @@ static int spl_fit_get_image_name(const struct spl_fit_info *ctx, bool found = true; conf_node = fit_find_config_node(ctx->fit); - if (conf_node < 0) { -#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT - printf("No matching DT out of these options:\n"); - for (node = fdt_first_subnode(ctx->fit, conf_node); - node >= 0; - node = fdt_next_subnode(ctx->fit, node)) { - name = fdt_getprop(ctx->fit, node, "description", &len); - printf(" %s\n", name); - } -#endif + if (conf_node < 0) return conf_node; - } name = fdt_getprop(ctx->fit, conf_node, type, &len); if (!name) { From 9e9aa0b473a3217796e7d76a51004af2f42161fd Mon Sep 17 00:00:00 2001 From: Alexandru Gagniuc Date: Wed, 20 Jan 2021 10:46:53 -0600 Subject: [PATCH 5/8] spl: fit: Only look up FIT configuration node once The configuration node a sub node under "/configurations", which describes the components to load from "/images". We only need to locate this node once. However, for each component, spl_fit_get_image_name() would parse the FIT image, looking for the correct node. Such work duplication is not necessary. Instead, once the node is found, cache it, and re-use it. Signed-off-by: Alexandru Gagniuc Reviewed-by: Simon Glass --- common/spl/spl_fit.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c index 78d25e160e..64e4e7ae53 100644 --- a/common/spl/spl_fit.c +++ b/common/spl/spl_fit.c @@ -31,6 +31,7 @@ struct spl_fit_info { const void *fit; /* Pointer to a valid FIT blob */ size_t ext_data_offset; /* Offset to FIT external data (end of FIT) */ int images_node; /* FDT offset to "/images" node */ + int conf_node; /* FDT offset to selected configuration node */ }; __weak void board_spl_fit_post_load(const void *fit) @@ -84,15 +85,10 @@ static int spl_fit_get_image_name(const struct spl_fit_info *ctx, struct udevice *sysinfo; const char *name, *str; __maybe_unused int node; - int conf_node; int len, i; bool found = true; - conf_node = fit_find_config_node(ctx->fit); - if (conf_node < 0) - return conf_node; - - name = fdt_getprop(ctx->fit, conf_node, type, &len); + name = fdt_getprop(ctx->fit, ctx->conf_node, type, &len); if (!name) { debug("cannot find property '%s': %d\n", type, len); return -EINVAL; @@ -551,12 +547,15 @@ static int spl_simple_fit_read(struct spl_fit_info *ctx, static int spl_simple_fit_parse(struct spl_fit_info *ctx) { - if (IS_ENABLED(CONFIG_SPL_FIT_SIGNATURE)) { - int conf_offset = fit_find_config_node(ctx->fit); + /* Find the correct subnode under "/configurations" */ + ctx->conf_node = fit_find_config_node(ctx->fit); + if (ctx->conf_node < 0) + return -EINVAL; + if (IS_ENABLED(CONFIG_SPL_FIT_SIGNATURE)) { printf("## Checking hash(es) for config %s ... ", - fit_get_name(ctx->fit, conf_offset, NULL)); - if (fit_config_verify(ctx->fit, conf_offset)) + fit_get_name(ctx->fit, ctx->conf_node, NULL)); + if (fit_config_verify(ctx->fit, ctx->conf_node)) return -EPERM; puts("OK\n"); } From 9e304239785dce3100303bf3dc211cf544247da0 Mon Sep 17 00:00:00 2001 From: Alexandru Gagniuc Date: Wed, 20 Jan 2021 10:46:54 -0600 Subject: [PATCH 6/8] image: Do not #if guard board_fit_image_post_process() prototype There's no point in guarding function prototypes with #ifdefs. If a function is not defined, the linker will notice. Having the prototype does not affect code size. What the #if guard takes away is the ability to use IS_ENABLED: if (CONFIG_IS ENABLED(FIT_IMAGE_POST_PROCESS)) board_fit_image_post_process(...) When the prototype is guarded, the above form cannot be used. This leads to the proliferation of #ifdefs, and unreadable code. The opportunity cost of the #if guard outweighs any benefits. Remove it. Since the original version of this patch, an empty definition was added by commit f14e6eec6c7f ("image: cleanup pre-processor usage"). The empty definition can cause silent failures, when an implementation of board_fit_image_post_process() is expected because the linker will not catch the missing function. Thus this patch removes this empty inline declaration. Fixes: f14e6eec6c7f ("image: cleanup pre-processor usage") Signed-off-by: Alexandru Gagniuc Reviewed-by: Simon Glass --- include/image.h | 7 ------- 1 file changed, 7 deletions(-) diff --git a/include/image.h b/include/image.h index d5a940313a..138c83dd28 100644 --- a/include/image.h +++ b/include/image.h @@ -1556,8 +1556,6 @@ bool android_image_print_dtb_contents(ulong hdr_addr); */ int board_fit_config_name_match(const char *name); -#if defined(CONFIG_SPL_FIT_IMAGE_POST_PROCESS) || \ - defined(CONFIG_FIT_IMAGE_POST_PROCESS) /** * board_fit_image_post_process() - Do any post-process on FIT binary data * @@ -1572,11 +1570,6 @@ int board_fit_config_name_match(const char *name); * @return no return value (failure should be handled internally) */ void board_fit_image_post_process(void **p_image, size_t *p_size); -#else -static inline void board_fit_image_post_process(void **p_image, size_t *p_size) -{ -} -#endif /* CONFIG_SPL_FIT_IMAGE_POST_PROCESS */ #define FDT_ERROR ((ulong)(-1)) From aeedeae40733131467de72c68e639cf9d795e059 Mon Sep 17 00:00:00 2001 From: Alexandru Gagniuc Date: Wed, 20 Jan 2021 10:46:55 -0600 Subject: [PATCH 7/8] spl: fit: Replace #ifdef blocks with more readable constructs Use the IS_ENABLED() macro to control code flow, instead of the caveman approach of sprinkling #ifdefs. Code size is not affected, as the linker garbage-collects unused functions. However, readability is improved significantly. Signed-off-by: Alexandru Gagniuc Reviewed-by: Simon Glass --- common/spl/spl_fit.c | 53 ++++++++++++++++++++------------------------ 1 file changed, 24 insertions(+), 29 deletions(-) diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c index 64e4e7ae53..50670bfc76 100644 --- a/common/spl/spl_fit.c +++ b/common/spl/spl_fit.c @@ -298,18 +298,16 @@ static int spl_load_fit_image(struct spl_load_info *info, ulong sector, src = (void *)data; } -#ifdef CONFIG_SPL_FIT_SIGNATURE - printf("## Checking hash(es) for Image %s ... ", - fit_get_name(fit, node, NULL)); - if (!fit_image_verify_with_data(fit, node, - src, length)) - return -EPERM; - puts("OK\n"); -#endif + if (CONFIG_IS_ENABLED(FIT_SIGNATURE)) { + printf("## Checking hash(es) for Image %s ... ", + fit_get_name(fit, node, NULL)); + if (!fit_image_verify_with_data(fit, node, src, length)) + return -EPERM; + puts("OK\n"); + } -#ifdef CONFIG_SPL_FIT_IMAGE_POST_PROCESS - board_fit_image_post_process(&src, &length); -#endif + if (CONFIG_IS_ENABLED(FIT_IMAGE_POST_PROCESS)) + board_fit_image_post_process(&src, &length); if (IS_ENABLED(CONFIG_SPL_GZIP) && image_comp == IH_COMP_GZIP) { size = length; @@ -374,7 +372,9 @@ static int spl_fit_append_fdt(struct spl_image_info *spl_image, /* Make the load-address of the FDT available for the SPL framework */ spl_image->fdt_addr = (void *)image_info.load_addr; -#if !CONFIG_IS_ENABLED(FIT_IMAGE_TINY) + if (CONFIG_IS_ENABLED(FIT_IMAGE_TINY)) + return 0; + if (CONFIG_IS_ENABLED(LOAD_FIT_APPLY_OVERLAY)) { void *tmpbuffer = NULL; @@ -432,7 +432,6 @@ static int spl_fit_append_fdt(struct spl_image_info *spl_image, ret = fdt_shrink_to_minimum(spl_image->fdt_addr, 8192); if (ret < 0) return ret; -#endif return ret; } @@ -441,10 +440,12 @@ static int spl_fit_record_loadable(const struct spl_fit_info *ctx, int index, void *blob, struct spl_image_info *image) { int ret = 0; -#if !CONFIG_IS_ENABLED(FIT_IMAGE_TINY) const char *name; int node; + if (CONFIG_IS_ENABLED(FIT_IMAGE_TINY)) + return 0; + ret = spl_fit_get_image_name(ctx, "loadables", index, &name); if (ret < 0) return ret; @@ -455,15 +456,15 @@ static int spl_fit_record_loadable(const struct spl_fit_info *ctx, int index, image->size, image->entry_point, fdt_getprop(ctx->fit, node, "type", NULL), fdt_getprop(ctx->fit, node, "os", NULL)); -#endif return ret; } static int spl_fit_image_get_os(const void *fit, int noffset, uint8_t *os) { -#if CONFIG_IS_ENABLED(FIT_IMAGE_TINY) && !defined(CONFIG_SPL_OS_BOOT) - const char *name = fdt_getprop(fit, noffset, FIT_OS_PROP, NULL); + if (!CONFIG_IS_ENABLED(FIT_IMAGE_TINY) || CONFIG_IS_ENABLED(OS_BOOT)) + return fit_image_get_os(fit, noffset, os); + const char *name = fdt_getprop(fit, noffset, FIT_OS_PROP, NULL); if (!name) return -ENOENT; @@ -478,9 +479,6 @@ static int spl_fit_image_get_os(const void *fit, int noffset, uint8_t *os) *os = IH_OS_INVALID; return 0; -#else - return fit_image_get_os(fit, noffset, os); -#endif } /* @@ -627,10 +625,10 @@ int spl_load_simple_fit(struct spl_image_info *spl_image, */ if (node < 0) node = spl_fit_get_image_node(&ctx, FIT_FIRMWARE_PROP, 0); -#ifdef CONFIG_SPL_OS_BOOT - if (node < 0) + + if (node < 0 && IS_ENABLED(CONFIG_SPL_OS_BOOT)) node = spl_fit_get_image_node(&ctx, FIT_KERNEL_PROP, 0); -#endif + if (node < 0) { debug("could not find firmware image, trying loadables...\n"); node = spl_fit_get_image_node(&ctx, "loadables", 0); @@ -657,10 +655,8 @@ int spl_load_simple_fit(struct spl_image_info *spl_image, */ if (!spl_fit_image_get_os(ctx.fit, node, &spl_image->os)) debug("Image OS is %s\n", genimg_get_os_name(spl_image->os)); -#if !defined(CONFIG_SPL_OS_BOOT) - else + else if (!IS_ENABLED(CONFIG_SPL_OS_BOOT)) spl_image->os = IH_OS_U_BOOT; -#endif /* * Booting a next-stage U-Boot may require us to append the FDT. @@ -729,9 +725,8 @@ int spl_load_simple_fit(struct spl_image_info *spl_image, spl_image->flags |= SPL_FIT_FOUND; -#ifdef CONFIG_IMX_HAB - board_spl_fit_post_load(ctx.fit); -#endif + if (IS_ENABLED(CONFIG_IMX_HAB)) + board_spl_fit_post_load(ctx.fit); return 0; } From 71551055cbdbb059be0e2051aad7976ee9847197 Mon Sep 17 00:00:00 2001 From: Alexandru Gagniuc Date: Wed, 20 Jan 2021 10:46:56 -0600 Subject: [PATCH 8/8] spl: fit: Load devicetree when a Linux payload is found When a FIT config specifies a devicetree, we should load it, no questions asked. In the case of the "simple" FIT loading path, a difficulty arises in selecting the load address of the FDT. The default FDT location is right after the "kernel" or "firmware" image. However, if that is an OP-TEE image, then the FDT may end up in secure DRAM, and not be accessible to normal world kernels. Although the best solution is to be more careful about the FDT address, a viable workaround is to only append the FDT after a u-boot or Linux image. This is identical to the previous logic, except that FDT loading is extended to IH_OS_LINUX images. Signed-off-by: Alexandru Gagniuc --- common/spl/spl_fit.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c index 50670bfc76..75c8ff065b 100644 --- a/common/spl/spl_fit.c +++ b/common/spl/spl_fit.c @@ -336,6 +336,18 @@ static int spl_load_fit_image(struct spl_load_info *info, ulong sector, return 0; } +static bool os_takes_devicetree(uint8_t os) +{ + switch (os) { + case IH_OS_U_BOOT: + return true; + case IH_OS_LINUX: + return IS_ENABLED(CONFIG_SPL_OS_BOOT); + default: + return false; + } +} + static int spl_fit_append_fdt(struct spl_image_info *spl_image, struct spl_load_info *info, ulong sector, const struct spl_fit_info *ctx) @@ -662,9 +674,9 @@ int spl_load_simple_fit(struct spl_image_info *spl_image, * Booting a next-stage U-Boot may require us to append the FDT. * We allow this to fail, as the U-Boot image might embed its FDT. */ - if (spl_image->os == IH_OS_U_BOOT) { + if (os_takes_devicetree(spl_image->os)) { ret = spl_fit_append_fdt(spl_image, info, sector, &ctx); - if (!IS_ENABLED(CONFIG_OF_EMBED) && ret < 0) + if (ret < 0 && spl_image->os != IH_OS_U_BOOT) return ret; } @@ -695,7 +707,7 @@ int spl_load_simple_fit(struct spl_image_info *spl_image, if (!spl_fit_image_get_os(ctx.fit, node, &os_type)) debug("Loadable is %s\n", genimg_get_os_name(os_type)); - if (os_type == IH_OS_U_BOOT) { + if (os_takes_devicetree(os_type)) { spl_fit_append_fdt(&image_info, info, sector, &ctx); spl_image->fdt_addr = image_info.fdt_addr; }