[U-Boot] [PATCH v2 0/2] Add 'bcb' command to read/modify/write Android BCB

[U-Boot] [PATCH v2 0/2] Add 'bcb' command to read/modify/write Android BCB

[Eugeniu Rosca]

The motivation behind the 'bcb' command is largely explained
in the second patch. The other patch performs polishing of
https://patchwork.ozlabs.org/patch/1099689/, which is a hard
prerequisite for this series.

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 (2):
  include: android_bootloader_message.h: Minimize the diff to AOSP
  cmd: Add 'bcb' command to read/modify/write BCB fields

 cmd/Kconfig                          |  17 ++
 cmd/Makefile                         |   1 +
 cmd/bcb.c                            | 330 +++++++++++++++++++++++++++
 include/android_bootloader_message.h | 126 +++++-----
 4 files changed, 416 insertions(+), 58 deletions(-)
 create mode 100644 cmd/bcb.c

-- 
2.21.0

[U-Boot] [PATCH v2 1/2] include: android_bootloader_message.h: Minimize the diff to AOSP

[Eugeniu Rosca]

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>
---
v2:
 - Newly pushed. No changes.
---
 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

[U-Boot] [PATCH v2 2/2] cmd: Add 'bcb' command to read/modify/write BCB fields

[Eugeniu Rosca]

'Bootloader Control Block' (BCB) is a well established term/acronym in
the Android namespace which refers to a location in a dedicated raw
(i.e. FS-unaware) flash (e.g. eMMC) partition, usually called "misc",
which is used as media for exchanging messages between Android userspace
(particularly recovery [1]) and an Android-capable bootloader.

On higher level, this allows implementing a subset of Android Bootloader
Requirements [2], amongst which is the Android-specific bootloader
flow [3]. Regardless how the latter is implemented in U-Boot ([3] being
the most memorable example), reading/writing/dumping the BCB fields in
the development process from inside the U-Boot is a convenient feature.
Hence, make it available to the users.

Some usage examples of the new command recorded on R-Car H3ULCB-KF
('>>>' is an overlay on top of the original console output):

=> bcb
bcb - Load/set/clear/test/dump/store Android BCB fields

Usage:
bcb load  <dev> <part>       - load  BCB from mmc <dev>:<part>
bcb set   <field> <val>      - set   BCB <field> to <val>
bcb clear [<field>]          - clear BCB <field> or all fields
bcb test  <field> <op> <val> - test  BCB <field> against <val>
bcb dump  <field>            - dump  BCB <field>
bcb store                    - store BCB back to mmc

Legend:
<dev>   - MMC device index containing the BCB partition
<part>  - MMC partition index or name containing the BCB
<field> - one of {command,status,recovery,stage,reserved}
<op>    - the binary operator used in 'bcb test':
          '=' returns true if <val> matches the string stored in <field>
          '~' returns true if <val> matches a subset of <field>'s string
<val>   - string/text provided as input to bcb {set,test}
          NOTE: any ':' character in <val> will be replaced by line feed
          during 'bcb set' and used as separator by upper layers

=> bcb dump command
Error: BCB not loaded!
 >>> Users must specify mmc device and partition before any other call

=> bcb load 1 misc
=> bcb load 1 1
 >>> The two calls are equivalent (assuming "misc" has index 1)

=> bcb dump command
00000000: 62 6f 6f 74 6f 6e 63 65 2d 73 68 65 6c 6c 00 72    bootonce-shell.r
00000010: 79 00 72 00 00 00 00 00 00 00 00 00 00 00 00 00    y.r.............
 >>> The output is in binary/string format for convenience
 >>> The output size matches the size of inspected BCB field
 >>> (32 bytes in case of 'command')

=> bcb test command = bootonce-shell && echo true
true
=> bcb test command = bootonce-shell- && echo true
=> bcb test command = bootonce-shel && echo true
 >>> The '=' operator returns 'true' on perfect match

=> bcb test command ~ bootonce-shel && echo true
true
=> bcb test command ~ bootonce-shell && echo true
true
 >>> The '~' operator returns 'true' on substring match

=> bcb set command recovery
=> bcb dump command
00000000: 72 65 63 6f 76 65 72 79 00 73 68 65 6c 6c 00 72    recovery.shell.r
00000010: 79 00 72 00 00 00 00 00 00 00 00 00 00 00 00 00    y.r.............
 >>> The new value is NULL-terminated and stored in the BCB field

=> bcb set recovery "msg1:msg2:msg3"
=> bcb dump recovery
00000040: 6d 73 67 31 0a 6d 73 67 32 0a 6d 73 67 33 00 00    msg1.msg2.msg3..
00000050: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
00000060: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
 >>> --- snip ---
 >>> Every ':' is replaced by line-feed '\n' (0xA). The latter is used
 >>> as separator between individual commands by Android userspace

=> bcb store
 >>> Flush/store the BCB structure to MMC

[1] https://android.googlesource.com/platform/bootable/recovery
[2] https://source.android.com/devices/bootloader
[3] https://patchwork.ozlabs.org/patch/746835/
    ("[U-Boot,5/6] Initial support for the Android Bootloader flow")

Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---
v2:
 - [Heinrich Schuchardt] Implement sub-commands via U_BOOT_CMD_MKENT.
 - Polished the code. Ensured no warnings returned by sparse, smatch,
   `cppcheck --force --enable=all --inconclusive`, make W=1.
 - Tested on R-Car-H3-ES20 ULCB-KF.

v1:
 - https://patchwork.ozlabs.org/patch/1080395/
---
 cmd/Kconfig  |  17 +++
 cmd/Makefile |   1 +
 cmd/bcb.c    | 330 +++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 348 insertions(+)
 create mode 100644 cmd/bcb.c

diff --git a/cmd/Kconfig b/cmd/Kconfig
index 0d36da2a5c43..495bc1052f50 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -631,6 +631,23 @@ config CMD_ADC
 	  Shows ADC device info and permit printing one-shot analog converted
 	  data from a named Analog to Digital Converter.
 
+config CMD_BCB
+	bool "bcb"
+	depends on MMC
+	depends on PARTITIONS
+	help
+	  Read/modify/write the fields of Bootloader Control Block, usually
+	  stored on the flash "misc" partition with its structure defined in:
+	  https://android.googlesource.com/platform/bootable/recovery/+/master/
+	  bootloader_message/include/bootloader_message/bootloader_message.h
+
+	  Some real-life use-cases include (but are not limited to):
+	  - Determine the "boot reason" (and act accordingly):
+	    https://source.android.com/devices/bootloader/boot-reason
+	  - Get/pass a list of commands from/to recovery:
+	    https://android.googlesource.com/platform/bootable/recovery
+	  - Inspect/dump the contents of the BCB fields
+
 config CMD_BIND
 	bool "bind/unbind - Bind or unbind a device to/from a driver"
 	depends on DM
diff --git a/cmd/Makefile b/cmd/Makefile
index 7864fcf95c36..8f31b478eb1b 100644
--- a/cmd/Makefile
+++ b/cmd/Makefile
@@ -16,6 +16,7 @@ obj-$(CONFIG_CMD_ADC) += adc.o
 obj-$(CONFIG_CMD_ARMFLASH) += armflash.o
 obj-y += blk_common.o
 obj-$(CONFIG_CMD_SOURCE) += source.o
+obj-$(CONFIG_CMD_BCB) += bcb.o
 obj-$(CONFIG_CMD_BDI) += bdinfo.o
 obj-$(CONFIG_CMD_BEDBUG) += bedbug.o
 obj-$(CONFIG_CMD_BIND) += bind.o
