From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Nelson Date: Sat, 8 Oct 2016 17:26:18 +0200 Subject: [U-Boot] [PATCH V3 1/8] tools: imximage: add plugin support In-Reply-To: <1475909892-8827-1-git-send-email-peng.fan@nxp.com> References: <1475909892-8827-1-git-send-email-peng.fan@nxp.com> Message-ID: <2c634318-4a3f-9b2d-cfd9-7b4b211df78c@nelint.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Peng, I'm sorry for taking so long to go though this. On 10/08/2016 08:58 AM, Peng Fan wrote: > Add plugin support for imximage. > This CONFIG setting doesn't actually affect mkimage or imximage: > Define CONFIG_USE_IMXIMG_PLUGIN in defconfig to enable using plugin. > > Signed-off-by: Peng Fan > Cc: Stefano Babic > Cc: Eric Nelson > Cc: Ye Li > Reviewed-by: Tom Rini > --- > > V3: > Fix compile error. > > V2: > Drop the CONFIG_USE_PLUGIN, make plugin always support in imximage. > > tools/imximage.c | 282 +++++++++++++++++++++++++++++++++++++++++++------------ > tools/imximage.h | 7 +- > 2 files changed, 229 insertions(+), 60 deletions(-) > > diff --git a/tools/imximage.c b/tools/imximage.c > index 092d550..7fa601e 100644 > --- a/tools/imximage.c > +++ b/tools/imximage.c > @@ -27,6 +27,7 @@ static table_entry_t imximage_cmds[] = { > {CMD_CHECK_BITS_CLR, "CHECK_BITS_CLR", "Reg Check bits clr", }, > {CMD_CSF, "CSF", "Command Sequence File", }, > {CMD_IMAGE_VERSION, "IMAGE_VERSION", "image version", }, > + {CMD_PLUGIN, "PLUGIN", "file plugin_addr", }, > {-1, "", "", }, > }; > > @@ -80,6 +81,9 @@ static uint32_t imximage_ivt_offset = UNDEFINED; > static uint32_t imximage_csf_size = UNDEFINED; > /* Initial Load Region Size */ > static uint32_t imximage_init_loadsize; These seem very limiting. >From what I can tell, there's no reason that you can't have multiple plugins, and the use of these variables isn't really needed. > +static uint32_t imximage_iram_free_start; > +static uint32_t imximage_plugin_size; > +static uint32_t plugin_image; > > static set_dcd_val_t set_dcd_val; > static set_dcd_param_t set_dcd_param; > @@ -118,7 +122,11 @@ static uint32_t detect_imximage_version(struct imx_header *imx_hdr) > > /* Try to detect V2 */ > if ((fhdr_v2->header.tag == IVT_HEADER_TAG) && > - (hdr_v2->dcd_table.header.tag == DCD_HEADER_TAG)) > + (hdr_v2->data.dcd_table.header.tag == DCD_HEADER_TAG)) > + return IMXIMAGE_V2; > + > + if ((fhdr_v2->header.tag == IVT_HEADER_TAG) && > + hdr_v2->boot_data.plugin) > return IMXIMAGE_V2; > > return IMXIMAGE_VER_INVALID; > @@ -165,7 +173,7 @@ static struct dcd_v2_cmd *gd_last_cmd; > static void set_dcd_param_v2(struct imx_header *imxhdr, uint32_t dcd_len, > int32_t cmd) > { I also don't understand why the choice to make the union with either a DCD table or a plugin. We should be able to have both, and this doesn't make the code any easier. > - dcd_v2_t *dcd_v2 = &imxhdr->header.hdr_v2.dcd_table; > + dcd_v2_t *dcd_v2 = &imxhdr->header.hdr_v2.data.dcd_table; > struct dcd_v2_cmd *d = gd_last_cmd; > struct dcd_v2_cmd *d2; > int len; > @@ -261,21 +269,23 @@ static void set_dcd_rst_v1(struct imx_header *imxhdr, uint32_t dcd_len, > static void set_dcd_rst_v2(struct imx_header *imxhdr, uint32_t dcd_len, > char *name, int lineno) > { > - dcd_v2_t *dcd_v2 = &imxhdr->header.hdr_v2.dcd_table; > - struct dcd_v2_cmd *d = gd_last_cmd; > - int len; > - > - if (!d) > - d = &dcd_v2->dcd_cmd; > - len = be16_to_cpu(d->write_dcd_command.length); > - if (len > 4) > - d = (struct dcd_v2_cmd *)(((char *)d) + len); > - > - len = (char *)d - (char *)&dcd_v2->header; > - > - dcd_v2->header.tag = DCD_HEADER_TAG; > - dcd_v2->header.length = cpu_to_be16(len); > - dcd_v2->header.version = DCD_VERSION; > + if (!imxhdr->header.hdr_v2.boot_data.plugin) { > + dcd_v2_t *dcd_v2 = &imxhdr->header.hdr_v2.data.dcd_table; > + struct dcd_v2_cmd *d = gd_last_cmd; > + int len; > + > + if (!d) > + d = &dcd_v2->dcd_cmd; > + len = be16_to_cpu(d->write_dcd_command.length); > + if (len > 4) > + d = (struct dcd_v2_cmd *)(((char *)d) + len); > + > + len = (char *)d - (char *)&dcd_v2->header; > + > + dcd_v2->header.tag = DCD_HEADER_TAG; > + dcd_v2->header.length = cpu_to_be16(len); > + dcd_v2->header.version = DCD_VERSION; > + } > } > > static void set_imx_hdr_v1(struct imx_header *imxhdr, uint32_t dcd_len, > @@ -317,24 +327,93 @@ static void set_imx_hdr_v2(struct imx_header *imxhdr, uint32_t dcd_len, > fhdr_v2->header.length = cpu_to_be16(sizeof(flash_header_v2_t)); > fhdr_v2->header.version = IVT_VERSION; /* 0x40 */ > It seems that the reason for a lot of this special-purpose code is to add support for the entry address. If mkimage is invoked with an entrypoint from the command-line: ~/$ ./tools/mkimage -n my.cfg -T imximage -e 0xentrypoint u-boot.img u-boot.imx This entry point is normally placed into the header, but if we have a plugin in the .cfg file like this: ~/$ grep PLUGIN my.cfg PLUGIN path/to/board/plugin.bin 0x00907000 Then we need to use the plugin's address instead of the command line address, and use the command-line address for the file which follows. > - fhdr_v2->entry = entry_point; > - fhdr_v2->reserved1 = fhdr_v2->reserved2 = 0; > - hdr_base = entry_point - imximage_init_loadsize + > - flash_offset; > - fhdr_v2->self = hdr_base; > - if (dcd_len > 0) > - fhdr_v2->dcd_ptr = hdr_base > - + offsetof(imx_header_v2_t, dcd_table); > - else > + if (!hdr_v2->boot_data.plugin) { > + fhdr_v2->entry = entry_point; > + fhdr_v2->reserved1 = 0; > + fhdr_v2->reserved1 = 0; > + hdr_base = entry_point - imximage_init_loadsize + > + flash_offset; > + fhdr_v2->self = hdr_base; > + if (dcd_len > 0) > + fhdr_v2->dcd_ptr = hdr_base + > + offsetof(imx_header_v2_t, data); > + else > + fhdr_v2->dcd_ptr = 0; > + fhdr_v2->boot_data_ptr = hdr_base > + + offsetof(imx_header_v2_t, boot_data); > + hdr_v2->boot_data.start = entry_point - imximage_init_loadsize; > + > + fhdr_v2->csf = 0; > + > + header_size_ptr = &hdr_v2->boot_data.size; > + csf_ptr = &fhdr_v2->csf; > + } else { > + imx_header_v2_t *next_hdr_v2; > + flash_header_v2_t *next_fhdr_v2; > + > + if (imximage_csf_size != 0) { > + fprintf(stderr, "Error: Header v2: SECURE_BOOT is only supported in DCD mode!"); > + exit(EXIT_FAILURE); > + } > + I think that only ->entry needs to be different between these two blocks. > + fhdr_v2->entry = imximage_iram_free_start + > + flash_offset + sizeof(flash_header_v2_t) + > + sizeof(boot_data_t); > + > + fhdr_v2->reserved1 = 0; > + fhdr_v2->reserved2 = 0; > + fhdr_v2->self = imximage_iram_free_start + flash_offset; > + > fhdr_v2->dcd_ptr = 0; > - fhdr_v2->boot_data_ptr = hdr_base > - + offsetof(imx_header_v2_t, boot_data); > - hdr_v2->boot_data.start = entry_point - imximage_init_loadsize; > > - fhdr_v2->csf = 0; > + fhdr_v2->boot_data_ptr = fhdr_v2->self + > + offsetof(imx_header_v2_t, boot_data); > + > + hdr_v2->boot_data.start = imximage_iram_free_start; > + /* > + * The actural size of plugin image is "imximage_plugin_size + > + * sizeof(flash_header_v2_t) + sizeof(boot_data_t)", plus the > + * flash_offset space.The ROM code only need to copy this size > + * to run the plugin code. However, later when copy the whole > + * U-Boot image to DDR, the ROM code use memcpy to copy the > + * first part of the image, and use the storage read function > + * to get the remaining part. This requires the dividing point > + * must be multiple of storage sector size. Here we set the > + * first section to be 16KB for this purpose. > + */ Where is the 16k limit coming from? I don't think this is necessary, and if it is, we won't be able to load SPL using a plugin. > + hdr_v2->boot_data.size = MAX_PLUGIN_CODE_SIZE; > + > + /* Security feature are not supported */ > + fhdr_v2->csf = 0; > + > + next_hdr_v2 = (imx_header_v2_t *)((char *)hdr_v2 + > + imximage_plugin_size); > + > + next_fhdr_v2 = &next_hdr_v2->fhdr; > + > + next_fhdr_v2->header.tag = IVT_HEADER_TAG; /* 0xD1 */ > + next_fhdr_v2->header.length = > + cpu_to_be16(sizeof(flash_header_v2_t)); > + next_fhdr_v2->header.version = IVT_VERSION; /* 0x40 */ > + > + next_fhdr_v2->entry = entry_point; > + hdr_base = entry_point - sizeof(struct imx_header); > + next_fhdr_v2->reserved1 = 0; > + next_fhdr_v2->reserved2 = 0; > + next_fhdr_v2->self = hdr_base + imximage_plugin_size; > + > + next_fhdr_v2->dcd_ptr = 0; > + next_fhdr_v2->boot_data_ptr = next_fhdr_v2->self + > + offsetof(imx_header_v2_t, boot_data); > + > + next_hdr_v2->boot_data.start = hdr_base - flash_offset; > + > + header_size_ptr = &next_hdr_v2->boot_data.size; > > - header_size_ptr = &hdr_v2->boot_data.size; > - csf_ptr = &fhdr_v2->csf; > + next_hdr_v2->boot_data.plugin = 0; > + > + next_fhdr_v2->csf = 0; > + } > } > > static void set_hdr_func(void) > @@ -393,16 +472,19 @@ static void print_hdr_v2(struct imx_header *imx_hdr) > { > imx_header_v2_t *hdr_v2 = &imx_hdr->header.hdr_v2; > flash_header_v2_t *fhdr_v2 = &hdr_v2->fhdr; > - dcd_v2_t *dcd_v2 = &hdr_v2->dcd_table; > - uint32_t size, version; > + dcd_v2_t *dcd_v2 = &hdr_v2->data.dcd_table; > + uint32_t size, version, plugin; > > - size = be16_to_cpu(dcd_v2->header.length); > - if (size > (MAX_HW_CFG_SIZE_V2 * sizeof(dcd_addr_data_t)) + 8) { > - fprintf(stderr, > - "Error: Image corrupt DCD size %d exceed maximum %d\n", > - (uint32_t)(size / sizeof(dcd_addr_data_t)), > - MAX_HW_CFG_SIZE_V2); > - exit(EXIT_FAILURE); > + plugin = hdr_v2->boot_data.plugin; > + if (!plugin) { > + size = be16_to_cpu(dcd_v2->header.length); > + if (size > (MAX_HW_CFG_SIZE_V2 * sizeof(dcd_addr_data_t))) { > + fprintf(stderr, > + "Error: Image corrupt DCD size %d exceed maximum %d\n", > + (uint32_t)(size / sizeof(dcd_addr_data_t)), > + MAX_HW_CFG_SIZE_V2); > + exit(EXIT_FAILURE); > + } > } > > version = detect_imximage_version(imx_hdr); > @@ -410,19 +492,81 @@ static void print_hdr_v2(struct imx_header *imx_hdr) > printf("Image Type: Freescale IMX Boot Image\n"); > printf("Image Ver: %x", version); > printf("%s\n", get_table_entry_name(imximage_versions, NULL, version)); > - printf("Data Size: "); > - genimg_print_size(hdr_v2->boot_data.size); > - printf("Load Address: %08x\n", (uint32_t)fhdr_v2->boot_data_ptr); > - printf("Entry Point: %08x\n", (uint32_t)fhdr_v2->entry); > - if (fhdr_v2->csf && (imximage_ivt_offset != UNDEFINED) && > - (imximage_csf_size != UNDEFINED)) { > - printf("HAB Blocks: %08x %08x %08x\n", > - (uint32_t)fhdr_v2->self, 0, > - hdr_v2->boot_data.size - imximage_ivt_offset - > - imximage_csf_size); > + printf("Mode: %s\n", plugin ? "PLUGIN" : "DCD"); > + if (!plugin) { > + printf("Data Size: "); > + genimg_print_size(hdr_v2->boot_data.size); > + printf("Load Address: %08x\n", (uint32_t)fhdr_v2->boot_data_ptr); > + printf("Entry Point: %08x\n", (uint32_t)fhdr_v2->entry); > + if (fhdr_v2->csf && (imximage_ivt_offset != UNDEFINED) && > + (imximage_csf_size != UNDEFINED)) { > + printf("HAB Blocks: %08x %08x %08x\n", > + (uint32_t)fhdr_v2->self, 0, > + hdr_v2->boot_data.size - imximage_ivt_offset - > + imximage_csf_size); > + } > + } else { > + imx_header_v2_t *next_hdr_v2; > + flash_header_v2_t *next_fhdr_v2; > + > + /*First Header*/ > + printf("Plugin Data Size: "); > + genimg_print_size(hdr_v2->boot_data.size); > + printf("Plugin Code Size: "); > + genimg_print_size(imximage_plugin_size); > + printf("Plugin Load Address: %08x\n", hdr_v2->boot_data.start); > + printf("Plugin Entry Point: %08x\n", (uint32_t)fhdr_v2->entry); > + > + /*Second Header*/ > + next_hdr_v2 = (imx_header_v2_t *)((char *)hdr_v2 + > + imximage_plugin_size); > + next_fhdr_v2 = &next_hdr_v2->fhdr; > + printf("U-Boot Data Size: "); > + genimg_print_size(next_hdr_v2->boot_data.size); > + printf("U-Boot Load Address: %08x\n", > + next_hdr_v2->boot_data.start); > + printf("U-Boot Entry Point: %08x\n", > + (uint32_t)next_fhdr_v2->entry); > } > } > > +static void copy_plugin_code(struct imx_header *imxhdr, char *plugin_file) > +{ > + int ifd = -1; > + struct stat sbuf; > + char *plugin_buf = imxhdr->header.hdr_v2.data.plugin_code; > + char *ptr; > + > + ifd = open(plugin_file, O_RDONLY|O_BINARY); > + if (fstat(ifd, &sbuf) < 0) { > + fprintf(stderr, "Can't stat %s: %s\n", > + plugin_file, > + strerror(errno)); > + exit(EXIT_FAILURE); > + } > + > + ptr = mmap(0, sbuf.st_size, PROT_READ, MAP_SHARED, ifd, 0); > + if (ptr == MAP_FAILED) { > + fprintf(stderr, "Can't read %s: %s\n", > + plugin_file, > + strerror(errno)); > + exit(EXIT_FAILURE); > + } > + > + if (sbuf.st_size > MAX_PLUGIN_CODE_SIZE) { > + printf("plugin binary size too large\n"); > + exit(EXIT_FAILURE); > + } > + > + memcpy(plugin_buf, ptr, sbuf.st_size); > + imximage_plugin_size = sbuf.st_size; > + > + (void) munmap((void *)ptr, sbuf.st_size); > + (void) close(ifd); > + > + imxhdr->header.hdr_v2.boot_data.plugin = 1; > +} > + > static void parse_cfg_cmd(struct imx_header *imxhdr, int32_t cmd, char *token, > char *name, int lineno, int fld, int dcd_len) > { > @@ -497,6 +641,10 @@ static void parse_cfg_cmd(struct imx_header *imxhdr, int32_t cmd, char *token, > if (unlikely(cmd_ver_first != 1)) > cmd_ver_first = 0; > break; This structure of parse_cfg_cmd to handle the first argument and parse_cfg_fld to handle the second argument is unfortunate. > + case CMD_PLUGIN: > + plugin_image = 1; > + copy_plugin_code(imxhdr, token); > + break; > } > } > > @@ -542,6 +690,10 @@ static void parse_cfg_fld(struct imx_header *imxhdr, int32_t *cmd, > } > } > break; > + case CMD_PLUGIN: > + value = get_cfg_value(token, name, lineno); > + imximage_iram_free_start = value; > + break; > default: > break; > } > @@ -649,6 +801,7 @@ static void imximage_set_header(void *ptr, struct stat *sbuf, int ifd, > { > struct imx_header *imxhdr = (struct imx_header *)ptr; > uint32_t dcd_len; > + uint32_t header_size; > > /* > * In order to not change the old imx cfg file > @@ -665,10 +818,15 @@ static void imximage_set_header(void *ptr, struct stat *sbuf, int ifd, > dcd_len = parse_cfg_file(imxhdr, params->imagename); > > if (imximage_version == IMXIMAGE_V2) { > - if (imximage_init_loadsize < imximage_ivt_offset + > - sizeof(imx_header_v2_t)) > + header_size = sizeof(flash_header_v2_t) + sizeof(boot_data_t); > + if (!plugin_image) > + header_size += sizeof(dcd_v2_t); > + else > + header_size += MAX_PLUGIN_CODE_SIZE; > + > + if (imximage_init_loadsize < imximage_ivt_offset + header_size) > imximage_init_loadsize = imximage_ivt_offset + > - sizeof(imx_header_v2_t); > + header_size; > } > > /* Set the imx header */ > @@ -721,7 +879,7 @@ static int imximage_generate(struct image_tool_params *params, > size_t alloc_len; > struct stat sbuf; > char *datafile = params->datafile; > - uint32_t pad_len; > + uint32_t pad_len, header_size; > > memset(&imximage_header, 0, sizeof(imximage_header)); > > @@ -742,15 +900,21 @@ static int imximage_generate(struct image_tool_params *params, > /* TODO: check i.MX image V1 handling, for now use 'old' style */ > if (imximage_version == IMXIMAGE_V1) { > alloc_len = 4096; > + header_size = 4096; > } else { > - if (imximage_init_loadsize < imximage_ivt_offset + > - sizeof(imx_header_v2_t)) > + header_size = sizeof(flash_header_v2_t) + sizeof(boot_data_t); > + if (!plugin_image) > + header_size += sizeof(dcd_v2_t); > + else > + header_size += MAX_PLUGIN_CODE_SIZE; > + > + if (imximage_init_loadsize < imximage_ivt_offset + header_size) > imximage_init_loadsize = imximage_ivt_offset + > - sizeof(imx_header_v2_t); > + header_size; > alloc_len = imximage_init_loadsize - imximage_ivt_offset; > } > > - if (alloc_len < sizeof(struct imx_header)) { > + if (alloc_len < header_size) { > fprintf(stderr, "%s: header error\n", > params->cmdname); > exit(EXIT_FAILURE); > diff --git a/tools/imximage.h b/tools/imximage.h > index c7b9b5c..fe4dd89 100644 > --- a/tools/imximage.h > +++ b/tools/imximage.h > @@ -9,6 +9,7 @@ > #define _IMXIMAGE_H_ > > #define MAX_HW_CFG_SIZE_V2 220 /* Max number of registers imx can set for v2 */ > +#define MAX_PLUGIN_CODE_SIZE (16*1024) > #define MAX_HW_CFG_SIZE_V1 60 /* Max number of registers imx can set for v1 */ > #define APP_CODE_BARKER 0xB1 > #define DCD_BARKER 0xB17219E9 > @@ -64,6 +65,7 @@ enum imximage_cmd { > CMD_CHECK_BITS_SET, > CMD_CHECK_BITS_CLR, > CMD_CSF, > + CMD_PLUGIN, > }; > > enum imximage_fld_types { > @@ -164,7 +166,10 @@ typedef struct { > typedef struct { > flash_header_v2_t fhdr; > boot_data_t boot_data; > - dcd_v2_t dcd_table; > + union { > + dcd_v2_t dcd_table; > + char plugin_code[MAX_PLUGIN_CODE_SIZE]; > + } data; > } imx_header_v2_t; > > /* The header must be aligned to 4k on MX53 for NAND boot */ > Again, I apologize for being slow to review this, but I'm hoping to use this to build a package of SPL (as a plugin) along with U-Boot and I still think it's doable. A quick proof-of-concept shows that we can load SPL directly as a plugin just by setting byte 0x28 to 1 (the plugin flag), and if we can test for serial download mode as Stefano does in this patch, we can return to the boot loader. http://lists.denx.de/pipermail/u-boot/2015-December/237555.html The subsequent patches to require plugin.S seem to get in the way of that. Regards, Eric