Hi Sam, On Tue, Feb 20, 2024 at 7:29 PM Sam Protsenko <semen.protse...@linaro.org> wrote: > > On Mon, Feb 19, 2024 at 4:16 AM Igor Opaniuk <igor.opan...@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: > 1. Bring the original changes only > 2. 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. > > > 1. 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/bootloader_message/include/bootloader_message/bootloader_message.h > > [2] > > https://android.googlesource.com/platform/hardware/interfaces/+/main/boot/1.1/default/boot_control/include/private/boot_control_definition.h > > > > CC: Alex Deymo <de...@google.com> > > CC: Sam Protsenko <semen.protse...@linaro.org> > > CC: Eugeniu Rosca <ero...@de.adit-jv.com> > > CC: Simon Glass <s...@chromium.org> > > Signed-off-by: Igor Opaniuk <igor.opan...@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! -- Best regards - Freundliche Grüsse - Meilleures salutations Igor Opaniuk Senior Software Engineer, Embedded & Security E: igor.opan...@foundries.io W: www.foundries.io