On Wed, Oct 14, 2026 at 09:49, Mattijs Korpershoek <[email protected]> wrote:
> 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. I've had a look. It's not impossible to do so, but the avb test as of today are buggy: some tests, such as test_avb_verify() use assert response.find(success_str) which is wrong. We should use 'assert response.find(success_str) != -1'. I will send a fix for this, but that also requires modifying the tests and actually providing a valid mmc.img that contains a vbmeta partition and a boot partition. Since the above is not trivial, I'd like this patch to be reworked as Igor suggested. Jan, can you please work on a v3 which keeps the old "lazy" init ? Please also address the other (valid) remarks from Igor and the same time. Thanks! Mattijs > > 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 >>>
