Secure Boot by Authenticating/Decrypting SPL FIT blobs

From: Daniel Allred <[email protected]>

The next stage boot loader image and the selected FDT can be post-processed by board/platform/device-specific code, which can include modifying the size and altering the starting source address before copying these binary blobs to their final desitination. This might be desired to do things like strip headers or footers attached to the images before they were packeaged into the FIT, or to perform operations such as decryption or authentication.

Signed-off-by Daniel Allred <[email protected]>

Signed-off-by Andreas Dannenberg <[email protected]>

---
 common/spl/spl_fit.c | 21 ++++++++++++++++-----
 include/image.h      | 15 +++++++++++++++
 2 files changed, 31 insertions(+), 5 deletions(-)

common/spl/spl_fit.c +0 -0

index 9874708..ecbcb97 100644

common/spl/spl_fit.c +13 -5

+
 int spl_load_simple_fit(struct spl_load_info *info, ulong sector, void *fit)
 {
        int sectors;

@@ -132,7 +136,7 @@ int spl_load_simple_fit(struct spl_load_info *info, ulong sector, void *fit)
        int data_offset, data_size;
        int base_offset, align_len = ARCH_DMA_MINALIGN - 1;
        int src_sector;
-       void *dst;
+       void *dst, *src;
 
        /*
         * Figure out where the external images start. This is the base for the

@@ -206,8 +210,11 @@ int spl_load_simple_fit(struct spl_load_info *info, ulong sector, void *fit)
                return -EIO;
        debug("image: dst=%p, data_offset=%x, size=%x\n", dst, data_offset,
              data_size);
-       memcpy(dst, dst + get_aligned_image_overhead(info, data_offset),
-              data_size);
+       src = dst + get_aligned_image_overhead(info, data_offset);
+
+       board_fit_image_post_process((void **)&src, (size_t *)&data_size);
+
+       memcpy(dst, src, data_size);
 
        /* Figure out which device tree the board wants to use */
        fdt_len = spl_fit_select_fdt(fit, images, &fdt_offset);

@@ -236,8 +243,12 @@ int spl_load_simple_fit(struct spl_load_info *info, ulong sector, void *fit)
         */
        debug("fdt: dst=%p, data_offset=%x, size=%x\n", dst, fdt_offset,
              fdt_len);
-       memcpy(load_ptr + data_size,
-              dst + get_aligned_image_overhead(info, fdt_offset), fdt_len);
+       src = dst + get_aligned_image_overhead(info, fdt_offset);
+       dst = load_ptr + data_size;
+
+       board_fit_image_post_process((void **)&src, (size_t *)&fdt_len);
+
+       memcpy(dst, src, fdt_len);
 
        return 0;
 }

include/image.h +15 -1

index a8f6bd1..9536874 100644
@@ -1172,4 +1172,19 @@ ulong android_image_get_kload(const struct andr_img_hdr *hdr);
  */
 int board_fit_config_name_match(const char *name);
 
+/**
+ * board_fit_image_post_process() - Do any post-process on FIT binary data
+ *
+ * This is used to do any sort of image manipulation, verification, decryption
+ * etc. in a platform or board specific way. Obviously, anything done here would
+ * need to be comprehended in how the images were prepared before being injected
+ * into the FIT creation (i.e. the binary blobs would have been pre-processed
+ * before being added to the FIT image).
+ *
+ * @image: pointer to the image start pointer
+ * @size: pointer to the image size
+ * @return no return value (failure should be handled internally)
+ */
+void board_fit_image_post_process(void **p_image, size_t *p_size);
+
 #endif /* __IMAGE_H__ */
--

2.6.4

From: Daniel Allred <[email protected]>

Adds missing flush_dcache_range and invalidate_dcache_range dummy (empty) placeholder functions to the #else portion of the #ifndef CONFIG_SYS_DCACHE_OFF, where full implementations of these functions are defined.

Signed-off-by Daniel Allred <[email protected]>

Signed-off-by Andreas Dannenberg <[email protected]>

---
 arch/arm/cpu/armv7/cache_v7.c | 8 ++++++++
 1 file changed, 8 insertions(+)

From: Daniel Allred <[email protected]>

Adds missing flush_dcache_range and invalidate_dcache_range dummy (empty) placeholder functions to the #else portion of the #ifndef CONFIG_SYS_DCACHE_OFF, where full implementations of these functions are defined.

Signed-off-by Daniel Allred <[email protected]>

Signed-off-by Andreas Dannenberg <[email protected]>

From: Daniel Allred <[email protected]>

Adds an interface for calling secure ROM APIs across a range of OMAP and OMAP compatible devices.

Signed-off-by Daniel Allred <[email protected]>

Signed-off-by Andreas Dannenberg <[email protected]>

