[U-Boot] [PATCH] tools: imximage: display DCD block offset, length

[Eric Nelson]

Hi Gary,

On 11/17/2016 05:16 AM, Gary Bisson wrote:
> Hi Eric, All,
> 
> On Wed, Nov 16, 2016 at 05:13:41PM -0700, Eric Nelson wrote:
>> These values can be used to sign a U-Boot image for use when
>> loading an image through the Serial Download Protocol (SDP).
>>
>> Note that the address of 0x910000 is usable with the stock
>> configuration of imx_usb_loader on i.MX6 and i.MX7 SOCs:
>>
>> https://github.com/boundarydevices/imx_usb_loader/blob/master/mx6_usb_work.conf#L3
>>
>> Refer to the section on imx_usb_loader in this post for more
>> details:
>>
>> https://boundarydevices.com/high-assurance-boot-hab-dummies/
>>
>> Signed-off-by: Eric Nelson <eric at nelint.com>
> 
> Thanks, indeed such patch would ease the life of anybody that needs to
> deal with HAB when creating the CSF files.
> 
>> ---
>>  tools/imximage.c | 10 +++++++++-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/imximage.c b/tools/imximage.c
>> index c9e42ec..2cd8d88 100644
>> --- a/tools/imximage.c
>> +++ b/tools/imximage.c
>> @@ -281,7 +281,6 @@ static void set_dcd_rst_v2(struct imx_header *imxhdr, uint32_t dcd_len,
>>  			d = (struct dcd_v2_cmd *)(((char *)d) + len);
>>  
>>  		len = (char *)d - (char *)&dcd_v2->header;
>> -
> 
> Is this part of the patch intended?
> 

Nope.

>>  		dcd_v2->header.tag = DCD_HEADER_TAG;
>>  		dcd_v2->header.length = cpu_to_be16(len);
>>  		dcd_v2->header.version = DCD_VERSION;
>> @@ -501,10 +500,19 @@ static void print_hdr_v2(struct imx_header *imx_hdr)
>>  		printf("Entry Point:  %08x\n", (uint32_t)fhdr_v2->entry);
>>  		if (fhdr_v2->csf && (imximage_ivt_offset != UNDEFINED) &&
>>  		    (imximage_csf_size != UNDEFINED)) {
>> +			uint16_t dcdlen;
>> +			int offs;
>> +
>> +			dcdlen = hdr_v2->data.dcd_table.header.length;
>> +			offs = (char *)&hdr_v2->data.dcd_table
>> +				- (char *)hdr_v2;
>> +
>>  			printf("HAB Blocks:   %08x %08x %08x\n",
>>  			       (uint32_t)fhdr_v2->self, 0,
> 
> This isn't part of the patch, but why is self cast into a uint32_t
> although it's already a uint32_t?
> 

Unrelated, but good catch!

>>  			       hdr_v2->boot_data.size - imximage_ivt_offset -
>>  			       imximage_csf_size);
>> +			printf("DCD Blocks:   00910000 %08x %08x\n",
>> +			       offs, be16_to_cpu(dcdlen));
>>  		}
> 
> Not sure if "DCD Blocks" is the best naming, cause it really just
> applies to SDP protocol.
> 
> This got me thinking and I think the printf above should also show the
> Blocks for encryption which is also missing right now.
> 
> What about something like the snippet below?
> 
> 		if (fhdr_v2->csf && (imximage_ivt_offset != UNDEFINED) &&
> 		    (imximage_csf_size != UNDEFINED)) {
> 			uint16_t dcdlen;
> 			uint32_t dcdoff;
> 			uint32_t entryoff;
> 
> 			dcdlen = hdr_v2->data.dcd_table.header.length;
> 			dcdoff = (char *)&hdr_v2->data.dcd_table
> 				- (char *)hdr_v2;
> 			entryoff = fhdr_v2->entry - fhdr_v2->self;
> 
> 			printf("[HAB][Signature]\n");
> 			printf("Blocks:   %08x %08x %08x\n",
> 			       (uint32_t)fhdr_v2->self, 0,

I think somebody just pointed out that self is already a uint32_t,
so why the cast?

> 			       hdr_v2->boot_data.size - imximage_ivt_offset -
> 			       imximage_csf_size);
> 			printf("[HAB][Encryption]\n");
> 			printf("Blocks:   %08x %08x %08x\n",
> 			       fhdr_v2->self, 0, dcdoff + be16_to_cpu(dcdlen));
> 			printf("Blocks:   %08x %08x %08x\n",
> 			       fhdr_v2->entry, entryoff,
> 			       hdr_v2->boot_data.size - imximage_ivt_offset -
> 			       imximage_csf_size - entryoff);
> 			printf("[HAB][SDP]\n");
> 			printf("Blocks:   00910000 %08x %08x\n",
> 			       dcdoff, be16_to_cpu(dcdlen));
> 		}

I like the more specific tags, but wonder if some minor edits wouldn't
make this more useful.

In particular, adding 0x before %08x would allow easier cut and paste
into signing scripts in a manual process.

I looked briefly at the output of 'mkimage -l' and found that it doesn't
reach this code block because imximage_ivt/csf_size variables are
set during the parsing of a .cfg file (they're UNDEFINED with
'mkimage -l').