diff mbox series

[v2,17/21] tools: mkfwumdata: migrate to metadata version 2

Message ID 20240212074712.3657076-18-sughosh.ganu@linaro.org
State New
Headers show
Series FWU: Migrate FWU metadata to version 2 | expand

Commit Message

Sughosh Ganu Feb. 12, 2024, 7:47 a.m. UTC 
Migrate the metadata generation tool to generate the version 2
metadata.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---

Changes since V1:
* Compute location of struct fwu_fw_store_desc using pointer
  arithmetic.

 tools/mkfwumdata.c | 45 ++++++++++++++++++++++++++++++++++-----------
 1 file changed, 34 insertions(+), 11 deletions(-)

Comments

Michal Simek Feb. 15, 2024, 2:31 p.m. UTC | #1 
Hi,

On 2/12/24 08:47, Sughosh Ganu wrote:
> Migrate the metadata generation tool to generate the version 2
> metadata.
> 
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
> 
> Changes since V1:
> * Compute location of struct fwu_fw_store_desc using pointer
>    arithmetic.
> 
>   tools/mkfwumdata.c | 45 ++++++++++++++++++++++++++++++++++-----------
>   1 file changed, 34 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/mkfwumdata.c b/tools/mkfwumdata.c
> index 9732a8ddc5..fb847e3a78 100644
> --- a/tools/mkfwumdata.c
> +++ b/tools/mkfwumdata.c
> @@ -14,12 +14,13 @@
>   #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
> +#define FWU_MDATA_VERSION		2
> +
> +#define MAX_BANKS			4
> +
> +#define BANK_INVALID			0xFF
> +#define BANK_ACCEPTED			0xFC

I think in previous version only active bank was accepted not others.
I don't think it is wrong behavior but please consider to select it too.
I was just surprised to see both banks in that state.

Thanks,
Michal
Sughosh Ganu Feb. 19, 2024, 11:42 a.m. UTC | #2 
hi Michal,

On Thu, 15 Feb 2024 at 20:01, Michal Simek <michal.simek@amd.com> wrote:
>
> Hi,
>
> On 2/12/24 08:47, Sughosh Ganu wrote:
> > Migrate the metadata generation tool to generate the version 2
> > metadata.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > ---
> >
> > Changes since V1:
> > * Compute location of struct fwu_fw_store_desc using pointer
> >    arithmetic.
> >
> >   tools/mkfwumdata.c | 45 ++++++++++++++++++++++++++++++++++-----------
> >   1 file changed, 34 insertions(+), 11 deletions(-)
> >
> > diff --git a/tools/mkfwumdata.c b/tools/mkfwumdata.c
> > index 9732a8ddc5..fb847e3a78 100644
> > --- a/tools/mkfwumdata.c
> > +++ b/tools/mkfwumdata.c
> > @@ -14,12 +14,13 @@
> >   #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
> > +#define FWU_MDATA_VERSION            2
> > +
> > +#define MAX_BANKS                    4
> > +
> > +#define BANK_INVALID                 0xFF
> > +#define BANK_ACCEPTED                        0xFC
>
> I think in previous version only active bank was accepted not others.
> I don't think it is wrong behavior but please consider to select it too.
> I was just surprised to see both banks in that state.

In the current version as well, we do accept all the images, for all
banks. This is happening in the fwu_parse_fill_image_uuid() function,
where on line 207 we are setting the accepted field to 1 for all banks
for a given image.

-sughosh
diff mbox series

Patch

diff --git a/tools/mkfwumdata.c b/tools/mkfwumdata.c
index 9732a8ddc5..fb847e3a78 100644
--- a/tools/mkfwumdata.c
+++ b/tools/mkfwumdata.c
@@ -14,12 +14,13 @@ 
 #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
+#define FWU_MDATA_VERSION		2
+
+#define MAX_BANKS			4
+
+#define BANK_INVALID			0xFF
+#define BANK_ACCEPTED			0xFC
 
 typedef uint8_t u8;
 typedef int16_t s16;
@@ -29,8 +30,6 @@  typedef uint64_t u64;
 
 #include <fwu_mdata.h>
 
