Re: [PATCHv3 5/5] tools: Add mkfwumdata tool for FWU metadata image

On 1/9/23 02:07, Jassi Brar wrote:

From: Masami Hiramatsu masami.hiramatsu@linaro.org

Add 'mkfwumdata' tool to generate FWU metadata image for the meta-data partition to be used in A/B Update imeplementation.

Signed-off-by: Masami Hiramatsu masami.hiramatsu@linaro.org Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org Signed-off-by: Jassi Brar jaswinder.singh@linaro.org

---

./scripts/checkpatch.pl --strict tools/mkfwumdata.c CHECK: 'Endianess' may be misspelled - perhaps 'Endianness'? #32: FILE: tools/mkfwumdata.c:32: +/* TODO: Endianess conversion may be required for some arch. */ ^^^^^^^^^

ERROR: do not initialise statics to 0 #68: FILE: tools/mkfwumdata.c:68: +static int active_bank = 0;

ERROR: do not initialise statics to false #70: FILE: tools/mkfwumdata.c:70: +static bool __use_guid = false;

ERROR: code indent should use tabs where possible #234: FILE: tools/mkfwumdata.c:234: + mobj->size - sizeof(uint32_t));$

CHECK: Alignment should match open parenthesis #234: FILE: tools/mkfwumdata.c:234: + mdata->crc32 = crc32(0, (const unsigned char *)&mdata->version, + mobj->size - sizeof(uint32_t));

WARNING: please, no spaces at the start of a line #234: FILE: tools/mkfwumdata.c:234: + mobj->size - sizeof(uint32_t));$

total: 3 errors, 1 warnings, 2 checks, 326 lines checked

NOTE: For some of the reported defects, checkpatch may be able to mechanically convert to the typical style using --fix or --fix-inplace.

NOTE: Whitespace errors detected. You may wish to use scripts/cleanpatch or scripts/cleanfile

tools/Kconfig | 9 ++ tools/Makefile | 4 + tools/mkfwumdata.c | 326 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 339 insertions(+) create mode 100644 tools/mkfwumdata.c

This is good but actually I can't see any example how you use this tool on your board. It would be good to list it in documentation you have written in previous patch.

diff --git a/tools/Kconfig b/tools/Kconfig index 539708f277..6e23f44d55 100644 --- a/tools/Kconfig +++ b/tools/Kconfig @@ -157,4 +157,13 @@ config LUT_SEQUENCE help Look Up Table Sequence

+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 26be0a7ba2..024d6c8eca 100644 --- a/tools/Makefile +++ b/tools/Makefile @@ -255,6 +255,10 @@ HOSTLDLIBS_mkeficapsule += \ $(shell pkg-config --libs uuid 2> /dev/null || echo "-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..e0b6702039 --- /dev/null +++ b/tools/mkfwumdata.c @@ -0,0 +1,326 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

• • Copyright (c) 2023, Linaro Limited
• */

+#include <errno.h> +#include <getopt.h> +#include <limits.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:p: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'},
• {"previous-bank", required_argument, NULL, 'p'},
• {"help", no_argument, NULL, 'h'},
• {NULL, 0, NULL, 0},

+};

+static void print_usage(void) +{

• fprintf(stderr, "Usage: mkfwumdata [options] <UUIDs list> <output file>\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-p, --previous-bank  <num>  Previous 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>,<images uuid list>\n"
  

• "\t    images uuid list syntax:\n"
  

• "\t\t    img_uuid_00,img_uuid_01...img_uuid_0b,\n"
  

• "\t\t    img_uuid_10,img_uuid_11...img_uuid_1b,\n"
  

• "\t\t    ...,\n"
  

• "\t\t    img_uuid_i0,img_uuid_i1...img_uuid_ib,\n"
  

• "\t\t    where 'b' and 'i' are number of banks and numbder of images in a bank respectively.\n"
  

typo

And personally \t\t is already nice indentation that you don't need to use additional spaces. And last line would be good to fit to 80 char limit.

•       );
  

+}

