* [U-Boot] [PATCH v3 0/3] Add 'bcb' command to read/modify/write Android BCB
@ 2019-05-23 15:32 Eugeniu Rosca
2019-05-23 15:32 ` [U-Boot] [PATCH v3 1/3] include: android_bootloader_message.h: Minimize the diff to AOSP Eugeniu Rosca
` (3 more replies)
0 siblings, 4 replies; 18+ messages in thread
From: Eugeniu Rosca @ 2019-05-23 15:32 UTC (permalink / raw)
To: u-boot
The motivation behind the 'bcb' command is explained in the
second ("cmd: Add 'bcb' command *") patch.
The first patch does the necessary fixing and polishing of
include/android_bootloader_message.h and is a hard prerequisite
for this series.
The third patch relocates the Android README to doc/android and creates
a brand new doc/android/bcb.txt covering the purpose/usage of 'bcb'.
v3:
- [Simon Glass] Lots of review comments handled in cmd/bcb.c.
- [Simon Glass, Sam Protsenko] Renamed and enriched android docs.
- Placed detailed stats in each patch.
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/cover/1080393/
Eugeniu Rosca (3):
include: android_bootloader_message.h: Minimize the diff to AOSP
cmd: Add 'bcb' command to read/modify/write BCB fields
doc: relocate/rename Android README and add BCB overview
cmd/Kconfig | 17 +
cmd/Makefile | 1 +
cmd/bcb.c | 340 ++++++++++++++++++
doc/{README.avb2 => android/avb2.txt} | 0
doc/android/bcb.txt | 89 +++++
.../fastboot.txt} | 0
include/android_bootloader_message.h | 126 ++++---
7 files changed, 515 insertions(+), 58 deletions(-)
create mode 100644 cmd/bcb.c
rename doc/{README.avb2 => android/avb2.txt} (100%)
create mode 100644 doc/android/bcb.txt
rename doc/{README.android-fastboot => android/fastboot.txt} (100%)
--
2.21.0
^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH v3 1/3] include: android_bootloader_message.h: Minimize the diff to AOSP
2019-05-23 15:32 [U-Boot] [PATCH v3 0/3] Add 'bcb' command to read/modify/write Android BCB Eugeniu Rosca
@ 2019-05-23 15:32 ` Eugeniu Rosca
2019-06-22 19:09 ` Simon Glass
2019-07-11 22:06 ` Tom Rini
2019-05-23 15:32 ` [U-Boot] [PATCH v3 2/3] cmd: Add 'bcb' command to read/modify/write BCB fields Eugeniu Rosca
` (2 subsequent siblings)
3 siblings, 2 replies; 18+ messages in thread
From: Eugeniu Rosca @ 2019-05-23 15:32 UTC (permalink / raw)
To: u-boot
Perform the following updates:
- Relocate the commit id from the file to the description of U-Boot
commit. The AOSP commit is c784ce50e8c10eaf70e1f97e24e8324aef45faf5.
This is done to avoid stale references in the file itself. The
reasoning is in https://patchwork.ozlabs.org/patch/1098056/#2170209.
- Minimize the diff to AOSP, to decrease the effort of the next AOSP
backports. The background can be found in:
https://patchwork.ozlabs.org/patch/1080394/#2168454.
- Guard the static_assert() calls by #ifndef __UBOOT__ ... #endif,
to avoid compilation failures of files including the header.
Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>
---
v3:
- Added 'Reviewed-by: Sam Protsenko'
v2:
- Newly pushed. No changes.
- https://patchwork.ozlabs.org/patch/1101108/
---
include/android_bootloader_message.h | 126 +++++++++++++++------------
1 file changed, 68 insertions(+), 58 deletions(-)
diff --git a/include/android_bootloader_message.h b/include/android_bootloader_message.h
index b84789f02227..286d7ab0f31e 100644
--- a/include/android_bootloader_message.h
+++ b/include/android_bootloader_message.h
@@ -2,7 +2,7 @@
* This is from the Android Project,
* Repository: https://android.googlesource.com/platform/bootable/recovery
* File: bootloader_message/include/bootloader_message/bootloader_message.h
- * Commit: c784ce50e8c10eaf70e1f97e24e8324aef45faf5
+ * Commit: See U-Boot commit description
*
* Copyright (C) 2008 The Android Open Source Project
*
@@ -12,18 +12,24 @@
#ifndef __ANDROID_BOOTLOADER_MESSAGE_H
#define __ANDROID_BOOTLOADER_MESSAGE_H
+#ifndef __UBOOT__
+#include <assert.h>
+#include <stddef.h>
+#include <stdint.h>
+#else
/* compiler.h defines the types that otherwise are included from stdint.h and
* stddef.h
*/
#include <compiler.h>
+#endif
-/* Spaces used by misc partition are as below:
- * 0 - 2K For bootloader_message
- * 2K - 16K Used by Vendor's bootloader (the 2K - 4K range may be optionally used
- * as bootloader_message_ab struct)
- * 16K - 64K Used by uncrypt and recovery to store wipe_package for A/B devices
- * Note that these offsets are admitted by bootloader,recovery and uncrypt, so they
- * are not configurable without changing all of them. */
+// Spaces used by misc partition are as below:
+// 0 - 2K For bootloader_message
+// 2K - 16K Used by Vendor's bootloader (the 2K - 4K range may be optionally used
+// as bootloader_message_ab struct)
+// 16K - 64K Used by uncrypt and recovery to store wipe_package for A/B devices
+// Note that these offsets are admitted by bootloader,recovery and uncrypt, so they
+// are not configurable without changing all of them.
static const size_t BOOTLOADER_MESSAGE_OFFSET_IN_MISC = 0;
static const size_t WIPE_PACKAGE_OFFSET_IN_MISC = 16 * 1024;
@@ -61,17 +67,17 @@ struct bootloader_message {
char status[32];
char recovery[768];
- /* The 'recovery' field used to be 1024 bytes. It has only ever
- * been used to store the recovery command line, so 768 bytes
- * should be plenty. We carve off the last 256 bytes to store the
- * stage string (for multistage packages) and possible future
- * expansion. */
+ // The 'recovery' field used to be 1024 bytes. It has only ever
+ // been used to store the recovery command line, so 768 bytes
+ // should be plenty. We carve off the last 256 bytes to store the
+ // stage string (for multistage packages) and possible future
+ // expansion.
char stage[32];
- /* The 'reserved' field used to be 224 bytes when it was initially
- * carved off from the 1024-byte recovery field. Bump it up to
- * 1184-byte so that the entire bootloader_message struct rounds up
- * to 2048-byte. */
+ // The 'reserved' field used to be 224 bytes when it was initially
+ // carved off from the 1024-byte recovery field. Bump it up to
+ // 1184-byte so that the entire bootloader_message struct rounds up
+ // to 2048-byte.
char reserved[1184];
};
@@ -79,10 +85,12 @@ struct bootloader_message {
* We must be cautious when changing the bootloader_message struct size,
* because A/B-specific fields may end up with different offsets.
*/
+#ifndef __UBOOT__
#if (__STDC_VERSION__ >= 201112L) || defined(__cplusplus)
static_assert(sizeof(struct bootloader_message) == 2048,
"struct bootloader_message size changes, which may break A/B devices");
#endif
+#endif /* __UBOOT__ */
/**
* The A/B-specific bootloader message structure (4-KiB).
@@ -108,7 +116,7 @@ struct bootloader_message_ab {
char slot_suffix[32];
char update_channel[128];
- /* Round up the entire struct to 4096-byte. */
+ // Round up the entire struct to 4096-byte.
char reserved[1888];
};
@@ -116,26 +124,28 @@ struct bootloader_message_ab {
* Be cautious about the struct size change, in case we put anything post
* bootloader_message_ab struct (b/29159185).
*/
+#ifndef __UBOOT__
#if (__STDC_VERSION__ >= 201112L) || defined(__cplusplus)
static_assert(sizeof(struct bootloader_message_ab) == 4096,
"struct bootloader_message_ab size changes");
#endif
+#endif /* __UBOOT__ */
#define BOOT_CTRL_MAGIC 0x42414342 /* Bootloader Control AB */
#define BOOT_CTRL_VERSION 1
struct slot_metadata {
- /* Slot priority with 15 meaning highest priority, 1 lowest
- * priority and 0 the slot is unbootable. */
+ // Slot priority with 15 meaning highest priority, 1 lowest
+ // priority and 0 the slot is unbootable.
uint8_t priority : 4;
- /* Number of times left attempting to boot this slot. */
+ // Number of times left attempting to boot this slot.
uint8_t tries_remaining : 3;
- /* 1 if this slot has booted successfully, 0 otherwise. */
+ // 1 if this slot has booted successfully, 0 otherwise.
uint8_t successful_boot : 1;
- /* 1 if this slot is corrupted from a dm-verity corruption, 0
- * otherwise. */
+ // 1 if this slot is corrupted from a dm-verity corruption, 0
+ // otherwise.
uint8_t verity_corrupted : 1;
- /* Reserved for further use. */
+ // Reserved for further use.
uint8_t reserved : 7;
} __attribute__((packed));
@@ -148,99 +158,99 @@ struct slot_metadata {
* mandatory.
*/
struct bootloader_control {
- /* NUL terminated active slot suffix. */
+ // NUL terminated active slot suffix.
char slot_suffix[4];
- /* Bootloader Control AB magic number (see BOOT_CTRL_MAGIC). */
+ // Bootloader Control AB magic number (see BOOT_CTRL_MAGIC).
uint32_t magic;
- /* Version of struct being used (see BOOT_CTRL_VERSION). */
+ // Version of struct being used (see BOOT_CTRL_VERSION).
uint8_t version;
- /* Number of slots being managed. */
+ // Number of slots being managed.
uint8_t nb_slot : 3;
- /* Number of times left attempting to boot recovery. */
+ // Number of times left attempting to boot recovery.
uint8_t recovery_tries_remaining : 3;
- /* Ensure 4-bytes alignment for slot_info field. */
+ // Ensure 4-bytes alignment for slot_info field.
uint8_t reserved0[2];
- /* Per-slot information. Up to 4 slots. */
+ // Per-slot information. Up to 4 slots.
struct slot_metadata slot_info[4];
- /* Reserved for further use. */
+ // Reserved for further use.
uint8_t reserved1[8];
- /* CRC32 of all 28 bytes preceding this field (little endian
- * format). */
+ // CRC32 of all 28 bytes preceding this field (little endian
+ // format).
uint32_t crc32_le;
} __attribute__((packed));
+#ifndef __UBOOT__
#if (__STDC_VERSION__ >= 201112L) || defined(__cplusplus)
static_assert(sizeof(struct bootloader_control) ==
sizeof(((struct bootloader_message_ab *)0)->slot_suffix),
"struct bootloader_control has wrong size");
#endif
+#endif /* __UBOOT__ */
#ifndef __UBOOT__
-
#ifdef __cplusplus
#include <string>
#include <vector>
-/* Return the block device name for the bootloader message partition and waits
- * for the device for up to 10 seconds. In case of error returns the empty
- * string. */
+// Return the block device name for the bootloader message partition and waits
+// for the device for up to 10 seconds. In case of error returns the empty
+// string.
std::string get_bootloader_message_blk_device(std::string* err);
-/* Read bootloader message into boot. Error message will be set in err. */
+// Read bootloader message into boot. Error message will be set in err.
bool read_bootloader_message(bootloader_message* boot, std::string* err);
-/* Read bootloader message from the specified misc device into boot. */
+// Read bootloader message from the specified misc device into boot.
bool read_bootloader_message_from(bootloader_message* boot, const std::string& misc_blk_device,
std::string* err);
-/* Write bootloader message to BCB. */
+// Write bootloader message to BCB.
bool write_bootloader_message(const bootloader_message& boot, std::string* err);
-/* Write bootloader message to the specified BCB device. */
+// Write bootloader message to the specified BCB device.
bool write_bootloader_message_to(const bootloader_message& boot,
const std::string& misc_blk_device, std::string* err);
-/* Write bootloader message (boots into recovery with the options) to BCB. Will
- * set the command and recovery fields, and reset the rest. */
+// Write bootloader message (boots into recovery with the options) to BCB. Will
+// set the command and recovery fields, and reset the rest.
bool write_bootloader_message(const std::vector<std::string>& options, std::string* err);
-/* Write bootloader message (boots into recovery with the options) to the specific BCB device. Will
- * set the command and recovery fields, and reset the rest. */
+// Write bootloader message (boots into recovery with the options) to the specific BCB device. Will
+// set the command and recovery fields, and reset the rest.
bool write_bootloader_message_to(const std::vector<std::string>& options,
const std::string& misc_blk_device, std::string* err);
-/* Update bootloader message (boots into recovery with the options) to BCB. Will
- * only update the command and recovery fields. */
+// Update bootloader message (boots into recovery with the options) to BCB. Will
+// only update the command and recovery fields.
bool update_bootloader_message(const std::vector<std::string>& options, std::string* err);
-/* Update bootloader message (boots into recovery with the |options|) in |boot|. Will only update
- * the command and recovery fields. */
+// Update bootloader message (boots into recovery with the |options|) in |boot|. Will only update
+// the command and recovery fields.
bool update_bootloader_message_in_struct(bootloader_message* boot,
const std::vector<std::string>& options);
-/* Clear BCB. */
+// Clear BCB.
bool clear_bootloader_message(std::string* err);
-/* Writes the reboot-bootloader reboot reason to the bootloader_message. */
+// Writes the reboot-bootloader reboot reason to the bootloader_message.
bool write_reboot_bootloader(std::string* err);
-/* Read the wipe package from BCB (from offset WIPE_PACKAGE_OFFSET_IN_MISC). */
+// Read the wipe package from BCB (from offset WIPE_PACKAGE_OFFSET_IN_MISC).
bool read_wipe_package(std::string* package_data, size_t size, std::string* err);
-/* Write the wipe package into BCB (to offset WIPE_PACKAGE_OFFSET_IN_MISC). */
+// Write the wipe package into BCB (to offset WIPE_PACKAGE_OFFSET_IN_MISC).
bool write_wipe_package(const std::string& package_data, std::string* err);
#else
#include <stdbool.h>
-/* C Interface. */
+// C Interface.
bool write_bootloader_message(const char* options);
bool write_reboot_bootloader(void);
-#endif /* ifdef __cplusplus */
-
+#endif // ifdef __cplusplus
#endif /* __UBOOT__ */
#endif /* __ANDROID_BOOTLOADER_MESSAGE_H */
--
2.21.0
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH v3 2/3] cmd: Add 'bcb' command to read/modify/write BCB fields
2019-05-23 15:32 [U-Boot] [PATCH v3 0/3] Add 'bcb' command to read/modify/write Android BCB Eugeniu Rosca
2019-05-23 15:32 ` [U-Boot] [PATCH v3 1/3] include: android_bootloader_message.h: Minimize the diff to AOSP Eugeniu Rosca
@ 2019-05-23 15:32 ` Eugeniu Rosca
2019-06-22 19:09 ` Simon Glass
2019-07-11 22:06 ` Tom Rini
2019-05-23 15:32 ` [U-Boot] [PATCH v3 3/3] doc: relocate/rename Android README and add BCB overview Eugeniu Rosca
2019-05-23 22:58 ` [U-Boot] [PATCH v3 0/3] Add 'bcb' command to read/modify/write Android BCB Sam Protsenko
3 siblings, 2 replies; 18+ messages in thread
From: Eugeniu Rosca @ 2019-05-23 15:32 UTC (permalink / raw)
To: u-boot
'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
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"
+);
--
2.21.0
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH v3 3/3] doc: relocate/rename Android README and add BCB overview
2019-05-23 15:32 [U-Boot] [PATCH v3 0/3] Add 'bcb' command to read/modify/write Android BCB Eugeniu Rosca
2019-05-23 15:32 ` [U-Boot] [PATCH v3 1/3] include: android_bootloader_message.h: Minimize the diff to AOSP Eugeniu Rosca
2019-05-23 15:32 ` [U-Boot] [PATCH v3 2/3] cmd: Add 'bcb' command to read/modify/write BCB fields Eugeniu Rosca
@ 2019-05-23 15:32 ` Eugeniu Rosca
2019-06-22 19:09 ` Simon Glass
` (2 more replies)
2019-05-23 22:58 ` [U-Boot] [PATCH v3 0/3] Add 'bcb' command to read/modify/write Android BCB Sam Protsenko
3 siblings, 3 replies; 18+ messages in thread
From: Eugeniu Rosca @ 2019-05-23 15:32 UTC (permalink / raw)
To: u-boot
Rename:
- doc/{README.avb2 => android/avb2.txt}
- doc/{README.android-fastboot => android/fastboot.txt}
Add a new file documenting the 'bcb' command:
- doc/android/bcb.txt
The new directory structure has been reviewed by Simon in
https://patchwork.ozlabs.org/patch/1101107/#2176031 .
Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---
v3:
- Newly pushed.
---
doc/{README.avb2 => android/avb2.txt} | 0
doc/android/bcb.txt | 89 +++++++++++++++++++
.../fastboot.txt} | 0
3 files changed, 89 insertions(+)
rename doc/{README.avb2 => android/avb2.txt} (100%)
create mode 100644 doc/android/bcb.txt
rename doc/{README.android-fastboot => android/fastboot.txt} (100%)
diff --git a/doc/README.avb2 b/doc/android/avb2.txt
similarity index 100%
rename from doc/README.avb2
rename to doc/android/avb2.txt
diff --git a/doc/android/bcb.txt b/doc/android/bcb.txt
new file mode 100644
index 000000000000..7b7177cacf21
--- /dev/null
+++ b/doc/android/bcb.txt
@@ -0,0 +1,89 @@
+Android Bootloader Control Block (BCB)
+
+The purpose behind this file is to:
+ - give an overview of BCB w/o duplicating public documentation
+ - describe the main BCB use-cases which concern U-Boot
+ - reflect current support status in U-Boot
+ - mention any relevant U-Boot build-time tunables
+ - precisely exemplify one or more use-cases
+
+Additions and fixes are welcome!
+
+
+1. OVERVIEW
+---------------------------------
+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, BCB provides a way to implement a subset of Android
+Bootloader Requirements [2], amongst which are:
+ - Android-specific bootloader flow [3]
+ - Get the "reboot reason" (and act accordingly) [4]
+ - Get/pass a list of commands from/to recovery [1]
+ - TODO
+
+
+2. 'BCB'. SHELL COMMAND OVERVIEW
+-----------------------------------
+The 'bcb' command provides a CLI to facilitate the development of the
+requirements enumerated above. Below is the command's help message:
+
+=> 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
+
+
+3. 'BCB'. EXAMPLE OF GETTING REBOOT REASON
+-----------------------------------
+if bcb load 1 misc; then
+ # valid BCB found
+ if bcb test command = bootonce-bootloader; then
+ bcb clear command; bcb store;
+ # do the equivalent of AOSP ${fastbootcmd}
+ # i.e. call fastboot
+ else if bcb test command = boot-recovery; then
+ bcb clear command; bcb store;
+ # do the equivalent of AOSP ${recoverycmd}
+ # i.e. do anything required for booting into recovery
+ else
+ # boot Android OS normally
+ fi
+else
+ # corrupted/non-existent BCB
+ # report error or boot non-Android OS (platform-specific)
+fi
+
+
+4. ENABLE ON YOUR BOARD
+-----------------------------------
+The following Kconfig options must be enabled:
+CONFIG_PARTITIONS=y
+CONFIG_MMC=y
+CONFIG_BCB=y
+
+[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")
+[4] https://source.android.com/devices/bootloader/boot-reason
diff --git a/doc/README.android-fastboot b/doc/android/fastboot.txt
similarity index 100%
rename from doc/README.android-fastboot
rename to doc/android/fastboot.txt
--
2.21.0
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH v3 0/3] Add 'bcb' command to read/modify/write Android BCB
2019-05-23 15:32 [U-Boot] [PATCH v3 0/3] Add 'bcb' command to read/modify/write Android BCB Eugeniu Rosca
` (2 preceding siblings ...)
2019-05-23 15:32 ` [U-Boot] [PATCH v3 3/3] doc: relocate/rename Android README and add BCB overview Eugeniu Rosca
@ 2019-05-23 22:58 ` Sam Protsenko
2019-05-27 15:26 ` Eugeniu Rosca
3 siblings, 1 reply; 18+ messages in thread
From: Sam Protsenko @ 2019-05-23 22:58 UTC (permalink / raw)
To: u-boot
For the whole series:
Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>
Thank you for handling this, Eugeniu!
On Thu, May 23, 2019 at 6:33 PM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
>
> The motivation behind the 'bcb' command is explained in the
> second ("cmd: Add 'bcb' command *") patch.
>
> The first patch does the necessary fixing and polishing of
> include/android_bootloader_message.h and is a hard prerequisite
> for this series.
>
> The third patch relocates the Android README to doc/android and creates
> a brand new doc/android/bcb.txt covering the purpose/usage of 'bcb'.
>
> v3:
> - [Simon Glass] Lots of review comments handled in cmd/bcb.c.
> - [Simon Glass, Sam Protsenko] Renamed and enriched android docs.
> - Placed detailed stats in each patch.
>
> 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/cover/1080393/
>
> Eugeniu Rosca (3):
> include: android_bootloader_message.h: Minimize the diff to AOSP
> cmd: Add 'bcb' command to read/modify/write BCB fields
> doc: relocate/rename Android README and add BCB overview
>
> cmd/Kconfig | 17 +
> cmd/Makefile | 1 +
> cmd/bcb.c | 340 ++++++++++++++++++
> doc/{README.avb2 => android/avb2.txt} | 0
> doc/android/bcb.txt | 89 +++++
> .../fastboot.txt} | 0
> include/android_bootloader_message.h | 126 ++++---
> 7 files changed, 515 insertions(+), 58 deletions(-)
> create mode 100644 cmd/bcb.c
> rename doc/{README.avb2 => android/avb2.txt} (100%)
> create mode 100644 doc/android/bcb.txt
> rename doc/{README.android-fastboot => android/fastboot.txt} (100%)
>
> --
> 2.21.0
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH v3 0/3] Add 'bcb' command to read/modify/write Android BCB
2019-05-23 22:58 ` [U-Boot] [PATCH v3 0/3] Add 'bcb' command to read/modify/write Android BCB Sam Protsenko
@ 2019-05-27 15:26 ` Eugeniu Rosca
2019-06-04 12:36 ` Sam Protsenko
0 siblings, 1 reply; 18+ messages in thread
From: Eugeniu Rosca @ 2019-05-27 15:26 UTC (permalink / raw)
To: u-boot
Hi Sam,
On Fri, May 24, 2019 at 01:58:18AM +0300, Sam Protsenko wrote:
> For the whole series:
>
> Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>
>
> Thank you for handling this, Eugeniu!
Thanks for your support. Assuming there will be little to no changes in
the 'bcb' command (which still awaits Simon's Ack), I think you are
unblocked with the A/B patches. Looking forward to seeing them.
--
Best Regards,
Eugeniu.
^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH v3 0/3] Add 'bcb' command to read/modify/write Android BCB
2019-05-27 15:26 ` Eugeniu Rosca
@ 2019-06-04 12:36 ` Sam Protsenko
2019-06-04 13:25 ` Tom Rini
0 siblings, 1 reply; 18+ messages in thread
From: Sam Protsenko @ 2019-06-04 12:36 UTC (permalink / raw)
To: u-boot
Hi Tom, Simon,
This one has been hanging here for a while. If there is no concerns,
can we please merge it? It's really wanted feature for Android boot...
Thanks!
On Mon, May 27, 2019 at 6:26 PM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
>
> Hi Sam,
>
> On Fri, May 24, 2019 at 01:58:18AM +0300, Sam Protsenko wrote:
> > For the whole series:
> >
> > Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>
> >
> > Thank you for handling this, Eugeniu!
>
> Thanks for your support. Assuming there will be little to no changes in
> the 'bcb' command (which still awaits Simon's Ack), I think you are
> unblocked with the A/B patches. Looking forward to seeing them.
>
> --
> Best Regards,
> Eugeniu.
^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH v3 0/3] Add 'bcb' command to read/modify/write Android BCB
2019-06-04 12:36 ` Sam Protsenko
@ 2019-06-04 13:25 ` Tom Rini
0 siblings, 0 replies; 18+ messages in thread
From: Tom Rini @ 2019-06-04 13:25 UTC (permalink / raw)
To: u-boot
On Tue, Jun 04, 2019 at 03:36:21PM +0300, Sam Protsenko wrote:
> Hi Tom, Simon,
>
> This one has been hanging here for a while. If there is no concerns,
> can we please merge it? It's really wanted feature for Android boot...
I'd appreciate it if you can confirm the patchwork state of the various
Android-related patches. At this point in the schedule, I'm inclined to
grab everything else for post-v2019.07, thanks!
>
> Thanks!
>
> On Mon, May 27, 2019 at 6:26 PM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
> >
> > Hi Sam,
> >
> > On Fri, May 24, 2019 at 01:58:18AM +0300, Sam Protsenko wrote:
> > > For the whole series:
> > >
> > > Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>
> > >
> > > Thank you for handling this, Eugeniu!
> >
> > Thanks for your support. Assuming there will be little to no changes in
> > the 'bcb' command (which still awaits Simon's Ack), I think you are
> > unblocked with the A/B patches. Looking forward to seeing them.
> >
> > --
> > Best Regards,
> > Eugeniu.
--
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190604/45aa615b/attachment.sig>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH v3 1/3] include: android_bootloader_message.h: Minimize the diff to AOSP
2019-05-23 15:32 ` [U-Boot] [PATCH v3 1/3] include: android_bootloader_message.h: Minimize the diff to AOSP Eugeniu Rosca
@ 2019-06-22 19:09 ` Simon Glass
2019-07-11 22:06 ` Tom Rini
1 sibling, 0 replies; 18+ messages in thread
From: Simon Glass @ 2019-06-22 19:09 UTC (permalink / raw)
To: u-boot
On Thu, 23 May 2019 at 16:33, Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
>
> Perform the following updates:
> - Relocate the commit id from the file to the description of U-Boot
> commit. The AOSP commit is c784ce50e8c10eaf70e1f97e24e8324aef45faf5.
> This is done to avoid stale references in the file itself. The
> reasoning is in https://patchwork.ozlabs.org/patch/1098056/#2170209.
> - Minimize the diff to AOSP, to decrease the effort of the next AOSP
> backports. The background can be found in:
> https://patchwork.ozlabs.org/patch/1080394/#2168454.
> - Guard the static_assert() calls by #ifndef __UBOOT__ ... #endif,
> to avoid compilation failures of files including the header.
>
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
> v3:
> - Added 'Reviewed-by: Sam Protsenko'
> v2:
> - Newly pushed. No changes.
> - https://patchwork.ozlabs.org/patch/1101108/
> ---
> include/android_bootloader_message.h | 126 +++++++++++++++------------
> 1 file changed, 68 insertions(+), 58 deletions(-)
>
Reviewed-by: Simon Glass <sjg@chromium.org>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH v3 2/3] cmd: Add 'bcb' command to read/modify/write BCB fields
2019-05-23 15:32 ` [U-Boot] [PATCH v3 2/3] cmd: Add 'bcb' command to read/modify/write BCB fields Eugeniu Rosca
@ 2019-06-22 19:09 ` Simon Glass
2019-07-06 16:18 ` Eugeniu Rosca
2019-07-11 22:06 ` Tom Rini
1 sibling, 1 reply; 18+ messages in thread
From: Simon Glass @ 2019-06-22 19:09 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH v3 3/3] doc: relocate/rename Android README and add BCB overview
2019-05-23 15:32 ` [U-Boot] [PATCH v3 3/3] doc: relocate/rename Android README and add BCB overview Eugeniu Rosca
@ 2019-06-22 19:09 ` Simon Glass
2019-07-02 21:25 ` Eugeniu Rosca
2019-07-02 18:11 ` Sam Protsenko
2019-07-11 22:06 ` Tom Rini
2 siblings, 1 reply; 18+ messages in thread
From: Simon Glass @ 2019-06-22 19:09 UTC (permalink / raw)
To: u-boot
Hi Eugeniu,
On Thu, 23 May 2019 at 16:33, Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
>
> Rename:
> - doc/{README.avb2 => android/avb2.txt}
This should be in a separate patch since it isn't related to adding
the new docs, actually.
> - doc/{README.android-fastboot => android/fastboot.txt}
>
> Add a new file documenting the 'bcb' command:
> - doc/android/bcb.txt
>
> The new directory structure has been reviewed by Simon in
> https://patchwork.ozlabs.org/patch/1101107/#2176031 .
>
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> ---
> v3:
> - Newly pushed.
> ---
> doc/{README.avb2 => android/avb2.txt} | 0
> doc/android/bcb.txt | 89 +++++++++++++++++++
> .../fastboot.txt} | 0
> 3 files changed, 89 insertions(+)
> rename doc/{README.avb2 => android/avb2.txt} (100%)
> create mode 100644 doc/android/bcb.txt
> rename doc/{README.android-fastboot => android/fastboot.txt} (100%)
Looks good.
Reviewed-by: Simon Glass <sjg@chromium.org>
Regards,
Simon
^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH v3 3/3] doc: relocate/rename Android README and add BCB overview
2019-05-23 15:32 ` [U-Boot] [PATCH v3 3/3] doc: relocate/rename Android README and add BCB overview Eugeniu Rosca
2019-06-22 19:09 ` Simon Glass
@ 2019-07-02 18:11 ` Sam Protsenko
2019-07-02 21:22 ` Eugeniu Rosca
2019-07-11 22:06 ` Tom Rini
2 siblings, 1 reply; 18+ messages in thread
From: Sam Protsenko @ 2019-07-02 18:11 UTC (permalink / raw)
To: u-boot
Hi Eugeniu,
On Thu, May 23, 2019 at 6:33 PM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
>
> Rename:
> - doc/{README.avb2 => android/avb2.txt}
> - doc/{README.android-fastboot => android/fastboot.txt}
>
> Add a new file documenting the 'bcb' command:
> - doc/android/bcb.txt
>
> The new directory structure has been reviewed by Simon in
> https://patchwork.ozlabs.org/patch/1101107/#2176031 .
>
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> ---
> v3:
> - Newly pushed.
> ---
> doc/{README.avb2 => android/avb2.txt} | 0
> doc/android/bcb.txt | 89 +++++++++++++++++++
> .../fastboot.txt} | 0
> 3 files changed, 89 insertions(+)
> rename doc/{README.avb2 => android/avb2.txt} (100%)
> create mode 100644 doc/android/bcb.txt
> rename doc/{README.android-fastboot => android/fastboot.txt} (100%)
>
Sorry for the late review, but it seems like some links to renamed doc
weren't changed properly (easily found by grep). Can you please add
change [1] to this patch, so that we don't need to send it separately
later?
[1] https://pastebin.ubuntu.com/p/nSd5z8JFtr/
Thanks!
> diff --git a/doc/README.avb2 b/doc/android/avb2.txt
> similarity index 100%
> rename from doc/README.avb2
> rename to doc/android/avb2.txt
> diff --git a/doc/android/bcb.txt b/doc/android/bcb.txt
> new file mode 100644
> index 000000000000..7b7177cacf21
> --- /dev/null
> +++ b/doc/android/bcb.txt
> @@ -0,0 +1,89 @@
> +Android Bootloader Control Block (BCB)
> +
> +The purpose behind this file is to:
> + - give an overview of BCB w/o duplicating public documentation
> + - describe the main BCB use-cases which concern U-Boot
> + - reflect current support status in U-Boot
> + - mention any relevant U-Boot build-time tunables
> + - precisely exemplify one or more use-cases
> +
> +Additions and fixes are welcome!
> +
> +
> +1. OVERVIEW
> +---------------------------------
> +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, BCB provides a way to implement a subset of Android
> +Bootloader Requirements [2], amongst which are:
> + - Android-specific bootloader flow [3]
> + - Get the "reboot reason" (and act accordingly) [4]
> + - Get/pass a list of commands from/to recovery [1]
> + - TODO
> +
> +
> +2. 'BCB'. SHELL COMMAND OVERVIEW
> +-----------------------------------
> +The 'bcb' command provides a CLI to facilitate the development of the
> +requirements enumerated above. Below is the command's help message:
> +
> +=> 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
> +
> +
> +3. 'BCB'. EXAMPLE OF GETTING REBOOT REASON
> +-----------------------------------
> +if bcb load 1 misc; then
> + # valid BCB found
> + if bcb test command = bootonce-bootloader; then
> + bcb clear command; bcb store;
> + # do the equivalent of AOSP ${fastbootcmd}
> + # i.e. call fastboot
> + else if bcb test command = boot-recovery; then
> + bcb clear command; bcb store;
> + # do the equivalent of AOSP ${recoverycmd}
> + # i.e. do anything required for booting into recovery
> + else
> + # boot Android OS normally
> + fi
> +else
> + # corrupted/non-existent BCB
> + # report error or boot non-Android OS (platform-specific)
> +fi
> +
> +
> +4. ENABLE ON YOUR BOARD
> +-----------------------------------
> +The following Kconfig options must be enabled:
> +CONFIG_PARTITIONS=y
> +CONFIG_MMC=y
> +CONFIG_BCB=y
> +
> +[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")
> +[4] https://source.android.com/devices/bootloader/boot-reason
> diff --git a/doc/README.android-fastboot b/doc/android/fastboot.txt
> similarity index 100%
> rename from doc/README.android-fastboot
> rename to doc/android/fastboot.txt
> --
> 2.21.0
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH v3 3/3] doc: relocate/rename Android README and add BCB overview
2019-07-02 18:11 ` Sam Protsenko
@ 2019-07-02 21:22 ` Eugeniu Rosca
0 siblings, 0 replies; 18+ messages in thread
From: Eugeniu Rosca @ 2019-07-02 21:22 UTC (permalink / raw)
To: u-boot
Hi Sam,
On Tue, Jul 02, 2019 at 09:11:45PM +0300, Sam Protsenko wrote:
> Hi Eugeniu,
[..]
> Sorry for the late review, but it seems like some links to renamed doc
> weren't changed properly (easily found by grep). Can you please add
> change [1] to this patch, so that we don't need to send it separately
> later?
>
> [1] https://pastebin.ubuntu.com/p/nSd5z8JFtr/
Sure. I will update the series asap. Thanks for commenting!
--
Best regards,
Eugeniu.
^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH v3 3/3] doc: relocate/rename Android README and add BCB overview
2019-06-22 19:09 ` Simon Glass
@ 2019-07-02 21:25 ` Eugeniu Rosca
0 siblings, 0 replies; 18+ messages in thread
From: Eugeniu Rosca @ 2019-07-02 21:25 UTC (permalink / raw)
To: u-boot
Hi Simon,
On Sat, Jun 22, 2019 at 08:09:51PM +0100, Simon Glass wrote:
> On Thu, 23 May 2019 at 16:33, Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
> >
> > Rename:
> > - doc/{README.avb2 => android/avb2.txt}
>
> This should be in a separate patch since it isn't related to adding
> the new docs, actually.
I will incorporate the finding in the upcoming revision.
Thanks for the review comments!
[..]
> Looks good.
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
--
Best regards,
Eugeniu.
^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH v3 2/3] cmd: Add 'bcb' command to read/modify/write BCB fields
2019-06-22 19:09 ` Simon Glass
@ 2019-07-06 16:18 ` Eugeniu Rosca
0 siblings, 0 replies; 18+ messages in thread
From: Eugeniu Rosca @ 2019-07-06 16:18 UTC (permalink / raw)
To: u-boot
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.
--
Best Regards,
Eugeniu.
^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH v3 1/3] include: android_bootloader_message.h: Minimize the diff to AOSP
2019-05-23 15:32 ` [U-Boot] [PATCH v3 1/3] include: android_bootloader_message.h: Minimize the diff to AOSP Eugeniu Rosca
2019-06-22 19:09 ` Simon Glass
@ 2019-07-11 22:06 ` Tom Rini
1 sibling, 0 replies; 18+ messages in thread
From: Tom Rini @ 2019-07-11 22:06 UTC (permalink / raw)
To: u-boot
On Thu, May 23, 2019 at 05:32:21PM +0200, Eugeniu Rosca wrote:
> Perform the following updates:
> - Relocate the commit id from the file to the description of U-Boot
> commit. The AOSP commit is c784ce50e8c10eaf70e1f97e24e8324aef45faf5.
> This is done to avoid stale references in the file itself. The
> reasoning is in https://patchwork.ozlabs.org/patch/1098056/#2170209.
> - Minimize the diff to AOSP, to decrease the effort of the next AOSP
> backports. The background can be found in:
> https://patchwork.ozlabs.org/patch/1080394/#2168454.
> - Guard the static_assert() calls by #ifndef __UBOOT__ ... #endif,
> to avoid compilation failures of files including the header.
>
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>
> Reviewed-by: Simon Glass <sjg@chromium.org>
Applied to u-boot/master, thanks!
--
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190711/db56620b/attachment.sig>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH v3 2/3] cmd: Add 'bcb' command to read/modify/write BCB fields
2019-05-23 15:32 ` [U-Boot] [PATCH v3 2/3] cmd: Add 'bcb' command to read/modify/write BCB fields Eugeniu Rosca
2019-06-22 19:09 ` Simon Glass
@ 2019-07-11 22:06 ` Tom Rini
1 sibling, 0 replies; 18+ messages in thread
From: Tom Rini @ 2019-07-11 22:06 UTC (permalink / raw)
To: u-boot
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!
--
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190711/f74ee731/attachment.sig>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH v3 3/3] doc: relocate/rename Android README and add BCB overview
2019-05-23 15:32 ` [U-Boot] [PATCH v3 3/3] doc: relocate/rename Android README and add BCB overview Eugeniu Rosca
2019-06-22 19:09 ` Simon Glass
2019-07-02 18:11 ` Sam Protsenko
@ 2019-07-11 22:06 ` Tom Rini
2 siblings, 0 replies; 18+ messages in thread
From: Tom Rini @ 2019-07-11 22:06 UTC (permalink / raw)
To: u-boot
On Thu, May 23, 2019 at 05:32:23PM +0200, Eugeniu Rosca wrote:
> Rename:
> - doc/{README.avb2 => android/avb2.txt}
> - doc/{README.android-fastboot => android/fastboot.txt}
>
> Add a new file documenting the 'bcb' command:
> - doc/android/bcb.txt
>
> The new directory structure has been reviewed by Simon in
> https://patchwork.ozlabs.org/patch/1101107/#2176031 .
>
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>
Applied to u-boot/master, thanks!
--
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190711/9792a6b0/attachment.sig>
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2019-07-11 22:06 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-23 15:32 [U-Boot] [PATCH v3 0/3] Add 'bcb' command to read/modify/write Android BCB Eugeniu Rosca
2019-05-23 15:32 ` [U-Boot] [PATCH v3 1/3] include: android_bootloader_message.h: Minimize the diff to AOSP Eugeniu Rosca
2019-06-22 19:09 ` Simon Glass
2019-07-11 22:06 ` Tom Rini
2019-05-23 15:32 ` [U-Boot] [PATCH v3 2/3] cmd: Add 'bcb' command to read/modify/write BCB fields Eugeniu Rosca
2019-06-22 19:09 ` Simon Glass
2019-07-06 16:18 ` Eugeniu Rosca
2019-07-11 22:06 ` Tom Rini
2019-05-23 15:32 ` [U-Boot] [PATCH v3 3/3] doc: relocate/rename Android README and add BCB overview Eugeniu Rosca
2019-06-22 19:09 ` Simon Glass
2019-07-02 21:25 ` Eugeniu Rosca
2019-07-02 18:11 ` Sam Protsenko
2019-07-02 21:22 ` Eugeniu Rosca
2019-07-11 22:06 ` Tom Rini
2019-05-23 22:58 ` [U-Boot] [PATCH v3 0/3] Add 'bcb' command to read/modify/write Android BCB Sam Protsenko
2019-05-27 15:26 ` Eugeniu Rosca
2019-06-04 12:36 ` Sam Protsenko
2019-06-04 13:25 ` Tom Rini
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.