[U-Boot] [PATCH v2 1/8] image: android: Add functions for handling dtb field

[Sam Protsenko]

Hi Eugeniu,

On Tue, Oct 29, 2019 at 3:49 AM Eugeniu Rosca <erosca at de.adit-jv.com> wrote:
>
> Hi Sam,
>
> On Wed, Oct 23, 2019 at 05:34:20PM +0300, Sam Protsenko wrote:
> > Android Boot Image v2 adds "DTB" payload (and corresponding field in the
> > image header). Provide functions for its handling:
>
> I believe this totally new degree of freedom brought by "Android Boot
> Image v2" might unintentionally make some users more unhappy [0], since
> it enables yet another way of passing a DTB to the Android kernel.
>
> It looks to me that there are at least three valid design choices for
> DTB storage/passing which platform maintainers have to consider:
>  A. Android Image second area [1-2]
>  B. Dedicated DTB/DTBO partitions on a block device
>  C. DTB area in Android Image v2
>
> While there are some major disadvantages for [A][1-2], [B] and [C] look
> more or less equivalent and will most likely only differ in the tooling
> used to generate and extract the useful/relevant artifacts.
>
> Not to mention that hybrids [B+C] are also possible.
> Which solution should we pick as a long-term one?
>

My opinion is next: we should provide means (commands) to achieve any
of [A,B,C] options. Then user (I mean, U-Boot developer who implements
boot for each particular board) should decide which approach to use.
Also [D] FIT image can be used instead of Android Boot Image. But we
should remember that Google requires *mandatory* for all *new* devices
(starting with Android 10) to stick with [C] approach only. Option [B]
might be harder to handle from AVB verification point of view. Btw,
when we come to "boot_android" command, I think we'll need to
implement only officially recommended boot method. Maybe provide a
param to choose whether to do Android-9 or Android-10 boot scheme.

Anyway, the short answer is: we should provide a bunch of isolated
commands to allow the user to implement any boot method.

Also, this particular patch doesn't change boot behavior (it only adds
functions to handle dtb field), so it should be backward-compatible.

> [0] https://en.wikipedia.org/wiki/The_Paradox_of_Choice
> [1] 104816142f9c6a ("parse the second area of android image")
> [2] 6a7b406aa8b981 ("fdt: support booting with dtb in Android image")
>
> [..]
>
> >  common/Makefile        |   2 +-
> >  common/image-android.c | 215 +++++++++++++++++++++++++++++++++++++++++
> >  include/image.h        |   5 +
> >  3 files changed, 221 insertions(+), 1 deletion(-)
> >
> > diff --git a/common/Makefile b/common/Makefile
> > index 302d8beaf3..7e5f5058b3 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
>
> I expect that in a not so distant future, Android users who care
> about performance aspects of their system (e.g. in automotive sector,
> where boot time is paramount to achieve ~2s to Rear View Camera) will
> attempt optimizing U-Boot by compiling out features they don't need.
>
> With this background:
>  - Would it make sense to allow users to selectively enable and disable
>    support for A/B-capable and non-A/B devices? My guess is that it
>    is currently not an important development/review criteria, since
>    particularly image-android-dt.c implements functions, some of which
>    are A/B-specific, some non-A/B-specific (e.g. android_image_get_dtbo)
>    and some are common.
>  - I would try to avoid wiring the same compilation unit to Kbuild
>    (e.g. image-android-dt.o) via multiple Kconfig symbols
>    (CONFIG_ANDROID_BOOT_IMAGE and CONFIG_CMD_DTIMG) since this makes
>    the relationship between the CONFIG symbols unclear. As a user, I
>    would like to see a simple mapping between compiled objects and
>    Kconfig symbols, eventually reflecting a hierarchy/dependency:
>
>    config ANDROID_BOOT_IMAGE
>       select ANDROID_BOOT_IMAGE_DT
>
>    config DTIMG
>       select ANDROID_BOOT_IMAGE_DT
>

I thought about that 4 months ago when implementing this patch, even
experimented with that. But decided to just add image-android-dt.o in
Makefile instead, don't remember for what reason exactly. Frankly,
don't really want to go there again and spend too much time (at least
not in the context of this patch series).

So here is what I suggest: can we approach this one-step-at-a-time?
This patch-set is a major step as it is, and it takes a lot of time to
get it merged. What you suggest makes sense but might have some
implications (even architectural) when trying to implement that. Can
we do that in another series?

> [..]
>
> > diff --git a/common/image-android.c b/common/image-android.c
> > index 3564a64221..5e721a27a7 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>
> >  #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,122 @@ 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;
> > +
> > +     hdr = map_sysmem(hdr_addr, sizeof(*hdr));
> > +     if (android_image_check_header(hdr)) {
> > +             printf("Error: Boot Image header is incorrect\n");
> > +             res = false;
> > +             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) {
> > +             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);
> > +
> > +     if (addr)
> > +             *addr = dtb_img_addr;
>
> In this recent series, I noticed a consistent preference towards
> doing a lot of processing with returning nothing to the caller in
> case of an invalid argument. Is this by choice?
>

In this particular case, this is probably just a leftover from
copy-paste from other DTBO related function, where such a parameter is
optional. Here we can just remove that "if" condition, I guess... Not
sure it's a major issue though. If you see any broken logic or
incorrect behavior, please comment in place.

Otherwise, if there are no major concerns, I suggest to take it as is,
and adding other desired features in other series. Please add your
Reviewed-by tag if you agree.

Thanks!

> --
> Best Regards,
> Eugeniu