---
 arch/arm/cpu/armv7/omap-common/lowlevel_init.S | 47 ++++++++++++++++++++++----
 arch/arm/include/asm/omap_common.h             |  2 ++
 2 files changed, 42 insertions(+), 7 deletions(-)

arch/arm/cpu/armv7/omap-common/lowlevel_init.S +40 -7

index 5283135..2a710ce 100644
@@ -16,6 +16,8 @@
 #include <asm/arch/spl.h>
 #include <linux/linkage.h>
 
+.arch_extension sec
+
 #ifdef CONFIG_SPL
 ENTRY(save_boot_params)
 

@@ -26,14 +28,45 @@ ENDPROC(save_boot_params)
 #endif
 
 ENTRY(omap_smc1)
-       PUSH    {r4-r12, lr}    @ save registers - ROM code may pollute
+       push    {r4-r12, lr}    @ save registers - ROM code may pollute
                                @ our registers
-       MOV     r12, r0         @ Service
-       MOV     r0, r1          @ Argument
-       DSB
-       DMB
-       .word   0xe1600070      @ SMC #0 - hand assembled for GCC versions
+       mov     r12, r0         @ Service
+       mov     r0, r1          @ Argument
+       dsb
+       dmb
+       smc     0               @ SMC #0 to enter monitor mode
                                @ call ROM Code API for the service requested
 
-       POP     {r4-r12, pc}
+       pop     {r4-r12, pc}
 ENDPROC(omap_smc1)
+
+ENTRY(omap_smc_sec)
+       push    {r4-r12, lr}    @ save registers - ROM code may pollute
+
+       mov     r6, #0xFF       @ Indicate new Task call
+
+       mov     r12, #0x00      @ Secure Service ID in R12
+
+       dsb
+       dmb
+
+       smc     0               @ SMC #0 to enter monitor mode
+       b       omap_smc_sec_end /* exit at end of the service execution */
+       nop
+
+       /* In case of IRQ happening in Secure, then ARM will branch here.
+        * At that moment, IRQ will be pending and ARM will jump to Non Secure
+        * IRQ handler
+         */
+       mov     r12, #0xFE
+
+       dsb
+       dmb
+
+       smc     0               @ SMC #0 to enter monitor mode
+
+omap_smc_sec_end:
+       pop     {r4-r12, lr}
+       bx      lr
+
+ENDPROC(omap_smc_sec)

arch/arm/include/asm/omap_common.h +0 -0

index 07f3848..5943e6f 100644

arch/arm/include/asm/omap_common.h +1 -1

+
 void enable_edma3_clocks(void);
 void disable_edma3_clocks(void);
 
--

2.6.4

From: Daniel Allred <[email protected]>

Adds a generic C-callable API for making secure ROM calls on OMAP and OMAP-compatible devices. This API provides the important function of flushing the ROM call arguments to memory from the cache, so that the secure world will have a coherent view of those arguments. Then is simply calls the omap_smc_sec routine.

Signed-off-by Daniel Allred <[email protected]>

Signed-off-by Andreas Dannenberg <[email protected]>

---
 arch/arm/cpu/armv7/omap-common/Makefile     |  4 +++
 arch/arm/cpu/armv7/omap-common/sec_bridge.c | 47 +++++++++++++++++++++++++++++
 arch/arm/include/asm/omap_common.h          |  4 +++
 3 files changed, 55 insertions(+)
 create mode 100644 arch/arm/cpu/armv7/omap-common/sec_bridge.c

arch/arm/cpu/armv7/omap-common/Makefile +4 -0

index 87a7ac0..4fc3926 100644
@@ -28,6 +28,10 @@ obj-y        += pipe3-phy.o
 obj-$(CONFIG_SCSI_AHCI_PLAT) += sata.o
 endif
 
+ifneq ($(CONFIG_TI_SECURE_DEVICE),)
+obj-y   += sec_bridge.o
+endif
+
 ifeq ($(CONFIG_SYS_DCACHE_OFF),)
 obj-y  += omap-cache.o
 endif

arch/arm/cpu/armv7/omap-common/sec_bridge.c new file +47 -0