diff --git a/cmd/bcb.c b/cmd/bcb.c
new file mode 100644
index 000000000000..5d8486684542
--- /dev/null
+++ b/cmd/bcb.c
@@ -0,0 +1,330 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2019 Eugeniu Rosca <rosca.eugeniu@gmail.com>
+ *
+ * Command to read/modify/write Android BCB fields
+ */
+
+#include <android_bootloader_message.h>
+#include <command.h>
+#include <common.h>
+#include <malloc.h>
+
+enum bcb_cmd {
+	BCB_CMD_LOAD,
+	BCB_CMD_FIELD_SET,
+	BCB_CMD_FIELD_CLEAR,
+	BCB_CMD_FIELD_TEST,
+	BCB_CMD_FIELD_DUMP,
+	BCB_CMD_STORE,
+};
+
+static int bcb_dev = -1;
+static int bcb_part = -1;
+static struct bootloader_message bcb = { { 0 } };
+
+static int bcb_cmd_get(char *cmd)
+{
+	if (!strncmp(cmd, "load", sizeof("load")))
+		return BCB_CMD_LOAD;
+	if (!strncmp(cmd, "set", sizeof("set")))
+		return BCB_CMD_FIELD_SET;
+	if (!strncmp(cmd, "clear", sizeof("clear")))
+		return BCB_CMD_FIELD_CLEAR;
+	if (!strncmp(cmd, "test", sizeof("test")))
+		return BCB_CMD_FIELD_TEST;
+	if (!strncmp(cmd, "store", sizeof("store")))
+		return BCB_CMD_STORE;
+	if (!strncmp(cmd, "dump", sizeof("dump")))
+		return BCB_CMD_FIELD_DUMP;
+	else
+		return -1;
+}
+
+static int do_bcb_load(cmd_tbl_t *cmdtp, int flag, int argc,
+		       char * const argv[])
+{
+	struct blk_desc *desc;
+	disk_partition_t info;
+	u64 cnt;
+	char *endp;
+	int part;
+
+	if (argc != 3)
+		return CMD_RET_USAGE;
+
+	if (blk_get_device_by_str("mmc", argv[1], &desc) < 0)
+		goto err_1;
+
+	part = simple_strtoul(argv[2], &endp, 0);
+	if (*endp == '\0') {
+		if (part_get_info(desc, part, &info))
+			goto err_1;
+	} else {
+		part = part_get_info_by_name(desc, argv[2], &info);
+		if (part < 0)
+			goto err_1;
+	}
+
+	cnt = DIV_ROUND_UP(sizeof(struct bootloader_message), info.blksz);
+	if (cnt > info.size)
+		goto err_2;
+
+	if (blk_dread(desc, info.start, cnt, &bcb) != cnt)
+		goto err_1;
+
+	bcb_dev = desc->devnum;
+	bcb_part = part;
+	debug("%s: Loaded from mmc %d:%d\n", __func__, bcb_dev, bcb_part);
+
+	return CMD_RET_SUCCESS;
+err_1:
+	printf("Error: Failed to read from mmc %s:%s\n", argv[1], argv[2]);
+	goto err;
+err_2:
+	printf("Error: mmc %s:%s too small!", argv[1], argv[2]);
+	goto err;
+err:
+	bcb_dev = -1;
+	bcb_part = -1;
+	return CMD_RET_FAILURE;
+}
+
+static int bcb_is_misused(int argc, char *const argv[])
+{
+	if (bcb_dev < 0 || bcb_part < 0) {
+		printf("Error: BCB not loaded!\n");
+		return -1;
+	}
+	switch (bcb_cmd_get(argv[0])) {
+	case BCB_CMD_LOAD:
+		/* Dedicated arg handling */
+		break;
+	case BCB_CMD_FIELD_SET:
+		if (argc != 3)
+			goto err;
+		break;
+	case BCB_CMD_FIELD_TEST:
+		if (argc != 4)
+			goto err;
+		break;
+	case BCB_CMD_FIELD_CLEAR:
+		if (argc != 1 && argc != 2)
+			goto err;
+		break;
+	case BCB_CMD_STORE:
+		if (argc != 1)
+			goto err;
+		break;
+	case BCB_CMD_FIELD_DUMP:
+		if (argc != 2)
+			goto err;
+		break;
+	default:
+		debug("%s: Error: Unexpected BCB command\n", __func__);
+		return -1;
+	}
+
+	return 0;
+err:
+	printf("Error: Bad usage of 'bcb %s'\n", argv[0]);
+	return -1;
+}
+
+static int
+bcb_field_get(char *name, char **field, int *size)
+{
+	if (!strncmp(name, "command", sizeof("command"))) {
+		*field = bcb.command;
+		*size = sizeof(bcb.command);
+	} else if (!strncmp(name, "status", sizeof("status"))) {
+		*field = bcb.status;
+		*size = sizeof(bcb.status);
+	} else if (!strncmp(name, "recovery", sizeof("recovery"))) {
+		*field = bcb.recovery;
+		*size = sizeof(bcb.recovery);
+	} else if (!strncmp(name, "stage", sizeof("stage"))) {
+		*field = bcb.stage;
+		*size = sizeof(bcb.stage);
+	} else if (!strncmp(name, "reserved", sizeof("reserved"))) {
+		*field = bcb.reserved;
+		*size = sizeof(bcb.reserved);
+	} else {
+		printf("Error: Unknown field '%s'\n", name);
+		return -1;
+	}
+	return 0;
+}
+
+static int do_bcb_set(cmd_tbl_t *cmdtp, int flag, int argc,
+		      char * const argv[])
+{
+	int size, len;
+	char *field, *str, *found, *keep;
+
+	if (bcb_is_misused(argc, argv))
+		return CMD_RET_FAILURE;
+
+	if (bcb_field_get(argv[1], &field, &size))
+		return CMD_RET_FAILURE;
+
+	len = strlen(argv[2]);
+	if (len >= size) {
+		printf("Error: sizeof('%s') = %d >= %d = sizeof(bcb.%s)\n",
+		       argv[2], len, size, argv[1]);
+		return CMD_RET_FAILURE;
+	}
+	str = strdup(argv[2]);
+	keep = str;
+
+	field[0] = '\0';
+	while ((found = strsep(&str, ":"))) {
+		if (field[0] != '\0')
+			strcat(field, "\n");
+		strcat(field, found);
+	}
+
+	free(keep);
+	return CMD_RET_SUCCESS;
+}
+
+static int do_bcb_clear(cmd_tbl_t *cmdtp, int flag, int argc,
+			char * const argv[])
+{
+	int size;
+	char *field;
+
+	if (bcb_is_misused(argc, argv))
+		return CMD_RET_FAILURE;
+
+	if (argc == 1) {
+		memset(&bcb, 0, sizeof(bcb));
+		return CMD_RET_SUCCESS;
+	}
+
+	if (bcb_field_get(argv[1], &field, &size))
+		return CMD_RET_FAILURE;
+
+	memset(field, 0, size);
+	return CMD_RET_SUCCESS;
+}
+
+static int do_bcb_dump(cmd_tbl_t *cmdtp, int flag, int argc,
+		       char * const argv[])
+{
+	int size;
+	char *field;
+
+	if (bcb_is_misused(argc, argv))
+		return CMD_RET_FAILURE;
+
+	if (bcb_field_get(argv[1], &field, &size))
+		return CMD_RET_FAILURE;
+
+	print_buffer((ulong)field - (ulong)&bcb, (void *)field, 1, size, 16);
+	return CMD_RET_SUCCESS;
+}
+
+static int do_bcb_test(cmd_tbl_t *cmdtp, int flag, int argc,
+		       char * const argv[])
+{
+	int size;
+	char *field;
+
+	if (bcb_is_misused(argc, argv))
+		return CMD_RET_FAILURE;
+
+	if (bcb_field_get(argv[1], &field, &size))
+		return CMD_RET_FAILURE;
+
+	if (!strncmp(argv[2], "=", sizeof("="))) {
+		if (!strncmp(argv[3], field, size))
+			return CMD_RET_SUCCESS;
+		else
+			return CMD_RET_FAILURE;
+	} else if (!strncmp(argv[2], "~", sizeof("~"))) {
+		if (!strstr(field, argv[3]))
+			return CMD_RET_FAILURE;
+		else
+			return CMD_RET_SUCCESS;
+	} else {
+		printf("Error: Unknown operator '%s'\n", argv[2]);
+	}
+	return CMD_RET_FAILURE;
+}
+
+static int do_bcb_store(cmd_tbl_t *cmdtp, int flag, int argc,
+			char * const argv[])
+{
+	struct blk_desc *desc;
+	disk_partition_t info;
+	u64 cnt;
+
+	if (bcb_is_misused(argc, argv))
+		return CMD_RET_FAILURE;
+
+	desc = blk_get_devnum_by_type(IF_TYPE_MMC, bcb_dev);
+	if (!desc)
+		goto err;
+
+	if (part_get_info(desc, bcb_part, &info))
+		goto err;
+
+	cnt = DIV_ROUND_UP(sizeof(struct bootloader_message), info.blksz);
+
+	if (blk_dwrite(desc, info.start, cnt, &bcb) != cnt)
+		goto err;
+
+	return CMD_RET_SUCCESS;
+err:
+	printf("Error: Failed to write to mmc %d:%d\n", bcb_dev, bcb_part);
+	return CMD_RET_FAILURE;
+}
+
+static cmd_tbl_t cmd_bcb_sub[] = {
+	U_BOOT_CMD_MKENT(load, CONFIG_SYS_MAXARGS, 1, do_bcb_load, "", ""),
+	U_BOOT_CMD_MKENT(set, CONFIG_SYS_MAXARGS, 1, do_bcb_set, "", ""),
+	U_BOOT_CMD_MKENT(clear, CONFIG_SYS_MAXARGS, 1, do_bcb_clear, "", ""),
+	U_BOOT_CMD_MKENT(test, CONFIG_SYS_MAXARGS, 1, do_bcb_test, "", ""),
+	U_BOOT_CMD_MKENT(dump, CONFIG_SYS_MAXARGS, 1, do_bcb_dump, "", ""),
+	U_BOOT_CMD_MKENT(store, CONFIG_SYS_MAXARGS, 1, do_bcb_store, "", ""),
+};
+
+static int do_bcb(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[])
+{
+	cmd_tbl_t *c;
+
+	if (argc < 2)
+		return CMD_RET_USAGE;
+
+	argc--;
+	argv++;
+
+	c = find_cmd_tbl(argv[0], cmd_bcb_sub, ARRAY_SIZE(cmd_bcb_sub));
+	if (c)
+		return c->cmd(cmdtp, flag, argc, argv);
+
+	return CMD_RET_USAGE;
+}
+
+U_BOOT_CMD(
+	bcb, CONFIG_SYS_MAXARGS, 1, do_bcb,
+	"Load/set/clear/test/dump/store Android BCB fields",
+	"load  <dev> <part>       - load  BCB from mmc <dev>:<part>\n"
+	"bcb set   <field> <val>      - set   BCB <field> to <val>\n"
+	"bcb clear [<field>]          - clear BCB <field> or all fields\n"
+	"bcb test  <field> <op> <val> - test  BCB <field> against <val>\n"
+	"bcb dump  <field>            - dump  BCB <field>\n"
+	"bcb store                    - store BCB back to mmc\n"
+	"\n"
+	"Legend:\n"
+	"<dev>   - MMC device index containing the BCB partition\n"
+	"<part>  - MMC partition index or name containing the BCB\n"
+	"<field> - one of {command,status,recovery,stage,reserved}\n"
+	"<op>    - the binary operator used in 'bcb test':\n"
+	"          '=' returns true if <val> matches the string stored in <field>\n"
+	"          '~' returns true if <val> matches a subset of <field>'s string\n"
+	"<val>   - string/text provided as input to bcb {set,test}\n"
+	"          NOTE: any ':' character in <val> will be replaced by line feed\n"
+	"          during 'bcb set' and used as separator by upper layers\n"
+);
-- 
2.21.0

[U-Boot] [PATCH v2 0/2] Add 'bcb' command to read/modify/write Android BCB

[Eugeniu Rosca]

Hi All,

cc: Stephen, Jeremy

FWIW/jFYI, the patchwork frontend appears to mangle/skip spaces
in the patch subjects. Examples:
 - https://patchwork.ozlabs.org/cover/1101106/
 - https://patchwork.ozlabs.org/patch/1101108/
 - https://patchwork.ozlabs.org/patch/1101107/

The same patches look fine on https://marc.info:
 - https://marc.info/?l=u-boot&m=155810440206070&w=2
 - https://marc.info/?l=u-boot&m=155810448306089&w=2
 - https://marc.info/?l=u-boot&m=155810444306080&w=2

-- 
Best Regards,
Eugeniu.

[U-Boot] [PATCH v2 0/2] Add 'bcb' command to read/modify/write Android BCB

[Stephen Finucane]

On Fri, 2019-05-17 at 17:24 +0200, Eugeniu Rosca wrote:
> Hi All,
> 
> cc: Stephen, Jeremy
> 
> FWIW/jFYI, the patchwork frontend appears to mangle/skip spaces
> in the patch subjects. Examples:
>  - https://patchwork.ozlabs.org/cover/1101106/
>  - https://patchwork.ozlabs.org/patch/1101108/
>  - https://patchwork.ozlabs.org/patch/1101107/
> 
> The same patches look fine on https://marc.info:
>  - https://marc.info/?l=u-boot&m=155810440206070&w=2
>  - https://marc.info/?l=u-boot&m=155810448306089&w=2
>  - https://marc.info/?l=u-boot&m=155810444306080&w=2
> 

http://patchwork.ozlabs.org/patch/1099264/

We're just waiting for ozlabs to apply that patch.

Stephen

[U-Boot] [PATCH v2 0/2] Add 'bcb' command to read/modify/write Android BCB

[Eugeniu Rosca]

On Fri, May 17, 2019 at 04:26:23PM +0100, Stephen Finucane wrote:
[..]
> http://patchwork.ozlabs.org/patch/1099264/
> 
> We're just waiting for ozlabs to apply that patch.
> 
> Stephen

Thanks!

-- 
Best Regards,
Eugeniu.

[U-Boot] [PATCH v2 2/2] cmd: Add 'bcb' command to read/modify/write BCB fields

[Simon Glass]

Hi Eugeniu,

