Re: [PATCH v1 2/4] cmd: ab: introduce 'ab_dump' command to print BCB block content

[Mattijs Korpershoek <mkorpershoek@baylibre.com>]

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.

>
>> + * 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.

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?

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