2016-06-21 11:35 GMT+09:00 Andreas Dannenberg <dannenb...@ti.com>: > Hi Simon, > thanks for the continued feedback. Comments below... > > On Mon, Jun 20, 2016 at 04:40:10PM -0600, Simon Glass wrote: >> +Masahiro for the Makefile question >> >> Hi Andreas, >> >> On 17 June 2016 at 10:13, Andreas Dannenberg <dannenb...@ti.com> wrote: >> > Hi Simon, >> > many thanks for your feedback on the series... I know it takes a lot of >> > effort to digest all that stuff. We'll see how we can tackle the >> > feedback one by one and send out an updated series. >> > >> > Regarding this patch, please see below... >> > >> > On Thu, Jun 16, 2016 at 09:52:52PM -0600, Simon Glass wrote: >> >> Hi Andreas, >> >> >> >> On 15 June 2016 at 13:26, Andreas Dannenberg <dannenb...@ti.com> wrote: >> >> > From: Daniel Allred <d-all...@ti.com> >> >> > >> >> > Adds commands so that when a secure device is in use and the SPL is >> >> > built to load a FIT image (with combined u-boot binary and various >> >> > DTBs), these components that get fed into the FIT are all processed to >> >> > be signed/encrypted/etc. as per the operations performed by the >> >> > secure-binary-image script of the TI SECDEV package. >> >> > >> >> > Signed-off-by: Daniel Allred <d-all...@ti.com> >> >> > Signed-off-by: Andreas Dannenberg <dannenb...@ti.com> >> >> > --- >> >> > arch/arm/cpu/armv7/omap-common/config_secure.mk | 57 >> >> > ++++++++++++++++++++++++- >> >> > arch/arm/cpu/armv7/omap5/config.mk | 3 ++ >> >> > 2 files changed, 58 insertions(+), 2 deletions(-) >> >> >> >> Reviewed-by: Simon Glass <s...@chromium.org> >> >> >> >> But please can you add a README for this somewhere? >> >> >> >> Also, can this tool be added to U-Boot instead of being external? >> > >> > Yes we will definitely enhance the readme in the PATCH version, this >> > wasn't quite ready yet by the time we submitted the RFC. >> > >> > Also regarding the external singing/encryption tool this is only >> > available from TI under NDA after customers engage with TI just like the >> > high-secure (HS) devices themselves (you can't just buy them on >> > Digi-Key). Personally I hope that we eventually lower this barrier of >> > entry (and this has been brought up more than once) but as many things >> > in the corporate world there is a complex decision process behind it. So >> > until this strategy changes (if ever) our poor developer's hands are >> > tied. All we can do for now is trying to allow this external tool to get >> > hooked into the U-Boot build process in the easiest way possible which >> > is already done today by way of the $TI_SECURE_DEV_PKG environmental >> > variable during SPL build. >> >> OK. That's odd because it should be the keys that are secure, not the >> tool! If anything, the tool should have as many eyes on it as >> possible. > > Yes that's the ideal-world case I was also arguing we should follow... > But as indicated decision processes in the corporate world sometimes are > complex :) > >> >> > >> >> > diff --git a/arch/arm/cpu/armv7/omap-common/config_secure.mk >> >> > b/arch/arm/cpu/armv7/omap-common/config_secure.mk >> >> > index c7bb101..c4514ad 100644 >> >> > --- a/arch/arm/cpu/armv7/omap-common/config_secure.mk >> >> > +++ b/arch/arm/cpu/armv7/omap-common/config_secure.mk >> >> > @@ -12,8 +12,8 @@ cmd_mkomapsecimg = >> >> > $(TI_SECURE_DEV_PKG)/scripts/create-boot-image.sh \ >> >> > $(if $(KBUILD_VERBOSE:1=), >/dev/null) >> >> > else >> >> > cmd_mkomapsecimg = $(TI_SECURE_DEV_PKG)/scripts/create-boot-image.sh \ >> >> > - $(patsubst u-boot_HS_%,%,$(@F)) $< $@ $(CONFIG_ISW_ENTRY_ADDR) \ >> >> > - $(if $(KBUILD_VERBOSE:1=), >/dev/null) >> >> > + $(patsubst u-boot_HS_%,%,$(@F)) $< $@ $(CONFIG_ISW_ENTRY_ADDR) \ >> >> > + $(if $(KBUILD_VERBOSE:1=), >/dev/null) >> >> > endif >> >> > else >> >> > cmd_mkomapsecimg = echo "WARNING:" \ >> >> > @@ -25,6 +25,26 @@ cmd_mkomapsecimg = echo "WARNING: TI_SECURE_DEV_PKG >> >> > environment" \ >> >> > "variable must be defined for TI secure devices. $@ was NOT >> >> > created!" >> >> > endif >> >> > >> >> > +ifdef CONFIG_SPL_LOAD_FIT >> >> > +quiet_cmd_omapsecureimg = SECURE $@ >> >> > +ifneq ($(TI_SECURE_DEV_PKG),) >> >> > +ifneq ($(wildcard >> >> > $(TI_SECURE_DEV_PKG)/scripts/secure-binary-image.sh),) >> >> > +cmd_omapsecureimg = >> >> > $(TI_SECURE_DEV_PKG)/scripts/secure-binary-image.sh \ >> >> > + $< $@ \ >> >> > + $(if $(KBUILD_VERBOSE:1=), >/dev/null) >> >> > +else >> >> > +cmd_omapsecureimg = echo "WARNING:" \ >> >> > + "$(TI_SECURE_DEV_PKG)/scripts/secure-binary-image.sh not >> >> > found." \ >> >> > + "$@ was NOT created!"; cp $< $@ >> >> > +endif >> >> > +else >> >> > +cmd_omapsecureimg = echo "WARNING: TI_SECURE_DEV_PKG environment" \ >> >> > + "variable must be defined for TI secure devices." \ >> >> > + "$@ was NOT created!"; cp $< $@ >> >> > +endif >> >> > +endif >> >> > + >> >> > + >> >> > # Standard X-LOADER target (QPSI, NOR flash) >> >> > u-boot-spl_HS_X-LOADER: $(obj)/u-boot-spl.bin >> >> > $(call if_changed,mkomapsecimg) >> >> > @@ -64,3 +84,36 @@ u-boot-spl_HS_SPI_X-LOADER: $(obj)/u-boot-spl.bin >> >> > # the mkomapsecimg command looks for a u-boot-HS_* prefix >> >> > u-boot_HS_XIP_X-LOADER: $(obj)/u-boot.bin >> >> > $(call if_changed,mkomapsecimg) >> >> > + >> >> > +# For supporting the SPL loading and interpreting >> >> > +# of FIT images whose components are pre-processed >> >> > +# before being integrated into the FIT image in order >> >> > +# to secure them in some way >> >> > +ifdef CONFIG_SPL_LOAD_FIT >> >> > + >> >> > +MKIMAGEFLAGS_u-boot_HS.img = -f auto -A $(ARCH) -T firmware -C none -O >> >> > u-boot \ >> >> > + -a $(CONFIG_SYS_TEXT_BASE) -e $(CONFIG_SYS_UBOOT_START) \ >> >> > + -n "U-Boot $(UBOOTRELEASE) for $(BOARD) board" -E \ >> >> > + $(patsubst %,-b arch/$(ARCH)/dts/%.dtb,$(subst >> >> > ",,$(CONFIG_OF_LIST))) >> >> > + >> >> > +OF_LIST_TARGETS = $(patsubst %,arch/$(ARCH)/dts/%.dtb,$(subst >> >> > ",,$(CONFIG_OF_LIST))) >> >> > +$(OF_LIST_TARGETS): dtbs >> >> > + >> >> > +%_HS.dtb: %.dtb >> >> > + $(call if_changed,omapsecureimg) >> >> > + $(Q)if [ -f $@ ]; then \ >> >> > + cp -f $@ $<; \ >> >> > + fi >> >> > + >> >> > +u-boot-nodtb_HS.bin: u-boot-nodtb.bin >> >> > + $(call if_changed,omapsecureimg) >> >> > + >> >> > +u-boot_HS.img: u-boot-nodtb_HS.bin u-boot.img $(patsubst >> >> > %.dtb,%_HS.dtb,$(OF_LIST_TARGETS)) >> >> > + $(call if_changed,mkimage) >> >> > + $(Q)if [ -f $@ ]; then \ >> >> > + cp -f $@ u-boot.img; \ >> >> > + fi >> >> > + >> >> > +.NOTPARALLEL: dtbs >> >> >> >> Why is that needed? >> > >> > Good catch! This issue actually caused me a fair amount of grief when >> > running into it. During development we found the build process works >> > with make -j1 but would fail for parallel builds (like make -j8) with >> > the following error.... >> > >> > ----------------------->8--------------------------- >> > $ make mrproper >> > $ make dra7xx_hs_evm_defconfig >> > $ make -j8 >> > .... >> > LD u-boot >> > OBJCOPY u-boot-nodtb.bin >> > OBJCOPY u-boot.srec >> > SYM u-boot.sym >> > DTC arch/arm/dts/dra72-evm.dtb >> > DTC arch/arm/dts/dra7-evm.dtb >> > DTC arch/arm/dts/dra72-evm.dtb >> > DTC arch/arm/dts/dra7-evm.dtb >> > Error: arch/arm/dts/dra7-evm.dts:492.15-16 syntax error >> > FATAL ERROR: Unable to parse input tree >> > make[2]: *** [arch/arm/dts/dra7-evm.dtb] Error 1 >> > make[2]: *** Waiting for unfinished jobs.... >> > Error: arch/arm/dts/dra72-evm.dts:149.26-150.2 syntax error >> > FATAL ERROR: Unable to parse input tree >> > make[2]: *** [arch/arm/dts/dra72-evm.dtb] Error 1 >> > make[1]: *** [arch-dtbs] Error 2 >> > make: *** [dtbs] Error 2 >> > make: *** Waiting for unfinished jobs.... >> > SHIPPED dts/dt.dtb >> > ----------------------->8--------------------------- >> >> Probably you are missing a dependency somewhere - but the failure to >> parse is odd. What is it missing? > > This failure in parsing is not the real issue. What I found (at least > this is how interpreted it) two concurrent DTC builds on the same file > are stomping over each other in terms of temp files. Besides this I'd > also have a very hard time explaining this error in any other way since > the dts file itself is perfectly fine. > >> > >> > However with the .NOTPARALLEL: dtbs line included it works. Also when >> > doing make a second time it works as well (clearly that's not a >> > solution:) >> > >> > On my quest trying to digest/root cause this I concluded that this may >> > be caused by the DTB files somehow getting compiled twice (see snippet). >> > This in combination with the fact that we post-process files like >> > dta7-evm.dtb (signing them) and creating new DTB files with the same >> > name (like dra7-evm.dtb) replacing the old ones causes issues during the >> > DTB compile step. >> >> Yes I think it is better to create new files, since otherwise the >> build system can't determine which version of the file it should >> target...it assumes a single version as I understand it. > > Yes this is how I had it initially. But as indicated the issue was that > the mkimage step that packages this newly-named DTB files would put them > into the u-boot.img FIT blob also with this odd new name. And then the > board match would not work anymore... And that was when I decided to > stop hacking stuff to get it to work. > >> > >> > Now I'm not a make guru but I tried to clean up the dependency chain as >> > good as I can but could not get it to the point where each DTB file is >> > only build once which most likely would allow us getting rid of >> > NOPARALLEL. >> > >> > On a related note we wanted to keep the same names for the DTB files >> > (rather than turning dra7-evm.dtb into dra7-evm-sec.dtb for example, as >> > I had it initially) since it's important as they get bundled into the >> > u-boot.img FIT blob so that they can be later matched by the SPL to a >> > board. But it appears like that trying to keep the same names despite >> > the additional signing step seems to complicate things from a make >> > perspective. >> >> Yes - best to create a new u-boot-sec.img I think. > > U-Boot itself is not really the issue here - in fact we are already > creating an u-boot_HS.img output artifact. The issue is with the DTBs... > >> > If you or anybody else has any smart suggestion how to address this in >> > a better way I'd love to hear about it. >> >> Masahiro is the expert, but my ideas are above. > > Awesome. Now that the kids are asleep I'm actually in the process > preparing/testing an optimized patch series based on your, Lokesh's, and > Tom's feedback. I should be able to send it out soon hopefully tonight. > It won't have 100% of feedback implemented (Make stuff is the same) but > it'll be a better base for continued discussion. > > Best Regards, >
Hi. Could you try this? http://patchwork.ozlabs.org/patch/639478/ I think you can remove the ".NOTPARALLEL" line. -- Best Regards Masahiro Yamada _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