On Fri, 17 May 2019 at 08:46, Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
>
> 'Bootloader Control Block' (BCB) is a well established term/acronym in
> the Android namespace which refers to a location in a dedicated raw
> (i.e. FS-unaware) flash (e.g. eMMC) partition, usually called "misc",
> which is used as media for exchanging messages between Android userspace
> (particularly recovery [1]) and an Android-capable bootloader.
>
> On higher level, this allows implementing a subset of Android Bootloader
> Requirements [2], amongst which is the Android-specific bootloader
> flow [3]. Regardless how the latter is implemented in U-Boot ([3] being
> the most memorable example), reading/writing/dumping the BCB fields in
> the development process from inside the U-Boot is a convenient feature.
> Hence, make it available to the users.
>
> Some usage examples of the new command recorded on R-Car H3ULCB-KF
> ('>>>' is an overlay on top of the original console output):
>
> => bcb
> bcb - Load/set/clear/test/dump/store Android BCB fields
>
> Usage:
> bcb load  <dev> <part>       - load  BCB from mmc <dev>:<part>
> bcb set   <field> <val>      - set   BCB <field> to <val>
> bcb clear [<field>]          - clear BCB <field> or all fields
> bcb test  <field> <op> <val> - test  BCB <field> against <val>
> bcb dump  <field>            - dump  BCB <field>
> bcb store                    - store BCB back to mmc
>
> Legend:
> <dev>   - MMC device index containing the BCB partition
> <part>  - MMC partition index or name containing the BCB
> <field> - one of {command,status,recovery,stage,reserved}
> <op>    - the binary operator used in 'bcb test':
>           '=' returns true if <val> matches the string stored in <field>
>           '~' returns true if <val> matches a subset of <field>'s string
> <val>   - string/text provided as input to bcb {set,test}
>           NOTE: any ':' character in <val> will be replaced by line feed
>           during 'bcb set' and used as separator by upper layers
>
> => bcb dump command
> Error: BCB not loaded!
>  >>> Users must specify mmc device and partition before any other call
>
> => bcb load 1 misc
> => bcb load 1 1
>  >>> The two calls are equivalent (assuming "misc" has index 1)
>
> => bcb dump command
> 00000000: 62 6f 6f 74 6f 6e 63 65 2d 73 68 65 6c 6c 00 72    bootonce-shell.r
> 00000010: 79 00 72 00 00 00 00 00 00 00 00 00 00 00 00 00    y.r.............
>  >>> The output is in binary/string format for convenience
>  >>> The output size matches the size of inspected BCB field
>  >>> (32 bytes in case of 'command')
>
> => bcb test command = bootonce-shell && echo true
> true
> => bcb test command = bootonce-shell- && echo true
> => bcb test command = bootonce-shel && echo true
>  >>> The '=' operator returns 'true' on perfect match
>
> => bcb test command ~ bootonce-shel && echo true
> true
> => bcb test command ~ bootonce-shell && echo true
> true
>  >>> The '~' operator returns 'true' on substring match
>
> => bcb set command recovery
> => bcb dump command
> 00000000: 72 65 63 6f 76 65 72 79 00 73 68 65 6c 6c 00 72    recovery.shell.r
> 00000010: 79 00 72 00 00 00 00 00 00 00 00 00 00 00 00 00    y.r.............
>  >>> The new value is NULL-terminated and stored in the BCB field
>
> => bcb set recovery "msg1:msg2:msg3"
> => bcb dump recovery
> 00000040: 6d 73 67 31 0a 6d 73 67 32 0a 6d 73 67 33 00 00    msg1.msg2.msg3..
> 00000050: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
> 00000060: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
>  >>> --- snip ---
>  >>> Every ':' is replaced by line-feed '\n' (0xA). The latter is used
>  >>> as separator between individual commands by Android userspace
>
> => bcb store
>  >>> Flush/store the BCB structure to MMC
>
> [1] https://android.googlesource.com/platform/bootable/recovery
> [2] https://source.android.com/devices/bootloader
> [3] https://patchwork.ozlabs.org/patch/746835/
>     ("[U-Boot,5/6] Initial support for the Android Bootloader flow")
>
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> ---
> v2:
>  - [Heinrich Schuchardt] Implement sub-commands via U_BOOT_CMD_MKENT.
>  - Polished the code. Ensured no warnings returned by sparse, smatch,
>    `cppcheck --force --enable=all --inconclusive`, make W=1.
>  - Tested on R-Car-H3-ES20 ULCB-KF.
>
> v1:
>  - https://patchwork.ozlabs.org/patch/1080395/
> ---
>  cmd/Kconfig  |  17 +++
>  cmd/Makefile |   1 +
>  cmd/bcb.c    | 330 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 348 insertions(+)
>  create mode 100644 cmd/bcb.c

Where is this documented? Perhaps it should go in README.avb2?

Should it default to enabled if avb is used?

Regards,
Simon

[U-Boot] [PATCH v2 2/2] cmd: Add 'bcb' command to read/modify/write BCB fields

[Eugeniu Rosca]

Hi Simon
cc: Sam, Igor, feel free to correct/augment anything of below

On Sat, May 18, 2019 at 10:33:02AM -0600, Simon Glass wrote:
> Hi Eugeniu,
> 
> On Fri, 17 May 2019 at 08:46, Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
> >
> > 'Bootloader Control Block' (BCB) is a well established term/acronym in
> > the Android namespace which refers to a location in a dedicated raw
> > (i.e. FS-unaware) flash (e.g. eMMC) partition, usually called "misc",
> > which is used as media for exchanging messages between Android userspace
> > (particularly recovery [1]) and an Android-capable bootloader.
> >
[..]
> 
> Where is this documented? Perhaps it should go in README.avb2?

README.avb2 is solely about the "verified/secure" booting of Android,
while the 'bcb' command proposed in this patch can be useful both in
verified boot scenarios (e.g. full-featured U-Boot builds), as well
as in non-avb ones (e.g. development, PoC, demos, custom
configurations). Thus, I think that README.avb2 is not the best
place for 'bcb'.

More generally, as somebody who uses git as an extension of himself, I
am quite happy with commit-only documentation. OTOH, for those who
receive U-Boot in tarballs or expect source-level docs/tutorials, I
agree that having the 'bcb' described in a README might be helpful.

I can identify two Android-dedicated README files, but none of them
seems to be suitable for the new command:
 - doc/README.android-fastboot
 - doc/README.avb2

Igor, Sam, what's your view on the above? Would you suggest creating
a doc/README.android-bcb or there is a more elegant solution to it?

> 
> Should it default to enabled if avb is used?

I think at this specific moment in time, 'bcb' is orthogonal (meaning it
is neither a direct, nor a reverse dependency) to any other Android
feature in U-Boot. This could be re-assessed, if platform maintainers
start to rely on 'bcb' in their U-Boot environments on regular basis.

-- 
Best Regards,
Eugeniu.

[U-Boot] [PATCH v2 2/2] cmd: Add 'bcb' command to read/modify/write BCB fields

[Alex Kiernan]

On Mon, May 20, 2019 at 8:23 AM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
>
> >
> > Should it default to enabled if avb is used?
>
> I think at this specific moment in time, 'bcb' is orthogonal (meaning it
> is neither a direct, nor a reverse dependency) to any other Android
> feature in U-Boot. This could be re-assessed, if platform maintainers
> start to rely on 'bcb' in their U-Boot environments on regular basis.
>

'bcb' looks like something I'd be interested in, not running Android
at all... currently I (ab)use the bootcounter to communicate between
the kernel and U-Boot when I want to force a board through recovery,
but this looks like something that might well give me a better
interface.

-- 
Alex Kiernan

[U-Boot] [PATCH v2 2/2] cmd: Add 'bcb' command to read/modify/write BCB fields

[Eugeniu Rosca]

Hi Alex,

On Mon, May 20, 2019 at 08:32:28AM +0100, Alex Kiernan wrote:
> On Mon, May 20, 2019 at 8:23 AM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
> >
> > >
> > > Should it default to enabled if avb is used?
> >
> > I think at this specific moment in time, 'bcb' is orthogonal (meaning it
> > is neither a direct, nor a reverse dependency) to any other Android
> > feature in U-Boot. This could be re-assessed, if platform maintainers
> > start to rely on 'bcb' in their U-Boot environments on regular basis.
> >
> 
> 'bcb' looks like something I'd be interested in, not running Android
> at all... currently I (ab)use the bootcounter to communicate between
> the kernel and U-Boot when I want to force a board through recovery,
> but this looks like something that might well give me a better
> interface.

Thanks for this piece of feedback. It means 'bcb' concept and
implementation might span beyond the Android use-cases/needs.
On small scale, this seems to approve my choice of not inserting the
"android" keyword into the command or Kconfig symbol names.

-- 
Best Regards,
Eugeniu.

[U-Boot] [PATCH v2 2/2] cmd: Add 'bcb' command to read/modify/write BCB fields

[Sam Protsenko]

Hi Eugeniu,


On Mon, May 20, 2019 at 10:23 AM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
>
> Hi Simon
> cc: Sam, Igor, feel free to correct/augment anything of below
>
> On Sat, May 18, 2019 at 10:33:02AM -0600, Simon Glass wrote:
> > Hi Eugeniu,
> >
> > On Fri, 17 May 2019 at 08:46, Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
> > >
> > > 'Bootloader Control Block' (BCB) is a well established term/acronym in
> > > the Android namespace which refers to a location in a dedicated raw
> > > (i.e. FS-unaware) flash (e.g. eMMC) partition, usually called "misc",
> > > which is used as media for exchanging messages between Android userspace
> > > (particularly recovery [1]) and an Android-capable bootloader.
> > >
> [..]
> >
> > Where is this documented? Perhaps it should go in README.avb2?
>
> README.avb2 is solely about the "verified/secure" booting of Android,
> while the 'bcb' command proposed in this patch can be useful both in
> verified boot scenarios (e.g. full-featured U-Boot builds), as well
> as in non-avb ones (e.g. development, PoC, demos, custom
> configurations). Thus, I think that README.avb2 is not the best
> place for 'bcb'.
>
> More generally, as somebody who uses git as an extension of himself, I
> am quite happy with commit-only documentation. OTOH, for those who
> receive U-Boot in tarballs or expect source-level docs/tutorials, I
> agree that having the 'bcb' described in a README might be helpful.
>
> I can identify two Android-dedicated README files, but none of them
> seems to be suitable for the new command:
>  - doc/README.android-fastboot
>  - doc/README.avb2
>
> Igor, Sam, what's your view on the above? Would you suggest creating
> a doc/README.android-bcb or there is a more elegant solution to it?
>

Documentation would be nice. Although you already provided a generic
description of 'bcb' command in 'help bcb', the user often wants to
read more high-level documentation. I'd say, you can copy the
description from commit message to doc/README.android-bcb, extending
it with usual use-cases, and how this command can be used in those
use-cases. For example, your pseudo-code for reboot reason you
provided to me earlier, etc. Also, it can be useful to mention if
Google have any requirements (mandatory or optional) for the
bootloader (misc partition, reboot reason, recovery, etc), and how
this 'bcb' command can help in those requirements implementation.

All that said, IMHO documentation/test wise: it's not critical in this
case, you can add that later, just to speed-up the whole development
process and divide it into iterations. But that's for maintainers to
decide, of course.

Also, I've a feeling that we have another problem, more common than
just a documentation. At the moment we have a bunch of Android related
features, which don't have namespace separation on several levels:
 - file/directories: we may want to move all Android related commands
to sub-directory
 - commands: we may want to add main command called "android" for all
Android-related commands, or maybe just a prefix
 - Kconfig: we may want to have some generic naming scheme for all
Android-related commands
 - Documentation, tests: the same here

So at some point we will probably need to discuss and fix that
somehow. Not here, of course :)

> >
> > Should it default to enabled if avb is used?
>
> I think at this specific moment in time, 'bcb' is orthogonal (meaning it
> is neither a direct, nor a reverse dependency) to any other Android
> feature in U-Boot. This could be re-assessed, if platform maintainers
> start to rely on 'bcb' in their U-Boot environments on regular basis.
>
> --
> Best Regards,
> Eugeniu.

[U-Boot] [PATCH v2 1/2] include: android_bootloader_message.h: Minimize the diff to AOSP

[Sam Protsenko]

Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>

On Fri, May 17, 2019 at 5:46 PM 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>
> ---
> v2:
>  - Newly pushed. No changes.
> ---
>  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
>

[U-Boot] [PATCH v2 2/2] cmd: Add 'bcb' command to read/modify/write BCB fields

[Sam Protsenko]

Anyway, from my side:

Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>

