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 <yamada.masahiro@socionext.com>

diff --git a/tools/fiptool/fiptool.c b/tools/fiptool/fiptool.c
index 4b91b1f9fec0fd95ea39a5d9fc270674d881e66c..6145e26d4b3d5e69ae768f373ca692e29628c46a 100644 (file)
--- 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)