Hi Igor, Jan, On Tue, Oct 13, 2026 at 17:31, Igor Opaniuk <[email protected]> wrote:
> Hi Jan, > > On Tue, Oct 6, 2026 at 3:00 PM Jan Kehren <[email protected]> wrote: >> >> AVB assumed its partitions were on mmc: avb_ops_alloc() took a bare device >> number and get_partition() called find_mmc_device(). bootmeth_android >> accepts any bootdev but passes only desc->devnum, so an Android bootflow >> found on nvme 0 or usb 0 is verified against mmc 0. > > I don't think this path can be reached today: android_check() rejects > every non-mmc bootdev through bootflow_iter_check_mmc(), and > android_read_bootflow() loads the BCB with > bcb_find_partition_and_load("mmc", ...). > > So the bootmeth hunk below is right, but it does not change behaviour yet. > The user that does benefit is "avb init <interface> <dev>", e.g. > board/armltd/total_compute/total_compute.env, which has been calling > "avb init ${blk_dev} 0" since 2024 and gets a usage error on next. Could > you reword the motivation along those lines and mention total_compute? > >> >> Carry a uclass id alongside the device number and resolve the block device >> with blk_get_devnum_by_uclass_id(), keeping mmc_init() and the hardware >> partition switch on the mmc path only. "avb init <dev>" still means mmc, >> "avb init <interface> <dev>" selects another bus, and both now look the >> device up before allocating. get_boot_interface() returns UCLASS_INVALID >> without ops, so a caller that skipped "avb init" fails rather than reading >> mmc. >> >> Signed-off-by: Jan Kehren <[email protected]> >> --- >> Changes in v2: >> - Split: the renames moved to the preceding patch >> - Put the "avb init <dev>" comment on the branch it describes, by testing >> argc == 2 rather than argc == 3 >> - Drop the single-use part_num and move the user-area comment next to >> mmc_switch_part() >> >> boot/bootmeth_android.c | 2 +- >> cmd/avb.c | 24 ++++++--- >> common/Kconfig | 7 ++- >> common/avb_verify.c | 71 +++++++++++++++++--------- >> doc/android/avb2.rst | 7 ++- >> include/avb_verify.h | 26 +++++++--- >> test/py/tests/test_android/test_avb.py | 28 ++++++++++ >> 7 files changed, 127 insertions(+), 38 deletions(-) >> >> diff --git a/boot/bootmeth_android.c b/boot/bootmeth_android.c >> index 0db08d4f86..ae05c08488 100644 >> --- a/boot/bootmeth_android.c >> +++ b/boot/bootmeth_android.c >> @@ -443,7 +443,7 @@ static int run_avb_verification(struct bootflow *bflow) >> bool unlocked = false; >> int ret; >> >> - avb_ops = avb_ops_alloc(desc->devnum); >> + avb_ops = avb_ops_alloc(desc->uclass_id, desc->devnum); >> if (!avb_ops) >> return log_msg_ret("avb ops", -ENOMEM); >> >> diff --git a/cmd/avb.c b/cmd/avb.c >> index 8fbd48ee5a..4c1f56a33a 100644 >> --- a/cmd/avb.c >> +++ b/cmd/avb.c >> @@ -16,17 +16,28 @@ static struct AvbOps *avb_ops; >> >> int do_avb_init(struct cmd_tbl *cmdtp, int flag, int argc, char *const >> argv[]) >> { >> - unsigned long mmc_dev; >> + struct blk_desc *desc; >> >> - if (argc != 2) >> + if (argc < 2 || argc > 3) >> return CMD_RET_USAGE; >> >> - mmc_dev = hextoul(argv[1], NULL); >> + /* "avb init <dev>" keeps its old meaning of mmc <dev> */ >> + if (argc == 2) >> + desc = blk_get_devnum_by_uclass_id(UCLASS_MMC, >> + hextoul(argv[1], NULL)); >> + else >> + desc = blk_get_devnum_by_uclass_idname(argv[1], >> + hextoul(argv[2], >> NULL)); >> + > > This up-front lookup is what breaks the existing sandbox tests: with the > series, test_avb_verify, test_avb_read_rb, test_avb_is_unlocked, > test_avb_persistent_values and the new test_avb_init_interface fail with > > Can't map file 'mmc1.img': Invalid argument > sandbox_mmc_probe() mmc1: Unable to map file 'mmc1.img' > Can't find the requested device > > while next gives 4 passed, 2 skipped for test_avb.py. > > Sandbox mmc1 has no backing image when these tests run > (test_android/ is collected before the tests that create mmc images), > and most of them only talk to the sandbox TA, so they never needed > the device before. Would you consider keeping > "avb init" lazy: validate the interface name here, store uclass + number, > and let get_partition() resolve the device on first access as before? > > That keeps the old semantics, and a missing device is still reported, by > the first partition access. Alternatively the tests would need to create > mmc1.img first. Indeed, the sandbox tests fail on u-boot-dfu here: https://git.u-boot-project.org/u-boot/custodians/u-boot-dfu/-/jobs/113402 Before applying this series: https://git.u-boot-project.org/u-boot/custodians/u-boot-dfu/-/jobs/113085 I will have a look. I do have a preference for the mmc1.img to be created in the tests first but I don't know if that's feasible. If I can't find a way for this, then we will have to rework this patch as Igor suggested: (Keep the old behaviour: "lazy" init in "avb init") > > >> + if (!desc) { >> + printf("Can't find the requested device\n"); >> + return CMD_RET_FAILURE; >> + } >> >> if (avb_ops) >> avb_ops_free(avb_ops); >> >> - avb_ops = avb_ops_alloc(mmc_dev); >> + avb_ops = avb_ops_alloc(desc->uclass_id, desc->devnum); >> if (avb_ops) >> return CMD_RET_SUCCESS; >> else >> @@ -428,7 +439,7 @@ int do_avb_write_pvalue(struct cmd_tbl *cmdtp, int flag, >> int argc, >> } >> >> static struct cmd_tbl cmd_avb[] = { >> - U_BOOT_CMD_MKENT(init, 2, 0, do_avb_init, "", ""), >> + U_BOOT_CMD_MKENT(init, 3, 0, do_avb_init, "", ""), >> U_BOOT_CMD_MKENT(read_rb, 2, 0, do_avb_read_rb, "", ""), >> U_BOOT_CMD_MKENT(write_rb, 3, 0, do_avb_write_rb, "", ""), >> U_BOOT_CMD_MKENT(is_unlocked, 1, 0, do_avb_is_unlocked, "", ""), >> @@ -464,7 +475,8 @@ static int do_avb(struct cmd_tbl *cmdtp, int flag, int >> argc, char *const argv[]) >> U_BOOT_CMD( >> avb, 29, 0, do_avb, >> "Provides commands for testing Android Verified Boot 2.0 >> functionality", >> - "init <dev> - initialize avb2 for <dev>\n" >> + "init [<interface>] <dev> - initialize avb2 for hex <dev> on >> <interface>\n" >> + " (mmc, nvme, usb, ...; defaults to mmc)\n" >> "avb read_rb <num> - read rollback index at location <num>\n" >> "avb write_rb <num> <rb> - write rollback index <rb> to <num>\n" >> "avb is_unlocked - returns unlock status of the device\n" >> diff --git a/common/Kconfig b/common/Kconfig >> index c739225621..5590fbfdb2 100644 >> --- a/common/Kconfig >> +++ b/common/Kconfig >> @@ -885,9 +885,14 @@ config AVB_VERIFY >> This option enables compilation of bootloader-dependent operations, >> used by Android Verified Boot 2.0 library (libavb). Includes: >> * Helpers to process strings in order to build OS bootargs. >> - * Helpers to access MMC, similar to drivers/fastboot/fb_mmc.c. >> + * Helpers to access block devices, similar to >> + drivers/fastboot/fb_mmc.c. >> * Helpers to alloc/init/free avb ops. >> >> + Only mmc is initialised by AVB itself. Any other interface must >> + already be enumerated when AVB runs, either by the board code or by >> + a command such as "usb start". >> + >> if AVB_VERIFY >> >> config AVB_BUF_ADDR >> diff --git a/common/avb_verify.c b/common/avb_verify.c >> index fc29433c3c..b898ebb78f 100644 >> --- a/common/avb_verify.c >> +++ b/common/avb_verify.c >> @@ -8,6 +8,7 @@ >> #include <cpu_func.h> >> #include <image.h> >> #include <malloc.h> >> +#include <mmc.h> >> #include <part.h> >> #include <tee.h> >> #include <tee/optee_ta_avb.h> >> @@ -386,44 +387,68 @@ static unsigned long avb_blk_write(struct avb_blk_part >> *part, lbaint_t start, >> start, sectors, tmp_buf); >> } >> >> -static struct avb_blk_part *get_partition(AvbOps *ops, const char >> *partition) >> +static int avb_init_mmc(int dev_num) >> { >> + struct mmc *mmc; >> int ret; >> - u8 dev_num; >> - int part_num = 0; >> - struct avb_blk_part *part; >> - struct blk_desc *blk; >> - >> - part = malloc(sizeof(struct avb_blk_part)); >> - if (!part) >> - return NULL; >> >> - dev_num = get_boot_device(ops); >> - part->mmc = find_mmc_device(dev_num); >> - if (!part->mmc) { >> + mmc = find_mmc_device(dev_num); >> + if (!mmc) { >> printf("%s: no MMC device at slot %x\n", __func__, dev_num); >> - goto err; >> + return -ENODEV; >> } >> >> - ret = mmc_init(part->mmc); >> + ret = mmc_init(mmc); >> if (ret) { >> printf("%s: MMC initialization failed, err = %d\n", >> __func__, ret); >> - goto err; >> + return ret; >> } >> >> - if (IS_MMC(part->mmc)) { >> - ret = mmc_switch_part(part->mmc, part_num); >> + if (IS_MMC(mmc)) { >> + /* Select the user area, where the GPT and our images live */ >> + ret = mmc_switch_part(mmc, 0); >> if (ret) { >> printf("%s: MMC part switch failed, err = %d\n", >> __func__, ret); >> - goto err; >> + return ret; >> } >> } >> >> - blk = mmc_get_blk_desc(part->mmc); >> + return 0; >> +} >> + >> +static struct avb_blk_part *get_partition(AvbOps *ops, const char >> *partition) >> +{ >> + enum uclass_id uclass_id; >> + struct avb_blk_part *part; >> + struct blk_desc *blk; >> + int dev_num; >> + int ret; >> + >> + part = malloc(sizeof(struct avb_blk_part)); >> + if (!part) >> + return NULL; >> + >> + dev_num = get_boot_device(ops); >> + uclass_id = get_boot_interface(ops); >> + if (uclass_id == UCLASS_INVALID || dev_num < 0) { >> + printf("%s: no storage interface, run 'avb init' first\n", >> + __func__); >> + goto err; >> + } >> + >> + if (uclass_id == UCLASS_MMC) { >> + ret = avb_init_mmc(dev_num); >> + if (ret) >> + goto err; >> + } >> + >> + blk = blk_get_devnum_by_uclass_id(uclass_id, dev_num); >> if (!blk) { >> - printf("%s: failed to obtain block descriptor\n", __func__); >> + /* Every bus but mmc must already be started when AVB runs */ >> + printf("%s: no %s device %d, is the bus started?\n", >> __func__, >> + blk_get_uclass_name(uclass_id), dev_num); > > A note for the doc/Kconfig text rather than the code: with the images on > virtio/NVMe/USB and the AVB TA keeping its state in eMMC RPMB, the TA's > first RPMB request becomes the first eMMC access and happens from inside > the supplicant RPC. > > On next that hangs: "avb init virtio 0; avb read_rb 0" > leaves OP-TEE spinning in do_notif, the nested RPMB announcement deadlock > fixed by "optee: do not announce RPMB from within a supplicant RPC" > (currently v2 on the list). Not a bug in this series, but that > configuration only works with that fix, so it may be worth a line in > avb2.rst. > >> goto err; >> } >> >> @@ -433,7 +458,6 @@ static struct avb_blk_part *get_partition(AvbOps *ops, >> const char *partition) >> goto err; >> } >> >> - part->dev_num = dev_num; >> part->blk = blk; >> >> return part; >> @@ -1111,7 +1135,7 @@ __weak AvbIOResult avb_read_root_key_digest(AvbOps >> *ops, uint8_t *digest) >> * AVB2.0 AvbOps alloc/initialisation/free >> * >> ============================================================================ >> */ >> -AvbOps *avb_ops_alloc(int boot_device) >> +AvbOps *avb_ops_alloc(enum uclass_id uclass_id, int dev_num) >> { >> struct AvbOpsData *ops_data; >> >> @@ -1134,7 +1158,8 @@ AvbOps *avb_ops_alloc(int boot_device) >> ops_data->ops.read_persistent_value = read_persistent_value; >> #endif >> ops_data->ops.get_size_of_partition = get_size_of_partition; >> - ops_data->mmc_dev = boot_device; >> + ops_data->uclass_id = uclass_id; >> + ops_data->dev_num = dev_num; >> >> return &ops_data->ops; >> } >> diff --git a/doc/android/avb2.rst b/doc/android/avb2.rst >> index a514701241..cef49d3e0d 100644 >> --- a/doc/android/avb2.rst >> +++ b/doc/android/avb2.rst >> @@ -29,6 +29,10 @@ integration in ``common/avb_verify.c`` and the platform >> port under >> >> For additional details check [1]_. >> >> +Only mmc is started by AVB itself. Any other interface must already be >> +enumerated when AVB runs, either by the board code or by a command such as >> +``usb start``, or the partition lookup fails. >> + >> AVB using OP-TEE (optional) >> ^^^^^^^^^^^^^^^^^^^^^^^^^^^ >> >> @@ -80,7 +84,8 @@ AVB 2.0 U-Boot shell commands >> Provides CLI interface to invoke AVB 2.0 verification + misc. commands for >> different testing purposes:: >> >> - avb init <dev> - initialize avb 2 for <dev> >> + avb init [<interface>] <dev> - initialize avb 2 for hex <dev> on >> + <interface> (mmc, nvme, usb, ...; defaults to mmc) > > This is the behaviour change that bites, see the test comment at the end: > the lookup probes the device (blk_get_device() -> device_probe(), and for > mmc the blk probe runs mmc_init()), whereas "avb init" used to only store > the number. Please at least state that it probes now. > >> avb read_rb <num> - read rollback index at location <num> >> avb write_rb <num> <rb> - write rollback index <rb> to <num> >> avb is_unlocked - returns unlock status of the device >> diff --git a/include/avb_verify.h b/include/avb_verify.h >> index 8c97adbfbf..6dbbb39a8a 100644 >> --- a/include/avb_verify.h >> +++ b/include/avb_verify.h >> @@ -7,8 +7,9 @@ >> #define _AVB_VERIFY_H >> >> #include <../lib/libavb/libavb.h> >> +#include <blk.h> >> #include <mapmem.h> >> -#include <mmc.h> >> +#include <part.h> >> >> #define AVB_MAX_ARGS 1024 >> #define VERITY_TABLE_OPT_RESTART "restart_on_corruption" >> @@ -24,7 +25,8 @@ enum avb_boot_state { >> >> struct AvbOpsData { >> struct AvbOps ops; >> - int mmc_dev; >> + enum uclass_id uclass_id; >> + int dev_num; >> enum avb_boot_state boot_state; >> #ifdef CONFIG_OPTEE_TA_AVB >> struct udevice *tee; >> @@ -33,8 +35,6 @@ struct AvbOpsData { >> }; >> >> struct avb_blk_part { >> - int dev_num; >> - struct mmc *mmc; >> struct blk_desc *blk; >> struct disk_partition info; >> }; >> @@ -44,7 +44,7 @@ enum avb_io_type { >> AVB_IO_WRITE >> }; >> >> -AvbOps *avb_ops_alloc(int boot_device); >> +AvbOps *avb_ops_alloc(enum uclass_id uclass_id, int dev_num); >> void avb_ops_free(AvbOps *ops); >> >> /** >> @@ -110,10 +110,24 @@ static inline int get_boot_device(AvbOps *ops) >> if (ops) { >> data = ops->user_data; >> if (data) >> - return data->mmc_dev; >> + return data->dev_num; >> } >> >> return -1; >> } >> >> +/* Mirrors get_boot_device(): no ops means no interface, not a default one >> */ >> +static inline enum uclass_id get_boot_interface(AvbOps *ops) >> +{ >> + struct AvbOpsData *data; >> + >> + if (ops) { >> + data = ops->user_data; >> + if (data) >> + return data->uclass_id; >> + } >> + >> + return UCLASS_INVALID; >> +} >> + >> #endif /* _AVB_VERIFY_H */ >> diff --git a/test/py/tests/test_android/test_avb.py >> b/test/py/tests/test_android/test_avb.py >> index 137d83e1de..4c5b3c6f59 100644 >> --- a/test/py/tests/test_android/test_avb.py >> +++ b/test/py/tests/test_android/test_avb.py >> @@ -135,3 +135,31 @@ def test_avb_persistent_values(ubman): >> >> response = ubman.run_command('avb read_pvalue test 12') >> assert response == 'Read 12 bytes, value = value_value' >> + >> + >> [email protected]('cmd_avb') >> [email protected]('cmd_mmc') >> +def test_avb_init_interface(ubman): >> + """Check that 'avb init' accepts an explicit interface >> + """ >> + >> + response = ubman.run_command('avb init mmc %s' % str(mmc_dev)) >> + assert response == '' > > Fails on sandbox for the same mmc1.img reason, see above. > > With the "avb init" behaviour sorted out, the rest works for me: > * "avb init 0" as the first eMMC access, read_rb, is_unlocked and > "avb verify _a" pass; > * "avb init nvme 0", "avb init mmc 5" and "avb init bogus 0" fail cleanly; > * with the Android images on virtio only (the eMMC holding just RPMB), > "avb init virtio 0; avb verify _a" passes, and "avb init 0" on the > empty eMMC correctly fails to find vbmeta_a; > >> + >> + response = ubman.run_command('avb init %s' % str(mmc_dev)) >> + assert response == '' >> + >> + >> [email protected]('cmd_avb') >> +def test_avb_init_bad_device(ubman): >> + """Check that 'avb init' rejects a device that does not exist >> + """ >> + >> + response = ubman.run_command('avb init mmc ff') >> + assert response.find("Can't find the requested device") != -1 >> + >> + response = ubman.run_command('avb init ff') >> + assert response.find("Can't find the requested device") != -1 >> + >> + response = ubman.run_command('avb init nonsense 0') >> + assert response.find("Can't find the requested device") != -1 >> -- >> 2.43.0 >>
