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

[Eugeniu Rosca <roscaeugeniu@gmail.com>]

Hi Sam,

On Mon, Dec 02, 2019 at 07:19:53PM +0200, Sam Protsenko wrote:
> On Tue, Oct 29, 2019 at 3:49 AM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
> > 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.

Fine. Thanks. But I hope you also keep in mind that the more design
choices you make available to the users (especially if redundant):
 - the more test coverage you will have to accomplish, which translates
   to more requests from Simon for test development and maintenance
 - the greater the attack surface/vector, i.e. increased likelihood for
   CVEs and the like

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

That's useful feedback. Thanks.

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

Agreed with the above warnings.

[..]

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

I totally agree with the following:
 - This series is a major step forward. Many thanks for this effort.
 - Bikeshedding and minor findings can be postponed.

However, for the sake of a non-chaotic git history and the right degree
of intuitiveness in the usage of all newly developed commands, I see
below review points as _major_ and better be fixed before merging:
 - The name of the "bootimg" command intrudes too much into the U-Boot
   global namespace. This has been flagged by Simon [*]
   with no reply from you yet. Do you plan to provide feedback?
 - Kconfig wiring and naming. IMHO the right decisions have to be made
   whenever new files are added to Kbuild. It is like placing the right
   pipe infrastructure into the basement of the building. Why would you
   want to make it twice? Renaming/deleting/relocating Kconfig symbols
   makes comparing [**] the configurations of distinct U-Boot/Linux
   versions pretty painful later on.
 - Agreeing on the _right_ usage scheme/pattern for any newly developed
   U-Boot command is incredibly important right from the start. What
   makes me so confident in stating that and do I have doubts about the
   current "bootimg" usage pattern? Yes, I do. That's because the
   "bootimg" arguments follow more or less the  "dtimg" usage pattern.
   My recent series [***], updating the "dtimg" Android command,
   _struggled_ with finding an intuitive way to get the DTB/DTBO based
   on user-provided "id" and/or "rev" fields, as documented in
   https://source.android.com/devices/architecture/dto/partitions . To
   give an example, making the <index> argument mandatory in
   "bootimg get_dtb_file <index> <addr_var> [size_var]" is probably not
   the best choice simply b/c searching/retrieving DTB/DTBO by absolute
   <index> in the image is just one possible search criteria. Our
   customers would _not_ like to use this criteria for obvious reasons.
   They would like to retrieve DTB/DTBO by "id" and "rev" field values.
   So, how do you interpret <index> in these latter cases if you keep
   it as a required argument? Or should we treat <index> as an optional
   parameter when "id" and/or "rev" are provided? I hope you can get a
   feeling of how important it is to agree on the usage concept for
   "bootimg" right now, not later.

[*] https://patchwork.ozlabs.org/patch/1182212/#2291600
[**] https://patchwork.kernel.org/patch/10230207/#21524437
[***] https://patchwork.ozlabs.org/cover/1202575/
     ("cmd: dtimg: Enhance with --id and --rev options (take #1)")

-- 
Best Regards,
Eugeniu