new file mode 100644
index 0000000..4eaba8e
@@ -0,0 +1,47 @@
+/*
+ *
+ * Common bridge function to make OMAP secure ROM calls
+ *
+ * (C) Copyright 2016
+ * Texas Instruments, <www.ti.com>
+ *
+ * Daniel Allred    <[email protected]>
+ *
+ * SPDX-License-Identifier: GPL-2.0+
+ */
+
+#include <common.h>
+#include <stdarg.h>
+
+#include <asm/arch/sys_proto.h>
+#include <asm/omap_common.h>
+
+static uint32_t secure_rom_call_args[5] __aligned(ARCH_DMA_MINALIGN);
+
+u32 secure_rom_call(u32 service, u32 proc_id, u32 flag, ...)
+{
+       int i;
+       u32 num_args;
+       va_list ap;
+
+       va_start(ap, flag);
+
+       num_args = va_arg(ap, u32);
+
+       /* Copy args to aligned args structure */
+       for (i = 0; i < num_args; i++)
+               secure_rom_call_args[i + 1] = va_arg(ap, u32);
+
+       secure_rom_call_args[0] = num_args;
+
+       va_end(ap);
+
+       /* if data cache is enabled, flush the aligned args structure */
+#ifndef CONFIG_SYS_DCACHE_OFF
+       flush_dcache_range(
+               (unsigned int)&secure_rom_call_args[0],
+               (unsigned int)&secure_rom_call_args[0] +
+               roundup(sizeof(secure_rom_call_args), ARCH_DMA_MINALIGN));
+#endif
+       return omap_smc_sec(service, proc_id, flag, secure_rom_call_args);
+}

arch/arm/include/asm/omap_common.h +0 -0

index 5943e6f..cb02c88 100644

arch/arm/include/asm/omap_common.h +2 -1

+#endif
+
 void enable_edma3_clocks(void);
 void disable_edma3_clocks(void);
 
--

2.6.4

From: Daniel Allred <[email protected]>

Adds a generic C-callable API for making secure ROM calls on OMAP and OMAP-compatible devices. This API provides the important function of flushing the ROM call arguments to memory from the cache, so that the secure world will have a coherent view of those arguments. Then is simply calls the omap_smc_sec routine.

Signed-off-by Daniel Allred <[email protected]>

Signed-off-by Andreas Dannenberg <[email protected]>

---
 arch/arm/cpu/armv7/omap-common/Makefile     |  4 +++
 arch/arm/cpu/armv7/omap-common/sec_bridge.c | 47 +++++++++++++++++++++++++++++
 arch/arm/include/asm/omap_common.h          |  4 +++
 3 files changed, 55 insertions(+)
 create mode 100644 arch/arm/cpu/armv7/omap-common/sec_bridge.c

arch/arm/cpu/armv7/omap-common/Makefile +0 -0

index 87a7ac0..4fc3926 100644

arch/arm/cpu/armv7/omap-common/Makefile +1 -0

+
 ifeq ($(CONFIG_SYS_DCACHE_OFF),)
 obj-y	+= omap-cache.o
 endif

arch/arm/cpu/armv7/omap-common/sec_bridge.c new file +20 -0

new file mode 100644
index 0000000..4eaba8e
@@ -0,0 +1,47 @@
+/*
+ *
+ * Common bridge function to make OMAP secure ROM calls
+ *
+ * (C) Copyright 2016
+ * Texas Instruments, <www.ti.com>
+ *
+ * Daniel Allred    <[email protected]>
+ *
+ * SPDX-License-Identifier: GPL-2.0+
+ */
+
+#include <common.h>
+#include <stdarg.h>
+
+#include <asm/arch/sys_proto.h>
+#include <asm/omap_common.h>
+
+static uint32_t secure_rom_call_args[5] __aligned(ARCH_DMA_MINALIGN);
+

arch/arm/cpu/armv7/omap-common/sec_bridge.c new file +5 -0

+	/* Copy args to aligned args structure */
+	for (i = 0; i < num_args; i++)
+		secure_rom_call_args[i + 1] = va_arg(ap, u32);
+
+	secure_rom_call_args[0] = num_args;

arch/arm/cpu/armv7/omap-common/sec_bridge.c new file +2 -0

+	return omap_smc_sec(service, proc_id, flag, secure_rom_call_args);
+}

arch/arm/include/asm/omap_common.h +4 -0

index 5943e6f..cb02c88 100644
@@ -629,6 +629,10 @@ void omap_smc1(u32 service, u32 val);
 
 u32 omap_smc_sec(u32 service, u32 proc_id, u32 flag, u32 *params);
 
+#ifdef CONFIG_TI_SECURE_DEVICE
+u32 secure_rom_call(u32 service, u32 proc_id, u32 flag, ...);
+#endif
+
 void enable_edma3_clocks(void);
 void disable_edma3_clocks(void);
 

From: Daniel Allred <[email protected]>

Adds an API that verifies a signature attached to an image (binary blob). This API is basically a entry to a secure ROM service provided by the device and accessed via an SMC call, using a particular calling convention.

Signed-off-by Daniel Allred <[email protected]>

Signed-off-by Andreas Dannenberg <[email protected]>

