[PATCH v1 0/4] android_ab: fix slot_suffix issue and introduce ab_dump command
The patch series include changes: - fix indentation problems for --no-dec parameter in the ab_select command - introduce the ab_dump command to print the content of the BCB block; it's seful for debugging A/B logic on supported boards - add a test for the ab_dump command to verify the accuracy of each field within the ABC data displayed. - fix the slot suffix format in the ABC block to align with official Android BCB specifications
Dmitry Rokosov (4): cmd: ab_select: fix indentation problems for --no-dec parameter cmd: ab: introduce 'ab_dump' command to print BCB block content test/py: introduce test for ab_dump command common: android_ab: fix slot suffix for abc block
boot/android_ab.c | 73 ++++++++++++++++++++++++++- cmd/ab_select.c | 34 ++++++++++++- include/android_ab.h | 9 ++++ test/py/tests/test_android/test_ab.py | 23 +++++++++ 4 files changed, 135 insertions(+), 4 deletions(-)
Command ab_select has wrong help description from indentation perspective.
Signed-off-by: Dmitry Rokosov ddrokosov@salutedevices.com --- cmd/ab_select.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/cmd/ab_select.c b/cmd/ab_select.c index bfb67b8236b6..9e2f74573c22 100644 --- a/cmd/ab_select.c +++ b/cmd/ab_select.c @@ -63,6 +63,6 @@ U_BOOT_CMD(ab_select, 5, 0, do_ab_select, " - If 'part_name' is passed, preceded with a # instead of :, the\n" " partition name whose label is 'part_name' will be looked up in\n" " the partition table. This is commonly the "misc" partition.\n" - " - If '--no-dec' is set, the number of tries remaining will not\n" - " decremented for the selected boot slot\n" + " - If '--no-dec' is set, the number of tries remaining will not\n" + " decremented for the selected boot slot\n" );
On Thu, 25 Jul 2024 at 14:55, Dmitry Rokosov ddrokosov@salutedevices.com wrote:
Command ab_select has wrong help description from indentation perspective.
Signed-off-by: Dmitry Rokosov ddrokosov@salutedevices.com
cmd/ab_select.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
Hi Dmitry,
Thank you for the patch.
On jeu., juil. 25, 2024 at 22:47, Dmitry Rokosov ddrokosov@salutedevices.com wrote:
Command ab_select has wrong help description from indentation perspective.
Signed-off-by: Dmitry Rokosov ddrokosov@salutedevices.com
Reviewed-by: Mattijs Korpershoek mkorpershoek@baylibre.com
cmd/ab_select.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/cmd/ab_select.c b/cmd/ab_select.c index bfb67b8236b6..9e2f74573c22 100644 --- a/cmd/ab_select.c +++ b/cmd/ab_select.c @@ -63,6 +63,6 @@ U_BOOT_CMD(ab_select, 5, 0, do_ab_select, " - If 'part_name' is passed, preceded with a # instead of :, the\n" " partition name whose label is 'part_name' will be looked up in\n" " the partition table. This is commonly the "misc" partition.\n"
" - If '--no-dec' is set, the number of tries remaining will not\n"" decremented for the selected boot slot\n"
" - If '--no-dec' is set, the number of tries remaining will not\n"" decremented for the selected boot slot\n");
2.43.0
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 + * 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 */
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);elseprintf(" (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);elseprintf(" (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);elseprintf(" (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);elseprintf(" (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:
- Easier to track/port changes from Google's fork [1]
- Better separation of responsabilities
- 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);elseprintf(" (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:
- Easier to track/port changes from Google's fork [1]
- Better separation of responsabilities
- 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);elseprintf(" (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:
- Easier to track/port changes from Google's fork [1]
- Better separation of responsabilities
- 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
The ab_dump command allows you to display ABC data directly on the U-Boot console. During an A/B test execution, this test verifies the accuracy of each field within the ABC data.
Signed-off-by: Dmitry Rokosov ddrokosov@salutedevices.com --- test/py/tests/test_android/test_ab.py | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)
diff --git a/test/py/tests/test_android/test_ab.py b/test/py/tests/test_android/test_ab.py index c79cb07fda35..8aa42fe67c33 100644 --- a/test/py/tests/test_android/test_ab.py +++ b/test/py/tests/test_android/test_ab.py @@ -54,6 +54,27 @@ def ab_disk_image(u_boot_console): di = ABTestDiskImage(u_boot_console) return di
+def ab_dump(u_boot_console, slot_num, crc): + output = u_boot_console.run_command('ab_dump host 0#misc') + header, slot0, slot1 = output.split('\r\r\n\r\r\n') + slots = [slot0, slot1] + slot_suffixes = ['_a', '_b'] + + header = dict(map(lambda x: map(str.strip, x.split(':')), header.split('\r\r\n'))) + assert header['Bootloader Control'] == '[misc]' + assert header['Active Slot'] == slot_suffixes[slot_num] + assert header['Magic Number'] == '0x42414342' + assert header['Version'] == '1' + assert header['Number of Slots'] == '2' + assert header['Recovery Tries Remaining'] == '0' + assert header['CRC'] == '{} (Valid)'.format(crc) + + slot = dict(map(lambda x: map(str.strip, x.split(':')), slots[slot_num].split('\r\r\n\t- ')[1:])) + assert slot['Priority'] == '15' + assert slot['Tries Remaining'] == '6' + assert slot['Successful Boot'] == '0' + assert slot['Verity Corrupted'] == '0' + @pytest.mark.boardspec('sandbox') @pytest.mark.buildconfigspec('android_ab') @pytest.mark.buildconfigspec('cmd_ab_select') @@ -68,8 +89,10 @@ def test_ab(ab_disk_image, u_boot_console): assert 'Attempting slot a, tries remaining 7' in output output = u_boot_console.run_command('printenv slot_name') assert 'slot_name=a' in output + ab_dump(u_boot_console, 0, '0xD438D1B9')
output = u_boot_console.run_command('ab_select slot_name host 0:1') assert 'Attempting slot b, tries remaining 7' in output output = u_boot_console.run_command('printenv slot_name') assert 'slot_name=b' in output + ab_dump(u_boot_console, 1, '0x011EC016')
Hi Dmitry,
On Thu, 25 Jul 2024 at 14:55, Dmitry Rokosov ddrokosov@salutedevices.com wrote:
The ab_dump command allows you to display ABC data directly on the U-Boot console. During an A/B test execution, this test verifies the accuracy of each field within the ABC data.
Signed-off-by: Dmitry Rokosov ddrokosov@salutedevices.com
test/py/tests/test_android/test_ab.py | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)
This looks good but please use lower-case hex.
Regards, Simon
Hello Simon,
On Sun, Jul 28, 2024 at 01:36:05PM -0600, Simon Glass wrote:
Hi Dmitry,
On Thu, 25 Jul 2024 at 14:55, Dmitry Rokosov ddrokosov@salutedevices.com wrote:
The ab_dump command allows you to display ABC data directly on the U-Boot console. During an A/B test execution, this test verifies the accuracy of each field within the ABC data.
Signed-off-by: Dmitry Rokosov ddrokosov@salutedevices.com
test/py/tests/test_android/test_ab.py | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)
This looks good but please use lower-case hex.
Thank you for the quick review!
I will change it to lower-case hex in the next version.
Hi Dmitry,
Thank you for the patch.
On lun., juil. 29, 2024 at 17:39, Dmitry Rokosov ddrokosov@salutedevices.com wrote:
Hello Simon,
On Sun, Jul 28, 2024 at 01:36:05PM -0600, Simon Glass wrote:
Hi Dmitry,
On Thu, 25 Jul 2024 at 14:55, Dmitry Rokosov ddrokosov@salutedevices.com wrote:
The ab_dump command allows you to display ABC data directly on the U-Boot console. During an A/B test execution, this test verifies the accuracy of each field within the ABC data.
Signed-off-by: Dmitry Rokosov ddrokosov@salutedevices.com
test/py/tests/test_android/test_ab.py | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)
This looks good but please use lower-case hex.
Thank you for the quick review!
I will change it to lower-case hex in the next version.
With Simon's suggestion addressed:
Reviewed-by: Mattijs Korpershoek mkorpershoek@baylibre.com
-- Thank you, Dmitry
To align with the official Android BCB (Boot Control Block) specifications, it's important to note that the slot_suffix should start with an underscore symbol.
For a comprehensive understanding of the expected slot_suffix format in userspace, please refer to the provided reference [1].
Links: [1] - https://source.android.com/docs/core/architecture/bootloader/updating#slots
Signed-off-by: Dmitry Rokosov ddrokosov@salutedevices.com --- boot/android_ab.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/boot/android_ab.c b/boot/android_ab.c index 359cc1a00428..45c154b10f1a 100644 --- a/boot/android_ab.c +++ b/boot/android_ab.c @@ -53,7 +53,7 @@ static int ab_control_default(struct bootloader_control *abc) if (!abc) return -EFAULT;
- memcpy(abc->slot_suffix, "a\0\0\0", 4); + memcpy(abc->slot_suffix, "_a\0\0", 4); abc->magic = BOOT_CTRL_MAGIC; abc->version = BOOT_CTRL_VERSION; abc->nb_slot = NUM_SLOTS; @@ -319,7 +319,8 @@ int ab_select_slot(struct blk_desc *dev_desc, struct disk_partition *part_info, * or the device tree. */ memset(slot_suffix, 0, sizeof(slot_suffix)); - slot_suffix[0] = BOOT_SLOT_NAME(slot); + slot_suffix[0] = '_'; + slot_suffix[1] = BOOT_SLOT_NAME(slot); if (memcmp(abc->slot_suffix, slot_suffix, sizeof(slot_suffix))) { memcpy(abc->slot_suffix, slot_suffix,
Hi Dmitry,
On Thu, 25 Jul 2024 at 14:55, Dmitry Rokosov ddrokosov@salutedevices.com wrote:
To align with the official Android BCB (Boot Control Block) specifications, it's important to note that the slot_suffix should start with an underscore symbol.
For a comprehensive understanding of the expected slot_suffix format in userspace, please refer to the provided reference [1].
Links: [1] - https://source.android.com/docs/core/architecture/bootloader/updating#slots
Signed-off-by: Dmitry Rokosov ddrokosov@salutedevices.com
boot/android_ab.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
(can the test cover this?)
diff --git a/boot/android_ab.c b/boot/android_ab.c index 359cc1a00428..45c154b10f1a 100644 --- a/boot/android_ab.c +++ b/boot/android_ab.c @@ -53,7 +53,7 @@ static int ab_control_default(struct bootloader_control *abc) if (!abc) return -EFAULT;
memcpy(abc->slot_suffix, "a\0\0\0", 4);
memcpy(abc->slot_suffix, "_a\0\0", 4); abc->magic = BOOT_CTRL_MAGIC; abc->version = BOOT_CTRL_VERSION; abc->nb_slot = NUM_SLOTS;@@ -319,7 +319,8 @@ int ab_select_slot(struct blk_desc *dev_desc, struct disk_partition *part_info, * or the device tree. */ memset(slot_suffix, 0, sizeof(slot_suffix));
slot_suffix[0] = BOOT_SLOT_NAME(slot);
slot_suffix[0] = '_';slot_suffix[1] = BOOT_SLOT_NAME(slot); if (memcmp(abc->slot_suffix, slot_suffix, sizeof(slot_suffix))) { memcpy(abc->slot_suffix, slot_suffix,-- 2.43.0
On Sun, Jul 28, 2024 at 01:36:06PM -0600, Simon Glass wrote:
Hi Dmitry,
On Thu, 25 Jul 2024 at 14:55, Dmitry Rokosov ddrokosov@salutedevices.com wrote:
To align with the official Android BCB (Boot Control Block) specifications, it's important to note that the slot_suffix should start with an underscore symbol.
For a comprehensive understanding of the expected slot_suffix format in userspace, please refer to the provided reference [1].
Links: [1] - https://source.android.com/docs/core/architecture/bootloader/updating#slots
Signed-off-by: Dmitry Rokosov ddrokosov@salutedevices.com
boot/android_ab.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
(can the test cover this?)
I understand your point. I will try to think about the test that could potentially uncover this problem.
Will send it in the next version.
diff --git a/boot/android_ab.c b/boot/android_ab.c index 359cc1a00428..45c154b10f1a 100644 --- a/boot/android_ab.c +++ b/boot/android_ab.c @@ -53,7 +53,7 @@ static int ab_control_default(struct bootloader_control *abc) if (!abc) return -EFAULT;
memcpy(abc->slot_suffix, "a\0\0\0", 4);
memcpy(abc->slot_suffix, "_a\0\0", 4); abc->magic = BOOT_CTRL_MAGIC; abc->version = BOOT_CTRL_VERSION; abc->nb_slot = NUM_SLOTS;@@ -319,7 +319,8 @@ int ab_select_slot(struct blk_desc *dev_desc, struct disk_partition *part_info, * or the device tree. */ memset(slot_suffix, 0, sizeof(slot_suffix));
slot_suffix[0] = BOOT_SLOT_NAME(slot);
slot_suffix[0] = '_';slot_suffix[1] = BOOT_SLOT_NAME(slot); if (memcmp(abc->slot_suffix, slot_suffix, sizeof(slot_suffix))) { memcpy(abc->slot_suffix, slot_suffix,-- 2.43.0
Hi Dmitry,
Thank you for the patch.
On jeu., juil. 25, 2024 at 22:47, Dmitry Rokosov ddrokosov@salutedevices.com wrote:
To align with the official Android BCB (Boot Control Block) specifications, it's important to note that the slot_suffix should start with an underscore symbol.
For a comprehensive understanding of the expected slot_suffix format in userspace, please refer to the provided reference [1].
Links: [1] - https://source.android.com/docs/core/architecture/bootloader/updating#slots
Signed-off-by: Dmitry Rokosov ddrokosov@salutedevices.com
Can we point to the AOSP change on which this is based in the commit message?
https://android-review.googlesource.com/c/platform/external/u-boot/+/1446439
With that added:
Reviewed-by: Mattijs Korpershoek mkorpershoek@baylibre.com
boot/android_ab.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/boot/android_ab.c b/boot/android_ab.c index 359cc1a00428..45c154b10f1a 100644 --- a/boot/android_ab.c +++ b/boot/android_ab.c @@ -53,7 +53,7 @@ static int ab_control_default(struct bootloader_control *abc) if (!abc) return -EFAULT;
- memcpy(abc->slot_suffix, "a\0\0\0", 4);
- memcpy(abc->slot_suffix, "_a\0\0", 4); abc->magic = BOOT_CTRL_MAGIC; abc->version = BOOT_CTRL_VERSION; abc->nb_slot = NUM_SLOTS;
@@ -319,7 +319,8 @@ int ab_select_slot(struct blk_desc *dev_desc, struct disk_partition *part_info, * or the device tree. */ memset(slot_suffix, 0, sizeof(slot_suffix));
slot_suffix[0] = BOOT_SLOT_NAME(slot);
slot_suffix[0] = '_';slot_suffix[1] = BOOT_SLOT_NAME(slot);-- 2.43.0
On Tue, Jul 30, 2024 at 10:43:13AM +0200, Mattijs Korpershoek wrote:
Hi Dmitry,
Thank you for the patch.
On jeu., juil. 25, 2024 at 22:47, Dmitry Rokosov ddrokosov@salutedevices.com wrote:
To align with the official Android BCB (Boot Control Block) specifications, it's important to note that the slot_suffix should start with an underscore symbol.
For a comprehensive understanding of the expected slot_suffix format in userspace, please refer to the provided reference [1].
Links: [1] - https://source.android.com/docs/core/architecture/bootloader/updating#slots
Signed-off-by: Dmitry Rokosov ddrokosov@salutedevices.com
Can we point to the AOSP change on which this is based in the commit message?
https://android-review.googlesource.com/c/platform/external/u-boot/+/1446439
With that added:
Reviewed-by: Mattijs Korpershoek mkorpershoek@baylibre.com
Sure, no problem. I will add the reference in the next version.
boot/android_ab.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/boot/android_ab.c b/boot/android_ab.c index 359cc1a00428..45c154b10f1a 100644 --- a/boot/android_ab.c +++ b/boot/android_ab.c @@ -53,7 +53,7 @@ static int ab_control_default(struct bootloader_control *abc) if (!abc) return -EFAULT;
- memcpy(abc->slot_suffix, "a\0\0\0", 4);
- memcpy(abc->slot_suffix, "_a\0\0", 4); abc->magic = BOOT_CTRL_MAGIC; abc->version = BOOT_CTRL_VERSION; abc->nb_slot = NUM_SLOTS;
@@ -319,7 +319,8 @@ int ab_select_slot(struct blk_desc *dev_desc, struct disk_partition *part_info, * or the device tree. */ memset(slot_suffix, 0, sizeof(slot_suffix));
slot_suffix[0] = BOOT_SLOT_NAME(slot);
slot_suffix[0] = '_';slot_suffix[1] = BOOT_SLOT_NAME(slot);-- 2.43.0
Azure pipeline is successfully DONE
https://github.com/u-boot/u-boot/pull/625/checks
On Thu, Jul 25, 2024 at 10:47:00PM +0300, Dmitry Rokosov wrote:
The patch series include changes: - fix indentation problems for --no-dec parameter in the ab_select command - introduce the ab_dump command to print the content of the BCB block; it's seful for debugging A/B logic on supported boards - add a test for the ab_dump command to verify the accuracy of each field within the ABC data displayed. - fix the slot suffix format in the ABC block to align with official Android BCB specifications
Dmitry Rokosov (4): cmd: ab_select: fix indentation problems for --no-dec parameter cmd: ab: introduce 'ab_dump' command to print BCB block content test/py: introduce test for ab_dump command common: android_ab: fix slot suffix for abc block
boot/android_ab.c | 73 ++++++++++++++++++++++++++- cmd/ab_select.c | 34 ++++++++++++- include/android_ab.h | 9 ++++ test/py/tests/test_android/test_ab.py | 23 +++++++++ 4 files changed, 135 insertions(+), 4 deletions(-)
-- 2.43.0
participants (3)
-
Dmitry Rokosov -
Mattijs Korpershoek -
Simon Glass