| Message ID | 20191023143427.10865-2-semen.protsenko@linaro.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | am57xx: Implement Android 10 boot flow | expand |
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? [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 [..] > 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?
Hi Eugeniu, On Tue, Oct 29, 2019 at 3:49 AM Eugeniu Rosca <erosca@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
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)")
Hi, On Tue, Dec 3, 2019 at 3:59 PM Eugeniu Rosca <roscaeugeniu@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@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
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 obj-$(CONFIG_$(SPL_TPL_)OF_LIBFDT) += image-fdt.o obj-$(CONFIG_$(SPL_TPL_)FIT) += image-fit.o obj-$(CONFIG_$(SPL_)MULTI_DTB_FIT) += boot_fit.o common_fit.o 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; + +exit: + unmap_sysmem(hdr); + return res; +} + +/** + * android_image_get_dtb_by_index() - Get address and size of file in DTB area. + * @hdr_addr: Boot image header address + * @index: Index of desired DTB in DTB area (starting from 0) + * @addr: If not NULL, will contain address to specified DTB + * @size: If not NULL, will contain size of specified DTB + * + * Get the address and size of DTB file by its index in DTB area of Android + * Boot Image in RAM. + * + * Return: true on success or false on error. + */ +bool android_image_get_dtb_by_index(ulong hdr_addr, u32 index, ulong *addr, + u32 *size) +{ + const struct andr_img_hdr *hdr; + bool res; + ulong dtb_img_addr; /* address of DTB part in boot image */ + u32 dtb_img_size; /* size of DTB payload in boot image */ + ulong dtb_addr; /* address of DTB file with specified index */ + u32 i; /* index iterator */ + + res = android_image_get_dtb_img_addr(hdr_addr, &dtb_img_addr); + if (!res) + return false; + + /* Check if DTB area of boot image is in DTBO format */ + if (android_dt_check_header(dtb_img_addr)) { + return android_dt_get_fdt_by_index(dtb_img_addr, index, addr, + size); + } + + /* Find out the address of DTB with specified index in concat blobs */ + hdr = map_sysmem(hdr_addr, sizeof(*hdr)); + dtb_img_size = hdr->dtb_size; + unmap_sysmem(hdr); + i = 0; + dtb_addr = dtb_img_addr; + while (dtb_addr < dtb_img_addr + dtb_img_size) { + const struct fdt_header *fdt; + u32 dtb_size; + + fdt = map_sysmem(dtb_addr, sizeof(*fdt)); + if (fdt_check_header(fdt) != 0) { + unmap_sysmem(fdt); + printf("Error: Invalid FDT header for index %u\n", i); + return false; + } + + dtb_size = fdt_totalsize(fdt); + unmap_sysmem(fdt); + + if (i == index) { + if (size) + *size = dtb_size; + if (addr) + *addr = dtb_addr; + return true; + } + + dtb_addr += dtb_size; + ++i; + } + + printf("Error: Index is out of bounds (%u/%u)\n", index, i); + return false; +} + #if !defined(CONFIG_SPL_BUILD) /** * android_print_contents - prints out the contents of the Android format image @@ -246,4 +364,101 @@ void android_print_contents(const struct andr_img_hdr *hdr) printf("%sdtb addr: %llx\n", p, hdr->dtb_addr); } } + +static bool android_image_print_dtb_info(const struct fdt_header *fdt, + u32 index) +{ + int root_node_off; + u32 fdt_size; + const char *model; + const char *compatible; + + root_node_off = fdt_path_offset(fdt, "/"); + if (root_node_off < 0) { + printf("Error: Root node not found\n"); + return false; + } + + fdt_size = fdt_totalsize(fdt); + compatible = fdt_getprop(fdt, root_node_off, "compatible", + NULL); + model = fdt_getprop(fdt, root_node_off, "model", NULL); + + printf(" - DTB #%u:\n", index); + printf(" (DTB)size = %d\n", fdt_size); + printf(" (DTB)model = %s\n", model ? model : "(unknown)"); + printf(" (DTB)compatible = %s\n", + compatible ? compatible : "(unknown)"); + + return true; +} + +/** + * android_image_print_dtb_contents() - Print info for DTB files in DTB area. + * @hdr_addr: Boot image header address + * + * DTB payload in Android Boot Image v2+ can be in one of following formats: + * 1. Concatenated DTB files + * 2. Android DTBO format (see CONFIG_CMD_DTIMG for details) + * + * This function does next: + * 1. Prints out the format used in DTB area + * 2. Iterates over all DTB files in DTB area and prints out the info for + * each file. + * + * Return: true on success or false on error. + */ +bool android_image_print_dtb_contents(ulong hdr_addr) +{ + const struct andr_img_hdr *hdr; + bool res; + ulong dtb_img_addr; /* address of DTB part in boot image */ + u32 dtb_img_size; /* size of DTB payload in boot image */ + ulong dtb_addr; /* address of DTB file with specified index */ + u32 i; /* index iterator */ + + res = android_image_get_dtb_img_addr(hdr_addr, &dtb_img_addr); + if (!res) + return false; + + /* Check if DTB area of boot image is in DTBO format */ + if (android_dt_check_header(dtb_img_addr)) { + printf("## DTB area contents (DTBO format):\n"); + android_dt_print_contents(dtb_img_addr); + return true; + } + + printf("## DTB area contents (concat format):\n"); + + /* Iterate over concatenated DTB files */ + hdr = map_sysmem(hdr_addr, sizeof(*hdr)); + dtb_img_size = hdr->dtb_size; + unmap_sysmem(hdr); + i = 0; + dtb_addr = dtb_img_addr; + while (dtb_addr < dtb_img_addr + dtb_img_size) { + const struct fdt_header *fdt; + u32 dtb_size; + + fdt = map_sysmem(dtb_addr, sizeof(*fdt)); + if (fdt_check_header(fdt) != 0) { + unmap_sysmem(fdt); + printf("Error: Invalid FDT header for index %u\n", i); + return false; + } + + res = android_image_print_dtb_info(fdt, i); + if (!res) { + unmap_sysmem(fdt); + return false; + } + + dtb_size = fdt_totalsize(fdt); + unmap_sysmem(fdt); + dtb_addr += dtb_size; + ++i; + } + + return true; +} #endif diff --git a/include/image.h b/include/image.h index c1065c06f9..042eb89a0f 100644 --- a/include/image.h +++ b/include/image.h @@ -1333,10 +1333,15 @@ int android_image_get_ramdisk(const struct andr_img_hdr *hdr, ulong *rd_data, ulong *rd_len); int android_image_get_second(const struct andr_img_hdr *hdr, ulong *second_data, ulong *second_len); +bool android_image_get_dtb_by_index(ulong hdr_addr, u32 index, ulong *addr, + u32 *size); ulong android_image_get_end(const struct andr_img_hdr *hdr); ulong android_image_get_kload(const struct andr_img_hdr *hdr); ulong android_image_get_kcomp(const struct andr_img_hdr *hdr); void android_print_contents(const struct andr_img_hdr *hdr); +#if !defined(CONFIG_SPL_BUILD) +bool android_image_print_dtb_contents(ulong hdr_addr); +#endif #endif /* CONFIG_ANDROID_BOOT_IMAGE */
Android Boot Image v2 adds "DTB" payload (and corresponding field in the image header). Provide functions for its handling: - android_image_get_dtb_by_index(): Obtain DTB file from "DTB" part of boot image, by file index - android_image_print_dtb_contents(): Iterate over all DTB files in "DTB" part of boot image and print those files info "DTB" payload might be in one of the following formats: 1. concatenated DTB files 2. Android DTBO format The latter requires "android-image-dt.c" functionality, so this commit selects that file for building for CONFIG_ANDROID_BOOT_IMAGE option. Right now this new functionality isn't used, but it can be used further. As it's required to apply some specific dtbo file(s) from "dtbo" partition, we can't automate this process inside of "bootm" command. But we can do next: - come up with some new command like "bootimg" to extract dtb file from boot image (using functions from this patch) - extract desired dtbo files from "dtbo" partition using "dtimg" command - merge dtbo files into dtb file using "fdt apply" command - pass resulting dtb file into bootm command in order to boot the Android kernel with Android ramdisk from boot image Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> --- Changes in v2: - provide mem mappings for sandbox - rebase on top of master common/Makefile | 2 +- common/image-android.c | 215 +++++++++++++++++++++++++++++++++++++++++ include/image.h | 5 + 3 files changed, 221 insertions(+), 1 deletion(-)