|
To align with the official Android BCB (Bootloader 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] -
Based-on:
Reviewed-by: Mattijs Korpershoek <mkorpershoek@...>
Reviewed-by: Simon Glass <sjg@...>
Tested-by: Guillaume La Roque <glaroque@...>
Signed-off-by: Dmitry Rokosov <ddrokosov@...>
---
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 c93e51541019d0fe793303c4b3d5286df061906f..a287eac04fe88ad08bdcf1b1b1d6e564d503d800 100644
--- a/boot/android_ab.c
+++ b/boot/android_ab.c
@@ -52,7 +52,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;
@@ -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);
if (memcmp(abc->slot_suffix, slot_suffix,
sizeof(slot_suffix))) {
memcpy(abc->slot_suffix, slot_suffix,
--
2.43.0
|
Hi Dmitry, On Thu, Oct 17, 2024 at 4:12?PM Dmitry Rokosov <ddrokosov@...> wrote:
To align with the official Android BCB (Bootloader 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] -
Based-on:
Reviewed-by: Mattijs Korpershoek <mkorpershoek@...>
Reviewed-by: Simon Glass <sjg@...>
Tested-by: Guillaume La Roque <glaroque@...>
Signed-off-by: Dmitry Rokosov <ddrokosov@...>
---
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 c93e51541019d0fe793303c4b3d5286df061906f..a287eac04fe88ad08bdcf1b1b1d6e564d503d800 100644
--- a/boot/android_ab.c
+++ b/boot/android_ab.c
@@ -52,7 +52,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;
@@ -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);
if (memcmp(abc->slot_suffix, slot_suffix,
sizeof(slot_suffix))) {
memcpy(abc->slot_suffix, slot_suffix,
--
2.43.0
Reviewed-by: Igor Opaniuk <igor.opaniuk@...> -- Best regards - Atentamente - Meilleures salutations Igor Opaniuk mailto: igor.opaniuk@... skype: igor.opanyuk
|
On Thu, Oct 17, 2024 at 9:12?AM Dmitry Rokosov <ddrokosov@...> wrote:
To align with the official Android BCB (Bootloader 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] -
Based-on:
Reviewed-by: Mattijs Korpershoek <mkorpershoek@...>
Reviewed-by: Simon Glass <sjg@...>
Tested-by: Guillaume La Roque <glaroque@...>
Signed-off-by: Dmitry Rokosov <ddrokosov@...>
Would be nice to add "Fixes:" tag here, pointing to the corresponding commit where the issue was introduced (see kernel docs for details). It could be quite useful for possible stable branches and other purposes, I'd recommend to add that tag for all fixes if you have more in this series. ---
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 c93e51541019d0fe793303c4b3d5286df061906f..a287eac04fe88ad08bdcf1b1b1d6e564d503d800 100644
--- a/boot/android_ab.c
+++ b/boot/android_ab.c
@@ -52,7 +52,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;
@@ -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 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); $ 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}; " \ if (memcmp(abc->slot_suffix, slot_suffix,
sizeof(slot_suffix))) {
memcpy(abc->slot_suffix, slot_suffix,
--
2.43.0
|
Hi Igor, Thank you for the review. On dim., nov. 03, 2024 at 10:43, Igor Opaniuk <igor.opaniuk@...> wrote: Hi Dmitry,
On Thu, Oct 17, 2024 at 4:12?PM Dmitry Rokosov
<ddrokosov@...> wrote:
To align with the official Android BCB (Bootloader 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] -
Based-on:
Reviewed-by: Mattijs Korpershoek <mkorpershoek@...>
Reviewed-by: Simon Glass <sjg@...>
Tested-by: Guillaume La Roque <glaroque@...>
Signed-off-by: Dmitry Rokosov <ddrokosov@...>
---
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 c93e51541019d0fe793303c4b3d5286df061906f..a287eac04fe88ad08bdcf1b1b1d6e564d503d800 100644
--- a/boot/android_ab.c
+++ b/boot/android_ab.c
@@ -52,7 +52,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;
@@ -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);
if (memcmp(abc->slot_suffix, slot_suffix,
sizeof(slot_suffix))) {
memcpy(abc->slot_suffix, slot_suffix,
--
2.43.0
Reviewed-by: Igor Opaniuk <igor.opaniuk@...> Sorry, I can't apply that R-b tag because this series has been merged into master already: --
Best regards - Atentamente - Meilleures salutations
Igor Opaniuk
mailto: igor.opaniuk@...
skype: igor.opanyuk
|
Hi Sam, Thank you for the review. On lun., nov. 04, 2024 at 17:06, Sam Protsenko <semen.protsenko@...> wrote: On Thu, Oct 17, 2024 at 9:12?AM Dmitry Rokosov
<ddrokosov@...> wrote:
To align with the official Android BCB (Bootloader 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] -
Based-on:
Reviewed-by: Mattijs Korpershoek <mkorpershoek@...>
Reviewed-by: Simon Glass <sjg@...>
Tested-by: Guillaume La Roque <glaroque@...>
Signed-off-by: Dmitry Rokosov <ddrokosov@...>
Would be nice to add "Fixes:" tag here, pointing to the corresponding
commit where the issue was introduced (see kernel docs for details).
It could be quite useful for possible stable branches and other
purposes, I'd recommend to add that tag for all fixes if you have more
in this series. I agree. Unfortunately, this series has already been merged here:
---
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 c93e51541019d0fe793303c4b3d5286df061906f..a287eac04fe88ad08bdcf1b1b1d6e564d503d800 100644
--- a/boot/android_ab.c
+++ b/boot/android_ab.c
@@ -52,7 +52,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;
@@ -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(). 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.
if (memcmp(abc->slot_suffix, slot_suffix,
sizeof(slot_suffix))) {
memcpy(abc->slot_suffix, slot_suffix,
--
2.43.0
|
Hi Sam, On mar., nov. 05, 2024 at 18:58, Sam Protsenko <semen.protsenko@...> wrote: On Tue, Nov 5, 2024 at 9:06?AM Mattijs Korpershoek
<mkorpershoek@...> 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: ;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) So implementations before 2019 that do not have this patch: 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: ;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]
|
Hi Sam, On mer., nov. 06, 2024 at 21:17, Sam Protsenko <semen.protsenko@...> wrote: On Wed, Nov 6, 2024 at 4:02?AM Mattijs Korpershoek
<mkorpershoek@...> wrote:
Hi Sam,
On mar., nov. 05, 2024 at 18:58, Sam Protsenko <semen.protsenko@...> wrote:
On Tue, Nov 5, 2024 at 9:06?AM Mattijs Korpershoek
<mkorpershoek@...> 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:
That probably still leaves the possible combination of some devices
running new U-Boot versions (with this patch applied) together with
older Android versions. E.g. in case when U-Boot is updated but
Android isn't, may be especially relevant for some dev boards out
there. Yes, you are right. Boards with Android older than 2019 and U-Boot v2025.01+ My experience is that most of the time, it's the bootloader that stays out of date and the Android that gets updated. But that might not be the general case.
"""
// 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. So even in recent Android versions it's being initialized from BCB in
case the property is not set.
Nope, the comment is wrong. If look at the lines below you can see that the function exits early when the property is not set. So in recent Android versions if the propery is not set, we just fail to initialize.
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:
;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)
So implementations before 2019 that do not have this patch:
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:
;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!
My point is, if it's possible to introduce this change but keep the
same old behavior (across both U-Boot and AOSP), it's usually better
to do that that way. If I'm not missing anything and that's a valid
concern, maybe a separate patch can be developed on top of the merged
patch series handling that. Anyways, I don't have enough time to work
on this right now, so just pointing out what I noticed, if it's useful
for anybody. I'll let the maintainers decide :)
Yes, thanks a lot for noticing and taking the time to report your valid concerns :)
Thanks!
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]
|
On Tue, Nov 5, 2024 at 9:06?AM Mattijs Korpershoek <mkorpershoek@...> 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. 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]
|
On Wed, Nov 6, 2024 at 4:02?AM Mattijs Korpershoek <mkorpershoek@...> wrote:
Hi Sam,
On mar., nov. 05, 2024 at 18:58, Sam Protsenko <semen.protsenko@...> wrote:
On Tue, Nov 5, 2024 at 9:06?AM Mattijs Korpershoek
<mkorpershoek@...> 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:
That probably still leaves the possible combination of some devices running new U-Boot versions (with this patch applied) together with older Android versions. E.g. in case when U-Boot is updated but Android isn't, may be especially relevant for some dev boards out there. """
// 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. So even in recent Android versions it's being initialized from BCB in case the property is not set. 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:
;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)
So implementations before 2019 that do not have this patch:
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:
;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!
My point is, if it's possible to introduce this change but keep the same old behavior (across both U-Boot and AOSP), it's usually better to do that that way. If I'm not missing anything and that's a valid concern, maybe a separate patch can be developed on top of the merged patch series handling that. Anyways, I don't have enough time to work on this right now, so just pointing out what I noticed, if it's useful for anybody. I'll let the maintainers decide :) Thanks!
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]
|
