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

Patrick DELAUNAY
2022-06-21T12:55:47Z

Hi,

one minor remark.

On 6/9/22 14:30, Sughosh Ganu 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
> +

adding typedef should be avoided

can you include "asm/types.h"  or "linux/types.h" to use the generic define

> +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},
> +};
> +

[...]

Regards

Patrick