Re: [PATCH v5 6/6] common: android_ab: fix slot suffix for abc block

[Mattijs Korpershoek <mkorpershoek@baylibre.com>]

Hi Sam,

On mar., nov. 05, 2024 at 18:58, Sam Protsenko <semen.protsenko@linaro.org> wrote:

> On Tue, Nov 5, 2024 at 9:06 AM Mattijs Korpershoek
> <mkorpershoek@baylibre.com> wrote:
>>
>> Hi Sam,
>>
>
> Hey Mattijs,
>
> [snip]
>
>> >> @@ -328,7 +328,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);
>> >
>> > AFAIU, this changes the behavior of two above functions, and
>> > consequently of "bcb ab_select" command? If so, just to double check:
>> > were all users of those reworked correspondingly? I can see next
>> > occurrences (there may be more):
>> >
>> > $ grep -sIrHn '"_' boot/bootmeth_android.c
>>
>> I thought the same when first reviewing the patch.
>> Looking a bit closer...
>>
>> >
>> > boot/bootmeth_android.c:74:    sprintf(partname, BOOT_PART_NAME "_%s",
>> > priv->slot);
>> > boot/bootmeth_android.c:111:    sprintf(partname,
>> > VENDOR_BOOT_PART_NAME "_%s", priv->slot);
>> > boot/bootmeth_android.c:156:    sprintf(slot_suffix, "_%s", priv->slot);
>> > boot/bootmeth_android.c:397:    sprintf(slot_suffix, "_%s", priv->slot);
>>
>> ... We can see that ab_select_slot() returns an integer
>> That integer is used later on to initialize priv->slot:
>>
>> """
>>         priv->slot[0] = BOOT_SLOT_NAME(ret);
>>         priv->slot[1] = '\0';
>> """
>>
>> The change from Dmitry only changes what we **write** to the BCB (into
>> the misc partition), not what is returned by ab_select_slot().
>>
>
> Sure. Just wanted to double check that the behavior is not changed in
> any related parts, as the commit message doesn't mention that. Btw,
> BCB is an interface between the bootloader and AOSP, so if this patch
> changes what's being written into BCB, does it affect AOSP part of it
> somehow? Especially for already existing devices with particular BCB
> data, in case U-Boot gets updated there.

Those are valid concerns.

Per my understanding, on recent Android versions the slot suffix is not
read from BCB, but from the ro.boot.slot_suffix property:

"""
  // Initialize the current_slot from the read-only property. If the property
  // was not set (from either the command line or the device tree), we can later
  // initialize it from the bootloader_control struct.
  std::string suffix_prop = android::base::GetProperty("ro.boot.slot_suffix", "");
  if (suffix_prop.empty()) {
    LOG(ERROR) << "Slot suffix property is not set";
    return false;
  }
  current_slot_ = SlotSuffixToIndex(suffix_prop.c_str());
"""

See:
https://cs.android.com/android/platform/superproject/main/+/main:hardware/interfaces/boot/1.1/default/boot_control/libboot_control.cpp;l=185;drc=86b8f575059a1799c760ca7012f540a528d68a9d;bpv=1;bpt=1

This has been the case since 2019.

If we look at earlier implementations of libboot_control (which was in
bootable/recovery)
https://android-review.googlesource.com/c/platform/bootable/recovery/+/1191517

So implementations before 2019 that do not have this patch:
https://android-review.googlesource.com/c/platform/bootable/recovery/+/1111899

Will get the slot suffix from the BCB (not from the commandline)

For these older implementations, we will go through the following:
BootControl::Init()
  LoadBootloaderControl(device.c_str(), &boot_ctrl)
  android::base::ReadFully(fd.get(), buffer, sizeof(bootloader_control)

And struct bootloader_control has:

struct bootloader_control {
    // NUL terminated active slot suffix.
    char slot_suffix[4];

And if we look at how the BCB is initialized from userspace in:
https://cs.android.com/android/platform/superproject/main/+/main:hardware/interfaces/boot/1.1/default/boot_control/libboot_control.cpp;l=120;drc=86b8f575059a1799c760ca7012f540a528d68a9d

We can see that we copy _a, not a (for example, if slot == 0).

So I think this is fine.

If needed, I can dig more for behaviour on older devices (<2019), let me know!

>
>> ab_select_slot() still returns an integer which needs to be converted
>> via the BOOT_SLOT_NAME() macro.
>>
>> >
>> > $ grep -sIrHn 'slot_suffix _' include/configs/
>> > include/configs/ti_omap5_common.h:107:    "setenv slot_suffix _${slot_name};"
>> > include/configs/meson64_android.h:65:        "setenv slot_suffix
>> > _${current_slot}; " \
>>
>> Same goes for these 2 examples, we use:
>> The "bcb ab_select current_slot" command to store the slot into the
>> "current_slot" environment variable.
>> Looking at cmd/bcb.c we can see:
>>
>> """
>>         ret = ab_select_slot(dev_desc, &part_info, dec_tries);
>>         if (ret < 0) {
>>                 printf("Android boot failed, error %d.\n", ret);
>>                 return CMD_RET_FAILURE;
>>         }
>>
>>         /* Android standard slot names are 'a', 'b', ... */
>>         slot[0] = BOOT_SLOT_NAME(ret);
>>         slot[1] = '\0';
>>         env_set(argv[1], slot);
>>         printf("ANDROID: Booting slot: %s\n", slot);
>> """
>>
>> So I think this is fine.
>>
>
> [snip]