From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Nelson Date: Thu, 17 Nov 2016 07:23:59 -0700 Subject: [U-Boot] [PATCH] tools: imximage: display DCD block offset, length In-Reply-To: <20161117121645.nrxecqtxznuacwg6@t450s.lan> References: <1479341621-30021-1-git-send-email-eric@nelint.com> <20161117121645.nrxecqtxznuacwg6@t450s.lan> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 > > 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').