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

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

[Eugeniu Rosca]

Hello U-Boot community,

The motivation behind adding the 'bcb' command is largely explained
in the second patch of this series. The other patch merely imports a
prerequisite header from AOSP.

This series (specifically the first patch) overlaps with [1], but it
is something agreed with Igor (see [2]).

Any comments would be appreciated.

Best regards,
Eugeniu.

[1] https://patchwork.ozlabs.org/cover/1044152/
    ("[U-Boot,v3,0/7] android: implement A/B boot process")
[2] https://patchwork.ozlabs.org/patch/1003998/#2145067

Eugeniu Rosca (2):
  include: android_bl_msg.h: Initial import
  cmd: Add 'bcb' command to read/modify/write BCB fields

 cmd/Kconfig              |  17 ++
 cmd/Makefile             |   1 +
 cmd/bcb.c                | 347 +++++++++++++++++++++++++++++++++++++++
 include/android_bl_msg.h | 273 ++++++++++++++++++++++++++++++
 4 files changed, 638 insertions(+)
 create mode 100644 cmd/bcb.c
 create mode 100644 include/android_bl_msg.h

-- 
2.21.0

[U-Boot] [PATCH 1/2] include: android_bl_msg.h: Initial import

[Eugeniu Rosca]

Import the bootloader_message.h (former bootloader.h) from AOSP.

Repository: https://android.googlesource.com/platform/bootable/recovery
Commit: 8e0821e6907d6016c19565b83319aff90ad1a034
Author: Tao Bao <tbao@google.com>
Date:   Wed Apr 3 23:48:21 2019 +0000

The bootloader_message.h basically defines the flash layout of a
dedicated partition (usually called 'misc') and is needed in U-Boot
in order to be able to implement a subset of Android Bootloader
Requirements [1], specifically dealing with:
 - Communication between the bootloader and recovery
 - Handling of A/B (Seamless) System Updates [2]

With respect to the in-tree vs out-of-tree file differences:
 - license matches https://patchwork.ozlabs.org/patch/1003998/
 - filename is changed to android_bl_msg.h, as per Simon's comment [3]
 - the struct/macro names have been shaped by [3-4], where the two main
   criterias are:
   - Improve the syntax/readability in the global U-Boot namespace
   - Minimize the future integration/update efforts from the source.
     Particularly, the __UBOOT__ macro helps with isolating the
     U-Boot-unrelated parts (e.g. includes/function prototypes/etc)

[1] https://source.android.com/devices/bootloader
[2] https://source.android.com/devices/tech/ota/ab/
[3] https://patchwork.ozlabs.org/patch/1003998/#2046141
[4] https://patchwork.ozlabs.org/patch/1003998/#2144955

Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---
 include/android_bl_msg.h | 273 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 273 insertions(+)
 create mode 100644 include/android_bl_msg.h

