[4/8] cmd: avb2.0: avb command for performing verification

Enable a "avb" command to execute Android Verified
Boot 2.0 operations. It includes such subcommands:
  avb init - initialize avb2 subsystem
  avb read_rb - read rollback index
  avb write_rb - write rollback index
  avb is_unlocked - check device lock state
  avb get_uuid - read and print uuid of a partition
  avb read_part - read data from partition
  avb read_part_hex - read data from partition and output to stdout
  avb write_part - write data to partition
  avb verify - run full verification chain

Signed-off-by: Igor Opaniuk <igor.opaniuk@linaro.org>
---
 cmd/Kconfig  |  15 +++
 cmd/Makefile |   3 +
 cmd/avb.c    | 351 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 369 insertions(+)
 create mode 100644 cmd/avb.c

On 25 April 2018 at 16:18, Igor Opaniuk <igor.opaniuk@linaro.org> wrote:
> Enable a "avb" command to execute Android Verified
> Boot 2.0 operations. It includes such subcommands:
>   avb init - initialize avb2 subsystem
>   avb read_rb - read rollback index
>   avb write_rb - write rollback index
>   avb is_unlocked - check device lock state
>   avb get_uuid - read and print uuid of a partition
>   avb read_part - read data from partition
>   avb read_part_hex - read data from partition and output to stdout
>   avb write_part - write data to partition
>   avb verify - run full verification chain
>
> Signed-off-by: Igor Opaniuk <igor.opaniuk@linaro.org>
> ---
>  cmd/Kconfig  |  15 +++
>  cmd/Makefile |   3 +
>  cmd/avb.c    | 351 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 369 insertions(+)
>  create mode 100644 cmd/avb.c
>
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index bc1d2f3..96695ff 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -1675,6 +1675,21 @@ config CMD_TRACE
>           for analsys (e.g. using bootchart). See doc/README.trace for full
>           details.
>
> +config CMD_AVB
> +       bool "avb - Android Verified Boot 2.0 operations"
> +       depends on LIBAVB_AB
> +       help
> +         Enables a "avb" command to perform verification of partitions using
> +         Android Verified Boot 2.0 functionality. It includes such subcommands:
> +           avb init - initialize avb2 subsystem
> +           avb read_rb - read rollback index
> +           avb write_rb - write rollback index
> +           avb is_unlocked - check device lock state
> +           avb get_uuid - read and print uuid of a partition
> +           avb read_part - read data from partition
> +           avb read_part_hex - read data from partition and output to stdout
> +           avb write_part - write data to partition
> +           avb verify - run full verification chain
>  endmenu
>
>  config CMD_UBI
> diff --git a/cmd/Makefile b/cmd/Makefile
> index c4269ac..bbf6c2a 100644
> --- a/cmd/Makefile
> +++ b/cmd/Makefile
> @@ -151,6 +151,9 @@ obj-$(CONFIG_CMD_REGULATOR) += regulator.o
>
>  obj-$(CONFIG_CMD_BLOB) += blob.o
>
> +# Android Verified Boot 2.0
> +obj-$(CONFIG_CMD_AVB) += avb.o
> +
>  obj-$(CONFIG_X86) += x86/
>  endif # !CONFIG_SPL_BUILD
>
> diff --git a/cmd/avb.c b/cmd/avb.c
> new file mode 100644
> index 0000000..d040906
> --- /dev/null
> +++ b/cmd/avb.c
> @@ -0,0 +1,351 @@
> +
> +/*
> + * (C) Copyright 2018, Linaro Limited
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + */
> +
> +#include <avb_verify.h>
> +#include <command.h>
> +#include <image.h>
> +#include <malloc.h>
> +#include <mmc.h>
> +
> +#define AVB_BOOTARGS   "avb_bootargs"
> +static struct AvbOps *avb_ops;
> +
> +static const char * const requested_partitions[] = {"boot",
> +                                            "system",
> +                                            "vendor",
> +                                            NULL};
> +
> +int do_avb_init(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> +{
> +       unsigned long mmc_dev;
> +
> +       if (argc != 2)
> +               return CMD_RET_USAGE;
> +
> +       mmc_dev = simple_strtoul(argv[1], NULL, 16);
> +
> +       if (avb_ops)
> +               avb_ops_free(avb_ops);
> +
> +       avb_ops = avb_ops_alloc(mmc_dev);
> +       if (avb_ops)
> +               return CMD_RET_SUCCESS;
> +
> +       return CMD_RET_FAILURE;
> +}
> +
> +int do_avb_read_part(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> +{
> +       const char *part;
> +       s64 offset;
> +       size_t bytes, bytes_read = 0;
> +       void *buffer;
> +
> +       if (!avb_ops) {
> +               printf("AVB 2.0 is not initialized, please run 'avb init'\n");
> +               return CMD_RET_USAGE;
> +       }
> +
> +       if (argc != 5)
> +               return CMD_RET_USAGE;
> +
> +       part = argv[1];
> +       offset = simple_strtoul(argv[2], NULL, 16);
> +       bytes = simple_strtoul(argv[3], NULL, 16);
> +       buffer = (void *)simple_strtoul(argv[4], NULL, 16);

+ Simon Glass

AFAIU, to make it possible to run this command on "sandbox", you
should use map_sysmem() and friends.

> +
> +       if (avb_ops->read_from_partition(avb_ops, part, offset, bytes,
> +                                        buffer, &bytes_read) ==
> +                                        AVB_IO_RESULT_OK) {
> +               printf("Read %zu bytes\n", bytes_read);
> +               return CMD_RET_SUCCESS;
> +       }
> +
> +       return CMD_RET_FAILURE;
> +}
> +
> +int do_avb_read_part_hex(cmd_tbl_t *cmdtp, int flag, int argc,
> +                        char *const argv[])
> +{
> +       const char *part;
> +       s64 offset;
> +       size_t bytes, bytes_read = 0;
> +       char *buffer;
> +
> +       if (!avb_ops) {
> +               printf("AVB 2.0 is not initialized, please run 'avb init'\n");
> +               return CMD_RET_USAGE;
> +       }
> +
> +       if (argc != 4)
> +               return CMD_RET_USAGE;
> +
> +       part = argv[1];
> +       offset = simple_strtoul(argv[2], NULL, 16);
> +       bytes = simple_strtoul(argv[3], NULL, 16);
> +
> +       buffer = malloc(bytes);
> +       if (!buffer) {
> +               printf("Failed to tlb_allocate buffer for data\n");
> +               return CMD_RET_FAILURE;
> +       }
> +       memset(buffer, 0, bytes);
> +
> +       if (avb_ops->read_from_partition(avb_ops, part, offset, bytes, buffer,
> +                                        &bytes_read) == AVB_IO_RESULT_OK) {
> +               printf("Requested %zu, read %zu bytes\n", bytes, bytes_read);
> +               printf("Data: ");
> +               for (int i = 0; i < bytes_read; i++)
> +                       printf("%02X", buffer[i]);
> +
> +               printf("\n");
> +
> +               free(buffer);
> +               return CMD_RET_SUCCESS;
> +       }
> +
> +       free(buffer);
> +       return CMD_RET_FAILURE;
> +}
> +
> +int do_avb_write_part(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> +{
> +       const char *part;
> +       s64 offset;
> +       size_t bytes;
> +       void *buffer;
> +
> +       if (!avb_ops) {
> +               printf("AVB 2.0 is not initialized, run 'avb init' first\n");
> +               return CMD_RET_FAILURE;
> +       }
> +
> +       if (argc != 5)
> +               return CMD_RET_USAGE;
> +
> +       part = argv[1];
> +       offset = simple_strtoul(argv[2], NULL, 16);
> +       bytes = simple_strtoul(argv[3], NULL, 16);
> +       buffer = (void *)simple_strtoul(argv[4], NULL, 16);
> +
> +       if (avb_ops->write_to_partition(avb_ops, part, offset, bytes, buffer) ==
> +           AVB_IO_RESULT_OK) {
> +               printf("Wrote %zu bytes\n", bytes);
> +               return CMD_RET_SUCCESS;
> +       }
> +
> +       return CMD_RET_FAILURE;
> +}
> +
> +int do_avb_read_rb(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> +{
> +       size_t index;
> +       u64 rb_idx;
> +
> +       if (!avb_ops) {
> +               printf("AVB 2.0 is not initialized, run 'avb init' first\n");
> +               return CMD_RET_FAILURE;
> +       }
> +
> +       if (argc != 2)
> +               return CMD_RET_USAGE;
> +
> +       index = (size_t)simple_strtoul(argv[1], NULL, 16);
> +
> +       if (avb_ops->read_rollback_index(avb_ops, index, &rb_idx) ==
> +           AVB_IO_RESULT_OK) {
> +               printf("Rollback index: %llu\n", rb_idx);
> +               return CMD_RET_SUCCESS;
> +       }
> +       return CMD_RET_FAILURE;
> +}
> +
> +int do_avb_write_rb(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> +{
> +       size_t index;
> +       u64 rb_idx;
> +
> +       if (!avb_ops) {
> +               printf("AVB 2.0 is not initialized, run 'avb init' first\n");
> +               return CMD_RET_FAILURE;
> +       }
> +
> +       if (argc != 3)
> +               return CMD_RET_USAGE;
> +
> +       index = (size_t)simple_strtoul(argv[1], NULL, 16);
> +       rb_idx = simple_strtoul(argv[2], NULL, 16);
> +
> +       if (avb_ops->write_rollback_index(avb_ops, index, rb_idx) ==
> +           AVB_IO_RESULT_OK)
> +               return CMD_RET_SUCCESS;
> +
> +       return CMD_RET_FAILURE;
> +}
> +
> +int do_avb_get_uuid(cmd_tbl_t *cmdtp, int flag,
> +                   int argc, char * const argv[])
> +{
> +       const char *part;
> +       char buffer[UUID_STR_LEN + 1];
> +
> +       if (!avb_ops) {
> +               printf("AVB 2.0 is not initialized, run 'avb init' first\n");
> +               return CMD_RET_FAILURE;
> +       }
> +
> +       if (argc != 2)
> +               return CMD_RET_USAGE;
> +
> +       part = argv[1];
> +
> +       if (avb_ops->get_unique_guid_for_partition(avb_ops, part, buffer,
> +                                                  UUID_STR_LEN + 1) ==
> +                                                  AVB_IO_RESULT_OK) {
> +               printf("'%s' UUID: %s\n", part, buffer);
> +               return CMD_RET_SUCCESS;
> +       }
> +
> +       return CMD_RET_FAILURE;
> +}
> +
> +int do_avb_verify_part(cmd_tbl_t *cmdtp, int flag,
> +                      int argc, char *const argv[])
> +{
> +       AvbSlotVerifyResult slot_result;
> +       AvbSlotVerifyData *out_data;
> +
> +       bool unlocked = false;
> +       int res = CMD_RET_FAILURE;
> +
> +       if (!avb_ops) {
> +               printf("AVB 2.0 is not initialized, run 'avb init' first\n");
> +               return CMD_RET_FAILURE;
> +       }
> +
> +       if (argc != 1)
> +               return CMD_RET_USAGE;
> +
> +       printf("## Android Verified Boot 2.0 version %s\n",
> +              avb_version_string());
> +
> +       if (avb_ops->read_is_device_unlocked(avb_ops, &unlocked) !=
> +           AVB_IO_RESULT_OK) {
> +               printf("Can't determine device lock state.\n");
> +               return CMD_RET_FAILURE;
> +       }
> +
> +       slot_result = avb_slot_verify(avb_ops, requested_partitions,
> +                                     "", unlocked, &out_data);
> +       switch (slot_result) {
> +       case AVB_SLOT_VERIFY_RESULT_OK:
> +               printf("Verification passed successfully\n");
> +
> +               /* export additional bootargs to AVB_BOOTARGS env var */
> +               env_set(AVB_BOOTARGS, out_data->cmdline);
> +
> +               res = CMD_RET_SUCCESS;
> +               break;
> +       case AVB_SLOT_VERIFY_RESULT_ERROR_VERIFICATION:
> +               printf("Verification failed\n");
> +               break;
> +       case AVB_SLOT_VERIFY_RESULT_ERROR_IO:
> +               printf("I/O error occurred during verification\n");
> +               break;
> +       case AVB_SLOT_VERIFY_RESULT_ERROR_OOM:
> +               printf("OOM error occurred during verification\n");
> +               break;
> +       case AVB_SLOT_VERIFY_RESULT_ERROR_INVALID_METADATA:
> +               printf("Corrupted dm-verity metadata detected\n");
> +               break;
> +       case AVB_SLOT_VERIFY_RESULT_ERROR_UNSUPPORTED_VERSION:
> +               printf("Unsupported version avbtool was used\n");
> +               break;
> +       case AVB_SLOT_VERIFY_RESULT_ERROR_ROLLBACK_INDEX:
> +               printf("Checking rollback index failed\n");
> +               break;
> +       case AVB_SLOT_VERIFY_RESULT_ERROR_PUBLIC_KEY_REJECTED:
> +               printf("Public key was rejected\n");
> +               break;
> +       default:
> +               printf("Unknown error occurred\n");
> +       }
> +
> +       return res;
> +}
> +
> +int do_avb_is_unlocked(cmd_tbl_t *cmdtp, int flag,
> +                      int argc, char * const argv[])
> +{
> +       bool unlock;
> +
> +       if (!avb_ops) {
> +               printf("AVB not initialized, run 'avb init' first\n");
> +               return CMD_RET_FAILURE;
> +       }
> +
> +       if (argc != 1) {
> +               printf("--%s(-1)\n", __func__);
> +               return CMD_RET_USAGE;
> +       }
> +
> +       if (avb_ops->read_is_device_unlocked(avb_ops, &unlock) ==
> +           AVB_IO_RESULT_OK) {
> +               printf("Unlocked = %d\n", unlock);
> +               return CMD_RET_SUCCESS;
> +       }
> +
> +       return CMD_RET_FAILURE;
> +}
> +
> +static cmd_tbl_t cmd_avb[] = {
> +       U_BOOT_CMD_MKENT(init, 2, 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, "", ""),
> +       U_BOOT_CMD_MKENT(get_uuid, 2, 0, do_avb_get_uuid, "", ""),
> +       U_BOOT_CMD_MKENT(read_part, 5, 0, do_avb_read_part, "", ""),
> +       U_BOOT_CMD_MKENT(read_part_hex, 4, 0, do_avb_read_part_hex, "", ""),
> +       U_BOOT_CMD_MKENT(write_part, 5, 0, do_avb_write_part, "", ""),
> +       U_BOOT_CMD_MKENT(verify, 1, 0, do_avb_verify_part, "", ""),
> +};
> +
> +static int do_avb(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> +{
> +       cmd_tbl_t *cp;
> +
> +       cp = find_cmd_tbl(argv[1], cmd_avb, ARRAY_SIZE(cmd_avb));
> +
> +       argc--;
> +       argv++;
> +
> +       if (!cp || argc > cp->maxargs)
> +               return CMD_RET_USAGE;
> +
> +       if (flag == CMD_FLAG_REPEAT)
> +               return CMD_RET_FAILURE;
> +
> +       return cp->cmd(cmdtp, flag, argc, 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"
> +       "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"
> +       "avb get_uuid <partname> - read and print uuid of partition <part>\n"
> +       "avb read_part <partname> <offset> <num> <addr> - read <num> bytes from\n"
> +       "    partition <partname> to buffer <addr>\n"
> +       "avb read_part_hex <partname> <offset> <num> - read <num> bytes from\n"
> +       "    partition <partname> and print to stdout\n"
> +       "avb write_part <partname> <offset> <num> <addr> - write <num> bytes to\n"
> +       "    <partname> by <offset> using data from <addr>\n"
> +       "avb verify - run verification process using hash data\n"
> +       "    from vbmeta structure\n"
> +       );
> --
> 2.7.4
>

Hi Igor,

On 25 April 2018 at 07:18, Igor Opaniuk <igor.opaniuk@linaro.org> wrote:
> Enable a "avb" command to execute Android Verified
> Boot 2.0 operations. It includes such subcommands:
>   avb init - initialize avb2 subsystem
>   avb read_rb - read rollback index
>   avb write_rb - write rollback index
>   avb is_unlocked - check device lock state
>   avb get_uuid - read and print uuid of a partition
>   avb read_part - read data from partition
>   avb read_part_hex - read data from partition and output to stdout
>   avb write_part - write data to partition
>   avb verify - run full verification chain
>
> Signed-off-by: Igor Opaniuk <igor.opaniuk@linaro.org>
> ---
>  cmd/Kconfig  |  15 +++
>  cmd/Makefile |   3 +
>  cmd/avb.c    | 351 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 369 insertions(+)
>  create mode 100644 cmd/avb.c
>
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index bc1d2f3..96695ff 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -1675,6 +1675,21 @@ config CMD_TRACE
>           for analsys (e.g. using bootchart). See doc/README.trace for full
>           details.
>
> +config CMD_AVB
> +       bool "avb - Android Verified Boot 2.0 operations"
> +       depends on LIBAVB_AB
> +       help
> +         Enables a "avb" command to perform verification of partitions using
> +         Android Verified Boot 2.0 functionality. It includes such subcommands:
> +           avb init - initialize avb2 subsystem
> +           avb read_rb - read rollback index
> +           avb write_rb - write rollback index
> +           avb is_unlocked - check device lock state
> +           avb get_uuid - read and print uuid of a partition
> +           avb read_part - read data from partition
> +           avb read_part_hex - read data from partition and output to stdout
> +           avb write_part - write data to partition
> +           avb verify - run full verification chain
>  endmenu
>
>  config CMD_UBI
> diff --git a/cmd/Makefile b/cmd/Makefile
> index c4269ac..bbf6c2a 100644
> --- a/cmd/Makefile
> +++ b/cmd/Makefile
> @@ -151,6 +151,9 @@ obj-$(CONFIG_CMD_REGULATOR) += regulator.o
>
>  obj-$(CONFIG_CMD_BLOB) += blob.o
>
> +# Android Verified Boot 2.0
> +obj-$(CONFIG_CMD_AVB) += avb.o
> +
>  obj-$(CONFIG_X86) += x86/
>  endif # !CONFIG_SPL_BUILD
>
> diff --git a/cmd/avb.c b/cmd/avb.c
> new file mode 100644
> index 0000000..d040906
> --- /dev/null
> +++ b/cmd/avb.c
> @@ -0,0 +1,351 @@
> +
> +/*
> + * (C) Copyright 2018, Linaro Limited
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + */
> +
> +#include <avb_verify.h>
> +#include <command.h>
> +#include <image.h>
> +#include <malloc.h>
> +#include <mmc.h>
> +
> +#define AVB_BOOTARGS   "avb_bootargs"
> +static struct AvbOps *avb_ops;
> +
> +static const char * const requested_partitions[] = {"boot",
> +                                            "system",
> +                                            "vendor",
> +                                            NULL};
> +
> +int do_avb_init(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> +{
> +       unsigned long mmc_dev;
> +
> +       if (argc != 2)
> +               return CMD_RET_USAGE;
> +
> +       mmc_dev = simple_strtoul(argv[1], NULL, 16);
> +
> +       if (avb_ops)
> +               avb_ops_free(avb_ops);
> +
> +       avb_ops = avb_ops_alloc(mmc_dev);
> +       if (avb_ops)
> +               return CMD_RET_SUCCESS;
> +
> +       return CMD_RET_FAILURE;
> +}
> +
> +int do_avb_read_part(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> +{
> +       const char *part;
> +       s64 offset;
> +       size_t bytes, bytes_read = 0;
> +       void *buffer;
> +
> +       if (!avb_ops) {
> +               printf("AVB 2.0 is not initialized, please run 'avb init'\n");
> +               return CMD_RET_USAGE;
> +       }
> +
> +       if (argc != 5)
> +               return CMD_RET_USAGE;
> +
> +       part = argv[1];
> +       offset = simple_strtoul(argv[2], NULL, 16);
> +       bytes = simple_strtoul(argv[3], NULL, 16);
> +       buffer = (void *)simple_strtoul(argv[4], NULL, 16);
> +
> +       if (avb_ops->read_from_partition(avb_ops, part, offset, bytes,
> +                                        buffer, &bytes_read) ==
> +                                        AVB_IO_RESULT_OK) {

Please can you make sure this uses driver model, and put wrappers for
these function calls in the uclass?

Regards,
Simon

Hi Simon,

I've dug into DriverModel documentation and even created a PoC for
existing avb commands. The problem is that (maybe I missed out some
key concepts) I'm still
not sure if it makes sense to follow it driver mode in the context of
AVB 2.0 feature and
what kind of extra devices can be used within the same uclass in this case?
Could you please explain in detail.
Thanks

Hi Sam,
Thanks, will fix!


On 3 May 2018 at 05:31, Simon Glass <sjg@chromium.org> wrote:
> Hi Igor,
>
> On 25 April 2018 at 07:18, Igor Opaniuk <igor.opaniuk@linaro.org> wrote:
>> Enable a "avb" command to execute Android Verified
>> Boot 2.0 operations. It includes such subcommands:
>>   avb init - initialize avb2 subsystem
>>   avb read_rb - read rollback index
>>   avb write_rb - write rollback index
>>   avb is_unlocked - check device lock state
>>   avb get_uuid - read and print uuid of a partition
>>   avb read_part - read data from partition
>>   avb read_part_hex - read data from partition and output to stdout
>>   avb write_part - write data to partition
>>   avb verify - run full verification chain
>>
>> Signed-off-by: Igor Opaniuk <igor.opaniuk@linaro.org>
>> ---
>>  cmd/Kconfig  |  15 +++
>>  cmd/Makefile |   3 +
>>  cmd/avb.c    | 351 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 369 insertions(+)
>>  create mode 100644 cmd/avb.c
>>
>> diff --git a/cmd/Kconfig b/cmd/Kconfig
>> index bc1d2f3..96695ff 100644
>> --- a/cmd/Kconfig
>> +++ b/cmd/Kconfig
>> @@ -1675,6 +1675,21 @@ config CMD_TRACE
>>           for analsys (e.g. using bootchart). See doc/README.trace for full
>>           details.
>>
>> +config CMD_AVB
>> +       bool "avb - Android Verified Boot 2.0 operations"
>> +       depends on LIBAVB_AB
>> +       help
>> +         Enables a "avb" command to perform verification of partitions using
>> +         Android Verified Boot 2.0 functionality. It includes such subcommands:
>> +           avb init - initialize avb2 subsystem
>> +           avb read_rb - read rollback index
>> +           avb write_rb - write rollback index
>> +           avb is_unlocked - check device lock state
>> +           avb get_uuid - read and print uuid of a partition
>> +           avb read_part - read data from partition
>> +           avb read_part_hex - read data from partition and output to stdout
>> +           avb write_part - write data to partition
>> +           avb verify - run full verification chain
>>  endmenu
>>
>>  config CMD_UBI
>> diff --git a/cmd/Makefile b/cmd/Makefile
>> index c4269ac..bbf6c2a 100644
>> --- a/cmd/Makefile
>> +++ b/cmd/Makefile
>> @@ -151,6 +151,9 @@ obj-$(CONFIG_CMD_REGULATOR) += regulator.o
>>
>>  obj-$(CONFIG_CMD_BLOB) += blob.o
>>
>> +# Android Verified Boot 2.0
>> +obj-$(CONFIG_CMD_AVB) += avb.o
>> +
>>  obj-$(CONFIG_X86) += x86/
>>  endif # !CONFIG_SPL_BUILD
>>
>> diff --git a/cmd/avb.c b/cmd/avb.c
>> new file mode 100644
>> index 0000000..d040906
>> --- /dev/null
>> +++ b/cmd/avb.c
>> @@ -0,0 +1,351 @@
>> +
>> +/*
>> + * (C) Copyright 2018, Linaro Limited
>> + *
>> + * SPDX-License-Identifier:    GPL-2.0+
>> + */
>> +
>> +#include <avb_verify.h>
>> +#include <command.h>
>> +#include <image.h>
>> +#include <malloc.h>
>> +#include <mmc.h>
>> +
>> +#define AVB_BOOTARGS   "avb_bootargs"
>> +static struct AvbOps *avb_ops;
>> +
>> +static const char * const requested_partitions[] = {"boot",
>> +                                            "system",
>> +                                            "vendor",
>> +                                            NULL};
>> +
>> +int do_avb_init(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>> +{
>> +       unsigned long mmc_dev;
>> +
>> +       if (argc != 2)
>> +               return CMD_RET_USAGE;
>> +
>> +       mmc_dev = simple_strtoul(argv[1], NULL, 16);
>> +
>> +       if (avb_ops)
>> +               avb_ops_free(avb_ops);
>> +
>> +       avb_ops = avb_ops_alloc(mmc_dev);
>> +       if (avb_ops)
>> +               return CMD_RET_SUCCESS;
>> +
>> +       return CMD_RET_FAILURE;
>> +}
>> +
>> +int do_avb_read_part(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>> +{
>> +       const char *part;
>> +       s64 offset;
>> +       size_t bytes, bytes_read = 0;
>> +       void *buffer;
>> +
>> +       if (!avb_ops) {
>> +               printf("AVB 2.0 is not initialized, please run 'avb init'\n");
>> +               return CMD_RET_USAGE;
>> +       }
>> +
>> +       if (argc != 5)
>> +               return CMD_RET_USAGE;
>> +
>> +       part = argv[1];
>> +       offset = simple_strtoul(argv[2], NULL, 16);
>> +       bytes = simple_strtoul(argv[3], NULL, 16);
>> +       buffer = (void *)simple_strtoul(argv[4], NULL, 16);
>> +
>> +       if (avb_ops->read_from_partition(avb_ops, part, offset, bytes,
>> +                                        buffer, &bytes_read) ==
>> +                                        AVB_IO_RESULT_OK) {
>
> Please can you make sure this uses driver model, and put wrappers for
> these function calls in the uclass?
>
> Regards,
> Simon

Hi Igor,

On 16 May 2018 at 01:44, Igor Opaniuk <igor.opaniuk@linaro.org> wrote:
> Hi Simon,
>
> I've dug into DriverModel documentation and even created a PoC for
> existing avb commands. The problem is that (maybe I missed out some
> key concepts) I'm still
> not sure if it makes sense to follow it driver mode in the context of
> AVB 2.0 feature and
> what kind of extra devices can be used within the same uclass in this case?
> Could you please explain in detail.
> Thanks

avb_ops_alloc() is allocating a struct and then assigning operations
to its members.

This is what driver model is designed to do. It handles the
allocations and the operations become part of the uclass interface.

At present it looks like you only have one driver. I'm not sure if it
would make sense to have a second one.

As a counter example see cros_ec uclass, which does things
differently. It models the EC interface (I2C, SPI, LPC) using DM, with
just a single impl on top.

Also can you please drop the CamelCase in the patches? We don't use
that in U-Boot.

>
> Hi Sam,
> Thanks, will fix!
>
>
> On 3 May 2018 at 05:31, Simon Glass <sjg@chromium.org> wrote:
>> Hi Igor,
>>
>> On 25 April 2018 at 07:18, Igor Opaniuk <igor.opaniuk@linaro.org> wrote:
>>> Enable a "avb" command to execute Android Verified
>>> Boot 2.0 operations. It includes such subcommands:
>>>   avb init - initialize avb2 subsystem
>>>   avb read_rb - read rollback index
>>>   avb write_rb - write rollback index
>>>   avb is_unlocked - check device lock state
>>>   avb get_uuid - read and print uuid of a partition
>>>   avb read_part - read data from partition
>>>   avb read_part_hex - read data from partition and output to stdout
>>>   avb write_part - write data to partition
>>>   avb verify - run full verification chain
>>>
>>> Signed-off-by: Igor Opaniuk <igor.opaniuk@linaro.org>
>>> ---
>>>  cmd/Kconfig  |  15 +++
>>>  cmd/Makefile |   3 +
>>>  cmd/avb.c    | 351 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 369 insertions(+)
>>>  create mode 100644 cmd/avb.c
>>>
>>> diff --git a/cmd/Kconfig b/cmd/Kconfig
>>> index bc1d2f3..96695ff 100644
>>> --- a/cmd/Kconfig
>>> +++ b/cmd/Kconfig
>>> @@ -1675,6 +1675,21 @@ config CMD_TRACE
>>>           for analsys (e.g. using bootchart). See doc/README.trace for full
>>>           details.
>>>
>>> +config CMD_AVB
>>> +       bool "avb - Android Verified Boot 2.0 operations"
>>> +       depends on LIBAVB_AB
>>> +       help
>>> +         Enables a "avb" command to perform verification of partitions using
>>> +         Android Verified Boot 2.0 functionality. It includes such subcommands:
>>> +           avb init - initialize avb2 subsystem
>>> +           avb read_rb - read rollback index
>>> +           avb write_rb - write rollback index
>>> +           avb is_unlocked - check device lock state
>>> +           avb get_uuid - read and print uuid of a partition
>>> +           avb read_part - read data from partition
>>> +           avb read_part_hex - read data from partition and output to stdout
>>> +           avb write_part - write data to partition
>>> +           avb verify - run full verification chain
>>>  endmenu
>>>
>>>  config CMD_UBI
>>> diff --git a/cmd/Makefile b/cmd/Makefile
>>> index c4269ac..bbf6c2a 100644
>>> --- a/cmd/Makefile
>>> +++ b/cmd/Makefile
>>> @@ -151,6 +151,9 @@ obj-$(CONFIG_CMD_REGULATOR) += regulator.o
>>>
>>>  obj-$(CONFIG_CMD_BLOB) += blob.o
>>>
>>> +# Android Verified Boot 2.0
>>> +obj-$(CONFIG_CMD_AVB) += avb.o
>>> +
>>>  obj-$(CONFIG_X86) += x86/
>>>  endif # !CONFIG_SPL_BUILD
>>>
>>> diff --git a/cmd/avb.c b/cmd/avb.c
>>> new file mode 100644
>>> index 0000000..d040906
>>> --- /dev/null
>>> +++ b/cmd/avb.c
>>> @@ -0,0 +1,351 @@
>>> +
>>> +/*
>>> + * (C) Copyright 2018, Linaro Limited
>>> + *
>>> + * SPDX-License-Identifier:    GPL-2.0+
>>> + */
>>> +
>>> +#include <avb_verify.h>
>>> +#include <command.h>
>>> +#include <image.h>
>>> +#include <malloc.h>
>>> +#include <mmc.h>
>>> +
>>> +#define AVB_BOOTARGS   "avb_bootargs"
>>> +static struct AvbOps *avb_ops;
>>> +
>>> +static const char * const requested_partitions[] = {"boot",
>>> +                                            "system",
>>> +                                            "vendor",
>>> +                                            NULL};
>>> +
>>> +int do_avb_init(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>>> +{
>>> +       unsigned long mmc_dev;
>>> +
>>> +       if (argc != 2)
>>> +               return CMD_RET_USAGE;
>>> +
>>> +       mmc_dev = simple_strtoul(argv[1], NULL, 16);
>>> +
>>> +       if (avb_ops)
>>> +               avb_ops_free(avb_ops);
>>> +
>>> +       avb_ops = avb_ops_alloc(mmc_dev);
>>> +       if (avb_ops)
>>> +               return CMD_RET_SUCCESS;
>>> +
>>> +       return CMD_RET_FAILURE;
>>> +}
>>> +
>>> +int do_avb_read_part(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>>> +{
>>> +       const char *part;
>>> +       s64 offset;
>>> +       size_t bytes, bytes_read = 0;
>>> +       void *buffer;
>>> +
>>> +       if (!avb_ops) {
>>> +               printf("AVB 2.0 is not initialized, please run 'avb init'\n");
>>> +               return CMD_RET_USAGE;
>>> +       }
>>> +
>>> +       if (argc != 5)
>>> +               return CMD_RET_USAGE;
>>> +
>>> +       part = argv[1];
>>> +       offset = simple_strtoul(argv[2], NULL, 16);
>>> +       bytes = simple_strtoul(argv[3], NULL, 16);
>>> +       buffer = (void *)simple_strtoul(argv[4], NULL, 16);
>>> +
>>> +       if (avb_ops->read_from_partition(avb_ops, part, offset, bytes,
>>> +                                        buffer, &bytes_read) ==
>>> +                                        AVB_IO_RESULT_OK) {
>>
>> Please can you make sure this uses driver model, and put wrappers for
>> these function calls in the uclass?

Regards,
Simon

On 15 May 2018 at 19:26, Simon Glass <sjg@chromium.org> wrote:
> Hi Igor,
>
> On 16 May 2018 at 01:44, Igor Opaniuk <igor.opaniuk@linaro.org> wrote:
>> Hi Simon,
>>
>> I've dug into DriverModel documentation and even created a PoC for
>> existing avb commands. The problem is that (maybe I missed out some
>> key concepts) I'm still
>> not sure if it makes sense to follow it driver mode in the context of
>> AVB 2.0 feature and
>> what kind of extra devices can be used within the same uclass in this case?
>> Could you please explain in detail.
>> Thanks
>
> avb_ops_alloc() is allocating a struct and then assigning operations
> to its members.
>
> This is what driver model is designed to do. It handles the
> allocations and the operations become part of the uclass interface.
>
> At present it looks like you only have one driver. I'm not sure if it
> would make sense to have a second one.
>
> As a counter example see cros_ec uclass, which does things
> differently. It models the EC interface (I2C, SPI, LPC) using DM, with
> just a single impl on top.

Right, I do understand what DriverModel is and why it should
be used in the case of real-device drivers. But regarding Android Verified
Boot 2.0 feature, which introduces verification capabilities
and leverages only device-independent APIs,
I see no reason why it should be used here.
Could you please check [1] and confirm that this set of commands should
really follow this model.

> Also can you please drop the CamelCase in the patches? We don't use
> that in U-Boot.

Frankly, I don't like it also, bit all CamelCase instances in
the code relate to libavb/libavb_ab library ([2]), which is planned to be
deviated from AOSP upstream in the future.
That's why a decision was made to not introduce any changes to simplify this
process, as Google intensively introduces new changes to it.
(I've left a notice in the cover letter that checkpatch
will fail on the commit, which introduces libavb; also, there is the ongoing
discussion there regarding why libavb/libavb_ab should be
kept as it's. Please join us, if you don't mind).

Thanks!

Best regards,
Igor

[1] https://android.googlesource.com/platform/external/avb/+/master/README.md
[2] https://android.googlesource.com/platform/external/avb/
>>
>> Hi Sam,
>> Thanks, will fix!
>>
>>
>> On 3 May 2018 at 05:31, Simon Glass <sjg@chromium.org> wrote:
>>> Hi Igor,
>>>
>>> On 25 April 2018 at 07:18, Igor Opaniuk <igor.opaniuk@linaro.org> wrote:
>>>> Enable a "avb" command to execute Android Verified
>>>> Boot 2.0 operations. It includes such subcommands:
>>>>   avb init - initialize avb2 subsystem
>>>>   avb read_rb - read rollback index
>>>>   avb write_rb - write rollback index
>>>>   avb is_unlocked - check device lock state
>>>>   avb get_uuid - read and print uuid of a partition
>>>>   avb read_part - read data from partition
>>>>   avb read_part_hex - read data from partition and output to stdout
>>>>   avb write_part - write data to partition
>>>>   avb verify - run full verification chain
>>>>
>>>> Signed-off-by: Igor Opaniuk <igor.opaniuk@linaro.org>
>>>> ---
>>>>  cmd/Kconfig  |  15 +++
>>>>  cmd/Makefile |   3 +
>>>>  cmd/avb.c    | 351 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  3 files changed, 369 insertions(+)
>>>>  create mode 100644 cmd/avb.c
>>>>
>>>> diff --git a/cmd/Kconfig b/cmd/Kconfig
>>>> index bc1d2f3..96695ff 100644
>>>> --- a/cmd/Kconfig
>>>> +++ b/cmd/Kconfig
>>>> @@ -1675,6 +1675,21 @@ config CMD_TRACE
>>>>           for analsys (e.g. using bootchart). See doc/README.trace for full
>>>>           details.
>>>>
>>>> +config CMD_AVB
>>>> +       bool "avb - Android Verified Boot 2.0 operations"
>>>> +       depends on LIBAVB_AB
>>>> +       help
>>>> +         Enables a "avb" command to perform verification of partitions using
>>>> +         Android Verified Boot 2.0 functionality. It includes such subcommands:
>>>> +           avb init - initialize avb2 subsystem
>>>> +           avb read_rb - read rollback index
>>>> +           avb write_rb - write rollback index
>>>> +           avb is_unlocked - check device lock state
>>>> +           avb get_uuid - read and print uuid of a partition
>>>> +           avb read_part - read data from partition
>>>> +           avb read_part_hex - read data from partition and output to stdout
>>>> +           avb write_part - write data to partition
>>>> +           avb verify - run full verification chain
>>>>  endmenu
>>>>
>>>>  config CMD_UBI
>>>> diff --git a/cmd/Makefile b/cmd/Makefile
>>>> index c4269ac..bbf6c2a 100644
>>>> --- a/cmd/Makefile
>>>> +++ b/cmd/Makefile
>>>> @@ -151,6 +151,9 @@ obj-$(CONFIG_CMD_REGULATOR) += regulator.o
>>>>
>>>>  obj-$(CONFIG_CMD_BLOB) += blob.o
>>>>
>>>> +# Android Verified Boot 2.0
>>>> +obj-$(CONFIG_CMD_AVB) += avb.o
>>>> +
>>>>  obj-$(CONFIG_X86) += x86/
>>>>  endif # !CONFIG_SPL_BUILD
>>>>
>>>> diff --git a/cmd/avb.c b/cmd/avb.c
>>>> new file mode 100644
>>>> index 0000000..d040906
>>>> --- /dev/null
>>>> +++ b/cmd/avb.c
>>>> @@ -0,0 +1,351 @@
>>>> +
>>>> +/*
>>>> + * (C) Copyright 2018, Linaro Limited
>>>> + *
>>>> + * SPDX-License-Identifier:    GPL-2.0+
>>>> + */
>>>> +
>>>> +#include <avb_verify.h>
>>>> +#include <command.h>
>>>> +#include <image.h>
>>>> +#include <malloc.h>
>>>> +#include <mmc.h>
>>>> +
>>>> +#define AVB_BOOTARGS   "avb_bootargs"
>>>> +static struct AvbOps *avb_ops;
>>>> +
>>>> +static const char * const requested_partitions[] = {"boot",
>>>> +                                            "system",
>>>> +                                            "vendor",
>>>> +                                            NULL};
>>>> +
>>>> +int do_avb_init(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>>>> +{
>>>> +       unsigned long mmc_dev;
>>>> +
>>>> +       if (argc != 2)
>>>> +               return CMD_RET_USAGE;
>>>> +
>>>> +       mmc_dev = simple_strtoul(argv[1], NULL, 16);
>>>> +
>>>> +       if (avb_ops)
>>>> +               avb_ops_free(avb_ops);
>>>> +
>>>> +       avb_ops = avb_ops_alloc(mmc_dev);
>>>> +       if (avb_ops)
>>>> +               return CMD_RET_SUCCESS;
>>>> +
>>>> +       return CMD_RET_FAILURE;
>>>> +}
>>>> +
>>>> +int do_avb_read_part(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>>>> +{
>>>> +       const char *part;
>>>> +       s64 offset;
>>>> +       size_t bytes, bytes_read = 0;
>>>> +       void *buffer;
>>>> +
>>>> +       if (!avb_ops) {
>>>> +               printf("AVB 2.0 is not initialized, please run 'avb init'\n");
>>>> +               return CMD_RET_USAGE;
>>>> +       }
>>>> +
>>>> +       if (argc != 5)
>>>> +               return CMD_RET_USAGE;
>>>> +
>>>> +       part = argv[1];
>>>> +       offset = simple_strtoul(argv[2], NULL, 16);
>>>> +       bytes = simple_strtoul(argv[3], NULL, 16);
>>>> +       buffer = (void *)simple_strtoul(argv[4], NULL, 16);
>>>> +
>>>> +       if (avb_ops->read_from_partition(avb_ops, part, offset, bytes,
>>>> +                                        buffer, &bytes_read) ==
>>>> +                                        AVB_IO_RESULT_OK) {
>>>
>>> Please can you make sure this uses driver model, and put wrappers for
>>> these function calls in the uclass?
>
> Regards,
> Simon

(Tom can you please comment on the CamelCase question?)

Hi Igor,

On 15 May 2018 at 11:31, Igor Opaniuk <igor.opaniuk@linaro.org> wrote:
> On 15 May 2018 at 19:26, Simon Glass <sjg@chromium.org> wrote:
>> Hi Igor,
>>
>> On 16 May 2018 at 01:44, Igor Opaniuk <igor.opaniuk@linaro.org> wrote:
>>> Hi Simon,
>>>
>>> I've dug into DriverModel documentation and even created a PoC for
>>> existing avb commands. The problem is that (maybe I missed out some
>>> key concepts) I'm still
>>> not sure if it makes sense to follow it driver mode in the context of
>>> AVB 2.0 feature and
>>> what kind of extra devices can be used within the same uclass in this case?
>>> Could you please explain in detail.
>>> Thanks
>>
>> avb_ops_alloc() is allocating a struct and then assigning operations
>> to its members.
>>
>> This is what driver model is designed to do. It handles the
>> allocations and the operations become part of the uclass interface.
>>
>> At present it looks like you only have one driver. I'm not sure if it
>> would make sense to have a second one.
>>
>> As a counter example see cros_ec uclass, which does things
>> differently. It models the EC interface (I2C, SPI, LPC) using DM, with
>> just a single impl on top.
>
> Right, I do understand what DriverModel is and why it should
> be used in the case of real-device drivers. But regarding Android Verified
> Boot 2.0 feature, which introduces verification capabilities
> and leverages only device-independent APIs,
> I see no reason why it should be used here.
> Could you please check [1] and confirm that this set of commands should
> really follow this model.

If there is no need for operations and indirection through function
pointers, why not just call the functions directly?

I do think (as the code is currently structured) that DM makes sense,
even though I understand that you are not creating a device. Perhaps
Tom might have thoughts on this though?

>
>> Also can you please drop the CamelCase in the patches? We don't use
>> that in U-Boot.
>
> Frankly, I don't like it also, bit all CamelCase instances in
> the code relate to libavb/libavb_ab library ([2]), which is planned to be
> deviated from AOSP upstream in the future.
> That's why a decision was made to not introduce any changes to simplify this
> process, as Google intensively introduces new changes to it.

Tom, what do you think? It's not how things usually work, but perhaps
there is precedent for this?

> (I've left a notice in the cover letter that checkpatch
> will fail on the commit, which introduces libavb; also, there is the ongoing
> discussion there regarding why libavb/libavb_ab should be
> kept as it's. Please join us, if you don't mind).

I do see the cover letter thread but could not find the camel case
discussion. Can you please point me to it?

>
> Thanks!
>
> Best regards,
> Igor
>
> [1] https://android.googlesource.com/platform/external/avb/+/master/README.md
> [2] https://android.googlesource.com/platform/external/avb/
>>>

[..]

Regards,
Simon

Hi Simon,

> If there is no need for operations and indirection through function
> pointers, why not just call the functions directly?

This is the way how libavb is integrated into bootloader. libavb interfaces with
the bootloader through the supplied AvbOps struct. This includes operations to
read and write data from partitions, read and write rollback indexes, check
if the public key used to make a signature should be accepted, and so on.
This is what Google AVB 2.0 specification says.
For additional details please check [1].

In this particular patch, all avb commands, except the main "avb
verify", are used
to debug these particular callback implementations
(can be used when enabling this feature on new platform etc.).
The reason I use indirection through function pointers is that they
are not supposed
to be public, and are not intended to be used somehow directly, only by libavb
(check [2] how libavb is using it)

>> (I've left a notice in the cover letter that checkpatch
>> will fail on the commit, which introduces libavb; also, there is the ongoing
>> discussion there regarding why libavb/libavb_ab should be
>> kept as it's. Please join us, if you don't mind).
>
> I do see the cover letter thread but could not find the camel case
> discussion. Can you please point me to it?

There is no any particular discussion about CamelCase, but a few general
statements why I should avoid introducing any changes in
libavb/libavb_ab libraries.
Regarding our particular case, AvbSlotVerifyResult and AvbSlotVerifyData types
(I guess you're talking about these CamelCase instances) are defined inside
libavb library, so obviously I can't simply replace it with something, that
conforms to the Linux kernel coding style

Thanks

[1] https://android.googlesource.com/platform/external/avb/+/master/README.md#system-dependencies
[2] https://android.googlesource.com/platform/external/avb/+/master/libavb/avb_cmdline.c#71

On 15 May 2018 at 21:28, Simon Glass <sjg@chromium.org> wrote:
> (Tom can you please comment on the CamelCase question?)
>
> Hi Igor,
>
> On 15 May 2018 at 11:31, Igor Opaniuk <igor.opaniuk@linaro.org> wrote:
>> On 15 May 2018 at 19:26, Simon Glass <sjg@chromium.org> wrote:
>>> Hi Igor,
>>>
>>> On 16 May 2018 at 01:44, Igor Opaniuk <igor.opaniuk@linaro.org> wrote:
>>>> Hi Simon,
>>>>
>>>> I've dug into DriverModel documentation and even created a PoC for
>>>> existing avb commands. The problem is that (maybe I missed out some
>>>> key concepts) I'm still
>>>> not sure if it makes sense to follow it driver mode in the context of
>>>> AVB 2.0 feature and
>>>> what kind of extra devices can be used within the same uclass in this case?
>>>> Could you please explain in detail.
>>>> Thanks
>>>
>>> avb_ops_alloc() is allocating a struct and then assigning operations
>>> to its members.
>>>
>>> This is what driver model is designed to do. It handles the
>>> allocations and the operations become part of the uclass interface.
>>>
>>> At present it looks like you only have one driver. I'm not sure if it
>>> would make sense to have a second one.
>>>
>>> As a counter example see cros_ec uclass, which does things
>>> differently. It models the EC interface (I2C, SPI, LPC) using DM, with
>>> just a single impl on top.
>>
>> Right, I do understand what DriverModel is and why it should
>> be used in the case of real-device drivers. But regarding Android Verified
>> Boot 2.0 feature, which introduces verification capabilities
>> and leverages only device-independent APIs,
>> I see no reason why it should be used here.
>> Could you please check [1] and confirm that this set of commands should
>> really follow this model.
>
> If there is no need for operations and indirection through function
> pointers, why not just call the functions directly?
>
> I do think (as the code is currently structured) that DM makes sense,
> even though I understand that you are not creating a device. Perhaps
> Tom might have thoughts on this though?
>
>>
>>> Also can you please drop the CamelCase in the patches? We don't use
>>> that in U-Boot.
>>
>> Frankly, I don't like it also, bit all CamelCase instances in
>> the code relate to libavb/libavb_ab library ([2]), which is planned to be
>> deviated from AOSP upstream in the future.
>> That's why a decision was made to not introduce any changes to simplify this
>> process, as Google intensively introduces new changes to it.
>
> Tom, what do you think? It's not how things usually work, but perhaps
> there is precedent for this?
>
>> (I've left a notice in the cover letter that checkpatch
>> will fail on the commit, which introduces libavb; also, there is the ongoing
>> discussion there regarding why libavb/libavb_ab should be
>> kept as it's. Please join us, if you don't mind).
>
> I do see the cover letter thread but could not find the camel case
> discussion. Can you please point me to it?
>
>>
>> Thanks!
>>
>> Best regards,
>> Igor
>>
>> [1] https://android.googlesource.com/platform/external/avb/+/master/README.md
>> [2] https://android.googlesource.com/platform/external/avb/
>>>>
>
> [..]
>
> Regards,
> Simon

Hi Igor,

On 16 May 2018 at 02:20, Igor Opaniuk <igor.opaniuk@linaro.org> wrote:
> Hi Simon,
>
>> If there is no need for operations and indirection through function
>> pointers, why not just call the functions directly?
>
> This is the way how libavb is integrated into bootloader. libavb interfaces with
> the bootloader through the supplied AvbOps struct. This includes operations to
> read and write data from partitions, read and write rollback indexes, check
> if the public key used to make a signature should be accepted, and so on.
> This is what Google AVB 2.0 specification says.
> For additional details please check [1].
>
> In this particular patch, all avb commands, except the main "avb
> verify", are used
> to debug these particular callback implementations
> (can be used when enabling this feature on new platform etc.).
> The reason I use indirection through function pointers is that they
> are not supposed
> to be public, and are not intended to be used somehow directly, only by libavb
> (check [2] how libavb is using it)
>
>>> (I've left a notice in the cover letter that checkpatch
>>> will fail on the commit, which introduces libavb; also, there is the ongoing
>>> discussion there regarding why libavb/libavb_ab should be
>>> kept as it's. Please join us, if you don't mind).
>>
>> I do see the cover letter thread but could not find the camel case
>> discussion. Can you please point me to it?
>
> There is no any particular discussion about CamelCase, but a few general
> statements why I should avoid introducing any changes in
> libavb/libavb_ab libraries.
> Regarding our particular case, AvbSlotVerifyResult and AvbSlotVerifyData types
> (I guess you're talking about these CamelCase instances) are defined inside
> libavb library, so obviously I can't simply replace it with something, that
> conforms to the Linux kernel coding style

Maybe I've lost track of the patches, but I thought you were
implementing this library in U-Boot and thus can call things whatever
you like?

We have had a similar thing with EFI, but in that case there was no
reference code to use, so it was easier to just rename things to
kernel style.

In this case it seems that you want to be able to easily merge changes
back and forth between U-Boot's copy of the library and the upstream
one.

I think we have to balance that desire against the impact of having
non-conforming code in the U-Boot code base. So far we have tended to
adjust the code to fit kernel style in these situations.

Anyway I think this is Tom's call and I'm happy with whatever he decides.

[..]

Regards,
Simon