[PATCH v3 1/9] image: android: Add functions for handling dtb field

[Simon Glass <sjg@chromium.org>]

Hi Sam,

On Tue, 24 Dec 2019 at 12:55, Sam Protsenko <semen.protsenko@linaro.org> wrote:
>
> Android Boot Image v2 adds "DTB" payload (and corresponding field in the
> image header). Provide functions for its handling:
>
>   - android_image_get_dtb_by_index(): Obtain DTB blob from "DTB" part of
>     boot image, by blob's index
>   - android_image_print_dtb_contents(): Iterate over all DTB blobs in
>     "DTB" part of boot image and print those blobs info
>
> "DTB" payload might be in one of the following formats:
>   1. concatenated DTB blobs
>   2. Android DTBO format
>
> The latter requires "android-image-dt.c" functionality, so this commit
> selects that file for building for CONFIG_ANDROID_BOOT_IMAGE option.
>
> Right now this new functionality isn't used, but it can be used further.
> As it's required to apply some specific dtbo blob(s) from "dtbo"
> partition, we can't automate this process inside of "bootm" command. But
> we can do next:
>   - come up with some new command like "abootimg" to extract dtb blob
>     from boot image (using functions from this patch)
>   - extract desired dtbo blobs from "dtbo" partition using "adtimg"
>     command
>   - merge dtbo blobs into dtb blob using "fdt apply" command
>   - pass resulting dtb blob into bootm command in order to boot the
>     Android kernel with Android ramdisk from boot image
>
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
>  common/Makefile        |   2 +-
>  common/image-android.c | 214 +++++++++++++++++++++++++++++++++++++++++
>  include/image.h        |   5 +
>  3 files changed, 220 insertions(+), 1 deletion(-)
>
> diff --git a/common/Makefile b/common/Makefile
> index 029cc0f2ce..1ffddc2f94 100644
> --- a/common/Makefile
> +++ b/common/Makefile
> @@ -108,7 +108,7 @@ endif
>
>  obj-y += image.o
>  obj-$(CONFIG_ANDROID_AB) += android_ab.o
> -obj-$(CONFIG_ANDROID_BOOT_IMAGE) += image-android.o
> +obj-$(CONFIG_ANDROID_BOOT_IMAGE) += image-android.o image-android-dt.o
>  obj-$(CONFIG_$(SPL_TPL_)OF_LIBFDT) += image-fdt.o
>  obj-$(CONFIG_$(SPL_TPL_)FIT) += image-fit.o
>  obj-$(CONFIG_$(SPL_)MULTI_DTB_FIT) += boot_fit.o common_fit.o
> diff --git a/common/image-android.c b/common/image-android.c
> index 3564a64221..1ccad6c556 100644
> --- a/common/image-android.c
> +++ b/common/image-android.c
> @@ -6,10 +6,12 @@
>  #include <common.h>
>  #include <env.h>
>  #include <image.h>
> +#include <image-android-dt.h>

Prefer underscores if possible.

>  #include <android_image.h>
>  #include <malloc.h>
>  #include <errno.h>
>  #include <asm/unaligned.h>
> +#include <mapmem.h>
>
>  #define ANDROID_IMAGE_DEFAULT_KERNEL_ADDR      0x10008000
>
> @@ -195,6 +197,121 @@ int android_image_get_second(const struct andr_img_hdr *hdr,
>         return 0;
>  }
>
> +/**
> + * android_image_get_dtb_img_addr() - Get the address of DTB area in boot image.
> + * @hdr_addr: Boot image header address
> + * @addr: Will contain the address of DTB area in boot image
> + *
> + * Return: true on success or false on fail.
> + */
> +static bool android_image_get_dtb_img_addr(ulong hdr_addr, ulong *addr)
> +{
> +       const struct andr_img_hdr *hdr;
> +       ulong dtb_img_addr;
> +       bool res = true;

Perhaps this isn't universal but at least with DM we use 'ret' for
return code instead of 'res' for result.

> +
> +       hdr = map_sysmem(hdr_addr, sizeof(*hdr));
> +       if (android_image_check_header(hdr)) {
> +               printf("Error: Boot Image header is incorrect\n");
> +               res = false;

Could this function return an error code?

> +               goto exit;
> +       }
> +
> +       if (hdr->header_version < 2) {
> +               printf("Error: header_version must be >= 2 to get dtb\n");
> +               res = false;
> +               goto exit;
> +       }
> +
> +       if (hdr->dtb_size == 0) {

if (!hdr...)

> +               printf("Error: dtb_size is 0\n");
> +               res = false;
> +               goto exit;
> +       }
> +
> +       /* Calculate the address of DTB area in boot image */
> +       dtb_img_addr = hdr_addr;
> +       dtb_img_addr += hdr->page_size;
> +       dtb_img_addr += ALIGN(hdr->kernel_size, hdr->page_size);
> +       dtb_img_addr += ALIGN(hdr->ramdisk_size, hdr->page_size);
> +       dtb_img_addr += ALIGN(hdr->second_size, hdr->page_size);
> +       dtb_img_addr += ALIGN(hdr->recovery_dtbo_size, hdr->page_size);
> +
> +       *addr = dtb_img_addr;
> +
> +exit:
> +       unmap_sysmem(hdr);
> +       return res;
> +}
> +
> +/**
> + * android_image_get_dtb_by_index() - Get address and size of blob in DTB area.
> + * @hdr_addr: Boot image header address
> + * @index: Index of desired DTB in DTB area (starting from 0)
> + * @addr: If not NULL, will contain address to specified DTB
> + * @size: If not NULL, will contain size of specified DTB
> + *
> + * Get the address and size of DTB blob by its index in DTB area of Android
> + * Boot Image in RAM.
> + *
> + * Return: true on success or false on error.

Let's return an error code.

> + */
> +bool android_image_get_dtb_by_index(ulong hdr_addr, u32 index, ulong *addr,
> +                                   u32 *size)

