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

Message ID	1395394886-8145-3-git-send-email-pchiu@nvidia.com
State	Superseded, archived
Headers	show Return-Path: <linux-tegra-owner@vger.kernel.org> From: Penny Chiu <pchiu@nvidia.com> To: <swarren@nvidia.com>, <amartin@nvidia.com>, <linux-tegra@vger.kernel.org> CC: Penny Chiu <pchiu@nvidia.com> Subject: [cbootimage PATCH 2/2] Add update BCT configs feature Date: Fri, 21 Mar 2014 17:41:26 +0800 Message-ID: <1395394886-8145-3-git-send-email-pchiu@nvidia.com> In-Reply-To: <1395394886-8145-1-git-send-email-pchiu@nvidia.com> References: <1395394886-8145-1-git-send-email-pchiu@nvidia.com> MIME-Version: 1.0 Content-Type: text/plain Sender: linux-tegra-owner@vger.kernel.org Precedence: bulk

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. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html

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);

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

Commit Message

Comments

Patch