---
 arch/arm/cpu/armv7/omap5/Makefile           |  1 +
 arch/arm/cpu/armv7/omap5/sec_fxns.c         | 70 +++++++++++++++++++++++++++++
 arch/arm/include/asm/arch-omap5/sys_proto.h |  4 ++
 3 files changed, 75 insertions(+)
 create mode 100644 arch/arm/cpu/armv7/omap5/sec_fxns.c

arch/arm/cpu/armv7/omap5/Makefile +1 -0

index 3caba86..d373bf4 100644
@@ -14,3 +14,4 @@ obj-y += hw_data.o
 obj-y  += abb.o
 obj-y  += fdt.o
 obj-$(CONFIG_IODELAY_RECALIBRATION) += dra7xx_iodelay.o
+obj-$(CONFIG_TI_SECURE_DEVICE) += sec_fxns.o

arch/arm/cpu/armv7/omap5/sec_fxns.c new file +70 -0

new file mode 100644
index 0000000..766333a
@@ -0,0 +1,70 @@
+/*
+ *
+ * Common security functions that rely on secure ROM services
+ *
+ * (C) Copyright 2016
+ * Texas Instruments, <www.ti.com>
+ *
+ * Daniel Allred    <[email protected]>
+ *
+ * SPDX-License-Identifier: GPL-2.0+
+ */
+
+#include <common.h>
+#include <asm/arch/sys_proto.h>
+#include <asm/omap_common.h>
+
+#define SIGNATURE_LENGTH                               (0x118)
+
+/* API Index for OMAP5, DRA7xx */
+#define API_HAL_KM_VERIFYCERTIFICATESIGNATURE_INDEX    (0x0000000E)
+
+int secure_boot_verify_image(void **image, size_t *size)
+{
+       int result = 1;
+       u32 cert_addr, sig_addr;
+       size_t cert_size;
+
+#ifndef CONFIG_SYS_DCACHE_OFF
+       /* Perform cache writeback on input buffer */
+       flush_dcache_range(
+               (u32)*image,
+               (u32)*image + roundup(*size, ARCH_DMA_MINALIGN));
+#endif
+       cert_addr = (uint32_t)*image;
+       *size -= SIGNATURE_LENGTH;   /* Subtract out the signature size */
+       cert_size = *size;
+       sig_addr = cert_addr + cert_size;
+
+       /* Check if image load address is 32-bit aligned */
+       if (0 != (0x3 & cert_addr)) {
+               puts("Image is not 4-byte aligned.\n");
+               result = 1;
+               goto auth_exit;
+       }
+
+       /* Image size also should be multiple of 4 */
+       if (0 != (0x3 & cert_size)) {
+               puts("Image size is not 4-byte aligned.\n");
+               result = 1;
+               goto auth_exit;
+       }
+
+       /* Call ROM HAL API to verify certificate signature */
+       debug("%s: load_addr = %x, size = %x, sig_addr = %x\n", __func__,
+             cert_addr, cert_size, sig_addr);
+
+       result = secure_rom_call(
+               API_HAL_KM_VERIFYCERTIFICATESIGNATURE_INDEX, 0, 0,
+               4, cert_addr, cert_size, sig_addr, 0xFFFFFFFF);
+auth_exit:
+       if (result != 0) {
+               puts("Authentication failed!\n");
+               printf("Return Value = %08X\n", result);
+               hang();
+       }
+
+       printf("Authentication passed: %s\n", (char *)sig_addr);
+
+       return result;
+}

arch/arm/include/asm/arch-omap5/sys_proto.h +0 -0

index ab0e7fa..b175124 100644

arch/arm/include/asm/arch-omap5/sys_proto.h +2 -1

+#endif
+
 #endif
--

2.6.4

Quoted 7 lines · Andreas Dannenberg

From: Daniel Allred <[email protected]>

Adds an API that verifies a signature attached to an image (binary blob). This API is basically a entry to a secure ROM service provided by the device and accessed via an SMC call, using a particular calling convention.

Quoted 8 lines

Signed-off-by Daniel Allred <[email protected]>

Signed-off-by Andreas Dannenberg <[email protected]>

---
 arch/arm/cpu/armv7/omap5/Makefile           |  1 +
 arch/arm/cpu/armv7/omap5/sec_fxns.c         | 70 +++++++++++++++++++++++++++++
 arch/arm/include/asm/arch-omap5/sys_proto.h |  4 ++
 3 files changed, 75 insertions(+)
 create mode 100644 arch/arm/cpu/armv7/omap5/sec_fxns.c

Reviewed-by Simon Glass <[email protected]>

Please see below.

Quoted 94 lines

arch/arm/cpu/armv7/omap5/Makefile +1 -0

index 3caba86..d373bf4 100644
@@ -14,3 +14,4 @@ obj-y += hw_data.o
 obj-y  += abb.o
 obj-y  += fdt.o
 obj-$(CONFIG_IODELAY_RECALIBRATION) += dra7xx_iodelay.o
