[cbootimage PATCH 0/2] Re-enable jtag function for Tegra124

[cbootimage PATCH 0/2] Re-enable jtag function for Tegra124

[Penny Chiu]

This patch series is used to re-enable jtag function for Tegra124.

Since the secure jtag is disabled by default once SECURITY_MODE fuse is
blown. BOOTROM will re-enable secure jtag function if BCT has
SecureJTAGControl=1 and a matching chip UID included.

After applying these changes, cbootimage can access jtag control and chip uid
fields. It can also read the BCT data from pre-built image, update the BCT
configs based on config file, and generate a new image file.

Penny Chiu (2):
  Add Tegra124 bct data access for jtag control and chip uid
  Add update BCT configs feature

 src/bct_dump.c           | 45 ++++++++++++++++++++---------
 src/cbootimage.c         | 70 +++++++++++++++++++++++++++++++++++---------
 src/cbootimage.h         |  7 ++++-
 src/data_layout.c        | 69 ++++++++++++++++++++++++++++++++++++++------
 src/data_layout.h        |  8 +++++-
 src/parse.c              | 75 +++++++++++++++++++++++++++++++++++++++++++++++-
 src/parse.h              |  2 ++
 src/set.c                | 54 ++++++++++++++++++++++++++++------
 src/set.h                |  5 ++++
 src/t124/nvbctlib_t124.c | 14 +++++++++
 10 files changed, 303 insertions(+), 46 deletions(-)

-- 
1.9.0

[cbootimage PATCH 1/2] Add Tegra124 bct data access for jtag control and chip uid

[Penny Chiu]

Add support for read secure_jtag_control and unique_chip_id from
cfg file and write them into BCT structure, and bct_dump can also
parse the two fields and show the data.

Signed-off-by: Penny Chiu <pchiu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 src/bct_dump.c           | 41 ++++++++++++++++++++--------
 src/cbootimage.h         |  2 ++
 src/parse.c              | 71 ++++++++++++++++++++++++++++++++++++++++++++++++
 src/parse.h              |  2 ++
 src/set.c                | 25 +++++++++++++++++
 src/set.h                |  4 +++
 src/t124/nvbctlib_t124.c | 14 ++++++++++
 7 files changed, 148 insertions(+), 11 deletions(-)

diff --git a/src/bct_dump.c b/src/bct_dump.c
index dbef913..fa3fdb7 100644
--- a/src/bct_dump.c
+++ b/src/bct_dump.c
@@ -37,7 +37,9 @@ static value_data const values[] = {
 	{ token_block_size_log2,     "BlockSize     = 0x%08x;\n" },
 	{ token_page_size_log2,      "PageSize      = 0x%08x;\n" },
 	{ token_partition_size,      "PartitionSize = 0x%08x;\n" },
-	{ token_odm_data,            "OdmData       = 0x%08x;\n\n" },
+	{ token_odm_data,            "OdmData       = 0x%08x;\n" },
+	{ token_secure_jtag_control, "JtagCtrl      = 0x%08x;\n" },
+	{ token_unique_chip_id,      "ChipUid       = %s;\n" },
 	{ token_bootloader_used,     "# Bootloader used       = %d;\n" },
 	{ token_bootloaders_max,     "# Bootloaders max       = %d;\n" },
 	{ token_bct_size,            "# BCT size              = %d;\n" },
@@ -154,17 +156,34 @@ int main(int argc, char *argv[])
 
 	/* Display root values */
 	for (i = 0; i < sizeof(values) / sizeof(values[0]); ++i) {
-		e = g_soc_config->get_value(values[i].id,
-						&data,
-						context.bct);
+		if (values[i].id == token_unique_chip_id) {
+			u_int8_t uid[16];
+			char uid_str[35] = "0x";
 
-		if (e != 0)
-			data = -1;
-		else if (values[i].id == token_block_size_log2 ||
-			 values[i].id == token_page_size_log2)
-			data = 1 << data;
-
-		printf(values[i].message, data);
+			e = g_soc_config->get_value(values[i].id,
+							(u_int32_t *)uid,
+							context.bct);
+			if (e != 0)
+				continue;
+
+			for (j = 15; j >= 0; j--)
+				sprintf(uid_str, "%s%02x", uid_str, uid[j]);
+			printf(values[i].message, uid_str);
+		} else {
+			e = g_soc_config->get_value(values[i].id,
+							&data,
+							context.bct);
+
+			if (e != 0) {
+				if (values[i].id == token_secure_jtag_control)
+					continue;
+				data = -1;
+			} else if (values[i].id == token_block_size_log2 ||
+				 values[i].id == token_page_size_log2)
+				data = 1 << data;
+
+			printf(values[i].message, data);
+		}
 	}
 
 	/* Display bootloader values */
diff --git a/src/cbootimage.h b/src/cbootimage.h
index 46e3b8b..252c41e 100644
--- a/src/cbootimage.h
+++ b/src/cbootimage.h
@@ -96,6 +96,8 @@ typedef struct build_image_context_rec
 	u_int32_t boot_data_version; /* The boot data version of BCT */
 	u_int8_t bct_init; /* The flag for the memory allocation of bct */
 	u_int32_t odm_data; /* The odm data value */
+	u_int8_t unique_chip_id[16]; /* The unique chip uid */
+	u_int8_t secure_jtag_control; /* The flag for enabling jtag control */
 } build_image_context;
 
 /* Function prototypes */
