Hi Sam,
On Tue, 24 Dec 2019 at 12:55, Sam Protsenko <semen.protse...@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.protse...@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