+obj-$(CONFIG_TI_SECURE_DEVICE) += sec_fxns.o

arch/arm/cpu/armv7/omap5/sec_fxns.c new file +70 -0

new file mode 100644
index 0000000..766333a
@@ -0,0 +1,70 @@
+/*
+ *
+ * Common security functions that rely on secure ROM services
+ *
+ * (C) Copyright 2016
+ * Texas Instruments, <www.ti.com>
+ *
+ * Daniel Allred    <[email protected]>
+ *
+ * SPDX-License-Identifier: GPL-2.0+
+ */
+
+#include <common.h>
+#include <asm/arch/sys_proto.h>
+#include <asm/omap_common.h>
+
+#define SIGNATURE_LENGTH                               (0x118)
+
+/* API Index for OMAP5, DRA7xx */
+#define API_HAL_KM_VERIFYCERTIFICATESIGNATURE_INDEX    (0x0000000E)
+
+int secure_boot_verify_image(void **image, size_t *size)
+{
+       int result = 1;
+       u32 cert_addr, sig_addr;
+       size_t cert_size;
+
+#ifndef CONFIG_SYS_DCACHE_OFF
+       /* Perform cache writeback on input buffer */
+       flush_dcache_range(
+               (u32)*image,
+               (u32)*image + roundup(*size, ARCH_DMA_MINALIGN));
+#endif
+       cert_addr = (uint32_t)*image;
+       *size -= SIGNATURE_LENGTH;   /* Subtract out the signature size */
+       cert_size = *size;
+       sig_addr = cert_addr + cert_size;
+
+       /* Check if image load address is 32-bit aligned */
+       if (0 != (0x3 & cert_addr)) {
+               puts("Image is not 4-byte aligned.\n");
+               result = 1;
+               goto auth_exit;
+       }
+
+       /* Image size also should be multiple of 4 */
+       if (0 != (0x3 & cert_size)) {
+               puts("Image size is not 4-byte aligned.\n");
+               result = 1;
+               goto auth_exit;
+       }
+
+       /* Call ROM HAL API to verify certificate signature */
+       debug("%s: load_addr = %x, size = %x, sig_addr = %x\n", __func__,
+             cert_addr, cert_size, sig_addr);
+
+       result = secure_rom_call(
+               API_HAL_KM_VERIFYCERTIFICATESIGNATURE_INDEX, 0, 0,
+               4, cert_addr, cert_size, sig_addr, 0xFFFFFFFF);
+auth_exit:
+       if (result != 0) {
+               puts("Authentication failed!\n");
+               printf("Return Value = %08X\n", result);
+               hang();
+       }
+
+       printf("Authentication passed: %s\n", (char *)sig_addr);
+
+       return result;
+}

arch/arm/include/asm/arch-omap5/sys_proto.h +0 -0

index ab0e7fa..b175124 100644
@@ -84,4 +84,8 @@ static inline u32 usec_to_32k(u32 usec)
 #define OMAP5_SERVICE_L2ACTLR_SET    0x104
 #define OMAP5_SERVICE_ACR_SET        0x107
 

arch/arm/include/asm/arch-omap5/sys_proto.h static inline u32 usec_to_32k(u32 usec) · line 88

Quoted 2 lines

arch/arm/include/asm/arch-omap5/sys_proto.h +2 -0

+#ifdef CONFIG_TI_SECURE_DEVICE
+int secure_boot_verify_image(void **p_image, size_t *p_size);

Function comment please.

From: Daniel Allred <[email protected]>

Adds a board specific FIT image post processing function for when CONFIG_SECURE_BOOT is defined. Also update the omap common config header to enable CONFIG_SECURE_BOOT always for secure TI devices (CONFIG_TI_SECURE_DEVICE is defined).

Signed-off-by Daniel Allred <[email protected]>

Signed-off-by Andreas Dannenberg <[email protected]>

---
 board/ti/am57xx/board.c           | 7 +++++++
 board/ti/dra7xx/evm.c             | 7 +++++++
 include/configs/ti_omap5_common.h | 4 ++++
 3 files changed, 18 insertions(+)

From: Daniel Allred <[email protected]>

Adds a board specific FIT image post processing function for when CONFIG_SECURE_BOOT is defined. Also update the omap common config header to enable CONFIG_SECURE_BOOT always for secure TI devices (CONFIG_TI_SECURE_DEVICE is defined).

Signed-off-by Daniel Allred <[email protected]>

Signed-off-by Andreas Dannenberg <[email protected]>

---
 board/ti/am57xx/board.c           | 7 +++++++
 board/ti/dra7xx/evm.c             | 7 +++++++
 include/configs/ti_omap5_common.h | 4 ++++
 3 files changed, 18 insertions(+)

