[PATCH v3 1/9] image: android: Add functions for handling dtb field
Simon Glass
sjg at chromium.org
Sat Mar 14 21:35:01 CET 2020
Hi Sam,
On Fri, 10 Jan 2020 at 08:33, Sam Protsenko <semen.protsenko at linaro.org> wrote:
>
> Hi Simon,
>
> On Wed, Jan 8, 2020 at 7:39 PM Simon Glass <sjg at chromium.org> wrote:
> >
> > Hi Sam,
> >
> > On Tue, 24 Dec 2019 at 12:55, Sam Protsenko <semen.protsenko at 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 at 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?
I added a comment here:
https://www.denx.de/wiki/U-Boot/CodingStyle
>
> > > #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?
Not major perhaps, but this sort of consistency makes the code much
easier to read, IMO. Using error codes as return values is pretty
universal in U-Boot so I think you should do that instead of
true/false.
>
> > > +
> > > + 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.
It is not a great idea to print an error message in a leaf function.
Ideally we would do all of this in the command that calls the
function, if it can be determined by the error code.
Also, 'boot image header is incorrect' doesn't help that much. What is
incorrect about it? You could print the return code so you know.
Following your philosophy you should add lots of printf() calls to
android_image_check_header().
Also log_err() is better than printf()
>
> > > + 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.
OK I guess it is just what you are used to.
>
> > > + 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).
See above.
>
> > > + */
> > > +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
> ;)
Again this is just a matter of style. Driver model uses 'p' and some
of image.h. I like Hungarians but not their notation either, except in
this case where is a really good visual indication of what is going
on.
>
> > > +{
> > > + 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'.
I'm not sure why.
>
> 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.
In general we should use natural sizes to avoid shifts/masks, etc. on
machines with a larger word size. The exception is structures, etc.
where you need a certain binary format.
>
> > > +
> > > + 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.
Yes please. Function comments should be added for non-trivial
functions even if they are static.
>
> > > +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.
See above, but I suggest you add some error codes, so the caller can
see what went wrong.
>
> > > + */
> > > +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).
OK but please put it in the header, which is where people look and
ctags takes you for the definition. The header file describes the API
for image.h
>
> > > 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
More information about the U-Boot
mailing list