+static int active_bank = 0; +static int previous_bank = INT_MAX; /* unset */ +static bool __use_guid = false;

+struct fwu_mdata_object {

• size_t images;
• size_t banks;
• size_t size;
• struct fwu_mdata *mdata;

+};

+struct fwu_mdata_object *fwu_alloc_mdata(size_t images, size_t banks)

static?

+{

• struct fwu_mdata_object *mobj;
• mobj = calloc(1, sizeof(*mobj));
• if (!mobj)

• return NULL;
  

• mobj->size = sizeof(struct fwu_mdata) +

• (sizeof(struct fwu_image_entry) +
  

•  sizeof(struct fwu_image_bank_info) * banks) * images;
  

• mobj->images = images;
• mobj->banks = banks;
• mobj->mdata = calloc(1, mobj->size);
• if (!mobj->mdata) {

• free(mobj);
  

• return NULL;
  

• }
• return mobj;

+}

+struct fwu_image_entry *fwu_get_image(struct fwu_mdata_object *mobj, size_t idx)

static?

+{

• size_t offset;
• offset = sizeof(struct fwu_mdata) +

• (sizeof(struct fwu_image_entry) +
  

•  sizeof(struct fwu_image_bank_info) * mobj->banks) * idx;
  

• return (struct fwu_image_entry *)((char *)mobj->mdata + offset);

+}

+struct fwu_image_bank_info *fwu_get_bank(struct fwu_mdata_object *mobj,

• 			 size_t img_idx, size_t bnk_idx)
  

static?

+{

• size_t offset;
• offset = sizeof(struct fwu_mdata) +

• (sizeof(struct fwu_image_entry) +
  

•  sizeof(struct fwu_image_bank_info) * mobj->banks) * img_idx +
  

• sizeof(struct fwu_image_entry) +
  

• sizeof(struct fwu_image_bank_info) * bnk_idx;
  

• return (struct fwu_image_bank_info *)((char *)mobj->mdata + offset);

+}

+/**

• • convert_uuid_to_guid() - convert UUID to GUID
• • @buf: UUID binary
• • UUID and GUID have the same data structure, but their binary
• • formats are different due to the endianness. See lib/uuid.c.
• • Since uuid_parse() can handle only UUID, this function must
• • be called to get correct data for GUID when parsing a string.
• • The correct data will be returned in @buf.
• */

+void convert_uuid_to_guid(unsigned char *buf)

static?

+{

• unsigned char c;
• c = buf[0];
• buf[0] = buf[3];
• buf[3] = c;
• c = buf[1];
• buf[1] = buf[2];
• buf[2] = c;
• c = buf[4];
• buf[4] = buf[5];
• buf[5] = c;
• c = buf[6];
• buf[6] = buf[7];
• buf[7] = c;

if this is just endian convertion isn't there any standard function which you can just call.

+}

+int uuid_guid_parse(char *uuidstr, unsigned char *uuid)

static

+{

• int ret;
• ret = uuid_parse(uuidstr, uuid);
• if (ret < 0)

• return ret;
  

• if (__use_guid)

• convert_uuid_to_guid(uuid);
  

• return ret;

+}

+int fwu_parse_fill_image_uuid(struct fwu_mdata_object *mobj,

• 	      size_t idx, char *uuids)
  

static - fix it everywhere.

