[v4,3/6] tools: Add mkfwumdata tool for FWU metadata image

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>
---
 tools/Kconfig      |   9 ++
 tools/Makefile     |   4 +
 tools/mkfwumdata.c | 334 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 347 insertions(+)
 create mode 100644 tools/mkfwumdata.c

On 3/27/23 23:16, jassisinghbrar@gmail.com 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>
> ---
>   tools/Kconfig      |   9 ++
>   tools/Makefile     |   4 +
>   tools/mkfwumdata.c | 334 +++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 347 insertions(+)
>   create mode 100644 tools/mkfwumdata.c
> 
> 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 e13effbb66..80eee71505 100644
> --- a/tools/Makefile
> +++ b/tools/Makefile
> @@ -247,6 +247,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..43dabf3b72
> --- /dev/null
> +++ b/tools/mkfwumdata.c
> @@ -0,0 +1,334 @@
> +// 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

These two are completely unused.

Thanks,
Michal

Hi,

On Tue, 28 Mar 2023 at 10:16, <jassisinghbrar@gmail.com> 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>
> ---
>  tools/Kconfig      |   9 ++
>  tools/Makefile     |   4 +
>  tools/mkfwumdata.c | 334 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 347 insertions(+)
>  create mode 100644 tools/mkfwumdata.c

Can you please look at putting this in binman instead, since we would
rather not have another tool with no tests.

Regards,
Simon

Am 27. März 2023 23:16:10 MESZ schrieb jassisinghbrar@gmail.com:
>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>
>---
> tools/Kconfig      |   9 ++
> tools/Makefile     |   4 +
> tools/mkfwumdata.c | 334 +++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 347 insertions(+)
> create mode 100644 tools/mkfwumdata.c

For each command line tool there should be a man-page.


>
>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 e13effbb66..80eee71505 100644
>--- a/tools/Makefile
>+++ b/tools/Makefile
>@@ -247,6 +247,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..43dabf3b72
>--- /dev/null
>+++ b/tools/mkfwumdata.c
>@@ -0,0 +1,334 @@
>+// 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: Endianness conversion may be required for some arch. */

Does the update work without UEFI? UEFI requires low endianness.

>+
>+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"

I would not know what this means.

Why can't you supply that single GUID in the position of the UUIDs parameter?

Best regards

Heinrich


>+		"\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        img_uuid_00,img_uuid_01...img_uuid_0b,\n"
>+		"\t        img_uuid_10,img_uuid_11...img_uuid_1b,\n"
>+		"\t        ...,\n"
>+		"\t        img_uuid_i0,img_uuid_i1...img_uuid_ib,\n"
>+		"\t          where 'b' and 'i' are number of banks and number\n"
>+		"\t          of images in a bank respectively.\n"
>+	       );
>+}
>+
>+struct fwu_mdata_object {
>+	size_t images;
>+	size_t banks;
>+	size_t size;
>+	struct fwu_mdata *mdata;
>+};
>+
>+static int previous_bank, active_bank;
>+static bool __use_guid;
>+
>+static struct fwu_mdata_object *fwu_alloc_mdata(size_t images, size_t banks)
>+{
>+	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;
>+}
>+
>+static struct fwu_image_entry *
>+fwu_get_image(struct fwu_mdata_object *mobj, size_t idx)
>+{
>+	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);
>+}
>+
>+static struct fwu_image_bank_info *
>+fwu_get_bank(struct fwu_mdata_object *mobj, size_t img_idx, size_t bnk_idx)
>+{
>+	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.
>+ */
>+static void convert_uuid_to_guid(unsigned char *buf)
>+{
>+	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;
>+}
>+
>+static int uuid_guid_parse(char *uuidstr, unsigned char *uuid)
>+{
>+	int ret;
>+
>+	ret = uuid_parse(uuidstr, uuid);
>+	if (ret < 0)
>+		return ret;
>+
>+	if (__use_guid)
>+		convert_uuid_to_guid(uuid);
>+
>+	return ret;
>+}
>+
>+static int
>+fwu_parse_fill_image_uuid(struct fwu_mdata_object *mobj,
>+			  size_t idx, char *uuids)
>+{
>+	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. */
>+static 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;
>+}
>+
>+static 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;
>+
>+	/* Explicitly initialize defaults */
>+	active_bank = 0;
>+	__use_guid = false;
>+	previous_bank = INT_MAX;
>+
>+	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;
>+}

