Hi Sam,
On Tue, Feb 20, 2024 at 7:29 PM Sam Protsenko semen.protsenko@linaro.org wrote:
On Mon, Feb 19, 2024 at 4:16 AM Igor Opaniuk igor.opaniuk@gmail.com wrote:
This takes the latest changes from AOSP from [1][2] (as this header was split on two) with minimal changes (this could lead to warnings reported by checkpatch).
Do we want to maybe follow that and also carry two different headers in U-Boot? Or it doesn't make much sense? I'm thinking in terms of future portability mostly: how easy it's to update this header right now, and how easy it's going to be further. I didn't form any opinion on that, hence asking.
The problem is licensing. android_bootloader_message.h was re-licensed by Alex Deymo from Google under BSD-3-Clause, which is GPLv2 compatible. I'm not sure it's legally correct to pull boot_control_definition.h from AOSP licensed under Apache as a separate file.
Another thing: are you sure that changing only the header won't break anything in U-Boot .c files that use this header?
I've tested both ab_select and avb verify in QEMU. Or do you mean something else additionally should be tested?
Some local changes have been applied:
Is it possible to split this work into two patches:
- Bring the original changes only
- Apply all necessary changes for U-Boot
Or does it break the build, etc? Again, thinking in terms of portability easiness, and not sure which approach is better -- just asking basically.
Yeah, that's the problem, as splitting this on two commits will lead to the first one reporting warnings/notes.
- Enable static_assert() for defined structures to be sure
that all of them have correct sizes. 2. Adjuste types in bootloader_control structure with bitfields
Adjuste -> adjust
Will fix, thanks!
(uint8_t -> uint16_t). It seems that gcc just doesn't like bitfields
I wonder if all those extra changes can be upstreamed back to AOSP? Ideally we'd want to just copy those headers over from AOSP to U-Boot with no changes, would make the porting work easier. What are your thoughts on that?
Technically we can, I was planning to do that.
that cross the width of the type. Changing the type doesn't change the layout though. This addresses this gcc note: In file included from boot/android_ab.c:7: include/android_bootloader_message.h:230:1: note: offset of packed bit-field ‘merge_status’ has changed in GCC 4.4 230 | } __attribute__((packed));
[1] https://android.googlesource.com/platform/bootable/recovery/+/main/bootloade... [2] https://android.googlesource.com/platform/hardware/interfaces/+/main/boot/1....
CC: Alex Deymo deymo@google.com CC: Sam Protsenko semen.protsenko@linaro.org CC: Eugeniu Rosca erosca@de.adit-jv.com CC: Simon Glass sjg@chromium.org Signed-off-by: Igor Opaniuk igor.opaniuk@gmail.com
include/android_bootloader_message.h | 104 +++++++++++++++++++++++---- 1 file changed, 92 insertions(+), 12 deletions(-)
diff --git a/include/android_bootloader_message.h b/include/android_bootloader_message.h index 286d7ab0f31..75198fc9dc2 100644 --- a/include/android_bootloader_message.h +++ b/include/android_bootloader_message.h @@ -21,17 +21,22 @@
- stddef.h
*/ #include <compiler.h> +#include <linux/build_bug.h> #endif
// Spaces used by misc partition are as below: // 0 - 2K For bootloader_message // 2K - 16K Used by Vendor's bootloader (the 2K - 4K range may be optionally used // as bootloader_message_ab struct) -// 16K - 64K Used by uncrypt and recovery to store wipe_package for A/B devices +// 16K - 32K Used by uncrypt and recovery to store wipe_package for A/B devices +// 32K - 64K System space, used for miscellanious AOSP features. See below. // Note that these offsets are admitted by bootloader,recovery and uncrypt, so they // are not configurable without changing all of them. static const size_t BOOTLOADER_MESSAGE_OFFSET_IN_MISC = 0; +static const size_t VENDOR_SPACE_OFFSET_IN_MISC = 2 * 1024; static const size_t WIPE_PACKAGE_OFFSET_IN_MISC = 16 * 1024; +static const size_t SYSTEM_SPACE_OFFSET_IN_MISC = 32 * 1024; +static const size_t SYSTEM_SPACE_SIZE_IN_MISC = 32 * 1024;
/* Bootloader Message (2-KiB)
@@ -81,24 +86,67 @@ struct bootloader_message { char reserved[1184]; };
+// Holds Virtual A/B merge status information. Current version is 1. New fields +// must be added to the end. +struct misc_virtual_ab_message {
- uint8_t version;
- uint32_t magic;
- uint8_t merge_status; // IBootControl 1.1, MergeStatus enum.
- uint8_t source_slot; // Slot number when merge_status was written.
- uint8_t reserved[57];
+} __attribute__((packed));
+struct misc_memtag_message {
- uint8_t version;
- uint32_t magic; // magic string for treble compat
- uint32_t memtag_mode;
- uint8_t reserved[55];
+} __attribute__((packed));
+struct misc_kcmdline_message {
- uint8_t version;
- uint32_t magic;
- uint64_t kcmdline_flags;
- uint8_t reserved[51];
+} __attribute__((packed));
+#define MISC_VIRTUAL_AB_MESSAGE_VERSION 2 +#define MISC_VIRTUAL_AB_MAGIC_HEADER 0x56740AB0
+#define MISC_MEMTAG_MESSAGE_VERSION 1 +#define MISC_MEMTAG_MAGIC_HEADER 0x5afefe5a +#define MISC_MEMTAG_MODE_MEMTAG 0x1 +#define MISC_MEMTAG_MODE_MEMTAG_ONCE 0x2 +#define MISC_MEMTAG_MODE_MEMTAG_KERNEL 0x4 +#define MISC_MEMTAG_MODE_MEMTAG_KERNEL_ONCE 0x8 +#define MISC_MEMTAG_MODE_MEMTAG_OFF 0x10 +// This is set when the state was overridden forcibly. This does not need to be +// interpreted by the bootloader but is only for bookkeeping purposes so +// userspace knows what to do when the override is undone. +// See system/extras/mtectrl in AOSP for more information. +#define MISC_MEMTAG_MODE_FORCED 0x20
+#define MISC_KCMDLINE_MESSAGE_VERSION 1 +#define MISC_KCMDLINE_MAGIC_HEADER 0x6ab5110c +#define MISC_KCMDLINE_BINDER_RUST 0x1 /**
- We must be cautious when changing the bootloader_message struct size,
- because A/B-specific fields may end up with different offsets.
*/ -#ifndef __UBOOT__
#if (__STDC_VERSION__ >= 201112L) || defined(__cplusplus) static_assert(sizeof(struct bootloader_message) == 2048, "struct bootloader_message size changes, which may break A/B devices"); #endif -#endif /* __UBOOT__ */
/**
- The A/B-specific bootloader message structure (4-KiB).
- 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.
- stays after struct bootloader_message, which belongs to the vendor
- space of /misc partition. Also, the A/B-specific contents 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
@@ -124,12 +172,10 @@ struct bootloader_message_ab {
- Be cautious about the struct size change, in case we put anything post
- bootloader_message_ab struct (b/29159185).
*/ -#ifndef __UBOOT__ #if (__STDC_VERSION__ >= 201112L) || defined(__cplusplus) static_assert(sizeof(struct bootloader_message_ab) == 4096, "struct bootloader_message_ab size changes"); #endif -#endif /* __UBOOT__ */
#define BOOT_CTRL_MAGIC 0x42414342 /* Bootloader Control AB */ #define BOOT_CTRL_VERSION 1 @@ -165,11 +211,13 @@ struct bootloader_control { // Version of struct being used (see BOOT_CTRL_VERSION). uint8_t version; // Number of slots being managed.
- uint8_t nb_slot : 3;
- uint16_t nb_slot : 3; // Number of times left attempting to boot recovery.
- uint8_t recovery_tries_remaining : 3;
- uint16_t recovery_tries_remaining : 3;
- // Status of any pending snapshot merge of dynamic partitions.
- uint16_t merge_status : 3; // Ensure 4-bytes alignment for slot_info field.
- uint8_t reserved0[2];
- uint8_t reserved0[1]; // Per-slot information. Up to 4 slots. struct slot_metadata slot_info[4]; // Reserved for further use.
@@ -179,25 +227,46 @@ struct bootloader_control { 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"); +static_assert(sizeof(struct misc_virtual_ab_message) == 64,
"struct misc_virtual_ab_message has wrong size");+static_assert(sizeof(struct misc_memtag_message) == 64,
"struct misc_memtag_message has wrong size");+static_assert(sizeof(struct misc_kcmdline_message) == 64,
"struct misc_kcmdline_message has wrong size");#endif -#endif /* __UBOOT__ */
#ifndef __UBOOT__
+// This struct is not meant to be used directly, rather, it is to make +// computation of offsets easier. New fields must be added to the end. +struct misc_system_space_layout {
- misc_virtual_ab_message virtual_ab_message;
- misc_memtag_message memtag_message;
- misc_kcmdline_message kcmdline_message;
+} __attribute__((packed));
#ifdef __cplusplus
#include <string> #include <vector>
+// Gets the block device name of /misc partition. +std::string get_misc_blk_device(std::string* err);
// 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);
+// Writes |size| bytes of data from buffer |p| to |misc_blk_device| at |offset|. If the write fails, +// sets the error message in |err|. +bool write_misc_partition(const void* p, size_t size, const std::string& misc_blk_device,
size_t offset, 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);
@@ -242,6 +311,17 @@ 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);
+// Read or write the Virtual A/B message from system space in /misc. +bool ReadMiscVirtualAbMessage(misc_virtual_ab_message* message, std::string* err); +bool WriteMiscVirtualAbMessage(const misc_virtual_ab_message& message, std::string* err);
+// Read or write the memtag message from system space in /misc. +bool ReadMiscMemtagMessage(misc_memtag_message* message, std::string* err); +bool WriteMiscMemtagMessage(const misc_memtag_message& message, std::string* err);
+// Read or write the kcmdline message from system space in /misc. +bool ReadMiscKcmdlineMessage(misc_kcmdline_message* message, std::string* err); +bool WriteMiscKcmdlineMessage(const misc_kcmdline_message& message, std::string* err);
AFAIU, these new prototypes brought by this patch don't have actual implementation yet. Does it affect the build somehow, e.g. maybe causing some build warnings? Or is the build log clean?
They are all wrapped with #ifndef __UBOOT__.
#else
#include <stdbool.h>
2.34.1
Thanks for your comments!