| Message ID | 20190517144503.3275-3-erosca@de.adit-jv.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Add 'bcb' command to read/modify/writeAndroid BCB | expand |
Hi Eugeniu, On Fri, 17 May 2019 at 08:46, Eugeniu Rosca <erosca@de.adit-jv.com> wrote: > > 'Bootloader Control Block' (BCB) is a well established term/acronym in > the Android namespace which refers to a location in a dedicated raw > (i.e. FS-unaware) flash (e.g. eMMC) partition, usually called "misc", > which is used as media for exchanging messages between Android userspace > (particularly recovery [1]) and an Android-capable bootloader. > > On higher level, this allows implementing a subset of Android Bootloader > Requirements [2], amongst which is the Android-specific bootloader > flow [3]. Regardless how the latter is implemented in U-Boot ([3] being > the most memorable example), reading/writing/dumping the BCB fields in > the development process from inside the U-Boot is a convenient feature. > Hence, make it available to the users. > > Some usage examples of the new command recorded on R-Car H3ULCB-KF > ('>>>' is an overlay on top of the original console output): > > => bcb > bcb - Load/set/clear/test/dump/store Android BCB fields > > Usage: > bcb load <dev> <part> - load BCB from mmc <dev>:<part> > bcb set <field> <val> - set BCB <field> to <val> > bcb clear [<field>] - clear BCB <field> or all fields > bcb test <field> <op> <val> - test BCB <field> against <val> > bcb dump <field> - dump BCB <field> > bcb store - store BCB back to mmc > > Legend: > <dev> - MMC device index containing the BCB partition > <part> - MMC partition index or name containing the BCB > <field> - one of {command,status,recovery,stage,reserved} > <op> - the binary operator used in 'bcb test': > '=' returns true if <val> matches the string stored in <field> > '~' returns true if <val> matches a subset of <field>'s string > <val> - string/text provided as input to bcb {set,test} > NOTE: any ':' character in <val> will be replaced by line feed > during 'bcb set' and used as separator by upper layers > > => bcb dump command > Error: BCB not loaded! > >>> Users must specify mmc device and partition before any other call > > => bcb load 1 misc > => bcb load 1 1 > >>> The two calls are equivalent (assuming "misc" has index 1) > > => bcb dump command > 00000000: 62 6f 6f 74 6f 6e 63 65 2d 73 68 65 6c 6c 00 72 bootonce-shell.r > 00000010: 79 00 72 00 00 00 00 00 00 00 00 00 00 00 00 00 y.r............. > >>> The output is in binary/string format for convenience > >>> The output size matches the size of inspected BCB field > >>> (32 bytes in case of 'command') > > => bcb test command = bootonce-shell && echo true > true > => bcb test command = bootonce-shell- && echo true > => bcb test command = bootonce-shel && echo true > >>> The '=' operator returns 'true' on perfect match > > => bcb test command ~ bootonce-shel && echo true > true > => bcb test command ~ bootonce-shell && echo true > true > >>> The '~' operator returns 'true' on substring match > > => bcb set command recovery > => bcb dump command > 00000000: 72 65 63 6f 76 65 72 79 00 73 68 65 6c 6c 00 72 recovery.shell.r > 00000010: 79 00 72 00 00 00 00 00 00 00 00 00 00 00 00 00 y.r............. > >>> The new value is NULL-terminated and stored in the BCB field > > => bcb set recovery "msg1:msg2:msg3" > => bcb dump recovery > 00000040: 6d 73 67 31 0a 6d 73 67 32 0a 6d 73 67 33 00 00 msg1.msg2.msg3.. > 00000050: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > 00000060: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > >>> --- snip --- > >>> Every ':' is replaced by line-feed '\n' (0xA). The latter is used > >>> as separator between individual commands by Android userspace > > => bcb store > >>> Flush/store the BCB structure to MMC > > [1] https://android.googlesource.com/platform/bootable/recovery > [2] https://source.android.com/devices/bootloader > [3] https://patchwork.ozlabs.org/patch/746835/ > ("[U-Boot,5/6] Initial support for the Android Bootloader flow") > > Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com> > --- > v2: > - [Heinrich Schuchardt] Implement sub-commands via U_BOOT_CMD_MKENT. > - Polished the code. Ensured no warnings returned by sparse, smatch, > `cppcheck --force --enable=all --inconclusive`, make W=1. > - Tested on R-Car-H3-ES20 ULCB-KF. > > v1: > - https://patchwork.ozlabs.org/patch/1080395/ > --- > cmd/Kconfig | 17 +++ > cmd/Makefile | 1 + > cmd/bcb.c | 330 +++++++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 348 insertions(+) > create mode 100644 cmd/bcb.c Where is this documented? Perhaps it should go in README.avb2? Should it default to enabled if avb is used? Regards, Simon
Hi Simon cc: Sam, Igor, feel free to correct/augment anything of below On Sat, May 18, 2019 at 10:33:02AM -0600, Simon Glass wrote: > Hi Eugeniu, > > On Fri, 17 May 2019 at 08:46, Eugeniu Rosca <erosca@de.adit-jv.com> wrote: > > > > 'Bootloader Control Block' (BCB) is a well established term/acronym in > > the Android namespace which refers to a location in a dedicated raw > > (i.e. FS-unaware) flash (e.g. eMMC) partition, usually called "misc", > > which is used as media for exchanging messages between Android userspace > > (particularly recovery [1]) and an Android-capable bootloader. > > [..] > > Where is this documented? Perhaps it should go in README.avb2? README.avb2 is solely about the "verified/secure" booting of Android, while the 'bcb' command proposed in this patch can be useful both in verified boot scenarios (e.g. full-featured U-Boot builds), as well as in non-avb ones (e.g. development, PoC, demos, custom configurations). Thus, I think that README.avb2 is not the best place for 'bcb'. More generally, as somebody who uses git as an extension of himself, I am quite happy with commit-only documentation. OTOH, for those who receive U-Boot in tarballs or expect source-level docs/tutorials, I agree that having the 'bcb' described in a README might be helpful. I can identify two Android-dedicated README files, but none of them seems to be suitable for the new command: - doc/README.android-fastboot - doc/README.avb2 Igor, Sam, what's your view on the above? Would you suggest creating a doc/README.android-bcb or there is a more elegant solution to it? > > Should it default to enabled if avb is used? I think at this specific moment in time, 'bcb' is orthogonal (meaning it is neither a direct, nor a reverse dependency) to any other Android feature in U-Boot. This could be re-assessed, if platform maintainers start to rely on 'bcb' in their U-Boot environments on regular basis.
On Mon, May 20, 2019 at 8:23 AM Eugeniu Rosca <erosca@de.adit-jv.com> wrote: > > > > > Should it default to enabled if avb is used? > > I think at this specific moment in time, 'bcb' is orthogonal (meaning it > is neither a direct, nor a reverse dependency) to any other Android > feature in U-Boot. This could be re-assessed, if platform maintainers > start to rely on 'bcb' in their U-Boot environments on regular basis. > 'bcb' looks like something I'd be interested in, not running Android at all... currently I (ab)use the bootcounter to communicate between the kernel and U-Boot when I want to force a board through recovery, but this looks like something that might well give me a better interface.
Hi Alex, On Mon, May 20, 2019 at 08:32:28AM +0100, Alex Kiernan wrote: > On Mon, May 20, 2019 at 8:23 AM Eugeniu Rosca <erosca@de.adit-jv.com> wrote: > > > > > > > > Should it default to enabled if avb is used? > > > > I think at this specific moment in time, 'bcb' is orthogonal (meaning it > > is neither a direct, nor a reverse dependency) to any other Android > > feature in U-Boot. This could be re-assessed, if platform maintainers > > start to rely on 'bcb' in their U-Boot environments on regular basis. > > > > 'bcb' looks like something I'd be interested in, not running Android > at all... currently I (ab)use the bootcounter to communicate between > the kernel and U-Boot when I want to force a board through recovery, > but this looks like something that might well give me a better > interface. Thanks for this piece of feedback. It means 'bcb' concept and implementation might span beyond the Android use-cases/needs. On small scale, this seems to approve my choice of not inserting the "android" keyword into the command or Kconfig symbol names.
Hi Eugeniu, On Mon, May 20, 2019 at 10:23 AM Eugeniu Rosca <erosca@de.adit-jv.com> wrote: > > Hi Simon > cc: Sam, Igor, feel free to correct/augment anything of below > > On Sat, May 18, 2019 at 10:33:02AM -0600, Simon Glass wrote: > > Hi Eugeniu, > > > > On Fri, 17 May 2019 at 08:46, Eugeniu Rosca <erosca@de.adit-jv.com> wrote: > > > > > > 'Bootloader Control Block' (BCB) is a well established term/acronym in > > > the Android namespace which refers to a location in a dedicated raw > > > (i.e. FS-unaware) flash (e.g. eMMC) partition, usually called "misc", > > > which is used as media for exchanging messages between Android userspace > > > (particularly recovery [1]) and an Android-capable bootloader. > > > > [..] > > > > Where is this documented? Perhaps it should go in README.avb2? > > README.avb2 is solely about the "verified/secure" booting of Android, > while the 'bcb' command proposed in this patch can be useful both in > verified boot scenarios (e.g. full-featured U-Boot builds), as well > as in non-avb ones (e.g. development, PoC, demos, custom > configurations). Thus, I think that README.avb2 is not the best > place for 'bcb'. > > More generally, as somebody who uses git as an extension of himself, I > am quite happy with commit-only documentation. OTOH, for those who > receive U-Boot in tarballs or expect source-level docs/tutorials, I > agree that having the 'bcb' described in a README might be helpful. > > I can identify two Android-dedicated README files, but none of them > seems to be suitable for the new command: > - doc/README.android-fastboot > - doc/README.avb2 > > Igor, Sam, what's your view on the above? Would you suggest creating > a doc/README.android-bcb or there is a more elegant solution to it? > Documentation would be nice. Although you already provided a generic description of 'bcb' command in 'help bcb', the user often wants to read more high-level documentation. I'd say, you can copy the description from commit message to doc/README.android-bcb, extending it with usual use-cases, and how this command can be used in those use-cases. For example, your pseudo-code for reboot reason you provided to me earlier, etc. Also, it can be useful to mention if Google have any requirements (mandatory or optional) for the bootloader (misc partition, reboot reason, recovery, etc), and how this 'bcb' command can help in those requirements implementation. All that said, IMHO documentation/test wise: it's not critical in this case, you can add that later, just to speed-up the whole development process and divide it into iterations. But that's for maintainers to decide, of course. Also, I've a feeling that we have another problem, more common than just a documentation. At the moment we have a bunch of Android related features, which don't have namespace separation on several levels: - file/directories: we may want to move all Android related commands to sub-directory - commands: we may want to add main command called "android" for all Android-related commands, or maybe just a prefix - Kconfig: we may want to have some generic naming scheme for all Android-related commands - Documentation, tests: the same here So at some point we will probably need to discuss and fix that somehow. Not here, of course :) > > > > Should it default to enabled if avb is used? > > I think at this specific moment in time, 'bcb' is orthogonal (meaning it > is neither a direct, nor a reverse dependency) to any other Android > feature in U-Boot. This could be re-assessed, if platform maintainers > start to rely on 'bcb' in their U-Boot environments on regular basis. > > -- > Best Regards, > Eugeniu.
Anyway, from my side: Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org> On Mon, May 20, 2019 at 6:16 PM Sam Protsenko <semen.protsenko@linaro.org> wrote: > > Hi Eugeniu, > > > On Mon, May 20, 2019 at 10:23 AM Eugeniu Rosca <erosca@de.adit-jv.com> wrote: > > > > Hi Simon > > cc: Sam, Igor, feel free to correct/augment anything of below > > > > On Sat, May 18, 2019 at 10:33:02AM -0600, Simon Glass wrote: > > > Hi Eugeniu, > > > > > > On Fri, 17 May 2019 at 08:46, Eugeniu Rosca <erosca@de.adit-jv.com> wrote: > > > > > > > > 'Bootloader Control Block' (BCB) is a well established term/acronym in > > > > the Android namespace which refers to a location in a dedicated raw > > > > (i.e. FS-unaware) flash (e.g. eMMC) partition, usually called "misc", > > > > which is used as media for exchanging messages between Android userspace > > > > (particularly recovery [1]) and an Android-capable bootloader. > > > > > > [..] > > > > > > Where is this documented? Perhaps it should go in README.avb2? > > > > README.avb2 is solely about the "verified/secure" booting of Android, > > while the 'bcb' command proposed in this patch can be useful both in > > verified boot scenarios (e.g. full-featured U-Boot builds), as well > > as in non-avb ones (e.g. development, PoC, demos, custom > > configurations). Thus, I think that README.avb2 is not the best > > place for 'bcb'. > > > > More generally, as somebody who uses git as an extension of himself, I > > am quite happy with commit-only documentation. OTOH, for those who > > receive U-Boot in tarballs or expect source-level docs/tutorials, I > > agree that having the 'bcb' described in a README might be helpful. > > > > I can identify two Android-dedicated README files, but none of them > > seems to be suitable for the new command: > > - doc/README.android-fastboot > > - doc/README.avb2 > > > > Igor, Sam, what's your view on the above? Would you suggest creating > > a doc/README.android-bcb or there is a more elegant solution to it? > > > > Documentation would be nice. Although you already provided a generic > description of 'bcb' command in 'help bcb', the user often wants to > read more high-level documentation. I'd say, you can copy the > description from commit message to doc/README.android-bcb, extending > it with usual use-cases, and how this command can be used in those > use-cases. For example, your pseudo-code for reboot reason you > provided to me earlier, etc. Also, it can be useful to mention if > Google have any requirements (mandatory or optional) for the > bootloader (misc partition, reboot reason, recovery, etc), and how > this 'bcb' command can help in those requirements implementation. > > All that said, IMHO documentation/test wise: it's not critical in this > case, you can add that later, just to speed-up the whole development > process and divide it into iterations. But that's for maintainers to > decide, of course. > > Also, I've a feeling that we have another problem, more common than > just a documentation. At the moment we have a bunch of Android related > features, which don't have namespace separation on several levels: > - file/directories: we may want to move all Android related commands > to sub-directory > - commands: we may want to add main command called "android" for all > Android-related commands, or maybe just a prefix > - Kconfig: we may want to have some generic naming scheme for all > Android-related commands > - Documentation, tests: the same here > > So at some point we will probably need to discuss and fix that > somehow. Not here, of course :) > > > > > > > Should it default to enabled if avb is used? > > > > I think at this specific moment in time, 'bcb' is orthogonal (meaning it > > is neither a direct, nor a reverse dependency) to any other Android > > feature in U-Boot. This could be re-assessed, if platform maintainers > > start to rely on 'bcb' in their U-Boot environments on regular basis. > > > > -- > > Best Regards, > > Eugeniu.
Hi Alex, > On Mon, May 20, 2019 at 8:23 AM Eugeniu Rosca <erosca@de.adit-jv.com> > wrote: > > > > > > > > Should it default to enabled if avb is used? > > > > I think at this specific moment in time, 'bcb' is orthogonal > > (meaning it is neither a direct, nor a reverse dependency) to any > > other Android feature in U-Boot. This could be re-assessed, if > > platform maintainers start to rely on 'bcb' in their U-Boot > > environments on regular basis. > > 'bcb' looks like something I'd be interested in, not running Android > at all... currently I (ab)use the bootcounter to communicate between > the kernel and U-Boot when I want to force a board through recovery, I don't know your exact use case, but wouldn't it be better to use envs (with redundancy) and fw_setenv / fw_printenv from Linux user space? Now those envs even support setting default values for u-boot (as there is now separate library used for it). Moreover there is OE/Yocto's recipe 'u-boot-fw-utils' which can be easily used and installed. > but this looks like something that might well give me a better > interface. > Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
On Tue, May 21, 2019 at 9:37 AM Lukasz Majewski <lukma@denx.de> wrote: > > Hi Alex, > > > On Mon, May 20, 2019 at 8:23 AM Eugeniu Rosca <erosca@de.adit-jv.com> > > wrote: > > > > > > > > > > > Should it default to enabled if avb is used? > > > > > > I think at this specific moment in time, 'bcb' is orthogonal > > > (meaning it is neither a direct, nor a reverse dependency) to any > > > other Android feature in U-Boot. This could be re-assessed, if > > > platform maintainers start to rely on 'bcb' in their U-Boot > > > environments on regular basis. > > > > 'bcb' looks like something I'd be interested in, not running Android > > at all... currently I (ab)use the bootcounter to communicate between > > the kernel and U-Boot when I want to force a board through recovery, > > I don't know your exact use case, but wouldn't it be better to use envs > (with redundancy) and fw_setenv / fw_printenv from Linux user space? > > Now those envs even support setting default values for u-boot (as there > is now separate library used for it). Moreover there is OE/Yocto's > recipe 'u-boot-fw-utils' which can be easily used and installed. > It's a long story... I'm constrained by historic choices, which makes using the environment problematic. But you're right.
Hi Lukasz, On Tue, May 21, 2019 at 10:02:34AM +0100, Alex Kiernan wrote: > On Tue, May 21, 2019 at 9:37 AM Lukasz Majewski <lukma@denx.de> wrote: > > > > Hi Alex, > > > > > On Mon, May 20, 2019 at 8:23 AM Eugeniu Rosca <erosca@de.adit-jv.com> > > > wrote: > > > > > > > > > > > > > > Should it default to enabled if avb is used? > > > > > > > > I think at this specific moment in time, 'bcb' is orthogonal > > > > (meaning it is neither a direct, nor a reverse dependency) to any > > > > other Android feature in U-Boot. This could be re-assessed, if > > > > platform maintainers start to rely on 'bcb' in their U-Boot > > > > environments on regular basis. > > > > > > 'bcb' looks like something I'd be interested in, not running Android > > > at all... currently I (ab)use the bootcounter to communicate between > > > the kernel and U-Boot when I want to force a board through recovery, > > > > I don't know your exact use case, but wouldn't it be better to use envs > > (with redundancy) and fw_setenv / fw_printenv from Linux user space? > > > > Now those envs even support setting default values for u-boot (as there > > is now separate library used for it). Moreover there is OE/Yocto's > > recipe 'u-boot-fw-utils' which can be easily used and installed. That's a truly constructive suggestion. Nevertheless, I believe this would not work in case of CONFIG_ENV_IS_NOWHERE=y, which is how U-Boot is built and used by the developers in our organization. > > It's a long story... I'm constrained by historic choices, which makes > using the environment problematic. But you're right. > > -- > Alex Kiernan
On Tue, 21 May 2019 11:13:52 +0200 Eugeniu Rosca <erosca@de.adit-jv.com> wrote: > Hi Lukasz, > > On Tue, May 21, 2019 at 10:02:34AM +0100, Alex Kiernan wrote: > > On Tue, May 21, 2019 at 9:37 AM Lukasz Majewski <lukma@denx.de> > > wrote: > > > > > > Hi Alex, > > > > > > > On Mon, May 20, 2019 at 8:23 AM Eugeniu Rosca > > > > <erosca@de.adit-jv.com> wrote: > > > > > > > > > > > > > > > > > Should it default to enabled if avb is used? > > > > > > > > > > I think at this specific moment in time, 'bcb' is orthogonal > > > > > (meaning it is neither a direct, nor a reverse dependency) to > > > > > any other Android feature in U-Boot. This could be > > > > > re-assessed, if platform maintainers start to rely on 'bcb' > > > > > in their U-Boot environments on regular basis. > > > > > > > > 'bcb' looks like something I'd be interested in, not running > > > > Android at all... currently I (ab)use the bootcounter to > > > > communicate between the kernel and U-Boot when I want to force > > > > a board through recovery, > > > > > > I don't know your exact use case, but wouldn't it be better to > > > use envs (with redundancy) and fw_setenv / fw_printenv from Linux > > > user space? > > > > > > Now those envs even support setting default values for u-boot (as > > > there is now separate library used for it). Moreover there is > > > OE/Yocto's recipe 'u-boot-fw-utils' which can be easily used and > > > installed. > > That's a truly constructive suggestion. Nevertheless, I believe this > would not work in case of CONFIG_ENV_IS_NOWHERE=y, which is how U-Boot > is built and used by the developers in our organization. I don't mind to see Android's "bcd" supported in U-Boot (I'm even happy for it). And yes - the CONFIG_ENV_IS_NOWHERE means that one loads the default envs (created at build time) to RAM for booting. Just one note (maybe you will find it useful) - it is possible to specify the default envs from external file: https://lists.denx.de/pipermail/u-boot/2018-March/323347.html As we don't have memory to store envs we cannot adjust or pass data via it. > > > > > It's a long story... I'm constrained by historic choices, which > > makes using the environment problematic. But you're right. > > > > -- > > Alex Kiernan > Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
On Tue, May 21, 2019 at 10:14 AM Eugeniu Rosca <erosca@de.adit-jv.com> wrote: > > Hi Lukasz, > > On Tue, May 21, 2019 at 10:02:34AM +0100, Alex Kiernan wrote: > > On Tue, May 21, 2019 at 9:37 AM Lukasz Majewski <lukma@denx.de> wrote: > > > > > > Hi Alex, > > > > > > > On Mon, May 20, 2019 at 8:23 AM Eugeniu Rosca <erosca@de.adit-jv.com> > > > > wrote: > > > > > > > > > > > > > > > > > Should it default to enabled if avb is used? > > > > > > > > > > I think at this specific moment in time, 'bcb' is orthogonal > > > > > (meaning it is neither a direct, nor a reverse dependency) to any > > > > > other Android feature in U-Boot. This could be re-assessed, if > > > > > platform maintainers start to rely on 'bcb' in their U-Boot > > > > > environments on regular basis. > > > > > > > > 'bcb' looks like something I'd be interested in, not running Android > > > > at all... currently I (ab)use the bootcounter to communicate between > > > > the kernel and U-Boot when I want to force a board through recovery, > > > > > > I don't know your exact use case, but wouldn't it be better to use envs > > > (with redundancy) and fw_setenv / fw_printenv from Linux user space? > > > > > > Now those envs even support setting default values for u-boot (as there > > > is now separate library used for it). Moreover there is OE/Yocto's > > > recipe 'u-boot-fw-utils' which can be easily used and installed. > > That's a truly constructive suggestion. Nevertheless, I believe this > would not work in case of CONFIG_ENV_IS_NOWHERE=y, which is how U-Boot > is built and used by the developers in our organization. > That's how we build/run too, but with with hackery like this in a boot script to pick pieces out of the legacy world: mmc dev ${mmcdev} mmc read ${loadaddr} 1300 100 env import -c ${loadaddr} 20000 ethaddr Yes, it's ugly...
On Tue, May 21, 2019 at 10:24:10AM +0100, Alex Kiernan wrote: > On Tue, May 21, 2019 at 10:14 AM Eugeniu Rosca <erosca@de.adit-jv.com> wrote: [..] > > That's a truly constructive suggestion. Nevertheless, I believe this > > would not work in case of CONFIG_ENV_IS_NOWHERE=y, which is how U-Boot > > is built and used by the developers in our organization. > > > > That's how we build/run too, but with with hackery like this in a boot > script to pick pieces out of the legacy world: > > mmc dev ${mmcdev} > mmc read ${loadaddr} 1300 100 > env import -c ${loadaddr} 20000 ethaddr FWIW, we have a cascaded load of boot scripts combined with `env import -t` and/or `source`, which allows to control the boot media by changing one line in a text file. This might be super ugly in terms of vanilla standards, but it works in the development environment and that's what matters the most.
Hi Sam, On Mon, May 20, 2019 at 06:16:38PM +0300, Sam Protsenko wrote: > Hi Eugeniu, > > On Mon, May 20, 2019 at 10:23 AM Eugeniu Rosca <erosca@de.adit-jv.com> wrote: [..] > > Igor, Sam, what's your view on the above? Would you suggest creating > > a doc/README.android-bcb or there is a more elegant solution to it? > > > > Documentation would be nice. Although you already provided a generic > description of 'bcb' command in 'help bcb', the user often wants to > read more high-level documentation. I'd say, you can copy the > description from commit message to doc/README.android-bcb, extending > it with usual use-cases, and how this command can be used in those > use-cases. For example, your pseudo-code for reboot reason you > provided to me earlier, etc. Agreed. In my queue. > Also, it can be useful to mention if > Google have any requirements (mandatory or optional) for the > bootloader (misc partition, reboot reason, recovery, etc), and how > this 'bcb' command can help in those requirements implementation. Well, a subset of the Android bootloader requirements is publicly available via https://source.android.com/devices/bootloader, but I saw traces of the "Android Boot/Bootloader Requirements" document name mentioned in the descriptions of U-Boot commits received from our suppliers. I _do not_ have access to the latter. If anybody from Google sees this message and can share the document or a subset of it, that would be great. To be clear, the background behind the 'bcb' command is not because I was assigned the task to implement any of the Android bootloader requirements, but because I saw improper implementations of some of those requirements in the patches received from our supplier and wanted to showcase a better/U-Boot-compliant way to accomplish those. So, I don't give any commitment to support the Android-related parts in U-Boot, but will signal and express my opinion whenever any features backported from AOSP don't follow the patterns established in community. > > All that said, IMHO documentation/test wise: it's not critical in this > case, you can add that later, just to speed-up the whole development > process and divide it into iterations. But that's for maintainers to > decide, of course. > > Also, I've a feeling that we have another problem, more common than > just a documentation. At the moment we have a bunch of Android related > features, which don't have namespace separation on several levels: > - file/directories: we may want to move all Android related commands > to sub-directory > - commands: we may want to add main command called "android" for all > Android-related commands, or maybe just a prefix > - Kconfig: we may want to have some generic naming scheme for all > Android-related commands > - Documentation, tests: the same here > > So at some point we will probably need to discuss and fix that > somehow. Not here, of course :) Agree with the most of the bullets. However, WRT merging all the available Android commands into a single command, I see room for more discussion. Here is some valuable feedback from Ruslan Trofymenko: --- quote from https://patchwork.ozlabs.org/patch/1004002/#2049412 --- On Thu, 6 Dec 2018 at 03:31, Simon Glass <sjg@chromium.org> wrote: > Perhaps we need a new 'android' command with an 'ab_select' > subcommand? Then the automatica abbreviation will work. > Agree with all your previous comments, will send v2 shortly. But this one I'd like to leave as is (I will drop android_ prefix though). I think adding "android" command may lead to unneeded architecture complexity in future, e.g.: - we will need to re-work next commands as sub-commands of "android" command: fastboot, dtimg, mmc_swrite (which can be hard as ab_select command file has BSD license and other commands have GPL license) - we will need to implement sub-sub-commands (e.g. for "android dtimg dump ..." etc.) - having "android" command can be inconvenient for users and may break existing API (e.g. it will force users to use "android fastboot" instead of just "fastboot" command) - actually I don't see any namespace issues that can be solved by adding "android" command Also, in subsequent patch, I will add the dedicated menu option for Android-related commands and will pull all existing Android commands (along with ab_select) to that menu entry. So I hope it's fine with you if I leave this command as "ab_select"? --- end quote https://patchwork.ozlabs.org/patch/1004002/#2049412 --- In addition, based on the feedback from Alex Kiernan, 'bcb' might be useful in non-Android contexts. If so, then embedding it as sub-command into a higher level command (e.g. 'android') does not make much sense.
Hi Eugeniu, On Mon, 20 May 2019 at 01:23, Eugeniu Rosca <erosca@de.adit-jv.com> wrote: > > Hi Simon > cc: Sam, Igor, feel free to correct/augment anything of below > > On Sat, May 18, 2019 at 10:33:02AM -0600, Simon Glass wrote: > > Hi Eugeniu, > > > > On Fri, 17 May 2019 at 08:46, Eugeniu Rosca <erosca@de.adit-jv.com> wrote: > > > > > > 'Bootloader Control Block' (BCB) is a well established term/acronym in > > > the Android namespace which refers to a location in a dedicated raw > > > (i.e. FS-unaware) flash (e.g. eMMC) partition, usually called "misc", > > > which is used as media for exchanging messages between Android userspace > > > (particularly recovery [1]) and an Android-capable bootloader. > > > > [..] > > > > Where is this documented? Perhaps it should go in README.avb2? > > README.avb2 is solely about the "verified/secure" booting of Android, > while the 'bcb' command proposed in this patch can be useful both in > verified boot scenarios (e.g. full-featured U-Boot builds), as well > as in non-avb ones (e.g. development, PoC, demos, custom > configurations). Thus, I think that README.avb2 is not the best > place for 'bcb'. > > More generally, as somebody who uses git as an extension of himself, I > am quite happy with commit-only documentation. OTOH, for those who > receive U-Boot in tarballs or expect source-level docs/tutorials, I > agree that having the 'bcb' described in a README might be helpful. > > I can identify two Android-dedicated README files, but none of them > seems to be suitable for the new command: > - doc/README.android-fastboot > - doc/README.avb2 > > Igor, Sam, what's your view on the above? Would you suggest creating > a doc/README.android-bcb or there is a more elegant solution to it? How about a new README.android which links to the other two and adds your new info? > > > > > Should it default to enabled if avb is used? > > I think at this specific moment in time, 'bcb' is orthogonal (meaning it > is neither a direct, nor a reverse dependency) to any other Android > feature in U-Boot. This could be re-assessed, if platform maintainers > start to rely on 'bcb' in their U-Boot environments on regular basis. OK. Also is there a sandbox driver for this? We should have a test. Regards, Simon
Hi Eugeniu, On Tue, May 21, 2019 at 2:20 PM Eugeniu Rosca <erosca@de.adit-jv.com> wrote: > > Hi Sam, > > On Mon, May 20, 2019 at 06:16:38PM +0300, Sam Protsenko wrote: > > Hi Eugeniu, > > > > On Mon, May 20, 2019 at 10:23 AM Eugeniu Rosca <erosca@de.adit-jv.com> wrote: > [..] > > > Igor, Sam, what's your view on the above? Would you suggest creating > > > a doc/README.android-bcb or there is a more elegant solution to it? > > > > > > > Documentation would be nice. Although you already provided a generic > > description of 'bcb' command in 'help bcb', the user often wants to > > read more high-level documentation. I'd say, you can copy the > > description from commit message to doc/README.android-bcb, extending > > it with usual use-cases, and how this command can be used in those > > use-cases. For example, your pseudo-code for reboot reason you > > provided to me earlier, etc. > > Agreed. In my queue. > Just to be clear: can we expect it to be sent in v3, or it will be separate patch-set? > > Also, it can be useful to mention if > > Google have any requirements (mandatory or optional) for the > > bootloader (misc partition, reboot reason, recovery, etc), and how > > this 'bcb' command can help in those requirements implementation. > > Well, a subset of the Android bootloader requirements is publicly > available via https://source.android.com/devices/bootloader, but I saw > traces of the "Android Boot/Bootloader Requirements" document name > mentioned in the descriptions of U-Boot commits received from our > suppliers. I _do not_ have access to the latter. If anybody from Google > sees this message and can share the document or a subset of it, that > would be great. > > To be clear, the background behind the 'bcb' command is not because I > was assigned the task to implement any of the Android bootloader > requirements, but because I saw improper implementations of some of > those requirements in the patches received from our supplier and > wanted to showcase a better/U-Boot-compliant way to accomplish those. > > So, I don't give any commitment to support the Android-related parts in > U-Boot, but will signal and express my opinion whenever any features > backported from AOSP don't follow the patterns established in community. > Sure, I just meant that it could be nice to mention it in the documentation, that 'bcb' command can help in those Google requirements implementation. > > > > All that said, IMHO documentation/test wise: it's not critical in this > > case, you can add that later, just to speed-up the whole development > > process and divide it into iterations. But that's for maintainers to > > decide, of course. > > > > Also, I've a feeling that we have another problem, more common than > > just a documentation. At the moment we have a bunch of Android related > > features, which don't have namespace separation on several levels: > > - file/directories: we may want to move all Android related commands > > to sub-directory > > - commands: we may want to add main command called "android" for all > > Android-related commands, or maybe just a prefix > > - Kconfig: we may want to have some generic naming scheme for all > > Android-related commands > > - Documentation, tests: the same here > > > > So at some point we will probably need to discuss and fix that > > somehow. Not here, of course :) > > Agree with the most of the bullets. However, WRT merging all the > available Android commands into a single command, I see room for > more discussion. Here is some valuable feedback from Ruslan Trofymenko: > > --- quote from https://patchwork.ozlabs.org/patch/1004002/#2049412 --- > On Thu, 6 Dec 2018 at 03:31, Simon Glass <sjg@chromium.org> wrote: > > Perhaps we need a new 'android' command with an 'ab_select' > > subcommand? Then the automatica abbreviation will work. > > > > Agree with all your previous comments, will send v2 shortly. But this > one I'd like to leave as is (I will drop android_ prefix though). I > think adding "android" command may lead to unneeded architecture > complexity in future, e.g.: > - we will need to re-work next commands as sub-commands of "android" > command: fastboot, dtimg, mmc_swrite (which can be hard as ab_select > command file has BSD license and other commands have GPL license) > - we will need to implement sub-sub-commands (e.g. for "android dtimg > dump ..." etc.) > - having "android" command can be inconvenient for users and may > break existing API (e.g. it will force users to use "android fastboot" > instead of just "fastboot" command) > - actually I don't see any namespace issues that can be solved by > adding "android" command > > Also, in subsequent patch, I will add the dedicated menu option for > Android-related commands and will pull all existing Android commands > (along with ab_select) to that menu entry. > > So I hope it's fine with you if I leave this command as "ab_select"? > --- end quote https://patchwork.ozlabs.org/patch/1004002/#2049412 --- > > In addition, based on the feedback from Alex Kiernan, 'bcb' might be > useful in non-Android contexts. If so, then embedding it as sub-command > into a higher level command (e.g. 'android') does not make much sense. > Yeah, let's keep it aside of this thread, I realize it's off-topic :) We probably just need to make sure that all Android-related commands are located in corresponding menu in menuconfig. > -- > Best Regards, > Eugeniu.
Hi Simon, On Tue, May 21, 2019 at 10:43:04AM -0600, Simon Glass wrote: > On Mon, 20 May 2019 at 01:23, Eugeniu Rosca <erosca@de.adit-jv.com> wrote: [..] > > I can identify two Android-dedicated README files, but none of them > > seems to be suitable for the new command: > > - doc/README.android-fastboot > > - doc/README.avb2 > > > > Igor, Sam, what's your view on the above? Would you suggest creating > > a doc/README.android-bcb or there is a more elegant solution to it? > > How about a new README.android which links to the other two and adds > your new info? How about below directory structure: u-boot $ tree doc/android doc/android ├── avb2.txt ├── bcb.txt └── fastboot.txt > > > > > > > > > Should it default to enabled if avb is used? > > > > I think at this specific moment in time, 'bcb' is orthogonal (meaning it > > is neither a direct, nor a reverse dependency) to any other Android > > feature in U-Boot. This could be re-assessed, if platform maintainers > > start to rely on 'bcb' in their U-Boot environments on regular basis. > > OK. Also is there a sandbox driver for this? We should have a test. Emulating and exposing MMC devices in sandbox should be the only prerequisite for sandbox testing and this seems to be already supported via drivers/mmc/sandbox_mmc.c. However, to be honest, I was unsuccessful bringing up the MMC devices on sandbox in the past. Particularly, booting the latest sandbox U-Boot (CMD_MMC=y) I get: => mmc list No MMC device available I think there is something elementary which I am missing? Regardless, I need some more days to implement the test and repartition the README files. I think Sam would appreciate if you can provide your Ack to the series as-is (it was extensively statically and dynamically tested on R-Car H3ULCB) and I submit the doc/test updates separately. Otherwise, I will push the next revision hopefully in a week or so. > > Regards, > Simon
Hi Eugeniu, On Tue, 21 May 2019 at 11:32, Eugeniu Rosca <erosca@de.adit-jv.com> wrote: > > Hi Simon, > > On Tue, May 21, 2019 at 10:43:04AM -0600, Simon Glass wrote: > > On Mon, 20 May 2019 at 01:23, Eugeniu Rosca <erosca@de.adit-jv.com> wrote: > [..] > > > I can identify two Android-dedicated README files, but none of them > > > seems to be suitable for the new command: > > > - doc/README.android-fastboot > > > - doc/README.avb2 > > > > > > Igor, Sam, what's your view on the above? Would you suggest creating > > > a doc/README.android-bcb or there is a more elegant solution to it? > > > > How about a new README.android which links to the other two and adds > > your new info? > > How about below directory structure: > > u-boot $ tree doc/android > doc/android > ├── avb2.txt > ├── bcb.txt > └── fastboot.txt > Sounds good > > > > > > > > > > > > > Should it default to enabled if avb is used? > > > > > > I think at this specific moment in time, 'bcb' is orthogonal (meaning it > > > is neither a direct, nor a reverse dependency) to any other Android > > > feature in U-Boot. This could be re-assessed, if platform maintainers > > > start to rely on 'bcb' in their U-Boot environments on regular basis. > > > > OK. Also is there a sandbox driver for this? We should have a test. > > Emulating and exposing MMC devices in sandbox should be the only > prerequisite for sandbox testing and this seems to be already supported > via drivers/mmc/sandbox_mmc.c. However, to be honest, I was unsuccessful > bringing up the MMC devices on sandbox in the past. Particularly, > booting the latest sandbox U-Boot (CMD_MMC=y) I get: > > => mmc list > No MMC device available > > I think there is something elementary which I am missing? Probably need to copy an mmc node over from test.dts to sandbox.dts > > Regardless, I need some more days to implement the test and repartition > the README files. I think Sam would appreciate if you can provide your > Ack to the series as-is (it was extensively statically and dynamically > tested on R-Car H3ULCB) and I submit the doc/test updates separately. > Otherwise, I will push the next revision hopefully in a week or so. That's OK with me, but let me review it first. Regards, Simon
Hi Eugeniu, On Fri, 17 May 2019 at 08:46, Eugeniu Rosca <erosca@de.adit-jv.com> wrote: > > 'Bootloader Control Block' (BCB) is a well established term/acronym in > the Android namespace which refers to a location in a dedicated raw > (i.e. FS-unaware) flash (e.g. eMMC) partition, usually called "misc", > which is used as media for exchanging messages between Android userspace > (particularly recovery [1]) and an Android-capable bootloader. > > On higher level, this allows implementing a subset of Android Bootloader > Requirements [2], amongst which is the Android-specific bootloader > flow [3]. Regardless how the latter is implemented in U-Boot ([3] being > the most memorable example), reading/writing/dumping the BCB fields in > the development process from inside the U-Boot is a convenient feature. > Hence, make it available to the users. > > Some usage examples of the new command recorded on R-Car H3ULCB-KF > ('>>>' is an overlay on top of the original console output): > > => bcb > bcb - Load/set/clear/test/dump/store Android BCB fields > > Usage: > bcb load <dev> <part> - load BCB from mmc <dev>:<part> > bcb set <field> <val> - set BCB <field> to <val> > bcb clear [<field>] - clear BCB <field> or all fields > bcb test <field> <op> <val> - test BCB <field> against <val> > bcb dump <field> - dump BCB <field> > bcb store - store BCB back to mmc > > Legend: > <dev> - MMC device index containing the BCB partition > <part> - MMC partition index or name containing the BCB > <field> - one of {command,status,recovery,stage,reserved} > <op> - the binary operator used in 'bcb test': > '=' returns true if <val> matches the string stored in <field> > '~' returns true if <val> matches a subset of <field>'s string > <val> - string/text provided as input to bcb {set,test} > NOTE: any ':' character in <val> will be replaced by line feed > during 'bcb set' and used as separator by upper layers > > => bcb dump command > Error: BCB not loaded! > >>> Users must specify mmc device and partition before any other call > > => bcb load 1 misc > => bcb load 1 1 > >>> The two calls are equivalent (assuming "misc" has index 1) > > => bcb dump command > 00000000: 62 6f 6f 74 6f 6e 63 65 2d 73 68 65 6c 6c 00 72 bootonce-shell.r > 00000010: 79 00 72 00 00 00 00 00 00 00 00 00 00 00 00 00 y.r............. > >>> The output is in binary/string format for convenience > >>> The output size matches the size of inspected BCB field > >>> (32 bytes in case of 'command') > > => bcb test command = bootonce-shell && echo true > true > => bcb test command = bootonce-shell- && echo true > => bcb test command = bootonce-shel && echo true > >>> The '=' operator returns 'true' on perfect match > > => bcb test command ~ bootonce-shel && echo true > true > => bcb test command ~ bootonce-shell && echo true > true > >>> The '~' operator returns 'true' on substring match > > => bcb set command recovery > => bcb dump command > 00000000: 72 65 63 6f 76 65 72 79 00 73 68 65 6c 6c 00 72 recovery.shell.r > 00000010: 79 00 72 00 00 00 00 00 00 00 00 00 00 00 00 00 y.r............. > >>> The new value is NULL-terminated and stored in the BCB field > > => bcb set recovery "msg1:msg2:msg3" > => bcb dump recovery > 00000040: 6d 73 67 31 0a 6d 73 67 32 0a 6d 73 67 33 00 00 msg1.msg2.msg3.. > 00000050: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > 00000060: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > >>> --- snip --- > >>> Every ':' is replaced by line-feed '\n' (0xA). The latter is used > >>> as separator between individual commands by Android userspace > > => bcb store > >>> Flush/store the BCB structure to MMC > > [1] https://android.googlesource.com/platform/bootable/recovery > [2] https://source.android.com/devices/bootloader > [3] https://patchwork.ozlabs.org/patch/746835/ > ("[U-Boot,5/6] Initial support for the Android Bootloader flow") As discussed, please add these docs somewhere in the U-Boot tree (can be in a separate patch) > > Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com> > --- > v2: > - [Heinrich Schuchardt] Implement sub-commands via U_BOOT_CMD_MKENT. > - Polished the code. Ensured no warnings returned by sparse, smatch, > `cppcheck --force --enable=all --inconclusive`, make W=1. > - Tested on R-Car-H3-ES20 ULCB-KF. > > v1: > - https://patchwork.ozlabs.org/patch/1080395/ > --- > cmd/Kconfig | 17 +++ > cmd/Makefile | 1 + > cmd/bcb.c | 330 +++++++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 348 insertions(+) > create mode 100644 cmd/bcb.c > > diff --git a/cmd/Kconfig b/cmd/Kconfig > index 0d36da2a5c43..495bc1052f50 100644 > --- a/cmd/Kconfig > +++ b/cmd/Kconfig > @@ -631,6 +631,23 @@ config CMD_ADC > Shows ADC device info and permit printing one-shot analog converted > data from a named Analog to Digital Converter. > > +config CMD_BCB > + bool "bcb" > + depends on MMC > + depends on PARTITIONS > + help > + Read/modify/write the fields of Bootloader Control Block, usually > + stored on the flash "misc" partition with its structure defined in: > + https://android.googlesource.com/platform/bootable/recovery/+/master/ > + bootloader_message/include/bootloader_message/bootloader_message.h > + > + Some real-life use-cases include (but are not limited to): > + - Determine the "boot reason" (and act accordingly): > + https://source.android.com/devices/bootloader/boot-reason > + - Get/pass a list of commands from/to recovery: > + https://android.googlesource.com/platform/bootable/recovery > + - Inspect/dump the contents of the BCB fields > + > config CMD_BIND > bool "bind/unbind - Bind or unbind a device to/from a driver" > depends on DM > diff --git a/cmd/Makefile b/cmd/Makefile > index 7864fcf95c36..8f31b478eb1b 100644 > --- a/cmd/Makefile > +++ b/cmd/Makefile > @@ -16,6 +16,7 @@ obj-$(CONFIG_CMD_ADC) += adc.o > obj-$(CONFIG_CMD_ARMFLASH) += armflash.o > obj-y += blk_common.o > obj-$(CONFIG_CMD_SOURCE) += source.o > +obj-$(CONFIG_CMD_BCB) += bcb.o > obj-$(CONFIG_CMD_BDI) += bdinfo.o > obj-$(CONFIG_CMD_BEDBUG) += bedbug.o > obj-$(CONFIG_CMD_BIND) += bind.o > diff --git a/cmd/bcb.c b/cmd/bcb.c > new file mode 100644 > index 000000000000..5d8486684542 > --- /dev/null > +++ b/cmd/bcb.c > @@ -0,0 +1,330 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Copyright (C) 2019 Eugeniu Rosca <rosca.eugeniu@gmail.com> > + * > + * Command to read/modify/write Android BCB fields > + */ > + > +#include <android_bootloader_message.h> > +#include <command.h> > +#include <common.h> > +#include <malloc.h> > + > +enum bcb_cmd { > + BCB_CMD_LOAD, > + BCB_CMD_FIELD_SET, > + BCB_CMD_FIELD_CLEAR, > + BCB_CMD_FIELD_TEST, > + BCB_CMD_FIELD_DUMP, > + BCB_CMD_STORE, > +}; > + > +static int bcb_dev = -1; > +static int bcb_part = -1; > +static struct bootloader_message bcb = { { 0 } }; > + > +static int bcb_cmd_get(char *cmd) > +{ > + if (!strncmp(cmd, "load", sizeof("load"))) > + return BCB_CMD_LOAD; > + if (!strncmp(cmd, "set", sizeof("set"))) > + return BCB_CMD_FIELD_SET; > + if (!strncmp(cmd, "clear", sizeof("clear"))) > + return BCB_CMD_FIELD_CLEAR; > + if (!strncmp(cmd, "test", sizeof("test"))) > + return BCB_CMD_FIELD_TEST; > + if (!strncmp(cmd, "store", sizeof("store"))) > + return BCB_CMD_STORE; > + if (!strncmp(cmd, "dump", sizeof("dump"))) > + return BCB_CMD_FIELD_DUMP; > + else > + return -1; > +} > + > +static int do_bcb_load(cmd_tbl_t *cmdtp, int flag, int argc, > + char * const argv[]) > +{ > + struct blk_desc *desc; > + disk_partition_t info; > + u64 cnt; > + char *endp; > + int part; > + > + if (argc != 3) > + return CMD_RET_USAGE; > + > + if (blk_get_device_by_str("mmc", argv[1], &desc) < 0) Error codes should be reported. > + goto err_1; > + > + part = simple_strtoul(argv[2], &endp, 0); > + if (*endp == '\0') { > + if (part_get_info(desc, part, &info)) > + goto err_1; > + } else { > + part = part_get_info_by_name(desc, argv[2], &info); > + if (part < 0) > + goto err_1; > + } > + > + cnt = DIV_ROUND_UP(sizeof(struct bootloader_message), info.blksz); > + if (cnt > info.size) > + goto err_2; > + > + if (blk_dread(desc, info.start, cnt, &bcb) != cnt) > + goto err_1; > + > + bcb_dev = desc->devnum; > + bcb_part = part; > + debug("%s: Loaded from mmc %d:%d\n", __func__, bcb_dev, bcb_part); > + > + return CMD_RET_SUCCESS; > +err_1: > + printf("Error: Failed to read from mmc %s:%s\n", argv[1], argv[2]); Add error code here. > + goto err; > +err_2: > + printf("Error: mmc %s:%s too small!", argv[1], argv[2]); > + goto err; > +err: > + bcb_dev = -1; > + bcb_part = -1; > + return CMD_RET_FAILURE; > +} > + > +static int bcb_is_misused(int argc, char *const argv[]) > +{ > + if (bcb_dev < 0 || bcb_part < 0) { > + printf("Error: BCB not loaded!\n"); > + return -1; > + } > + switch (bcb_cmd_get(argv[0])) { Why have a switch() here, when you could just put the below code in each function? Or put the call to this function from the main do_bcb_set() function. > + case BCB_CMD_LOAD: > + /* Dedicated arg handling */ > + break; > + case BCB_CMD_FIELD_SET: > + if (argc != 3) > + goto err; > + break; > + case BCB_CMD_FIELD_TEST: > + if (argc != 4) > + goto err; > + break; > + case BCB_CMD_FIELD_CLEAR: > + if (argc != 1 && argc != 2) > + goto err; > + break; > + case BCB_CMD_STORE: > + if (argc != 1) > + goto err; > + break; > + case BCB_CMD_FIELD_DUMP: > + if (argc != 2) > + goto err; > + break; > + default: > + debug("%s: Error: Unexpected BCB command\n", __func__); > + return -1; > + } > + > + return 0; > +err: > + printf("Error: Bad usage of 'bcb %s'\n", argv[0]); > + return -1; > +} > + > +static int > +bcb_field_get(char *name, char **field, int *size) > +{ > + if (!strncmp(name, "command", sizeof("command"))) { > + *field = bcb.command; > + *size = sizeof(bcb.command); > + } else if (!strncmp(name, "status", sizeof("status"))) { > + *field = bcb.status; > + *size = sizeof(bcb.status); > + } else if (!strncmp(name, "recovery", sizeof("recovery"))) { > + *field = bcb.recovery; > + *size = sizeof(bcb.recovery); > + } else if (!strncmp(name, "stage", sizeof("stage"))) { > + *field = bcb.stage; > + *size = sizeof(bcb.stage); > + } else if (!strncmp(name, "reserved", sizeof("reserved"))) { > + *field = bcb.reserved; > + *size = sizeof(bcb.reserved); > + } else { > + printf("Error: Unknown field '%s'\n", name); > + return -1; > + } > + return 0; > +} > + > +static int do_bcb_set(cmd_tbl_t *cmdtp, int flag, int argc, > + char * const argv[]) > +{ > + int size, len; > + char *field, *str, *found, *keep; > + > + if (bcb_is_misused(argc, argv)) > + return CMD_RET_FAILURE; > + > + if (bcb_field_get(argv[1], &field, &size)) > + return CMD_RET_FAILURE; > + > + len = strlen(argv[2]); > + if (len >= size) { > + printf("Error: sizeof('%s') = %d >= %d = sizeof(bcb.%s)\n", > + argv[2], len, size, argv[1]); > + return CMD_RET_FAILURE; > + } > + str = strdup(argv[2]); It is OK to change the args if you like. > + keep = str; > + > + field[0] = '\0'; > + while ((found = strsep(&str, ":"))) { > + if (field[0] != '\0') > + strcat(field, "\n"); > + strcat(field, found); > + } > + > + free(keep); > + return CMD_RET_SUCCESS; > +} > + > +static int do_bcb_clear(cmd_tbl_t *cmdtp, int flag, int argc, > + char * const argv[]) > +{ > + int size; > + char *field; > + > + if (bcb_is_misused(argc, argv)) > + return CMD_RET_FAILURE; > + > + if (argc == 1) { > + memset(&bcb, 0, sizeof(bcb)); > + return CMD_RET_SUCCESS; > + } > + > + if (bcb_field_get(argv[1], &field, &size)) > + return CMD_RET_FAILURE; > + > + memset(field, 0, size); > + return CMD_RET_SUCCESS; > +} > + > +static int do_bcb_dump(cmd_tbl_t *cmdtp, int flag, int argc, > + char * const argv[]) > +{ > + int size; > + char *field; > + > + if (bcb_is_misused(argc, argv)) > + return CMD_RET_FAILURE; You should return CMD_RET_USAGE if there is a usage problem. > + > + if (bcb_field_get(argv[1], &field, &size)) > + return CMD_RET_FAILURE; > + > + print_buffer((ulong)field - (ulong)&bcb, (void *)field, 1, size, 16); Please add newline before return > + return CMD_RET_SUCCESS; > +} > + > +static int do_bcb_test(cmd_tbl_t *cmdtp, int flag, int argc, > + char * const argv[]) > +{ > + int size; > + char *field; > + > + if (bcb_is_misused(argc, argv)) > + return CMD_RET_FAILURE; > + > + if (bcb_field_get(argv[1], &field, &size)) > + return CMD_RET_FAILURE; > + > + if (!strncmp(argv[2], "=", sizeof("="))) { Think about code size: if (*argv[2] == '=') > + if (!strncmp(argv[3], field, size)) > + return CMD_RET_SUCCESS; > + else > + return CMD_RET_FAILURE; > + } else if (!strncmp(argv[2], "~", sizeof("~"))) { > + if (!strstr(field, argv[3])) > + return CMD_RET_FAILURE; > + else > + return CMD_RET_SUCCESS; > + } else { > + printf("Error: Unknown operator '%s'\n", argv[2]); > + } > + return CMD_RET_FAILURE; > +} > + > +static int do_bcb_store(cmd_tbl_t *cmdtp, int flag, int argc, > + char * const argv[]) > +{ > + struct blk_desc *desc; > + disk_partition_t info; > + u64 cnt; > + > + if (bcb_is_misused(argc, argv)) > + return CMD_RET_FAILURE; > + > + desc = blk_get_devnum_by_type(IF_TYPE_MMC, bcb_dev); > + if (!desc) > + goto err; > + > + if (part_get_info(desc, bcb_part, &info)) > + goto err; Again, error numbers need to be printed. > + > + cnt = DIV_ROUND_UP(sizeof(struct bootloader_message), info.blksz); > + > + if (blk_dwrite(desc, info.start, cnt, &bcb) != cnt) > + goto err; > + > + return CMD_RET_SUCCESS; > +err: > + printf("Error: Failed to write to mmc %d:%d\n", bcb_dev, bcb_part); > + return CMD_RET_FAILURE; > +} > + > +static cmd_tbl_t cmd_bcb_sub[] = { > + U_BOOT_CMD_MKENT(load, CONFIG_SYS_MAXARGS, 1, do_bcb_load, "", ""), > + U_BOOT_CMD_MKENT(set, CONFIG_SYS_MAXARGS, 1, do_bcb_set, "", ""), > + U_BOOT_CMD_MKENT(clear, CONFIG_SYS_MAXARGS, 1, do_bcb_clear, "", ""), > + U_BOOT_CMD_MKENT(test, CONFIG_SYS_MAXARGS, 1, do_bcb_test, "", ""), > + U_BOOT_CMD_MKENT(dump, CONFIG_SYS_MAXARGS, 1, do_bcb_dump, "", ""), > + U_BOOT_CMD_MKENT(store, CONFIG_SYS_MAXARGS, 1, do_bcb_store, "", ""), > +}; > + > +static int do_bcb(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[]) > +{ > + cmd_tbl_t *c; > + > + if (argc < 2) > + return CMD_RET_USAGE; > + > + argc--; > + argv++; > + > + c = find_cmd_tbl(argv[0], cmd_bcb_sub, ARRAY_SIZE(cmd_bcb_sub)); > + if (c) > + return c->cmd(cmdtp, flag, argc, argv); > + > + return CMD_RET_USAGE; > +} > + > +U_BOOT_CMD( > + bcb, CONFIG_SYS_MAXARGS, 1, do_bcb, > + "Load/set/clear/test/dump/store Android BCB fields", > + "load <dev> <part> - load BCB from mmc <dev>:<part>\n" > + "bcb set <field> <val> - set BCB <field> to <val>\n" > + "bcb clear [<field>] - clear BCB <field> or all fields\n" > + "bcb test <field> <op> <val> - test BCB <field> against <val>\n" > + "bcb dump <field> - dump BCB <field>\n" > + "bcb store - store BCB back to mmc\n" > + "\n" > + "Legend:\n" > + "<dev> - MMC device index containing the BCB partition\n" > + "<part> - MMC partition index or name containing the BCB\n" > + "<field> - one of {command,status,recovery,stage,reserved}\n" > + "<op> - the binary operator used in 'bcb test':\n" > + " '=' returns true if <val> matches the string stored in <field>\n" > + " '~' returns true if <val> matches a subset of <field>'s string\n" > + "<val> - string/text provided as input to bcb {set,test}\n" > + " NOTE: any ':' character in <val> will be replaced by line feed\n" > + " during 'bcb set' and used as separator by upper layers\n" > +); > -- > 2.21.0 > Regards, Simon
Hi Simon, Thanks for the review. Would you please reply to my questions below? On Tue, May 21, 2019 at 06:53:29PM -0600, Simon Glass wrote: > Hi Eugeniu, > > On Fri, 17 May 2019 at 08:46, Eugeniu Rosca <erosca@de.adit-jv.com> wrote: > > > > > > [1] https://android.googlesource.com/platform/bootable/recovery > > [2] https://source.android.com/devices/bootloader > > [3] https://patchwork.ozlabs.org/patch/746835/ > > ("[U-Boot,5/6] Initial support for the Android Bootloader flow") > > As discussed, please add these docs somewhere in the U-Boot tree (can > be in a separate patch) Sure. > > + if (blk_get_device_by_str("mmc", argv[1], &desc) < 0) > > Error codes should be reported. > > + printf("Error: Failed to read from mmc %s:%s\n", argv[1], argv[2]); > > Add error code here. Will address. > > + switch (bcb_cmd_get(argv[0])) { > > Why have a switch() here, when you could just put the below code in > each function? Or put the call to this function from the main > do_bcb_set() function. s/do_bcb_set/do_bcb/ ? The switch() is there to tell the user that he misused specifically {sub}command 'bcb X'. If we just throw CMD_RET_USAGE (which means printing full-blown help text) on _any_ kind of command misuse , we don't help the user _at all_, IMHO. I would consider being user-friendly as the higher and more important policy. However, if you prioritize the code size over user experience, then I am open to rewrite the function. Would you please clarify which policy takes precedence between the two? > > > + str = strdup(argv[2]); > > It is OK to change the args if you like. I will try getting rid of strdup. > > + if (bcb_is_misused(argc, argv)) > > + return CMD_RET_FAILURE; > > You should return CMD_RET_USAGE if there is a usage problem. This again connects with my previous statement on the user-experience. I would like to tell the user where exactly he did the mistake in using the 'bcb' rather than throwing a full-blown help message. > > + if (bcb_field_get(argv[1], &field, &size)) > > + return CMD_RET_FAILURE; > > + > > + print_buffer((ulong)field - (ulong)&bcb, (void *)field, 1, size, 16); > > Please add newline before return Fine. > > + if (!strncmp(argv[2], "=", sizeof("="))) { > > Think about code size: > > if (*argv[2] == '=') Checking a single character will not detect the difference between '=' and '=anything' So, I have to validate, in addition, that the NULL terminator is there. But I agree with the comment in general. > > + if (part_get_info(desc, bcb_part, &info)) > > + goto err; > > Again, error numbers need to be printed. Will address. > Regards, > Simon Thanks!
Hi Sam, On Tue, May 21, 2019 at 07:46:22PM +0300, Sam Protsenko wrote: > On Tue, May 21, 2019 at 2:20 PM Eugeniu Rosca <erosca@de.adit-jv.com> wrote: [..] > > Agreed. In my queue. > > Just to be clear: can we expect it to be sent in v3, or it will be > separate patch-set? We'll have a v3 for fixing Simon's review comments. I will append the docs if I have time.
Hi Eugeniu, On Wed, 22 May 2019 at 01:11, Eugeniu Rosca <erosca@de.adit-jv.com> wrote: > > Hi Simon, > > Thanks for the review. Would you please reply to my questions below? > > On Tue, May 21, 2019 at 06:53:29PM -0600, Simon Glass wrote: > > Hi Eugeniu, > > > > On Fri, 17 May 2019 at 08:46, Eugeniu Rosca <erosca@de.adit-jv.com> wrote: > > > > > > > > > [1] https://android.googlesource.com/platform/bootable/recovery > > > [2] https://source.android.com/devices/bootloader > > > [3] https://patchwork.ozlabs.org/patch/746835/ > > > ("[U-Boot,5/6] Initial support for the Android Bootloader flow") > > > > As discussed, please add these docs somewhere in the U-Boot tree (can > > be in a separate patch) > > Sure. > > > > + if (blk_get_device_by_str("mmc", argv[1], &desc) < 0) > > > > Error codes should be reported. > > > > + printf("Error: Failed to read from mmc %s:%s\n", argv[1], argv[2]); > > > > Add error code here. > > Will address. > > > > + switch (bcb_cmd_get(argv[0])) { > > > > Why have a switch() here, when you could just put the below code in > > each function? Or put the call to this function from the main > > do_bcb_set() function. > > s/do_bcb_set/do_bcb/ ? Yes > > The switch() is there to tell the user that he misused specifically > {sub}command 'bcb X'. If we just throw CMD_RET_USAGE (which means > printing full-blown help text) on _any_ kind of command misuse , we > don't help the user _at all_, IMHO. I would consider being user-friendly > as the higher and more important policy. However, if you prioritize the > code size over user experience, then I am open to rewrite the function. > Would you please clarify which policy takes precedence between the two? I think code size in commands is not the major priority, although we do tend to try to keep it moderate. Here I wasn't suggesting removing code. I was just suggesting that the error handling could be in each specific command's function. So take the code out of each case statement and put into the function that it relates to. Or continue to use the generic error handler, but call it from the generic function. It seems like we have: do_bcb() { switch (cmd) { case CMD_FRED do_bcb_fred(); ... } ... } do_bcb_fred() { check_args(CMD_FRED); ... } check_args(int cmd) { switch (cmd) { case CMD_FRED: print error } } I mean, put 'print error' inside do_bcb_fred() or, call check_args() from do_bcb() > > > > > > + str = strdup(argv[2]); > > > > It is OK to change the args if you like. > > I will try getting rid of strdup. > > > > + if (bcb_is_misused(argc, argv)) > > > + return CMD_RET_FAILURE; > > > > You should return CMD_RET_USAGE if there is a usage problem. > > This again connects with my previous statement on the user-experience. > I would like to tell the user where exactly he did the mistake in using > the 'bcb' rather than throwing a full-blown help message. Yes this is good. See above where I tried to explain what I mean. > > > > + if (bcb_field_get(argv[1], &field, &size)) > > > + return CMD_RET_FAILURE; > > > + > > > + print_buffer((ulong)field - (ulong)&bcb, (void *)field, 1, size, 16); > > > > Please add newline before return > > Fine. > > > > + if (!strncmp(argv[2], "=", sizeof("="))) { > > > > Think about code size: > > > > if (*argv[2] == '=') > > Checking a single character will not detect the difference between > '=' and '=anything' So, I have to validate, in addition, that the > NULL terminator is there. But I agree with the comment in general. Yes, understood (I would calll this 'nul', BTW) [..] Regards, Simon
Hello Simon,
I am really grateful for your review comments.
I think I tackled all of them in
https://patchwork.ozlabs.org/cover/1104242/
("[U-Boot,v3,0/3] Add 'bcb' command to read/modify/write Android BCB")
I would appreciate if you can have one more/final look.
diff --git a/cmd/Kconfig b/cmd/Kconfig index 0d36da2a5c43..495bc1052f50 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -631,6 +631,23 @@ config CMD_ADC Shows ADC device info and permit printing one-shot analog converted data from a named Analog to Digital Converter. +config CMD_BCB + bool "bcb" + depends on MMC + depends on PARTITIONS + help + Read/modify/write the fields of Bootloader Control Block, usually + stored on the flash "misc" partition with its structure defined in: + https://android.googlesource.com/platform/bootable/recovery/+/master/ + bootloader_message/include/bootloader_message/bootloader_message.h + + Some real-life use-cases include (but are not limited to): + - Determine the "boot reason" (and act accordingly): + https://source.android.com/devices/bootloader/boot-reason + - Get/pass a list of commands from/to recovery: + https://android.googlesource.com/platform/bootable/recovery + - Inspect/dump the contents of the BCB fields + config CMD_BIND bool "bind/unbind - Bind or unbind a device to/from a driver" depends on DM diff --git a/cmd/Makefile b/cmd/Makefile index 7864fcf95c36..8f31b478eb1b 100644 --- a/cmd/Makefile +++ b/cmd/Makefile @@ -16,6 +16,7 @@ obj-$(CONFIG_CMD_ADC) += adc.o obj-$(CONFIG_CMD_ARMFLASH) += armflash.o obj-y += blk_common.o obj-$(CONFIG_CMD_SOURCE) += source.o +obj-$(CONFIG_CMD_BCB) += bcb.o obj-$(CONFIG_CMD_BDI) += bdinfo.o obj-$(CONFIG_CMD_BEDBUG) += bedbug.o obj-$(CONFIG_CMD_BIND) += bind.o diff --git a/cmd/bcb.c b/cmd/bcb.c new file mode 100644 index 000000000000..5d8486684542 --- /dev/null +++ b/cmd/bcb.c @@ -0,0 +1,330 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Copyright (C) 2019 Eugeniu Rosca <rosca.eugeniu@gmail.com> + * + * Command to read/modify/write Android BCB fields + */ + +#include <android_bootloader_message.h> +#include <command.h> +#include <common.h> +#include <malloc.h> + +enum bcb_cmd { + BCB_CMD_LOAD, + BCB_CMD_FIELD_SET, + BCB_CMD_FIELD_CLEAR, + BCB_CMD_FIELD_TEST, + BCB_CMD_FIELD_DUMP, + BCB_CMD_STORE, +}; + +static int bcb_dev = -1; +static int bcb_part = -1; +static struct bootloader_message bcb = { { 0 } }; + +static int bcb_cmd_get(char *cmd) +{ + if (!strncmp(cmd, "load", sizeof("load"))) + return BCB_CMD_LOAD; + if (!strncmp(cmd, "set", sizeof("set"))) + return BCB_CMD_FIELD_SET; + if (!strncmp(cmd, "clear", sizeof("clear"))) + return BCB_CMD_FIELD_CLEAR; + if (!strncmp(cmd, "test", sizeof("test"))) + return BCB_CMD_FIELD_TEST; + if (!strncmp(cmd, "store", sizeof("store"))) + return BCB_CMD_STORE; + if (!strncmp(cmd, "dump", sizeof("dump"))) + return BCB_CMD_FIELD_DUMP; + else + return -1; +} + +static int do_bcb_load(cmd_tbl_t *cmdtp, int flag, int argc, + char * const argv[]) +{ + struct blk_desc *desc; + disk_partition_t info; + u64 cnt; + char *endp; + int part; + + if (argc != 3) + return CMD_RET_USAGE; + + if (blk_get_device_by_str("mmc", argv[1], &desc) < 0) + goto err_1; + + part = simple_strtoul(argv[2], &endp, 0); + if (*endp == '\0') { + if (part_get_info(desc, part, &info)) + goto err_1; + } else { + part = part_get_info_by_name(desc, argv[2], &info); + if (part < 0) + goto err_1; + } + + cnt = DIV_ROUND_UP(sizeof(struct bootloader_message), info.blksz); + if (cnt > info.size) + goto err_2; + + if (blk_dread(desc, info.start, cnt, &bcb) != cnt) + goto err_1; + + bcb_dev = desc->devnum; + bcb_part = part; + debug("%s: Loaded from mmc %d:%d\n", __func__, bcb_dev, bcb_part); + + return CMD_RET_SUCCESS; +err_1: + printf("Error: Failed to read from mmc %s:%s\n", argv[1], argv[2]); + goto err; +err_2: + printf("Error: mmc %s:%s too small!", argv[1], argv[2]); + goto err; +err: + bcb_dev = -1; + bcb_part = -1; + return CMD_RET_FAILURE; +} + +static int bcb_is_misused(int argc, char *const argv[]) +{ + if (bcb_dev < 0 || bcb_part < 0) { + printf("Error: BCB not loaded!\n"); + return -1; + } + switch (bcb_cmd_get(argv[0])) { + case BCB_CMD_LOAD: + /* Dedicated arg handling */ + break; + case BCB_CMD_FIELD_SET: + if (argc != 3) + goto err; + break; + case BCB_CMD_FIELD_TEST: + if (argc != 4) + goto err; + break; + case BCB_CMD_FIELD_CLEAR: + if (argc != 1 && argc != 2) + goto err; + break; + case BCB_CMD_STORE: + if (argc != 1) + goto err; + break; + case BCB_CMD_FIELD_DUMP: + if (argc != 2) + goto err; + break; + default: + debug("%s: Error: Unexpected BCB command\n", __func__); + return -1; + } + + return 0; +err: + printf("Error: Bad usage of 'bcb %s'\n", argv[0]); + return -1; +} + +static int +bcb_field_get(char *name, char **field, int *size) +{ + if (!strncmp(name, "command", sizeof("command"))) { + *field = bcb.command; + *size = sizeof(bcb.command); + } else if (!strncmp(name, "status", sizeof("status"))) { + *field = bcb.status; + *size = sizeof(bcb.status); + } else if (!strncmp(name, "recovery", sizeof("recovery"))) { + *field = bcb.recovery; + *size = sizeof(bcb.recovery); + } else if (!strncmp(name, "stage", sizeof("stage"))) { + *field = bcb.stage; + *size = sizeof(bcb.stage); + } else if (!strncmp(name, "reserved", sizeof("reserved"))) { + *field = bcb.reserved; + *size = sizeof(bcb.reserved); + } else { + printf("Error: Unknown field '%s'\n", name); + return -1; + } + return 0; +} + +static int do_bcb_set(cmd_tbl_t *cmdtp, int flag, int argc, + char * const argv[]) +{ + int size, len; + char *field, *str, *found, *keep; + + if (bcb_is_misused(argc, argv)) + return CMD_RET_FAILURE; + + if (bcb_field_get(argv[1], &field, &size)) + return CMD_RET_FAILURE; + + len = strlen(argv[2]); + if (len >= size) { + printf("Error: sizeof('%s') = %d >= %d = sizeof(bcb.%s)\n", + argv[2], len, size, argv[1]); + return CMD_RET_FAILURE; + } + str = strdup(argv[2]); + keep = str; + + field[0] = '\0'; + while ((found = strsep(&str, ":"))) { + if (field[0] != '\0') + strcat(field, "\n"); + strcat(field, found); + } + + free(keep); + return CMD_RET_SUCCESS; +} + +static int do_bcb_clear(cmd_tbl_t *cmdtp, int flag, int argc, + char * const argv[]) +{ + int size; + char *field; + + if (bcb_is_misused(argc, argv)) + return CMD_RET_FAILURE; + + if (argc == 1) { + memset(&bcb, 0, sizeof(bcb)); + return CMD_RET_SUCCESS; + } + + if (bcb_field_get(argv[1], &field, &size)) + return CMD_RET_FAILURE; + + memset(field, 0, size); + return CMD_RET_SUCCESS; +} + +static int do_bcb_dump(cmd_tbl_t *cmdtp, int flag, int argc, + char * const argv[]) +{ + int size; + char *field; + + if (bcb_is_misused(argc, argv)) + return CMD_RET_FAILURE; + + if (bcb_field_get(argv[1], &field, &size)) + return CMD_RET_FAILURE; + + print_buffer((ulong)field - (ulong)&bcb, (void *)field, 1, size, 16); + return CMD_RET_SUCCESS; +} + +static int do_bcb_test(cmd_tbl_t *cmdtp, int flag, int argc, + char * const argv[]) +{ + int size; + char *field; + + if (bcb_is_misused(argc, argv)) + return CMD_RET_FAILURE; + + if (bcb_field_get(argv[1], &field, &size)) + return CMD_RET_FAILURE; + + if (!strncmp(argv[2], "=", sizeof("="))) { + if (!strncmp(argv[3], field, size)) + return CMD_RET_SUCCESS; + else + return CMD_RET_FAILURE; + } else if (!strncmp(argv[2], "~", sizeof("~"))) { + if (!strstr(field, argv[3])) + return CMD_RET_FAILURE; + else + return CMD_RET_SUCCESS; + } else { + printf("Error: Unknown operator '%s'\n", argv[2]); + } + return CMD_RET_FAILURE; +} + +static int do_bcb_store(cmd_tbl_t *cmdtp, int flag, int argc, + char * const argv[]) +{ + struct blk_desc *desc; + disk_partition_t info; + u64 cnt; + + if (bcb_is_misused(argc, argv)) + return CMD_RET_FAILURE; + + desc = blk_get_devnum_by_type(IF_TYPE_MMC, bcb_dev); + if (!desc) + goto err; + + if (part_get_info(desc, bcb_part, &info)) + goto err; + + cnt = DIV_ROUND_UP(sizeof(struct bootloader_message), info.blksz); + + if (blk_dwrite(desc, info.start, cnt, &bcb) != cnt) + goto err; + + return CMD_RET_SUCCESS; +err: + printf("Error: Failed to write to mmc %d:%d\n", bcb_dev, bcb_part); + return CMD_RET_FAILURE; +} + +static cmd_tbl_t cmd_bcb_sub[] = { + U_BOOT_CMD_MKENT(load, CONFIG_SYS_MAXARGS, 1, do_bcb_load, "", ""), + U_BOOT_CMD_MKENT(set, CONFIG_SYS_MAXARGS, 1, do_bcb_set, "", ""), + U_BOOT_CMD_MKENT(clear, CONFIG_SYS_MAXARGS, 1, do_bcb_clear, "", ""), + U_BOOT_CMD_MKENT(test, CONFIG_SYS_MAXARGS, 1, do_bcb_test, "", ""), + U_BOOT_CMD_MKENT(dump, CONFIG_SYS_MAXARGS, 1, do_bcb_dump, "", ""), + U_BOOT_CMD_MKENT(store, CONFIG_SYS_MAXARGS, 1, do_bcb_store, "", ""), +}; + +static int do_bcb(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[]) +{ + cmd_tbl_t *c; + + if (argc < 2) + return CMD_RET_USAGE; + + argc--; + argv++; + + c = find_cmd_tbl(argv[0], cmd_bcb_sub, ARRAY_SIZE(cmd_bcb_sub)); + if (c) + return c->cmd(cmdtp, flag, argc, argv); + + return CMD_RET_USAGE; +} + +U_BOOT_CMD( + bcb, CONFIG_SYS_MAXARGS, 1, do_bcb, + "Load/set/clear/test/dump/store Android BCB fields", + "load <dev> <part> - load BCB from mmc <dev>:<part>\n" + "bcb set <field> <val> - set BCB <field> to <val>\n" + "bcb clear [<field>] - clear BCB <field> or all fields\n" + "bcb test <field> <op> <val> - test BCB <field> against <val>\n" + "bcb dump <field> - dump BCB <field>\n" + "bcb store - store BCB back to mmc\n" + "\n" + "Legend:\n" + "<dev> - MMC device index containing the BCB partition\n" + "<part> - MMC partition index or name containing the BCB\n" + "<field> - one of {command,status,recovery,stage,reserved}\n" + "<op> - the binary operator used in 'bcb test':\n" + " '=' returns true if <val> matches the string stored in <field>\n" + " '~' returns true if <val> matches a subset of <field>'s string\n" + "<val> - string/text provided as input to bcb {set,test}\n" + " NOTE: any ':' character in <val> will be replaced by line feed\n" + " during 'bcb set' and used as separator by upper layers\n" +);
'Bootloader Control Block' (BCB) is a well established term/acronym in the Android namespace which refers to a location in a dedicated raw (i.e. FS-unaware) flash (e.g. eMMC) partition, usually called "misc", which is used as media for exchanging messages between Android userspace (particularly recovery [1]) and an Android-capable bootloader. On higher level, this allows implementing a subset of Android Bootloader Requirements [2], amongst which is the Android-specific bootloader flow [3]. Regardless how the latter is implemented in U-Boot ([3] being the most memorable example), reading/writing/dumping the BCB fields in the development process from inside the U-Boot is a convenient feature. Hence, make it available to the users. Some usage examples of the new command recorded on R-Car H3ULCB-KF ('>>>' is an overlay on top of the original console output): => bcb bcb - Load/set/clear/test/dump/store Android BCB fields Usage: bcb load <dev> <part> - load BCB from mmc <dev>:<part> bcb set <field> <val> - set BCB <field> to <val> bcb clear [<field>] - clear BCB <field> or all fields bcb test <field> <op> <val> - test BCB <field> against <val> bcb dump <field> - dump BCB <field> bcb store - store BCB back to mmc Legend: <dev> - MMC device index containing the BCB partition <part> - MMC partition index or name containing the BCB <field> - one of {command,status,recovery,stage,reserved} <op> - the binary operator used in 'bcb test': '=' returns true if <val> matches the string stored in <field> '~' returns true if <val> matches a subset of <field>'s string <val> - string/text provided as input to bcb {set,test} NOTE: any ':' character in <val> will be replaced by line feed during 'bcb set' and used as separator by upper layers => bcb dump command Error: BCB not loaded! >>> Users must specify mmc device and partition before any other call => bcb load 1 misc => bcb load 1 1 >>> The two calls are equivalent (assuming "misc" has index 1) => bcb dump command 00000000: 62 6f 6f 74 6f 6e 63 65 2d 73 68 65 6c 6c 00 72 bootonce-shell.r 00000010: 79 00 72 00 00 00 00 00 00 00 00 00 00 00 00 00 y.r............. >>> The output is in binary/string format for convenience >>> The output size matches the size of inspected BCB field >>> (32 bytes in case of 'command') => bcb test command = bootonce-shell && echo true true => bcb test command = bootonce-shell- && echo true => bcb test command = bootonce-shel && echo true >>> The '=' operator returns 'true' on perfect match => bcb test command ~ bootonce-shel && echo true true => bcb test command ~ bootonce-shell && echo true true >>> The '~' operator returns 'true' on substring match => bcb set command recovery => bcb dump command 00000000: 72 65 63 6f 76 65 72 79 00 73 68 65 6c 6c 00 72 recovery.shell.r 00000010: 79 00 72 00 00 00 00 00 00 00 00 00 00 00 00 00 y.r............. >>> The new value is NULL-terminated and stored in the BCB field => bcb set recovery "msg1:msg2:msg3" => bcb dump recovery 00000040: 6d 73 67 31 0a 6d 73 67 32 0a 6d 73 67 33 00 00 msg1.msg2.msg3.. 00000050: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ 00000060: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ >>> --- snip --- >>> Every ':' is replaced by line-feed '\n' (0xA). The latter is used >>> as separator between individual commands by Android userspace => bcb store >>> Flush/store the BCB structure to MMC [1] https://android.googlesource.com/platform/bootable/recovery [2] https://source.android.com/devices/bootloader [3] https://patchwork.ozlabs.org/patch/746835/ ("[U-Boot,5/6] Initial support for the Android Bootloader flow") Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com> --- v2: - [Heinrich Schuchardt] Implement sub-commands via U_BOOT_CMD_MKENT. - Polished the code. Ensured no warnings returned by sparse, smatch, `cppcheck --force --enable=all --inconclusive`, make W=1. - Tested on R-Car-H3-ES20 ULCB-KF. v1: - https://patchwork.ozlabs.org/patch/1080395/ --- cmd/Kconfig | 17 +++ cmd/Makefile | 1 + cmd/bcb.c | 330 +++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 348 insertions(+) create mode 100644 cmd/bcb.c