On Wed, 29 Mar 2023 at 07:29, Michal Simek <michal.simek@amd.com> wrote:
> On 3/27/23 23:16, jassisinghbrar@gmail.com wrote:

> > diff --git a/tools/mkfwumdata.c b/tools/mkfwumdata.c
> > new file mode 100644
> > index 0000000000..43dabf3b72
> > --- /dev/null
> > +++ b/tools/mkfwumdata.c
> > @@ -0,0 +1,334 @@
> > +// 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
>
> These two are completely unused.
>
these are necessary for include/fwu_mdata.h that comes later.

cheers.

On Thu, 30 Mar 2023 at 01:12, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> Am 27. März 2023 23:16:10 MESZ schrieb jassisinghbrar@gmail.com:
> >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>
> >---
> > tools/Kconfig      |   9 ++
> > tools/Makefile     |   4 +
> > tools/mkfwumdata.c | 334 +++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 347 insertions(+)
> > create mode 100644 tools/mkfwumdata.c
>
> For each command line tool there should be a man-page.
>
ok

.....
> >+
> >+/* 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: Endianness conversion may be required for some arch. */
>
> Does the update work without UEFI? UEFI requires low endianness.
>
Not sure what you mean. This only creates a meta-data image that goes
into some offset in mtd.


> >+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"
>
> I would not know what this means.
>
> Why can't you supply that single GUID in the position of the UUIDs parameter?
>
I guess the original authors wanted to have one explicitly specify
what's going on, rather than 'hacking' around (?)

thanks

On Wed, 29 Mar 2023 at 15:02, Simon Glass <sjg@chromium.org> wrote:
>
> Hi,
>
> On Tue, 28 Mar 2023 at 10:16, <jassisinghbrar@gmail.com> 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>
> > ---
> >  tools/Kconfig      |   9 ++
> >  tools/Makefile     |   4 +
> >  tools/mkfwumdata.c | 334 +++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 347 insertions(+)
> >  create mode 100644 tools/mkfwumdata.c
>
> Can you please look at putting this in binman instead, since we would
> rather not have another tool with no tests.
>
Must I do that? I have no history with binman and it seems the
mkfwumdata.c would need to be rewritten in python?

regards.

On 4/10/23 06:25, Jassi Brar wrote:
> On Wed, 29 Mar 2023 at 15:02, Simon Glass <sjg@chromium.org> wrote:
>>
>> Hi,
>>
>> On Tue, 28 Mar 2023 at 10:16, <jassisinghbrar@gmail.com> 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>
>>> ---
>>>   tools/Kconfig      |   9 ++
>>>   tools/Makefile     |   4 +
>>>   tools/mkfwumdata.c | 334 +++++++++++++++++++++++++++++++++++++++++++++
>>>   3 files changed, 347 insertions(+)
>>>   create mode 100644 tools/mkfwumdata.c
>>
>> Can you please look at putting this in binman instead, since we would
>> rather not have another tool with no tests.
>>
> Must I do that? I have no history with binman and it seems the
> mkfwumdata.c would need to be rewritten in python?

I think it is about calling this utility from python not about rewriting it to 
python.

M

On 4/10/23 06:05, Jassi Brar wrote:
> On Wed, 29 Mar 2023 at 07:29, Michal Simek <michal.simek@amd.com> wrote:
>> On 3/27/23 23:16, jassisinghbrar@gmail.com wrote:
> 
>>> diff --git a/tools/mkfwumdata.c b/tools/mkfwumdata.c
>>> new file mode 100644
>>> index 0000000000..43dabf3b72
>>> --- /dev/null
>>> +++ b/tools/mkfwumdata.c
>>> @@ -0,0 +1,334 @@
>>> +// 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
>>
>> These two are completely unused.
>>
> these are necessary for include/fwu_mdata.h that comes later.

If that's come later, add it later.

M

On Fri, Apr 14, 2023 at 8:53 AM Michal Simek <michal.simek@amd.com> wrote:
>
>
>
> On 4/10/23 06:05, Jassi Brar wrote:
> > On Wed, 29 Mar 2023 at 07:29, Michal Simek <michal.simek@amd.com> wrote:
> >> On 3/27/23 23:16, jassisinghbrar@gmail.com wrote:
> >
> >>> diff --git a/tools/mkfwumdata.c b/tools/mkfwumdata.c
> >>> new file mode 100644
> >>> index 0000000000..43dabf3b72
> >>> --- /dev/null
> >>> +++ b/tools/mkfwumdata.c
> >>> @@ -0,0 +1,334 @@
> >>> +// 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
> >>
> >> These two are completely unused.
> >>
> > these are necessary for include/fwu_mdata.h that comes later.
>
> If that's come later, add it later.
>
Can you please actually look at the code to see the underlying constraint?

