[PATCH v1 0/4] android_ab: fix slot_suffix issue and introduce ab_dump command

[PATCH v1 0/4] android_ab: fix slot_suffix issue and introduce ab_dump command

[Dmitry Rokosov]

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

[PATCH v1 1/4] cmd: ab_select: fix indentation problems for --no-dec parameter

[Dmitry Rokosov]

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"
 );
-- 
2.43.0

Re: [PATCH v1 1/4] cmd: ab_select: fix indentation problems for --no-dec parameter

[Simon Glass]

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>

Re: [PATCH v1 1/4] cmd: ab_select: fix indentation problems for --no-dec parameter

[Mattijs Korpershoek]

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

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

[Dmitry Rokosov]

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 */
-- 
2.43.0

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

[Simon Glass]

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

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

[Dmitry Rokosov]

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.

-- 
Thank you,
Dmitry

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

[Mattijs Korpershoek]

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

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

[Simon Glass]

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

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

[Dmitry Rokosov]

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

-- 
Thank you,
Dmitry

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

[Mattijs Korpershoek]

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

[PATCH v1 3/4] test/py: introduce test for ab_dump command

[Dmitry Rokosov]

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')
-- 
2.43.0

Re: [PATCH v1 3/4] test/py: introduce test for ab_dump command

[Simon Glass]

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

Re: [PATCH v1 3/4] test/py: introduce test for ab_dump command

[Dmitry Rokosov]

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.

-- 
Thank you,
Dmitry

Re: [PATCH v1 3/4] test/py: introduce test for ab_dump command

[Mattijs Korpershoek]

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

[PATCH v1 4/4] common: android_ab: fix slot suffix for abc block

[Dmitry Rokosov]

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

Re: [PATCH v1 4/4] common: android_ab: fix slot suffix for abc block

[Simon Glass]

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
>

Re: [PATCH v1 4/4] common: android_ab: fix slot suffix for abc block

[Dmitry Rokosov]

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

-- 
Thank you,
Dmitry

Re: [PATCH v1 4/4] common: android_ab: fix slot suffix for abc block

[Mattijs Korpershoek]

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);
>  		if (memcmp(abc->slot_suffix, slot_suffix,
>  			   sizeof(slot_suffix))) {
>  			memcpy(abc->slot_suffix, slot_suffix,
> -- 
> 2.43.0

Re: [PATCH v1 4/4] common: android_ab: fix slot suffix for abc block

[Dmitry Rokosov]

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);
> >  		if (memcmp(abc->slot_suffix, slot_suffix,
> >  			   sizeof(slot_suffix))) {
> >  			memcpy(abc->slot_suffix, slot_suffix,
> > -- 
> > 2.43.0

-- 
Thank you,
Dmitry

Re: [PATCH v1 0/4] android_ab: fix slot_suffix issue and introduce ab_dump command

[Dmitry Rokosov]

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
> 

-- 
Thank you,
Dmitry