On Mon, May 20, 2019 at 6:16 PM Sam Protsenko
<semen.protsenko@linaro.org> wrote:
>
> Hi Eugeniu,
>
>
> On Mon, May 20, 2019 at 10:23 AM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
> >
> > Hi Simon
> > cc: Sam, Igor, feel free to correct/augment anything of below
> >
> > On Sat, May 18, 2019 at 10:33:02AM -0600, Simon Glass wrote:
> > > Hi Eugeniu,
> > >
> > > On Fri, 17 May 2019 at 08:46, Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
> > > >
> > > > 'Bootloader Control Block' (BCB) is a well established term/acronym in
> > > > the Android namespace which refers to a location in a dedicated raw
> > > > (i.e. FS-unaware) flash (e.g. eMMC) partition, usually called "misc",
> > > > which is used as media for exchanging messages between Android userspace
> > > > (particularly recovery [1]) and an Android-capable bootloader.
> > > >
> > [..]
> > >
> > > Where is this documented? Perhaps it should go in README.avb2?
> >
> > README.avb2 is solely about the "verified/secure" booting of Android,
> > while the 'bcb' command proposed in this patch can be useful both in
> > verified boot scenarios (e.g. full-featured U-Boot builds), as well
> > as in non-avb ones (e.g. development, PoC, demos, custom
> > configurations). Thus, I think that README.avb2 is not the best
> > place for 'bcb'.
> >
> > More generally, as somebody who uses git as an extension of himself, I
> > am quite happy with commit-only documentation. OTOH, for those who
> > receive U-Boot in tarballs or expect source-level docs/tutorials, I
> > agree that having the 'bcb' described in a README might be helpful.
> >
> > I can identify two Android-dedicated README files, but none of them
> > seems to be suitable for the new command:
> >  - doc/README.android-fastboot
> >  - doc/README.avb2
> >
> > Igor, Sam, what's your view on the above? Would you suggest creating
> > a doc/README.android-bcb or there is a more elegant solution to it?
> >
>
> Documentation would be nice. Although you already provided a generic
> description of 'bcb' command in 'help bcb', the user often wants to
> read more high-level documentation. I'd say, you can copy the
> description from commit message to doc/README.android-bcb, extending
> it with usual use-cases, and how this command can be used in those
> use-cases. For example, your pseudo-code for reboot reason you
> provided to me earlier, etc. Also, it can be useful to mention if
> Google have any requirements (mandatory or optional) for the
> bootloader (misc partition, reboot reason, recovery, etc), and how
> this 'bcb' command can help in those requirements implementation.
>
> All that said, IMHO documentation/test wise: it's not critical in this
> case, you can add that later, just to speed-up the whole development
> process and divide it into iterations. But that's for maintainers to
> decide, of course.
>
> Also, I've a feeling that we have another problem, more common than
> just a documentation. At the moment we have a bunch of Android related
> features, which don't have namespace separation on several levels:
>  - file/directories: we may want to move all Android related commands
> to sub-directory
>  - commands: we may want to add main command called "android" for all
> Android-related commands, or maybe just a prefix
>  - Kconfig: we may want to have some generic naming scheme for all
> Android-related commands
>  - Documentation, tests: the same here
>
> So at some point we will probably need to discuss and fix that
> somehow. Not here, of course :)
>
> > >
> > > Should it default to enabled if avb is used?
> >
> > I think at this specific moment in time, 'bcb' is orthogonal (meaning it
> > is neither a direct, nor a reverse dependency) to any other Android
> > feature in U-Boot. This could be re-assessed, if platform maintainers
> > start to rely on 'bcb' in their U-Boot environments on regular basis.
> >
> > --
> > Best Regards,
> > Eugeniu.

[U-Boot] [PATCH v2 2/2] cmd: Add 'bcb' command to read/modify/write BCB fields

[Lukasz Majewski]

Hi Alex,

> On Mon, May 20, 2019 at 8:23 AM Eugeniu Rosca <erosca@de.adit-jv.com>
> wrote:
> >  
> > >
> > > Should it default to enabled if avb is used?  
> >
> > I think at this specific moment in time, 'bcb' is orthogonal
> > (meaning it is neither a direct, nor a reverse dependency) to any
> > other Android feature in U-Boot. This could be re-assessed, if
> > platform maintainers start to rely on 'bcb' in their U-Boot
> > environments on regular basis. 
> 
> 'bcb' looks like something I'd be interested in, not running Android
> at all... currently I (ab)use the bootcounter to communicate between
> the kernel and U-Boot when I want to force a board through recovery,

I don't know your exact use case, but wouldn't it be better to use envs
(with redundancy) and fw_setenv / fw_printenv from Linux user space?

Now those envs even support setting default values for u-boot (as there
is now separate library used for it). Moreover there is OE/Yocto's
recipe 'u-boot-fw-utils' which can be easily used and installed.

> but this looks like something that might well give me a better
> interface.
> 



Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190521/469a1648/attachment.sig>

[U-Boot] [PATCH v2 2/2] cmd: Add 'bcb' command to read/modify/write BCB fields

[Alex Kiernan]

On Tue, May 21, 2019 at 9:37 AM Lukasz Majewski <lukma@denx.de> wrote:
>
> Hi Alex,
>
> > On Mon, May 20, 2019 at 8:23 AM Eugeniu Rosca <erosca@de.adit-jv.com>
> > wrote:
> > >
> > > >
> > > > Should it default to enabled if avb is used?
> > >
> > > I think at this specific moment in time, 'bcb' is orthogonal
> > > (meaning it is neither a direct, nor a reverse dependency) to any
> > > other Android feature in U-Boot. This could be re-assessed, if
> > > platform maintainers start to rely on 'bcb' in their U-Boot
> > > environments on regular basis.
> >
> > 'bcb' looks like something I'd be interested in, not running Android
> > at all... currently I (ab)use the bootcounter to communicate between
> > the kernel and U-Boot when I want to force a board through recovery,
>
> I don't know your exact use case, but wouldn't it be better to use envs
> (with redundancy) and fw_setenv / fw_printenv from Linux user space?
>
> Now those envs even support setting default values for u-boot (as there
> is now separate library used for it). Moreover there is OE/Yocto's
> recipe 'u-boot-fw-utils' which can be easily used and installed.
>

It's a long story... I'm constrained by historic choices, which makes
using the environment problematic. But you're right.

-- 
Alex Kiernan

[U-Boot] [PATCH v2 2/2] cmd: Add 'bcb' command to read/modify/write BCB fields

[Eugeniu Rosca]

Hi Lukasz,

On Tue, May 21, 2019 at 10:02:34AM +0100, Alex Kiernan wrote:
> On Tue, May 21, 2019 at 9:37 AM Lukasz Majewski <lukma@denx.de> wrote:
> >
> > Hi Alex,
> >
> > > On Mon, May 20, 2019 at 8:23 AM Eugeniu Rosca <erosca@de.adit-jv.com>
> > > wrote:
> > > >
> > > > >
> > > > > Should it default to enabled if avb is used?
> > > >
> > > > I think at this specific moment in time, 'bcb' is orthogonal
> > > > (meaning it is neither a direct, nor a reverse dependency) to any
> > > > other Android feature in U-Boot. This could be re-assessed, if
> > > > platform maintainers start to rely on 'bcb' in their U-Boot
> > > > environments on regular basis.
> > >
> > > 'bcb' looks like something I'd be interested in, not running Android
> > > at all... currently I (ab)use the bootcounter to communicate between
> > > the kernel and U-Boot when I want to force a board through recovery,
> >
> > I don't know your exact use case, but wouldn't it be better to use envs
> > (with redundancy) and fw_setenv / fw_printenv from Linux user space?
> >
> > Now those envs even support setting default values for u-boot (as there
> > is now separate library used for it). Moreover there is OE/Yocto's
> > recipe 'u-boot-fw-utils' which can be easily used and installed.

That's a truly constructive suggestion. Nevertheless, I believe this
would not work in case of CONFIG_ENV_IS_NOWHERE=y, which is how U-Boot
is built and used by the developers in our organization.

> 
> It's a long story... I'm constrained by historic choices, which makes
> using the environment problematic. But you're right.
> 
> -- 
> Alex Kiernan

-- 
Best Regards,
Eugeniu.

[U-Boot] [PATCH v2 2/2] cmd: Add 'bcb' command to read/modify/write BCB fields

[Lukasz Majewski]

On Tue, 21 May 2019 11:13:52 +0200
Eugeniu Rosca <erosca@de.adit-jv.com> wrote:

> Hi Lukasz,
> 
> On Tue, May 21, 2019 at 10:02:34AM +0100, Alex Kiernan wrote:
> > On Tue, May 21, 2019 at 9:37 AM Lukasz Majewski <lukma@denx.de>
> > wrote:  
> > >
> > > Hi Alex,
> > >  
> > > > On Mon, May 20, 2019 at 8:23 AM Eugeniu Rosca
> > > > <erosca@de.adit-jv.com> wrote:  
> > > > >  
> > > > > >
> > > > > > Should it default to enabled if avb is used?  
> > > > >
> > > > > I think at this specific moment in time, 'bcb' is orthogonal
> > > > > (meaning it is neither a direct, nor a reverse dependency) to
> > > > > any other Android feature in U-Boot. This could be
> > > > > re-assessed, if platform maintainers start to rely on 'bcb'
> > > > > in their U-Boot environments on regular basis.  
> > > >
> > > > 'bcb' looks like something I'd be interested in, not running
> > > > Android at all... currently I (ab)use the bootcounter to
> > > > communicate between the kernel and U-Boot when I want to force
> > > > a board through recovery,  
> > >
> > > I don't know your exact use case, but wouldn't it be better to
> > > use envs (with redundancy) and fw_setenv / fw_printenv from Linux
> > > user space?
> > >
> > > Now those envs even support setting default values for u-boot (as
> > > there is now separate library used for it). Moreover there is
> > > OE/Yocto's recipe 'u-boot-fw-utils' which can be easily used and
> > > installed.  
> 
> That's a truly constructive suggestion. Nevertheless, I believe this
> would not work in case of CONFIG_ENV_IS_NOWHERE=y, which is how U-Boot
> is built and used by the developers in our organization.

I don't mind to see Android's "bcd" supported in U-Boot (I'm even
happy for it). 

And yes - the CONFIG_ENV_IS_NOWHERE means that one loads the default
envs (created at build time) to RAM for booting.

Just one note (maybe you will find it useful) - it is possible to
specify the default envs from external file:
https://lists.denx.de/pipermail/u-boot/2018-March/323347.html

As we don't have memory to store envs we cannot adjust or pass data via
it.

> 
> > 
> > It's a long story... I'm constrained by historic choices, which
> > makes using the environment problematic. But you're right.
> > 
> > -- 
> > Alex Kiernan  
> 




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190521/78029a36/attachment.sig>

[U-Boot] [PATCH v2 2/2] cmd: Add 'bcb' command to read/modify/write BCB fields

[Alex Kiernan]

On Tue, May 21, 2019 at 10:14 AM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
>
> Hi Lukasz,
>
> On Tue, May 21, 2019 at 10:02:34AM +0100, Alex Kiernan wrote:
> > On Tue, May 21, 2019 at 9:37 AM Lukasz Majewski <lukma@denx.de> wrote:
> > >
> > > Hi Alex,
> > >
> > > > On Mon, May 20, 2019 at 8:23 AM Eugeniu Rosca <erosca@de.adit-jv.com>
> > > > wrote:
> > > > >
> > > > > >
> > > > > > Should it default to enabled if avb is used?
> > > > >
> > > > > I think at this specific moment in time, 'bcb' is orthogonal
> > > > > (meaning it is neither a direct, nor a reverse dependency) to any
> > > > > other Android feature in U-Boot. This could be re-assessed, if
> > > > > platform maintainers start to rely on 'bcb' in their U-Boot
> > > > > environments on regular basis.
> > > >
> > > > 'bcb' looks like something I'd be interested in, not running Android
> > > > at all... currently I (ab)use the bootcounter to communicate between
> > > > the kernel and U-Boot when I want to force a board through recovery,
> > >
> > > I don't know your exact use case, but wouldn't it be better to use envs
> > > (with redundancy) and fw_setenv / fw_printenv from Linux user space?
> > >
> > > Now those envs even support setting default values for u-boot (as there
> > > is now separate library used for it). Moreover there is OE/Yocto's
> > > recipe 'u-boot-fw-utils' which can be easily used and installed.
>
> That's a truly constructive suggestion. Nevertheless, I believe this
> would not work in case of CONFIG_ENV_IS_NOWHERE=y, which is how U-Boot
> is built and used by the developers in our organization.
>

That's how we build/run too, but with with hackery like this in a boot
script to pick pieces out of the legacy world:

        mmc dev ${mmcdev}
        mmc read ${loadaddr} 1300 100
        env import -c ${loadaddr} 20000 ethaddr

Yes, it's ugly...

-- 
Alex Kiernan

[U-Boot] [PATCH v2 2/2] cmd: Add 'bcb' command to read/modify/write BCB fields

[Eugeniu Rosca]