Suggest adding a 'p' suffix to return values, i.e. addrp and sizep.

> +{
> +       const struct andr_img_hdr *hdr;
> +       bool res;
> +       ulong dtb_img_addr;     /* address of DTB part in boot image */
> +       u32 dtb_img_size;       /* size of DTB payload in boot image */
> +       ulong dtb_addr;         /* address of DTB blob with specified index  */
> +       u32 i;                  /* index iterator */

Why use u32 for these variables? Natural types should be used where
possible, i.e. uint or int.

> +
> +       res = android_image_get_dtb_img_addr(hdr_addr, &dtb_img_addr);
> +       if (!res)
> +               return false;
> +
> +       /* Check if DTB area of boot image is in DTBO format */
> +       if (android_dt_check_header(dtb_img_addr)) {
> +               return android_dt_get_fdt_by_index(dtb_img_addr, index, addr,
> +                                                  size);
> +       }
> +
> +       /* Find out the address of DTB with specified index in concat blobs */
> +       hdr = map_sysmem(hdr_addr, sizeof(*hdr));
> +       dtb_img_size = hdr->dtb_size;
> +       unmap_sysmem(hdr);
> +       i = 0;
> +       dtb_addr = dtb_img_addr;
> +       while (dtb_addr < dtb_img_addr + dtb_img_size) {
> +               const struct fdt_header *fdt;
> +               u32 dtb_size;
> +
> +               fdt = map_sysmem(dtb_addr, sizeof(*fdt));
> +               if (fdt_check_header(fdt) != 0) {
> +                       unmap_sysmem(fdt);
> +                       printf("Error: Invalid FDT header for index %u\n", i);
> +                       return false;
> +               }
> +
> +               dtb_size = fdt_totalsize(fdt);
> +               unmap_sysmem(fdt);
> +
> +               if (i == index) {
> +                       if (size)
> +                               *size = dtb_size;
> +                       if (addr)
> +                               *addr = dtb_addr;
> +                       return true;
> +               }
> +
> +               dtb_addr += dtb_size;
> +               ++i;are these are these are these
> +       }
> +
> +       printf("Error: Index is out of bounds (%u/%u)\n", index, i);
> +       return false;
> +}
> +
>  #if !defined(CONFIG_SPL_BUILD)
>  /**
>   * android_print_contents - prints out the contents of the Android format image
> @@ -246,4 +363,101 @@ void android_print_contents(const struct andr_img_hdr *hdr)
>                 printf("%sdtb addr:             %llx\n", p, hdr->dtb_addr);
>         }
>  }
> +

Function comment...what is index?

> +static bool android_image_print_dtb_info(const struct fdt_header *fdt,
> +                                        u32 index)
> +{
> +       int root_node_off;
> +       u32 fdt_size;
> +       const char *model;
> +       const char *compatible;
> +
> +       root_node_off = fdt_path_offset(fdt, "/");
> +       if (root_node_off < 0) {
> +               printf("Error: Root node not found\n");
> +               return false;
> +       }
> +
> +       fdt_size = fdt_totalsize(fdt);
> +       compatible = fdt_getprop(fdt, root_node_off, "compatible",
> +                                NULL);
> +       model = fdt_getprop(fdt, root_node_off, "model", NULL);
> +
> +       printf(" - DTB #%u:\n", index);
> +       printf("           (DTB)size = %d\n", fdt_size);
> +       printf("          (DTB)model = %s\n", model ? model : "(unknown)");
> +       printf("     (DTB)compatible = %s\n",
> +              compatible ? compatible : "(unknown)");
> +
> +       return true;
> +}
> +
> +/**
> + * android_image_print_dtb_contents() - Print info for DTB blobs in DTB area.
> + * @hdr_addr: Boot image header address
> + *
> + * DTB payload in Android Boot Image v2+ can be in one of following formats:
> + *   1. Concatenated DTB blobs
> + *   2. Android DTBO format (see CONFIG_CMD_ADTIMG for details)
> + *
> + * This function does next:
> + *   1. Prints out the format used in DTB area
> + *   2. Iterates over all DTB blobs in DTB area and prints out the info for
> + *      each blob.
> + *
> + * Return: true on success or false on error.

Again I think an error code is better.

> + */
> +bool android_image_print_dtb_contents(ulong hdr_addr)
> +{
> +       const struct andr_img_hdr *hdr;
> +       bool res;
> +       ulong dtb_img_addr;     /* address of DTB part in boot image */
> +       u32 dtb_img_size;       /* size of DTB payload in boot image */
> +       ulong dtb_addr;         /* address of DTB blob with specified index  */
> +       u32 i;                  /* index iterator */
> +
> +       res = android_image_get_dtb_img_addr(hdr_addr, &dtb_img_addr);
> +       if (!res)
> +               return false;
> +
> +       /* Check if DTB area of boot image is in DTBO format */
> +       if (android_dt_check_header(dtb_img_addr)) {
> +               printf("## DTB area contents (DTBO format):\n");
> +               android_dt_print_contents(dtb_img_addr);
> +               return true;
> +       }
> +
> +       printf("## DTB area contents (concat format):\n");
> +
> +       /* Iterate over concatenated DTB blobs */
> +       hdr = map_sysmem(hdr_addr, sizeof(*hdr));
> +       dtb_img_size = hdr->dtb_size;
> +       unmap_sysmem(hdr);
> +       i = 0;
> +       dtb_addr = dtb_img_addr;
> +       while (dtb_addr < dtb_img_addr + dtb_img_size) {
> +               const struct fdt_header *fdt;
> +               u32 dtb_size;
> +
> +               fdt = map_sysmem(dtb_addr, sizeof(*fdt));
> +               if (fdt_check_header(fdt) != 0) {
> +                       unmap_sysmem(fdt);
> +                       printf("Error: Invalid FDT header for index %u\n", i);
> +                       return false;
> +               }
> +
> +               res = android_image_print_dtb_info(fdt, i);
> +               if (!res) {
> +                       unmap_sysmem(fdt);
> +                       return false;
> +               }
> +
> +               dtb_size = fdt_totalsize(fdt);
> +               unmap_sysmem(fdt);
> +               dtb_addr += dtb_size;
> +               ++i;
> +       }
> +
> +       return true;
> +}
>  #endif
> diff --git a/include/image.h b/include/image.h
> index f4d2aaf53e..8e81166be4 100644
> --- a/include/image.h
> +++ b/include/image.h
> @@ -1333,10 +1333,15 @@ int android_image_get_ramdisk(const struct andr_img_hdr *hdr,
>                               ulong *rd_data, ulong *rd_len);
>  int android_image_get_second(const struct andr_img_hdr *hdr,
>                               ulong *second_data, ulong *second_len);
> +bool android_image_get_dtb_by_index(ulong hdr_addr, u32 index, ulong *addr,
> +                                   u32 *size);

Function comment

>  ulong android_image_get_end(const struct andr_img_hdr *hdr);
>  ulong android_image_get_kload(const struct andr_img_hdr *hdr);
>  ulong android_image_get_kcomp(const struct andr_img_hdr *hdr);
>  void android_print_contents(const struct andr_img_hdr *hdr);
> +#if !defined(CONFIG_SPL_BUILD)
> +bool android_image_print_dtb_contents(ulong hdr_addr);
> +#endif
>
>  #endif /* CONFIG_ANDROID_BOOT_IMAGE */
>
> --
> 2.24.0
>

Regards,
SImon