Re: [PATCH v5 15/23] tools: Add mkfwumdata tool for FWU metadata image

Michal Simek
2022-06-21T12:59:31Z

On 6/21/22 12:57, Etienne Carriere wrote:
> On Thu, 9 Jun 2022 at 14:31, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>>
>> From: Masami Hiramatsu <masami.hiramatsu@linaro.org>
>>
>> Add 'mkfwumdata' tool which can generate an image of the FWU metadata
>> which is required for initializing the platform.
>>
>> Usage:
>> mkfwumdata -i NR_IMAGES -b NR_BANKS [--guid] \
>> LOCATION_UUID0,IMAGE_TYPE_UUID0,BANK0_IMAGE_UUID[,BANK1_IMAGE_UUID[,...]] \
>> LOCATION_UUID1,... \
>> IMAGE_FILE
>>
>> '-i' takes the number of images and '-b' takes the number of
>> banks. This takes lists of uuids for the images on arguments,
>> and the last argument must be the output image file name.
>>
>> '--guid' (or '-g' in short) allows user to specify the location UUID
>> and image IDs in GUID instead of UUID. This option is useful if the
>> platform uses GPT partiotion. In this case, the UUID list
>> (for an image) becomes;
>>
>> DiskGUID,ParitionTypeGUID,UniquePartitionGUID,...
>>
>> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
>> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
>> ---
>> tools/Kconfig | 9 ++
>> tools/Makefile | 4 +
>> tools/mkfwumdata.c | 298 +++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 311 insertions(+)
>> create mode 100644 tools/mkfwumdata.c
>>
>> diff --git a/tools/Kconfig b/tools/Kconfig
>> index 117c921da3..3484be99d0 100644
>> --- a/tools/Kconfig
>> +++ b/tools/Kconfig
>> @@ -98,4 +98,13 @@ config TOOLS_MKEFICAPSULE
>> optionally sign that file. If you want to enable UEFI capsule
>> update feature on your target, you certainly need this.
>>
>> +config TOOLS_MKFWUMDATA
>> + bool "Build mkfwumdata command"
>> + default y if FWU_MULTI_BANK_UPDATE
>> + help
>> + This command allows users to create a raw image of the FWU
>> + metadata for initial installation of the FWU multi bank
>> + update on the board. The installation method depends on
>> + the platform.
>> +
>> endmenu
>> diff --git a/tools/Makefile b/tools/Makefile
>> index 9f2339666a..cd39e5ff6f 100644
>> --- a/tools/Makefile
>> +++ b/tools/Makefile
>> @@ -245,6 +245,10 @@ HOSTCFLAGS_asn1_compiler.o = -idirafter $(srctree)/include
>> HOSTLDLIBS_mkeficapsule += -lgnutls -luuid
>> hostprogs-$(CONFIG_TOOLS_MKEFICAPSULE) += mkeficapsule
>>
>> +mkfwumdata-objs := mkfwumdata.o lib/crc32.o
>> +HOSTLDLIBS_mkfwumdata += -luuid
>> +hostprogs-$(CONFIG_TOOLS_MKFWUMDATA) += mkfwumdata
>> +
>> # We build some files with extra pedantic flags to try to minimize things
>> # that won't build on some weird host compiler -- though there are lots of
>> # exceptions for files that aren't complaint.
>> diff --git a/tools/mkfwumdata.c b/tools/mkfwumdata.c
>> new file mode 100644
>> index 0000000000..4eb304cae3
>> --- /dev/null
>> +++ b/tools/mkfwumdata.c
>> @@ -0,0 +1,298 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +
>> +#include <errno.h>
>> +#include <getopt.h>
>> +#include <stdio.h>
>> +#include <stdint.h>
>> +#include <stdlib.h>
>> +#include <string.h>
>> +#include <u-boot/crc.h>
>> +#include <unistd.h>
>> +#include <uuid/uuid.h>
>> +
>> +/* This will dynamically allocate the fwu_mdata */
>> +#define CONFIG_FWU_NUM_BANKS 0
>> +#define CONFIG_FWU_NUM_IMAGES_PER_BANK 0
>> +
>> +/* Since we can not include fwu.h, redefine version here. */
>> +#define FWU_MDATA_VERSION 1
>> +
>> +typedef uint8_t u8;
>> +typedef int16_t s16;
>> +typedef uint16_t u16;
>> +typedef uint32_t u32;
>> +typedef uint64_t u64;
>> +
>> +#include <fwu_mdata.h>
>> +
>> +/* TODO: Endianess conversion may be required for some arch. */
>> +
>> +static const char *opts_short = "b:i:a:gh";
>> +
>> +static struct option options[] = {
>> + {"banks", required_argument, NULL, 'b'},
>> + {"images", required_argument, NULL, 'i'},
>> + {"guid", required_argument, NULL, 'g'},
>> + {"active-bank", required_argument, NULL, 'a'},
>> + {"help", no_argument, NULL, 'h'},
>> + {NULL, 0, NULL, 0},
>> +};
>> +
>> +static void print_usage(void)
>> +{
>> + fprintf(stderr, "Usage: mkfwumdata [options] <UUIDs list> [<UUIDs list>...] \n");
>> + fprintf(stderr, "Options:\n"
>> + "\t-i, --images <num> Number of images\n"
>> + "\t-b, --banks <num> Number of banks\n"
>> + "\t-a, --active-bank <num> Active bank\n"
>> + "\t-g, --guid Use GUID instead of UUID\n"
>> + "\t-h, --help print a help message\n"
>> + );
>> + fprintf(stderr, "UUIDs list syntax:\n"
>> + "\t<location uuid>,<image type uuid>,<image uuid>[,<image uuid>]\n"
>> + "\n\tYou must specify # of banks of image-uuid and # of images of the lists.\n"
>
> It's not really explicit how many ',<image uuid>' occurrences are needed.
> Maybe:
> In a <UUIDs list> item, there must be as many <image uuid>
> occurrences as the given number of banks.
> There must be as many <UUIDs list> items as the given number of images.

+1 on this.
I was trying 2 bank 2 image configuration and it is not clear how it should be
specified. Better description would be good.

Thanks,
Michal