On Tue, May 21, 2019 at 10:24:10AM +0100, Alex Kiernan wrote:
> On Tue, May 21, 2019 at 10:14 AM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
[..]
> > That's a truly constructive suggestion. Nevertheless, I believe this
> > would not work in case of CONFIG_ENV_IS_NOWHERE=y, which is how U-Boot
> > is built and used by the developers in our organization.
> >
> 
> That's how we build/run too, but with with hackery like this in a boot
> script to pick pieces out of the legacy world:
> 
>         mmc dev ${mmcdev}
>         mmc read ${loadaddr} 1300 100
>         env import -c ${loadaddr} 20000 ethaddr

FWIW, we have a cascaded load of boot scripts combined with
`env import -t` and/or `source`, which allows to control the boot media
by changing one line in a text file. This might be super ugly in terms of
vanilla standards, but it works in the development environment and
that's what matters the most.

-- 
Best regards,
Eugeniu.

[U-Boot] [PATCH v2 2/2] cmd: Add 'bcb' command to read/modify/write BCB fields

[Eugeniu Rosca]

Hi Sam,

On Mon, May 20, 2019 at 06:16:38PM +0300, Sam Protsenko wrote:
> Hi Eugeniu,
> 
> On Mon, May 20, 2019 at 10:23 AM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
[..]
> > Igor, Sam, what's your view on the above? Would you suggest creating
> > a doc/README.android-bcb or there is a more elegant solution to it?
> >
> 
> Documentation would be nice. Although you already provided a generic
> description of 'bcb' command in 'help bcb', the user often wants to
> read more high-level documentation. I'd say, you can copy the
> description from commit message to doc/README.android-bcb, extending
> it with usual use-cases, and how this command can be used in those
> use-cases. For example, your pseudo-code for reboot reason you
> provided to me earlier, etc.

Agreed. In my queue.

> Also, it can be useful to mention if
> Google have any requirements (mandatory or optional) for the
> bootloader (misc partition, reboot reason, recovery, etc), and how
> this 'bcb' command can help in those requirements implementation.

Well, a subset of the Android bootloader requirements is publicly
available via https://source.android.com/devices/bootloader, but I saw
traces of the "Android Boot/Bootloader Requirements" document name
mentioned in the descriptions of U-Boot commits received from our
suppliers. I _do not_ have access to the latter. If anybody from Google
sees this message and can share the document or a subset of it, that
would be great.

To be clear, the background behind the 'bcb' command is not because I
was assigned the task to implement any of the Android bootloader
requirements, but because I saw improper implementations of some of
those requirements in the patches received from our supplier and
wanted to showcase a better/U-Boot-compliant way to accomplish those.

So, I don't give any commitment to support the Android-related parts in
U-Boot, but will signal and express my opinion whenever any features
backported from AOSP don't follow the patterns established in community.

> 
> All that said, IMHO documentation/test wise: it's not critical in this
> case, you can add that later, just to speed-up the whole development
> process and divide it into iterations. But that's for maintainers to
> decide, of course.
> 
> Also, I've a feeling that we have another problem, more common than
> just a documentation. At the moment we have a bunch of Android related
> features, which don't have namespace separation on several levels:
>  - file/directories: we may want to move all Android related commands
> to sub-directory
>  - commands: we may want to add main command called "android" for all
> Android-related commands, or maybe just a prefix
>  - Kconfig: we may want to have some generic naming scheme for all
> Android-related commands
>  - Documentation, tests: the same here
> 
> So at some point we will probably need to discuss and fix that
> somehow. Not here, of course :)

Agree with the most of the bullets. However, WRT merging all the
available Android commands into a single command, I see room for
more discussion. Here is some valuable feedback from Ruslan Trofymenko:

 --- quote from https://patchwork.ozlabs.org/patch/1004002/#2049412 ---
On Thu, 6 Dec 2018 at 03:31, Simon Glass <sjg@chromium.org> wrote:
> Perhaps we need a new 'android' command with an 'ab_select'
> subcommand? Then the automatica abbreviation will work.
>

Agree with all your previous comments, will send v2 shortly. But this
one I'd like to leave as is (I will drop android_ prefix though). I
think adding "android" command may lead to unneeded architecture
complexity in future, e.g.:
  - we will need to re-work next commands as sub-commands of "android"
command: fastboot, dtimg, mmc_swrite (which can be hard as ab_select
command file has BSD license and other commands have GPL license)
 - we will need to implement sub-sub-commands (e.g. for "android dtimg
dump ..." etc.)
 - having "android" command can be inconvenient for users and may
break existing API (e.g. it will force users to use "android fastboot"
instead of just "fastboot" command)
 - actually I don't see any namespace issues that can be solved by
adding "android" command

Also, in subsequent patch, I will add the dedicated menu option for
Android-related commands and will pull all existing Android commands
(along with ab_select) to that menu entry.

So I hope it's fine with you if I leave this command as "ab_select"?
 --- end quote https://patchwork.ozlabs.org/patch/1004002/#2049412 ---

In addition, based on the feedback from Alex Kiernan, 'bcb' might be
useful in non-Android contexts. If so, then embedding it as sub-command
into a higher level command (e.g. 'android') does not make much sense.

-- 
Best Regards,
Eugeniu.

[U-Boot] [PATCH v2 2/2] cmd: Add 'bcb' command to read/modify/write BCB fields

[Simon Glass]

Hi Eugeniu,

On Mon, 20 May 2019 at 01:23, Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
>
> Hi Simon
> cc: Sam, Igor, feel free to correct/augment anything of below
>
> On Sat, May 18, 2019 at 10:33:02AM -0600, Simon Glass wrote:
> > Hi Eugeniu,
> >
> > On Fri, 17 May 2019 at 08:46, Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
> > >
> > > 'Bootloader Control Block' (BCB) is a well established term/acronym in
> > > the Android namespace which refers to a location in a dedicated raw
> > > (i.e. FS-unaware) flash (e.g. eMMC) partition, usually called "misc",
> > > which is used as media for exchanging messages between Android userspace
> > > (particularly recovery [1]) and an Android-capable bootloader.
> > >
> [..]
> >
> > Where is this documented? Perhaps it should go in README.avb2?
>
> README.avb2 is solely about the "verified/secure" booting of Android,
> while the 'bcb' command proposed in this patch can be useful both in
> verified boot scenarios (e.g. full-featured U-Boot builds), as well
> as in non-avb ones (e.g. development, PoC, demos, custom
> configurations). Thus, I think that README.avb2 is not the best
> place for 'bcb'.
>
> More generally, as somebody who uses git as an extension of himself, I
> am quite happy with commit-only documentation. OTOH, for those who
> receive U-Boot in tarballs or expect source-level docs/tutorials, I
> agree that having the 'bcb' described in a README might be helpful.
>
> I can identify two Android-dedicated README files, but none of them
> seems to be suitable for the new command:
>  - doc/README.android-fastboot
>  - doc/README.avb2
>
> Igor, Sam, what's your view on the above? Would you suggest creating
> a doc/README.android-bcb or there is a more elegant solution to it?

How about a new README.android which links to the other two and adds
your new info?

>
> >
> > Should it default to enabled if avb is used?
>
> I think at this specific moment in time, 'bcb' is orthogonal (meaning it
> is neither a direct, nor a reverse dependency) to any other Android
> feature in U-Boot. This could be re-assessed, if platform maintainers
> start to rely on 'bcb' in their U-Boot environments on regular basis.

OK. Also is there a sandbox driver for this? We should have a test.

Regards,
Simon

[U-Boot] [PATCH v2 2/2] cmd: Add 'bcb' command to read/modify/write BCB fields

[Sam Protsenko]

Hi Eugeniu,

On Tue, May 21, 2019 at 2:20 PM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
>
> Hi Sam,
>
> On Mon, May 20, 2019 at 06:16:38PM +0300, Sam Protsenko wrote:
> > Hi Eugeniu,
> >
> > On Mon, May 20, 2019 at 10:23 AM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
> [..]
> > > Igor, Sam, what's your view on the above? Would you suggest creating
> > > a doc/README.android-bcb or there is a more elegant solution to it?
> > >
> >
> > Documentation would be nice. Although you already provided a generic
> > description of 'bcb' command in 'help bcb', the user often wants to
> > read more high-level documentation. I'd say, you can copy the
> > description from commit message to doc/README.android-bcb, extending
> > it with usual use-cases, and how this command can be used in those
> > use-cases. For example, your pseudo-code for reboot reason you
> > provided to me earlier, etc.
>
> Agreed. In my queue.
>

Just to be clear: can we expect it to be sent in v3, or it will be
separate patch-set?

> > Also, it can be useful to mention if
> > Google have any requirements (mandatory or optional) for the
> > bootloader (misc partition, reboot reason, recovery, etc), and how
> > this 'bcb' command can help in those requirements implementation.
>
> Well, a subset of the Android bootloader requirements is publicly
> available via https://source.android.com/devices/bootloader, but I saw
> traces of the "Android Boot/Bootloader Requirements" document name
> mentioned in the descriptions of U-Boot commits received from our
> suppliers. I _do not_ have access to the latter. If anybody from Google
> sees this message and can share the document or a subset of it, that
> would be great.
>
> To be clear, the background behind the 'bcb' command is not because I
> was assigned the task to implement any of the Android bootloader
> requirements, but because I saw improper implementations of some of
> those requirements in the patches received from our supplier and
> wanted to showcase a better/U-Boot-compliant way to accomplish those.
>
> So, I don't give any commitment to support the Android-related parts in
> U-Boot, but will signal and express my opinion whenever any features
> backported from AOSP don't follow the patterns established in community.
>

Sure, I just meant that it could be nice to mention it in the
documentation, that 'bcb' command can help in those Google
requirements implementation.

> >
> > All that said, IMHO documentation/test wise: it's not critical in this
> > case, you can add that later, just to speed-up the whole development
> > process and divide it into iterations. But that's for maintainers to
> > decide, of course.
> >
> > Also, I've a feeling that we have another problem, more common than
> > just a documentation. At the moment we have a bunch of Android related
> > features, which don't have namespace separation on several levels:
> >  - file/directories: we may want to move all Android related commands
> > to sub-directory
> >  - commands: we may want to add main command called "android" for all
> > Android-related commands, or maybe just a prefix
> >  - Kconfig: we may want to have some generic naming scheme for all
> > Android-related commands
> >  - Documentation, tests: the same here
> >
> > So at some point we will probably need to discuss and fix that
> > somehow. Not here, of course :)
>
> Agree with the most of the bullets. However, WRT merging all the
> available Android commands into a single command, I see room for
> more discussion. Here is some valuable feedback from Ruslan Trofymenko:
>
>  --- quote from https://patchwork.ozlabs.org/patch/1004002/#2049412 ---
> On Thu, 6 Dec 2018 at 03:31, Simon Glass <sjg@chromium.org> wrote:
> > Perhaps we need a new 'android' command with an 'ab_select'
> > subcommand? Then the automatica abbreviation will work.
> >
>
> Agree with all your previous comments, will send v2 shortly. But this
> one I'd like to leave as is (I will drop android_ prefix though). I
> think adding "android" command may lead to unneeded architecture
> complexity in future, e.g.:
>   - we will need to re-work next commands as sub-commands of "android"
> command: fastboot, dtimg, mmc_swrite (which can be hard as ab_select
> command file has BSD license and other commands have GPL license)
>  - we will need to implement sub-sub-commands (e.g. for "android dtimg
> dump ..." etc.)
>  - having "android" command can be inconvenient for users and may
> break existing API (e.g. it will force users to use "android fastboot"
> instead of just "fastboot" command)
>  - actually I don't see any namespace issues that can be solved by
> adding "android" command
>
> Also, in subsequent patch, I will add the dedicated menu option for
> Android-related commands and will pull all existing Android commands
> (along with ab_select) to that menu entry.
>
> So I hope it's fine with you if I leave this command as "ab_select"?
>  --- end quote https://patchwork.ozlabs.org/patch/1004002/#2049412 ---
>
> In addition, based on the feedback from Alex Kiernan, 'bcb' might be
> useful in non-Android contexts. If so, then embedding it as sub-command
> into a higher level command (e.g. 'android') does not make much sense.
>

Yeah, let's keep it aside of this thread, I realize it's off-topic :)
We probably just need to make sure that all Android-related commands
are located in corresponding menu in menuconfig.