-j

On 4/14/23 17:02, Jassi Brar wrote:
> On Fri, Apr 14, 2023 at 8:53 AM Michal Simek <michal.simek@amd.com> wrote:
>>
>>
>>
>> On 4/10/23 06:05, Jassi Brar wrote:
>>> On Wed, 29 Mar 2023 at 07:29, Michal Simek <michal.simek@amd.com> wrote:
>>>> On 3/27/23 23:16, jassisinghbrar@gmail.com wrote:
>>>
>>>>> diff --git a/tools/mkfwumdata.c b/tools/mkfwumdata.c
>>>>> new file mode 100644
>>>>> index 0000000000..43dabf3b72
>>>>> --- /dev/null
>>>>> +++ b/tools/mkfwumdata.c
>>>>> @@ -0,0 +1,334 @@
>>>>> +// 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
>>>>
>>>> These two are completely unused.
>>>>
>>> these are necessary for include/fwu_mdata.h that comes later.
>>
>> If that's come later, add it later.
>>
> Can you please actually look at the code to see the underlying constraint?

Ok. I misunderstand your comment.

You have it there because of #include <fwu_mdata.h> and using macros there.
Can you just allocated that structures at run time instead of statically defined 
them via array? That would clean that design and also fix checkpatch issue.

Thanks,
Michal

$ ./scripts/checkpatch.pl -f tools/mkfwumdata.c
ERROR: All CONFIG symbols are managed by Kconfig
#18: FILE: tools/mkfwumdata.c:18:
+#define CONFIG_FWU_NUM_BANKS		0

ERROR: All CONFIG symbols are managed by Kconfig
#19: FILE: tools/mkfwumdata.c:19:
+#define CONFIG_FWU_NUM_IMAGES_PER_BANK	0

total: 2 errors, 0 warnings, 0 checks, 334 lines checked

On Mon, 17 Apr 2023 at 01:38, Michal Simek <michal.simek@amd.com> wrote:
> On 4/14/23 17:02, Jassi Brar wrote:

> >>>>> +
> >>>>> +/* This will dynamically allocate the fwu_mdata */
> >>>>> +#define CONFIG_FWU_NUM_BANKS         0
> >>>>> +#define CONFIG_FWU_NUM_IMAGES_PER_BANK       0
> >>>>
> >>>> These two are completely unused.
> >>>>
> >>> these are necessary for include/fwu_mdata.h that comes later.
> >>
> >> If that's come later, add it later.
> >>
> > Can you please actually look at the code to see the underlying constraint?
>
> Ok. I misunderstand your comment.
>
> You have it there because of #include <fwu_mdata.h> and using macros there.
> Can you just allocated that structures at run time instead of statically defined
> them via array? That would clean that design and also fix checkpatch issue.
>
I can't see how dynamically allocating an array of packed structures
inside an array of packed structures , makes the design any cleaner.

I think this is a valid case where we can ignore the checkpatch
warning because we know what we are doing.

-jassi

On 4/17/23 15:48, Jassi Brar wrote:
> On Mon, 17 Apr 2023 at 01:38, Michal Simek <michal.simek@amd.com> wrote:
>> On 4/14/23 17:02, Jassi Brar wrote:
> 
>>>>>>> +
>>>>>>> +/* This will dynamically allocate the fwu_mdata */
>>>>>>> +#define CONFIG_FWU_NUM_BANKS         0
>>>>>>> +#define CONFIG_FWU_NUM_IMAGES_PER_BANK       0
>>>>>>
>>>>>> These two are completely unused.
>>>>>>
>>>>> these are necessary for include/fwu_mdata.h that comes later.
>>>>
>>>> If that's come later, add it later.
>>>>
>>> Can you please actually look at the code to see the underlying constraint?
>>
>> Ok. I misunderstand your comment.
>>
>> You have it there because of #include <fwu_mdata.h> and using macros there.
>> Can you just allocated that structures at run time instead of statically defined
>> them via array? That would clean that design and also fix checkpatch issue.
>>
> I can't see how dynamically allocating an array of packed structures
> inside an array of packed structures , makes the design any cleaner.

