[U-Boot] [PATCH v3 2/3] cmd: Add 'bcb' command to read/modify/write BCB fields
Simon Glass
sjg at chromium.org
Sat Jun 22 19:09:48 UTC 2019
Hi Eugeniu,
On Thu, 23 May 2019 at 16:33, Eugeniu Rosca <erosca at 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 at 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 at 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
More information about the U-Boot
mailing list