> --
> Best Regards,
> Eugeniu.

[U-Boot] [PATCH v2 2/2] cmd: Add 'bcb' command to read/modify/write BCB fields

[Eugeniu Rosca]

Hi Simon,

On Tue, May 21, 2019 at 10:43:04AM -0600, Simon Glass wrote:
> On Mon, 20 May 2019 at 01:23, Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
[..]
> > I can identify two Android-dedicated README files, but none of them
> > seems to be suitable for the new command:
> >  - doc/README.android-fastboot
> >  - doc/README.avb2
> >
> > Igor, Sam, what's your view on the above? Would you suggest creating
> > a doc/README.android-bcb or there is a more elegant solution to it?
> 
> How about a new README.android which links to the other two and adds
> your new info?

How about below directory structure:

 u-boot $ tree doc/android
 doc/android
 ├── avb2.txt
 ├── bcb.txt
 └── fastboot.txt

> 
> >
> > >
> > > Should it default to enabled if avb is used?
> >
> > I think at this specific moment in time, 'bcb' is orthogonal (meaning it
> > is neither a direct, nor a reverse dependency) to any other Android
> > feature in U-Boot. This could be re-assessed, if platform maintainers
> > start to rely on 'bcb' in their U-Boot environments on regular basis.
> 
> OK. Also is there a sandbox driver for this? We should have a test.

Emulating and exposing MMC devices in sandbox should be the only
prerequisite for sandbox testing and this seems to be already supported
via drivers/mmc/sandbox_mmc.c. However, to be honest, I was unsuccessful
bringing up the MMC devices on sandbox in the past. Particularly,
booting the latest sandbox U-Boot (CMD_MMC=y) I get:

=> mmc list
No MMC device available

I think there is something elementary which I am missing?

Regardless, I need some more days to implement the test and repartition
the README files. I think Sam would appreciate if you can provide your
Ack to the series as-is (it was extensively statically and dynamically
tested on R-Car H3ULCB) and I submit the doc/test updates separately.
Otherwise, I will push the next revision hopefully in a week or so.

> 
> Regards,
> Simon

-- 
Best Regards,
Eugeniu.

[U-Boot] [PATCH v2 2/2] cmd: Add 'bcb' command to read/modify/write BCB fields

[Simon Glass]

Hi Eugeniu,

On Tue, 21 May 2019 at 11:32, Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
>
> Hi Simon,
>
> On Tue, May 21, 2019 at 10:43:04AM -0600, Simon Glass wrote:
> > On Mon, 20 May 2019 at 01:23, Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
> [..]
> > > I can identify two Android-dedicated README files, but none of them
> > > seems to be suitable for the new command:
> > >  - doc/README.android-fastboot
> > >  - doc/README.avb2
> > >
> > > Igor, Sam, what's your view on the above? Would you suggest creating
> > > a doc/README.android-bcb or there is a more elegant solution to it?
> >
> > How about a new README.android which links to the other two and adds
> > your new info?
>
> How about below directory structure:
>
>  u-boot $ tree doc/android
>  doc/android
>  ├── avb2.txt
>  ├── bcb.txt
>  └── fastboot.txt
>

Sounds good

> >
> > >
> > > >
> > > > Should it default to enabled if avb is used?
> > >
> > > I think at this specific moment in time, 'bcb' is orthogonal (meaning it
> > > is neither a direct, nor a reverse dependency) to any other Android
> > > feature in U-Boot. This could be re-assessed, if platform maintainers
> > > start to rely on 'bcb' in their U-Boot environments on regular basis.
> >
> > OK. Also is there a sandbox driver for this? We should have a test.
>
> Emulating and exposing MMC devices in sandbox should be the only
> prerequisite for sandbox testing and this seems to be already supported
> via drivers/mmc/sandbox_mmc.c. However, to be honest, I was unsuccessful
> bringing up the MMC devices on sandbox in the past. Particularly,
> booting the latest sandbox U-Boot (CMD_MMC=y) I get:
>
> => mmc list
> No MMC device available
>
> I think there is something elementary which I am missing?

Probably need to copy an mmc node over from test.dts to sandbox.dts


>
> Regardless, I need some more days to implement the test and repartition
> the README files. I think Sam would appreciate if you can provide your
> Ack to the series as-is (it was extensively statically and dynamically
> tested on R-Car H3ULCB) and I submit the doc/test updates separately.
> Otherwise, I will push the next revision hopefully in a week or so.

That's OK with me, but let me review it first.

Regards,
Simon

[U-Boot] [PATCH v2 2/2] cmd: Add 'bcb' command to read/modify/write BCB fields

[Simon Glass]

Hi Eugeniu,

On Fri, 17 May 2019 at 08:46, Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
>
> 'Bootloader Control Block' (BCB) is a well established term/acronym in
> the Android namespace which refers to a location in a dedicated raw
> (i.e. FS-unaware) flash (e.g. eMMC) partition, usually called "misc",
> which is used as media for exchanging messages between Android userspace
> (particularly recovery [1]) and an Android-capable bootloader.
>
> On higher level, this allows implementing a subset of Android Bootloader
> Requirements [2], amongst which is the Android-specific bootloader
> flow [3]. Regardless how the latter is implemented in U-Boot ([3] being
> the most memorable example), reading/writing/dumping the BCB fields in
> the development process from inside the U-Boot is a convenient feature.
> Hence, make it available to the users.
>
> Some usage examples of the new command recorded on R-Car H3ULCB-KF
> ('>>>' is an overlay on top of the original console output):
>
> => bcb
> bcb - Load/set/clear/test/dump/store Android BCB fields
>
> Usage:
> bcb load  <dev> <part>       - load  BCB from mmc <dev>:<part>
> bcb set   <field> <val>      - set   BCB <field> to <val>
> bcb clear [<field>]          - clear BCB <field> or all fields
> bcb test  <field> <op> <val> - test  BCB <field> against <val>
> bcb dump  <field>            - dump  BCB <field>
> bcb store                    - store BCB back to mmc
>
> Legend:
> <dev>   - MMC device index containing the BCB partition
> <part>  - MMC partition index or name containing the BCB
> <field> - one of {command,status,recovery,stage,reserved}
> <op>    - the binary operator used in 'bcb test':
>           '=' returns true if <val> matches the string stored in <field>
>           '~' returns true if <val> matches a subset of <field>'s string
> <val>   - string/text provided as input to bcb {set,test}
>           NOTE: any ':' character in <val> will be replaced by line feed
>           during 'bcb set' and used as separator by upper layers
>
> => bcb dump command
> Error: BCB not loaded!
>  >>> Users must specify mmc device and partition before any other call
>
> => bcb load 1 misc
> => bcb load 1 1
>  >>> The two calls are equivalent (assuming "misc" has index 1)
>
> => bcb dump command
> 00000000: 62 6f 6f 74 6f 6e 63 65 2d 73 68 65 6c 6c 00 72    bootonce-shell.r
> 00000010: 79 00 72 00 00 00 00 00 00 00 00 00 00 00 00 00    y.r.............
>  >>> The output is in binary/string format for convenience
>  >>> The output size matches the size of inspected BCB field
>  >>> (32 bytes in case of 'command')
>
> => bcb test command = bootonce-shell && echo true
> true
> => bcb test command = bootonce-shell- && echo true
> => bcb test command = bootonce-shel && echo true
>  >>> The '=' operator returns 'true' on perfect match
>
> => bcb test command ~ bootonce-shel && echo true
> true
> => bcb test command ~ bootonce-shell && echo true
> true
>  >>> The '~' operator returns 'true' on substring match
>
> => bcb set command recovery
> => bcb dump command
> 00000000: 72 65 63 6f 76 65 72 79 00 73 68 65 6c 6c 00 72    recovery.shell.r
> 00000010: 79 00 72 00 00 00 00 00 00 00 00 00 00 00 00 00    y.r.............
>  >>> The new value is NULL-terminated and stored in the BCB field
>
> => bcb set recovery "msg1:msg2:msg3"
> => bcb dump recovery
> 00000040: 6d 73 67 31 0a 6d 73 67 32 0a 6d 73 67 33 00 00    msg1.msg2.msg3..
> 00000050: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
> 00000060: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
>  >>> --- snip ---
>  >>> Every ':' is replaced by line-feed '\n' (0xA). The latter is used
>  >>> as separator between individual commands by Android userspace
>
> => bcb store
>  >>> Flush/store the BCB structure to MMC
>
> [1] https://android.googlesource.com/platform/bootable/recovery
> [2] https://source.android.com/devices/bootloader
> [3] https://patchwork.ozlabs.org/patch/746835/
>     ("[U-Boot,5/6] Initial support for the Android Bootloader flow")

As discussed, please add these docs somewhere in the U-Boot tree (can
be in a separate patch)