diff --git a/src/parse.c b/src/parse.c
index 464ee8f..90d37e7 100644
--- a/src/parse.c
+++ b/src/parse.c
@@ -45,6 +45,7 @@ set_array(build_image_context *context,
 			u_int32_t value);
 static char *parse_u32(char *str, u_int32_t *val);
 static char *parse_u8(char *str, u_int32_t *val);
+static char *parse_chipuid(char *str, u_int8_t *val);
 static char *parse_filename(char *str, char *name, int chars_remaining);
 static char *parse_enum(build_image_context *context,
 			char *str,
@@ -64,6 +65,10 @@ parse_bootloader(build_image_context *context, parse_token token, char *rest);
 static int
 parse_value_u32(build_image_context *context, parse_token token, char *rest);
 static int
+parse_value_chipuid(build_image_context *context,
+			parse_token token,
+			char *rest);
+static int
 parse_bct_file(build_image_context *context, parse_token token, char *rest);
 static char
 *parse_end_state(char *str, char *uname, int chars_remaining);
@@ -102,6 +107,8 @@ static parse_item s_top_level_items[] = {
 	{ "Bctcopy=",       token_bct_copy,		parse_value_u32 },
 	{ "Version=",       token_version,		parse_value_u32 },
 	{ "OdmData=",       token_odm_data,		parse_value_u32 },
+	{ "ChipUid=",       token_unique_chip_id,	parse_value_chipuid },
+	{ "JtagCtrl=",	    token_secure_jtag_control,	parse_value_u32 },
 	{ NULL, 0, NULL } /* Must be last */
 };
 
@@ -165,6 +172,45 @@ parse_u8(char *str, u_int32_t *val)
 	return retval;
 }
 
+/*
+ * Parse the given string and transfer to chip uid.
+ *
+ * @param str		String to parse
+ * @param chipuid	Returns chip uid that was parsed
+ * @return the remainder of the string after the number was parsed
+ */
+static char *
+parse_chipuid(char *str, u_int8_t *chipuid)
+{
+	while (*str == '0')
+		str++;
+
+	if (tolower(*str) == 'x') {
+		int byte_index = 0;
+		int paddings = 0;
+		char byte_str[3];
+
+		str++;
+		paddings = strlen(str) % 2;
+		byte_index = strlen(str) / 2 + paddings;
+
+		while (*str != '\0' && byte_index > 0) {
+			u_int32_t value = 0;
+
+			strncpy(byte_str, str, 2 - paddings);
+			byte_str[2-paddings] = '\0';
+			str = str + 2 - paddings;
+
+			sscanf(byte_str, "%x", &value);
+			*(chipuid + byte_index - 1) = value;
+
+			byte_index--;
+			paddings = 0;
+		}
+	}
+
+	return str;
+}
 
 /*
  * Parse the given string and find the file name then
@@ -486,6 +532,31 @@ static int parse_value_u32(build_image_context *context,
 }
 
 /*
+ * General handler for setting chip uid in config files.
+ *
+ * @param context	The main context pointer
+ * @param token		The parse token value
+ * @param rest		String to parse
+ * @return 0 and 1 for success and failure
+ */
+static int parse_value_chipuid(build_image_context *context,
+			parse_token token,
+			char *rest)
+{
+	u_int8_t value[16];
+
+	assert(context != NULL);
+	assert(rest != NULL);
+
+	memset(value, 0, sizeof(value));
+	rest = parse_chipuid(rest, value);
+	if (rest == NULL)
+		return 1;
+
+	return context_set_chipuid(context, value);
+}
+
+/*
  * Parse the given string and find the bct file name.
  *
  * @param context	The main context pointer
diff --git a/src/parse.h b/src/parse.h
index 80f42c4..ffa9c17 100644
--- a/src/parse.h
+++ b/src/parse.h
@@ -108,6 +108,8 @@ typedef enum
 	token_dev_type_spi,
 	token_num_sdram_sets,
 	token_pre_bct_pad_blocks,
+	token_unique_chip_id,
+	token_secure_jtag_control,
 
 	token_nand_clock_divider,
 	token_nand_nand_timing,
diff --git a/src/set.c b/src/set.c
index 08c9fb6..99b242c 100644
--- a/src/set.c
+++ b/src/set.c
@@ -199,8 +199,33 @@ int context_set_value(build_image_context *context,
 		context->pre_bct_pad_blocks = value;
 		break;
 
+	case token_secure_jtag_control:
+		context->secure_jtag_control = value;
+		g_soc_config->set_value(token_secure_jtag_control,
+			value, context->bct);
+		break;
+
 	DEFAULT();
 	}
 
 	return 0;
 }
+
+/*
+ * General handler for setting chip uid in config files.
+ *
+ * @param context	The main context pointer
+ * @param data		The value to set
+ * @return 0 for success
+ */
+int context_set_chipuid(build_image_context *context,
+		u_int8_t *data)
+{
+	assert(context != NULL);
+
+	memcpy(context->unique_chip_id, data, 16);
+	g_soc_config->set_data(token_unique_chip_id, data,
+		16, context->bct);
+
+	return 0;
+}
diff --git a/src/set.h b/src/set.h
index 1515fcd..dbde479 100644
--- a/src/set.h
+++ b/src/set.h
@@ -41,6 +41,10 @@ context_set_value(build_image_context	*context,
 		u_int32_t	value);
 
 int
