On Wed, Jul 31, 2024 at 08:38:49AM -0600, Simon Glass wrote: > Hi Mattijs, > > On Tue, 30 Jul 2024 at 02:19, Mattijs Korpershoek > <mkorpersh...@baylibre.com> wrote: > > > > Hi Dmitry, > > > > Thank you for the patch. > > > > Hi Simon, > > > > On dim., juil. 28, 2024 at 13:36, Simon Glass <s...@chromium.org> wrote: > > > > > Hi Dmitry, > > > > > > On Thu, 25 Jul 2024 at 14:55, Dmitry Rokosov > > > <ddroko...@salutedevices.com> wrote: > > >> > > >> It's really helpful to have the ability to dump BCB block for debugging > > >> A/B logic on the board supported this partition schema. > > >> > > >> Command 'ab_dump' prints all fields of bootloader_control struct > > >> including slot_metadata for all presented slots. > > >> > > >> Output example: > > >> ===== > > >> > board# ab_dump ubi 0#misc > > >> > Read 512 bytes from volume misc to 000000000bf51900 > > >> > Bootloader Control: [misc] > > >> > Active Slot: _a > > >> > Magic Number: 0x42414342 > > >> > Version: 1 > > >> > Number of Slots: 2 > > >> > Recovery Tries Remaining: 7 > > >> > CRC: 0x61378F6F (Valid) > > >> > > > >> > Slot[0] Metadata: > > >> > - Priority: 15 > > >> > - Tries Remaining: 4 > > >> > - Successful Boot: 0 > > >> > - Verity Corrupted: 0 > > >> > > > >> > Slot[1] Metadata: > > >> > - Priority: 15 > > >> > - Tries Remaining: 5 > > >> > - Successful Boot: 0 > > >> > - Verity Corrupted: 0 > > >> ==== > > >> > > >> Signed-off-by: Dmitry Rokosov <ddroko...@salutedevices.com> > > >> --- > > >> boot/android_ab.c | 68 ++++++++++++++++++++++++++++++++++++++++++++ > > >> cmd/ab_select.c | 30 +++++++++++++++++++ > > >> include/android_ab.h | 9 ++++++ > > >> 3 files changed, 107 insertions(+) > > >> > > >> diff --git a/boot/android_ab.c b/boot/android_ab.c > > >> index 1e5aa81b7503..359cc1a00428 100644 > > >> --- a/boot/android_ab.c > > >> +++ b/boot/android_ab.c > > >> @@ -363,3 +363,71 @@ int ab_select_slot(struct blk_desc *dev_desc, > > >> struct disk_partition *part_info, > > >> > > >> return slot; > > >> } > > >> + > > >> +int ab_dump_abc(struct blk_desc *dev_desc, struct disk_partition > > >> *part_info) > > >> +{ > > >> + struct bootloader_control *abc; > > >> + u32 crc32_le; > > >> + int i, ret; > > >> + struct slot_metadata *slot; > > >> + > > >> + if (!dev_desc || !part_info) { > > >> + log_err("ANDROID: Empty device descriptor or partition > > >> info\n"); > > >> + return -EINVAL; > > >> + } > > >> + > > >> + ret = ab_control_create_from_disk(dev_desc, part_info, &abc, 0); > > >> + if (ret < 0) { > > >> + log_err("ANDROID: Cannot create bcb from disk %d\n", > > >> ret); > > >> + return ret; > > >> + } > > >> + > > >> + if (abc->magic != BOOT_CTRL_MAGIC) { > > >> + log_err("ANDROID: Unknown A/B metadata: %.8x\n", > > >> abc->magic); > > >> + ret = -ENODATA; > > >> + goto error; > > >> + } > > >> + > > >> + if (abc->version > BOOT_CTRL_VERSION) { > > >> + log_err("ANDROID: Unsupported A/B metadata version: > > >> %.8x\n", > > >> + abc->version); > > >> + ret = -ENODATA; > > >> + goto error; > > >> + } > > >> + > > >> + if (abc->nb_slot > ARRAY_SIZE(abc->slot_info)) { > > >> + log_err("ANDROID: Wrong number of slots %u, expected > > >> %zu\n", > > >> + abc->nb_slot, ARRAY_SIZE(abc->slot_info)); > > >> + ret = -ENODATA; > > >> + goto error; > > >> + } > > >> + > > >> + printf("Bootloader Control: \t[%s]\n", part_info->name); > > >> + printf("Active Slot: \t\t%s\n", abc->slot_suffix); > > >> + printf("Magic Number: \t\t0x%X\n", abc->magic); > > >> + printf("Version: \t\t%u\n", abc->version); > > >> + printf("Number of Slots: \t%u\n", abc->nb_slot); > > >> + printf("Recovery Tries Remaining: %u\n", > > >> abc->recovery_tries_remaining); > > > > In the console, this rendered not perfectly aligned, which is a bit of a > > shame: > > > > (done on sandbox) > > > > => ab_dump mmc 7#misc > > Bootloader Control: [misc] > > Active Slot: _a > > Magic Number: 0x42414342 > > Version: 1 > > Number of Slots: 2 > > Recovery Tries Remaining: 0 > > CRC: 0x321FEF27 (Valid) > > > > >> + > > >> + printf("CRC: \t\t\t0x%.8X", abc->crc32_le); > > >> + > > >> + crc32_le = ab_control_compute_crc(abc); > > >> + if (abc->crc32_le != crc32_le) > > >> + printf(" (Invalid, Expected: \t0x%.8X)\n", crc32_le); > > >> + else > > >> + printf(" (Valid)\n"); > > >> + > > >> + for (i = 0; i < abc->nb_slot; ++i) { > > >> + slot = &abc->slot_info[i]; > > >> + printf("\nSlot[%d] Metadata:\n", i); > > >> + printf("\t- Priority: \t\t%u\n", slot->priority); > > >> + printf("\t- Tries Remaining: \t%u\n", > > >> slot->tries_remaining); > > >> + printf("\t- Successful Boot: \t%u\n", > > >> slot->successful_boot); > > >> + printf("\t- Verity Corrupted: \t%u\n", > > >> slot->verity_corrupted); > > >> + } > > >> + > > >> +error: > > >> + free(abc); > > >> + > > >> + return ret; > > >> +} > > >> diff --git a/cmd/ab_select.c b/cmd/ab_select.c > > >> index 9e2f74573c22..1d34150ceea9 100644 > > >> --- a/cmd/ab_select.c > > >> +++ b/cmd/ab_select.c > > >> @@ -51,6 +51,31 @@ static int do_ab_select(struct cmd_tbl *cmdtp, int > > >> flag, int argc, > > >> return CMD_RET_SUCCESS; > > >> } > > >> > > >> +static int do_ab_dump(struct cmd_tbl *cmdtp, int flag, int argc, > > >> + char *const argv[]) > > >> +{ > > >> + int ret; > > >> + struct blk_desc *dev_desc; > > >> + struct disk_partition part_info; > > >> + > > >> + if (argc < 3) > > >> + return CMD_RET_USAGE; > > >> + > > >> + if (part_get_info_by_dev_and_name_or_num(argv[1], argv[2], > > >> + &dev_desc, &part_info, > > >> + false) < 0) { > > >> + return CMD_RET_FAILURE; > > >> + } > > >> + > > >> + ret = ab_dump_abc(dev_desc, &part_info); > > >> + if (ret < 0) { > > >> + printf("Cannot dump ABC data, error %d.\n", ret); > > >> + return CMD_RET_FAILURE; > > >> + } > > >> + > > >> + return CMD_RET_SUCCESS; > > >> +} > > >> + > > >> U_BOOT_CMD(ab_select, 5, 0, do_ab_select, > > >> "Select the slot used to boot from and register the boot > > >> attempt.", > > >> "<slot_var_name> <interface> <dev[:part|#part_name]> > > >> [--no-dec]\n" > > >> @@ -66,3 +91,8 @@ U_BOOT_CMD(ab_select, 5, 0, do_ab_select, > > >> " - If '--no-dec' is set, the number of tries remaining > > >> will not\n" > > >> " decremented for the selected boot slot\n" > > >> ); > > >> + > > >> +U_BOOT_CMD(ab_dump, 3, 0, do_ab_dump, > > >> + "Dump boot_control information from specific partition.", > > >> + "<interface> <dev[:part|#part_name]>\n" > > >> +); > > >> diff --git a/include/android_ab.h b/include/android_ab.h > > >> index 1fee7582b90a..e53bf7eb6a02 100644 > > >> --- a/include/android_ab.h > > >> +++ b/include/android_ab.h > > >> @@ -33,4 +33,13 @@ struct disk_partition; > > >> int ab_select_slot(struct blk_desc *dev_desc, struct disk_partition > > >> *part_info, > > >> bool dec_tries); > > >> > > >> +/** > > >> + * Dump ABC information for specific partition. > > >> + * > > >> + * @param[in] dev_desc Device description pointer > > >> + * @param[in] part_info Partition information > > > > > > We have moved to the @ notation now: > > > > > > @dev_desc: ... > > > > I agree with this comment, but the file uses @param[in] already. We > > should to a preparatory patch to convert this file to the new notation. > > OK, can do later. >
I can provide a separate patch in the next patch series to rework the files to the new notation. > > > > > > > >> + * Return: 0 on success, or a negative on error > > >> + */ > > >> +int ab_dump_abc(struct blk_desc *dev_desc, struct disk_partition > > >> *part_info); > > >> + > > >> #endif /* __ANDROID_AB_H */ > > >> -- > > >> 2.43.0 > > >> > > > > > > Rather than creating a new command I think this should be a subcommand > > > of abootimg. > > > > To me, they are not the same thing. > > > > - ab_* commands are for manipulating specific bits from the BCB (Boot > > Control Block, usually "misc" partition) > > ab_* operates on partitions > > > > - abootimg is for manipulating boot.img and vendor_boot.img headers > > (which are not on the same partitions) > > abootimg operations on memory regions (so someone else is responsible > > for reading the partitions) > > > > We also have a 3rd command "bcb". "bcb" also reads the "misc" partition > > but can only read the "boot reason". > > If we really want to merge ab_select and ab_dump into another command, > > "bcb" is more relevant, in my opinion. > > That sounds good. > >From my perspective, the 'bcb' command seems like a suitable choice here. It already has the necessary subcommands to work with the BCB block. Since the commands 'ab_select' and the new 'ab_dump' are also BCB operations, I believe it would be beneficial to merge them. I will work on preparing the next version with the merging of the 'bcb' command. > > > > I'd prefer to keep 3 commands for the following reasons: > > > > 1. Easier to track/port changes from Google's fork [1] > > 2. Better separation of responsabilities > > 3. Merging the commands requires the update of the existing U-Boot > > environment users (meson64_android.h for example) > > > > I don't strongly disagree with merging, but I'd prefer to keep it this way. > > > > [1] https://android.googlesource.com/platform/external/u-boot > > > > Simon, can you elaborate on why we should merge the commands? Do you > > think that for U-Boot users it will be easier to have a single command > > for all Android related topics? > > Not necessarily, it's just that we do try to keep similar pieces of > functionality together. Perhaps we should create an entirely new > command to bring all (or most) these things together, with aliases for > compatibility? > > Note that 'boota' might be a better name for actually booting an > Android image, since we have bootm, booti, etc. Having said that, with > bootstd it might just be automatic. > > Why does Android have its own fork? I am not keen on any argument that > says that mainline needs to follow a fork! > > > > > > > > > > Can you please create some docs in doc/usage/cmd/abootimg for the command? > > > > > > I also wonder if ab_select should move under that command? > > Regards, > SImon -- Thank you, Dmitry