[U-Boot] [PATCH 0/5] Secure EMIF firewall and memory reservation on DRA7xx/AM57x devices
These patches to add secure memory reservations and EMIF firewall config to SDRAM init code. The reservation and firewall config is done using PPA installed HAL APIs, so they are not common to all platforms (so they are put in omap5 path instead of omap-common).
With these patches applied, a secure memory reservation can be specified with the following configs:
CONFIG_TI_SECURE_EMIF_REGION_START - start location of region. If it is not specified, then the region will be placed at the end of the SDRAM. CONFIG_TI_SECURE_EMIF_TOTAL_REGION_SIZE - total size of complete region CONFIG_TI_SECURE_EMIF_PROTECTED_REGION_SIZE - size (less than total) made secure using secure firewalls. The secured region begins at the start location (it comes first), and anything left over will be non-secure but still reserved from use by u-boot and the kernel
The secure_emif_reserve() API will make use of the above configs to make a part of the SDRAM secure, with the lowest enforcing priority, giving access to the ARM TrustZone world only. The secure_emif_firewall_setup() API is also introduced. This API allows setting other EMIF firewall regions with particular permissions (for other cores, etc). The current code does not use this API, but it exists to help satisfy particular system requirements that users might need.
After all the configuration is done, the secure_emif_firewall_lock() API should be called to make the previous two APIs stop working. This is important in order to prevent a later compromise of public supervisor code from being able to modify the EMIF firewalls. This API is in the code, so any use of the secure_emif_firewall_setup() API must be inserted before the lock API is called.
Daniel Allred (5): ti: omap5: Add Kconfig options for secure EMIF reservations arm: omap5: secure API for EMIF memory reservations ARM: DRA7: Add secure emif setup calls ti_omap5_common: mark region of DRAM protected on HS parts ARM: omap5: add fdt secure dram reservation fixup
arch/arm/cpu/armv7/omap-common/emif-common.c | 15 ++++ arch/arm/cpu/armv7/omap5/Kconfig | 26 ++++++ arch/arm/cpu/armv7/omap5/Makefile | 1 + arch/arm/cpu/armv7/omap5/fdt.c | 64 +++++++++++++- arch/arm/cpu/armv7/omap5/sec-fxns.c | 126 +++++++++++++++++++++++++++ arch/arm/include/asm/omap_sec_common.h | 24 +++++ include/configs/ti_omap5_common.h | 8 ++ 7 files changed, 262 insertions(+), 2 deletions(-) create mode 100644 arch/arm/cpu/armv7/omap5/sec-fxns.c
Adds start address and size config options for setting aside a portion of the EMIF memory space for usage by security software (like a secure OS/TEE). There are two sizes, a total size and a protected size. The region is divided into protected (secure) and unprotected (public) regions, that are contiguous and start at the start address given. If the start address is zero, the intention is that the region will be automatically placed at the end of the available external DRAM space.
Signed-off-by: Daniel Allred d-allred@ti.com --- arch/arm/cpu/armv7/omap5/Kconfig | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)
diff --git a/arch/arm/cpu/armv7/omap5/Kconfig b/arch/arm/cpu/armv7/omap5/Kconfig index a8600b1..25474ed 100644 --- a/arch/arm/cpu/armv7/omap5/Kconfig +++ b/arch/arm/cpu/armv7/omap5/Kconfig @@ -24,6 +24,32 @@ endchoice config SYS_SOC default "omap5"
+config TI_SECURE_EMIF_REGION_START + hex "Reserved EMIF region start address" + depends on TI_SECURE_DEVICE + default 0x0 + help + Reserved EMIF region start address. Set to "0" to auto-select + to be at the end of the external memory region. + +config TI_SECURE_EMIF_TOTAL_REGION_SIZE + hex "Reserved EMIF region size" + depends on TI_SECURE_DEVICE + default 0x0 + help + Total reserved EMIF region size. Default is 0, which means no reserved EMIF + region on secure devices. + +config TI_SECURE_EMIF_PROTECTED_REGION_SIZE + hex "Size of protected region within reserved EMIF region" + depends on TI_SECURE_DEVICE + default 0x0 + help + This config option is used to specify the size of the portion of the total + reserved EMIF region set aside for secure OS needs that will be protected + using hardware memory firewalls. This value must be smaller than the + TI_SECURE_EMIF_TOTAL_REGION_SIZE value. + source "board/compulab/cm_t54/Kconfig" source "board/ti/omap5_uevm/Kconfig" source "board/ti/dra7xx/Kconfig"
On Fri, Sep 02, 2016 at 12:40:20AM -0500, Daniel Allred wrote:
Adds start address and size config options for setting aside a portion of the EMIF memory space for usage by security software (like a secure OS/TEE). There are two sizes, a total size and a protected size. The region is divided into protected (secure) and unprotected (public) regions, that are contiguous and start at the start address given. If the start address is zero, the intention is that the region will be automatically placed at the end of the available external DRAM space.
Signed-off-by: Daniel Allred d-allred@ti.com
arch/arm/cpu/armv7/omap5/Kconfig | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)
diff --git a/arch/arm/cpu/armv7/omap5/Kconfig b/arch/arm/cpu/armv7/omap5/Kconfig index a8600b1..25474ed 100644 --- a/arch/arm/cpu/armv7/omap5/Kconfig +++ b/arch/arm/cpu/armv7/omap5/Kconfig @@ -24,6 +24,32 @@ endchoice config SYS_SOC default "omap5"
I think we should hide these based on some config option and then have a menu here with these entries. That also means that you don't need to have "0 to disable" for the size.
On 09/02/2016 12:40 AM, Daniel Allred wrote:
Adds start address and size config options for setting aside a portion of the EMIF memory space for usage by security software (like a secure OS/TEE). There are two sizes, a total size and a protected size. The region is divided into protected (secure) and unprotected (public) regions, that are contiguous and start at the start address given. If the start address is zero, the intention is that the region will be automatically placed at the end of the available external DRAM space.
Signed-off-by: Daniel Allred d-allred@ti.com
arch/arm/cpu/armv7/omap5/Kconfig | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)
diff --git a/arch/arm/cpu/armv7/omap5/Kconfig b/arch/arm/cpu/armv7/omap5/Kconfig index a8600b1..25474ed 100644 --- a/arch/arm/cpu/armv7/omap5/Kconfig +++ b/arch/arm/cpu/armv7/omap5/Kconfig @@ -24,6 +24,32 @@ endchoice config SYS_SOC default "omap5"
+config TI_SECURE_EMIF_REGION_START
- hex "Reserved EMIF region start address"
- depends on TI_SECURE_DEVICE
- default 0x0
- help
Reserved EMIF region start address. Set to "0" to auto-selectto be at the end of the external memory region.
The secure image that is placed in this location (OPTEE OS for now) will probably not be relocatable, what happens when we add RAM to a board (some boards take regular ram sticks), will our secure world suddenly break? What do we gain from having the location auto-selected?
+config TI_SECURE_EMIF_TOTAL_REGION_SIZE
Instead of having the total combined size of both the protected and non-protected secure region then subtracting out the protected below to get the non-protected, why not have:
TI_SECURE_EMIF_PUBLIC_REGION_SIZE TI_SECURE_EMIF_PROTECTED_REGION_SIZE
then add the two to get the total region size? This would also remove some checks to make sure the total isn't smaller than one part, plus it removes the need for any mental math in figuring the size of the public region.
- hex "Reserved EMIF region size"
- depends on TI_SECURE_DEVICE
- default 0x0
- help
Total reserved EMIF region size. Default is 0, which means no reserved EMIFregion on secure devices.+config TI_SECURE_EMIF_PROTECTED_REGION_SIZE
- hex "Size of protected region within reserved EMIF region"
- depends on TI_SECURE_DEVICE
- default 0x0
- help
This config option is used to specify the size of the portion of the totalreserved EMIF region set aside for secure OS needs that will be protectedusing hardware memory firewalls. This value must be smaller than theTI_SECURE_EMIF_TOTAL_REGION_SIZE value.source "board/compulab/cm_t54/Kconfig" source "board/ti/omap5_uevm/Kconfig" source "board/ti/dra7xx/Kconfig"
On 9/6/2016 3:23 PM, Andrew F. Davis wrote:
On 09/02/2016 12:40 AM, Daniel Allred wrote:
Adds start address and size config options for setting aside a portion of the EMIF memory space for usage by security software (like a secure OS/TEE). There are two sizes, a total size and a protected size. The region is divided into protected (secure) and unprotected (public) regions, that are contiguous and start at the start address given. If the start address is zero, the intention is that the region will be automatically placed at the end of the available external DRAM space.
Signed-off-by: Daniel Allred d-allred@ti.com
arch/arm/cpu/armv7/omap5/Kconfig | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)
diff --git a/arch/arm/cpu/armv7/omap5/Kconfig b/arch/arm/cpu/armv7/omap5/Kconfig index a8600b1..25474ed 100644 --- a/arch/arm/cpu/armv7/omap5/Kconfig +++ b/arch/arm/cpu/armv7/omap5/Kconfig @@ -24,6 +24,32 @@ endchoice config SYS_SOC default "omap5"
+config TI_SECURE_EMIF_REGION_START
- hex "Reserved EMIF region start address"
- depends on TI_SECURE_DEVICE
- default 0x0
- help
Reserved EMIF region start address. Set to "0" to auto-selectto be at the end of the external memory region.The secure image that is placed in this location (OPTEE OS for now) will probably not be relocatable, what happens when we add RAM to a board (some boards take regular ram sticks), will our secure world suddenly break? What do we gain from having the location auto-selected?
See the answer I provided to the other patch about PRAM. Auto-selection isn't the key, but putting it at the end out of the way of all potential conflicts is. If the secure OS/TEE is not PIC, then re-linking will be required if the RAM size changes. But I personally see a change in RAM size as a new platform/board, so I don't think rebuilding some of the SW for that platform is completely unreasonable. Ultimately, it will depend on the secure OS/TEE.
+config TI_SECURE_EMIF_TOTAL_REGION_SIZE
Instead of having the total combined size of both the protected and non-protected secure region then subtracting out the protected below to get the non-protected, why not have:
TI_SECURE_EMIF_PUBLIC_REGION_SIZE TI_SECURE_EMIF_PROTECTED_REGION_SIZE
then add the two to get the total region size? This would also remove some checks to make sure the total isn't smaller than one part, plus it removes the need for any mental math in figuring the size of the public region.
We had it this way previously, but the code ended up more complicated (though maybe the preprocessor checks were simpler). Having seen it both ways, I think it is a wash.
Daniel
- hex "Reserved EMIF region size"
- depends on TI_SECURE_DEVICE
- default 0x0
- help
Total reserved EMIF region size. Default is 0, which means no reserved EMIFregion on secure devices.+config TI_SECURE_EMIF_PROTECTED_REGION_SIZE
- hex "Size of protected region within reserved EMIF region"
- depends on TI_SECURE_DEVICE
- default 0x0
- help
This config option is used to specify the size of the portion of the totalreserved EMIF region set aside for secure OS needs that will be protectedusing hardware memory firewalls. This value must be smaller than theTI_SECURE_EMIF_TOTAL_REGION_SIZE value.source "board/compulab/cm_t54/Kconfig" source "board/ti/omap5_uevm/Kconfig" source "board/ti/dra7xx/Kconfig"
On Fri, Sep 02, 2016 at 12:40:20AM -0500, Daniel Allred wrote:
Adds start address and size config options for setting aside a portion of the EMIF memory space for usage by security software (like a secure OS/TEE). There are two sizes, a total size and a protected size. The region is divided into protected (secure) and unprotected (public) regions, that are contiguous and start at the start address given. If the start address is zero, the intention is that the region will be automatically placed at the end of the available external DRAM space.
Signed-off-by: Daniel Allred d-allred@ti.com
Applied to u-boot/master, thanks!
Create a few public APIs which rely on secure world ROM/HAL APIs for their implementation. These are intended to be used to reserve a portion of the EMIF memory and configure hardware firewalls around that region to prevent public code from manipulating or interfering with that memory.
Signed-off-by: Daniel Allred d-allred@ti.com --- arch/arm/cpu/armv7/omap5/Makefile | 1 + arch/arm/cpu/armv7/omap5/sec-fxns.c | 126 +++++++++++++++++++++++++++++++++ arch/arm/include/asm/omap_sec_common.h | 24 +++++++ 3 files changed, 151 insertions(+) create mode 100644 arch/arm/cpu/armv7/omap5/sec-fxns.c
diff --git a/arch/arm/cpu/armv7/omap5/Makefile b/arch/arm/cpu/armv7/omap5/Makefile index 3caba86..0212df7 100644 --- a/arch/arm/cpu/armv7/omap5/Makefile +++ b/arch/arm/cpu/armv7/omap5/Makefile @@ -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 diff --git a/arch/arm/cpu/armv7/omap5/sec-fxns.c b/arch/arm/cpu/armv7/omap5/sec-fxns.c new file mode 100644 index 0000000..33d4ea4 --- /dev/null +++ b/arch/arm/cpu/armv7/omap5/sec-fxns.c @@ -0,0 +1,126 @@ +/* + * + * Security related functions for OMAP5 class devices + * + * (C) Copyright 2016 + * Texas Instruments, <www.ti.com> + * + * Daniel Allred d-allred@ti.com + * Harinarayan Bhatta harinarayan@ti.com + * + * SPDX-License-Identifier: GPL-2.0+ + */ + +#include <common.h> +#include <stdarg.h> + +#include <asm/arch/sys_proto.h> +#include <asm/omap_common.h> +#include <asm/omap_sec_common.h> +#include <asm/spl.h> +#include <spl.h> + +/* Index for signature PPA-based TI HAL APIs */ +#define PPA_HAL_SERVICES_START_INDEX (0x200) +#define PPA_SERV_HAL_SETUP_SEC_RESVD_REGION (PPA_HAL_SERVICES_START_INDEX + 25) +#define PPA_SERV_HAL_SETUP_EMIF_FW_REGION (PPA_HAL_SERVICES_START_INDEX + 26) +#define PPA_SERV_HAL_LOCK_EMIF_FW (PPA_HAL_SERVICES_START_INDEX + 27) + +static u32 get_sec_mem_start(void) +{ + u32 sec_mem_start = CONFIG_TI_SECURE_EMIF_REGION_START; + u32 sec_mem_size = CONFIG_TI_SECURE_EMIF_TOTAL_REGION_SIZE; + /* + * Total reserved region is all contiguous with protected + * region coming first, followed by the non-secure region. + * If 0x0 start address is given, we simply put the reserved + * region at the end of the external DRAM. + */ + if (sec_mem_start == 0) + sec_mem_start = + (CONFIG_SYS_SDRAM_BASE + + (omap_sdram_size() - sec_mem_size)); + return sec_mem_start; +} + +int secure_emif_firewall_setup(uint8_t region_num, uint32_t start_addr, + uint32_t size, uint32_t access_perm, + uint32_t initiator_perm) +{ + int result = 1; + + /* + * Call PPA HAL API to do any other general firewall + * configuration for regions 1-6 of the EMIF firewall. + */ + debug("%s: regionNum = %x, startAddr = %x, size = %x", __func__, + region_num, start_addr, size); + + result = secure_rom_call( + PPA_SERV_HAL_SETUP_EMIF_FW_REGION, 0, 0, 4, + (start_addr & 0xFFFFFFF0) | (region_num & 0x0F), + size, access_perm, initiator_perm); + + if (result != 0) { + puts("Secure EMIF Firewall Setup failed!\n"); + debug("Return Value = %x\n", result); + } + + return result; +} + +#if (CONFIG_TI_SECURE_EMIF_TOTAL_REGION_SIZE < \ + CONFIG_TI_SECURE_EMIF_PROTECTED_REGION_SIZE) +#error "TI Secure EMIF: Protected size cannot be larger than total size." +#endif +int secure_emif_reserve(void) +{ + int result = 1; + u32 sec_mem_start = get_sec_mem_start(); + u32 sec_prot_size = CONFIG_TI_SECURE_EMIF_PROTECTED_REGION_SIZE; + + /* If there is no protected region, there is no reservation to make */ + if (sec_prot_size == 0) + return 0; + + /* + * Call PPA HAL API to reserve a chunk of EMIF SDRAM + * for secure world use. This region should be carved out + * from use by any public code. EMIF firewall region 7 + * will be used to protect this block of memory. + */ + result = secure_rom_call( + PPA_SERV_HAL_SETUP_SEC_RESVD_REGION, + 0, 0, 2, sec_mem_start, sec_prot_size); + + if (result != 0) { + puts("SDRAM Firewall: Secure memory reservation failed!\n"); + debug("Return Value = %x\n", result); + } + + return result; +} + +int secure_emif_firewall_lock(void) +{ + int result = 1; + + /* + * Call PPA HAL API to lock the EMIF firewall configurations. + * After this API is called, none of the PPA HAL APIs for + * configuring the EMIF firewalls will be usable again (that + * is, calls to those APIs will return failure and have no + * effect). + */ + + result = secure_rom_call( + PPA_SERV_HAL_LOCK_EMIF_FW, + 0, 0, 0); + + if (result != 0) { + puts("Secure EMIF Firewall Lock failed!\n"); + debug("Return Value = %x\n", result); + } + + return result; +} diff --git a/arch/arm/include/asm/omap_sec_common.h b/arch/arm/include/asm/omap_sec_common.h index 842f2af..4bde93f 100644 --- a/arch/arm/include/asm/omap_sec_common.h +++ b/arch/arm/include/asm/omap_sec_common.h @@ -27,4 +27,28 @@ u32 secure_rom_call(u32 service, u32 proc_id, u32 flag, ...); */ int secure_boot_verify_image(void **p_image, size_t *p_size);
+/* + * Invoke a secure HAL API that allows configuration of the external memory + * firewall regions. + */ +int secure_emif_firewall_setup(uint8_t region_num, uint32_t start_addr, + uint32_t size, uint32_t access_perm, + uint32_t initiator_perm); + +/* + * Invoke a secure HAL API on high-secure (HS) device variants that reserves a + * region of external memory for secure world use, and protects it using memory + * firewalls that prevent public world access. This API is intended to setaside + * memory that will be used for a secure world OS/TEE. + */ +int secure_emif_reserve(void); + +/* + * Invoke a secure HAL API to lock the external memory firewall configurations. + * After this API is called, none of the HAL APIs for configuring the that + * firewall will be usable (calls to those APIs will return failure and have + * no effect). + */ +int secure_emif_firewall_lock(void); + #endif /* _OMAP_SEC_COMMON_H_ */
On Fri, Sep 02, 2016 at 12:40:21AM -0500, Daniel Allred wrote:
Create a few public APIs which rely on secure world ROM/HAL APIs for their implementation. These are intended to be used to reserve a portion of the EMIF memory and configure hardware firewalls around that region to prevent public code from manipulating or interfering with that memory.
Signed-off-by: Daniel Allred d-allred@ti.com
Reviewed-by: Tom Rini trini@konsulko.com
On Fri, Sep 02, 2016 at 12:40:21AM -0500, Daniel Allred wrote:
Create a few public APIs which rely on secure world ROM/HAL APIs for their implementation. These are intended to be used to reserve a portion of the EMIF memory and configure hardware firewalls around that region to prevent public code from manipulating or interfering with that memory.
Signed-off-by: Daniel Allred d-allred@ti.com Reviewed-by: Tom Rini trini@konsulko.com
Applied to u-boot/master, thanks!
After EMIF DRAM is configured, but before it is used, calls are made on secure devices to reserve any configured memory region needed by the secure world and then to lock the EMIF firewall configuration. If any other firewall configuration needs to be applied, it must happen before the lock call.
Signed-off-by: Daniel Allred d-allred@ti.com --- arch/arm/cpu/armv7/omap-common/emif-common.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/arch/arm/cpu/armv7/omap-common/emif-common.c b/arch/arm/cpu/armv7/omap-common/emif-common.c index 2b79010..b26984e 100644 --- a/arch/arm/cpu/armv7/omap-common/emif-common.c +++ b/arch/arm/cpu/armv7/omap-common/emif-common.c @@ -14,6 +14,7 @@ #include <asm/arch/clock.h> #include <asm/arch/sys_proto.h> #include <asm/omap_common.h> +#include <asm/omap_sec_common.h> #include <asm/utils.h> #include <linux/compiler.h>
@@ -1477,6 +1478,20 @@ void sdram_init(void) debug("get_ram_size() successful"); }
+#if defined(CONFIG_TI_SECURE_DEVICE) + /* + * On HS devices, do static EMIF firewall configuration + * but only do it if not already running in SDRAM + */ + if (!in_sdram) + if (0 != secure_emif_reserve()) + hang(); + + /* On HS devices, ensure static EMIF firewall APIs are locked */ + if (0 != secure_emif_firewall_lock()) + hang(); +#endif + if (sdram_type == EMIF_SDRAM_TYPE_DDR3 && (!in_sdram && !warm_reset()) && (!is_dra7xx())) { if (emif1_enabled)
On Fri, Sep 02, 2016 at 12:40:22AM -0500, Daniel Allred wrote:
After EMIF DRAM is configured, but before it is used, calls are made on secure devices to reserve any configured memory region needed by the secure world and then to lock the EMIF firewall configuration. If any other firewall configuration needs to be applied, it must happen before the lock call.
Signed-off-by: Daniel Allred d-allred@ti.com
arch/arm/cpu/armv7/omap-common/emif-common.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/arch/arm/cpu/armv7/omap-common/emif-common.c b/arch/arm/cpu/armv7/omap-common/emif-common.c index 2b79010..b26984e 100644 --- a/arch/arm/cpu/armv7/omap-common/emif-common.c +++ b/arch/arm/cpu/armv7/omap-common/emif-common.c @@ -14,6 +14,7 @@ #include <asm/arch/clock.h> #include <asm/arch/sys_proto.h> #include <asm/omap_common.h> +#include <asm/omap_sec_common.h> #include <asm/utils.h> #include <linux/compiler.h>
@@ -1477,6 +1478,20 @@ void sdram_init(void) debug("get_ram_size() successful"); }
+#if defined(CONFIG_TI_SECURE_DEVICE)
- /*
* On HS devices, do static EMIF firewall configuration* but only do it if not already running in SDRAM*/- if (!in_sdram)
if (0 != secure_emif_reserve())hang();- /* On HS devices, ensure static EMIF firewall APIs are locked */
- if (0 != secure_emif_firewall_lock())
hang();
Those are awkward tests (should be func() != val), and since it's just checking for function return status, we should just write that normally. Thanks!
On 9/2/2016 9:54 AM, Tom Rini wrote:
On Fri, Sep 02, 2016 at 12:40:22AM -0500, Daniel Allred wrote:
After EMIF DRAM is configured, but before it is used, calls are made on secure devices to reserve any configured memory region needed by the secure world and then to lock the EMIF firewall configuration. If any other firewall configuration needs to be applied, it must happen before the lock call.
Signed-off-by: Daniel Allred d-allred@ti.com
arch/arm/cpu/armv7/omap-common/emif-common.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/arch/arm/cpu/armv7/omap-common/emif-common.c b/arch/arm/cpu/armv7/omap-common/emif-common.c index 2b79010..b26984e 100644 --- a/arch/arm/cpu/armv7/omap-common/emif-common.c +++ b/arch/arm/cpu/armv7/omap-common/emif-common.c @@ -14,6 +14,7 @@ #include <asm/arch/clock.h> #include <asm/arch/sys_proto.h> #include <asm/omap_common.h> +#include <asm/omap_sec_common.h> #include <asm/utils.h> #include <linux/compiler.h>
@@ -1477,6 +1478,20 @@ void sdram_init(void) debug("get_ram_size() successful"); }
+#if defined(CONFIG_TI_SECURE_DEVICE)
- /*
* On HS devices, do static EMIF firewall configuration* but only do it if not already running in SDRAM*/- if (!in_sdram)
if (0 != secure_emif_reserve())hang();- /* On HS devices, ensure static EMIF firewall APIs are locked */
- if (0 != secure_emif_firewall_lock())
hang();Those are awkward tests (should be func() != val), and since it's just checking for function return status, we should just write that normally. Thanks!
Fair point, will change.
On 09/02/2016 12:40 AM, Daniel Allred wrote:
After EMIF DRAM is configured, but before it is used, calls are made on secure devices to reserve any configured memory region needed by the secure world and then to lock the EMIF firewall configuration. If any other firewall configuration needs to be applied, it must happen before the lock call.
Signed-off-by: Daniel Allred d-allred@ti.com
arch/arm/cpu/armv7/omap-common/emif-common.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/arch/arm/cpu/armv7/omap-common/emif-common.c b/arch/arm/cpu/armv7/omap-common/emif-common.c index 2b79010..b26984e 100644 --- a/arch/arm/cpu/armv7/omap-common/emif-common.c +++ b/arch/arm/cpu/armv7/omap-common/emif-common.c @@ -14,6 +14,7 @@ #include <asm/arch/clock.h> #include <asm/arch/sys_proto.h> #include <asm/omap_common.h> +#include <asm/omap_sec_common.h> #include <asm/utils.h> #include <linux/compiler.h>
@@ -1477,6 +1478,20 @@ void sdram_init(void) debug("get_ram_size() successful"); }
+#if defined(CONFIG_TI_SECURE_DEVICE)
- /*
* On HS devices, do static EMIF firewall configuration* but only do it if not already running in SDRAM*/- if (!in_sdram)
Should we hang in this case?
if (0 != secure_emif_reserve())
"0 != " could be removed here and below.
hang();- /* On HS devices, ensure static EMIF firewall APIs are locked */
- if (0 != secure_emif_firewall_lock())
hang();+#endif
- if (sdram_type == EMIF_SDRAM_TYPE_DDR3 && (!in_sdram && !warm_reset()) && (!is_dra7xx())) { if (emif1_enabled)
On 9/6/2016 3:41 PM, Andrew F. Davis wrote:
On 09/02/2016 12:40 AM, Daniel Allred wrote:
After EMIF DRAM is configured, but before it is used, calls are made on secure devices to reserve any configured memory region needed by the secure world and then to lock the EMIF firewall configuration. If any other firewall configuration needs to be applied, it must happen before the lock call.
Signed-off-by: Daniel Allred d-allred@ti.com
arch/arm/cpu/armv7/omap-common/emif-common.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/arch/arm/cpu/armv7/omap-common/emif-common.c b/arch/arm/cpu/armv7/omap-common/emif-common.c index 2b79010..b26984e 100644 --- a/arch/arm/cpu/armv7/omap-common/emif-common.c +++ b/arch/arm/cpu/armv7/omap-common/emif-common.c @@ -14,6 +14,7 @@ #include <asm/arch/clock.h> #include <asm/arch/sys_proto.h> #include <asm/omap_common.h> +#include <asm/omap_sec_common.h> #include <asm/utils.h> #include <linux/compiler.h>
@@ -1477,6 +1478,20 @@ void sdram_init(void) debug("get_ram_size() successful"); }
+#if defined(CONFIG_TI_SECURE_DEVICE)
- /*
* On HS devices, do static EMIF firewall configuration* but only do it if not already running in SDRAM*/- if (!in_sdram)
Should we hang in this case?
The check is supposed to ensure that if we are already in DRAM, we don't apply the firewall config. It might make sense to still (attempt to) do the lock. I need to think a little more on it, but I don't think hanging if in sdram is the right thing.
if (0 != secure_emif_reserve())"0 != " could be removed here and below.
hang();- /* On HS devices, ensure static EMIF firewall APIs are locked */
- if (0 != secure_emif_firewall_lock())
hang();+#endif
- if (sdram_type == EMIF_SDRAM_TYPE_DDR3 && (!in_sdram && !warm_reset()) && (!is_dra7xx())) { if (emif1_enabled)
On Fri, Sep 02, 2016 at 12:40:22AM -0500, Daniel Allred wrote:
After EMIF DRAM is configured, but before it is used, calls are made on secure devices to reserve any configured memory region needed by the secure world and then to lock the EMIF firewall configuration. If any other firewall configuration needs to be applied, it must happen before the lock call.
Signed-off-by: Daniel Allred d-allred@ti.com
Applied to u-boot/master, thanks!
If the ending portion of the DRAM is reserved for secure world use, then u-boot cannot use this memory for its relocation purposes. To prevent issues, we mark this memory as PRAM and this prevents it from being used by u-boot at all.
Signed-off-by: Daniel Allred d-allred@ti.com --- include/configs/ti_omap5_common.h | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/include/configs/ti_omap5_common.h b/include/configs/ti_omap5_common.h index e42c88e..70fdc6e 100644 --- a/include/configs/ti_omap5_common.h +++ b/include/configs/ti_omap5_common.h @@ -146,6 +146,14 @@ */ #define TI_OMAP5_SECURE_BOOT_RESV_SRAM_SZ 0x1000 #define CONFIG_SPL_TEXT_BASE 0x40301350 +/* If no specific start address is specified then the secure EMIF + * region will be placed at the end of the DDR space. In order to prevent + * the main u-boot relocation from clobbering that memory and causing a + * firewall violation, we tell u-boot that memory is protected RAM (PRAM) + */ +#if (CONFIG_TI_SECURE_EMIF_REGION_START == 0) +#define CONFIG_PRAM (CONFIG_TI_SECURE_EMIF_TOTAL_REGION_SIZE) >> 10 +#endif #else /* * For all booting on GP parts, the flash loader image is
On Fri, Sep 02, 2016 at 12:40:23AM -0500, Daniel Allred wrote:
If the ending portion of the DRAM is reserved for secure world use, then u-boot cannot use this memory for its relocation purposes. To prevent issues, we mark this memory as PRAM and this prevents it from being used by u-boot at all.
Signed-off-by: Daniel Allred d-allred@ti.com
include/configs/ti_omap5_common.h | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/include/configs/ti_omap5_common.h b/include/configs/ti_omap5_common.h index e42c88e..70fdc6e 100644 --- a/include/configs/ti_omap5_common.h +++ b/include/configs/ti_omap5_common.h @@ -146,6 +146,14 @@ */ #define TI_OMAP5_SECURE_BOOT_RESV_SRAM_SZ 0x1000 #define CONFIG_SPL_TEXT_BASE 0x40301350 +/* If no specific start address is specified then the secure EMIF
- region will be placed at the end of the DDR space. In order to prevent
- the main u-boot relocation from clobbering that memory and causing a
- firewall violation, we tell u-boot that memory is protected RAM (PRAM)
- */
+#if (CONFIG_TI_SECURE_EMIF_REGION_START == 0) +#define CONFIG_PRAM (CONFIG_TI_SECURE_EMIF_TOTAL_REGION_SIZE) >> 10 +#endif #else /*
- For all booting on GP parts, the flash loader image is
Can you move PRAM to Kconfig please? Thinking about this more, I think what you want for the size option is CONFIG_PRAM and then to use that to expand out the size.
On 09/02/2016 12:40 AM, Daniel Allred wrote:
If the ending portion of the DRAM is reserved for secure world use, then u-boot cannot use this memory for its relocation purposes. To prevent issues, we mark this memory as PRAM and this prevents it from being used by u-boot at all.
Signed-off-by: Daniel Allred d-allred@ti.com
include/configs/ti_omap5_common.h | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/include/configs/ti_omap5_common.h b/include/configs/ti_omap5_common.h index e42c88e..70fdc6e 100644 --- a/include/configs/ti_omap5_common.h +++ b/include/configs/ti_omap5_common.h @@ -146,6 +146,14 @@ */ #define TI_OMAP5_SECURE_BOOT_RESV_SRAM_SZ 0x1000 #define CONFIG_SPL_TEXT_BASE 0x40301350 +/* If no specific start address is specified then the secure EMIF
- region will be placed at the end of the DDR space. In order to prevent
- the main u-boot relocation from clobbering that memory and causing a
- firewall violation, we tell u-boot that memory is protected RAM (PRAM)
- */
+#if (CONFIG_TI_SECURE_EMIF_REGION_START == 0)
What about if we set the start address manually, what prevents u-boot from over-writing that memory?
+#define CONFIG_PRAM (CONFIG_TI_SECURE_EMIF_TOTAL_REGION_SIZE) >> 10 +#endif #else /*
- For all booting on GP parts, the flash loader image is
On Tue, Sep 06, 2016 at 03:54:31PM -0500, Andrew F. Davis wrote:
On 09/02/2016 12:40 AM, Daniel Allred wrote:
If the ending portion of the DRAM is reserved for secure world use, then u-boot cannot use this memory for its relocation purposes. To prevent issues, we mark this memory as PRAM and this prevents it from being used by u-boot at all.
Signed-off-by: Daniel Allred d-allred@ti.com
include/configs/ti_omap5_common.h | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/include/configs/ti_omap5_common.h b/include/configs/ti_omap5_common.h index e42c88e..70fdc6e 100644 --- a/include/configs/ti_omap5_common.h +++ b/include/configs/ti_omap5_common.h @@ -146,6 +146,14 @@ */ #define TI_OMAP5_SECURE_BOOT_RESV_SRAM_SZ 0x1000 #define CONFIG_SPL_TEXT_BASE 0x40301350 +/* If no specific start address is specified then the secure EMIF
- region will be placed at the end of the DDR space. In order to prevent
- the main u-boot relocation from clobbering that memory and causing a
- firewall violation, we tell u-boot that memory is protected RAM (PRAM)
- */
+#if (CONFIG_TI_SECURE_EMIF_REGION_START == 0)
What about if we set the start address manually, what prevents u-boot from over-writing that memory?
I think that's another reason this needs to be made to use the existing pram mechanism as we make sure that if pram isn't set in the environment we set it to the default of CONFIG_PRAM.
On 9/6/2016 3:54 PM, Andrew F. Davis wrote:
On 09/02/2016 12:40 AM, Daniel Allred wrote:
If the ending portion of the DRAM is reserved for secure world use, then u-boot cannot use this memory for its relocation purposes. To prevent issues, we mark this memory as PRAM and this prevents it from being used by u-boot at all.
Signed-off-by: Daniel Allred d-allred@ti.com
include/configs/ti_omap5_common.h | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/include/configs/ti_omap5_common.h b/include/configs/ti_omap5_common.h index e42c88e..70fdc6e 100644 --- a/include/configs/ti_omap5_common.h +++ b/include/configs/ti_omap5_common.h @@ -146,6 +146,14 @@ */ #define TI_OMAP5_SECURE_BOOT_RESV_SRAM_SZ 0x1000 #define CONFIG_SPL_TEXT_BASE 0x40301350 +/* If no specific start address is specified then the secure EMIF
- region will be placed at the end of the DDR space. In order to prevent
- the main u-boot relocation from clobbering that memory and causing a
- firewall violation, we tell u-boot that memory is protected RAM (PRAM)
- */
+#if (CONFIG_TI_SECURE_EMIF_REGION_START == 0)
What about if we set the start address manually, what prevents u-boot from over-writing that memory?
Nothing. The PRAM mechanism, as it is currently defined in the u-boot code, can only protect the RAM at the end. It prevents u-boot from seeing that memory so that it won't try to relocate itself there during the u-boot relocation. We found this was needed because the firewalls are warm-reset insensitive and so u-boot could cause firewall violations after a warm reset. So we really need to put this memory somewhere where it can be set aside from all other uses, and the CONFIG_PRAM mechanism accomplishes this.
If you manually place it anywhere else in the DRAM, you have to make sure that memory will not be used by any u-boot code. I think we saw one case early on in development where the location of the secure OS ended up conflicting with the Ethernet buffer memory, thus breaking network/NFS booting (that was when we were loading the TEE under Linux, so we only saw the violation on warm reset, not the initial boot). Since all other DRAM users in u-boot avoid the end of DRAM because of its use for u-boot relocation, using the PRAM option to put it there avoids all possible issues. So manual placement can work, but it has more risks. But you can then place the secure OS/TEE at a fixed location, which could be helpful if it is not PIC and you want to support it across quite a few platforms with different memory sizes without a re-link.
Regards, Daniel
+#define CONFIG_PRAM (CONFIG_TI_SECURE_EMIF_TOTAL_REGION_SIZE) >> 10 +#endif #else /*
- For all booting on GP parts, the flash loader image is
On Fri, Sep 02, 2016 at 12:40:23AM -0500, Daniel Allred wrote:
If the ending portion of the DRAM is reserved for secure world use, then u-boot cannot use this memory for its relocation purposes. To prevent issues, we mark this memory as PRAM and this prevents it from being used by u-boot at all.
Signed-off-by: Daniel Allred d-allred@ti.com
Applied to u-boot/master, thanks!
Adds a secure dram reservation fixup for secure devices, when a region in the emif has been set aside for secure world use. The size is defined by the CONFIG_TI_SECURE_EMIF_TOTAL_REGION_SIZE config option.
Signed-off-by: Daniel Allred d-allred@ti.com --- arch/arm/cpu/armv7/omap5/fdt.c | 64 ++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 62 insertions(+), 2 deletions(-)
diff --git a/arch/arm/cpu/armv7/omap5/fdt.c b/arch/arm/cpu/armv7/omap5/fdt.c index 0493cd1..da8d59b 100644 --- a/arch/arm/cpu/armv7/omap5/fdt.c +++ b/arch/arm/cpu/armv7/omap5/fdt.c @@ -153,13 +153,73 @@ static int ft_hs_fixup_sram(void *fdt, bd_t *bd) static int ft_hs_fixup_sram(void *fdt, bd_t *bd) { return 0; } #endif
+#if (CONFIG_TI_SECURE_EMIF_TOTAL_REGION_SIZE != 0) +static int ft_hs_fixup_dram(void *fdt, bd_t *bd) +{ + const char *path, *subpath; + int offs; + u32 sec_mem_start = CONFIG_TI_SECURE_EMIF_REGION_START; + u32 sec_mem_size = CONFIG_TI_SECURE_EMIF_TOTAL_REGION_SIZE; + fdt64_t temp[2]; + + /* If start address is zero, place at end of DRAM */ + if (0 == sec_mem_start) + sec_mem_start = + (CONFIG_SYS_SDRAM_BASE + + (omap_sdram_size() - sec_mem_size)); + + /* Delete any original secure_reserved node */ + path = "/reserved-memory/secure_reserved"; + offs = fdt_path_offset(fdt, path); + if (offs >= 0) + fdt_del_node(fdt, offs); + + /* Add new secure_reserved node */ + path = "/reserved-memory"; + offs = fdt_path_offset(fdt, path); + if (offs < 0) { + debug("Node %s not found\n", path); + path = "/"; + subpath = "reserved-memory"; + fdt_path_offset(fdt, path); + offs = fdt_add_subnode(fdt, offs, subpath); + if (offs < 0) { + printf("Could not create %s%s node.\n", path, subpath); + return 1; + } + path = "/reserved-memory"; + offs = fdt_path_offset(fdt, path); + } + + subpath = "secure_reserved"; + offs = fdt_add_subnode(fdt, offs, subpath); + if (offs < 0) { + printf("Could not create %s%s node.\n", path, subpath); + return 1; + } + + temp[0] = cpu_to_fdt64(((u64)sec_mem_start)); + temp[1] = cpu_to_fdt64(((u64)sec_mem_size)); + fdt_setprop_string(fdt, offs, "compatible", + "ti,dra7-secure-memory"); + fdt_setprop_string(fdt, offs, "status", "okay"); + fdt_setprop(fdt, offs, "no-map", NULL, 0); + fdt_setprop(fdt, offs, "reg", temp, sizeof(temp)); + + return 0; +} +#else +static int ft_hs_fixup_dram(void *fdt, bd_t *bd) { return 0; } +#endif + static void ft_hs_fixups(void *fdt, bd_t *bd) { /* Check we are running on an HS/EMU device type */ if (GP_DEVICE != get_device_type()) { if ((ft_hs_fixup_crossbar(fdt, bd) == 0) && (ft_hs_disable_rng(fdt, bd) == 0) && - (ft_hs_fixup_sram(fdt, bd) == 0)) + (ft_hs_fixup_sram(fdt, bd) == 0) && + (ft_hs_fixup_dram(fdt, bd) == 0)) return; } else { printf("ERROR: Incorrect device type (GP) detected!"); @@ -171,7 +231,7 @@ static void ft_hs_fixups(void *fdt, bd_t *bd) static void ft_hs_fixups(void *fdt, bd_t *bd) { } -#endif +#endif /* #ifdef CONFIG_TI_SECURE_DEVICE */
/* * Place for general cpu/SoC FDT fixups. Board specific
On Fri, Sep 02, 2016 at 12:40:24AM -0500, Daniel Allred wrote:
Adds a secure dram reservation fixup for secure devices, when a region in the emif has been set aside for secure world use. The size is defined by the CONFIG_TI_SECURE_EMIF_TOTAL_REGION_SIZE config option.
Signed-off-by: Daniel Allred d-allred@ti.com
Reviewed-by: Tom Rini trini@konsulko.com
On 09/02/2016 12:40 AM, Daniel Allred wrote:
Adds a secure dram reservation fixup for secure devices, when a region in the emif has been set aside for secure world use. The size is defined by the CONFIG_TI_SECURE_EMIF_TOTAL_REGION_SIZE config option.
Signed-off-by: Daniel Allred d-allred@ti.com
arch/arm/cpu/armv7/omap5/fdt.c | 64 ++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 62 insertions(+), 2 deletions(-)
diff --git a/arch/arm/cpu/armv7/omap5/fdt.c b/arch/arm/cpu/armv7/omap5/fdt.c index 0493cd1..da8d59b 100644 --- a/arch/arm/cpu/armv7/omap5/fdt.c +++ b/arch/arm/cpu/armv7/omap5/fdt.c @@ -153,13 +153,73 @@ static int ft_hs_fixup_sram(void *fdt, bd_t *bd) static int ft_hs_fixup_sram(void *fdt, bd_t *bd) { return 0; } #endif
+#if (CONFIG_TI_SECURE_EMIF_TOTAL_REGION_SIZE != 0) +static int ft_hs_fixup_dram(void *fdt, bd_t *bd) +{
- const char *path, *subpath;
- int offs;
- u32 sec_mem_start = CONFIG_TI_SECURE_EMIF_REGION_START;
- u32 sec_mem_size = CONFIG_TI_SECURE_EMIF_TOTAL_REGION_SIZE;
- fdt64_t temp[2];
- /* If start address is zero, place at end of DRAM */
- if (0 == sec_mem_start)
sec_mem_start =(CONFIG_SYS_SDRAM_BASE +(omap_sdram_size() - sec_mem_size));- /* Delete any original secure_reserved node */
- path = "/reserved-memory/secure_reserved";
- offs = fdt_path_offset(fdt, path);
- if (offs >= 0)
fdt_del_node(fdt, offs);- /* Add new secure_reserved node */
- path = "/reserved-memory";
- offs = fdt_path_offset(fdt, path);
- if (offs < 0) {
debug("Node %s not found\n", path);path = "/";subpath = "reserved-memory";fdt_path_offset(fdt, path);
Are we supposed to be doing something with the result of this call? Who sets 'offs' in this case?
offs = fdt_add_subnode(fdt, offs, subpath);if (offs < 0) {printf("Could not create %s%s node.\n", path, subpath);return 1;}path = "/reserved-memory";offs = fdt_path_offset(fdt, path);- }
- subpath = "secure_reserved";
- offs = fdt_add_subnode(fdt, offs, subpath);
- if (offs < 0) {
printf("Could not create %s%s node.\n", path, subpath);return 1;- }
- temp[0] = cpu_to_fdt64(((u64)sec_mem_start));
- temp[1] = cpu_to_fdt64(((u64)sec_mem_size));
- fdt_setprop_string(fdt, offs, "compatible",
"ti,dra7-secure-memory");- fdt_setprop_string(fdt, offs, "status", "okay");
- fdt_setprop(fdt, offs, "no-map", NULL, 0);
- fdt_setprop(fdt, offs, "reg", temp, sizeof(temp));
- return 0;
+} +#else +static int ft_hs_fixup_dram(void *fdt, bd_t *bd) { return 0; } +#endif
static void ft_hs_fixups(void *fdt, bd_t *bd) { /* Check we are running on an HS/EMU device type */ if (GP_DEVICE != get_device_type()) { if ((ft_hs_fixup_crossbar(fdt, bd) == 0) && (ft_hs_disable_rng(fdt, bd) == 0) &&
(ft_hs_fixup_sram(fdt, bd) == 0))
(ft_hs_fixup_sram(fdt, bd) == 0) &&(ft_hs_fixup_dram(fdt, bd) == 0)) return;@@ -171,7 +231,7 @@ static void ft_hs_fixups(void *fdt, bd_t *bd) static void ft_hs_fixups(void *fdt, bd_t *bd) { } -#endif +#endif /* #ifdef CONFIG_TI_SECURE_DEVICE */
/*
- Place for general cpu/SoC FDT fixups. Board specific
On Fri, Sep 02, 2016 at 12:40:24AM -0500, Daniel Allred wrote:
Adds a secure dram reservation fixup for secure devices, when a region in the emif has been set aside for secure world use. The size is defined by the CONFIG_TI_SECURE_EMIF_TOTAL_REGION_SIZE config option.
Signed-off-by: Daniel Allred d-allred@ti.com Reviewed-by: Tom Rini trini@konsulko.com
Applied to u-boot/master, thanks!
participants (4)
-
Allred, Daniel -
Andrew F. Davis -
Daniel Allred
-
Tom Rini