From 11c0a4ff08b0789e5ba7002a9161db1763b90dbd Mon Sep 17 00:00:00 2001 From: Masahiro Yamada Date: Sat, 14 Jan 2017 23:22:02 +0900 Subject: [PATCH] fiptool: fix add_image() and add_image_desc() implementation The "make fip" shows the content of the generated FIP at the end of the build. (This is shown by "fiptool info" command.) Prior to commit e0f083a09b29 ("fiptool: Prepare ground for expanding the set of images at runtime"), the last part of the build log of make CROSS_COMPILE=aarch64-linux-gnu- BL33=../u-boot/u-boot.bin fip was like follows: Trusted Boot Firmware BL2: offset=0xB0, size=0x4188, cmdline="--tb-fw" EL3 Runtime Firmware BL31: offset=0x4238, size=0x6090, cmdline="--soc-fw" Non-Trusted Firmware BL33: offset=0xA2C8, size=0x58B51, cmdline="--nt-fw" With that commit, now it is displayed like follows: Non-Trusted Firmware BL33: offset=0xB0, size=0x58B51, cmdline="--nt-fw" EL3 Runtime Firmware BL31: offset=0x58C01, size=0x6090, cmdline="--soc-fw" Trusted Boot Firmware BL2: offset=0x5EC91, size=0x4188, cmdline="--tb-fw" You will notice two differences: - the contents are displayed in BL33, BL31, BL2 order - the offset values are wrong The latter is more serious, and means "fiptool info" is broken. Another interesting change is "fiptool update" every time reverses the image order. For example, if you input FIP with BL2, BL31, BL33 in this order, the command will pack BL33, BL31, BL2 into FIP, in this order. Of course, the order of components is not a big deal except that users will have poor impression about this. The root cause is in the implementation of add_image(); the image_head points to the last added image. For example, if you call add_image() for BL2, BL31, BL33 in this order, the resulted image chain is: image_head -> BL33 -> BL31 -> BL2 Then, they are processed from the image_head in "for" loops: for (image = image_head; image != NULL; image = image->next) { This means images are handled in Last-In First-Out manner. Interestingly, "fiptool create" is still correct because add_image_desc() also reverses the descriptor order and the command works as before due to the double reverse. The implementation of add_image() is efficient, but it made the situation too complicated. Let's make image_head point to the first added image. This will add_image() inefficient because every call of add_image() follows the ->next chain to get the tail. We can solve it by adopting a nicer linked list structure, but I am not doing as far as that because we handle only limited number of images anyway. Do likewise for add_image_desc(). Fixes: e0f083a09b29 ("fiptool: Prepare ground for expanding the set of images at runtime") Signed-off-by: Masahiro Yamada --- tools/fiptool/fiptool.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/tools/fiptool/fiptool.c b/tools/fiptool/fiptool.c index 4b91b1f9f..6145e26d4 100644 --- a/tools/fiptool/fiptool.c +++ b/tools/fiptool/fiptool.c @@ -200,9 +200,14 @@ static void free_image_desc(image_desc_t *desc) static void add_image_desc(image_desc_t *desc) { + image_desc_t **p = &image_desc_head; + assert(lookup_image_desc_from_uuid(&desc->uuid) == NULL); - desc->next = image_desc_head; - image_desc_head = desc; + + while (*p) + p = &(*p)->next; + + *p = desc; nr_image_descs++; } @@ -237,9 +242,15 @@ static void fill_image_descs(void) static void add_image(image_t *image) { + image_t **p = &image_head; + assert(lookup_image_from_uuid(&image->uuid) == NULL); - image->next = image_head; - image_head = image; + + while (*p) + p = &(*p)->next; + + *p = image; + nr_images++; } @@ -397,7 +408,7 @@ static int parse_fip(const char *filename, fip_toc_header_t *toc_header_out) * Build a new image out of the ToC entry and add it to the * table of images. */ - image = xmalloc(sizeof(*image), + image = xzalloc(sizeof(*image), "failed to allocate memory for image"); memcpy(&image->uuid, &toc_entry->uuid, sizeof(uuid_t)); image->buffer = xmalloc(toc_entry->size, @@ -454,7 +465,7 @@ static image_t *read_image_from_file(const uuid_t *uuid, const char *filename) if (fstat(fileno(fp), &st) == -1) log_errx("fstat %s", filename); - image = xmalloc(sizeof(*image), "failed to allocate memory for image"); + image = xzalloc(sizeof(*image), "failed to allocate memory for image"); memcpy(&image->uuid, uuid, sizeof(uuid_t)); image->buffer = xmalloc(st.st_size, "failed to allocate image buffer"); if (fread(image->buffer, 1, st.st_size, fp) != st.st_size)