It is not marked as __packed and based on what you are saying it should be 
marked like that.

> 
> I think this is a valid case where we can ignore the checkpatch
> warning because we know what we are doing.

I will let maintainer, who will merge this code, to decide on this.
I would at least make that comment much bigger to explain it better
that you are doing it because you don't want to redefine structures from 
fwu_mdata.h.

Thanks,
Michal

On Mon, 17 Apr 2023 at 09:29, Michal Simek <michal.simek@amd.com> wrote:
>
>
>
> On 4/17/23 15:48, Jassi Brar wrote:
> > On Mon, 17 Apr 2023 at 01:38, Michal Simek <michal.simek@amd.com> wrote:
> >> On 4/14/23 17:02, Jassi Brar wrote:
> >
> >>>>>>> +
> >>>>>>> +/* This will dynamically allocate the fwu_mdata */
> >>>>>>> +#define CONFIG_FWU_NUM_BANKS         0
> >>>>>>> +#define CONFIG_FWU_NUM_IMAGES_PER_BANK       0
> >>>>>>
> >>>>>> These two are completely unused.
> >>>>>>
> >>>>> these are necessary for include/fwu_mdata.h that comes later.
> >>>>
> >>>> If that's come later, add it later.
> >>>>
> >>> Can you please actually look at the code to see the underlying constraint?
> >>
> >> Ok. I misunderstand your comment.
> >>
> >> You have it there because of #include <fwu_mdata.h> and using macros there.
> >> Can you just allocated that structures at run time instead of statically defined
> >> them via array? That would clean that design and also fix checkpatch issue.
> >>
> > I can't see how dynamically allocating an array of packed structures
> > inside an array of packed structures , makes the design any cleaner.
>
> It is not marked as __packed and based on what you are saying it should be
> marked like that.
>
Those structures already existed before my patchset. And I definitely
remember they were suggested to be marked as packed but were not. So
now you have to read to code to understand what they are.

> >
> > I think this is a valid case where we can ignore the checkpatch
> > warning because we know what we are doing.
>
> I will let maintainer, who will merge this code, to decide on this.
> I would at least make that comment much bigger to explain it better
> that you are doing it because you don't want to redefine structures from
> fwu_mdata.h.
>
you mean redeclare, right?  That is what the two define's do effectively.

-j

Hi,

On Fri, 14 Apr 2023 at 07:53, Michal Simek <michal.simek@amd.com> wrote:
>
>
>
> On 4/10/23 06:25, Jassi Brar wrote:
> > On Wed, 29 Mar 2023 at 15:02, Simon Glass <sjg@chromium.org> wrote:
> >>
> >> Hi,
> >>
> >> On Tue, 28 Mar 2023 at 10:16, <jassisinghbrar@gmail.com> 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>
> >>> ---
> >>>   tools/Kconfig      |   9 ++
> >>>   tools/Makefile     |   4 +
> >>>   tools/mkfwumdata.c | 334 +++++++++++++++++++++++++++++++++++++++++++++
> >>>   3 files changed, 347 insertions(+)
> >>>   create mode 100644 tools/mkfwumdata.c
> >>
> >> Can you please look at putting this in binman instead, since we would
> >> rather not have another tool with no tests.
> >>
> > Must I do that? I have no history with binman and it seems the
> > mkfwumdata.c would need to be rewritten in python?
>
> I think it is about calling this utility from python not about rewriting it to
> python.

Yes that's the question. If this tool is for creating firmware
updates, then how are they created? I would expect binman to handle
this when U-Boot is built. Then you can build in some tests in binman
perhaps?

How does one know what parameters to pass? Is the documentation for
this tool elsewhere? Where are the tests?

It is also unfortunate that this seems to be inventing yet another
format (I recall that FIP was invented at one point also), when it
could use FIT.

Regards,
Simon