+context_set_chipuid(build_image_context	*context,
+		u_int8_t	*data);
+
+int
 read_from_image(char *filename,
 		u_int8_t	**Image,
 		u_int32_t	*actual_size,
diff --git a/src/t124/nvbctlib_t124.c b/src/t124/nvbctlib_t124.c
index 27e5a62..bb83f53 100644
--- a/src/t124/nvbctlib_t124.c
+++ b/src/t124/nvbctlib_t124.c
@@ -926,6 +926,7 @@ t124_bct_get_value(parse_token id, u_int32_t *data, u_int8_t *bct)
 	CASE_GET_NVU32(num_sdram_sets);
 	CASE_GET_NVU32(bootloader_used);
 	CASE_GET_NVU32(odm_data);
+	CASE_GET_NVU32(secure_jtag_control);
 
 	/*
 	 * Constants.
@@ -940,6 +941,12 @@ t124_bct_get_value(parse_token id, u_int32_t *data, u_int8_t *bct)
 		sizeof(nvboot_hash));
 		break;
 
+	case token_unique_chip_id:
+		memcpy(data,
+		&(bct_ptr->unique_chip_id),
+		sizeof(nvboot_ecid));
+		break;
+
 	case token_reserved_offset:
 		*data = (u_int8_t *)&(samplebct.reserved)
 				- (u_int8_t *)&samplebct;
@@ -1005,6 +1012,7 @@ t124_bct_set_value(parse_token id, u_int32_t data, u_int8_t *bct)
 	CASE_SET_NVU32(num_sdram_sets);
 	CASE_SET_NVU32(bootloader_used);
 	CASE_SET_NVU32(odm_data);
+	CASE_SET_NVU32(secure_jtag_control);
 
 	default:
 		return -ENODATA;
@@ -1032,6 +1040,12 @@ t124_bct_set_data(parse_token id,
 		memcpy(&bct_ptr->signature.crypto_hash, data, sizeof(nvboot_hash));
 		break;
 
+	case token_unique_chip_id:
+		if (length > sizeof(nvboot_ecid))
+			return -ENODATA;
+		memcpy(&bct_ptr->unique_chip_id, data, length);
+		break;
+
 	default:
 		return -ENODATA;
 	}
-- 
1.9.0

[cbootimage PATCH 2/2] Add update BCT configs feature

[Penny Chiu]

This feature reads the BCT data from BCT or BCT with bootloader
appended binary, updates the BCT data based on config file, then
writes to new image file.

Signed-off-by: Penny Chiu <pchiu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 src/bct_dump.c    |  4 ++--
 src/cbootimage.c  | 70 ++++++++++++++++++++++++++++++++++++++++++++-----------
 src/cbootimage.h  |  5 +++-
 src/data_layout.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++-------
 src/data_layout.h |  8 ++++++-
 src/parse.c       |  4 +++-
 src/set.c         | 29 ++++++++++++++++-------
 src/set.h         |  1 +
 8 files changed, 155 insertions(+), 35 deletions(-)

diff --git a/src/bct_dump.c b/src/bct_dump.c
index fa3fdb7..288c0e1 100644
--- a/src/bct_dump.c
+++ b/src/bct_dump.c
@@ -148,9 +148,9 @@ int main(int argc, char *argv[])
 		usage();
 
 	memset(&context, 0, sizeof(build_image_context));
-	context.bct_filename = argv[1];
+	context.input_image_filename = argv[1];
 
-	e = read_bct_file(&context);
+	e = read_bct_file(&context, 0);
 	if (e != 0)
 		return e;
 
diff --git a/src/cbootimage.c b/src/cbootimage.c
index 1332c5f..b447c19 100644
--- a/src/cbootimage.c
+++ b/src/cbootimage.c
@@ -49,6 +49,7 @@ struct option cbootcmd[] = {
 	{"tegra", 1, NULL, 't'},
 	{"odmdata", 1, NULL, 'o'},
 	{"soc", 1, NULL, 's'},
+	{"update", 0, NULL, 'u'},
 	{0, 0, 0, 0},
 };
 
@@ -63,7 +64,7 @@ write_image_file(build_image_context *context)
 static void
 usage(void)
 {
-	printf("Usage: cbootimage [options] configfile imagename\n");
+	printf("Usage: cbootimage [options] configfile [inputimage] outputimage\n");
 	printf("    options:\n");
 	printf("    -h, --help, -?        Display this message.\n");
 	printf("    -d, --debug           Output debugging information.\n");
@@ -75,18 +76,22 @@ usage(void)
 	printf("    -s|--soc tegraNN      Select target device. Must be one of:\n");
 	printf("                          tegra20, tegra30, tegra114, tegra124.\n");
 	printf("                          Default: tegra20.\n");
+	printf("    -u|--update           Copy input image data and update bct\n");
+	printf("                          configs into new image file.\n");
 	printf("    configfile            File with configuration information\n");
-	printf("    imagename             Output image name\n");
+	printf("    inputimage            Input image name. This is required\n");
+	printf("                          if -u|--update option is used.\n");
+	printf("    outputimage           Output image name\n");
 }
 
 static int
 process_command_line(int argc, char *argv[], build_image_context *context)
 {
-	int c;
+	int c, num_of_filenames = 2;
 
 	context->generate_bct = 0;
 
-	while ((c = getopt_long(argc, argv, "hdg:t:o:s:", cbootcmd, NULL)) != -1) {
+	while ((c = getopt_long(argc, argv, "hdg:t:o:s:u", cbootcmd, NULL)) != -1) {
 		switch (c) {
 		case 'h':
 			help_only = 1;
@@ -131,10 +136,14 @@ process_command_line(int argc, char *argv[], build_image_context *context)
 		case 'o':
 			context->odm_data = strtoul(optarg, NULL, 16);
 			break;
+		case 'u':
+			context->update_bct = 1;
+			num_of_filenames = 3;
+			break;
 		}
 	}
 
-	if (argc - optind != 2) {
+	if (argc - optind != num_of_filenames) {
 		printf("Missing configuration and/or image file name.\n");
 		usage();
 		return -EINVAL;
@@ -145,14 +154,19 @@ process_command_line(int argc, char *argv[], build_image_context *context)
 		t20_get_soc_config(context, &g_soc_config);
 
 	/* Open the configuration file. */
-	context->config_file = fopen(argv[optind], "r");
+	context->config_file = fopen(argv[optind++], "r");
+
 	if (context->config_file == NULL) {
 		printf("Error opening config file.\n");
 		return -ENODATA;
 	}
 
+	/* Record the input image filename if update_bct is necessary */
+	if (context->update_bct)
+		context->input_image_filename = argv[optind++];
+
 	/* Record the output filename */
-	context->image_filename = argv[optind + 1];
+	context->output_image_filename = argv[optind++];
 
 	return 0;
 }
