From: Eugeniu Rosca Date: Tue, 3 Dec 2019 14:59:46 +0100 To: Sam Protsenko CC: Eugeniu Rosca , U-Boot Mailing List , Alistair Strachan , Alex Deymo , Eugeniu Rosca , Praneeth Bajjuri , Mykhailo Sopiha Subject: Re: [U-Boot] [PATCH v2 1/8] image: android: Add functions for handling dtb field Message-ID: <20191203135946.GA29671@x230> Content-Type: text/plain; charset="us-ascii" In-Reply-To: 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 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 argument mandatory in "bootimg get_dtb_file [size_var]" is probably not the best choice simply b/c searching/retrieving DTB/DTBO by absolute 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 in these latter cases if you keep it as a required argument? Or should we treat 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