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
> <mkorpersh...@baylibre.com> wrote:
> >
> > Hi Dmitry,
> >
> > Thank you for the patch.
> >
> > Hi Simon,
> >
> > On dim., juil. 28, 2024 at 13:36, Simon Glass <s...@chromium.org> wrote:
> >
> > > Hi Dmitry,
> > >
> > > On Thu, 25 Jul 2024 at 14:55, Dmitry Rokosov
> > > <ddroko...@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 <ddroko...@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
