Hi Simon,

On mer., juil. 31, 2024 at 08:38, Simon Glass sjg@chromium.org wrote:

Hi Mattijs,

On Tue, 30 Jul 2024 at 02:19, Mattijs Korpershoek mkorpershoek@baylibre.com wrote:

...

Hi Dmitry,

Thank you for the patch.

Hi Simon,

On dim., juil. 28, 2024 at 13:36, Simon Glass sjg@chromium.org wrote:

...

Hi Dmitry,

On Thu, 25 Jul 2024 at 14:55, Dmitry Rokosov ddrokosov@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 ddrokosov@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.

...

...

...

• • 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.

...

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?

I think that merging the ab_* features into BCB is the most relevant.

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.

Boards relying on boot* commands for booting android use the bootm command, see: include/configs/meson64_android.h

In my opinion, everyone booting Android should migrate to use bootstd.

Why does Android have its own fork? I am not keen on any argument that says that mainline needs to follow a fork!

I don't know why Android has its own fork, but it's unfortunate.

Looking at the code, I suspect: - Android bootflow (for cuttlefish) (cmd/boot_android) - Fixes on fastboot - Fixes on AB - Fixes on AVB (support for block devices instead of only mmc)

For example, here are some relevant fixes that I want to port to upstream:

- https://android-review.googlesource.com/c/platform/external/u-boot/+/1446442 - https://android-review.googlesource.com/c/platform/external/u-boot/+/1451016

I completely agree with you. It's non-sense say that "mainline needs to follow a fork". I'm just a bit worried that porting over relevant changes from this particular fork might get more difficult in the future.

Thinking again about this, if we just move the command bits and keep the boot/android_ab.c part it should be too troublesome.

So I'm ok to proceed with this.

Thanks!

...

...

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