| Message ID | 20190523153223.18185-3-erosca@de.adit-jv.com |
|---|---|
| State | Accepted |
| Commit | db7b7a05b2671c63cd49955dee58157045c68f05 |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Add 'bcb' command to read/modify/write Android BCB | expand |
Hi Eugeniu, On Thu, 23 May 2019 at 16:33, 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: Please, load BCB first! > >>> 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> > --- > v3: > - [Simon Glass] Allow 'strsep' to modify the argv[n] string in-place > rather than duplicating it with 'strdup'. Remove #include <malloc.h> > - [Simon Glass] Call bcb_is_misused() _once_ in do_bcb() > - [Simon Glass] Report error codes if IO fails in do_bcb_{load,store} > - [Simon Glass] Leave one empty line above top-level returns > (note: checkpatch is indifferent about this) > - [Simon Glass] Replace strncmp with '==' operator for single-character > strings in do_bcb_test(), which reduces compiled object size > - Reorder the functions to match the cmd_bcb_sub table > - Improve error reporting: > - s/Error: Unknown field/Error: Unknown bcb field/ > - s/debug Error: Unexpected BCB command/ > printf Error: 'bcb %s' not supported/ > - s/Error: BCB not loaded/Error: Please, load BCB first/ > - Make sure static analysis is clean: > - cppcheck --force --enable=all --inconclusive > - sparse/make C=2 > - make W=1 > - smatch > - Re-test on R-Car H3ULCB-KF > - Compile/boot/smoke-test on sandbox > 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. > - https://patchwork.ozlabs.org/patch/1101107/ > > v1: > - https://patchwork.ozlabs.org/patch/1080395/ > --- > cmd/Kconfig | 17 +++ > cmd/Makefile | 1 + > cmd/bcb.c | 340 +++++++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 358 insertions(+) > create mode 100644 cmd/bcb.c I'm going to make some general comments as I still feel that this code is really odd. > > 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..2bd5a744deb5 > --- /dev/null > +++ b/cmd/bcb.c > @@ -0,0 +1,340 @@ > +// 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> > + > +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, > +}; It looks like this ^^ can be removed, see below. > + > +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"))) Is this strncmp() for a security reason? I don't think that is necessary. We typically would avoid using the command line in secure situations. Better I think to check just the first 1-2 chars which is enough to distinguish the command. > + 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; > +} and this function ^^ > + > +static int bcb_is_misused(int argc, char *const argv[]) > +{ > + int cmd = bcb_cmd_get(argv[0]); > + > + switch (cmd) { > + case BCB_CMD_LOAD: > + if (argc != 3) > + goto err; > + break; To me it does not make sense to hold the validation code for 'load' here. It just makes it harder to maintain if we update it, since we need to change the code and and below. > + 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: > + printf("Error: 'bcb %s' not supported\n", argv[0]); > + return -1; > + } > + > + if (cmd != BCB_CMD_LOAD && (bcb_dev < 0 || bcb_part < 0)) { > + printf("Error: Please, load BCB first!\n"); > + 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) How about fieldp and sizep to indicate that these values are returned? > +{ > + 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 bcb field '%s'\n", name); > + return -1; > + } > + > + return 0; > +} > + > +static int Would prefer not to split the line here so we can search for 'int do_', for example. > +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, ret; > + > + ret = blk_get_device_by_str("mmc", argv[1], &desc); > + if (ret < 0) > + goto err_1; > + > + part = simple_strtoul(argv[2], &endp, 0); > + if (*endp == '\0') { > + ret = part_get_info(desc, part, &info); > + if (ret) > + goto err_1; > + } else { > + part = part_get_info_by_name(desc, argv[2], &info); > + if (part < 0) { > + ret = part; > + 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) { > + ret = -EIO; Actually the error code is the return value of blk_dread(). Although perhaps it is badly documented. > + 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: How about err_read_fail > + printf("Error: mmc %s:%s read failed (%d)\n", argv[1], argv[2], ret); > + goto err; > +err_2: err_too_small > + printf("Error: mmc %s:%s too small!", argv[1], argv[2]); > + goto err; > +err: > + bcb_dev = -1; > + bcb_part = -1; Why these two lines? > + > + return CMD_RET_FAILURE; > +} > + > +static int do_bcb_set(cmd_tbl_t *cmdtp, int flag, int argc, > + char * const argv[]) > +{ > + int size, len; > + char *field, *str, *found; > + > + 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 = argv[2]; > + > + field[0] = '\0'; > + while ((found = strsep(&str, ":"))) { > + if (field[0] != '\0') > + strcat(field, "\n"); > + strcat(field, found); > + } > + > + 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 (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_test(cmd_tbl_t *cmdtp, int flag, int argc, > + char * const argv[]) > +{ > + int size; > + char *field; > + char *op = argv[2]; > + > + if (bcb_field_get(argv[1], &field, &size)) > + return CMD_RET_FAILURE; > + > + if (*op == '=' && *(op + 1) == '\0') { > + if (!strncmp(argv[3], field, size)) > + return CMD_RET_SUCCESS; > + else > + return CMD_RET_FAILURE; > + } else if (*op == '~' && *(op + 1) == '\0') { > + if (!strstr(field, argv[3])) > + return CMD_RET_FAILURE; > + else > + return CMD_RET_SUCCESS; > + } else { > + printf("Error: Unknown operator '%s'\n", op); > + } > + > + return CMD_RET_FAILURE; > +} > + > +static int do_bcb_dump(cmd_tbl_t *cmdtp, int flag, int argc, > + char * const argv[]) > +{ > + int size; > + char *field; > + > + 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_store(cmd_tbl_t *cmdtp, int flag, int argc, > + char * const argv[]) > +{ > + struct blk_desc *desc; > + disk_partition_t info; > + u64 cnt; > + int ret; > + > + desc = blk_get_devnum_by_type(IF_TYPE_MMC, bcb_dev); > + if (!desc) { > + ret = -ENODEV; > + goto err; > + } > + > + ret = part_get_info(desc, bcb_part, &info); > + if (ret) > + goto err; > + > + cnt = DIV_ROUND_UP(sizeof(struct bootloader_message), info.blksz); > + > + if (blk_dwrite(desc, info.start, cnt, &bcb) != cnt) { as above > + ret = -EIO; > + goto err; > + } > + > + return CMD_RET_SUCCESS; > +err: > + printf("Error: mmc %d:%d write failed (%d)\n", bcb_dev, bcb_part, ret); > + > + 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 CMD_RET_USAGE; > + > + if (bcb_is_misused(argc, argv)) { > + /* We try to improve the user experience by reporting the /* * We try ... * ... */ > + * root-cause of misusage, so don't return CMD_RET_USAGE, > + * since the latter prints out the full-blown help text Right, but that's the idea...this is how people get the syntax. > + */ > + return CMD_RET_FAILURE; > + } > + > + return c->cmd(cmdtp, flag, argc, argv); > +} > + > +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, On Sat, Jun 22, 2019 at 08:09:48PM +0100, Simon Glass wrote: > On Thu, 23 May 2019 at 16:33, Eugeniu Rosca <erosca@de.adit-jv.com> wrote: [..] > I'm going to make some general comments as I still feel that this code > is really odd. Thanks. My replies below. > > +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, > > +}; > > It looks like this ^^ can be removed, see below. My reply below. > > +static int bcb_cmd_get(char *cmd) > > +{ > > + if (!strncmp(cmd, "load", sizeof("load"))) > > Is this strncmp() for a security reason? I don't think that is > necessary. We typically would avoid using the command line in secure > situations. strncmp() is chosen for the sake of paranoid/defensive programming. Indeed, strncmp() is not really needed when comparing a variable with a string literal. We expect strcmp() to behave safely even if the string variable is not NUL-terminated. In the same scenario, Linux v5.2-rc7 uses both strcmp() and strncmp(), but the frequency of strcmp() is higher: $ git --version git version 2.22.0 $ (Linux 5.2-rc7) git grep -En 'strncmp\([^"]*"[[:alnum:]]+"' | wc -l 1066 $ (Linux 5.2-rc7) git grep -En 'strcmp\([^"]*"[[:alnum:]]+"' | wc -l 1968 A quick "strcmp vs strncmp" object size test shows that strcmp() generates smaller memory footprint (gcc-8, x86_64): $ (U-Boot) size cmd/bcb-strncmp.o cmd/bcb-strcmp.o text data bss dec hex filename 3373 400 2048 5821 16bd cmd/bcb-strncmp.o 3314 400 2048 5762 1682 cmd/bcb-strcmp.o So, overall, I agree to use strcmp() whenever variables are compared with string literals. > > Better I think to check just the first 1-2 chars which is enough to > distinguish the command. I personally think this will make the code less readable and will increase maintenance effort. This will especially be annoying when differentiating sub-commands like set/see, start/status, etc, which have a long common prefix. > > > + 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; > > +} > > and this function ^^ Do you propose zapping both bcb_cmd_get() and bcb_is_misused()? I stress again that bcb_is_misused() aims to centralize error reporting. Without bcb_is_misused(), I would need to create multiple instances of below error messages (since they would need to be reported by several sub-commands), increasing the code size: - printf("Error: Please, load BCB first!\n"); - printf("Error: Bad usage of 'bcb %s'\n", subcommand); > > > + > > +static int bcb_is_misused(int argc, char *const argv[]) > > +{ > > + int cmd = bcb_cmd_get(argv[0]); > > + > > + switch (cmd) { > > + case BCB_CMD_LOAD: > > + if (argc != 3) > > + goto err; > > + break; > > To me it does not make sense to hold the validation code for 'load' > here. It just makes it harder to maintain if we update it, since we > need to change the code and and below. Please, make me understand. Are you unhappy about 'bcb load' only or are you unhappy about the bcb_is_misused()? I provided some arguments above to still keep this function in place. If this function is preserved, I see no reason to treat 'bcb load' differently from other sub-commands. > > +static int bcb_field_get(char *name, char **field, int *size) > > How about fieldp and sizep to indicate that these values are returned? Makes sense. Will be updated. > > + > > +static int > > Would prefer not to split the line here so we can search for 'int > do_', for example. Will update that, although I see 355 occurrences of this pattern in U-Boot and 23980 occurrences in Linux v5.2-rc7: $ (U-Boot/master) git grep -E "^static\s+[[:alnum:]]+$" -- "*.c" "*.h" | wc -l 355 $ (Linux v5.2-rc7) git grep -E "^static\s+[[:alnum:]]+$" -- "*.c" "*.h" | wc -l 23980 This makes me wonder where the border between subjective preferences of the maintainer and objective coding style requirements really is? > > + if (blk_dread(desc, info.start, cnt, &bcb) != cnt) { > > + ret = -EIO; > > Actually the error code is the return value of blk_dread(). Although > perhaps it is badly documented. Based on my reading, blk_dread() returns ulong, not a negative error code. I could find no blk_dread() caller which reuses the return value as error code. I will make no change here. > > +err_1: > > How about err_read_fail > > > +err_2: > > err_too_small Agreed. Will be updated. > > > + printf("Error: mmc %s:%s too small!", argv[1], argv[2]); > > + goto err; > > +err: > > + bcb_dev = -1; > > + bcb_part = -1; > > Why these two lines? They act as an indicator for wrongly loaded or unloaded BCB. > > + if (blk_dwrite(desc, info.start, cnt, &bcb) != cnt) { > > as above My feedback on blk_dread() applies here too. > > + if (bcb_is_misused(argc, argv)) { > > + /* We try to improve the user experience by reporting the > > /* > * We try ... > * ... > */ Will be updated. > > > + * root-cause of misusage, so don't return CMD_RET_USAGE, > > + * since the latter prints out the full-blown help text > > Right, but that's the idea...this is how people get the syntax. I prefer to tell the users what's the reason of their command being rejected rather than throwing a full-blown help message which they didn't ask for.
On Thu, May 23, 2019 at 05:32:22PM +0200, Eugeniu Rosca 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: Please, load BCB first! > >>> 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> Applied to u-boot/master, thanks!
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..2bd5a744deb5 --- /dev/null +++ b/cmd/bcb.c @@ -0,0 +1,340 @@ +// 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> + +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 bcb_is_misused(int argc, char *const argv[]) +{ + int cmd = bcb_cmd_get(argv[0]); + + switch (cmd) { + case BCB_CMD_LOAD: + if (argc != 3) + goto err; + 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: + printf("Error: 'bcb %s' not supported\n", argv[0]); + return -1; + } + + if (cmd != BCB_CMD_LOAD && (bcb_dev < 0 || bcb_part < 0)) { + printf("Error: Please, load BCB first!\n"); + 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 bcb field '%s'\n", name); + return -1; + } + + return 0; +} + +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, ret; + + ret = blk_get_device_by_str("mmc", argv[1], &desc); + if (ret < 0) + goto err_1; + + part = simple_strtoul(argv[2], &endp, 0); + if (*endp == '\0') { + ret = part_get_info(desc, part, &info); + if (ret) + goto err_1; + } else { + part = part_get_info_by_name(desc, argv[2], &info); + if (part < 0) { + ret = part; + 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) { + ret = -EIO; + 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: mmc %s:%s read failed (%d)\n", argv[1], argv[2], ret); + 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 do_bcb_set(cmd_tbl_t *cmdtp, int flag, int argc, + char * const argv[]) +{ + int size, len; + char *field, *str, *found; + + 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 = argv[2]; + + field[0] = '\0'; + while ((found = strsep(&str, ":"))) { + if (field[0] != '\0') + strcat(field, "\n"); + strcat(field, found); + } + + 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 (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_test(cmd_tbl_t *cmdtp, int flag, int argc, + char * const argv[]) +{ + int size; + char *field; + char *op = argv[2]; + + if (bcb_field_get(argv[1], &field, &size)) + return CMD_RET_FAILURE; + + if (*op == '=' && *(op + 1) == '\0') { + if (!strncmp(argv[3], field, size)) + return CMD_RET_SUCCESS; + else + return CMD_RET_FAILURE; + } else if (*op == '~' && *(op + 1) == '\0') { + if (!strstr(field, argv[3])) + return CMD_RET_FAILURE; + else + return CMD_RET_SUCCESS; + } else { + printf("Error: Unknown operator '%s'\n", op); + } + + return CMD_RET_FAILURE; +} + +static int do_bcb_dump(cmd_tbl_t *cmdtp, int flag, int argc, + char * const argv[]) +{ + int size; + char *field; + + 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_store(cmd_tbl_t *cmdtp, int flag, int argc, + char * const argv[]) +{ + struct blk_desc *desc; + disk_partition_t info; + u64 cnt; + int ret; + + desc = blk_get_devnum_by_type(IF_TYPE_MMC, bcb_dev); + if (!desc) { + ret = -ENODEV; + goto err; + } + + ret = part_get_info(desc, bcb_part, &info); + if (ret) + goto err; + + cnt = DIV_ROUND_UP(sizeof(struct bootloader_message), info.blksz); + + if (blk_dwrite(desc, info.start, cnt, &bcb) != cnt) { + ret = -EIO; + goto err; + } + + return CMD_RET_SUCCESS; +err: + printf("Error: mmc %d:%d write failed (%d)\n", bcb_dev, bcb_part, ret); + + 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 CMD_RET_USAGE; + + if (bcb_is_misused(argc, argv)) { + /* We try to improve the user experience by reporting the + * root-cause of misusage, so don't return CMD_RET_USAGE, + * since the latter prints out the full-blown help text + */ + return CMD_RET_FAILURE; + } + + return c->cmd(cmdtp, flag, argc, argv); +} + +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: Please, load BCB first! >>> 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> --- v3: - [Simon Glass] Allow 'strsep' to modify the argv[n] string in-place rather than duplicating it with 'strdup'. Remove #include <malloc.h> - [Simon Glass] Call bcb_is_misused() _once_ in do_bcb() - [Simon Glass] Report error codes if IO fails in do_bcb_{load,store} - [Simon Glass] Leave one empty line above top-level returns (note: checkpatch is indifferent about this) - [Simon Glass] Replace strncmp with '==' operator for single-character strings in do_bcb_test(), which reduces compiled object size - Reorder the functions to match the cmd_bcb_sub table - Improve error reporting: - s/Error: Unknown field/Error: Unknown bcb field/ - s/debug Error: Unexpected BCB command/ printf Error: 'bcb %s' not supported/ - s/Error: BCB not loaded/Error: Please, load BCB first/ - Make sure static analysis is clean: - cppcheck --force --enable=all --inconclusive - sparse/make C=2 - make W=1 - smatch - Re-test on R-Car H3ULCB-KF - Compile/boot/smoke-test on sandbox 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. - https://patchwork.ozlabs.org/patch/1101107/ v1: - https://patchwork.ozlabs.org/patch/1080395/ --- cmd/Kconfig | 17 +++ cmd/Makefile | 1 + cmd/bcb.c | 340 +++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 358 insertions(+) create mode 100644 cmd/bcb.c