Boot mem handling #7039

jenswi-linaro · 2024-09-13T11:25:19Z

This pull request handles previously unused memory during boot—currently mostly temporary memory allocations, but that will be extended in the next pull requests.

The commit "core: mm: allocate temporary memory map array" demonstrates this by using dynamic allocation instead of hard-coded allocation of static_mmap_regions[].

jforissier

For "mk/clang.mk: -Wno-gnu-alignof-expression":

s/dereferensing/dereferencing/
s/CLANG/Clang/

Reviewed-by: Jerome Forissier <[email protected]>

jforissier

For "core: arm64: increase thread stack size for debug":
Reviewed-by: Jerome Forissier <[email protected]>

jforissier

For "core: mm: add vaddr_to_phys()":
Reviewed-by: Jerome Forissier <[email protected]>

jforissier

For "core: remove CORE_MEM_TA_RAM":
Reviewed-by: Jerome Forissier <[email protected]>

jenswi-linaro · 2024-09-15T16:37:27Z

OK to squash in the fixes and move up the last commits a bit?

jforissier · 2024-09-16T06:59:57Z

OK to squash in the fixes and move up the last commits a bit?

Fine with me

jenswi-linaro · 2024-09-16T08:35:01Z

Tags applied, squashed in fixes, and moved the last added commits a bit earlier to fix problems before they occur.

jforissier

For "core: add VCORE_FREE_{PA,SZ,END_PA}":
s/CVORE/VCORE/
s/paractise/practice/
Acked-by: Jerome Forissier <[email protected]>

jforissier

For "core: mm: allow unmapping VCORE_FREE":
Reviewed-by: Jerome Forissier <[email protected]>

jforissier

Comment on "core: arm,pager: make __vcore_init_ro_start follow __vcore_init_rx_end".

jforissier · 2024-09-17T12:30:06Z

core/arch/arm/kernel/kern.ld.S

+ASSERT(__vcore_init_ro_start == __vcore_init_ro_end,
+       "__vcore_init_ro_start should follow __vcore_init_ro_end")


s/__vcore_init_ro_end/__vcore_init_rx_end/? And in the commit description "after __vcore_init_rx_end"?

You're right, I'll update the commit.

jenswi-linaro · 2024-09-18T08:04:30Z

Force push to update "core: arm,pager: make __vcore_init_ro_start follow __vcore_init_rx_end" and applied the tags.

etienne-lms

Very minor review comments.

I was looking at the CFG_BOOT_MEM part and experienced the patches on my Armv7 boards. Commit "core: arm: enable CFG_BOOT_MEM unconditionally" makes my platforms to segfault at primary core init (only when pager is disabled). I dumped the traces below, in case it helps:

I/TC: Early console on UART#4
D/TC:0   add_phys_mem:750 VCORE_UNPG_RX_PA type TEE_RAM_RX 0xde000000 size 0x00076000
D/TC:0   add_phys_mem:750 VCORE_UNPG_RW_PA type TEE_RAM_RW 0xde076000 size 0x0005a000
D/TC:0   add_phys_mem:750 VCORE_UNPG_RW_PA type SEC_RAM_OVERALL 0xde076000 size 0x0005a000
D/TC:0   add_phys_mem:750 VCORE_FREE_PA type TEE_RAM_RW 0xde0d0000 size 0x00130000
D/TC:0   merge_mmaps:703 Merging 0xde076000..0xde0cffff and 0xde0d0000..0xde1fffff
D/TC:0   add_phys_mem:750 VCORE_FREE_PA type SEC_RAM_OVERALL 0xde0d0000 size 0x00130000
D/TC:0   merge_mmaps:703 Merging 0xde076000..0xde0cffff and 0xde0d0000..0xde1fffff
D/TC:0   add_phys_mem:750 next_pa type SEC_RAM_OVERALL 0xde200000 size 0x01e00000
D/TC:0   merge_mmaps:703 Merging 0xde076000..0xde1fffff and 0xde200000..0xdfffffff
D/TC:0   add_phys_mem:750 GIC_BASE type IO_SEC 0xa0000000 size 0x00200000
...
D/TC:0   add_phys_mem:750 APB1_BASE type IO_NSEC 0x40000000 size 0x00200000
D/TC:0   add_va_space:794 type RES_VASPACE size 0x00a00000
D/TC:0   add_va_space:794 type SHM_VASPACE size 0x02000000
D/TC:0   dump_mmap_table:920 type SEC_RAM_OVERALL va 0xd76ec000..0xd9675fff pa 0xde076000..0xdfffffff size 0x01f8a000 (pgdir)
D/TC:0   dump_mmap_table:920 type IO_SEC       va 0xd9800000..0xd99fffff pa 0xa0000000..0xa01fffff size 0x00200000 (pgdir)
...
D/TC:0   dump_mmap_table:920 type IO_NSEC      va 0xdb000000..0xdb1fffff pa 0x40000000..0x401fffff size 0x00200000 (pgdir)
D/TC:0   dump_mmap_table:920 type RES_VASPACE  va 0xdb200000..0xdbbfffff pa 0x00000000..0x009fffff size 0x00a00000 (pgdir)
D/TC:0   dump_mmap_table:920 type SHM_VASPACE  va 0xdbe00000..0xdddfffff pa 0x00000000..0x01ffffff size 0x02000000 (pgdir)
D/TC:0   dump_mmap_table:920 type TEE_RAM_RX   va 0xde000000..0xde075fff pa 0xde000000..0xde075fff size 0x00076000 (smallpg)
D/TC:0   dump_mmap_table:920 type TEE_RAM_RW   va 0xde076000..0xde1fffff pa 0xde076000..0xde1fffff size 0x0018a000 (smallpg)
D/TC:0   core_mmu_xlat_table_alloc:527 xlat tables used 1 / 4
D/TC:0   core_mmu_xlat_table_alloc:527 xlat tables used 2 / 4
D/TC:0   core_mmu_xlat_table_alloc:527 xlat tables used 3 / 4
D/TC:0   carve_out_core_mem:2654 0xde000000 .. 0xde200000
D/TC:0   boot_mem_release_unused:152 Allocated 0 bytes at va 0xde0d0000 pa 0xde0d0000
D/TC:0   boot_mem_release_unused:156 Tempalloc 4380 bytes at va 0xde1feee4
D/TC:0   boot_mem_release_unused:178 Carving out 0xde000000..0xde0cffff
D/TC:0   boot_mem_release_unused:187 Releasing 1236992 bytes from va 0xde0d0000
I/TC: 
I/TC: Embedded DTB found
I/TC: OP-TEE version: 4.3.0-141-g0e3fb1edc (gcc version 11.3.1 20220712 (Arm GNU Toolchain 11.3.Rel1)) #36 Thu Sep 19 07:43:17 UTC 2024 arm
...
I/TC: Primary CPU initializing
...
D/TC:0 0 boot_mem_release_tmp_alloc:227 Releasing 8192 bytes from va 0xde1fe000
...
I/TC: Primary CPU switching to normal world boot
E/TC:0 0 
E/TC:0 0 Core data-abort at address 0xde0d0000 (translation fault)
E/TC:0 0  fsr 0x00002a07  ttbr0 0xde0ba000  ttbr1 0x00000000  cidr 0x0
E/TC:0 0  cpu #0          cpsr 0x800001d3
E/TC:0 0  r0 0xde0d0000      r4 0x2fff2d84    r8 0xde078828   r12 0x00000000
E/TC:0 0  r1 0xde1ff018      r5 0x00000000    r9 0xde0cf5c0    sp 0xde0cf5c0
E/TC:0 0  r2 0x00000040      r6 0xc0400000   r10 0xafaeadac    lr 0xde00021c
E/TC:0 0  r3 0x0000003f      r7 0x00000000   r11 0x00000000    pc 0xde000d24
E/TC:0 0 TEE load address @ 0xde000000
E/TC:0 0 Call stack:
E/TC:0 0  0xde000d24 dcache_cleaninv_range at core/arch/arm/kernel/cache_helpers_a32.S:53
E/TC:0 0  0xde00021c clear_bss at core/arch/arm/kernel/entry_a32.S:588

