[U-Boot] [RESEND PATCH] tools: imximage: refactor header length calculations for imximage v1

[Stefano Babic]

Hi Martin,

On 02/01/2017 22:24, Martin Kaiser wrote:
> From: Martin Kaiser <martin at kaiser.cx>
> 
> We can use the same header length calculations for both imximage v1 and
> v2. This addresses TODO comments about imximage v1 in the current code.
> 

I see, absoulutely a good idea.

> With this patch applied, *header_size_ptr in imximage_set_header() will
> have the correct value for both imximage v1 and v2. This is necessary
> for people wanting to add proprietary data behind the created imximage.
> 
> Signed-off-by: Martin Kaiser <martin at kaiser.cx>
> Cc: sbabic at denx.de
> ---
> 
> I discovered the problem when I tried to use csf_ptr with imximage v1
> (as part of a private modification).
> 
> *csf_ptr = params->ep + *header_size_ptr - imximage_init_loadsize;
>          = params->ep + sbuf->st_size +
>                      imximage_ivt_offset - imximage_init_loadsize;
>          = params->ep + padded data file size + alloc_len -
>                      (imximage_init_loadsize - imximage_ivt_offset);
> 
> This works only if
> alloc_len == imximage_init_loadsize - imximage_ivt_offset,
> not if alloc_len is always 4096.
> 

I remember that this was a "reasonable" value that initially avoided
overlapping. But it was not computed.

> I rebased my tree to make sure that the patch still applies against
> current master. I'd really appreciate any feedback, this has been
> pending since September.
> 
>  tools/imximage.c |   38 ++++++++++++++++----------------------
>  1 file changed, 16 insertions(+), 22 deletions(-)
> 
> diff --git a/tools/imximage.c b/tools/imximage.c
> index 2cd8d88..0c43196 100644
> --- a/tools/imximage.c
> +++ b/tools/imximage.c
> @@ -300,8 +300,7 @@ static void set_imx_hdr_v1(struct imx_header *imxhdr, uint32_t dcd_len,
>  	/* Set magic number */
>  	fhdr_v1->app_code_barker = APP_CODE_BARKER;
>  
> -	/* TODO: check i.MX image V1 handling, for now use 'old' style */
> -	hdr_base = entry_point - 4096;
> +	hdr_base = entry_point - imximage_init_loadsize + flash_offset;
>  	fhdr_v1->app_dest_ptr = hdr_base - flash_offset;
>  	fhdr_v1->app_code_jump_vector = entry_point;
>  
> @@ -833,18 +832,19 @@ static void imximage_set_header(void *ptr, struct stat *sbuf, int ifd,
>  	/* Parse dcd configuration file */
>  	dcd_len = parse_cfg_file(imxhdr, params->imagename);
>  
> -	if (imximage_version == IMXIMAGE_V2) {
> +	if (imximage_version == IMXIMAGE_V1)
> +		header_size = sizeof(flash_header_v1_t);
> +	else {
>  		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 +
> -					header_size;
>  	}
>  
> +	if (imximage_init_loadsize < imximage_ivt_offset + header_size)
> +			imximage_init_loadsize = imximage_ivt_offset + header_size;
> +
>  	/* Set the imx header */
>  	(*set_imx_hdr)(imxhdr, dcd_len, params->ep, imximage_ivt_offset);
>  
> @@ -913,23 +913,21 @@ static int imximage_generate(struct image_tool_params *params,
>  	/* Parse dcd configuration file */
>  	parse_cfg_file(&imximage_header, params->imagename);
>  
> -	/* 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_version == IMXIMAGE_V1)
> +		header_size = sizeof(imx_header_v1_t);
> +	else {
>  		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 +
> -					header_size;
> -		alloc_len = imximage_init_loadsize - imximage_ivt_offset;
>  	}
>  
> +	if (imximage_init_loadsize < imximage_ivt_offset + header_size)
> +			imximage_init_loadsize = imximage_ivt_offset + header_size;
> +
> +	alloc_len = imximage_init_loadsize - imximage_ivt_offset;
> +
>  	if (alloc_len < header_size) {
>  		fprintf(stderr, "%s: header error\n",
>  			params->cmdname);
> @@ -959,11 +957,7 @@ static int imximage_generate(struct image_tool_params *params,
>  
>  	pad_len = ROUND(sbuf.st_size, 4096) - sbuf.st_size;
>  
> -	/* TODO: check i.MX image V1 handling, for now use 'old' style */
> -	if (imximage_version == IMXIMAGE_V1)
> -		return 0;
> -	else
> -		return pad_len;
> +	return pad_len;
>  }

IMHO it is ok and it was pending since a lot of time. I merge it.

Best regards,
Stefano Babic


-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================