[U-Boot] [PATCH v2 3/8] cmd: bootimg: Add bootimg command

Eugeniu Rosca erosca at de.adit-jv.com
Tue Dec 3 20:29:10 CET 2019


Hi Sam,
Cc: Aleksandr, Roman

As expressed in the attached e-mail, to minimize the headaches extending
the argument list of "bootimg" in future, can we please agree on below?

On Wed, Oct 23, 2019 at 05:34:22PM +0300, Sam Protsenko wrote:
> +U_BOOT_CMD(
> +	bootimg, CONFIG_SYS_MAXARGS, 0, do_bootimg,
> +	"manipulate Android Boot Image",
> +	"set_addr <addr>\n"
> +	"    - set the address in RAM where boot image is located\n"
> +	"      ($loadaddr is used by default)\n"
> +	"bootimg ver <varname>\n"

Can we make <varname> optional, with the background provided in [1]?

> +	"    - get header version\n"
> +	"bootimg get_dtbo <addr_var> [size_var]\n"

How about converting <addr_var> to an optional argument too?

> +	"    - get address and size (hex) of recovery DTBO area in the image\n"
> +	"      <addr_var>: variable name to contain DTBO area address\n"
> +	"      [size_var]: variable name to contain DTBO area size\n"
> +	"bootimg dtb_dump\n"
> +	"    - print info for all files in DTB area\n"
> +	"bootimg dtb_load_addr <varname>\n"

Same as above w.r.t. <varname>.

> +	"    - get load address (hex) of DTB\n"
> +	"bootimg get_dtb_file <index> <addr_var> [size_var]\n"

How about converting <addr_var> to an optional argument too?
How do you foresee getting a blob entry based on id=<i> and/or
rev=<r>? Which of below is your favorite option?

 - get_dtb_file <index> [--id=<i>] [--rev=<r>] [addr_var] [size_var]
   where: - in case <i> and/or <r> are provided, <index> tells the
            command to pick the N's entry in the selection filtered by
            <i> and <r>
          - in case neither <i> nor <r> is provided, <index>
            behaves like in the current patch (selects a blob entry
            found at absolute index value ${index} in the image)

 - get_dtb_file [<index>|[--id=<i>|--rev=<r>]] [addr_var] [size_var]
   To make it clear, some example commands would be:
   - get_dtb_file <index>
     => current behavior
   - get_dtb_file --id=<i>
     => get _first_ blob entry matching id value <i>
   - get_dtb_file --rev=<r>
     => get _first_ blob entry matching rev value <r>
   - get_dtb_file --id=<i> --rev=<r>
     => get _first_ blob entry matching id <i> _and_ rev=<r>
   - get_dtb_file <index> --id=<i>
   - get_dtb_file <index> --rev=<r>
     => Wrong usage

 - get_dtb_file  anything else?

I think it is crucial to agree on the above, since the very first
revision of "bootimg"/"abootimg" may impose strict limitations on how
the command can be extended in future.

[just noticed your reply shedding more light on a subset of these
questions, but still sending this out; please skip if already answered]

> +	"    - get address and size (hex) of DTB file in the image\n"
> +	"      <index>: index of desired DTB file in DTB area\n"
> +	"      <addr_var>: variable name to contain DTB file address\n"
> +	"      [size_var]: variable name to contain DTB file size\n"
> +);

[1] https://patchwork.ozlabs.org/patch/1202579/
    ("cmd: dtimg: Make <varname> an optional argument")

-- 
Best Regards,
Eugeniu
-------------- next part --------------
From: Eugeniu Rosca <roscaeugeniu at gmail.com>
Date: Tue, 3 Dec 2019 14:59:46 +0100
To: Sam Protsenko <semen.protsenko at linaro.org>
CC: Eugeniu Rosca <erosca at de.adit-jv.com>, U-Boot Mailing List
	<u-boot at lists.denx.de>, Alistair Strachan <adelva at google.com>, Alex Deymo
	<deymo at google.com>, Eugeniu Rosca <roscaeugeniu at gmail.com>, Praneeth Bajjuri
	<praneeth at ti.com>, Mykhailo Sopiha <mykhailo.sopiha at linaro.org>
Subject: Re: [U-Boot] [PATCH v2 1/8] image: android: Add functions for
 handling dtb field
Message-ID: <20191203135946.GA29671 at x230>
Content-Type: text/plain; charset="us-ascii"
In-Reply-To: <CAKaJLVtWbWjHc70yfJn8X6+HYUQE0p5ybgmP=uKYQb-+Wivthg at mail.gmail.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
MIME-Version: 1.0

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


More information about the U-Boot mailing list