board/ti/am57xx/board.c +7 -0

index 08cf14d..a9635c2 100644
@@ -750,3 +750,10 @@ int board_fit_config_name_match(const char *name)
 		return -1;
 }
 #endif
+
+#ifdef CONFIG_SECURE_BOOT
+void board_fit_image_post_process(void **p_image, size_t *p_size)
+{
+	secure_boot_verify_image(p_image, p_size);
+}
+#endif

board/ti/dra7xx/evm.c +7 -0

index 3fbbc9b..03eefb6 100644
@@ -739,3 +739,10 @@ int board_fit_config_name_match(const char *name)
 		return -1;
 }
 #endif
+
+#ifdef CONFIG_SECURE_BOOT
+void board_fit_image_post_process(void **p_image, size_t *p_size)
+{
+	secure_boot_verify_image(p_image, p_size);
+}
+#endif

include/configs/ti_omap5_common.h +0 -0

index 2e4c8e9..9db6da2 100644

include/configs/ti_omap5_common.h +1 -0

+
 /*
  * For memory booting on HS parts, the first 4KB of the internal RAM is
  * reserved for secure world use and the flash loader image is

From: Madan Srinivas <[email protected]>

Adds an API that verifies a signature attached to an image (binary blob). This API is basically a entry to a secure ROM service provided by the device and accessed via an SMC call, using a particular calling convention. This API is common across AM3x HS and AM4x HS devices.

Signed-off-by Madan Srinivas <[email protected]>

Signed-off-by Andreas Dannenberg <[email protected]>

---
 arch/arm/cpu/armv7/am33xx/Makefile           |  2 +
 arch/arm/cpu/armv7/am33xx/sec_fxns.c         | 90 ++++++++++++++++++++++++++++
 arch/arm/include/asm/arch-am33xx/sys_proto.h |  6 +-
 3 files changed, 97 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm/cpu/armv7/am33xx/sec_fxns.c

arch/arm/cpu/armv7/am33xx/Makefile +2 -0

index 6fda482..d2b3e37 100644
@@ -20,3 +20,5 @@ obj-y += board.o
 obj-y  += mux.o
 
 obj-$(CONFIG_CLOCK_SYNTHESIZER)        += clk_synthesizer.o
+
+obj-$(CONFIG_TI_SECURE_DEVICE) += sec_fxns.o

arch/arm/cpu/armv7/am33xx/sec_fxns.c new file +40 -0

new file mode 100644
index 0000000..560297c
@@ -0,0 +1,90 @@
+/*
+ * sec_fxns.c
+ *
+ * Common security functions for AMxx devices that rely
+ * on secure ROM services.
+ *
+ * Copyright (C) 2013, Texas Instruments, Incorporated - http://www.ti.com/
+ *
+ * SPDX-License-Identifier:    GPL-2.0+
+ */
+
+#include <common.h>
+#include <asm/arch/sys_proto.h>
+#include <asm/omap_common.h>
+
+/* Index for signature verify ROM API*/
+#define API_HAL_KM_VERIFYCERTIFICATESIGNATURE_INDEX    (0x0000000E)
+
+static u32 find_sig_start(char *image, size_t size)
+{
+       char *image_end = image + size;
+       char *sig_start_magic = "CERT_";
+       int magic_str_len = strlen(sig_start_magic);
+       char *ch;
+
+       while (--image_end > image) {
+               if (*image_end == '_') {
+                       ch = image_end - magic_str_len + 1;
+                       if (!strncmp(ch, sig_start_magic, magic_str_len))
+                               return (u32)ch;
+               }
+       }
+       return 0;
+}
+
+int secure_boot_verify_image(void **image, size_t *size)
+{
+       int result = 1;
+       u32 cert_addr, sig_addr;
+       size_t cert_size;

arch/arm/cpu/armv7/am33xx/sec_fxns.c new file +23 -0

+       if (sig_addr == 0) {
+               puts("No signature found in image.\n");
+               result = 1;
+               goto auth_exit;
+       }
+
+       *size = sig_addr - cert_addr;   /* Subtract out the signature size */
+       cert_size = *size;
+
+       /* Check if image load address is 32-bit aligned */
+       if (0 != (0x3 & cert_addr)) {
+               puts("Image is not 4-byte aligned.\n");
+               result = 1;
+               goto auth_exit;
+       }
+
+       /* Image size also should be multiple of 4 */
+       if (0 != (0x3 & cert_size)) {
+               puts("Image size is not 4-byte aligned.\n");
+               result = 1;
+               goto auth_exit;
+       }
+

arch/arm/cpu/armv7/am33xx/sec_fxns.c new file +7 -0

+               printf("Return Value = %08X\n", result);
+               hang();
+       }
+
+       printf("Authentication passed: %s\n", (char *)sig_addr);
+       return result;
+}