+{

• struct fwu_image_entry *image = fwu_get_image(mobj, idx);
• struct fwu_image_bank_info *bank;
• char *p = uuids, *uuid;
• int i;
• if (!image)

• return -ENOENT;
  

• /* Image location UUID */
• uuid = strsep(&p, ",");
• if (!uuid)

• return -EINVAL;
  

• if (strcmp(uuid, "0") &&

•    uuid_guid_parse(uuid, (unsigned char *)&image->location_uuid) < 0)
  

• return -EINVAL;
  

• /* Image type UUID */
• uuid = strsep(&p, ",");
• if (!uuid)

• return -EINVAL;
  

• if (uuid_guid_parse(uuid, (unsigned char *)&image->image_type_uuid) < 0)

• return -EINVAL;
  

• /* Fill bank image-UUID */
• for (i = 0; i < mobj->banks; i++) {

• bank = fwu_get_bank(mobj, idx, i);
  

• if (!bank)
  

• 	return -ENOENT;
  

• bank->accepted = 1;
  

• uuid = strsep(&p, ",");
  

• if (!uuid)
  

• 	return -EINVAL;
  

• if (strcmp(uuid, "0") &&
  

•     uuid_guid_parse(uuid, (unsigned char *)&bank->image_uuid) < 0)
  

• 	return -EINVAL;
  

• }
• return 0;

+}

+/* Caller must ensure that @uuids[] has @mobj->images entries. */ +int fwu_parse_fill_uuids(struct fwu_mdata_object *mobj, char *uuids[]) +{

• struct fwu_mdata *mdata = mobj->mdata;
• int i, ret;
• mdata->version = FWU_MDATA_VERSION;
• mdata->active_index = active_bank;
• mdata->previous_active_index = previous_bank;
• for (i = 0; i < mobj->images; i++) {

• ret = fwu_parse_fill_image_uuid(mobj, i, uuids[i]);
  

• if (ret < 0)
  

• 	return ret;
  

• }
• mdata->crc32 = crc32(0, (const unsigned char *)&mdata->version,

•                         mobj->size - sizeof(uint32_t));
  

• return 0;

+}

+int fwu_make_mdata(size_t images, size_t banks, char *uuids[], char *output) +{

• struct fwu_mdata_object *mobj;
• FILE *file;
• int ret;
• mobj = fwu_alloc_mdata(images, banks);
• if (!mobj)

• return -ENOMEM;
  

• ret = fwu_parse_fill_uuids(mobj, uuids);
• if (ret < 0)

• goto done_make;
  

• file = fopen(output, "w");
• if (!file) {

• ret = -errno;
  

• goto done_make;
  

• }
• ret = fwrite(mobj->mdata, mobj->size, 1, file);
• if (ret != mobj->size)

• ret = -errno;
  

• else

• ret = 0;
  

• fclose(file);

+done_make:

• free(mobj->mdata);
• free(mobj);
• return ret;

+}

+int main(int argc, char *argv[]) +{

• unsigned long banks = 0, images = 0;
• int c, ret;
• do {

• c = getopt_long(argc, argv, opts_short, options, NULL);
  

• switch (c) {
  

• case 'h':
  

• 	print_usage();
  

• 	return 0;
  

• case 'b':
  

• 	banks = strtoul(optarg, NULL, 0);
  

• 	break;
  

• case 'i':
  

• 	images = strtoul(optarg, NULL, 0);
  

• 	break;
  

• case 'g':
  

• 	__use_guid = true;
  

• 	break;
  

• case 'p':
  

• 	previous_bank = strtoul(optarg, NULL, 0);
  

• 	break;
  

• case 'a':
  

• 	active_bank = strtoul(optarg, NULL, 0);
  

• 	break;
  

• }
  

• } while (c != -1);
• if (!banks || !images) {

• fprintf(stderr, "Error: The number of banks and images must not be 0.\n");
  

• return -EINVAL;
  

• }
• /* This command takes UUIDs * images and output file. */
• if (optind + images + 1 != argc) {

• fprintf(stderr, "Error: UUID list or output file is not specified or too much.\n");
  

• print_usage();
  

• return -ERANGE;
  

• }
• if (previous_bank == INT_MAX) {

• /* set to the earlier bank in round-robin scheme */
  

• previous_bank = active_bank > 0 ? active_bank - 1 : banks - 1;
  

• }
• ret = fwu_make_mdata(images, banks, argv + optind, argv[argc - 1]);
• if (ret < 0)

• fprintf(stderr, "Error: Failed to parse and write image: %s\n",
  

• 	strerror(-ret));
  

• return ret;

+}

M