>
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> ---
> v2:
>  - [Heinrich Schuchardt] Implement sub-commands via U_BOOT_CMD_MKENT.
>  - Polished the code. Ensured no warnings returned by sparse, smatch,
>    `cppcheck --force --enable=all --inconclusive`, make W=1.
>  - Tested on R-Car-H3-ES20 ULCB-KF.
>
> v1:
>  - https://patchwork.ozlabs.org/patch/1080395/
> ---
>  cmd/Kconfig  |  17 +++
>  cmd/Makefile |   1 +
>  cmd/bcb.c    | 330 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 348 insertions(+)
>  create mode 100644 cmd/bcb.c
>
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index 0d36da2a5c43..495bc1052f50 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -631,6 +631,23 @@ config CMD_ADC
>           Shows ADC device info and permit printing one-shot analog converted
>           data from a named Analog to Digital Converter.
>
> +config CMD_BCB
> +       bool "bcb"
> +       depends on MMC
> +       depends on PARTITIONS
> +       help
> +         Read/modify/write the fields of Bootloader Control Block, usually
> +         stored on the flash "misc" partition with its structure defined in:
> +         https://android.googlesource.com/platform/bootable/recovery/+/master/
> +         bootloader_message/include/bootloader_message/bootloader_message.h
> +
> +         Some real-life use-cases include (but are not limited to):
> +         - Determine the "boot reason" (and act accordingly):
> +           https://source.android.com/devices/bootloader/boot-reason
> +         - Get/pass a list of commands from/to recovery:
> +           https://android.googlesource.com/platform/bootable/recovery
> +         - Inspect/dump the contents of the BCB fields
> +
>  config CMD_BIND
>         bool "bind/unbind - Bind or unbind a device to/from a driver"
>         depends on DM
> diff --git a/cmd/Makefile b/cmd/Makefile
> index 7864fcf95c36..8f31b478eb1b 100644
> --- a/cmd/Makefile
> +++ b/cmd/Makefile
> @@ -16,6 +16,7 @@ obj-$(CONFIG_CMD_ADC) += adc.o
>  obj-$(CONFIG_CMD_ARMFLASH) += armflash.o
>  obj-y += blk_common.o
>  obj-$(CONFIG_CMD_SOURCE) += source.o
> +obj-$(CONFIG_CMD_BCB) += bcb.o
>  obj-$(CONFIG_CMD_BDI) += bdinfo.o
>  obj-$(CONFIG_CMD_BEDBUG) += bedbug.o
>  obj-$(CONFIG_CMD_BIND) += bind.o
> diff --git a/cmd/bcb.c b/cmd/bcb.c
> new file mode 100644
> index 000000000000..5d8486684542
> --- /dev/null
> +++ b/cmd/bcb.c
> @@ -0,0 +1,330 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2019 Eugeniu Rosca <rosca.eugeniu@gmail.com>
> + *
> + * Command to read/modify/write Android BCB fields
> + */
> +
> +#include <android_bootloader_message.h>
> +#include <command.h>
> +#include <common.h>
> +#include <malloc.h>
> +
> +enum bcb_cmd {
> +       BCB_CMD_LOAD,
> +       BCB_CMD_FIELD_SET,
> +       BCB_CMD_FIELD_CLEAR,
> +       BCB_CMD_FIELD_TEST,
> +       BCB_CMD_FIELD_DUMP,
> +       BCB_CMD_STORE,
> +};
> +
> +static int bcb_dev = -1;
> +static int bcb_part = -1;
> +static struct bootloader_message bcb = { { 0 } };
> +
> +static int bcb_cmd_get(char *cmd)
> +{
> +       if (!strncmp(cmd, "load", sizeof("load")))
> +               return BCB_CMD_LOAD;
> +       if (!strncmp(cmd, "set", sizeof("set")))
> +               return BCB_CMD_FIELD_SET;
> +       if (!strncmp(cmd, "clear", sizeof("clear")))
> +               return BCB_CMD_FIELD_CLEAR;
> +       if (!strncmp(cmd, "test", sizeof("test")))
> +               return BCB_CMD_FIELD_TEST;
> +       if (!strncmp(cmd, "store", sizeof("store")))
> +               return BCB_CMD_STORE;
> +       if (!strncmp(cmd, "dump", sizeof("dump")))
> +               return BCB_CMD_FIELD_DUMP;
> +       else
> +               return -1;
> +}
> +
> +static int do_bcb_load(cmd_tbl_t *cmdtp, int flag, int argc,
> +                      char * const argv[])
> +{
> +       struct blk_desc *desc;
> +       disk_partition_t info;
> +       u64 cnt;
> +       char *endp;
> +       int part;
> +
> +       if (argc != 3)
> +               return CMD_RET_USAGE;
> +
> +       if (blk_get_device_by_str("mmc", argv[1], &desc) < 0)

Error codes should be reported.

> +               goto err_1;
> +
> +       part = simple_strtoul(argv[2], &endp, 0);
> +       if (*endp == '\0') {
> +               if (part_get_info(desc, part, &info))
> +                       goto err_1;
> +       } else {
> +               part = part_get_info_by_name(desc, argv[2], &info);
> +               if (part < 0)
> +                       goto err_1;
> +       }
> +
> +       cnt = DIV_ROUND_UP(sizeof(struct bootloader_message), info.blksz);
> +       if (cnt > info.size)
> +               goto err_2;
> +
> +       if (blk_dread(desc, info.start, cnt, &bcb) != cnt)
> +               goto err_1;
> +
> +       bcb_dev = desc->devnum;
> +       bcb_part = part;
> +       debug("%s: Loaded from mmc %d:%d\n", __func__, bcb_dev, bcb_part);
> +
> +       return CMD_RET_SUCCESS;
> +err_1:
> +       printf("Error: Failed to read from mmc %s:%s\n", argv[1], argv[2]);

Add error code here.

> +       goto err;
> +err_2:
> +       printf("Error: mmc %s:%s too small!", argv[1], argv[2]);
> +       goto err;
> +err:
> +       bcb_dev = -1;
> +       bcb_part = -1;
> +       return CMD_RET_FAILURE;
> +}
> +
> +static int bcb_is_misused(int argc, char *const argv[])
> +{
> +       if (bcb_dev < 0 || bcb_part < 0) {
> +               printf("Error: BCB not loaded!\n");
> +               return -1;
> +       }
> +       switch (bcb_cmd_get(argv[0])) {

Why have a switch() here, when you could just put the below code in
each function? Or put the call to this function from the main
do_bcb_set() function.

> +       case BCB_CMD_LOAD:
> +               /* Dedicated arg handling */
> +               break;
> +       case BCB_CMD_FIELD_SET:
> +               if (argc != 3)
> +                       goto err;
> +               break;
> +       case BCB_CMD_FIELD_TEST:
> +               if (argc != 4)
> +                       goto err;
> +               break;
> +       case BCB_CMD_FIELD_CLEAR:
> +               if (argc != 1 && argc != 2)
> +                       goto err;
> +               break;
> +       case BCB_CMD_STORE:
> +               if (argc != 1)
> +                       goto err;
> +               break;
> +       case BCB_CMD_FIELD_DUMP:
> +               if (argc != 2)
> +                       goto err;
> +               break;
> +       default:
> +               debug("%s: Error: Unexpected BCB command\n", __func__);
> +               return -1;
> +       }
> +
> +       return 0;
> +err:
> +       printf("Error: Bad usage of 'bcb %s'\n", argv[0]);
> +       return -1;
> +}
> +
> +static int
> +bcb_field_get(char *name, char **field, int *size)
> +{
> +       if (!strncmp(name, "command", sizeof("command"))) {
> +               *field = bcb.command;
> +               *size = sizeof(bcb.command);
> +       } else if (!strncmp(name, "status", sizeof("status"))) {
> +               *field = bcb.status;
> +               *size = sizeof(bcb.status);
> +       } else if (!strncmp(name, "recovery", sizeof("recovery"))) {
> +               *field = bcb.recovery;
> +               *size = sizeof(bcb.recovery);
> +       } else if (!strncmp(name, "stage", sizeof("stage"))) {
> +               *field = bcb.stage;
> +               *size = sizeof(bcb.stage);
> +       } else if (!strncmp(name, "reserved", sizeof("reserved"))) {
> +               *field = bcb.reserved;
> +               *size = sizeof(bcb.reserved);
> +       } else {
> +               printf("Error: Unknown field '%s'\n", name);
> +               return -1;
> +       }
> +       return 0;
> +}
> +
> +static int do_bcb_set(cmd_tbl_t *cmdtp, int flag, int argc,
> +                     char * const argv[])
> +{
> +       int size, len;
> +       char *field, *str, *found, *keep;
> +
> +       if (bcb_is_misused(argc, argv))
> +               return CMD_RET_FAILURE;
> +
> +       if (bcb_field_get(argv[1], &field, &size))
> +               return CMD_RET_FAILURE;
> +
> +       len = strlen(argv[2]);
> +       if (len >= size) {
> +               printf("Error: sizeof('%s') = %d >= %d = sizeof(bcb.%s)\n",
> +                      argv[2], len, size, argv[1]);
> +               return CMD_RET_FAILURE;
> +       }
> +       str = strdup(argv[2]);

It is OK to change the args if you like.


> +       keep = str;
> +
> +       field[0] = '\0';
> +       while ((found = strsep(&str, ":"))) {
> +               if (field[0] != '\0')
> +                       strcat(field, "\n");
> +               strcat(field, found);
> +       }
> +
> +       free(keep);
> +       return CMD_RET_SUCCESS;
> +}
> +
> +static int do_bcb_clear(cmd_tbl_t *cmdtp, int flag, int argc,
> +                       char * const argv[])
> +{
> +       int size;
> +       char *field;
> +
> +       if (bcb_is_misused(argc, argv))
> +               return CMD_RET_FAILURE;
> +
> +       if (argc == 1) {
> +               memset(&bcb, 0, sizeof(bcb));
> +               return CMD_RET_SUCCESS;
> +       }
> +
> +       if (bcb_field_get(argv[1], &field, &size))
> +               return CMD_RET_FAILURE;
> +
> +       memset(field, 0, size);
> +       return CMD_RET_SUCCESS;
> +}
> +
> +static int do_bcb_dump(cmd_tbl_t *cmdtp, int flag, int argc,
> +                      char * const argv[])
> +{
> +       int size;
> +       char *field;
> +
> +       if (bcb_is_misused(argc, argv))
> +               return CMD_RET_FAILURE;

You should return CMD_RET_USAGE if there is a usage problem.

> +
> +       if (bcb_field_get(argv[1], &field, &size))
> +               return CMD_RET_FAILURE;
> +
> +       print_buffer((ulong)field - (ulong)&bcb, (void *)field, 1, size, 16);

Please add newline before return

> +       return CMD_RET_SUCCESS;
> +}
> +
> +static int do_bcb_test(cmd_tbl_t *cmdtp, int flag, int argc,
> +                      char * const argv[])
> +{
> +       int size;
> +       char *field;
> +
> +       if (bcb_is_misused(argc, argv))
> +               return CMD_RET_FAILURE;
> +
> +       if (bcb_field_get(argv[1], &field, &size))
> +               return CMD_RET_FAILURE;
> +
> +       if (!strncmp(argv[2], "=", sizeof("="))) {

Think about code size:

if (*argv[2] == '=')

> +               if (!strncmp(argv[3], field, size))
> +                       return CMD_RET_SUCCESS;
> +               else
> +                       return CMD_RET_FAILURE;
> +       } else if (!strncmp(argv[2], "~", sizeof("~"))) {
> +               if (!strstr(field, argv[3]))
> +                       return CMD_RET_FAILURE;
> +               else
> +                       return CMD_RET_SUCCESS;
> +       } else {
> +               printf("Error: Unknown operator '%s'\n", argv[2]);
> +       }
> +       return CMD_RET_FAILURE;
> +}
> +
> +static int do_bcb_store(cmd_tbl_t *cmdtp, int flag, int argc,
> +                       char * const argv[])
> +{
> +       struct blk_desc *desc;
> +       disk_partition_t info;
> +       u64 cnt;
> +
> +       if (bcb_is_misused(argc, argv))
> +               return CMD_RET_FAILURE;
> +
> +       desc = blk_get_devnum_by_type(IF_TYPE_MMC, bcb_dev);
> +       if (!desc)
> +               goto err;
> +
> +       if (part_get_info(desc, bcb_part, &info))
> +               goto err;

Again, error numbers need to be printed.

> +
> +       cnt = DIV_ROUND_UP(sizeof(struct bootloader_message), info.blksz);
> +
> +       if (blk_dwrite(desc, info.start, cnt, &bcb) != cnt)
> +               goto err;
> +
> +       return CMD_RET_SUCCESS;
> +err:
> +       printf("Error: Failed to write to mmc %d:%d\n", bcb_dev, bcb_part);
> +       return CMD_RET_FAILURE;
> +}
> +
> +static cmd_tbl_t cmd_bcb_sub[] = {
> +       U_BOOT_CMD_MKENT(load, CONFIG_SYS_MAXARGS, 1, do_bcb_load, "", ""),
> +       U_BOOT_CMD_MKENT(set, CONFIG_SYS_MAXARGS, 1, do_bcb_set, "", ""),
> +       U_BOOT_CMD_MKENT(clear, CONFIG_SYS_MAXARGS, 1, do_bcb_clear, "", ""),
> +       U_BOOT_CMD_MKENT(test, CONFIG_SYS_MAXARGS, 1, do_bcb_test, "", ""),
> +       U_BOOT_CMD_MKENT(dump, CONFIG_SYS_MAXARGS, 1, do_bcb_dump, "", ""),
> +       U_BOOT_CMD_MKENT(store, CONFIG_SYS_MAXARGS, 1, do_bcb_store, "", ""),
> +};
> +
> +static int do_bcb(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[])
> +{
> +       cmd_tbl_t *c;
> +
> +       if (argc < 2)
> +               return CMD_RET_USAGE;
> +
> +       argc--;
> +       argv++;
> +
> +       c = find_cmd_tbl(argv[0], cmd_bcb_sub, ARRAY_SIZE(cmd_bcb_sub));
> +       if (c)
> +               return c->cmd(cmdtp, flag, argc, argv);
> +
> +       return CMD_RET_USAGE;
> +}
> +
> +U_BOOT_CMD(
> +       bcb, CONFIG_SYS_MAXARGS, 1, do_bcb,
> +       "Load/set/clear/test/dump/store Android BCB fields",
> +       "load  <dev> <part>       - load  BCB from mmc <dev>:<part>\n"
> +       "bcb set   <field> <val>      - set   BCB <field> to <val>\n"
> +       "bcb clear [<field>]          - clear BCB <field> or all fields\n"
> +       "bcb test  <field> <op> <val> - test  BCB <field> against <val>\n"
> +       "bcb dump  <field>            - dump  BCB <field>\n"
> +       "bcb store                    - store BCB back to mmc\n"
> +       "\n"
> +       "Legend:\n"
> +       "<dev>   - MMC device index containing the BCB partition\n"
> +       "<part>  - MMC partition index or name containing the BCB\n"
> +       "<field> - one of {command,status,recovery,stage,reserved}\n"
> +       "<op>    - the binary operator used in 'bcb test':\n"
> +       "          '=' returns true if <val> matches the string stored in <field>\n"
> +       "          '~' returns true if <val> matches a subset of <field>'s string\n"
> +       "<val>   - string/text provided as input to bcb {set,test}\n"
> +       "          NOTE: any ':' character in <val> will be replaced by line feed\n"
> +       "          during 'bcb set' and used as separator by upper layers\n"
> +);
> --
> 2.21.0
>

