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

[Stefano Babic <sbabic@denx.de>]

Hi Eric, Gary,

On 17/11/2016 15:23, Eric Nelson wrote:
> 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@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.

Right.

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

Most important is to get values. How easy is then to copy&paste, well,
it is not so important.

I have applied the patch to u-boot-imx - thanks !

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
=====================================================================