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

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(+)

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);
> +
> +       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: ...

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

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

On Sun, Jul 28, 2024 at 01:36:04PM -0600, Simon Glass 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);
> > +
> > +       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: ...
> 
> > + * 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.
> 
> 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?

I believe it's an excellent idea to utilize abootimg for modifying and
dumping Android related stuff, as this tool is specifically designed for
such purposes. Moreover, the A/B approach is predominantly an
Android-based feature.

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

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?

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

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

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

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