On Tue, 18 Apr 2023 at 20:46, Simon Glass <sjg@chromium.org> wrote:
>
> Hi,
>
> On Fri, 14 Apr 2023 at 07:53, Michal Simek <michal.simek@amd.com> wrote:
> >
> >
> >
> > On 4/10/23 06:25, Jassi Brar wrote:
> > > On Wed, 29 Mar 2023 at 15:02, Simon Glass <sjg@chromium.org> wrote:
> > >>
> > >> Hi,
> > >>
> > >> On Tue, 28 Mar 2023 at 10:16, <jassisinghbrar@gmail.com> 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>
> > >>> ---
> > >>>   tools/Kconfig      |   9 ++
> > >>>   tools/Makefile     |   4 +
> > >>>   tools/mkfwumdata.c | 334 +++++++++++++++++++++++++++++++++++++++++++++
> > >>>   3 files changed, 347 insertions(+)
> > >>>   create mode 100644 tools/mkfwumdata.c
> > >>
> > >> Can you please look at putting this in binman instead, since we would
> > >> rather not have another tool with no tests.
> > >>
> > > Must I do that? I have no history with binman and it seems the
> > > mkfwumdata.c would need to be rewritten in python?
> >
> > I think it is about calling this utility from python not about rewriting it to
> > python.
>
> Yes that's the question. If this tool is for creating firmware
> updates, then how are they created? I would expect binman to handle
> this when U-Boot is built. Then you can build in some tests in binman
> perhaps?
>
The FWU meta-data format is specified in a standards document created
by ARM and not tied to u-boot.
U-Boot may not necessarily be the bootloader using the output of
mkfwumdata. The u-boot/tools/ is more like a welcoming host.

Ideally mkfwumdata should be a reference implementation, also created
by ARM. But it is trivial enough that nobody thought there could be
any confusion about the format, I guess.

> How does one know what parameters to pass? Is the documentation for
> this tool elsewhere?
>
In the latest submission I also created a man-page for it.

>  Where are the tests?
>
I am open to learning what could be tested and how.

> It is also unfortunate that this seems to be inventing yet another
> format (I recall that FIP was invented at one point also), when it
> could use FIT.
>
Hopefully it won't be that bad of a predicament after my explanation above.

cheers.

Hi Jassi,

On Tue, 18 Apr 2023 at 20:58, Jassi Brar <jaswinder.singh@linaro.org> wrote:
>
> On Tue, 18 Apr 2023 at 20:46, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi,
> >
> > On Fri, 14 Apr 2023 at 07:53, Michal Simek <michal.simek@amd.com> wrote:
> > >
> > >
> > >
> > > On 4/10/23 06:25, Jassi Brar wrote:
> > > > On Wed, 29 Mar 2023 at 15:02, Simon Glass <sjg@chromium.org> wrote:
> > > >>
> > > >> Hi,
> > > >>
> > > >> On Tue, 28 Mar 2023 at 10:16, <jassisinghbrar@gmail.com> 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>
> > > >>> ---
> > > >>>   tools/Kconfig      |   9 ++
> > > >>>   tools/Makefile     |   4 +
> > > >>>   tools/mkfwumdata.c | 334 +++++++++++++++++++++++++++++++++++++++++++++
> > > >>>   3 files changed, 347 insertions(+)
> > > >>>   create mode 100644 tools/mkfwumdata.c
> > > >>
> > > >> Can you please look at putting this in binman instead, since we would
> > > >> rather not have another tool with no tests.
> > > >>
> > > > Must I do that? I have no history with binman and it seems the
> > > > mkfwumdata.c would need to be rewritten in python?
> > >
> > > I think it is about calling this utility from python not about rewriting it to
> > > python.
> >
> > Yes that's the question. If this tool is for creating firmware
> > updates, then how are they created? I would expect binman to handle
> > this when U-Boot is built. Then you can build in some tests in binman
> > perhaps?
> >
> The FWU meta-data format is specified in a standards document created
> by ARM and not tied to u-boot.
> U-Boot may not necessarily be the bootloader using the output of
> mkfwumdata. The u-boot/tools/ is more like a welcoming host.
>
> Ideally mkfwumdata should be a reference implementation, also created
> by ARM. But it is trivial enough that nobody thought there could be
> any confusion about the format, I guess.

OK

>
> > How does one know what parameters to pass? Is the documentation for
> > this tool elsewhere?
> >
> In the latest submission I also created a man-page for it.

Good

>
> >  Where are the tests?
> >
> I am open to learning what could be tested and how.

Well normally we would have a binman test which uses the tool to
create an image, then checks that it works. See ftest.py for some
examples.

>
> > It is also unfortunate that this seems to be inventing yet another
> > format (I recall that FIP was invented at one point also), when it
> > could use FIT.
> >
> Hopefully it won't be that bad of a predicament after my explanation above.

Regards,
Simon