arm: cache: cp15: don't map reserved region with no-map property
Hi,
On STM32MP15x platform we can use OP-TEE, loaded in DDR in a region protected by a firewall. This region is reserved in device with "no-map" property.
But sometime the platform boot failed in U-boot on a Cortex A7 access to this region (depending of the binary and the issue can change with compiler version or with code alignment), then the firewall raise a error, for example:
E/TC:0 tzc_it_handler:19 TZC permission failure E/TC:0 dump_fail_filter:420 Permission violation on filter 0 E/TC:0 dump_fail_filter:425 Violation @0xde5c6bf0, non-secure privileged read, AXI ID 5c0 E/TC:0 Panic
After investigation, the forbidden access is a speculative request performed by the Cortex A7 because all the DDR is mapped as MEMORY with CACHEABLE property.
The issue is solved only when the region reserved by OP-TEE is no more mapped in U-Boot (mapped as DEVICE/NON-CACHEABLE wasn't enough) as it is already done in Linux kernel.
I think that can be a general issue for ARM architecture: the no-map tag in device should be respected by U-boot, so I propose a generic solution in arm/lib/cache-cp15.c:dram_bank_mmu_setup().
This RFC serie is composed by 7 patches - 1..4/7: preliminary steps to support flags in library in lmb (as it is done in memblock.c in Linux) - 5/7: unitary test on the added feature in lmb lib - 6/7: save the no-map flags in lmb when the device tree is parsed - 7/7: update the generic behavior for "no-map" region in arm/lib/cache-cp15.c::dram_bank_mmu_setup()
I can change this last patch if it is required by other ARM architecture; it is a weak function so I can avoid to map the region with "no-map" property in device tree only for STM32MP architecture (in arch/arm/mach-stm32mp/cpu.c).
See also [1] which handle same speculative access on armv8 for area with Executable attribute.
[1] http://patchwork.ozlabs.org/project/uboot/patch/20200903000106.5016-1-marek....
Regards Patrick
Patrick Delaunay (7): lmb: Add support of flags for no-map properties lmb: add lmb_is_reserved_flags lmb: remove lmb_region.size lmb: add lmb_dump_region() function test: lmb: add test for lmb_reserve_flags image-fdt: save no-map parameter of reserve-memory arm: cache: cp15: don't map the reserved region with no-map property
arch/arm/include/asm/system.h | 3 + arch/arm/lib/cache-cp15.c | 17 +++++- common/image-fdt.c | 23 +++++--- include/lmb.h | 22 +++++++- lib/lmb.c | 100 +++++++++++++++++++++++----------- test/lib/lmb.c | 89 ++++++++++++++++++++++++++++++ 6 files changed, 210 insertions(+), 44 deletions(-)
Add "flags" in lmp_property to save the "no-map" property of reserved region and a new function lmb_reserve_flags() to check this flag.
The default allocation use flags = LMB_NONE.
The adjacent reserved memory region are merged only when they have the same flags value.
This patch is partially based on flags support done in Linux kernel mm/memblock .c (previously lmb.c); it is why LMB_NOMAP = 0x4, it is aligned with MEMBLOCK_NOMAP value.
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com ---
include/lmb.h | 20 ++++++++++++++++++++ lib/lmb.c | 52 ++++++++++++++++++++++++++++++++++++++++++--------- 2 files changed, 63 insertions(+), 9 deletions(-)
diff --git a/include/lmb.h b/include/lmb.h index e9f19b16ea..1c73f4851b 100644 --- a/include/lmb.h +++ b/include/lmb.h @@ -14,9 +14,20 @@
#define MAX_LMB_REGIONS 8
+/** + * enum lmb_flags - definition of memory region attributes + * @LMB_NONE: no special request + * @LMB_NOMAP: don't add to mmu configuration + */ +enum lmb_flags { + LMB_NONE = 0x0, /* No special request */ + LMB_NOMAP = 0x4, /* don't add to mmu config */ +}; + struct lmb_property { phys_addr_t base; phys_size_t size; + enum lmb_flags flags; };
struct lmb_region { @@ -37,6 +48,8 @@ extern void lmb_init_and_reserve_range(struct lmb *lmb, phys_addr_t base, phys_size_t size, void *fdt_blob); extern long lmb_add(struct lmb *lmb, phys_addr_t base, phys_size_t size); extern long lmb_reserve(struct lmb *lmb, phys_addr_t base, phys_size_t size); +extern long lmb_reserve_flags(struct lmb *lmb, phys_addr_t base, + phys_size_t size, enum lmb_flags flags); extern phys_addr_t lmb_alloc(struct lmb *lmb, phys_size_t size, ulong align); extern phys_addr_t lmb_alloc_base(struct lmb *lmb, phys_size_t size, ulong align, phys_addr_t max_addr); @@ -60,6 +73,13 @@ lmb_size_bytes(struct lmb_region *type, unsigned long region_nr) void board_lmb_reserve(struct lmb *lmb); void arch_lmb_reserve(struct lmb *lmb);
+/* Low level functions */ + +static inline bool lmb_is_nomap(struct lmb_property *m) +{ + return !!(m->flags & LMB_NOMAP); +} + #endif /* __KERNEL__ */
#endif /* _LINUX_LMB_H */ diff --git a/lib/lmb.c b/lib/lmb.c index 75082f3559..e85e08635c 100644 --- a/lib/lmb.c +++ b/lib/lmb.c @@ -27,6 +27,8 @@ void lmb_dump_all_force(struct lmb *lmb) (unsigned long long)lmb->memory.region[i].base); printf(" .size = 0x%llx\n", (unsigned long long)lmb->memory.region[i].size); + printf(" .flags = 0x%x\n", + lmb->memory.region[i].flags); }
printf("\n reserved.cnt = 0x%lx\n", lmb->reserved.cnt); @@ -37,6 +39,8 @@ void lmb_dump_all_force(struct lmb *lmb) (unsigned long long)lmb->reserved.region[i].base); printf(" .size = 0x%llx\n", (unsigned long long)lmb->reserved.region[i].size); + printf(" .flags = 0x%x\n", + lmb->reserved.region[i].flags); } }
@@ -85,6 +89,7 @@ static void lmb_remove_region(struct lmb_region *rgn, unsigned long r) for (i = r; i < rgn->cnt - 1; i++) { rgn->region[i].base = rgn->region[i + 1].base; rgn->region[i].size = rgn->region[i + 1].size; + rgn->region[i].flags = rgn->region[i + 1].flags; } rgn->cnt--; } @@ -146,7 +151,8 @@ void lmb_init_and_reserve_range(struct lmb *lmb, phys_addr_t base, }
/* This routine called with relocation disabled. */ -static long lmb_add_region(struct lmb_region *rgn, phys_addr_t base, phys_size_t size) +static long lmb_add_region_flags(struct lmb_region *rgn, phys_addr_t base, + phys_size_t size, enum lmb_flags flags) { unsigned long coalesced = 0; long adjacent, i; @@ -154,6 +160,7 @@ static long lmb_add_region(struct lmb_region *rgn, phys_addr_t base, phys_size_t if (rgn->cnt == 0) { rgn->region[0].base = base; rgn->region[0].size = size; + rgn->region[0].flags = flags; rgn->cnt = 1; return 0; } @@ -162,18 +169,27 @@ static long lmb_add_region(struct lmb_region *rgn, phys_addr_t base, phys_size_t for (i = 0; i < rgn->cnt; i++) { phys_addr_t rgnbase = rgn->region[i].base; phys_size_t rgnsize = rgn->region[i].size; + phys_size_t rgnflags = rgn->region[i].flags;
- if ((rgnbase == base) && (rgnsize == size)) - /* Already have this region, so we're done */ - return 0; + if (rgnbase == base && rgnsize == size) { + if (flags == rgnflags) + /* Already have this region, so we're done */ + return 0; + else + return -1; /* regions with new flags */ + }
adjacent = lmb_addrs_adjacent(base, size, rgnbase, rgnsize); if (adjacent > 0) { + if (flags != rgnflags) + break; rgn->region[i].base -= size; rgn->region[i].size += size; coalesced++; break; } else if (adjacent < 0) { + if (flags != rgnflags) + break; rgn->region[i].size += size; coalesced++; break; @@ -184,8 +200,10 @@ static long lmb_add_region(struct lmb_region *rgn, phys_addr_t base, phys_size_t }
if ((i < rgn->cnt - 1) && lmb_regions_adjacent(rgn, i, i + 1)) { - lmb_coalesce_regions(rgn, i, i + 1); - coalesced++; + if (rgn->region[i].flags == rgn->region[i + 1].flags) { + lmb_coalesce_regions(rgn, i, i + 1); + coalesced++; + } }
if (coalesced) @@ -198,9 +216,11 @@ static long lmb_add_region(struct lmb_region *rgn, phys_addr_t base, phys_size_t if (base < rgn->region[i].base) { rgn->region[i + 1].base = rgn->region[i].base; rgn->region[i + 1].size = rgn->region[i].size; + rgn->region[i + 1].flags = rgn->region[i].flags; } else { rgn->region[i + 1].base = base; rgn->region[i + 1].size = size; + rgn->region[i + 1].flags = flags; break; } } @@ -208,6 +228,7 @@ static long lmb_add_region(struct lmb_region *rgn, phys_addr_t base, phys_size_t if (base < rgn->region[0].base) { rgn->region[0].base = base; rgn->region[0].size = size; + rgn->region[0].flags = flags; }
rgn->cnt++; @@ -215,6 +236,12 @@ static long lmb_add_region(struct lmb_region *rgn, phys_addr_t base, phys_size_t return 0; }
+static long lmb_add_region(struct lmb_region *rgn, phys_addr_t base, + phys_size_t size) +{ + return lmb_add_region_flags(rgn, base, size, LMB_NONE); +} + /* This routine may be called with relocation disabled. */ long lmb_add(struct lmb *lmb, phys_addr_t base, phys_size_t size) { @@ -269,14 +296,21 @@ long lmb_free(struct lmb *lmb, phys_addr_t base, phys_size_t size) * beginging of the hole and add the region after hole. */ rgn->region[i].size = base - rgn->region[i].base; - return lmb_add_region(rgn, end + 1, rgnend - end); + return lmb_add_region_flags(rgn, end + 1, rgnend - end, + rgn->region[i].flags); }
-long lmb_reserve(struct lmb *lmb, phys_addr_t base, phys_size_t size) +long lmb_reserve_flags(struct lmb *lmb, phys_addr_t base, phys_size_t size, + enum lmb_flags flags) { struct lmb_region *_rgn = &(lmb->reserved);
- return lmb_add_region(_rgn, base, size); + return lmb_add_region_flags(_rgn, base, size, flags); +} + +long lmb_reserve(struct lmb *lmb, phys_addr_t base, phys_size_t size) +{ + return lmb_reserve_flags(lmb, base, size, LMB_NONE); }
static long lmb_overlaps_region(struct lmb_region *rgn, phys_addr_t base,
Add a new function lmb_is_reserved_flags to check is a address is reserved with a specific flags.
This function can be used to check if an address had be reserved with no-map flags with:
lmb_is_reserved_flags(lmb, addr, LMB_NOMAP);
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com ---
include/lmb.h | 1 + lib/lmb.c | 10 ++++++++-- 2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/include/lmb.h b/include/lmb.h index 1c73f4851b..68e9cc007a 100644 --- a/include/lmb.h +++ b/include/lmb.h @@ -59,6 +59,7 @@ extern phys_addr_t lmb_alloc_addr(struct lmb *lmb, phys_addr_t base, phys_size_t size); extern phys_size_t lmb_get_free_size(struct lmb *lmb, phys_addr_t addr); extern int lmb_is_reserved(struct lmb *lmb, phys_addr_t addr); +extern int lmb_is_reserved_flags(struct lmb *lmb, phys_addr_t addr, int flags); extern long lmb_free(struct lmb *lmb, phys_addr_t base, phys_size_t size);
extern void lmb_dump_all(struct lmb *lmb); diff --git a/lib/lmb.c b/lib/lmb.c index e85e08635c..3d64521a32 100644 --- a/lib/lmb.c +++ b/lib/lmb.c @@ -445,7 +445,7 @@ phys_size_t lmb_get_free_size(struct lmb *lmb, phys_addr_t addr) return 0; }
-int lmb_is_reserved(struct lmb *lmb, phys_addr_t addr) +int lmb_is_reserved_flags(struct lmb *lmb, phys_addr_t addr, int flags) { int i;
@@ -453,11 +453,17 @@ int lmb_is_reserved(struct lmb *lmb, phys_addr_t addr) phys_addr_t upper = lmb->reserved.region[i].base + lmb->reserved.region[i].size - 1; if ((addr >= lmb->reserved.region[i].base) && (addr <= upper)) - return 1; + return !!((lmb->reserved.region[i].flags & flags) + == flags); } return 0; }
+int lmb_is_reserved(struct lmb *lmb, phys_addr_t addr) +{ + return lmb_is_reserved_flags(lmb, addr, LMB_NONE); +} + __weak void board_lmb_reserve(struct lmb *lmb) { /* please define platform specific board_lmb_reserve() */
Remove the unused field size of struct lmb_region as it is initialized to 0 and never used after in lmb library.
See Linux kernel commit 4734b594c6ca ("memblock: Remove memblock_type.size and add memblock.memory_size instead")
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com ---
include/lmb.h | 1 - lib/lmb.c | 6 ------ 2 files changed, 7 deletions(-)
diff --git a/include/lmb.h b/include/lmb.h index 68e9cc007a..9be422103c 100644 --- a/include/lmb.h +++ b/include/lmb.h @@ -32,7 +32,6 @@ struct lmb_property {
struct lmb_region { unsigned long cnt; - phys_size_t size; struct lmb_property region[MAX_LMB_REGIONS+1]; };
diff --git a/lib/lmb.c b/lib/lmb.c index 3d64521a32..eac0143501 100644 --- a/lib/lmb.c +++ b/lib/lmb.c @@ -20,8 +20,6 @@ void lmb_dump_all_force(struct lmb *lmb)
printf("lmb_dump_all:\n"); printf(" memory.cnt = 0x%lx\n", lmb->memory.cnt); - printf(" memory.size = 0x%llx\n", - (unsigned long long)lmb->memory.size); for (i = 0; i < lmb->memory.cnt; i++) { printf(" memory.reg[0x%lx].base = 0x%llx\n", i, (unsigned long long)lmb->memory.region[i].base); @@ -32,8 +30,6 @@ void lmb_dump_all_force(struct lmb *lmb) }
printf("\n reserved.cnt = 0x%lx\n", lmb->reserved.cnt); - printf(" reserved.size = 0x%llx\n", - (unsigned long long)lmb->reserved.size); for (i = 0; i < lmb->reserved.cnt; i++) { printf(" reserved.reg[0x%lx].base = 0x%llx\n", i, (unsigned long long)lmb->reserved.region[i].base); @@ -105,9 +101,7 @@ static void lmb_coalesce_regions(struct lmb_region *rgn, unsigned long r1, void lmb_init(struct lmb *lmb) { lmb->memory.cnt = 0; - lmb->memory.size = 0; lmb->reserved.cnt = 0; - lmb->reserved.size = 0; }
static void lmb_reserve_common(struct lmb *lmb, void *fdt_blob)
Add lmb_dump_region() function, to simplify lmb_dump_all_force(). This patch is based on Linux memblock dump function.
A example of bdinfo output is:
..... fdt_size = 0x000146a0 FB base = 0xfdd00000 lmb_dump_all: memory.cnt = 0x1 memory[0] [0xc0000000-0xffffffff], 0x40000000 bytes flags: 0 reserved.cnt = 0x6 reserved[0] [0x10000000-0x10045fff], 0x00046000 bytes flags: 4 reserved[1] [0x30000000-0x3003ffff], 0x00040000 bytes flags: 4 reserved[2] [0x38000000-0x3800ffff], 0x00010000 bytes flags: 4 reserved[3] [0xe8000000-0xefffffff], 0x08000000 bytes flags: 4 reserved[4] [0xfbaea344-0xfdffffff], 0x02515cbc bytes flags: 0 reserved[5] [0xfe000000-0xffffffff], 0x02000000 bytes flags: 4 arch_number = 0x00000000 TLB addr = 0xfdff0000 ....
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com ---
lib/lmb.c | 40 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-)
diff --git a/lib/lmb.c b/lib/lmb.c index eac0143501..793fe411f2 100644 --- a/lib/lmb.c +++ b/lib/lmb.c @@ -14,32 +14,32 @@
#define LMB_ALLOC_ANYWHERE 0
-void lmb_dump_all_force(struct lmb *lmb) +static void lmb_dump_region(struct lmb_region *rgn, char *name) { - unsigned long i; + unsigned long long base, size, end; + enum lmb_flags flags; + int i;
- printf("lmb_dump_all:\n"); - printf(" memory.cnt = 0x%lx\n", lmb->memory.cnt); - for (i = 0; i < lmb->memory.cnt; i++) { - printf(" memory.reg[0x%lx].base = 0x%llx\n", i, - (unsigned long long)lmb->memory.region[i].base); - printf(" .size = 0x%llx\n", - (unsigned long long)lmb->memory.region[i].size); - printf(" .flags = 0x%x\n", - lmb->memory.region[i].flags); - } + printf(" %s.cnt = 0x%lx\n", name, rgn->cnt);
- printf("\n reserved.cnt = 0x%lx\n", lmb->reserved.cnt); - for (i = 0; i < lmb->reserved.cnt; i++) { - printf(" reserved.reg[0x%lx].base = 0x%llx\n", i, - (unsigned long long)lmb->reserved.region[i].base); - printf(" .size = 0x%llx\n", - (unsigned long long)lmb->reserved.region[i].size); - printf(" .flags = 0x%x\n", - lmb->reserved.region[i].flags); + for (i = 0; i < rgn->cnt; i++) { + base = rgn->region[i].base; + size = rgn->region[i].size; + end = base + size - 1; + flags = rgn->region[i].flags; + + printf(" %s[%d]\t[0x%llx-0x%llx], 0x%08llx bytes flags: %x\n", + name, i, base, end, size, flags); } }
+void lmb_dump_all_force(struct lmb *lmb) +{ + printf("lmb_dump_all:\n"); + lmb_dump_region(&lmb->memory, "memory"); + lmb_dump_region(&lmb->reserved, "reserved"); +} + void lmb_dump_all(struct lmb *lmb) { #ifdef DEBUG
Add a test to check the management of reserved region with flags.
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com ---
test/lib/lmb.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 89 insertions(+)
diff --git a/test/lib/lmb.c b/test/lib/lmb.c index 644ee78758..d7bd826190 100644 --- a/test/lib/lmb.c +++ b/test/lib/lmb.c @@ -659,3 +659,92 @@ static int lib_test_lmb_get_free_size(struct unit_test_state *uts)
DM_TEST(lib_test_lmb_get_free_size, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT); + +static int lib_test_lmb_flags(struct unit_test_state *uts) +{ + const phys_addr_t ram = 0x40000000; + const phys_size_t ram_size = 0x20000000; + struct lmb lmb; + long ret; + + lmb_init(&lmb); + + ret = lmb_add(&lmb, ram, ram_size); + ut_asserteq(ret, 0); + + /* reserve, same flag */ + ret = lmb_reserve_flags(&lmb, 0x40010000, 0x10000, LMB_NOMAP); + ut_asserteq(ret, 0); + ASSERT_LMB(&lmb, ram, ram_size, 1, 0x40010000, 0x10000, + 0, 0, 0, 0); + + /* reserve again, same flag */ + ret = lmb_reserve_flags(&lmb, 0x40010000, 0x10000, LMB_NOMAP); + ut_asserteq(ret, 0); + ASSERT_LMB(&lmb, ram, ram_size, 1, 0x40010000, 0x10000, + 0, 0, 0, 0); + + /* reserve again, new flag */ + ret = lmb_reserve_flags(&lmb, 0x40010000, 0x10000, LMB_NONE); + ut_asserteq(ret, -1); + ASSERT_LMB(&lmb, ram, ram_size, 1, 0x40010000, 0x10000, + 0, 0, 0, 0); + + ut_asserteq(lmb_is_nomap(&lmb.reserved.region[0]), 1); + + /* merge after */ + ret = lmb_reserve_flags(&lmb, 0x40020000, 0x10000, LMB_NOMAP); + ut_asserteq(ret, 1); + ASSERT_LMB(&lmb, ram, ram_size, 1, 0x40010000, 0x20000, + 0, 0, 0, 0); + + /* merge before */ + ret = lmb_reserve_flags(&lmb, 0x40000000, 0x10000, LMB_NOMAP); + ut_asserteq(ret, 1); + ASSERT_LMB(&lmb, ram, ram_size, 1, 0x40000000, 0x30000, + 0, 0, 0, 0); + + ut_asserteq(lmb_is_nomap(&lmb.reserved.region[0]), 1); + + ret = lmb_reserve_flags(&lmb, 0x40030000, 0x10000, LMB_NONE); + ut_asserteq(ret, 0); + ASSERT_LMB(&lmb, ram, ram_size, 2, 0x40000000, 0x30000, + 0x40030000, 0x10000, 0, 0); + + ut_asserteq(lmb_is_nomap(&lmb.reserved.region[0]), 1); + ut_asserteq(lmb_is_nomap(&lmb.reserved.region[1]), 0); + + /* test that old API use LMB_NONE */ + ret = lmb_reserve(&lmb, 0x40040000, 0x10000); + ut_asserteq(ret, 1); + ASSERT_LMB(&lmb, ram, ram_size, 2, 0x40000000, 0x30000, + 0x40030000, 0x20000, 0, 0); + + ut_asserteq(lmb_is_nomap(&lmb.reserved.region[0]), 1); + ut_asserteq(lmb_is_nomap(&lmb.reserved.region[1]), 0); + + ret = lmb_reserve_flags(&lmb, 0x40070000, 0x10000, LMB_NOMAP); + ut_asserteq(ret, 0); + ASSERT_LMB(&lmb, ram, ram_size, 3, 0x40000000, 0x30000, + 0x40030000, 0x20000, 0x40070000, 0x10000); + + ret = lmb_reserve_flags(&lmb, 0x40050000, 0x10000, LMB_NOMAP); + ut_asserteq(ret, 0); + ASSERT_LMB(&lmb, ram, ram_size, 4, 0x40000000, 0x30000, + 0x40030000, 0x20000, 0x40050000, 0x10000); + + /* merge with 2 adjacent regions */ + ret = lmb_reserve_flags(&lmb, 0x40060000, 0x10000, LMB_NOMAP); + ut_asserteq(ret, 2); + ASSERT_LMB(&lmb, ram, ram_size, 3, 0x40000000, 0x30000, + 0x40030000, 0x20000, 0x40050000, 0x30000); + + ut_asserteq(lmb_is_nomap(&lmb.reserved.region[0]), 1); + ut_asserteq(lmb_is_nomap(&lmb.reserved.region[1]), 0); + ut_asserteq(lmb_is_nomap(&lmb.reserved.region[2]), 1); + + return 0; +} + +DM_TEST(lib_test_lmb_flags, + UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);
Save the no-map information present in reserve-memory node to allow correct handling when the MMU is configured in board to avoid speculative access.
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com ---
common/image-fdt.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-)
diff --git a/common/image-fdt.c b/common/image-fdt.c index f13eefb061..85346c9672 100644 --- a/common/image-fdt.c +++ b/common/image-fdt.c @@ -74,18 +74,20 @@ static const image_header_t *image_get_fdt(ulong fdt_addr) #endif
static void boot_fdt_reserve_region(struct lmb *lmb, uint64_t addr, - uint64_t size) + uint64_t size, enum lmb_flags flags) { long ret;
- ret = lmb_reserve(lmb, addr, size); + ret = lmb_reserve_flags(lmb, addr, size, flags); if (ret >= 0) { - debug(" reserving fdt memory region: addr=%llx size=%llx\n", - (unsigned long long)addr, (unsigned long long)size); + debug(" reserving fdt memory region: addr=%llx size=%llx flags=%x\n", + (unsigned long long)addr, + (unsigned long long)size, flags); } else { puts("ERROR: reserving fdt memory region failed "); - printf("(addr=%llx size=%llx)\n", - (unsigned long long)addr, (unsigned long long)size); + printf("(addr=%llx size=%llx flags=%x)\n", + (unsigned long long)addr, + (unsigned long long)size, flags); } }
@@ -105,6 +107,7 @@ void boot_fdt_add_mem_rsv_regions(struct lmb *lmb, void *fdt_blob) int i, total, ret; int nodeoffset, subnode; struct fdt_resource res; + enum lmb_flags flags;
if (fdt_check_header(fdt_blob) != 0) return; @@ -114,7 +117,7 @@ void boot_fdt_add_mem_rsv_regions(struct lmb *lmb, void *fdt_blob) for (i = 0; i < total; i++) { if (fdt_get_mem_rsv(fdt_blob, i, &addr, &size) != 0) continue; - boot_fdt_reserve_region(lmb, addr, size); + boot_fdt_reserve_region(lmb, addr, size, LMB_NONE); }
/* process reserved-memory */ @@ -126,9 +129,13 @@ void boot_fdt_add_mem_rsv_regions(struct lmb *lmb, void *fdt_blob) ret = fdt_get_resource(fdt_blob, subnode, "reg", 0, &res); if (!ret && fdtdec_get_is_enabled(fdt_blob, subnode)) { + flags = LMB_NONE; + if (fdtdec_get_bool(fdt_blob, subnode, + "no-map")) + flags = LMB_NOMAP; addr = res.start; size = res.end - res.start + 1; - boot_fdt_reserve_region(lmb, addr, size); + boot_fdt_reserve_region(lmb, addr, size, flags); }
subnode = fdt_next_subnode(fdt_blob, subnode);
No more map the reserved region with "no-map" property by marking the corresponding TLB entries with invalid entry (=0) to avoid speculative access.
This patch fixes an issue on STM32MP15x where predictive read access on secure DDR area are caught by OP-TEE.
Signed-off-by: Patrick Delaunay patrick.delaunay@st.com ---
arch/arm/include/asm/system.h | 3 +++ arch/arm/lib/cache-cp15.c | 17 +++++++++++++++-- 2 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/arch/arm/include/asm/system.h b/arch/arm/include/asm/system.h index ce552944b7..932f12af1c 100644 --- a/arch/arm/include/asm/system.h +++ b/arch/arm/include/asm/system.h @@ -458,6 +458,7 @@ static inline void set_dacr(unsigned int val)
/* options available for data cache on each page */ enum dcache_option { + INVALID_ENTRY = 0, DCACHE_OFF = TTB_SECT | TTB_SECT_MAIR(0) | TTB_SECT_XN_MASK, DCACHE_WRITETHROUGH = TTB_SECT | TTB_SECT_MAIR(1), DCACHE_WRITEBACK = TTB_SECT | TTB_SECT_MAIR(2), @@ -488,6 +489,7 @@ enum dcache_option { * 1 1 1 Outer/Inner Write-Back, Read-Allocate Write-Allocate */ enum dcache_option { + INVALID_ENTRY = 0, DCACHE_OFF = TTB_SECT_DOMAIN(0) | TTB_SECT_XN_MASK | TTB_SECT, DCACHE_WRITETHROUGH = DCACHE_OFF | TTB_SECT_C_MASK, DCACHE_WRITEBACK = DCACHE_WRITETHROUGH | TTB_SECT_B_MASK, @@ -497,6 +499,7 @@ enum dcache_option { #define TTB_SECT_AP (3 << 10) /* options available for data cache on each page */ enum dcache_option { + INVALID_ENTRY = 0, DCACHE_OFF = 0x12, DCACHE_WRITETHROUGH = 0x1a, DCACHE_WRITEBACK = 0x1e, diff --git a/arch/arm/lib/cache-cp15.c b/arch/arm/lib/cache-cp15.c index abd81d21c7..b3ec8d2513 100644 --- a/arch/arm/lib/cache-cp15.c +++ b/arch/arm/lib/cache-cp15.c @@ -6,6 +6,7 @@
#include <common.h> #include <cpu_func.h> +#include <lmb.h> #include <log.h> #include <asm/system.h> #include <asm/cache.h> @@ -105,18 +106,30 @@ void mmu_set_region_dcache_behaviour(phys_addr_t start, size_t size, __weak void dram_bank_mmu_setup(int bank) { struct bd_info *bd = gd->bd; + struct lmb lmb; int i;
/* bd->bi_dram is available only after relocation */ if ((gd->flags & GD_FLG_RELOC) == 0) return;
+ /* + * don't allow cache on reserved memory tagged 'no-map' in DT + * => avoid speculative access to "secure" data + */ + lmb_init_and_reserve(&lmb, bd, (void *)gd->fdt_blob); + debug("%s: bank: %d\n", __func__, bank); for (i = bd->bi_dram[bank].start >> MMU_SECTION_SHIFT; i < (bd->bi_dram[bank].start >> MMU_SECTION_SHIFT) + (bd->bi_dram[bank].size >> MMU_SECTION_SHIFT); - i++) - set_section_dcache(i, DCACHE_DEFAULT_OPTION); + i++) { + if (lmb_is_reserved_flags(&lmb, i << MMU_SECTION_SHIFT, + LMB_NOMAP)) + set_section_dcache(i, INVALID_ENTRY); + else + set_section_dcache(i, DCACHE_DEFAULT_OPTION); + } }
/* to activate the MMU we need to set up virtual memory: use 1M areas */
On Fri, 4 Sep 2020 at 13:51, Patrick Delaunay patrick.delaunay@st.com wrote:
arm: cache: cp15: don't map reserved region with no-map property
Hi,
On STM32MP15x platform we can use OP-TEE, loaded in DDR in a region protected by a firewall. This region is reserved in device with "no-map" property.
But sometime the platform boot failed in U-boot on a Cortex A7 access to this region (depending of the binary and the issue can change with compiler version or with code alignment), then the firewall raise a error, for example:
E/TC:0 tzc_it_handler:19 TZC permission failure E/TC:0 dump_fail_filter:420 Permission violation on filter 0 E/TC:0 dump_fail_filter:425 Violation @0xde5c6bf0, non-secure privileged read, AXI ID 5c0 E/TC:0 Panic
After investigation, the forbidden access is a speculative request performed by the Cortex A7 because all the DDR is mapped as MEMORY with CACHEABLE property.
The issue is solved only when the region reserved by OP-TEE is no more mapped in U-Boot (mapped as DEVICE/NON-CACHEABLE wasn't enough) as it is already done in Linux kernel.
The only speculative accesses to such regions permitted by the architecture are instruction fetches, which is why setting the nx attribute is required on v7 for device mappings.
Does uboot currently honour this requirement?
I think that can be a general issue for ARM architecture: the no-map tag in device should be respected by U-boot, so I propose a generic solution in arm/lib/cache-cp15.c:dram_bank_mmu_setup().
This RFC serie is composed by 7 patches
- 1..4/7: preliminary steps to support flags in library in lmb (as it is done in memblock.c in Linux)
- 5/7: unitary test on the added feature in lmb lib
- 6/7: save the no-map flags in lmb when the device tree is parsed
- 7/7: update the generic behavior for "no-map" region in arm/lib/cache-cp15.c::dram_bank_mmu_setup()
I can change this last patch if it is required by other ARM architecture; it is a weak function so I can avoid to map the region with "no-map" property in device tree only for STM32MP architecture (in arch/arm/mach-stm32mp/cpu.c).
See also [1] which handle same speculative access on armv8 for area with Executable attribute.
[1] http://patchwork.ozlabs.org/project/uboot/patch/20200903000106.5016-1-marek....
Regards Patrick
Patrick Delaunay (7): lmb: Add support of flags for no-map properties lmb: add lmb_is_reserved_flags lmb: remove lmb_region.size lmb: add lmb_dump_region() function test: lmb: add test for lmb_reserve_flags image-fdt: save no-map parameter of reserve-memory arm: cache: cp15: don't map the reserved region with no-map property
arch/arm/include/asm/system.h | 3 + arch/arm/lib/cache-cp15.c | 17 +++++- common/image-fdt.c | 23 +++++--- include/lmb.h | 22 +++++++- lib/lmb.c | 100 +++++++++++++++++++++++----------- test/lib/lmb.c | 89 ++++++++++++++++++++++++++++++ 6 files changed, 210 insertions(+), 44 deletions(-)
-- 2.17.1
On Fri, 4 Sep 2020 at 14:25, Ard Biesheuvel ardb@kernel.org wrote:
On Fri, 4 Sep 2020 at 13:51, Patrick Delaunay patrick.delaunay@st.com wrote:
arm: cache: cp15: don't map reserved region with no-map property
Hi,
On STM32MP15x platform we can use OP-TEE, loaded in DDR in a region protected by a firewall. This region is reserved in device with "no-map" property.
But sometime the platform boot failed in U-boot on a Cortex A7 access to this region (depending of the binary and the issue can change with compiler version or with code alignment), then the firewall raise a error, for example:
E/TC:0 tzc_it_handler:19 TZC permission failure E/TC:0 dump_fail_filter:420 Permission violation on filter 0 E/TC:0 dump_fail_filter:425 Violation @0xde5c6bf0, non-secure privileged read, AXI ID 5c0 E/TC:0 Panic
After investigation, the forbidden access is a speculative request performed by the Cortex A7 because all the DDR is mapped as MEMORY with CACHEABLE property.
The issue is solved only when the region reserved by OP-TEE is no more mapped in U-Boot (mapped as DEVICE/NON-CACHEABLE wasn't enough) as it is already done in Linux kernel.
The only speculative accesses to such regions permitted by the architecture are instruction fetches, which is why setting the nx attribute is required on v7 for device mappings.
Does uboot currently honour this requirement?
I think current U-Boot honours the speculative side by using a IO memory default mapping strategy. If the reserved memory is used by a driver with a specific mapping, u-boot cannot ensure the mapping won't conflict with default mapping. For example OP-TEE defines a portion of this memory as non-secure cached shared memory. With Arm, we cannot map an area both as cached memory and as IO memory (Device/StronglyOrdered). So here, using a not-mapped strategy for "no-map" property looks better.
Regards, Etienne
I think that can be a general issue for ARM architecture: the no-map tag in device should be respected by U-boot, so I propose a generic solution in arm/lib/cache-cp15.c:dram_bank_mmu_setup().
This RFC serie is composed by 7 patches
- 1..4/7: preliminary steps to support flags in library in lmb (as it is done in memblock.c in Linux)
- 5/7: unitary test on the added feature in lmb lib
- 6/7: save the no-map flags in lmb when the device tree is parsed
- 7/7: update the generic behavior for "no-map" region in arm/lib/cache-cp15.c::dram_bank_mmu_setup()
I can change this last patch if it is required by other ARM architecture; it is a weak function so I can avoid to map the region with "no-map" property in device tree only for STM32MP architecture (in arch/arm/mach-stm32mp/cpu.c).
See also [1] which handle same speculative access on armv8 for area with Executable attribute.
[1] http://patchwork.ozlabs.org/project/uboot/patch/20200903000106.5016-1-marek....
Regards Patrick
Patrick Delaunay (7): lmb: Add support of flags for no-map properties lmb: add lmb_is_reserved_flags lmb: remove lmb_region.size lmb: add lmb_dump_region() function test: lmb: add test for lmb_reserve_flags image-fdt: save no-map parameter of reserve-memory arm: cache: cp15: don't map the reserved region with no-map property
arch/arm/include/asm/system.h | 3 + arch/arm/lib/cache-cp15.c | 17 +++++- common/image-fdt.c | 23 +++++--- include/lmb.h | 22 +++++++- lib/lmb.c | 100 +++++++++++++++++++++++----------- test/lib/lmb.c | 89 ++++++++++++++++++++++++++++++ 6 files changed, 210 insertions(+), 44 deletions(-)
-- 2.17.1
participants (3)
-
Ard Biesheuvel -
Etienne Carriere -
Patrick Delaunay