The data-abort occurs in reset_primary before switch to non-secure world, fluhsing vmem that is not mapped:
core/arch/arm/kernel/entry_a32.S:

	/*
	 * In case we've touched memory that secondary CPUs will use before
	 * they have turned on their D-cache, clean and invalidate the
	 * D-cache before exiting to normal world.
	 */
588:	flush_cache_vrange(cached_mem_start, cached_mem_end)

Obviously not reproduced on vexpress-qemu_virt.

etienne-lms · 2024-09-18T13:27:43Z

core/mm/core_mmu.c

+	 */
+	if (IS_ENABLED(CFG_NS_VIRTUALIZATION)) {
+		paddr_t b1 = 0;
+		paddr_size_t s1 = 0;



Commit "core: merge core_mmu_init_phys_mem() and core_mmu_init_virtualization()":

Intentionally removed the info level trace IMSG("Initializing virtualization support"); ?
(previously printed from core/arch/arm/kernel/boot.c)

inline comment above ... TA are loaded/executedNsec ... is pasted from previous implementation but is is quite buggy. I think it's time to fix it.

Commit "core: merge core_mmu_init_phys_mem() and core_mmu_init_virtualization()":

Intentionally removed the info level trace IMSG("Initializing virtualization support"); ?
(previously printed from core/arch/arm/kernel/boot.c)

Yes, I saw no point in keeping it.

inline comment above ... TA are loaded/executedNsec ... is pasted from previous implementation but is is quite buggy. I think it's time to fix it.

Right, I'll remove it.

Commit "core: merge core_mmu_init_phys_mem() and core_mmu_init_virtualization()":

Intentionally removed the info level trace IMSG("Initializing virtualization support"); ?
(previously printed from core/arch/arm/kernel/boot.c)

Yes, I saw no point in keeping it.

Do we have another way of knowing that the OP-TEE binary was compiled with virtualization support? Linux driver traces perhaps?

I've added "core: arm: add CFG_NS_VIRTUALIZATION boot log" to take care of that.

etienne-lms · 2024-09-18T13:39:18Z

core/arch/arm/arm.mk

@@ -15,6 +15,10 @@ include mk/cc-option.mk
 # Only applicable when paging is enabled.
 CFG_CORE_TZSRAM_EMUL_SIZE ?= 458752

+# Mandatory for arch/arm to avoid ifdefs in the arch specific code
+CFG_BOOT_MEM=y


remove this line

This one is needed or we'll get an error:

core/arch/arm/arm.mk:19: *** CFG_BOOT_MEM is set to 'n' (from file) but its value must be 'y'. Stop.

This is due to the default value assigned in mk/config.mk

I think we should rather have in mk/config.mk something like:

ifeq ($(ARCH),arm) $(call force,CFG_BOOT_MEM,y) else CFG_BOOT_MEM ?= n endif

etienne-lms · 2024-09-19T07:28:23Z

core/mm/core_mmu.c

+#ifdef TEE_SDP_TEST_MEM_BASE
+		if (TEE_SDP_TEST_MEM_SIZE) {
+			pa = vaddr_to_phys(TEE_SDP_TEST_MEM_BASE);
+			carve_out_core_mem(pa, pa + TEE_SDP_TEST_MEM_BASE);


s/TEE_SDP_TEST_MEM_BASE/TEE_SDP_TEST_MEM_SIZE/

etienne-lms · 2024-09-19T07:29:31Z

core/mm/core_mmu.c

+{
+	tee_mm_entry_t *mm __maybe_unused = NULL;
+
+	DMSG("%#"PRIxPA" .. %#"PRIxPASZ, pa, end_pa);


s/PRIxPASZ/PRIxPA/

etienne-lms · 2024-09-19T14:20:12Z

To sumup, the cache flush operation at entry_a32.S:588 above flushes a VA range (TEE RAM) that includes VA areas not mapped (released boot-mem areas IIUC), they trigger a data-abort exception in the CPU. Testing a bit, it seems Qemu (at least the tool/config we use) does not trap such accesses as illegals.

jenswi-linaro · 2024-09-20T08:42:34Z

To sumup, the cache flush operation at entry_a32.S:588 above flushes a VA range (TEE RAM) that includes VA areas not mapped (released boot-mem areas IIUC), they trigger a data-abort exception in the CPU. Testing a bit, it seems Qemu (at least the tool/config we use) does not trap such accesses as illegals.

Thanks for the detailed analysis, I hope the update fixes the problem.

jenswi-linaro · 2024-09-20T08:56:02Z

Update, comments addressed.

jforissier · 2024-09-20T14:26:16Z

For "core: arm: add CFG_NS_VIRTUALIZATION boot log":

Reviewed-by: Jerome Forissier <[email protected]>

etienne-lms · 2024-09-23T06:38:25Z

core/arch/arm/kernel/boot.c

@@ -934,7 +936,15 @@ static void init_primary(unsigned long pageable_part, unsigned long nsec_entry)

 	core_mmu_save_mem_map();
 	core_mmu_init_phys_mem();
-	boot_mem_release_unused();
+	va = boot_mem_release_unused();
+	if (IS_ENABLED(CFG_ARM32_core) && !IS_ENABLED(CFG_WITH_PAGER)) {


Sorry, my comment was maybe ambiguous. Don't you expect the same kind of issues on 64 bit platforms?
I've tested on my 64bi platform and indeed the same issue arises: data abort on the data cache flush at core/arch/arm/kernel/entry_a64.S:432 on the VA range unmapped from boot_mem.c.

You're right, I've pushed an update for that.

etienne-lms · 2024-09-23T06:43:54Z

core/arch/arm/kernel/boot.c

@@ -540,17 +536,17 @@ static void init_runtime(unsigned long pageable_part)
 	asan_memcpy_unchecked(hashes, tmp_hashes, hash_size);

 	/*
-	 * Need physical memory pool initialized to be able to allocate
-	 * secure physical memory below.
+	 * The pager is about the be enabled below, evenual temporary boot


s/evenual/eventual/ ?

Thanks, I'll fix.

etienne-lms · 2024-09-23T06:52:31Z

core/include/kernel/linker.h

+					  __vcore_free_start))
+#define VCORE_FREE_END_PA	((unsigned long)__vcore_free_end)
+#else
+#define VCORE_FREE_PA		PADDR_MAX


Add an explicit inline comment like /* Pager consumes all core physical memory: no free area */?

Sure, I'll add a comment.

jenswi-linaro · 2024-09-23T08:04:16Z

Update. Let me know if I should squash in the updates.

etienne-lms · 2024-09-23T13:24:24Z

I'm fine you squash the fixup commits.

jenswi-linaro · 2024-09-23T13:42:06Z

Squashed in the updates.

etienne-lms

Some comments on commit "core: mm: replace MEM_AREA_TA_RAM".

etienne-lms · 2024-09-26T09:23:54Z

core/arch/arm/kernel/boot.c

-	case MEM_AREA_NEX_RAM_RW:
-	case MEM_AREA_TEE_ASAN:
-	case MEM_AREA_TA_RAM:
+	case MEM_AREA_SEC_RAM_OVERALL:


Asan memory is still referenced with MEM_AREA_TEE_ASAN memory area ID when CFG_WITH_PAGER is enabled. Won't this switch case need to be supported?

MEM_AREA_TEE_RAM is still used when CFG_CORE_RWDATA_NOEXEC is disabled. IS that an issue? Maybe it's a bit inconsistent to enable CFG_MEMTAG but not CFG_CORE_RWDATA_NOEXEC.

Is is OK here to no more clear memtags for MEM_AREA_TEE_RAM_RW and MEM_AREA_NEX_RAM_RW areas?

Asan memory is still referenced with MEM_AREA_TEE_ASAN memory area ID when CFG_WITH_PAGER is enabled. Won't this switch case need to be supported?

mk/config.mk:999: *** CFG_CORE_SANITIZE_KADDRESS and CFG_MEMTAG are not compatible. Stop.

I don't think we need to care about MEM_AREA_TEE_ASAN since it's not used with memory tagging enabled.

MEM_AREA_TEE_RAM is still used when CFG_CORE_RWDATA_NOEXEC is disabled. IS that an issue? Maybe it's a bit inconsistent to enable CFG_MEMTAG but not CFG_CORE_RWDATA_NOEXEC.

That shouldn't be a problem, everything should be covered by MEM_AREA_SEC_RAM_OVERALL nonetheless.

Is is OK here to no more clear memtags for MEM_AREA_TEE_RAM_RW and MEM_AREA_NEX_RAM_RW areas?

The same here, is covered by MEM_AREA_SEC_RAM_OVERALL.

However, while looking at logs etc to double-check I noticed:

D/TC:0 dump_mmap_table:923 type SEC_RAM_OVERALL va 0x12100000..0x122fffff pa 0x0e100000..0x0e2fffff size 0x00200000 (pgdir) ... D/TC:0 mmap_clear_memtag:392 Clearing tags for VA 0x12000000..0x121fffff

We may have this error in more than one place, see commit f01690c ("core: fix mapping init debug trace") for the dump_mmap_table() print. I'll investigate further.

etienne-lms · 2024-09-26T09:30:35Z

core/mm/core_mmu.c

@@ -66,7 +66,7 @@ static struct tee_mmap_region static_mmap_regions[CFG_MMAP_REGIONS
 #if defined(CFG_CORE_ASLR) || defined(CFG_CORE_PHYS_RELOCATABLE)
 						+ 1
 #endif
-						+ 2] __nex_bss;
+						+ 3] __nex_bss;


(commit "core: mm,pager: map remaining physical memory")
Since this is needed only when pager is enabled, I suggest an explicit case like:

#if defined(CFG_CORE_ASLR) || defined(CFG_CORE_PHYS_RELOCATABLE) + 1 #endif #ifdef CFG_WITH_PAGER /* Entry to cover the whole physical RAM */ + 1 #endif 2] __nex_bss;

etienne-lms · 2024-09-26T09:39:33Z

core/mm/core_mmu.c

+		 */
+		ADD_PHYS_MEM(MEM_AREA_TEE_RAM, ram_start,
+			     VCORE_UNPG_RX_PA - ram_start);
+		ADD_PHYS_MEM(MEM_AREA_SEC_RAM_OVERALL, ram_start,


This changes to mapping of the physical memory range from TEE_RAM_START to TEE_LOAD_ADDR, from read-only/tagged to read-write/tagged.
I see that some platforms (at least plat-ti with pager enabled) has a 4k page between TEE_RAM_START and TEE_LOAD_ADDR. Most platforms default config also allow to define a CFG_TEE_LOAD_ADDR value different from TEE_RAM_START.

Maybe to discuss with @glneo.

Would it make more sense only to map the gap between TEE_RAM_START and TEE_LOAD_ADDR in MEM_AREA_SEC_RAM_OVERALL?

I think it would, unless some platforms load a TEE core image made on core binary image prepend with some data expected to be read-only mapped. Could be strange but current implementation allows that.
Maybe this old scheme could be deprecated. In that case, at least the commit message should explicitly state it IMHO.

etienne-lms · 2024-09-26T10:19:02Z

core/mm/core_mmu.c

@@ -66,7 +66,7 @@ static struct tee_mmap_region static_mmap_regions[CFG_MMAP_REGIONS
 #if defined(CFG_CORE_ASLR) || defined(CFG_CORE_PHYS_RELOCATABLE)
 						+ 1
 #endif
-						+ 1] __nex_bss;
+						+ 2] __nex_bss;


This change adds 1, 3 or 4 cells in static_mmap_regions (depending on CFG_NS_VIRTUALIZATION and CFG_CORE_RWDATA_NOEXEC).
Should this be reflected here? Or maybe add 4 cells, unconditionally to cover all cases? Or use explicit config cases (below, not that very helpful I guess)?

static struct tee_mmap_region static_mmap_regions[CFG_MMAP_REGIONS #if defined(CFG_CORE_ASLR) || defined(CFG_CORE_PHYS_RELOCATABLE) + 1 #endif +#ifdef CFG_CORE_RWDATA_NOEXEC +#ifdef CFG_NS_VIRTUALIZATION + + 4 +#else + + 3 +#endif +#else + + 1 +#endif /*CFG_CORE_RWDATA_NOEXEC*/ + 1] __nex_bss;

Yeah, let's do that to keep things simple.

etienne-lms

Few review tags: Reviewed-by: Etienne Carriere <[email protected]> for commits:
"mk/clang.mk: -Wno-gnu-alignof-expression",
"core: arm64: increase thread stack size for debug",
"core: mm: add vaddr_to_phys()",
"core: remove CORE_MEM_TA_RAM",
"core: add VCORE_FREE_{PA,SZ,END_PA}",
"core: mm: allow unmapping VCORE_FREE",
"core: mm: unify secure core and TA memory",
"core: virt: phys_mem_core_alloc() use both pools",
"core: arm: core_mmu_v7.c: increase MAX_XLAT_TABLES by 2" and
"core: arm,pager: make __vcore_init_ro_start follow __vcore_init_rx_end"

A concern with commit "core: mm: map memory using requested block size".

etienne-lms · 2024-09-30T08:14:21Z

core/mm/core_mmu.c

 			panic("Impossible memory alignment");

 		if (map_is_tee_ram(mem_map->map + n))
 			mem_map->map[n].region_size = SMALL_PAGE_SIZE;
+		else
+			mem_map->map[n].region_size = CORE_MMU_PGDIR_SIZE;


Previous implementation allowed to page map cacheable memory outside TEE RAM, (MEM_AREA_RAM_NSEC, MEM_AREA_RAM_SEC, MEM_AREA_ROM_SEC). I fear memory firewall permission violation on speculative accesses for such memories, and likely few other corner issues on some platform specific memories.

This is the requested granularity. It will not mean mapping more memory than specified. So some ranges will be mapped by of mix of small-page and pgdir sized mappings (level 3 and level 2 with LPAE).

Take QEMU with hafnium as an example:

TEE_RAM_RWX va 0x0e304000..0x0e503fff pa 0x0e304000..0x0e503fff size 0x00200000 (smallpg) RES_VASPACE va 0x0e600000..0x0effffff pa 0x00000000..0x009fffff size 0x00a00000 (pgdir) SHM_VASPACE va 0x0f000000..0x10ffffff pa 0x00000000..0x01ffffff size 0x02000000 (pgdir) IO_SEC va 0x11000000..0x1a080fff pa 0x09000000..0x12080fff size 0x09081000 (pgdir) SEC_RAM_OVERALL va 0x1a300000..0x1affffff pa 0x0e300000..0x0effffff size 0x00d00000 (pgdir)

IO_SEC is then mapped as:

D/TC:0 [LVL2] VA:0x0011000000 PA:0x0009000000 DEV-RW-XN-S D/TC:0 [LVL2] VA:0x0011200000 PA:0x0009200000 DEV-RW-XN-S ... D/TC:0 [LVL2] VA:0x001a000000 TBL:0x000e3fa000 D/TC:0 [LVL3] VA:0x001a000000 PA:0x0012000000 DEV-RW-XN-S D/TC:0 [LVL3] VA:0x001a001000 PA:0x0012001000 DEV-RW-XN-S ... D/TC:0 [LVL3] VA:0x001a080000 PA:0x0012080000 DEV-RW-XN-S D/TC:0 [LVL3] VA:0x001a081000 INVALID

SEC_RAM_OVERALL as:

D/TC:0 [LVL3] VA:0x001a300000 PA:0x000e300000 MEM-RW-XN-S D/TC:0 [LVL3] VA:0x001a301000 PA:0x000e301000 MEM-RW-XN-S ... D/TC:0 [LVL3] VA:0x001a3ff000 PA:0x000e3ff000 MEM-RW-XN-S D/TC:0 [LVL2] VA:0x001a400000 PA:0x000e400000 MEM-RW-XN-S ... D/TC:0 [LVL2] VA:0x001ae00000 PA:0x000ee00000 MEM-RW-XN-S D/TC:0 [LVL2] VA:0x001b000000 INVALID

So pgdir-sized region_size uses as few translation tables as possible, but it will not result in mapping beyond the requested range.

jenswi-linaro · 2024-09-30T09:12:55Z

Can I squash in the updates?

etienne-lms · 2024-09-30T09:38:38Z

Can I squash in the updates?

Fine with me.

jenswi-linaro · 2024-09-30T10:15:37Z

Tags applied

etienne-lms · 2024-11-07T14:20:33Z

Fine with me. (sorry for the late feedback)

Add -Wno-gnu-alignof-expression to the warnings flag for Clang in order to avoid warnings like: '_Alignof' applied to an expression is a GNU extension [-Werror,-Wgnu-alignof-expression] when alignof() is applied on an expression like dereferencing a pointer to get the alignment of type. Signed-off-by: Jens Wiklander <[email protected]> Reviewed-by: Jerome Forissier <[email protected]> Reviewed-by: Etienne Carriere <[email protected]>

Increase STACK_THREAD_SIZE when CFG_CORE_DEBUG_CHECK_STACKS=y. Signed-off-by: Jens Wiklander <[email protected]> Reviewed-by: Jerome Forissier <[email protected]> Reviewed-by: Etienne Carriere <[email protected]>

Add a wrapper function for virt_to_phys() using vaddr_t instead of a void pointer. Signed-off-by: Jens Wiklander <[email protected]> Reviewed-by: Jerome Forissier <[email protected]> Reviewed-by: Etienne Carriere <[email protected]>

The buffer attribute CORE_MEM_TA_RAM isn't used to query the status of a buffer anywhere. So remove the attribute to allow future simplifications. Signed-off-by: Jens Wiklander <[email protected]> Reviewed-by: Jerome Forissier <[email protected]> Reviewed-by: Etienne Carriere <[email protected]>

Add VCORE_FREE_{PA,SZ,END_PA} defines to identify the unused and free memory range at the end of TEE_RAM_START..(TEE_RAM_START + TEE_RAM_VA_SIZE). VCORE_FREE_SZ is 0 in a pager configuration since all the memory is used by the pager. The VCORE_FREE range is excluded from the TEE_RAM_RW area for CFG_NS_VIRTUALIZATION=y and instead put in a separate NEX_RAM_RW area. This makes each partition use a bit less memory and leaves the VCORE_FREE range available for the Nexus. The VCORE_FREE range is added to the TEE_RAM_RW area for the normal configuration with CFG_NS_VIRTUALIZATION=n and CFG_WITH_PAGER=n. It's in practice unchanged behaviour in this configuration. Signed-off-by: Jens Wiklander <[email protected]> Acked-by: Jerome Forissier <[email protected]> Reviewed-by: Etienne Carriere <[email protected]>

Allow unmapping core memory in the VCORE_FREE range when the original boot mapping isn't needed any more. Signed-off-by: Jens Wiklander <[email protected]> Reviewed-by: Jerome Forissier <[email protected]> Reviewed-by: Etienne Carriere <[email protected]>

jenswi-linaro · 2024-11-11T10:58:50Z

Squashed and rebased.

jenswi-linaro · 2024-11-14T11:27:56Z

Reviews needed for:

"core: mm: unify secure core and TA memory"
"core: mm: map memory using requested block size"
"core: mm,pager: map remaining physical memory"
"core: add CFG_BOOT_MEM and boot_mem_*() functions"
"core: arm: add boot_cached_mem_end"
"core: arm: enable CFG_BOOT_MEM unconditionally"
"core: mm: allocate temporary memory map array"
"core: initialize guest physical memory early"
"core: merge core_mmu_init_phys_mem() and core_mmu_init_virtualization()"

The commit "core: mm: allocate temporary memory map array" demonstrates the ridding of the hardcoded size of the array static_mmap_regions. I plan to do the same for the translation tables, number of cores, number of threads, and the heap. Much of the groundwork is already done, but this PR is needed before I can do the rest.

jenswi-linaro · 2024-11-21T07:20:05Z

Ping?

etienne-lms · 2024-11-21T09:15:52Z

FYI, i'm looking at it. Still struggling to make my ind clear on commit "core: mm: replace MEM_AREA_TA_RAM". I'll try to give some feedback soon.

etienne-lms

A tiny bit of progress in the review...

For commit "core: mm: replace MEM_AREA_TA_RAM": 2 comments otherwise looks good to me. Acked-by: Etienne Carriere <[email protected]> with these 2 comments addressed or not (no effective functional changes).

Reviewed-by: Etienne Carriere <[email protected]> for commit
"core: mm: map memory using requested block size",
"core: mm,pager: map remaining physical memory" and
"core: arm: add boot_cached_mem_end" (with #ifdef issue fixed).

etienne-lms · 2024-11-26T16:30:47Z

core/mm/core_mmu.c

 			     VCORE_UNPG_RX_PA - ram_start);
+		assert(VCORE_UNPG_RX_PA >= ram_start);
+		tee_ram_initial_offs = VCORE_UNPG_RX_PA - ram_start;
+		DMSG("tee_ram_initial_offs %zx", tee_ram_initial_offs);


Add 0x prefix? (%#zx)

That makes sense.

etienne-lms · 2024-11-27T10:59:02Z

core/arch/arm/kernel/entry_a64.S

@@ -468,12 +470,9 @@ clear_nex_bss:
 END_FUNC _start
 DECLARE_KEEP_INIT _start

+#ifndef CFG_WITH_PAGER


Commit "core: arm: add boot_cached_mem_end": this change belongs to commit
"core: arm: enable CFG_BOOT_MEM unconditionally".

Thanks, I'll fix.

etienne-lms · 2024-11-29T08:38:48Z

core/mm/core_mmu.c

@@ -1096,41 +1109,51 @@ static void collect_mem_ranges(struct memory_map *mem_map)
 		if (IS_ENABLED(CFG_NS_VIRTUALIZATION)) {
 			ADD_PHYS_MEM(MEM_AREA_NEX_RAM_RO, VCORE_UNPG_RW_PA,
 				     VCORE_UNPG_RW_SZ);
+			ADD_PHYS_MEM(MEM_AREA_SEC_RAM_OVERALL, VCORE_UNPG_RW_PA,
+				     VCORE_UNPG_RW_SZ);


When CFG_WITH_PAGER=y, the mapping added here and at lines 1117 and 1128 add SEC_RAM_OVERALL mapped areas that will not be used and look somewhat strange in the static mapping description known that SEC_RAM_OVERALL is considered a less secure memory when pager is used. E.g. on qemu_v8a:

D/TC:0 dump_mmap_table:924 type IDENTITY_MAP_RX va 0x0e100000..0x0e101fff pa 0x0e100000..0x0e101fff size 0x00002000 (smallpg) D/TC:0 dump_mmap_table:924 type NSEC_SHM va 0x62600000..0x627fffff pa 0x42000000..0x421fffff size 0x00200000 (pgdir) D/TC:0 dump_mmap_table:924 type SEC_RAM_OVERALL va 0x62800000..0x635fffff pa 0x0e200000..0x0effffff size 0x00e00000 (pgdir) ---> D/TC:0 dump_mmap_table:924 type SEC_RAM_OVERALL va 0x63800000..0x63836fff pa 0x0e115000..0x0e14bfff size 0x00037000 (pgdir) D/TC:0 dump_mmap_table:924 type IO_SEC va 0x63a00000..0x64bfffff pa 0x08000000..0x091fffff size 0x01200000 (pgdir) D/TC:0 dump_mmap_table:924 type SHM_VASPACE va 0x64e00000..0x66dfffff pa 0x00000000..0x01ffffff size 0x02000000 (pgdir) D/TC:0 dump_mmap_table:924 type RES_VASPACE va 0x67000000..0x679fffff pa 0x00000000..0x009fffff size 0x00a00000 (pgdir) D/TC:0 dump_mmap_table:924 type TEE_RAM_RX va 0x67c23000..0x67c37fff pa 0x0e100000..0x0e114fff size 0x00015000 (smallpg) D/TC:0 dump_mmap_table:924 type TEE_RAM_RW va 0x67c38000..0x67c6efff pa 0x0e115000..0x0e14bfff size 0x00037000 (smallpg) D/TC:0 dump_mmap_table:924 type INIT_RAM_RX va 0x67c6f000..0x67c79fff pa 0x0e14c000..0x0e156fff size 0x0000b000 (smallpg) D/TC:0 dump_mmap_table:924 type TEE_RAM_RWX va 0x67c7a000..0x67ca2fff pa 0x0e157000..0x0e17ffff size 0x00029000 (smallpg) D/TC:0 dump_mmap_table:924 type PAGER_VASPACE va 0x67ca3000..0x67e22fff pa 0x00000000..0x0017ffff size 0x00180000 (smallpg)

I wonder if it would be worth to not apply them, e.g. with hacks like:

ADD_PHYS_MEM(MEM_AREA_NEX_RAM_RO, VCORE_UNPG_RW_PA, VCORE_UNPG_RW_SZ); + if (!IS_ENABLED(CFG_WITH_PAGER)) + ADD_PHYS_MEM(MEM_AREA_SEC_RAM_OVERALL, + VCORE_UNPG_RW_PA, + VCORE_UNPG_RW_SZ);

That said, it is not a strong opinion, can keep as is.

The idea is that MEM_AREA_SEC_RAM_OVERALL should cover all memory that doesn't have to be mapped RO or RX. It makes it easier when we know need to update data at a physical address, for instance when traversing and updating translation tables. All secure physical memory can be reached via a lookup with
phys_to_virt(pa, MEM_AREA_SEC_RAM_OVERALL, sz)

MEM_AREA_SEC_RAM_OVERALL isn't about the security level of the memory it's to have easy access via a present mapping. We should normally not save virtual pointers into MEM_AREA_SEC_RAM_OVERALL when it's an aliased mapping. We may need to make exceptions to that, but this is a starting point.

So I'd rather not complicate things more for the pager configuration.

Replace MEM_AREA_TA_RAM with MEM_AREA_SEC_RAM_OVERALL. All read/write secure memory is covered by MEM_AREA_SEC_RAM_OVERALL, sometimes using an aliased map. But secure read-only or execute core memory is not covered as that would defeat the purpose of CFG_CORE_RWDATA_NOEXEC. Since the partition TA memory isn't accessed via MEM_AREA_TA_RAM any longer, don't map it using the partition specific map. This is needed later where unification of OP-TEE core and physical TA memory is possible. Signed-off-by: Jens Wiklander <[email protected]> Acked-by: Etienne Carriere <[email protected]>

In configurations where secure core and TA memory is allocated from the same contiguous physical memory block, carve out the memory needed by OP-TEE core and make the rest available as TA memory. This is needed by later patches where more core memory is allocated as needed from the pool of TA memory. Signed-off-by: Jens Wiklander <[email protected]> Reviewed-by: Etienne Carriere <[email protected]>

With CFG_NS_VIRTUALIZATION=y let phys_mem_core_alloc() allocate from both the core_pool and ta_pool since both pools keep equally secure memory. This is needed in later patches when some translation tables are dynamically allocated from spare physical core memory. Signed-off-by: Jens Wiklander <[email protected]> Reviewed-by: Etienne Carriere <[email protected]>

Increase MAX_XLAT_TABLES by 2 to be able to map all TEE memory with 4k pages. Signed-off-by: Jens Wiklander <[email protected]> Reviewed-by: Etienne Carriere <[email protected]>

TEE memory is always supposed to be mapped with 4k pages for maximum flexibility, but can_map_at_level() doesn't check the requested block size for a region, so fix that. However, assign_mem_granularity() assigns smaller than necessary block sizes on page aligned regions, so fix that by only requesting 4k granularity for TEE memory and PGDIR granularity for the rest. This is needed in later patches where some TEE memory is unmapped. Signed-off-by: Jens Wiklander <[email protected]> Reviewed-by: Etienne Carriere <[email protected]>

This concerns configurations with CFG_WITH_PAGER=y. Until this patch, even if __vcore_init_ro_size (VCORE_INIT_RO_SZ) is 0 for CFG_CORE_RODATA_NOEXEC=n, __vcore_init_ro_start was using some value smaller than __vcore_init_rx_end. To simplify code trying to find the end of VCORE_INIT_RX and VCORE_INIT_RO parts of the binary, make sure that __vcore_init_ro_start follows right after __vcore_init_rx_end. Signed-off-by: Jens Wiklander <[email protected]> Reviewed-by: Etienne Carriere <[email protected]>

For CFG_WITH_PAGER=y map the remaining memory following the VCORE_INIT_RO memory to make sure that all physical TEE memory is mapped even if VCORE_INIT_RO doesn't cover it entirely. This will be used in later patches to use the temporarily unused memory while booting. Signed-off-by: Jens Wiklander <[email protected]> Reviewed-by: Etienne Carriere <[email protected]>

Adds CFG_BOOT_MEM to support stack-like memory allocations during boot before a heap has been configured. Signed-off-by: Jens Wiklander <[email protected]>

Add boot_cached_mem_end in C code, replacing the previous read-only mapped cached_mem_end. This allows updates to boot_cached_mem_end after MMU has been enabled. Signed-off-by: Jens Wiklander <[email protected]> Reviewed-by: Etienne Carriere <[email protected]>

Enable CFG_BOOT_MEM unconditionally and call the boot_mem_*() functions as needed from entry_*.S and boot.c. The pager will reuse all boot_mem memory internally when configured. The non-pager configuration will unmap the memory and make it available for TAs if needed. __FLATMAP_PAGER_TRAILING_SPACE is removed from the link script, collect_mem_ranges() in core/mm/core_mmu.c maps the memory following VCORE_INIT_RO automatically. Signed-off-by: Jens Wiklander <[email protected]>

With CFG_BOOT_MEM enabled, allocate a temporary memory map array using boot_mem_alloc_tmp() instead of using the global static_mmap_regions[]. core_mmu_save_mem_map() is added and called from boot_init_primary_late() before the temporary memory is reused. Signed-off-by: Jens Wiklander <[email protected]>

Initialize guest physical memory in virt_guest_created() before the first entry into the guest from normal world. This replaces the call to core_mmu_init_phys_mem() in init_tee_runtime(). Remove unused code in core_mmu_init_phys_mem() and the now unused functions core_mmu_get_ta_range() and virt_get_ta_ram(). Signed-off-by: Jens Wiklander <[email protected]>

Moves the implementation of core_mmu_init_virtualization() into core_mmu_init_phys_mem(). This simplifies init_primary() in core/arch/arm/kernel/boot.c. Signed-off-by: Jens Wiklander <[email protected]>

Add a log entry when CFG_NS_VIRTUALIZATION is enabled, for example: D/TC:0 0 boot_init_primary_late:1028 NS-Virtualization enabled, supporting 2 guests Signed-off-by: Jens Wiklander <[email protected]> Reviewed-by: Jerome Forissier <[email protected]>

jenswi-linaro · 2024-11-29T12:27:19Z

Comments addressed (except for the MEM_AREA_SEC_RAM_OVERALL comment) and tags applied.

jenswi-linaro force-pushed the boot-mem branch from beeb6a6 to e09cd0a Compare September 13, 2024 11:44

jforissier reviewed Sep 13, 2024

View reviewed changes

jenswi-linaro force-pushed the boot-mem branch from f7c663b to 893dbc0 Compare September 16, 2024 08:33

jforissier reviewed Sep 17, 2024

View reviewed changes

jenswi-linaro force-pushed the boot-mem branch from 893dbc0 to 0e3fb1e Compare September 18, 2024 08:04

etienne-lms reviewed Sep 19, 2024

View reviewed changes

etienne-lms reviewed Sep 23, 2024

View reviewed changes

jenswi-linaro force-pushed the boot-mem branch from 13036b5 to 45c5964 Compare September 23, 2024 13:41

etienne-lms reviewed Sep 26, 2024

View reviewed changes

etienne-lms reviewed Sep 30, 2024

View reviewed changes

jenswi-linaro force-pushed the boot-mem branch from 45c5964 to 5cc4816 Compare September 30, 2024 10:15

jenswi-linaro removed the Stale label Oct 31, 2024

jenswi-linaro added 6 commits November 11, 2024 10:27

core: arm64: increase thread stack size for debug

1e2e6a1

Increase STACK_THREAD_SIZE when CFG_CORE_DEBUG_CHECK_STACKS=y. Signed-off-by: Jens Wiklander <[email protected]> Reviewed-by: Jerome Forissier <[email protected]> Reviewed-by: Etienne Carriere <[email protected]>

core: mm: add vaddr_to_phys()

dcb1c82

Add a wrapper function for virt_to_phys() using vaddr_t instead of a void pointer. Signed-off-by: Jens Wiklander <[email protected]> Reviewed-by: Jerome Forissier <[email protected]> Reviewed-by: Etienne Carriere <[email protected]>

jenswi-linaro force-pushed the boot-mem branch from a6a21d1 to 05c3bad Compare November 11, 2024 10:57

etienne-lms reviewed Nov 29, 2024

View reviewed changes

jenswi-linaro added 14 commits November 29, 2024 13:22

core: arm: core_mmu_v7.c: increase MAX_XLAT_TABLES by 2

a01e6bf

Increase MAX_XLAT_TABLES by 2 to be able to map all TEE memory with 4k pages. Signed-off-by: Jens Wiklander <[email protected]> Reviewed-by: Etienne Carriere <[email protected]>

core: add CFG_BOOT_MEM and boot_mem_*() functions

88d3a66

Adds CFG_BOOT_MEM to support stack-like memory allocations during boot before a heap has been configured. Signed-off-by: Jens Wiklander <[email protected]>

core: merge core_mmu_init_phys_mem() and core_mmu_init_virtualization()

c3e137b

Moves the implementation of core_mmu_init_virtualization() into core_mmu_init_phys_mem(). This simplifies init_primary() in core/arch/arm/kernel/boot.c. Signed-off-by: Jens Wiklander <[email protected]>

jenswi-linaro force-pushed the boot-mem branch from 05c3bad to c3aa82d Compare November 29, 2024 12:26

		ASSERT(__vcore_init_ro_start == __vcore_init_ro_end,
		"__vcore_init_ro_start should follow __vcore_init_ro_end")

Boot mem handling #7039

Are you sure you want to change the base?

Boot mem handling #7039

Conversation

jenswi-linaro commented Sep 13, 2024

jforissier left a comment

Choose a reason for hiding this comment

jforissier left a comment

Choose a reason for hiding this comment

jforissier left a comment

Choose a reason for hiding this comment

jforissier left a comment

Choose a reason for hiding this comment

jenswi-linaro commented Sep 15, 2024

jforissier commented Sep 16, 2024

jenswi-linaro commented Sep 16, 2024

jforissier left a comment

Choose a reason for hiding this comment

jforissier left a comment

Choose a reason for hiding this comment

jforissier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenswi-linaro commented Sep 18, 2024

etienne-lms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etienne-lms commented Sep 19, 2024

jenswi-linaro commented Sep 20, 2024

jenswi-linaro commented Sep 20, 2024

jforissier commented Sep 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenswi-linaro commented Sep 23, 2024

etienne-lms commented Sep 23, 2024

jenswi-linaro commented Sep 23, 2024

etienne-lms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etienne-lms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenswi-linaro commented Sep 30, 2024

etienne-lms commented Sep 30, 2024

jenswi-linaro commented Sep 30, 2024

etienne-lms commented Nov 7, 2024

jenswi-linaro commented Nov 11, 2024

jenswi-linaro commented Nov 14, 2024

jenswi-linaro commented Nov 21, 2024

etienne-lms commented Nov 21, 2024

etienne-lms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment