Dynamic allocation of mmu translation tables #7192

jenswi-linaro · 2024-12-20T12:01:51Z

No description provided.

jforissier

For "core: allow partially unmapped MEM_AREA_TEE_RAM_RW":
Acked-by: Jerome Forissier <[email protected]>
For "core: boot_mem: allow NULL pointers while relocating":
Reviewed-by: Jerome Forissier <[email protected]>

jforissier

For commit "core: boot_mem: keep track of padding", with the minor comment below addressed:
Acked-by: Jerome Forissier <[email protected]>

jforissier · 2024-12-20T17:12:01Z

core/mm/boot_mem.c

@@ -39,6 +51,7 @@ struct boot_mem_desc {
 	vaddr_t mem_start;
 	vaddr_t mem_end;
 	struct boot_mem_reloc *reloc;
+	struct boot_mem_padding *padding;


Add @padding to the documentation

I'll update

jforissier

For "core: arm: add boot mem paddings to the heap":
Acked-by: Jerome Forissier <[email protected]>

jforissier

For "core: mm: extend temporary dummy memory map":
Acked-by: Jerome Forissier <[email protected]>

jforissier · 2024-12-20T17:23:03Z

core/arch/arm/mm/core_mmu_lpae.c

+ *                entry1
+ *                entry2
+ *                entry3
+ * core0: table1: entry (maps only EL0)


Do you mean entry0 here? Or is it on purpose to differentiate from entry0 while entry1 etc. are the same in both tables?

It's a typo, it should be entry0. I'll update the comment.

jforissier · 2024-12-20T17:27:30Z

CFG_DYN_CONFIG=y should definitely be added to CI (both for v7 and LPAE).

jenswi-linaro · 2024-12-23T07:36:25Z

Unless paging is enabled, CFG_DYN_CONFIG=y is defaulted on all arm platforms.

jenswi-linaro · 2025-01-06T07:25:09Z

Ping?

etienne-lms

Reviewed-by: Etienne Carriere <[email protected]> for commits
"core: allow partially unmapped MEM_AREA_TEE_RAM_RW",
"core: boot_mem: allow NULL pointers while relocating",
"core: boot_mem: keep track of padding" (with a minor comment to address and a free suggestion),
"core: arm: add boot mem paddings to the heap" and
"core: mm: extend temporary dummy memory map".

I'm still looking at the remaining commits.

(edited)

etienne-lms · 2025-01-06T07:37:25Z

core/mm/boot_mem.c

+/*
+ * struct boot_mem_padding - unused memory between allocations
+ * @start: Start of padding
+ * @start: Length of padding


s/start/len/

etienne-lms · 2025-01-06T10:35:58Z

core/arch/arm/mm/core_mmu_v7.c

+			p = boot_mem_alloc(SMALL_PAGE_SIZE, SMALL_PAGE_SIZE);
+			if (prtn->last_l2_page)
+				assert(prtn->last_l2_page + SMALL_PAGE_SIZE ==
+				       p);


at this stage prtn->last_l2_page is always NULL.

Good catch, I'll remove the if and assert. This was a leftover from a previous version.

etienne-lms · 2025-01-06T10:38:09Z

core/arch/arm/mm/core_mmu_v7.c

+				assert(prtn->last_l2_page + SMALL_PAGE_SIZE ==
+				       p);
+			if (!prtn->tables_used)
+				boot_mem_add_reloc(&prtn->last_l2_page);


IIUC this allows only a single chunk of 4 32bit MMU page table references. Isn't it an issue?

It's OK. I'll add this comment:

/* * L2 tables are allocated four at a time as a 4k * page. The pointer prtn->last_l2_page keeps track * of that page until all the l2 tables has been * used. That pointer may need to be relocated when * the MMU is enabled so the address of the pointer * is recorded, but it must only be recorded once. * * The already used 4k pages are only referenced * via recorded physical addresses in the l1 table * so those pointers don't need to be updated for * relocation. */

Right, crystal clear. Thanks.
s/has been/have been/

Comment addressed.

etienne-lms · 2025-01-06T10:52:37Z

core/arch/arm/mm/core_mmu_v7.c

@@ -214,19 +217,23 @@ static l2_xlat_tbl_t main_mmu_l2_ttb[MAX_XLAT_TABLES]
 /* MMU L1 table for TAs, one for each thread */
 static ul1_xlat_tbl_t main_mmu_ul1_ttb[CFG_NUM_THREADS]
 		__aligned(UL1_ALIGNMENT) __section(".nozi.mmu.ul1");
+#endif

 struct mmu_partition {


Would you be ok to add an inline description as you've added in the LPAE implementation, something like:

/* * struct mmu_partition - core virtual memory tables * @l1_table: Level 1 translation tables base address * @l2_table: Level 2 translation tables base address (CFG_DYN_CONFIG=n) * @last_l2_page: Pre-allocated Level 2 table chunk (CFG_DYN_CONFIG=y) * @ul1_tables: Level 1 translation tab le for EL0 mapping * @tables_used: Number of level 2 tables already used */

Sure, thanks.

Comment addressed.

etienne-lms · 2025-01-07T15:12:28Z

core/mm/boot_mem.c

@@ -158,6 +200,33 @@ void *boot_mem_alloc_tmp(size_t len, size_t align)
 	return mem_alloc_tmp(boot_mem_desc, len, align);
 }

+void boot_mem_foreach_padding(bool (*func)(vaddr_t va, size_t len, void *ptr),


Maybe add a little helper inline description telling that this function call func() for each registered padding instance and unregisters (consumes) it when the func() return true.

jenswi-linaro · 2025-01-08T08:49:20Z

Update

jenswi-linaro · 2025-01-08T11:15:28Z

Update.

etienne-lms

Comments for commit "core: arm: mm: refactor LPAE translation table handling".

etienne-lms · 2025-01-08T10:15:49Z

core/arch/arm/mm/core_mmu_lpae.c

+ *
+ * TTBR is assigned a base translation table of NUM_BASE_LEVEL_ENTRIES
+ * entries.  NUM_BASE_LEVEL_ENTRIES is determined based on
+ * CFG_LPAE_ADDR_SPACE_BITS.  CFG_LPAE_ADDR_SPACE_BITS is by default 32


nitpicking: 2 space chars

etienne-lms · 2025-01-08T10:17:25Z

core/arch/arm/mm/core_mmu_lpae.c

+ * EL0 where EL1 is unmapped except for a minimal trampoline needed to
+ * restore EL1 mappings on exception from EL0.
+ *
+ * Each CPU core is assign a unique set of base translation tables as:


s/assign/assigned/

etienne-lms · 2025-01-08T11:03:43Z

core/arch/arm/mm/core_mmu_lpae.c

+{
+	size_t idx = 0;
+
+	idx = ((vaddr_t)tbl - (vaddr_t)prtn->xlat_tables) / XLAT_TABLE_SIZE;


Previous implementation used &tbl[user_va_idx] not tbl.

Yes, but I think it was incorrect. user_va_idx is typically 1 so that would give the address of the second entry in the translation table. That difference is lost with the division so it doesn't harm except that it's confusing.

etienne-lms · 2025-01-08T11:06:03Z

core/arch/arm/mm/core_mmu_lpae.c

+
+	idx = ((vaddr_t)tbl - (vaddr_t)prtn->xlat_tables) / XLAT_TABLE_SIZE;
+	if (idx > MAX_XLAT_TABLES)
+		panic();


The previous implementation tested idx index against the number of tables already consumed (prtn->xlat_table_used). It is intentional to allow up to MAX_XLAT_TABLES now?

Good catch, I'll restore the assert().

I'm having my doubts about this array of indices. With a hardcoded limit, it's easy to ensure that the index type is wide enough, but with a more dynamic approach, we may be better off with pointers. It's also slightly less complicated and that should be welcome in this file. If you have a VA space larger than 39 bits, you can afford 6-7 extra bytes per CPU (in addition to the extra 4k page needed). The decision can wait until we have a dynamic configuration of the number of cores.

jenswi-linaro · 2025-01-08T14:14:16Z

Update

etienne-lms

Reviewed-by: Etienne Carriere <[email protected]> for commit
"core: arm: mm: dynamic allocation of LPAE translation tables".

Commit "core: arm: mm: dynamic allocation of v7 translation tables" looks good to me once comments are addressed. Feel free to squash all fixes in your next P-R update.

etienne-lms · 2025-01-10T08:24:09Z

core/arch/arm/mm/core_mmu_v7.c

+
+		new_table = prtn->l2_tables[prtn->tables_used];
+		prtn->tables_used++;
+		DMSG("L2 table used: %d/%d", prtn->tables_used,


Nitpicking: typo in previous implementation, let's fix it?
"L2 table used: %"PRIu32"/%d"

etienne-lms · 2025-01-10T08:43:05Z

core/arch/arm/mm/core_mmu_v7.c

+				assert(prtn->last_l2_page + SMALL_PAGE_SIZE ==
+				       p);
+			if (!prtn->tables_used)
+				boot_mem_add_reloc(&prtn->last_l2_page);


Comment addressed.

etienne-lms · 2025-01-10T08:43:18Z

core/arch/arm/mm/core_mmu_v7.c

@@ -214,19 +217,23 @@ static l2_xlat_tbl_t main_mmu_l2_ttb[MAX_XLAT_TABLES]
 /* MMU L1 table for TAs, one for each thread */
 static ul1_xlat_tbl_t main_mmu_ul1_ttb[CFG_NUM_THREADS]
 		__aligned(UL1_ALIGNMENT) __section(".nozi.mmu.ul1");
+#endif

 struct mmu_partition {


Comment addressed.

etienne-lms

Reviewed-by: Etienne Carriere <[email protected]> for commit
"core: arm: mm: refactor LPAE translation table handling".

Add special checks in phys_to_virt_tee_ram() to see that a virtual address indeed is mapped before return the address if the memory area is MEM_AREA_TEE_RAM_RW since the VCORE_FREE may be unmapped. Signed-off-by: Jens Wiklander <[email protected]> Acked-by: Jerome Forissier <[email protected]> Reviewed-by: Etienne Carriere <[email protected]>

In boot_mem_relocate() when relocating registered pointers, allow the pointer to be NULL. NULL pointers are not relocated. Signed-off-by: Jens Wiklander <[email protected]> Reviewed-by: Jerome Forissier <[email protected]> Reviewed-by: Etienne Carriere <[email protected]>

When boot_mem_alloc() allocates memory up to alignment - 1 number of bytes may have be skipped to satisfy the required alignment of the returned pointer. If the skipped bytes, or padding, is large enough, it's recorded in a list of padding. The list of paddings can be processed and consumed with boot_mem_foreach_padding(). This allows sufficiently large paddings to be added to for instance the heap instead of being wasted. Signed-off-by: Jens Wiklander <[email protected]> Acked-by: Jerome Forissier <[email protected]> Reviewed-by: Etienne Carriere <[email protected]>

Add the paddings added due to requested alignment in boot mem allocations to the heap. Signed-off-by: Jens Wiklander <[email protected]> Acked-by: Jerome Forissier <[email protected]> Reviewed-by: Etienne Carriere <[email protected]>

core_init_mmu_map() uses a temporary dummy memory map for the virt_to_phys() and phys_to_virt() conversions to avoid asserting while setting up translation tables before the MMU is enabled. CFG_DYN_CONFIG will need a larger range of memory since translation tables might not be allocated from .nozi memory only. So for CFG_DYN_CONFIG extend of end of the unused memory range that the boot_mem_*() functions allocate memory from. Introduce CFG_DYN_CONFIG, enabled by default if CFG_BOOT_MEM is enabled and CFG_WITH_PAGER disabled. CFG_DYN_CONFIG conflicts with CFG_WITH_PAGER since the pager uses a different mechanism for memory allocation. Signed-off-by: Jens Wiklander <[email protected]> Acked-by: Jerome Forissier <[email protected]> Reviewed-by: Etienne Carriere <[email protected]>

Refactor translation table handling to use a more flexible layout of the translation tables in memory. Instead of relying on multidimensional array use helper functions to calculate the address of each translation table as needed. Preparing for future changes, no change in behaviour. Signed-off-by: Jens Wiklander <[email protected]> Reviewed-by: Etienne Carriere <[email protected]>

With CFG_DYN_CONFIG enabled allocate translation tables using the boot_mem_*() functions. Static allocation from global variables is still used with CFG_DYN_CONFIG disabled. Signed-off-by: Jens Wiklander <[email protected]> Reviewed-by: Etienne Carriere <[email protected]>

jenswi-linaro · 2025-01-10T09:11:14Z

Comments addressed, commits squashed, rebased, and tags applied.

jenswi-linaro · 2025-01-10T09:17:46Z

The PR touches very core parts and although it's stable I don't mind if it's merged after the release to minimize the risk of last-minute problems.

jforissier reviewed Dec 20, 2024

View reviewed changes

etienne-lms reviewed Jan 7, 2025

View reviewed changes

etienne-lms reviewed Jan 8, 2025

View reviewed changes

etienne-lms reviewed Jan 10, 2025

View reviewed changes

jenswi-linaro added 8 commits January 10, 2025 10:08

core: arm: add boot mem paddings to the heap

af4a896

Add the paddings added due to requested alignment in boot mem allocations to the heap. Signed-off-by: Jens Wiklander <[email protected]> Acked-by: Jerome Forissier <[email protected]> Reviewed-by: Etienne Carriere <[email protected]>

jenswi-linaro force-pushed the dyn_mmu branch from 12f11a8 to 489a140 Compare January 10, 2025 09:10

Dynamic allocation of mmu translation tables #7192

Are you sure you want to change the base?

Dynamic allocation of mmu translation tables #7192

Conversation

jenswi-linaro commented Dec 20, 2024

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

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

jenswi-linaro Dec 23, 2024 • edited Loading

Choose a reason for hiding this comment

jforissier commented Dec 20, 2024

jenswi-linaro commented Dec 23, 2024

jenswi-linaro commented Jan 6, 2025

etienne-lms left a comment • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenswi-linaro commented Jan 8, 2025

jenswi-linaro commented Jan 8, 2025

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

jenswi-linaro commented Jan 8, 2025

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

etienne-lms left a comment

Choose a reason for hiding this comment

jenswi-linaro commented Jan 10, 2025

jenswi-linaro commented Jan 10, 2025

jenswi-linaro Dec 23, 2024 •

edited

Loading

etienne-lms left a comment •

edited

Loading