diff --git a/include/android_bl_msg.h b/include/android_bl_msg.h
new file mode 100644
index 000000000000..c48a1de2762b
--- /dev/null
+++ b/include/android_bl_msg.h
@@ -0,0 +1,273 @@
+// SPDX-License-Identifier: BSD-2-Clause
+/*
+ * This file was taken from the AOSP Project.
+ * Repository: https://android.googlesource.com/platform/bootable/recovery/
+ * File: bootloader_message/include/bootloader_message/bootloader_message.h
+ * Commit: see U-Boot commit importing/updating the file in-tree
+ *
+ * Copyright (C) 2008 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef _BOOTLOADER_MESSAGE_H
+#define _BOOTLOADER_MESSAGE_H
+
+#ifndef __UBOOT__
+#include <assert.h>
+#include <stddef.h>
+#include <stdint.h>
+#else
+#include <compiler.h>
+#include <linux/sizes.h>
+#endif
+
+#ifdef __UBOOT__
+/* U-Boot-specific types for improved syntax/readability */
+typedef struct bootloader_message	andr_bl_msg;
+typedef struct bootloader_message_ab	andr_bl_msg_ab;
+typedef struct bootloader_control	andr_bl_control;
+#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.
+
+#ifndef __UBOOT__
+static const size_t BOOTLOADER_MESSAGE_OFFSET_IN_MISC = 0;
+static const size_t WIPE_PACKAGE_OFFSET_IN_MISC = 16 * 1024;
+#else
+#define ANDROID_MISC_BM_OFFSET		0
+#define ANDROID_MISC_WIPE_OFFSET	SZ_16K
+#endif
+
+/* Bootloader Message (2-KiB)
+ *
+ * This structure describes the content of a block in flash
+ * that is used for recovery and the bootloader to talk to
+ * each other.
+ *
+ * The command field is updated by linux when it wants to
+ * reboot into recovery or to update radio or bootloader firmware.
+ * It is also updated by the bootloader when firmware update
+ * is complete (to boot into recovery for any final cleanup)
+ *
+ * The status field was used by the bootloader after the completion
+ * of an "update-radio" or "update-hboot" command, which has been
+ * deprecated since Froyo.
+ *
+ * The recovery field is only written by linux and used
+ * for the system to send a message to recovery or the
+ * other way around.
+ *
+ * The stage field is written by packages which restart themselves
+ * multiple times, so that the UI can reflect which invocation of the
+ * package it is.  If the value is of the format "#/#" (eg, "1/3"),
+ * the UI will add a simple indicator of that status.
+ *
+ * We used to have slot_suffix field for A/B boot control metadata in
+ * this struct, which gets unintentionally cleared by recovery or
+ * uncrypt. Move it into struct bootloader_message_ab to avoid the
+ * issue.
+ */
+struct bootloader_message {
+    char command[32];
+    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.
+    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.
+    char reserved[1184];
+};
+
+/**
+ * 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
+
+/**
+ * The A/B-specific bootloader message structure (4-KiB).
+ *
+ * We separate A/B boot control metadata from the regular bootloader
+ * message struct and keep it here. Everything that's A/B-specific
+ * stays after struct bootloader_message, which should be managed by
+ * the A/B-bootloader or boot control HAL.
+ *
+ * The slot_suffix field is used for A/B implementations where the
+ * bootloader does not set the androidboot.ro.boot.slot_suffix kernel
+ * commandline parameter. This is used by fs_mgr to mount /system and
+ * other partitions with the slotselect flag set in fstab. A/B
+ * implementations are free to use all 32 bytes and may store private
+ * data past the first NUL-byte in this field. It is encouraged, but
+ * not mandatory, to use 'struct bootloader_control' described below.
+ *
+ * The update_channel field is used to store the Omaha update channel
+ * if update_engine is compiled with Omaha support.
+ */
+struct bootloader_message_ab {
+    struct bootloader_message message;
+    char slot_suffix[32];
+    char update_channel[128];
+
+    // Round up the entire struct to 4096-byte.
+    char reserved[1888];
+};
+
+/**
+ * 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
+
+#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.
+    uint8_t priority : 4;
+    // Number of times left attempting to boot this slot.
+    uint8_t tries_remaining : 3;
+    // 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.
+    uint8_t verity_corrupted : 1;
+    // Reserved for further use.
+    uint8_t reserved : 7;
+} __attribute__((packed));
+
+/* Bootloader Control AB
+ *
+ * This struct can be used to manage A/B metadata. It is designed to
+ * be put in the 'slot_suffix' field of the 'bootloader_message'
+ * structure described above. It is encouraged to use the
+ * 'bootloader_control' structure to store the A/B metadata, but not
+ * mandatory.
+ */
+struct bootloader_control {
+    // NUL terminated active slot suffix.
+    char slot_suffix[4];
+    // Bootloader Control AB magic number (see BOOT_CTRL_MAGIC).
+    uint32_t magic;
+    // Version of struct being used (see BOOT_CTRL_VERSION).
+    uint8_t version;
+    // Number of slots being managed.
+    uint8_t nb_slot : 3;
+    // Number of times left attempting to boot recovery.
+    uint8_t recovery_tries_remaining : 3;
+    // Ensure 4-bytes alignment for slot_info field.
+    uint8_t reserved0[2];
+    // Per-slot information.  Up to 4 slots.
+    struct slot_metadata slot_info[4];
+    // Reserved for further use.
+    uint8_t reserved1[8];
+    // 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
+
+#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.
+std::string get_bootloader_message_blk_device(std::string* 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.
+bool read_bootloader_message_from(bootloader_message* boot, const std::string& misc_blk_device,
+                                  std::string* err);
+
+// Write bootloader message to BCB.
+bool write_bootloader_message(const bootloader_message& boot, std::string* err);
+
+// 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.
+bool write_bootloader_message(const std::vector<std::string>& options, std::string* err);
+
+// 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.
+bool update_bootloader_message_in_struct(bootloader_message* boot,
+                                         const std::vector<std::string>& options);
+
+// Clear BCB.
+bool clear_bootloader_message(std::string* err);
+
+// 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).
+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).
+bool write_wipe_package(const std::string& package_data, std::string* err);
+
+#else
+
+#include <stdbool.h>
+
+// C Interface.
+bool write_bootloader_message(const char* options);
+bool write_reboot_bootloader(void);
+
+#endif  // ifdef __cplusplus
+#endif
+
+#endif  // _BOOTLOADER_MESSAGE_H
-- 
2.21.0

[U-Boot] [PATCH 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 notorious 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>
---
 cmd/Kconfig  |  17 +++
 cmd/Makefile |   1 +
 cmd/bcb.c    | 347 +++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 365 insertions(+)
 create mode 100644 cmd/bcb.c

diff --git a/cmd/Kconfig b/cmd/Kconfig
index 0b07b3b9d777..2c32d9b85b51 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -621,6 +621,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 acb85f49fba8..b89d5187060b 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..3084f5033d27
--- /dev/null
+++ b/cmd/bcb.c
@@ -0,0 +1,347 @@
+// 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_bl_msg.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 andr_bl_msg bcb = { { 0 } };
+
+static int bcb_cmd_get(char *cmd)
+{
+	if (!strncmp(cmd, "load", sizeof("load")))
+		return BCB_CMD_LOAD;
+	if (!strncmp(cmd, "set", sizeof("set")))
+		return BCB_CMD_FIELD_SET;
+	if (!strncmp(cmd, "clear", sizeof("clear")))
+		return BCB_CMD_FIELD_CLEAR;
+	if (!strncmp(cmd, "test", sizeof("test")))
+		return BCB_CMD_FIELD_TEST;
+	if (!strncmp(cmd, "store", sizeof("store")))
+		return BCB_CMD_STORE;
+	if (!strncmp(cmd, "dump", sizeof("dump")))
+		return BCB_CMD_FIELD_DUMP;
+	else
+		return -1;
+}
+
+static int
+bcb_load(int argc, char *const argv[], andr_bl_msg *bcb,
+	 int *bcb_dev, int *bcb_part)
+{
+	struct blk_desc *desc;
+	disk_partition_t info;
+	u64 cnt;
+	char *endp;
+	int part;
+
+	if (argc != 3)
+		return CMD_RET_USAGE;
+
+	if (!bcb || !bcb_dev || !bcb_part)
+		return CMD_RET_FAILURE;
+
+	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(andr_bl_msg), 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("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 = *bcb_part = -1;
+	return CMD_RET_FAILURE;
+}
+
+static int
+bcb_is_misused(int argc, char *const argv[], andr_bl_msg *bcb,
+	       int bcb_dev, int bcb_part)
+{
+	if (bcb_dev < 0 || bcb_part < 0) {
+		printf("Error: BCB not loaded!\n");
+		return -1;
+	}
+	if (!bcb) {
+		debug("%s: Error: NULL BCB\n", __func__);
+		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, andr_bl_msg *bcb, 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
+bcb_field_set(int argc, char *const argv[], andr_bl_msg *bcb,
+	      int bcb_dev, int bcb_part)
+{
+	int size, len;
+	char *field, *str, *found, *keep;
+
+	if (bcb_is_misused(argc, argv, bcb, bcb_dev, bcb_part))
+		return CMD_RET_FAILURE;
+
+	if (bcb_field_get(argv[1], bcb, &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
+bcb_field_clear(int argc, char *const argv[], andr_bl_msg *bcb,
+		int bcb_dev, int bcb_part)
+{
+	int size;
+	char *field;
+
+	if (bcb_is_misused(argc, argv, bcb, bcb_dev, bcb_part))
+		return CMD_RET_FAILURE;
+
+	if (argc == 1) {
+		memset(bcb, 0, sizeof(*bcb));
+		return CMD_RET_SUCCESS;
+	}
+
+	if (bcb_field_get(argv[1], bcb, &field, &size))
+		return CMD_RET_FAILURE;
+
+	memset(field, 0, size);
+	return CMD_RET_SUCCESS;
+}
+
+static int
+bcb_field_dump(int argc, char *const argv[], andr_bl_msg *bcb,
+	       int bcb_dev, int bcb_part)
+{
+	int size;
+	char *field;
+
+	if (bcb_is_misused(argc, argv, bcb, bcb_dev, bcb_part))
+		return CMD_RET_FAILURE;
+
+	if (bcb_field_get(argv[1], bcb, &field, &size))
+		return CMD_RET_FAILURE;
+
+	print_buffer((ulong)field - (ulong)bcb, (void *)field, 1, size, 16);
+	return CMD_RET_SUCCESS;
+}
+
+static int
+bcb_field_test(int argc, char *const argv[], andr_bl_msg *bcb,
+	       int bcb_dev, int bcb_part)
+{
+	int size;
+	char *field;
+
+	if (bcb_is_misused(argc, argv, bcb, bcb_dev, bcb_part))
+		return CMD_RET_FAILURE;
+
+	if (bcb_field_get(argv[1], bcb, &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
+bcb_store(int argc, char *const argv[], andr_bl_msg *bcb,
+	  int bcb_dev, int bcb_part)
+{
+	struct blk_desc *desc;
+	disk_partition_t info;
+	u64 cnt;
+
+	if (bcb_is_misused(argc, argv, bcb, bcb_dev, bcb_part))
+		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(andr_bl_msg), info.blksz);
+
+	if (blk_dwrite(desc, info.start, cnt, bcb) != cnt)
+		goto err;
+
+	return CMD_RET_SUCCESS;
+err:
+	printf("Failed to write to mmc %d:%d\n", bcb_dev, bcb_part);
+	return CMD_RET_FAILURE;
+}
+
+static int do_bcb(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[])
+{
+	if (argc < 2)
+		return CMD_RET_USAGE;
+
+	argc--;
+	argv++;
+
+	switch (bcb_cmd_get(argv[0])) {
+	case BCB_CMD_LOAD:
+		return bcb_load(argc, argv, &bcb, &bcb_dev, &bcb_part);
+	case BCB_CMD_FIELD_SET:
+		return bcb_field_set(argc, argv, &bcb, bcb_dev, bcb_part);
+	case BCB_CMD_FIELD_CLEAR:
+		return bcb_field_clear(argc, argv, &bcb, bcb_dev, bcb_part);
+	case BCB_CMD_FIELD_TEST:
+		return bcb_field_test(argc, argv, &bcb, bcb_dev, bcb_part);
+	case BCB_CMD_FIELD_DUMP:
+		return bcb_field_dump(argc, argv, &bcb, bcb_dev, bcb_part);
+	case BCB_CMD_STORE:
+		return bcb_store(argc, argv, &bcb, bcb_dev, bcb_part);
+	default:
+		return CMD_RET_USAGE;
+	}
+
+	return CMD_RET_SUCCESS;
+}
+
+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 2/2] cmd: Add 'bcb' command to read/modify/write BCB fields

[Heinrich Schuchardt]

On 4/8/19 12:02 AM, Eugeniu Rosca wrote:
> 'Bootloader Control Block' (BCB) is a well established term/acronym in
> the Android namespace which refers to a location in a dedicated raw
> (i.e. FS-unaware) flash (e.g. eMMC) partition, usually called "misc",
> which is used as media for exchanging messages between Android userspace
> (particularly recovery [1]) and an Android-capable bootloader.
>
> On higher level, this allows implementing a subset of Android Bootloader
> Requirements [2], amongst which is the Android-specific bootloader
> flow [3]. Regardless how the latter is implemented in U-Boot ([3] being
> the most notorious 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>
> ---
>  cmd/Kconfig  |  17 +++
>  cmd/Makefile |   1 +
>  cmd/bcb.c    | 347 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 365 insertions(+)
>  create mode 100644 cmd/bcb.c
>
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index 0b07b3b9d777..2c32d9b85b51 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -621,6 +621,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 acb85f49fba8..b89d5187060b 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..3084f5033d27
> --- /dev/null
> +++ b/cmd/bcb.c
> @@ -0,0 +1,347 @@
> +// 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_bl_msg.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 andr_bl_msg bcb = { { 0 } };
> +
> +static int bcb_cmd_get(char *cmd)
> +{
> +	if (!strncmp(cmd, "load", sizeof("load")))
> +		return BCB_CMD_LOAD;
> +	if (!strncmp(cmd, "set", sizeof("set")))
> +		return BCB_CMD_FIELD_SET;
> +	if (!strncmp(cmd, "clear", sizeof("clear")))
> +		return BCB_CMD_FIELD_CLEAR;
> +	if (!strncmp(cmd, "test", sizeof("test")))
> +		return BCB_CMD_FIELD_TEST;
> +	if (!strncmp(cmd, "store", sizeof("store")))
> +		return BCB_CMD_STORE;
> +	if (!strncmp(cmd, "dump", sizeof("dump")))
> +		return BCB_CMD_FIELD_DUMP;
> +	else
> +		return -1;
> +}
> +
> +static int
> +bcb_load(int argc, char *const argv[], andr_bl_msg *bcb,
> +	 int *bcb_dev, int *bcb_part)
> +{
> +	struct blk_desc *desc;
> +	disk_partition_t info;
> +	u64 cnt;
> +	char *endp;
> +	int part;
> +
> +	if (argc != 3)
> +		return CMD_RET_USAGE;
> +
> +	if (!bcb || !bcb_dev || !bcb_part)
> +		return CMD_RET_FAILURE;
> +
> +	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(andr_bl_msg), 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("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 = *bcb_part = -1;
> +	return CMD_RET_FAILURE;
> +}
> +
> +static int
> +bcb_is_misused(int argc, char *const argv[], andr_bl_msg *bcb,
> +	       int bcb_dev, int bcb_part)
> +{
> +	if (bcb_dev < 0 || bcb_part < 0) {
> +		printf("Error: BCB not loaded!\n");
> +		return -1;
> +	}
> +	if (!bcb) {
> +		debug("%s: Error: NULL BCB\n", __func__);
> +		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, andr_bl_msg *bcb, 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
> +bcb_field_set(int argc, char *const argv[], andr_bl_msg *bcb,
> +	      int bcb_dev, int bcb_part)
> +{
> +	int size, len;
> +	char *field, *str, *found, *keep;
> +
> +	if (bcb_is_misused(argc, argv, bcb, bcb_dev, bcb_part))
> +		return CMD_RET_FAILURE;
> +
> +	if (bcb_field_get(argv[1], bcb, &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
> +bcb_field_clear(int argc, char *const argv[], andr_bl_msg *bcb,
> +		int bcb_dev, int bcb_part)
> +{
> +	int size;
> +	char *field;
> +
> +	if (bcb_is_misused(argc, argv, bcb, bcb_dev, bcb_part))
> +		return CMD_RET_FAILURE;
> +
> +	if (argc == 1) {
> +		memset(bcb, 0, sizeof(*bcb));
> +		return CMD_RET_SUCCESS;
> +	}
> +
> +	if (bcb_field_get(argv[1], bcb, &field, &size))
> +		return CMD_RET_FAILURE;
> +
> +	memset(field, 0, size);
> +	return CMD_RET_SUCCESS;
> +}
> +
> +static int
> +bcb_field_dump(int argc, char *const argv[], andr_bl_msg *bcb,
> +	       int bcb_dev, int bcb_part)
> +{
> +	int size;
> +	char *field;
> +
> +	if (bcb_is_misused(argc, argv, bcb, bcb_dev, bcb_part))
> +		return CMD_RET_FAILURE;
> +
> +	if (bcb_field_get(argv[1], bcb, &field, &size))
> +		return CMD_RET_FAILURE;
> +
> +	print_buffer((ulong)field - (ulong)bcb, (void *)field, 1, size, 16);
> +	return CMD_RET_SUCCESS;
> +}
> +
> +static int
> +bcb_field_test(int argc, char *const argv[], andr_bl_msg *bcb,
> +	       int bcb_dev, int bcb_part)
> +{
> +	int size;
> +	char *field;
> +
> +	if (bcb_is_misused(argc, argv, bcb, bcb_dev, bcb_part))
> +		return CMD_RET_FAILURE;
> +
> +	if (bcb_field_get(argv[1], bcb, &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
> +bcb_store(int argc, char *const argv[], andr_bl_msg *bcb,
> +	  int bcb_dev, int bcb_part)
> +{
> +	struct blk_desc *desc;
> +	disk_partition_t info;
> +	u64 cnt;
> +
> +	if (bcb_is_misused(argc, argv, bcb, bcb_dev, bcb_part))
> +		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(andr_bl_msg), info.blksz);
> +
> +	if (blk_dwrite(desc, info.start, cnt, bcb) != cnt)
> +		goto err;
> +
> +	return CMD_RET_SUCCESS;
> +err:
> +	printf("Failed to write to mmc %d:%d\n", bcb_dev, bcb_part);
> +	return CMD_RET_FAILURE;
> +}
> +
> +static int do_bcb(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[])
> +{
> +	if (argc < 2)
> +		return CMD_RET_USAGE;
> +
> +	argc--;
> +	argv++;
> +
> +	switch (bcb_cmd_get(argv[0])) {
> +	case BCB_CMD_LOAD:
> +		return bcb_load(argc, argv, &bcb, &bcb_dev, &bcb_part);
> +	case BCB_CMD_FIELD_SET:
> +		return bcb_field_set(argc, argv, &bcb, bcb_dev, bcb_part);
> +	case BCB_CMD_FIELD_CLEAR:
> +		return bcb_field_clear(argc, argv, &bcb, bcb_dev, bcb_part);
> +	case BCB_CMD_FIELD_TEST:
> +		return bcb_field_test(argc, argv, &bcb, bcb_dev, bcb_part);
> +	case BCB_CMD_FIELD_DUMP:
> +		return bcb_field_dump(argc, argv, &bcb, bcb_dev, bcb_part);
> +	case BCB_CMD_STORE:
> +		return bcb_store(argc, argv, &bcb, bcb_dev, bcb_part);
> +	default:
> +		return CMD_RET_USAGE;
> +	}

Please, implement the sub-commands as described in doc/README.commands
using the U_BOOT_CMD_MKENT macro.

Best regards

Heinrich

> +
> +	return CMD_RET_SUCCESS;> +}
> +
> +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"
> +);
>

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

[Eugeniu Rosca]

Hi Heinrich,

On Mon, Apr 08, 2019 at 12:11:59AM +0200, Heinrich Schuchardt wrote:
[..]
> > +	switch (bcb_cmd_get(argv[0])) {
> > +	case BCB_CMD_LOAD:
> > +		return bcb_load(argc, argv, &bcb, &bcb_dev, &bcb_part);
> > +	case BCB_CMD_FIELD_SET:
> > +		return bcb_field_set(argc, argv, &bcb, bcb_dev, bcb_part);
> > +	case BCB_CMD_FIELD_CLEAR:
> > +		return bcb_field_clear(argc, argv, &bcb, bcb_dev, bcb_part);
> > +	case BCB_CMD_FIELD_TEST:
> > +		return bcb_field_test(argc, argv, &bcb, bcb_dev, bcb_part);
> > +	case BCB_CMD_FIELD_DUMP:
> > +		return bcb_field_dump(argc, argv, &bcb, bcb_dev, bcb_part);
> > +	case BCB_CMD_STORE:
> > +		return bcb_store(argc, argv, &bcb, bcb_dev, bcb_part);
> > +	default:
> > +		return CMD_RET_USAGE;
> > +	}
> 
> Please, implement the sub-commands as described in doc/README.commands
> using the U_BOOT_CMD_MKENT macro.

Thanks for the lightning-fast reply. Will fix in v2, once hopefully
more review comments are collected.

> Best regards
> 
> Heinrich

Best regards,
Eugeniu.

[U-Boot] [PATCH 1/2] include: android_bl_msg.h: Initial import

[Sam Protsenko]

Hi Eugeniu,

Please see my comments below. Overall, we will need to think it over
and merge 3 patch series somehow (this one, the one for A/B support
from Ruslan, and one internal patch series Ruslan sent for reboot
reason support). I will provide more details a bit later, working on
this right now.

On Mon, Apr 8, 2019 at 1:02 AM Eugeniu Rosca <roscaeugeniu@gmail.com> wrote:
>
> Import the bootloader_message.h (former bootloader.h) from AOSP.
>
> Repository: https://android.googlesource.com/platform/bootable/recovery
> Commit: 8e0821e6907d6016c19565b83319aff90ad1a034
> Author: Tao Bao <tbao@google.com>
> Date:   Wed Apr 3 23:48:21 2019 +0000
>
> The bootloader_message.h basically defines the flash layout of a
> dedicated partition (usually called 'misc') and is needed in U-Boot
> in order to be able to implement a subset of Android Bootloader
> Requirements [1], specifically dealing with:
>  - Communication between the bootloader and recovery
>  - Handling of A/B (Seamless) System Updates [2]
>
> With respect to the in-tree vs out-of-tree file differences:
>  - license matches https://patchwork.ozlabs.org/patch/1003998/
>  - filename is changed to android_bl_msg.h, as per Simon's comment [3]
>  - the struct/macro names have been shaped by [3-4], where the two main
>    criterias are:
>    - Improve the syntax/readability in the global U-Boot namespace
>    - Minimize the future integration/update efforts from the source.
>      Particularly, the __UBOOT__ macro helps with isolating the
>      U-Boot-unrelated parts (e.g. includes/function prototypes/etc)
>
> [1] https://source.android.com/devices/bootloader
> [2] https://source.android.com/devices/tech/ota/ab/
> [3] https://patchwork.ozlabs.org/patch/1003998/#2046141
> [4] https://patchwork.ozlabs.org/patch/1003998/#2144955
>
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> ---
>  include/android_bl_msg.h | 273 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 273 insertions(+)
>  create mode 100644 include/android_bl_msg.h
>
> diff --git a/include/android_bl_msg.h b/include/android_bl_msg.h
> new file mode 100644
> index 000000000000..c48a1de2762b
> --- /dev/null
> +++ b/include/android_bl_msg.h
> @@ -0,0 +1,273 @@
> +// SPDX-License-Identifier: BSD-2-Clause
> +/*
> + * This file was taken from the AOSP Project.
> + * Repository: https://android.googlesource.com/platform/bootable/recovery/
> + * File: bootloader_message/include/bootloader_message/bootloader_message.h
> + * Commit: see U-Boot commit importing/updating the file in-tree
> + *
> + * Copyright (C) 2008 The Android Open Source Project
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at
> + *
> + *      http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#ifndef _BOOTLOADER_MESSAGE_H
> +#define _BOOTLOADER_MESSAGE_H
> +
> +#ifndef __UBOOT__
> +#include <assert.h>
> +#include <stddef.h>
> +#include <stdint.h>
> +#else
> +#include <compiler.h>
> +#include <linux/sizes.h>
> +#endif
> +
> +#ifdef __UBOOT__
> +/* U-Boot-specific types for improved syntax/readability */
> +typedef struct bootloader_message      andr_bl_msg;
> +typedef struct bootloader_message_ab   andr_bl_msg_ab;
> +typedef struct bootloader_control      andr_bl_control;

I would argue against creating user types for structures. It breaks
next rule from Linux kernel coding style (which we use in U-Boot):

    "It’s a mistake to use typedef for structures and pointers."

Which more important, it breaks that rule outside of this file. Also,
as you mentioned before, it's probably wise to keep this file as close
as possible to original, as we will need to update it at some point.

> +#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.
> +
> +#ifndef __UBOOT__

This stuff is very nice. I guess we can upstream it further to AOSP,
then this file will be almost the same in AOSP and in U-Boot.

> +static const size_t BOOTLOADER_MESSAGE_OFFSET_IN_MISC = 0;
> +static const size_t WIPE_PACKAGE_OFFSET_IN_MISC = 16 * 1024;
> +#else
> +#define ANDROID_MISC_BM_OFFSET         0
> +#define ANDROID_MISC_WIPE_OFFSET       SZ_16K
> +#endif
> +
> +/* Bootloader Message (2-KiB)
> + *
> + * This structure describes the content of a block in flash
> + * that is used for recovery and the bootloader to talk to
> + * each other.
> + *
> + * The command field is updated by linux when it wants to
> + * reboot into recovery or to update radio or bootloader firmware.
> + * It is also updated by the bootloader when firmware update
> + * is complete (to boot into recovery for any final cleanup)
> + *
> + * The status field was used by the bootloader after the completion
> + * of an "update-radio" or "update-hboot" command, which has been
> + * deprecated since Froyo.
> + *
> + * The recovery field is only written by linux and used
> + * for the system to send a message to recovery or the
> + * other way around.
> + *
> + * The stage field is written by packages which restart themselves
> + * multiple times, so that the UI can reflect which invocation of the
> + * package it is.  If the value is of the format "#/#" (eg, "1/3"),
> + * the UI will add a simple indicator of that status.
> + *
> + * We used to have slot_suffix field for A/B boot control metadata in
> + * this struct, which gets unintentionally cleared by recovery or
> + * uncrypt. Move it into struct bootloader_message_ab to avoid the
> + * issue.
> + */
> +struct bootloader_message {
> +    char command[32];
> +    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.
> +    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.
> +    char reserved[1184];
> +};
> +
> +/**
> + * 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
> +
> +/**
> + * The A/B-specific bootloader message structure (4-KiB).
> + *
> + * We separate A/B boot control metadata from the regular bootloader
> + * message struct and keep it here. Everything that's A/B-specific
> + * stays after struct bootloader_message, which should be managed by
> + * the A/B-bootloader or boot control HAL.
> + *
> + * The slot_suffix field is used for A/B implementations where the
> + * bootloader does not set the androidboot.ro.boot.slot_suffix kernel
> + * commandline parameter. This is used by fs_mgr to mount /system and
> + * other partitions with the slotselect flag set in fstab. A/B
> + * implementations are free to use all 32 bytes and may store private
> + * data past the first NUL-byte in this field. It is encouraged, but
> + * not mandatory, to use 'struct bootloader_control' described below.
> + *
> + * The update_channel field is used to store the Omaha update channel
> + * if update_engine is compiled with Omaha support.
> + */
> +struct bootloader_message_ab {
> +    struct bootloader_message message;
> +    char slot_suffix[32];
> +    char update_channel[128];
> +
> +    // Round up the entire struct to 4096-byte.
> +    char reserved[1888];
> +};
> +
> +/**
> + * 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
> +
> +#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.
> +    uint8_t priority : 4;
> +    // Number of times left attempting to boot this slot.
> +    uint8_t tries_remaining : 3;
> +    // 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.
> +    uint8_t verity_corrupted : 1;
> +    // Reserved for further use.
> +    uint8_t reserved : 7;
> +} __attribute__((packed));
> +
> +/* Bootloader Control AB
> + *
> + * This struct can be used to manage A/B metadata. It is designed to
> + * be put in the 'slot_suffix' field of the 'bootloader_message'
> + * structure described above. It is encouraged to use the
> + * 'bootloader_control' structure to store the A/B metadata, but not
> + * mandatory.
> + */
> +struct bootloader_control {
> +    // NUL terminated active slot suffix.
> +    char slot_suffix[4];
> +    // Bootloader Control AB magic number (see BOOT_CTRL_MAGIC).
> +    uint32_t magic;
> +    // Version of struct being used (see BOOT_CTRL_VERSION).
> +    uint8_t version;
> +    // Number of slots being managed.
> +    uint8_t nb_slot : 3;
> +    // Number of times left attempting to boot recovery.
> +    uint8_t recovery_tries_remaining : 3;
> +    // Ensure 4-bytes alignment for slot_info field.
> +    uint8_t reserved0[2];
> +    // Per-slot information.  Up to 4 slots.
> +    struct slot_metadata slot_info[4];
> +    // Reserved for further use.
> +    uint8_t reserved1[8];
> +    // 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
> +
> +#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.
> +std::string get_bootloader_message_blk_device(std::string* 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.
> +bool read_bootloader_message_from(bootloader_message* boot, const std::string& misc_blk_device,
> +                                  std::string* err);
> +
> +// Write bootloader message to BCB.
> +bool write_bootloader_message(const bootloader_message& boot, std::string* err);
> +
> +// 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.
> +bool write_bootloader_message(const std::vector<std::string>& options, std::string* err);
> +
> +// 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.
> +bool update_bootloader_message_in_struct(bootloader_message* boot,
> +                                         const std::vector<std::string>& options);
> +
> +// Clear BCB.
> +bool clear_bootloader_message(std::string* err);
> +
> +// 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).
> +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).
> +bool write_wipe_package(const std::string& package_data, std::string* err);
> +
> +#else
> +
> +#include <stdbool.h>
> +
> +// C Interface.
> +bool write_bootloader_message(const char* options);
> +bool write_reboot_bootloader(void);
> +
> +#endif  // ifdef __cplusplus
> +#endif
> +
> +#endif  // _BOOTLOADER_MESSAGE_H
> --
> 2.21.0
>

[U-Boot] [PATCH 1/2] include: android_bl_msg.h: Initial import

[Sam Protsenko]

Hi Tom,

Have a generic architecture related question regarding this code
(below, inline).

On Mon, Apr 8, 2019 at 1:02 AM Eugeniu Rosca <roscaeugeniu@gmail.com> wrote:
>
> Import the bootloader_message.h (former bootloader.h) from AOSP.
>
> Repository: https://android.googlesource.com/platform/bootable/recovery
> Commit: 8e0821e6907d6016c19565b83319aff90ad1a034
> Author: Tao Bao <tbao@google.com>
> Date:   Wed Apr 3 23:48:21 2019 +0000
>
> The bootloader_message.h basically defines the flash layout of a
> dedicated partition (usually called 'misc') and is needed in U-Boot
> in order to be able to implement a subset of Android Bootloader
> Requirements [1], specifically dealing with:
>  - Communication between the bootloader and recovery
>  - Handling of A/B (Seamless) System Updates [2]
>
> With respect to the in-tree vs out-of-tree file differences:
>  - license matches https://patchwork.ozlabs.org/patch/1003998/
>  - filename is changed to android_bl_msg.h, as per Simon's comment [3]
>  - the struct/macro names have been shaped by [3-4], where the two main
>    criterias are:
>    - Improve the syntax/readability in the global U-Boot namespace
>    - Minimize the future integration/update efforts from the source.
>      Particularly, the __UBOOT__ macro helps with isolating the
>      U-Boot-unrelated parts (e.g. includes/function prototypes/etc)
>
> [1] https://source.android.com/devices/bootloader
> [2] https://source.android.com/devices/tech/ota/ab/
> [3] https://patchwork.ozlabs.org/patch/1003998/#2046141
> [4] https://patchwork.ozlabs.org/patch/1003998/#2144955
>
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> ---
>  include/android_bl_msg.h | 273 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 273 insertions(+)
>  create mode 100644 include/android_bl_msg.h
>
> diff --git a/include/android_bl_msg.h b/include/android_bl_msg.h
> new file mode 100644
> index 000000000000..c48a1de2762b
> --- /dev/null
> +++ b/include/android_bl_msg.h
> @@ -0,0 +1,273 @@
> +// SPDX-License-Identifier: BSD-2-Clause
> +/*
> + * This file was taken from the AOSP Project.
> + * Repository: https://android.googlesource.com/platform/bootable/recovery/
> + * File: bootloader_message/include/bootloader_message/bootloader_message.h
> + * Commit: see U-Boot commit importing/updating the file in-tree
> + *
> + * Copyright (C) 2008 The Android Open Source Project
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at
> + *
> + *      http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#ifndef _BOOTLOADER_MESSAGE_H
> +#define _BOOTLOADER_MESSAGE_H
> +
> +#ifndef __UBOOT__
> +#include <assert.h>
> +#include <stddef.h>
> +#include <stdint.h>
> +#else
> +#include <compiler.h>
> +#include <linux/sizes.h>
> +#endif
> +
> +#ifdef __UBOOT__
> +/* U-Boot-specific types for improved syntax/readability */
> +typedef struct bootloader_message      andr_bl_msg;
> +typedef struct bootloader_message_ab   andr_bl_msg_ab;
> +typedef struct bootloader_control      andr_bl_control;

In files like this, which we copy from another projects, should we:
  a) keep file as close as possible to original, so that it's easy to
keep it in sync with upstream
  b) change structures names (like bootloader_message ->
andr_bootloader_message, slot_metadata -> andr_slot_metadata, etc), so
that we keep namespace clear?

Also, this file contains a lot of C++ related code, which is right now
discarded by #ifdef __UBOOT__. And it also not in kernel style, so
checkpatch is not happy. Should we keep it like this, or it's better
to remove all not needed code to keep this file clear, and fix coding
style?

Thanks.

> +#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.
> +
> +#ifndef __UBOOT__
> +static const size_t BOOTLOADER_MESSAGE_OFFSET_IN_MISC = 0;
> +static const size_t WIPE_PACKAGE_OFFSET_IN_MISC = 16 * 1024;
> +#else
> +#define ANDROID_MISC_BM_OFFSET         0
> +#define ANDROID_MISC_WIPE_OFFSET       SZ_16K
> +#endif
> +
> +/* Bootloader Message (2-KiB)
> + *
> + * This structure describes the content of a block in flash
> + * that is used for recovery and the bootloader to talk to
> + * each other.
> + *
> + * The command field is updated by linux when it wants to
> + * reboot into recovery or to update radio or bootloader firmware.
> + * It is also updated by the bootloader when firmware update
> + * is complete (to boot into recovery for any final cleanup)
> + *
> + * The status field was used by the bootloader after the completion
> + * of an "update-radio" or "update-hboot" command, which has been
> + * deprecated since Froyo.
> + *
> + * The recovery field is only written by linux and used
> + * for the system to send a message to recovery or the
> + * other way around.
> + *
> + * The stage field is written by packages which restart themselves
> + * multiple times, so that the UI can reflect which invocation of the
> + * package it is.  If the value is of the format "#/#" (eg, "1/3"),
> + * the UI will add a simple indicator of that status.
> + *
> + * We used to have slot_suffix field for A/B boot control metadata in
> + * this struct, which gets unintentionally cleared by recovery or
> + * uncrypt. Move it into struct bootloader_message_ab to avoid the
> + * issue.
> + */
> +struct bootloader_message {
> +    char command[32];
> +    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.
> +    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.
> +    char reserved[1184];
> +};
> +
> +/**
> + * 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
> +
> +/**
> + * The A/B-specific bootloader message structure (4-KiB).
> + *
> + * We separate A/B boot control metadata from the regular bootloader
> + * message struct and keep it here. Everything that's A/B-specific
> + * stays after struct bootloader_message, which should be managed by
> + * the A/B-bootloader or boot control HAL.
> + *
> + * The slot_suffix field is used for A/B implementations where the
> + * bootloader does not set the androidboot.ro.boot.slot_suffix kernel
> + * commandline parameter. This is used by fs_mgr to mount /system and
> + * other partitions with the slotselect flag set in fstab. A/B
> + * implementations are free to use all 32 bytes and may store private
> + * data past the first NUL-byte in this field. It is encouraged, but
> + * not mandatory, to use 'struct bootloader_control' described below.
> + *
> + * The update_channel field is used to store the Omaha update channel
> + * if update_engine is compiled with Omaha support.
> + */
> +struct bootloader_message_ab {
> +    struct bootloader_message message;
> +    char slot_suffix[32];
> +    char update_channel[128];
> +
> +    // Round up the entire struct to 4096-byte.
> +    char reserved[1888];
> +};
> +
> +/**
> + * 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
> +
> +#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.
> +    uint8_t priority : 4;
> +    // Number of times left attempting to boot this slot.
> +    uint8_t tries_remaining : 3;
> +    // 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.
> +    uint8_t verity_corrupted : 1;
> +    // Reserved for further use.
> +    uint8_t reserved : 7;
> +} __attribute__((packed));
> +
> +/* Bootloader Control AB
> + *
> + * This struct can be used to manage A/B metadata. It is designed to
> + * be put in the 'slot_suffix' field of the 'bootloader_message'
> + * structure described above. It is encouraged to use the
> + * 'bootloader_control' structure to store the A/B metadata, but not
> + * mandatory.
> + */
> +struct bootloader_control {
> +    // NUL terminated active slot suffix.
> +    char slot_suffix[4];
> +    // Bootloader Control AB magic number (see BOOT_CTRL_MAGIC).
> +    uint32_t magic;
> +    // Version of struct being used (see BOOT_CTRL_VERSION).
> +    uint8_t version;
> +    // Number of slots being managed.
> +    uint8_t nb_slot : 3;
> +    // Number of times left attempting to boot recovery.
> +    uint8_t recovery_tries_remaining : 3;
> +    // Ensure 4-bytes alignment for slot_info field.
> +    uint8_t reserved0[2];
> +    // Per-slot information.  Up to 4 slots.
> +    struct slot_metadata slot_info[4];
> +    // Reserved for further use.
> +    uint8_t reserved1[8];
> +    // 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
> +
> +#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.
> +std::string get_bootloader_message_blk_device(std::string* 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.
> +bool read_bootloader_message_from(bootloader_message* boot, const std::string& misc_blk_device,
> +                                  std::string* err);
> +
> +// Write bootloader message to BCB.
> +bool write_bootloader_message(const bootloader_message& boot, std::string* err);
> +
> +// 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.
> +bool write_bootloader_message(const std::vector<std::string>& options, std::string* err);
> +
> +// 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.
> +bool update_bootloader_message_in_struct(bootloader_message* boot,
> +                                         const std::vector<std::string>& options);
> +
> +// Clear BCB.
> +bool clear_bootloader_message(std::string* err);
> +
> +// 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).
> +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).
> +bool write_wipe_package(const std::string& package_data, std::string* err);
> +
> +#else
> +
> +#include <stdbool.h>
> +
> +// C Interface.
> +bool write_bootloader_message(const char* options);
> +bool write_reboot_bootloader(void);
> +
> +#endif  // ifdef __cplusplus
> +#endif
> +
> +#endif  // _BOOTLOADER_MESSAGE_H
> --
> 2.21.0
>

[U-Boot] [PATCH 1/2] include: android_bl_msg.h: Initial import

[Eugeniu Rosca]

Hi Sam,

On Wed, May 08, 2019 at 05:26:04PM +0300, Sam Protsenko wrote:
> Hi Eugeniu,
> 
> Please see my comments below. Overall, we will need to think it over
> and merge 3 patch series somehow (this one, the one for A/B support
> from Ruslan, and one internal patch series Ruslan sent for reboot
> reason support). I will provide more details a bit later, working on
> this right now.

Thanks for letting me know. I am willing to cooperate, if needed.
To my understanding, this patch is the only overlapping area, so I
think the idea is to agree on its contents/structure. Then, you could
include it in your series in the agreed shape. Once it is merged, I
will rebase my work on top of u-boot/master.

> On Mon, Apr 8, 2019 at 1:02 AM Eugeniu Rosca <roscaeugeniu@gmail.com> wrote:
[..]
> > +
> > +#ifdef __UBOOT__
> > +/* U-Boot-specific types for improved syntax/readability */
> > +typedef struct bootloader_message      andr_bl_msg;
> > +typedef struct bootloader_message_ab   andr_bl_msg_ab;
> > +typedef struct bootloader_control      andr_bl_control;
> 
> I would argue against creating user types for structures. It breaks
> next rule from Linux kernel coding style (which we use in U-Boot):
> 
>     "It’s a mistake to use typedef for structures and pointers."
> 
> Which more important, it breaks that rule outside of this file. Also,
> as you mentioned before, it's probably wise to keep this file as close
> as possible to original, as we will need to update it at some point.

I will wait for comments from Tom and will stick to whichever policy is
established when importing out-of-tree headers.

> 
> > +#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.
> > +
> > +#ifndef __UBOOT__
> 
> This stuff is very nice. I guess we can upstream it further to AOSP,
> then this file will be almost the same in AOSP and in U-Boot.

Sounds good to me.

-- 
Best Regards,
Eugeniu.

[U-Boot] [PATCH 1/2] include: android_bl_msg.h: Initial import

[Sam Protsenko]

Hi Eugeniu,

I created GitHub repo with all related patches applied on top of most
recent mainline U-Boot master: [1]. It contains next patches (on
"misc-reboot-reason" branch):

1. Series from Ruslan Trofymenko: [PATCH v3 0/7] android: implement
A/B boot process:
  * cmd: part: Add 'number' sub-command
  * disk: part: Extend API to get partition info
  * common: Implement A/B metadata
  * cmd: Add 'ab_select' command
  * test/py: Add base test case for A/B updates
  * doc: android: Add simple guide for A/B updates
  * env: am57xx: Implement A/B boot process
2. Series from Ruslan Trofymenko: [U-Boot][PATCH 0/4] Implement Reboot
reason support:
  * common: Implement Reboot reason flow
  * cmd: Add 'rb_reason' command
  * env: am57xx: Add Reboot reason support
  * configs: am57xx_evm: Enable Reboot reason support
  * Rename android_bl_msg.h -> android_bl_msg2.h (my patch to make
this series apply along with next patches from Eugeniu)
3. Series from Eugeniu Rosca: [PATCH 0/2] Add 'bcb' command to
read/modify/write Android BCB:
  * include: android_bl_msg.h: Initial import
  * cmd: Add 'bcb' command to read/modify/write BCB fields
4. My local patches to enable it all on X15 board, make it work
correctly, and use original android_bl_msg.h (as close as possible to
AOSP file):
  * configs: am57xx: Enable BCB command
  * environment: ti: Fix USB controller number
  * android: reboot reason: Use original android_bl_msg.h
  * android: ab: Use original android_bl_msg.h
  * android: Remove android_bl_msg2.h
  * android: bcb: Use original android_bl_msg.h API

With all those patches applied, I'm able to do next (after "adb reboot
bootloader" command):
1. Use "bcb" command by Eugeniu:
    => bcb load 1 4
    => bcb dump command
2. Use "rb_reason" command by Ruslan:
    => rb_reason mmc 1:4
    => rb_reason mmc 1:misc
3. U-Boot automatically gets into fastboot mode when "adb reboot
bootloader" command was issued

Now we need to figure out how to do next (w.r.t. repo [1]):
  1. How to merge your "bcb" command and Ruslan's "rb_reason" command;
I can see they both are working with "misc" BCB. So maybe it's good
idea to merge them into one command.
  2. How to handle android_bl_msg.h: it's a dependency for all 3 patch
series (A/B, "reboot reason" command, BCB command). Maybe we should
rework it and send as a separate patch, mentioning why it's needed,
and after it's applied, we can re-send our patch series without that
file included.

Please let me know what do you think.

Thanks!

[1] https://github.com/joe-skb7/u-boot-misc/commits/misc-reboot-reason

On Wed, May 8, 2019 at 5:46 PM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
>
> Hi Sam,
>
> On Wed, May 08, 2019 at 05:26:04PM +0300, Sam Protsenko wrote:
> > Hi Eugeniu,
> >
> > Please see my comments below. Overall, we will need to think it over
> > and merge 3 patch series somehow (this one, the one for A/B support
> > from Ruslan, and one internal patch series Ruslan sent for reboot
> > reason support). I will provide more details a bit later, working on
> > this right now.
>
> Thanks for letting me know. I am willing to cooperate, if needed.
> To my understanding, this patch is the only overlapping area, so I
> think the idea is to agree on its contents/structure. Then, you could
> include it in your series in the agreed shape. Once it is merged, I
> will rebase my work on top of u-boot/master.
>
> > On Mon, Apr 8, 2019 at 1:02 AM Eugeniu Rosca <roscaeugeniu@gmail.com> wrote:
> [..]
> > > +
> > > +#ifdef __UBOOT__
> > > +/* U-Boot-specific types for improved syntax/readability */
> > > +typedef struct bootloader_message      andr_bl_msg;
> > > +typedef struct bootloader_message_ab   andr_bl_msg_ab;
> > > +typedef struct bootloader_control      andr_bl_control;
> >
> > I would argue against creating user types for structures. It breaks
> > next rule from Linux kernel coding style (which we use in U-Boot):
> >
> >     "It’s a mistake to use typedef for structures and pointers."
> >
> > Which more important, it breaks that rule outside of this file. Also,
> > as you mentioned before, it's probably wise to keep this file as close
> > as possible to original, as we will need to update it at some point.
>
> I will wait for comments from Tom and will stick to whichever policy is
> established when importing out-of-tree headers.
>
> >
> > > +#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.
> > > +
> > > +#ifndef __UBOOT__
> >
> > This stuff is very nice. I guess we can upstream it further to AOSP,
> > then this file will be almost the same in AOSP and in U-Boot.
>
> Sounds good to me.
>
> --
> Best Regards,
> Eugeniu.

[U-Boot] [PATCH 1/2] include: android_bl_msg.h: Initial import

[Tom Rini]

On Wed, May 08, 2019 at 05:38:13PM +0300, Sam Protsenko wrote:
> Hi Tom,
> 
> Have a generic architecture related question regarding this code
> (below, inline).
> 
> On Mon, Apr 8, 2019 at 1:02 AM Eugeniu Rosca <roscaeugeniu@gmail.com> wrote:
> >
> > Import the bootloader_message.h (former bootloader.h) from AOSP.
> >
> > Repository: https://android.googlesource.com/platform/bootable/recovery
> > Commit: 8e0821e6907d6016c19565b83319aff90ad1a034
> > Author: Tao Bao <tbao@google.com>
> > Date:   Wed Apr 3 23:48:21 2019 +0000
> >
> > The bootloader_message.h basically defines the flash layout of a
> > dedicated partition (usually called 'misc') and is needed in U-Boot
> > in order to be able to implement a subset of Android Bootloader
> > Requirements [1], specifically dealing with:
> >  - Communication between the bootloader and recovery
> >  - Handling of A/B (Seamless) System Updates [2]
> >
> > With respect to the in-tree vs out-of-tree file differences:
> >  - license matches https://patchwork.ozlabs.org/patch/1003998/
> >  - filename is changed to android_bl_msg.h, as per Simon's comment [3]
> >  - the struct/macro names have been shaped by [3-4], where the two main
> >    criterias are:
> >    - Improve the syntax/readability in the global U-Boot namespace
> >    - Minimize the future integration/update efforts from the source.
> >      Particularly, the __UBOOT__ macro helps with isolating the
> >      U-Boot-unrelated parts (e.g. includes/function prototypes/etc)
> >
> > [1] https://source.android.com/devices/bootloader
> > [2] https://source.android.com/devices/tech/ota/ab/
> > [3] https://patchwork.ozlabs.org/patch/1003998/#2046141
> > [4] https://patchwork.ozlabs.org/patch/1003998/#2144955
> >
> > Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> > ---
> >  include/android_bl_msg.h | 273 +++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 273 insertions(+)
> >  create mode 100644 include/android_bl_msg.h
> >
> > diff --git a/include/android_bl_msg.h b/include/android_bl_msg.h
> > new file mode 100644
> > index 000000000000..c48a1de2762b
> > --- /dev/null
> > +++ b/include/android_bl_msg.h
> > @@ -0,0 +1,273 @@
> > +// SPDX-License-Identifier: BSD-2-Clause
> > +/*
> > + * This file was taken from the AOSP Project.
> > + * Repository: https://android.googlesource.com/platform/bootable/recovery/
> > + * File: bootloader_message/include/bootloader_message/bootloader_message.h
> > + * Commit: see U-Boot commit importing/updating the file in-tree

Please include the branch / hash this matches in the commit message at
least too.

> > + *
> > + * Copyright (C) 2008 The Android Open Source Project
> > + *
> > + * Licensed under the Apache License, Version 2.0 (the "License");
> > + * you may not use this file except in compliance with the License.
> > + * You may obtain a copy of the License at
> > + *
> > + *      http://www.apache.org/licenses/LICENSE-2.0
> > + *
> > + * Unless required by applicable law or agreed to in writing, software
> > + * distributed under the License is distributed on an "AS IS" BASIS,
> > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> > + * See the License for the specific language governing permissions and
> > + * limitations under the License.
> > + */
> > +
> > +#ifndef _BOOTLOADER_MESSAGE_H
> > +#define _BOOTLOADER_MESSAGE_H
> > +
> > +#ifndef __UBOOT__
> > +#include <assert.h>
> > +#include <stddef.h>
> > +#include <stdint.h>
> > +#else
> > +#include <compiler.h>
> > +#include <linux/sizes.h>
> > +#endif
> > +
> > +#ifdef __UBOOT__
> > +/* U-Boot-specific types for improved syntax/readability */
> > +typedef struct bootloader_message      andr_bl_msg;
> > +typedef struct bootloader_message_ab   andr_bl_msg_ab;
> > +typedef struct bootloader_control      andr_bl_control;
> 
> In files like this, which we copy from another projects, should we:
>   a) keep file as close as possible to original, so that it's easy to
> keep it in sync with upstream
>   b) change structures names (like bootloader_message ->
> andr_bootloader_message, slot_metadata -> andr_slot_metadata, etc), so
> that we keep namespace clear?

First, to be clear, I don't see adding typedefs as making the namespace
clear.  I can see the argument for making the code easier to read, but
typedefs make the namespace less, not more, clear.

> Also, this file contains a lot of C++ related code, which is right now
> discarded by #ifdef __UBOOT__. And it also not in kernel style, so
> checkpatch is not happy. Should we keep it like this, or it's better
> to remove all not needed code to keep this file clear, and fix coding
> style?

But otherwise, yes, I agree with option a, we want this to match up as
much as possible with the external authoritative file to make future
syncs easy and clear about what changed.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190508/359ffe60/attachment.sig>

[U-Boot] [PATCH 1/2] include: android_bl_msg.h: Initial import

[Eugeniu Rosca]

Hi Sam,

Thanks for the amazing effort to put the recent BCB/AB-related
advancements together and make them work. I really look forward to
seeing this part of mainline. Still, I have some concerns/questions
and hope to get your feedback.

First, the ("Implement Reboot reason support") series included in your
"misc-reboot-reason" branch looks to be brand new, i.e. it hasn't been
previously submitted for community review. I have comments in that area.

Second, some review comments of mine posted in various patches of
https://patchwork.ozlabs.org/cover/1044152/ ("[U-Boot,v3,0/7] android:
implement A/B boot process") still haven't been addressed. Who is going
to handle that work?

Third, yes, there is more overlap than I initially expected. It is
mostly between ("Add 'bcb' command to read/modify/write Android BCB")
and ("Implement Reboot reason support"). The resolution is not as
straightforward as one might assume. It is both a conflict of code
and a conflict of perspective on how Android bootloader flow
should be implemented in U-Boot. More on that later.

Fourth, some words on commit order and split. I think the most natural
commit order is the one reflecting the development of features in AOSP,
i.e. I would expect getting the reboot reason to come before the A/B
support, just b/c BCB structure with its "command" field existed in
AOSP for ages (since the inception of "recovery" repository in 2008)
while A/B support came _much_ (at least 7 years) later. WRT commit
split, one comment is that we would potentially like to import getting
reboot reason w/o importing A/B support. Surprisingly, this is not
possible if the bootloader message header is glued to commit
("common: Implement A/B metadata"). So, the best order to me is:
 - add android_bl_msg.h in a standalone commit
 - add bcb command
 - add getting reboot reason (if needed at all, more on it later)
 - add A/B support
 - update platform support

More comments below.

On Wed, May 08, 2019 at 08:25:15PM +0300, Sam Protsenko wrote:
> Hi Eugeniu,
> 
> I created GitHub repo with all related patches applied on top of most
> recent mainline U-Boot master: [1]. It contains next patches (on
> "misc-reboot-reason" branch):
> 
> 1. Series from Ruslan Trofymenko: [PATCH v3 0/7] android: implement
> A/B boot process:
>   * cmd: part: Add 'number' sub-command
>   * disk: part: Extend API to get partition info
>   * common: Implement A/B metadata
>   * cmd: Add 'ab_select' command
>   * test/py: Add base test case for A/B updates
>   * doc: android: Add simple guide for A/B updates
>   * env: am57xx: Implement A/B boot process
> 2. Series from Ruslan Trofymenko: [U-Boot][PATCH 0/4] Implement Reboot
> reason support:
>   * common: Implement Reboot reason flow
>   * cmd: Add 'rb_reason' command
>   * env: am57xx: Add Reboot reason support
>   * configs: am57xx_evm: Enable Reboot reason support
>   * Rename android_bl_msg.h -> android_bl_msg2.h (my patch to make
> this series apply along with next patches from Eugeniu)
> 3. Series from Eugeniu Rosca: [PATCH 0/2] Add 'bcb' command to
> read/modify/write Android BCB:
>   * include: android_bl_msg.h: Initial import
>   * cmd: Add 'bcb' command to read/modify/write BCB fields
> 4. My local patches to enable it all on X15 board, make it work
> correctly, and use original android_bl_msg.h (as close as possible to
> AOSP file):
>   * configs: am57xx: Enable BCB command
>   * environment: ti: Fix USB controller number
>   * android: reboot reason: Use original android_bl_msg.h
>   * android: ab: Use original android_bl_msg.h
>   * android: Remove android_bl_msg2.h
>   * android: bcb: Use original android_bl_msg.h API
> 
> With all those patches applied, I'm able to do next (after "adb reboot
> bootloader" command):
> 1. Use "bcb" command by Eugeniu:
>     => bcb load 1 4

"bcb load 1 misc" should be also supported, in case it helps.

>     => bcb dump command
> 2. Use "rb_reason" command by Ruslan:
>     => rb_reason mmc 1:4
>     => rb_reason mmc 1:misc

Well, here we have two different perspectives.
Below should be a 'bcb' equivalent (mostly pseudo code, not tested):

if bcb load 1 misc; then
    # Valid BCB found
    if bcb test command = bootonce-bootloader; then
        bcb clear command; bcb store;
        # do the equivalent of $fastbootcmd
    else if bcb test command = boot-recovery; then
        bcb clear command; bcb store;
        # do the equivalent of $recoverycmd
    else
        # boot Android OS normally
    fi
else
    # Corrupted/non-existent BCB
    # Boot non-Android OS?
fi

Here I see some room for discussion, since we have two approaches to
getting the reboot reason and act accordingly. I'll point out some
pros (+) and cons (-) in each case (IMHO):

rb_reason:
+ compact when used (one-liner)
- does much more than just reading the boot reason:
   - clears the 'command' field in BCB
   - runs $fastbootcmd/$recoverycmd, presumably populated beforehand
 => the above means that:
   - command name does not reflect its function, i.e. the name is
     misleading
   - U-Boot gets sprinkled by and its flow becomes dependent on a
     number of prerequisite environment variables (fastbootcmd/
     recoverycmd). Boot flow becomes more complex and harder to
     comprehend/follow/debug. It's dispersed across several components
     communicating via environment variables (not at all centralized)
- If we need to read any other BCB fields (status, recovery, stage) as
  part of Android boot flow management, we will need to either spawn
  new U-Boot commands or further obfuscate rb_reason.

In comparison, bcb:
- less compact when used (multi-line)
+ brings more transparency via sub-commands
+ frees the need for fastbootcmd/recoverycmd, i.e. centralizes
  Android boot flow management in the U-Boot environment. This is easier
  to read and debug.
+ can be used to take action based on the contents of other BCB fields

The above is my subjective view and I am open for different opinions.

> 3. U-Boot automatically gets into fastboot mode when "adb reboot
> bootloader" command was issued
> 
> Now we need to figure out how to do next (w.r.t. repo [1]):
>   1. How to merge your "bcb" command and Ruslan's "rb_reason" command;
> I can see they both are working with "misc" BCB. So maybe it's good
> idea to merge them into one command.
>   2. How to handle android_bl_msg.h: it's a dependency for all 3 patch
> series (A/B, "reboot reason" command, BCB command). Maybe we should
> rework it and send as a separate patch, mentioning why it's needed,
> and after it's applied, we can re-send our patch series without that
> file included.
> 
> Please let me know what do you think.

I guess I touched most of your comments.
I look forward for your feedback!

> 
> Thanks!

Likewise!

> 
> [1] https://github.com/joe-skb7/u-boot-misc/commits/misc-reboot-reason
> 
> > --
> > Best Regards,
> > Eugeniu.

[U-Boot] [PATCH 1/2] include: android_bl_msg.h: Initial import

[Sam Protsenko]

Hi Eugeniu,

On Thu, May 9, 2019 at 4:41 AM Eugeniu Rosca <roscaeugeniu@gmail.com> wrote:
>
> Hi Sam,
>
> Thanks for the amazing effort to put the recent BCB/AB-related
> advancements together and make them work. I really look forward to
> seeing this part of mainline. Still, I have some concerns/questions
> and hope to get your feedback.
>
> First, the ("Implement Reboot reason support") series included in your
> "misc-reboot-reason" branch looks to be brand new, i.e. it hasn't been
> previously submitted for community review. I have comments in that area.
>

It's just a "proof-of-concept" code. It was sent by engineer from my
team some time ago, to our internal mailing list. Of course, it's not
mean to be accepted as is, it's just a "food for thought". Now that I
checked both "bcb" and "rb_reason" implementation, I'm inclined to
think that it's better to add missing functionality to "bcb" command,
and drop "rb_reason".

> Second, some review comments of mine posted in various patches of
> https://patchwork.ozlabs.org/cover/1044152/ ("[U-Boot,v3,0/7] android:
> implement A/B boot process") still haven't been addressed. Who is going
> to handle that work?
>

It will be either Igor Opaniuk or me.

> Third, yes, there is more overlap than I initially expected. It is
> mostly between ("Add 'bcb' command to read/modify/write Android BCB")
> and ("Implement Reboot reason support"). The resolution is not as
> straightforward as one might assume. It is both a conflict of code
> and a conflict of perspective on how Android bootloader flow
> should be implemented in U-Boot. More on that later.
>

Correct. After reading all the code I came to the same conclusion.

> Fourth, some words on commit order and split. I think the most natural
> commit order is the one reflecting the development of features in AOSP,
> i.e. I would expect getting the reboot reason to come before the A/B
> support, just b/c BCB structure with its "command" field existed in
> AOSP for ages (since the inception of "recovery" repository in 2008)
> while A/B support came _much_ (at least 7 years) later. WRT commit
> split, one comment is that we would potentially like to import getting
> reboot reason w/o importing A/B support. Surprisingly, this is not
> possible if the bootloader message header is glued to commit
> ("common: Implement A/B metadata"). So, the best order to me is:
>  - add android_bl_msg.h in a standalone commit
>  - add bcb command
>  - add getting reboot reason (if needed at all, more on it later)
>  - add A/B support
>  - update platform support
>

Agreed on suggested order.

> More comments below.
>
> On Wed, May 08, 2019 at 08:25:15PM +0300, Sam Protsenko wrote:
> > Hi Eugeniu,
> >
> > I created GitHub repo with all related patches applied on top of most
> > recent mainline U-Boot master: [1]. It contains next patches (on
> > "misc-reboot-reason" branch):
> >
> > 1. Series from Ruslan Trofymenko: [PATCH v3 0/7] android: implement
> > A/B boot process:
> >   * cmd: part: Add 'number' sub-command
> >   * disk: part: Extend API to get partition info
> >   * common: Implement A/B metadata
> >   * cmd: Add 'ab_select' command
> >   * test/py: Add base test case for A/B updates
> >   * doc: android: Add simple guide for A/B updates
> >   * env: am57xx: Implement A/B boot process
> > 2. Series from Ruslan Trofymenko: [U-Boot][PATCH 0/4] Implement Reboot
> > reason support:
> >   * common: Implement Reboot reason flow
> >   * cmd: Add 'rb_reason' command
> >   * env: am57xx: Add Reboot reason support
> >   * configs: am57xx_evm: Enable Reboot reason support
> >   * Rename android_bl_msg.h -> android_bl_msg2.h (my patch to make
> > this series apply along with next patches from Eugeniu)
> > 3. Series from Eugeniu Rosca: [PATCH 0/2] Add 'bcb' command to
> > read/modify/write Android BCB:
> >   * include: android_bl_msg.h: Initial import
> >   * cmd: Add 'bcb' command to read/modify/write BCB fields
> > 4. My local patches to enable it all on X15 board, make it work
> > correctly, and use original android_bl_msg.h (as close as possible to
> > AOSP file):
> >   * configs: am57xx: Enable BCB command
> >   * environment: ti: Fix USB controller number
> >   * android: reboot reason: Use original android_bl_msg.h
> >   * android: ab: Use original android_bl_msg.h
> >   * android: Remove android_bl_msg2.h
> >   * android: bcb: Use original android_bl_msg.h API
> >
> > With all those patches applied, I'm able to do next (after "adb reboot
> > bootloader" command):
> > 1. Use "bcb" command by Eugeniu:
> >     => bcb load 1 4
>
> "bcb load 1 misc" should be also supported, in case it helps.
>

Yes. Can you please send v2 for "bcb" command, accounting for next:
  1. I've already sent android_bl_msg.h as a separate patch, no need
to re-send it
  2. Fix pending comments on "bcb" patch review
  3. Add referencing partitions by names (not only by number)
    3.1 It should be reflected in "help bcb" as well

Or, optionally, (3) can be done in subsequent patch, in order to
accelerate the process. Let's try and do that ASAP so we can unblock
Igor w.r.t. A/B patches. Also, I don't have much time left on this
task...

Once "bcb" patch is merged

> >     => bcb dump command
> > 2. Use "rb_reason" command by Ruslan:
> >     => rb_reason mmc 1:4
> >     => rb_reason mmc 1:misc
>
> Well, here we have two different perspectives.
> Below should be a 'bcb' equivalent (mostly pseudo code, not tested):
>
> if bcb load 1 misc; then
>     # Valid BCB found
>     if bcb test command = bootonce-bootloader; then
>         bcb clear command; bcb store;
>         # do the equivalent of $fastbootcmd
>     else if bcb test command = boot-recovery; then
>         bcb clear command; bcb store;
>         # do the equivalent of $recoverycmd
>     else
>         # boot Android OS normally
>     fi
> else
>     # Corrupted/non-existent BCB
>     # Boot non-Android OS?
> fi
>
> Here I see some room for discussion, since we have two approaches to
> getting the reboot reason and act accordingly. I'll point out some
> pros (+) and cons (-) in each case (IMHO):
>
> rb_reason:
> + compact when used (one-liner)
> - does much more than just reading the boot reason:
>    - clears the 'command' field in BCB
>    - runs $fastbootcmd/$recoverycmd, presumably populated beforehand
>  => the above means that:
>    - command name does not reflect its function, i.e. the name is
>      misleading
>    - U-Boot gets sprinkled by and its flow becomes dependent on a
>      number of prerequisite environment variables (fastbootcmd/
>      recoverycmd). Boot flow becomes more complex and harder to
>      comprehend/follow/debug. It's dispersed across several components
>      communicating via environment variables (not at all centralized)
> - If we need to read any other BCB fields (status, recovery, stage) as
>   part of Android boot flow management, we will need to either spawn
>   new U-Boot commands or further obfuscate rb_reason.
>
> In comparison, bcb:
> - less compact when used (multi-line)
> + brings more transparency via sub-commands
> + frees the need for fastbootcmd/recoverycmd, i.e. centralizes
>   Android boot flow management in the U-Boot environment. This is easier
>   to read and debug.
> + can be used to take action based on the contents of other BCB fields
>
> The above is my subjective view and I am open for different opinions.
>

I like "bcb" command more. Let's go ahead and merge it (as I mentioned
above). Then we can think how we can use/extend it for the further
implementation of "reboot reason" feature (probably the way you
mentioned in your pseudo-code section).

> > 3. U-Boot automatically gets into fastboot mode when "adb reboot
> > bootloader" command was issued
> >
> > Now we need to figure out how to do next (w.r.t. repo [1]):
> >   1. How to merge your "bcb" command and Ruslan's "rb_reason" command;
> > I can see they both are working with "misc" BCB. So maybe it's good
> > idea to merge them into one command.
> >   2. How to handle android_bl_msg.h: it's a dependency for all 3 patch
> > series (A/B, "reboot reason" command, BCB command). Maybe we should
> > rework it and send as a separate patch, mentioning why it's needed,
> > and after it's applied, we can re-send our patch series without that
> > file included.
> >
> > Please let me know what do you think.
>
> I guess I touched most of your comments.
> I look forward for your feedback!
>
> >
> > Thanks!
>
> Likewise!
>
> >
> > [1] https://github.com/joe-skb7/u-boot-misc/commits/misc-reboot-reason
> >
> > > --
> > > Best Regards,
> > > Eugeniu.

[U-Boot] [PATCH 1/2] include: android_bl_msg.h: Initial import

[Eugeniu Rosca]

Hello Sam,

On Fri, May 10, 2019 at 05:23:26PM +0300, Sam Protsenko wrote:
> On Thu, May 9, 2019 at 4:41 AM Eugeniu Rosca <roscaeugeniu@gmail.com> wrote:
[..]
> > "bcb load 1 misc" should be also supported, in case it helps.
> 
> Yes. Can you please send v2 for "bcb" command, accounting for next:
>   1. I've already sent android_bl_msg.h as a separate patch, no need
> to re-send it

I'll provide my review comments to that.

>   2. Fix pending comments on "bcb" patch review

Agreed.

>   3. Add referencing partitions by names (not only by number)
>     3.1 It should be reflected in "help bcb" as well

Sorry for not making myself properly understood. This is already
supported, i.e. 'bcb load 1 4' and 'bcb load 1 misc' in your example
should lead to the same result.

> 
> Or, optionally, (3) can be done in subsequent patch, in order to
> accelerate the process. Let's try and do that ASAP so we can unblock
> Igor w.r.t. A/B patches. Also, I don't have much time left on this
> task...

No worries. I'll assist with that.

> > >
> > > > --
> > > > Best Regards,
> > > > Eugeniu.

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

[Eugeniu Rosca]

Superseded by https://patchwork.ozlabs.org/cover/1101106/
("[PATCH v2 0/2] Add 'bcb' command to read/modify/write Android BCB")

-- 
Best Regards,
Eugeniu.