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

Tue Dec 3 20:24:25 CET 2019

Hi,

On Tue, Dec 3, 2019 at 3:59 PM Eugeniu Rosca <roscaeugeniu at gmail.com> wrote:
>
> 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 at 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?

Already renamed to 'abootimg' and replied to Simon. Will send v3 soon.

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

Ok, will look into that tomorrow. And either will improve that, or at
least will provide some proper explanation as to why I don't want to
do so :)

Do I understand correctly that your point is: we should be able to
disable all DTBO related code when using 'bootimg' command (or just
when enabling ANDROID_BOOT_IMAGE)?

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

Agreed. Do you have some particular way of making 'bootimg' more
extensible? I'd like to keep index way as well (because if dtb files
are just concatenated this is probably the only way to specify it).
But I can change 'bootimg' interface to something like:

    bootimg get_dtb_file index <index> <addr_var> [size_var]
    bootimg get_dtb_file id <id> <addr_var> [size_var]
    bootimg get_dtb_file rev <rev> <addr_var> [size_var]

This approach seems to be easy to extend in future. That would be ok
with you, or you see some better way to fix this?

As for 'dtimg' command: after giving it some thought, I think not much
people using it yet. So in this particular case I don't have some
strong preference, and if you think the 'dtimg' interface is ugly, and
it overcomes "don't break interfaces" rule, maybe now is a good time
to rework it (before it gets widely used). Anyway, let's continue
'dtimg' related discussion in dtimg thread, it's kinda off-topic here.

Thanks for detailed review!

> [*] 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