@@ -190,18 +204,46 @@ main(int argc, char *argv[])
 	}
 
 	/* Open the raw output file. */
-	context.raw_file = fopen(context.image_filename,
+	context.raw_file = fopen(context.output_image_filename,
 		                 "w+");
 	if (context.raw_file == NULL) {
 		printf("Error opening raw file %s.\n",
-			context.image_filename);
+			context.output_image_filename);
 		goto fail;
 	}
 
-	/* first, if we aren't generating the bct, read in config file */
-	if (context.generate_bct == 0) {
-		process_config_file(&context, 1);
+	/* Read the bct data from image if bct configs needs to be updated */
+	if (context.update_bct) {
+		u_int32_t offset = 0;
+		struct stat stats;
+
+		if (stat(context.input_image_filename, &stats) != 0) {
+			printf("Error: Unable to query info on input file path %s\n",
+			context.input_image_filename);
+			goto fail;
+		}
+
+		while (stats.st_size > offset) {
+			if (!read_bct_file(&context, offset)) {
+				process_config_file(&context, 0);
+				e = sign_bct(&context, context.bct);
+				if (e != 0) {
+					printf("Signing BCT failed, error: %d.\n", e);
+					goto fail;
+				}
+				write_bct_raw(&context);
+			} else
+				write_data_raw(&context, offset, NVBOOT_CONFIG_TABLE_SIZE);
+
+			offset += NVBOOT_CONFIG_TABLE_SIZE;
+		}
+		printf("Image file %s has been successfully generated!\n",
+			context.output_image_filename);
+		goto fail;
 	}
+	/* If we aren't generating the bct, read in config file */
+	else if (context.generate_bct == 0)
+		process_config_file(&context, 1);
 	/* Generate the new bct file */
 	else {
 		/* Initialize the bct memory */
@@ -218,7 +260,7 @@ main(int argc, char *argv[])
 		fwrite(context.bct, 1, context.bct_size,
 			context.raw_file);
 		printf("New BCT file %s has been successfully generated!\n",
-			context.image_filename);
+			context.output_image_filename);
 		goto fail;
 	}
 
@@ -234,7 +276,7 @@ main(int argc, char *argv[])
 		printf("Error writing image file.\n");
 	else
 		printf("Image file %s has been successfully generated!\n",
-				context.image_filename);
+				context.output_image_filename);
 
  fail:
 	/* Close the file(s). */
diff --git a/src/cbootimage.h b/src/cbootimage.h
index 252c41e..d8a77d9 100644
--- a/src/cbootimage.h
+++ b/src/cbootimage.h
@@ -37,6 +37,7 @@
 #define MAX_BOOTLOADER_SIZE (16 * 1024 * 1024)
 #define NVBOOT_BOOTDATA_VERSION(a, b) ((((a)&0xffff) << 16) | ((b)&0xffff))
 #define NVBOOT_BAD_BLOCK_TABLE_SIZE 4096
+#define NVBOOT_CONFIG_TABLE_SIZE 8192
 #define NV_MAX(a, b) (((a) > (b)) ? (a) : (b))
 
 #define BOOTDATA_VERSION_T20		NVBOOT_BOOTDATA_VERSION(0x2, 0x1)
@@ -60,7 +61,8 @@ typedef enum
 typedef struct build_image_context_rec
 {
 	FILE *config_file;
-	char *image_filename;
+	char *output_image_filename;
+	char *input_image_filename;
 	FILE *raw_file;
 	u_int32_t block_size;
 	u_int32_t block_size_log2;
@@ -98,6 +100,7 @@ typedef struct build_image_context_rec
 	u_int32_t odm_data; /* The odm data value */
 	u_int8_t unique_chip_id[16]; /* The unique chip uid */
 	u_int8_t secure_jtag_control; /* The flag for enabling jtag control */
+	u_int8_t update_bct;
 } build_image_context;
 
 /* Function prototypes */