-/* TODO: Endianness conversion may be required for some arch. */
-
 static const char *opts_short = "b:i:a:p:gh";
 
 static struct option options[] = {
@@ -48,7 +47,7 @@  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 (mandatory)\n"
-		"\t-b, --banks  <num>          Number of banks (mandatory)\n"
+		"\t-b, --banks  <num>          Number of banks(1-4) (mandatory)\n"
 		"\t-a, --active-bank  <num>    Active bank (default=0)\n"
 		"\t-p, --previous-bank  <num>  Previous active bank (default=active_bank - 1)\n"
 		"\t-g, --guid                  Use GUID instead of UUID\n"
@@ -85,6 +84,7 @@  static struct fwu_mdata_object *fwu_alloc_mdata(size_t images, size_t banks)
 		return NULL;
 
 	mobj->size = sizeof(struct fwu_mdata) +
+		sizeof(struct fwu_fw_store_desc) +
 		(sizeof(struct fwu_image_entry) +
 		 sizeof(struct fwu_image_bank_info) * banks) * images;
 	mobj->images = images;
@@ -105,6 +105,7 @@  fwu_get_image(struct fwu_mdata_object *mobj, size_t idx)
 	size_t offset;
 
 	offset = sizeof(struct fwu_mdata) +
+		sizeof(struct fwu_fw_store_desc) +
 		(sizeof(struct fwu_image_entry) +
 		 sizeof(struct fwu_image_bank_info) * mobj->banks) * idx;
 
@@ -117,6 +118,7 @@  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_fw_store_desc) +
 		(sizeof(struct fwu_image_entry) +
 		 sizeof(struct fwu_image_bank_info) * mobj->banks) * img_idx +
 		sizeof(struct fwu_image_entry) +
@@ -188,7 +190,7 @@  fwu_parse_fill_image_uuid(struct fwu_mdata_object *mobj,
 		return -EINVAL;
 
 	if (strcmp(uuid, "0") &&
-	    uuid_guid_parse(uuid, (unsigned char *)&image->location_uuid) < 0)
+	    uuid_guid_parse(uuid, (unsigned char *)&image->location_guid) < 0)
 		return -EINVAL;
 
 	/* Image type UUID */
@@ -196,7 +198,7 @@  fwu_parse_fill_image_uuid(struct fwu_mdata_object *mobj,
 	if (!uuid)
 		return -EINVAL;
 
-	if (uuid_guid_parse(uuid, (unsigned char *)&image->image_type_uuid) < 0)
+	if (uuid_guid_parse(uuid, (unsigned char *)&image->image_type_guid) < 0)
 		return -EINVAL;
 
 	/* Fill bank image-UUID */
@@ -210,7 +212,7 @@  fwu_parse_fill_image_uuid(struct fwu_mdata_object *mobj,
 			return -EINVAL;
 
 		if (strcmp(uuid, "0") &&
-		    uuid_guid_parse(uuid, (unsigned char *)&bank->image_uuid) < 0)
+		    uuid_guid_parse(uuid, (unsigned char *)&bank->image_guid) < 0)
 			return -EINVAL;
 	}
 	return 0;
@@ -220,11 +222,26 @@  fwu_parse_fill_image_uuid(struct fwu_mdata_object *mobj,
 static int fwu_parse_fill_uuids(struct fwu_mdata_object *mobj, char *uuids[])
 {
 	struct fwu_mdata *mdata = mobj->mdata;
+	struct fwu_fw_store_desc *fw_desc;
 	int i, ret;
 
 	mdata->version = FWU_MDATA_VERSION;
 	mdata->active_index = active_bank;
 	mdata->previous_active_index = previous_bank;
+	mdata->metadata_size = mobj->size;
+	mdata->desc_offset = sizeof(struct fwu_mdata);
+
+	for (i = 0; i < MAX_BANKS; i++)
+		mdata->bank_state[i] = i < mobj->banks ?
+			BANK_ACCEPTED : BANK_INVALID;
+
+	fw_desc = (struct fwu_fw_store_desc *)((u8 *)mdata + sizeof(*mdata));
+	fw_desc->num_banks = mobj->banks;
+	fw_desc->num_images = mobj->images;
+	fw_desc->img_entry_size = sizeof(struct fwu_image_entry) +
+		(sizeof(struct fwu_image_bank_info) * mobj->banks);
+	fw_desc->bank_info_entry_size =
+		sizeof(struct fwu_image_bank_info);
 
 	for (i = 0; i < mobj->images; i++) {
 		ret = fwu_parse_fill_image_uuid(mobj, i, uuids[i]);
@@ -313,6 +330,12 @@  int main(int argc, char *argv[])
 		return -EINVAL;
 	}
 
+	if (banks > MAX_BANKS) {
+		fprintf(stderr, "Error: The number of banks must not be more than %u.\n",
+			MAX_BANKS);
+		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");