arch/arm/include/asm/arch-am33xx/sys_proto.h +0 -0

index 8f573d2..f5fc916 100644

arch/arm/include/asm/arch-am33xx/sys_proto.h +2 -1

+#endif
+#endif
--

2.6.4

From: Madan Srinivas <[email protected]>

Adds a board specific FIT image post processing function when u-boot is

compiled for the high-secure (HS) device variant.

Signed-off-by Madan Srinivas <[email protected]>

Signed-off-by Andreas Dannenberg <[email protected]>

---
 board/ti/am43xx/board.c | 7 +++++++
 1 file changed, 7 insertions(+)

From: Madan Srinivas <[email protected]>

Adds a board specific FIT image post processing function when u-boot is compiled for the high-secure (HS) device variant.

Signed-off-by Madan Srinivas <[email protected]>

Signed-off-by Andreas Dannenberg <[email protected]>

---
 board/ti/am43xx/board.c | 7 +++++++
 1 file changed, 7 insertions(+)

board/ti/am43xx/board.c +0 -0

index f005762..fc0b38b 100644

board/ti/am43xx/board.c +5 -0

+void board_fit_image_post_process(void **p_image, size_t *p_size)
+{
+	secure_boot_verify_image(p_image, p_size);
+}
+#endif

From: Daniel Allred <[email protected]>

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 <[email protected]>

Signed-off-by Andreas Dannenberg <[email protected]>

---
 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(-)

arch/arm/cpu/armv7/omap-common/config_secure.mk +43 -2

index c7bb101..c4514ad 100644
@@ -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
+

arch/arm/cpu/armv7/omap-common/config_secure.mk +2 -0

+
+endif

arch/arm/cpu/armv7/omap5/config.mk +3 -1

index a7e55a5..503f31c 100644
@@ -15,5 +15,8 @@ else
 ALL-y  += MLO
 endif
 else
+ifeq ($(CONFIG_TI_SECURE_DEVICE)$(CONFIG_SECURE_BOOT),yy)
+ALL-y   += u-boot_HS.img
+endif
 ALL-y  += u-boot.img
 endif
--

2.6.4

Hi Andreas,

Quoted 8 lines · Andreas Dannenberg

From: Daniel Allred <[email protected]>

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.

Quoted 6 lines

Signed-off-by Daniel Allred <[email protected]>

Signed-off-by Andreas Dannenberg <[email protected]>

---
 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 <[email protected]>

But please can you add a README for this somewhere?

Also, can this tool be added to U-Boot instead of being external?

Quoted 72 lines

arch/arm/cpu/armv7/omap-common/config_secure.mk +47 -2

index c7bb101..c4514ad 100644
@@ -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))

arch/arm/cpu/armv7/omap-common/config_secure.mk u-boot-spl_HS_SPI_X-LOADER: $(obj)/u-boot-spl.bin · line 117

Quoted 6 lines

arch/arm/cpu/armv7/omap-common/config_secure.mk +6 -0

+       $(call if_changed,mkimage)
+       $(Q)if [ -f $@ ]; then \
+               cp -f $@ u-boot.img; \
+       fi
+
+.NOTPARALLEL: dtbs

Why is that needed?

Quoted 16 lines

arch/arm/cpu/armv7/omap-common/config_secure.mk +2 -0

+
+endif

arch/arm/cpu/armv7/omap5/config.mk +3 -1

index a7e55a5..503f31c 100644
@@ -15,5 +15,8 @@ else
 ALL-y  += MLO
 endif
 else
+ifeq ($(CONFIG_TI_SECURE_DEVICE)$(CONFIG_SECURE_BOOT),yy)
+ALL-y   += u-boot_HS.img
+endif
 ALL-y  += u-boot.img
 endif
--

Quoted 2 lines

2.6.4

Regards,
Simon

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

Quoted 18 lines · Simon Glass

Hi Andreas,

Quoted 8 lines · Andreas Dannenberg

From: Daniel Allred <[email protected]>

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.

Quoted 6 lines

Signed-off-by Daniel Allred <[email protected]>

Signed-off-by Andreas Dannenberg <[email protected]>

---
 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(-)

Quoted 5 lines

Reviewed-by Simon Glass <[email protected]>

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.

Quoted 73 lines

Quoted 73 lines

arch/arm/cpu/armv7/omap-common/config_secure.mk +47 -2

index c7bb101..c4514ad 100644
@@ -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))

Quoted 8 lines

arch/arm/cpu/armv7/omap-common/config_secure.mk u-boot-spl_HS_SPI_X-LOADER: $(obj)/u-boot-spl.bin · line 117