diff --git a/src/data_layout.c b/src/data_layout.c
index ba3361a..c3ccbb4 100644
--- a/src/data_layout.c
+++ b/src/data_layout.c
@@ -450,6 +450,7 @@ write_bootloaders(build_image_context *context)
 
 	/* Read the BL into memory. */
 	if (read_from_image(context->newbl_filename,
+		0,
 		&bl_storage,
 		&bl_actual_size,
 		bl_filetype) == 1) {
@@ -657,23 +658,38 @@ init_bct(struct build_image_context_rec *context)
  *   according to the boot data version in bct file.
  *
  * @param context		The main context pointer
+ * @param offset		Begin offset for file read
  * @return 0 for success
  */
 int
-read_bct_file(struct build_image_context_rec *context)
+read_bct_file(struct build_image_context_rec *context, u_int32_t offset)
 {
 	u_int8_t  *bct_storage; /* Holds the Bl after reading */
 	u_int32_t  bct_actual_size; /* In bytes */
-	file_type bct_filetype = file_type_bct;
 	int err = 0;
 
-	if (read_from_image(context->bct_filename,
-		&bct_storage,
-		&bct_actual_size,
-		bct_filetype) == 1) {
-		printf("Error reading bct file %s.\n", context->bct_filename);
-		exit(1);
+	if (context->generate_bct) {
+		if (read_from_image(context->bct_filename,
+			offset,
+			&bct_storage,
+			&bct_actual_size,
+			file_type_bct) == 1) {
+			printf("Error reading bct file %s.\n",
+				context->bct_filename);
+			goto fail;
+		}
+	} else {
+		if (read_from_image(context->input_image_filename,
+			offset,
+			&bct_storage,
+			&bct_actual_size,
+			file_type_bct) == 1) {
+			printf("Error reading image file %s.\n",
+				context->input_image_filename);
+			goto fail;
+		}
 	}
+
 	context->bct_size = bct_actual_size;
 	if (context->bct_init != 1)
 		err = init_bct(context);
@@ -694,6 +710,7 @@ read_bct_file(struct build_image_context_rec *context)
 	if (if_bct_is_t124_get_soc_config(context, &g_soc_config))
 		return 0;
 
+fail:
 	return ENODATA;
 }
 /*
@@ -896,3 +913,39 @@ write_block_raw(build_image_context *context)
 	free(empty_blk);
 	return 0;
 }
+
+int write_bct_raw(build_image_context *context)
+{
+	/* write out the raw bct */
+	if (fwrite(context->bct, 1, NVBOOT_CONFIG_TABLE_SIZE,
+				context->raw_file) != NVBOOT_CONFIG_TABLE_SIZE)
+		return -1;
+
+	return 0;
+}
+
+int write_data_raw(build_image_context *context,
+	u_int32_t offset, u_int32_t size)
+{
+	FILE *fp_input;
+	u_int8_t *bytes;
+	u_int32_t read_size;
+
+	fp_input = fopen(context->input_image_filename, "r");
+	if (!fp_input)
+		return -1;
+
+	bytes = malloc(size);
+
+	/* write out the remained raw image data */
+	if (fseek(fp_input, offset, 0))
+		return -1;
+
+	read_size = fread(bytes, 1, size, fp_input);
+	fwrite(bytes, 1, read_size, context->raw_file);
+
+	free(bytes);
+	fclose(fp_input);
+
+	return 0;
+}
diff --git a/src/data_layout.h b/src/data_layout.h
index 9ee8267..797e465 100644
--- a/src/data_layout.h
+++ b/src/data_layout.h
@@ -41,7 +41,7 @@ void
 update_context(struct build_image_context_rec *context);
 
 int
-read_bct_file(struct build_image_context_rec *context);
+read_bct_file(struct build_image_context_rec *context, u_int32_t offset);
 
 int
 init_bct(struct build_image_context_rec *context);
@@ -50,6 +50,12 @@ int
 write_block_raw(struct build_image_context_rec *context);
 
 int
+write_bct_raw(build_image_context *context);
+
+int
+write_data_raw(build_image_context *context, u_int32_t offset, u_int32_t size);
+
+int
 begin_update(build_image_context *context);
 
 #endif /* #ifndef INCLUDED_DATA_LAYOUT_H */
diff --git a/src/parse.c b/src/parse.c
index 90d37e7..ebc47e9 100644
--- a/src/parse.c
+++ b/src/parse.c
@@ -580,7 +580,7 @@ parse_bct_file(build_image_context *context, parse_token token, char *rest)
 	/* Parsing has finished - set the bctfile */
 	context->bct_filename = filename;
 	/* Read the bct file to buffer */
-	if (read_bct_file(context))
+	if (read_bct_file(context, 0))
 		return 1;
 
 	update_context(context);
@@ -782,6 +782,8 @@ void process_config_file(build_image_context *context, u_int8_t simple_parse)
 	assert(context != NULL);
 	assert(context->config_file != NULL);
 
+	fseek(context->config_file, 0, SEEK_SET);
+
 	while ((current = fgetc(context->config_file)) != EOF) {
 		if (space >= (MAX_BUFFER-1)) {
 			/* if we exceeded the max buffer size, it is likely
diff --git a/src/set.c b/src/set.c
index 99b242c..19580e2 100644
--- a/src/set.c
+++ b/src/set.c
@@ -43,6 +43,7 @@
 
 int
 read_from_image(char	*filename,
+		u_int32_t	offset,
 		u_int8_t	**image,
 		u_int32_t	*actual_size,
 		file_type	f_type)
@@ -57,6 +58,8 @@ read_from_image(char	*filename,
 		return result;
 	}
 
+	fseek(fp, offset, SEEK_SET);
+
 	if (stat(filename, &stats) != 0) {
 		printf("Error: Unable to query info on bootloader path %s\n",
 			filename);
@@ -64,13 +67,22 @@ read_from_image(char	*filename,
 		goto cleanup;
 	}
 
-	*actual_size  = (u_int32_t)stats.st_size;
-
-	if (f_type == file_type_bl && *actual_size > MAX_BOOTLOADER_SIZE) {
-		printf("Error: Bootloader file %s is too large.\n",
-			filename);
-		result = 1;
-		goto cleanup;
+	if (f_type == file_type_bl) {
+		if ((stats.st_size - offset) > MAX_BOOTLOADER_SIZE) {
+			printf("Error: Bootloader file %s is too large.\n",
+				filename);
+			result = 1;
+			goto cleanup;
+		}
+		*actual_size = (u_int32_t)stats.st_size;
+	} else {
+		if ((stats.st_size - offset) < NVBOOT_CONFIG_TABLE_SIZE) {
+			printf("Error: Image file %s is too small.\n",
+				filename);
+			result = 1;
+			goto cleanup;
+		}
+		*actual_size = NVBOOT_CONFIG_TABLE_SIZE;
 	}
 	*image = malloc(*actual_size);
 	if (*image == NULL) {
@@ -80,7 +92,8 @@ read_from_image(char	*filename,
 
 	memset(*image, 0, *actual_size);
 
-	if (fread(*image, 1, (size_t)stats.st_size, fp) != stats.st_size) {
+	if (fread(*image, 1, (size_t)(*actual_size), fp) !=
+		(size_t)(*actual_size)) {
 		result = 1;
 		goto cleanup;
 	}
diff --git a/src/set.h b/src/set.h
index dbde479..b2c77ec 100644
--- a/src/set.h
+++ b/src/set.h
@@ -46,6 +46,7 @@ context_set_chipuid(build_image_context	*context,
 
 int
 read_from_image(char *filename,
+		u_int32_t	offset,
 		u_int8_t	**Image,
 		u_int32_t	*actual_size,
 		file_type	f_type);
-- 
1.9.0

Re: [cbootimage PATCH 1/2] Add Tegra124 bct data access for jtag control and chip uid

[Stephen Warren]

On 03/21/2014 03:41 AM, Penny Chiu wrote:
> Add support for read secure_jtag_control and unique_chip_id from
> cfg file and write them into BCT structure, and bct_dump can also
> parse the two fields and show the data.

> diff --git a/src/bct_dump.c b/src/bct_dump.c

> @@ -37,7 +37,9 @@ static value_data const values[] = {
>  	{ token_block_size_log2,     "BlockSize     = 0x%08x;\n" },
>  	{ token_page_size_log2,      "PageSize      = 0x%08x;\n" },
>  	{ token_partition_size,      "PartitionSize = 0x%08x;\n" },
> -	{ token_odm_data,            "OdmData       = 0x%08x;\n\n" },
> +	{ token_odm_data,            "OdmData       = 0x%08x;\n" },
> +	{ token_secure_jtag_control, "JtagCtrl      = 0x%08x;\n" },
> +	{ token_unique_chip_id,      "ChipUid       = %s;\n" },

Please (in a separate patch up-front) add an additional field to this
table, which is the function to use to format the data value. I would
expect the result to be:

{ token_page_size_log2,      "PageSize      = ", format_u32_hex8 },
{ token_partition_size,      "PartitionSize = ", format_u32_hex8 },
{ token_odm_data,            "OdmData       = ", format_u32_hex8 },
{ token_secure_jtag_control, "JtagCtrl      = ", format_u32_hex8 },
{ token_unique_chip_id,      "ChipUid       = ", format_chipuid },

This would (a) make it consistent with parse_item s_top_level_items[] in
src/parse.c (b) remove the need for the dumping loop in main() to make
custom decisions about particular tokens; everything can be moved into
the format_xxx() functions.

> @@ -154,17 +156,34 @@ int main(int argc, char *argv[])
>  
>  	/* Display root values */
>  	for (i = 0; i < sizeof(values) / sizeof(values[0]); ++i) {
> +		if (values[i].id == token_unique_chip_id) {
> +			u_int8_t uid[16];
> +			char uid_str[35] = "0x";
>  
> +			e = g_soc_config->get_value(values[i].id,
> +							(u_int32_t *)uid,
> +							context.bct);
> +			if (e != 0)
> +				continue;

If there's an error getting the value, shouldn't the tool at least warn
the user, if not return an error? Why would get_value() ever return an
error?

Please send a separate patch to eliminate the need for the (u_int32_t *)
cast. get/set_value() should accept a void* not a uint32_t*. Same for
context_get/set_value(). That way, you won't need to introduce separate
functions for context_get/set_chipuid().

> +
> +			for (j = 15; j >= 0; j--)
> +				sprintf(uid_str, "%s%02x", uid_str, uid[j]);

That's a bit dangerous; I would guess some implementations of sprintf()
could end up in an infinite loop there. It would be better to do
something more like:

char *s = uid_str;
for (j = 15; j >= 0; j--, s += 2)
	sprintf(s, "%02x", uid[j]);

Also, why not use a more descriptive variable name than j - perhaps
"byte_index"?

...
> +			e = g_soc_config->get_value(values[i].id,
> +							&data,
> +							context.bct);
> +
> +			if (e != 0) {
> +				if (values[i].id == token_secure_jtag_control)
> +					continue;
> +				data = -1;

Why would this ever fail? I think the tool should error out if it does.

Perhaps this is because only some tokens are only supported on some
SoCs? If so, the top of the loop should really look like:

for (i = 0; i < sizeof(values) / sizeof(values[0]); ++i) {
	if (!g_soc_config->token_supported(values[i].id))
		continue;

> diff --git a/src/parse.c b/src/parse.c

> +/*
> + * Parse the given string and transfer to chip uid.
> + *
> + * @param str		String to parse
> + * @param chipuid	Returns chip uid that was parsed
> + * @return the remainder of the string after the number was parsed
> + */
> +static char *
> +parse_chipuid(char *str, u_int8_t *chipuid)
> +{
> +	while (*str == '0')
> +		str++;
> +
> +	if (tolower(*str) == 'x') {

Why allow more than one 0, and why ignore and mis-formatted data (i.e.
the character after 0 not being x)?

This should be:

if (*str++ != '0')
	raise an error;
if (*str++ != 'x')
	raise an error;

> +		str++;
> +		paddings = strlen(str) % 2;
> +		byte_index = strlen(str) / 2 + paddings;

Please validate that byte_index is not out-of-range, so that writes to
*chip_uid don't overflow the buffer.

> +		while (*str != '\0' && byte_index > 0) {
> +			u_int32_t value = 0;
> +
> +			strncpy(byte_str, str, 2 - paddings);
> +			byte_str[2-paddings] = '\0';
> +			str = str + 2 - paddings;
> +
> +			sscanf(byte_str, "%x", &value);
> +			*(chipuid + byte_index - 1) = value;

Uggh. Why strcpy() the data at all, and why use the heavy-weight
sscanf() for data conversion?

I assume the input data can be modified. If not, then just strdup() the
whole string in 1 pass.

Instead, since the string is parsed backwards, you can write a NULL
immediately after every chunk to be parsed. Something like the code
below. Also, use strtoul() to convert rather than sscanf(); it's much
simpler. While you're at it, please send a separate patch to fix all the
same issues in parse_u32; it's basically re-implementing strtoul()!

while (*str != '\0' && byte_index > 0) {
	char *endptr;
	chipuid[byte_index] = strtoul(str, &endptr, 16);
	if (*endptr)
		raise an error;
	byte_index--;
	*str = 0;
	str += 2 - paddings;
	paddings = 0;
}

> +static int parse_value_chipuid(build_image_context *context,
...
> +	return context_set_chipuid(context, value);

As mentioned above, this should be able to use context_set_value(), by
fixing that function's prototype.

> diff --git a/src/set.c b/src/set.c

> +/*
> + * General handler for setting chip uid in config files.
> + *
> + * @param context	The main context pointer
> + * @param data		The value to set
> + * @return 0 for success
> + */
> +int context_set_chipuid(build_image_context *context,

Please fix context_set_value() to handle the UID token type, rather than
introducing a separate function for this one token.

Re: [cbootimage PATCH 1/2] Add Tegra124 bct data access for jtag control and chip uid

[Stephen Warren]

On 03/21/2014 03:41 AM, Penny Chiu wrote:
...
> @@ -940,6 +941,12 @@ t124_bct_get_value(parse_token id, u_int32_t *data, u_int8_t *bct)

> +	case token_unique_chip_id:
> +		memcpy(data,
> +		&(bct_ptr->unique_chip_id),
> +		sizeof(nvboot_ecid));
> +		break;

The indentation there is wrong; parameters need to be indented at least
one level more than the function they're being passed to, and I think
you can wrap at least some of them one fewer lines.

Re: [cbootimage PATCH 2/2] Add update BCT configs feature

[Stephen Warren]

On 03/21/2014 03:41 AM, Penny Chiu wrote:
> This feature reads the BCT data from BCT or BCT with bootloader
> appended binary, updates the BCT data based on config file, then
> writes to new image file.

> diff --git a/src/cbootimage.c b/src/cbootimage.c

> @@ -131,10 +136,14 @@ process_command_line(int argc, char *argv[], build_image_context *context)
>  		case 'o':
>  			context->odm_data = strtoul(optarg, NULL, 16);
>  			break;
> +		case 'u':
> +			context->update_bct = 1;

I think rename update_bct to update_image; I believe the -u flag can be
used to update a file containing any of:

1) Just a BCT
2) A BCT, with random data appended to the end
3) A BCT, plus bootloader, plus padding
4) A BCT, plus bootloader, plus random data in between or appended to
the end.

(If the current patch doesn't cover all those cases, it probably should,
or at the very least, both (1) and (3)).

> +			num_of_filenames = 3;

s/num_of_filenames/num_filenames/

>  	/* Open the raw output file. */
> -	context.raw_file = fopen(context.image_filename,
> +	context.raw_file = fopen(context.output_image_filename,
>  		                 "w+");

Does "w+" really need to be wrapped onto a separate line?

> +		while (stats.st_size > offset) {
> +			if (!read_bct_file(&context, offset)) {
> +				process_config_file(&context, 0);
> +				e = sign_bct(&context, context.bct);
> +				if (e != 0) {
> +					printf("Signing BCT failed, error: %d.\n", e);
> +					goto fail;
> +				}
> +				write_bct_raw(&context);
> +			} else
> +				write_data_raw(&context, offset, NVBOOT_CONFIG_TABLE_SIZE);

Two things look wrong with this:

a) Presumably read_bct_file() can fail for a number of reasons:
1) I/O error
2) The data at that offset was read from the file OK, but isn't a BCT.

This loop doesn't appear to distinguish between those two cases.

b) The code appears to assume that even if read_bct_file() returns an
error, the file data has been read in and cached somewhere, so that
write_data_raw() can copy it to the output file. This is rather
implicit, and doesn't feel right. I think it would be better for the
loop to:

read a block of data into memory
if the memory is a valid BCT:
    update the BCT
write the block of data to the output file

... with file I/O and in particular, storage for "the block of data"
being owned by the loop, rather than implicitly in &context. Or at the
very least, split read_data_from_file() into a separate function from
read_bct_file(), and call data_is_valid_bct() rather than
read_bct_file() to identify the memory content.

Actually, now that I look at the implementation of write_data_raw(),
that function is not just writing but actually copying the data. So, the
function is named incorrectly. That's quite inefficient, since
read_bct_file() would have read the data once, and then write_data_raw()
reads it again to copy it. If the loop was reworked as I describe above,
this double read could be eliminated.

> +
> +			offset += NVBOOT_CONFIG_TABLE_SIZE;
...
> diff --git a/src/cbootimage.h b/src/cbootimage.h
...
> +#define NVBOOT_CONFIG_TABLE_SIZE 8192

That value isn't a constant. Different memory devices (with differnt
page/block sizes) and/or different SoCs cause BCTs to be aligned to
different boundaries. I think you might want to either replace the
#define with some dynamic function (which might involve reading the
original BCT configuration file), or at least drop the value to some
much smaller minimum alignment (say 256?), and do something a bit more
complicated to search for BCTs (e.g. read the whole file into memory,
then step through it 256 bytes at a time to find all valid BCTs).

You might want to consult with the boot ROM team to find out what the
minimum page size is, and/or whether the boot ROM hard-codes 8192, or
whether the value is dynamic based on memory type etc.

>  typedef struct build_image_context_rec
...
> +	u_int8_t update_bct;

You might want to merge the existing generate_bct field and this new
field into a tri-state enum (generate BCT, generate image, update image)

> diff --git a/src/data_layout.c b/src/data_layout.c

> +read_bct_file(struct build_image_context_rec *context, u_int32_t offset)

> +	if (context->generate_bct) {
> +		if (read_from_image(context->bct_filename,
> +			offset,
> +			&bct_storage,
> +			&bct_actual_size,
> +			file_type_bct) == 1) {
> +			printf("Error reading bct file %s.\n",
> +				context->bct_filename);
> +			goto fail;
> +		}
> +	} else {
> +		if (read_from_image(context->input_image_filename,
> +			offset,
> +			&bct_storage,
> +			&bct_actual_size,
> +			file_type_bct) == 1) {
> +			printf("Error reading image file %s.\n",
> +				context->input_image_filename);
> +			goto fail;
> +		}
>  	}

Why not pass the filename into the function, rather than putting
"application logic" into a utility function?

If this function must determine the filename, rather than having it
passed in, then the indentation of the continuation parameters should be
different from the indentation of the code block that follows. In other
words, instead of:

if (function(param,
	param,
	param))
	code

do this:

if (function(param,
		param,
		param))
	code

At least some of the parameters can be wrapped onto fewer lines.

Finally, there's no need to duplicate the entire call to
read_from_image() just to change the value of one parameter. Instead, do
this:

char *fn, *fdesc;

if (context->generate_bct) {
	fn = context->bct_filename;
	fdesc = "bct";
} else {
	fn = context->input_image_filename;
	fdesc = "image";
}
if (read_from_image(context->bct_filename, offset,
		&bct_storage, &bct_actual_size,	file_type_bct) == 1) {
	printf("Error reading %s file %s.\n", fdesc, fn);
	goto fail;
}

> +int write_bct_raw(build_image_context *context)
> +{
> +	/* write out the raw bct */
> +	if (fwrite(context->bct, 1, NVBOOT_CONFIG_TABLE_SIZE,
> +				context->raw_file) != NVBOOT_CONFIG_TABLE_SIZE)

The BCT isn't always 8192 bytes. It looks like on Tegra20 it's 4080
bytes, and on Tegra30 it's 6128 bytes.

Now, perhaps the BCT in an image is always padded out to some
sector-/page-aligned size (which as I mentioned above still might not be
8192 bytes), but if the code is updating just a BCT file not a full
image file, this code won't work correctly.

> +int write_data_raw(build_image_context *context,
> +	u_int32_t offset, u_int32_t size)

This function doesn't write some data to a file, but rather copies it
from one file to another. copy_data_raw() would be a better function
name. However, please see my comments on the main loop above; it would
be better for the whole process to be:

* Read entire input image into RAM
* Do all data manipulation in RAM
* Write entire output image to output file

I think if you do that, you can even have use the existing
write_image_file() to create the output file at the end of the
update-image process.

Perhaps both the create-new-image and update-image cases be basically
the same:

if (!context.generate_bct) {
	if (context.update_bct)
		read existing image into RAM
	else
		create empty in-RAM data structure
	process_config_file();
	write_image_file();
} else

... although process_config_file() would need to iterate over all
pre-existing BCTs it found in RAM rather than generating new BCTs; I
don't know how easy that is. I don't expect it's that hard though?

> @@ -782,6 +782,8 @@ void process_config_file(build_image_context *context, u_int8_t simple_parse)
>  	assert(context != NULL);
>  	assert(context->config_file != NULL);
>  
> +	fseek(context->config_file, 0, SEEK_SET);

This should really be part of the main loop of the update-image code;
it's not needed as part of the normal create-image handling.

> diff --git a/src/set.c b/src/set.c

> @@ -64,13 +67,22 @@ read_from_image(char	*filename,
>  		goto cleanup;
>  	}
>  
> +	if (f_type == file_type_bl) {
> +		if ((stats.st_size - offset) > MAX_BOOTLOADER_SIZE) {
> +			printf("Error: Bootloader file %s is too large.\n",
> +				filename);
> +			result = 1;
> +			goto cleanup;
> +		}
> +		*actual_size = (u_int32_t)stats.st_size;
> +	} else {
> +		if ((stats.st_size - offset) < NVBOOT_CONFIG_TABLE_SIZE) {
> +			printf("Error: Image file %s is too small.\n",
> +				filename);
> +			result = 1;
> +			goto cleanup;
> +		}
> +		*actual_size = NVBOOT_CONFIG_TABLE_SIZE;
>  	}

Again, NVBOOT_CONFIG_TABLE_SIZE isn't correct for earlier SoCs.