Regards,
Simon

[U-Boot] [PATCH v2 2/2] cmd: Add 'bcb' command to read/modify/write BCB fields

[Eugeniu Rosca]

Hi Simon,

Thanks for the review. Would you please reply to my questions below?

On Tue, May 21, 2019 at 06:53:29PM -0600, Simon Glass wrote:
> Hi Eugeniu,
> 
> On Fri, 17 May 2019 at 08:46, Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
> >
> >
> > [1] https://android.googlesource.com/platform/bootable/recovery
> > [2] https://source.android.com/devices/bootloader
> > [3] https://patchwork.ozlabs.org/patch/746835/
> >     ("[U-Boot,5/6] Initial support for the Android Bootloader flow")
> 
> As discussed, please add these docs somewhere in the U-Boot tree (can
> be in a separate patch)

Sure.

> > +       if (blk_get_device_by_str("mmc", argv[1], &desc) < 0)
> 
> Error codes should be reported.

> > +       printf("Error: Failed to read from mmc %s:%s\n", argv[1], argv[2]);
> 
> Add error code here.

Will address.

> > +       switch (bcb_cmd_get(argv[0])) {
> 
> Why have a switch() here, when you could just put the below code in
> each function? Or put the call to this function from the main
> do_bcb_set() function.

s/do_bcb_set/do_bcb/ ?

The switch() is there to tell the user that he misused specifically
{sub}command 'bcb X'. If we just throw CMD_RET_USAGE (which means
printing full-blown help text) on _any_ kind of command misuse , we
don't help the user _at all_, IMHO. I would consider being user-friendly
as the higher and more important policy. However, if you prioritize the
code size over user experience, then I am open to rewrite the function.
Would you please clarify which policy takes precedence between the two?

> 
> > +       str = strdup(argv[2]);
> 
> It is OK to change the args if you like.

I will try getting rid of strdup.

> > +       if (bcb_is_misused(argc, argv))
> > +               return CMD_RET_FAILURE;
> 
> You should return CMD_RET_USAGE if there is a usage problem.

This again connects with my previous statement on the user-experience.
I would like to tell the user where exactly he did the mistake in using
the 'bcb' rather than throwing a full-blown help message.

> > +       if (bcb_field_get(argv[1], &field, &size))
> > +               return CMD_RET_FAILURE;
> > +
> > +       print_buffer((ulong)field - (ulong)&bcb, (void *)field, 1, size, 16);
> 
> Please add newline before return

Fine.

> > +       if (!strncmp(argv[2], "=", sizeof("="))) {
> 
> Think about code size:
> 
> if (*argv[2] == '=')

Checking a single character will not detect the difference between
'=' and '=anything' So, I have to validate, in addition, that the
NULL terminator is there. But I agree with the comment in general.

> > +       if (part_get_info(desc, bcb_part, &info))
> > +               goto err;
> 
> Again, error numbers need to be printed.

Will address.

> Regards,
> Simon

Thanks!

-- 
Best Regards,
Eugeniu.

[U-Boot] [PATCH v2 2/2] cmd: Add 'bcb' command to read/modify/write BCB fields

[Eugeniu Rosca]

Hi Sam,

On Tue, May 21, 2019 at 07:46:22PM +0300, Sam Protsenko wrote:
> On Tue, May 21, 2019 at 2:20 PM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
[..]
> > Agreed. In my queue.
> 
> Just to be clear: can we expect it to be sent in v3, or it will be
> separate patch-set?

We'll have a v3 for fixing Simon's review comments.
I will append the docs if I have time.

-- 
Best Regards,
Eugeniu.

[U-Boot] [PATCH v2 2/2] cmd: Add 'bcb' command to read/modify/write BCB fields

[Simon Glass]

Hi Eugeniu,

On Wed, 22 May 2019 at 01:11, Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
>
> Hi Simon,
>
> Thanks for the review. Would you please reply to my questions below?
>
> On Tue, May 21, 2019 at 06:53:29PM -0600, Simon Glass wrote:
> > Hi Eugeniu,
> >
> > On Fri, 17 May 2019 at 08:46, Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
> > >
> > >
> > > [1] https://android.googlesource.com/platform/bootable/recovery
> > > [2] https://source.android.com/devices/bootloader
> > > [3] https://patchwork.ozlabs.org/patch/746835/
> > >     ("[U-Boot,5/6] Initial support for the Android Bootloader flow")
> >
> > As discussed, please add these docs somewhere in the U-Boot tree (can
> > be in a separate patch)
>
> Sure.
>
> > > +       if (blk_get_device_by_str("mmc", argv[1], &desc) < 0)
> >
> > Error codes should be reported.
>
> > > +       printf("Error: Failed to read from mmc %s:%s\n", argv[1], argv[2]);
> >
> > Add error code here.
>
> Will address.
>
> > > +       switch (bcb_cmd_get(argv[0])) {
> >
> > Why have a switch() here, when you could just put the below code in
> > each function? Or put the call to this function from the main
> > do_bcb_set() function.
>
> s/do_bcb_set/do_bcb/ ?

Yes

>
> The switch() is there to tell the user that he misused specifically
> {sub}command 'bcb X'. If we just throw CMD_RET_USAGE (which means
> printing full-blown help text) on _any_ kind of command misuse , we
> don't help the user _at all_, IMHO. I would consider being user-friendly
> as the higher and more important policy. However, if you prioritize the
> code size over user experience, then I am open to rewrite the function.
> Would you please clarify which policy takes precedence between the two?

I think code size in commands is not the major priority, although we
do tend to try to keep it moderate.

Here I wasn't suggesting removing code. I was just suggesting that the
error handling could be in each specific command's function. So take
the code out of each case statement and put into the function that it
relates to.

Or continue to use the generic error handler, but call it from the
generic function. It seems like we have:

do_bcb() {
   switch (cmd) {
   case CMD_FRED
      do_bcb_fred();
     ...
   }
  ...
}

do_bcb_fred() {
   check_args(CMD_FRED);
...
}

check_args(int cmd) {
   switch (cmd) {
      case CMD_FRED:
        print error
      }
   }


I mean, put 'print error' inside do_bcb_fred()

or, call check_args() from do_bcb()

>
> >
> > > +       str = strdup(argv[2]);
> >
> > It is OK to change the args if you like.
>
> I will try getting rid of strdup.
>
> > > +       if (bcb_is_misused(argc, argv))
> > > +               return CMD_RET_FAILURE;
> >
> > You should return CMD_RET_USAGE if there is a usage problem.
>
> This again connects with my previous statement on the user-experience.
> I would like to tell the user where exactly he did the mistake in using
> the 'bcb' rather than throwing a full-blown help message.

Yes this is good. See above where I tried to explain what I mean.
>
> > > +       if (bcb_field_get(argv[1], &field, &size))
> > > +               return CMD_RET_FAILURE;
> > > +
> > > +       print_buffer((ulong)field - (ulong)&bcb, (void *)field, 1, size, 16);
> >
> > Please add newline before return
>
> Fine.
>
> > > +       if (!strncmp(argv[2], "=", sizeof("="))) {
> >
> > Think about code size:
> >
> > if (*argv[2] == '=')
>
> Checking a single character will not detect the difference between
> '=' and '=anything' So, I have to validate, in addition, that the
> NULL terminator is there. But I agree with the comment in general.

Yes, understood (I would calll this 'nul', BTW)

[..]

Regards,
Simon

[U-Boot] [PATCH v2 2/2] cmd: Add 'bcb' command to read/modify/write BCB fields

[Eugeniu Rosca]

Hello Simon,

I am really grateful for your review comments.

I think I tackled all of them in
https://patchwork.ozlabs.org/cover/1104242/
("[U-Boot,v3,0/3] Add 'bcb' command to read/modify/write Android BCB")

I would appreciate if you can have one more/final look.

-- 
Best Regards,
Eugeniu.

[U-Boot] [PATCH v2 0/2] Add 'bcb' command to read/modify/write Android BCB

[Eugeniu Rosca]

Superseded by https://patchwork.ozlabs.org/cover/1104242/
("[U-Boot,v3,0/3] Add 'bcb' command to read/modify/write Android BCB")

-- 
Best Regards,
Eugeniu.

[U-Boot] [PATCH v2 0/2] Add 'bcb' command to read/modify/write Android BCB

[Eugeniu Rosca]

Hello Stephen,

On Fri, May 17, 2019 at 05:28:25PM +0200, Eugeniu Rosca wrote:
> On Fri, May 17, 2019 at 04:26:23PM +0100, Stephen Finucane wrote:
> [..]
> > http://patchwork.ozlabs.org/patch/1099264/
> > 
> > We're just waiting for ozlabs to apply that patch.
> > 
> > Stephen
> 
> Thanks!

jFYI/FWIW, a recent patch series no longer suffers from this problem:
https://patchwork.ozlabs.org/cover/1104242/
("[U-Boot,v3,0/3] Add 'bcb' command to read/modify/write Android BCB")

However, the old series still exhibits the issue, as if the old series
is rendered by the old patchwork version:
https://patchwork.ozlabs.org/cover/1101106/
("[U-Boot,v2,0/2] Add 'bcb' command to read/modify/write Android BCB")

-- 
Best Regards,
Eugeniu.

[U-Boot] [PATCH v2 0/2] Add 'bcb' command to read/modify/write Android BCB

[Jeremy Kerr]

Hi Eugeniu,

> jFYI/FWIW, a recent patch series no longer suffers from this problem:
> https://patchwork.ozlabs.org/cover/1104242/
> ("[U-Boot,v3,0/3] Add 'bcb' command to read/modify/write Android BCB")

OK, super. I'd applied the fix for that issue on patchwork.ozlabs.org 
last week.

> However, the old series still exhibits the issue, as if the old series
> is rendered by the old patchwork version:
> https://patchwork.ozlabs.org/cover/1101106/
> ("[U-Boot,v2,0/2] Add 'bcb' command to read/modify/write Android BCB")

The issue was with the parser. Since the patch was parsed before the fix 
was applied, it'll remain like that.

Cheers,


Jeremy

[U-Boot] [PATCH v2 0/2] Add 'bcb' command to read/modify/write Android BCB

[Eugeniu Rosca]

Hi Jeremy,

On Sat, May 25, 2019 at 07:37:06AM +0800, Jeremy Kerr wrote:
> Hi Eugeniu,
> 
> >jFYI/FWIW, a recent patch series no longer suffers from this problem:
> >https://patchwork.ozlabs.org/cover/1104242/
> >("[U-Boot,v3,0/3] Add 'bcb' command to read/modify/write Android BCB")
> 
> OK, super. I'd applied the fix for that issue on patchwork.ozlabs.org last
> week.

Awesome. Thanks!

> >However, the old series still exhibits the issue, as if the old series
> >is rendered by the old patchwork version:
> >https://patchwork.ozlabs.org/cover/1101106/
> >("[U-Boot,v2,0/2] Add 'bcb' command to read/modify/write Android BCB")
> 
> The issue was with the parser. Since the patch was parsed before the fix was
> applied, it'll remain like that.

Good to know :)

-- 
Best regards,
Eugeniu.