Quoted 6 lines

arch/arm/cpu/armv7/omap-common/config_secure.mk +6 -0

+       $(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---------------------------

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.

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.

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 for the Makefile question

Hi Andreas,

Quoted 35 lines · Andreas Dannenberg

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

Quoted 18 lines · Simon Glass

Hi Andreas,

Quoted 8 lines · Andreas Dannenberg

From: Daniel Allred <[email protected]>

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.

Quoted 6 lines

Signed-off-by Daniel Allred <[email protected]>

Signed-off-by Andreas Dannenberg <[email protected]>

---
 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(-)

Quoted 5 lines

Reviewed-by Simon Glass <[email protected]>

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.

Quoted 11 lines

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.

Quoted 103 lines

Quoted 72 lines

Quoted 72 lines

arch/arm/cpu/armv7/omap-common/config_secure.mk +47 -2

index c7bb101..c4514ad 100644
@@ -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))

Quoted 8 lines

arch/arm/cpu/armv7/omap-common/config_secure.mk u-boot-spl_HS_SPI_X-LOADER: $(obj)/u-boot-spl.bin · line 117

Quoted 6 lines

arch/arm/cpu/armv7/omap-common/config_secure.mk +6 -0

+       $(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....

Quoted 8 lines

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?

Quoted 11 lines

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.

Quoted 6 lines

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.

Quoted 7 lines

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.

Quoted 2 lines

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.

Hi Simon,
thanks for the continued feedback. Comments below...

Quoted 52 lines · Simon Glass

+Masahiro for the Makefile question

Hi Andreas,

Quoted 35 lines · Andreas Dannenberg

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

Quoted 18 lines · Simon Glass

Hi Andreas,

Quoted 8 lines · Andreas Dannenberg

From: Daniel Allred <[email protected]>

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.

Quoted 6 lines

Signed-off-by Daniel Allred <[email protected]>

Signed-off-by Andreas Dannenberg <[email protected]>

---
 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(-)

Quoted 5 lines

Reviewed-by Simon Glass <[email protected]>

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.

Quoted 11 lines

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.

Quoted 3 lines

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 :)

Quoted 112 lines

Quoted 103 lines

Quoted 72 lines

Quoted 72 lines

arch/arm/cpu/armv7/omap-common/config_secure.mk +47 -2

index c7bb101..c4514ad 100644
@@ -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))

Quoted 8 lines

arch/arm/cpu/armv7/omap-common/config_secure.mk u-boot-spl_HS_SPI_X-LOADER: $(obj)/u-boot-spl.bin · line 117

Quoted 6 lines

arch/arm/cpu/armv7/omap-common/config_secure.mk +6 -0

+       $(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....

Quoted 8 lines

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

Quoted 2 lines

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.

Quoted 12 lines

Quoted 11 lines

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.

Quoted 3 lines

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.

Quoted 14 lines

Quoted 6 lines

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.

Quoted 7 lines

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.

Quoted 1 line

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

Quoted 4 lines

Quoted 2 lines

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,

2016-06-21 11:35 GMT+09:00 Andreas Dannenberg <[email protected]>:

Quoted 231 lines

Hi Simon,
thanks for the continued feedback. Comments below...

Quoted 52 lines · Simon Glass

+Masahiro for the Makefile question

Hi Andreas,

Quoted 35 lines · Andreas Dannenberg

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

Quoted 18 lines · Simon Glass

Hi Andreas,

Quoted 8 lines · Andreas Dannenberg

From: Daniel Allred <[email protected]>

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.

Quoted 6 lines

Signed-off-by Daniel Allred <[email protected]>

Signed-off-by Andreas Dannenberg <[email protected]>

---
 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(-)

Quoted 5 lines

Reviewed-by Simon Glass <[email protected]>

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.

Quoted 11 lines

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.

Quoted 3 lines

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 :)

Quoted 112 lines

Quoted 103 lines

Quoted 72 lines

Quoted 72 lines

arch/arm/cpu/armv7/omap-common/config_secure.mk +47 -2

index c7bb101..c4514ad 100644
@@ -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))

Quoted 8 lines

arch/arm/cpu/armv7/omap-common/config_secure.mk u-boot-spl_HS_SPI_X-LOADER: $(obj)/u-boot-spl.bin · line 117

Quoted 6 lines

arch/arm/cpu/armv7/omap-common/config_secure.mk +6 -0

+       $(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....

Quoted 8 lines

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

Quoted 2 lines

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.

Quoted 12 lines

Quoted 11 lines

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.

Quoted 3 lines

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.

Quoted 14 lines

Quoted 6 lines

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.

Quoted 7 lines

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.

Quoted 1 line

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

Quoted 4 lines

Quoted 2 lines

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.

Quoted 8 lines

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.