Message ID | 20190407220206.7655-2-erosca@de.adit-jv.com |
---|---|
State | Changes Requested |
Delegated to: | Heinrich Schuchardt |
Headers | show |
Series | Add 'bcb' command to read/modify/write Android BCB | expand |
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 >
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 >
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.
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.
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.
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.
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.
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.
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
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