On Thu, Jun 23, 2016 at 01:59:40PM +0900, Masahiro Yamada wrote: > 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.
Yamada-san, yes your patch fixes this. Thanks for spending time thinking about this issue. Will remove the .NOTPARALLEL hack in the next revision of this patch series. Regards, -- Andreas Dannenberg Texas Instruments Inc _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
