[PATCH] avb: Extend support to non-eMMC interfaces
Mattijs Korpershoek
mkorpershoek at baylibre.com
Mon Oct 10 16:52:58 CEST 2022
Hi Alistair,
Thank you for your patch.
On lun., sept. 26, 2022 at 22:02, Alistair Delva <adelva at google.com> wrote:
> From: Jiyong Park <jiyong at google.com>
>
> Previously Android AVB supported block devices only on eMMC. This change
> eliminates the restriction by using the generic block driver model.
>
> The `avb init' command is modified to accept another parameter which
> specifies the interface type. e.g., `avb init virtio 0' initializes
> avb for the first (0) disk that is accessible via the virtio interface.
>
> [adelva: The "avb init" command is updated directly, as this is
> considered a "debug command" that can't be usefully used in u-boot
> scripts.]
It seems that:
* include/configs/ti_omap5_common.h
* include/configs/meson64_android.h
* configs/total_compute_defconfig
All call "avb init"
Aren't we breaking these boot scripts with this change?
Since all of them used mmc before, it should be trivial to patch these
environments as well.
If you do so, please cc me in next version so I can give this a try on
the khadas vim3l board.
Maybe those devices are doing the wrong thing though. since this is
considered a debug command I imagine we should not be calling this at
all.
If so, do you have any alternatives in mind?
>
> Signed-off-by: Alistair Delva <adelva at google.com>
> Cc: Igor Opaniuk <igor.opaniuk at gmail.com>
> Cc: Ram Muthiah <rammuthiah at google.com>
> Cc: Jiyong Park <jiyong at google.com>
> Cc: Simon Glass <sjg at chromium.org>
> ---
> cmd/avb.c | 16 ++++---
> common/Kconfig | 1 -
> common/avb_verify.c | 105 +++++++++++++++++++++----------------------
> include/avb_verify.h | 31 ++++---------
Should we also update the documentation in doc/android/avb2.rst ?
I also think this might break:
test/py/tests/test_android/test_avb.py
> 4 files changed, 69 insertions(+), 84 deletions(-)
>
> diff --git a/cmd/avb.c b/cmd/avb.c
> index 783f51b816..8bffe49011 100644
> --- a/cmd/avb.c
> +++ b/cmd/avb.c
> @@ -10,24 +10,25 @@
> #include <env.h>
> #include <image.h>
> #include <malloc.h>
> -#include <mmc.h>
>
> #define AVB_BOOTARGS "avb_bootargs"
> static struct AvbOps *avb_ops;
>
> int do_avb_init(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
> {
> - unsigned long mmc_dev;
> + const char *iface;
> + const char *devnum;
>
> - if (argc != 2)
> + if (argc != 3)
> return CMD_RET_USAGE;
>
> - mmc_dev = hextoul(argv[1], NULL);
> + iface = argv[1];
> + devnum = argv[2];
>
> if (avb_ops)
> avb_ops_free(avb_ops);
>
> - avb_ops = avb_ops_alloc(mmc_dev);
> + avb_ops = avb_ops_alloc(iface, devnum);
> if (avb_ops)
> return CMD_RET_SUCCESS;
>
> @@ -419,7 +420,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, "", ""),
> @@ -455,7 +456,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> <devnum> - initialize avb2 for the disk <devnum> which\n"
> + " is on the interface <interface>\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 ebee856e56..a66060767c 100644
> --- a/common/Kconfig
> +++ b/common/Kconfig
> @@ -703,7 +703,6 @@ config HASH
> config AVB_VERIFY
> bool "Build Android Verified Boot operations"
> depends on LIBAVB
> - depends on MMC
> depends on PARTITION_UUIDS
> help
> This option enables compilation of bootloader-dependent operations,
> diff --git a/common/avb_verify.c b/common/avb_verify.c
> index 0520a71455..d30bbb5726 100644
> --- a/common/avb_verify.c
> +++ b/common/avb_verify.c
> @@ -253,10 +253,10 @@ char *avb_set_enforce_verity(const char *cmdline)
>
> /**
> * ============================================================================
> - * IO(mmc) auxiliary functions
> + * IO auxiliary functions
> * ============================================================================
> */
> -static unsigned long mmc_read_and_flush(struct mmc_part *part,
> +static unsigned long blk_read_and_flush(struct avb_part *part,
> lbaint_t start,
> lbaint_t sectors,
> void *buffer)
> @@ -291,7 +291,7 @@ static unsigned long mmc_read_and_flush(struct mmc_part *part,
> tmp_buf = buffer;
> }
>
> - blks = blk_dread(part->mmc_blk,
> + blks = blk_dread(part->blk,
> start, sectors, tmp_buf);
> /* flush cache after read */
> flush_cache((ulong)tmp_buf, sectors * part->info.blksz);
> @@ -302,7 +302,7 @@ static unsigned long mmc_read_and_flush(struct mmc_part *part,
> return blks;
> }
>
> -static unsigned long mmc_write(struct mmc_part *part, lbaint_t start,
> +static unsigned long blk_write(struct avb_part *part, lbaint_t start,
> lbaint_t sectors, void *buffer)
> {
> void *tmp_buf;
> @@ -330,69 +330,59 @@ static unsigned long mmc_write(struct mmc_part *part, lbaint_t start,
> tmp_buf = buffer;
> }
>
> - return blk_dwrite(part->mmc_blk,
> + return blk_dwrite(part->blk,
> start, sectors, tmp_buf);
> }
>
> -static struct mmc_part *get_partition(AvbOps *ops, const char *partition)
> +static struct avb_part *get_partition(AvbOps *ops, const char *partition)
> {
> - int ret;
> - u8 dev_num;
> - int part_num = 0;
> - struct mmc_part *part;
> - struct blk_desc *mmc_blk;
> + struct avb_part *part;
> + struct AvbOpsData *data;
> + size_t dev_part_str_len;
> + char *dev_part_str;
>
> - part = malloc(sizeof(struct mmc_part));
> + part = malloc(sizeof(struct avb_part));
> if (!part)
> return NULL;
>
> - dev_num = get_boot_device(ops);
> - part->mmc = find_mmc_device(dev_num);
> - if (!part->mmc) {
> - printf("No MMC device at slot %x\n", dev_num);
> - goto err;
> - }
> -
> - if (mmc_init(part->mmc)) {
> - printf("MMC initialization failed\n");
> - goto err;
> - }
> + if (!ops)
> + return NULL;
>
> - ret = mmc_switch_part(part->mmc, part_num);
> - if (ret)
> - goto err;
> + data = ops->user_data;
> + if (!data)
> + return NULL;
>
> - mmc_blk = mmc_get_blk_desc(part->mmc);
> - if (!mmc_blk) {
> - printf("Error - failed to obtain block descriptor\n");
> - goto err;
> + // format is "<devnum>#<partition>\0"
> + dev_part_str_len = strlen(data->devnum) + 1 + strlen(partition) + 1;
> + dev_part_str = (char *)malloc(dev_part_str_len);
> + if (!dev_part_str) {
> + free(part);
> + return NULL;
> }
>
> - ret = part_get_info_by_name(mmc_blk, partition, &part->info);
> - if (ret < 0) {
> - printf("Can't find partition '%s'\n", partition);
> - goto err;
> + snprintf(dev_part_str, dev_part_str_len, "%s#%s", data->devnum,
> + partition);
> + if (part_get_info_by_dev_and_name_or_num(data->iface, dev_part_str,
> + &part->blk, &part->info,
> + false) < 0) {
> + free(part);
> + part = NULL;
> }
>
> - part->dev_num = dev_num;
> - part->mmc_blk = mmc_blk;
> -
> + free(dev_part_str);
> return part;
> -err:
> - free(part);
> - return NULL;
> }
>
> -static AvbIOResult mmc_byte_io(AvbOps *ops,
> +static AvbIOResult blk_byte_io(AvbOps *ops,
> const char *partition,
> s64 offset,
> size_t num_bytes,
> void *buffer,
> size_t *out_num_read,
> - enum mmc_io_type io_type)
> + enum io_type io_type)
> {
> ulong ret;
> - struct mmc_part *part;
> + struct avb_part *part;
> u64 start_offset, start_sector, sectors, residue;
> u8 *tmp_buf;
> size_t io_cnt = 0;
> @@ -425,7 +415,7 @@ static AvbIOResult mmc_byte_io(AvbOps *ops,
> }
>
> if (io_type == IO_READ) {
> - ret = mmc_read_and_flush(part,
> + ret = blk_read_and_flush(part,
> part->info.start +
> start_sector,
> 1, tmp_buf);
> @@ -442,7 +432,7 @@ static AvbIOResult mmc_byte_io(AvbOps *ops,
> tmp_buf += (start_offset % part->info.blksz);
> memcpy(buffer, (void *)tmp_buf, residue);
> } else {
> - ret = mmc_read_and_flush(part,
> + ret = blk_read_and_flush(part,
> part->info.start +
> start_sector,
> 1, tmp_buf);
> @@ -456,7 +446,7 @@ static AvbIOResult mmc_byte_io(AvbOps *ops,
> start_offset % part->info.blksz,
> buffer, residue);
>
> - ret = mmc_write(part, part->info.start +
> + ret = blk_write(part, part->info.start +
> start_sector, 1, tmp_buf);
> if (ret != 1) {
> printf("%s: write error (%ld, %lld)\n",
> @@ -474,12 +464,12 @@ static AvbIOResult mmc_byte_io(AvbOps *ops,
>
> if (sectors) {
> if (io_type == IO_READ) {
> - ret = mmc_read_and_flush(part,
> + ret = blk_read_and_flush(part,
> part->info.start +
> start_sector,
> sectors, buffer);
> } else {
> - ret = mmc_write(part,
> + ret = blk_write(part,
> part->info.start +
> start_sector,
> sectors, buffer);
> @@ -535,7 +525,7 @@ static AvbIOResult read_from_partition(AvbOps *ops,
> void *buffer,
> size_t *out_num_read)
> {
> - return mmc_byte_io(ops, partition_name, offset_from_partition,
> + return blk_byte_io(ops, partition_name, offset_from_partition,
> num_bytes, buffer, out_num_read, IO_READ);
> }
>
> @@ -562,7 +552,7 @@ static AvbIOResult write_to_partition(AvbOps *ops,
> size_t num_bytes,
> const void *buffer)
> {
> - return mmc_byte_io(ops, partition_name, offset_from_partition,
> + return blk_byte_io(ops, partition_name, offset_from_partition,
> num_bytes, (void *)buffer, NULL, IO_WRITE);
> }
>
> @@ -803,7 +793,7 @@ static AvbIOResult get_unique_guid_for_partition(AvbOps *ops,
> char *guid_buf,
> size_t guid_buf_size)
> {
> - struct mmc_part *part;
> + struct avb_part *part;
> size_t uuid_size;
>
> part = get_partition(ops, partition);
> @@ -837,7 +827,7 @@ static AvbIOResult get_size_of_partition(AvbOps *ops,
> const char *partition,
> u64 *out_size_num_bytes)
> {
> - struct mmc_part *part;
> + struct avb_part *part;
>
> if (!out_size_num_bytes)
> return AVB_IO_RESULT_ERROR_INSUFFICIENT_SPACE;
> @@ -976,7 +966,7 @@ free_name:
> * AVB2.0 AvbOps alloc/initialisation/free
> * ============================================================================
> */
> -AvbOps *avb_ops_alloc(int boot_device)
> +AvbOps *avb_ops_alloc(const char *iface, const char *devnum)
> {
> struct AvbOpsData *ops_data;
>
> @@ -999,7 +989,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->iface = avb_strdup(iface);
> + ops_data->devnum = avb_strdup(devnum);
>
> return &ops_data->ops;
> }
> @@ -1018,6 +1009,12 @@ void avb_ops_free(AvbOps *ops)
> if (ops_data->tee)
> tee_close_session(ops_data->tee, ops_data->session);
> #endif
> + if (ops_data->iface)
> + avb_free((void*)ops_data->iface);
> +
> + if (ops_data->devnum)
> + avb_free((void*)ops_data->devnum);
> +
> avb_free(ops_data);
> }
> }
> diff --git a/include/avb_verify.h b/include/avb_verify.h
> index 1e787ba666..732839f74b 100644
> --- a/include/avb_verify.h
> +++ b/include/avb_verify.h
> @@ -9,8 +9,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"
> @@ -26,7 +27,8 @@ enum avb_boot_state {
>
> struct AvbOpsData {
> struct AvbOps ops;
> - int mmc_dev;
> + const char *iface;
> + const char *devnum;
> enum avb_boot_state boot_state;
> #ifdef CONFIG_OPTEE_TA_AVB
> struct udevice *tee;
> @@ -34,19 +36,17 @@ struct AvbOpsData {
> #endif
> };
>
> -struct mmc_part {
> - int dev_num;
> - struct mmc *mmc;
> - struct blk_desc *mmc_blk;
> +struct avb_part {
> + struct blk_desc *blk;
> struct disk_partition info;
> };
>
> -enum mmc_io_type {
> +enum io_type {
> IO_READ,
> IO_WRITE
> };
>
> -AvbOps *avb_ops_alloc(int boot_device);
> +AvbOps *avb_ops_alloc(const char *iface, const char *devnum);
> void avb_ops_free(AvbOps *ops);
>
> char *avb_set_state(AvbOps *ops, enum avb_boot_state boot_state);
> @@ -60,7 +60,7 @@ char *append_cmd_line(char *cmdline_orig, char *cmdline_new);
> * I/O helper inline functions
> * ============================================================================
> */
> -static inline uint64_t calc_offset(struct mmc_part *part, int64_t offset)
> +static inline uint64_t calc_offset(struct avb_part *part, int64_t offset)
> {
> u64 part_size = part->info.size * part->info.blksz;
>
> @@ -85,17 +85,4 @@ static inline bool is_buf_unaligned(void *buffer)
> return (bool)((uintptr_t)buffer % ALLOWED_BUF_ALIGN);
> }
>
> -static inline int get_boot_device(AvbOps *ops)
> -{
> - struct AvbOpsData *data;
> -
> - if (ops) {
> - data = ops->user_data;
> - if (data)
> - return data->mmc_dev;
> - }
> -
> - return -1;
> -}
> -
> #endif /* _AVB_VERIFY_H */
> --
> 2.30.2
More information about the U-Boot
mailing list