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

[Sam Protsenko <semen.protsenko@linaro.org>]

Hi Simon,

On Wed, Jan 8, 2020 at 7:39 PM Simon Glass <sjg@chromium.org> wrote:
>
> 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.
>

It's existing header file, related to another feature. So it can be
renamed in some separate patch outside of this patch series. Btw, is
there any background on why we should stick to underscore in file
names?

> >  #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.
>

I think it's just a matter of taste. Not a major one, right?

> > +
> > +       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?
>

Frankly, don't see much value in error code in this particular case.
All we can do to handle any error in this function further is just to
print corresponding error message, which I do in this function already
(sticking to principle to print error messages where we actually know
what happened). So I'd stick to just bool, if you don't mind, to not
over-complicate this without actual reason.

> > +               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...)
>

Actually I really wanted to emphasize here that we test if size is 0,
so I'd keep that as is, if you don't mind.

> > +               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.
>

Sorry, I don't agree. Just by looking how we handle (or can handle in
future) error cases when executing this function, I can't see any
value in error code. Error message is already printed on each
particular error case in this function. So if you don't mind, I'd keep
"bool" here (because I don't see some really good reason why not to).

> > + */
> > +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.
>

Sorry, I really don't like cluttering variable names with suffixes. We
already know that those are pointers, just by looking at function
signature. I'd better stay away from stuff like Hungarian notation,
but maybe it's just my Windows development induced trauma is talking
;)

> > +{
> > +       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.
>

'dtb_img_size' should be u32, because struct andr_img_hdr::dtb_size is
u32, and it's being assigned to 'dtb_img_size'.

As for 'i', it's used to check the 'index', which in turn is used to
check struct dt_table_header::dt_entry_count, in
android_dt_get_fdt_by_index() function.

Although I agree that in common case we should stick to int/ulong, in
this particular case I guess u32 choice is valid.

> > +
> > +       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?
>

It's static, so I decided the comment can be ommited in this case. But
ok, I'll add it in v4, for consistency.

> > +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.
>

As stated above, I don't see any actual usages of error code value...
So I'd stick to bool as more simplistic choice, if it's not critical
with you.

> > + */
> > +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
>

It's present next to function implementation, in
common/image-android.c (like it's usually done in Linux kernel).

> >  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