llext: add support for PTL DRAM detached sections with MMU enabled #9893

abonislawski · 2025-03-12T17:22:32Z

Set MMU permissions for detached sections
+
Enable CONFIG_COLD_STORE_EXECUTE_DRAM on PTL

Required patches:

lgirdwood

Just one question if map would ever fail.

lgirdwood · 2025-03-12T17:30:27Z

src/library_manager/llext_manager.c

 	return sys_mm_drv_unmap_region(aligned_vma, ALIGN_UP(pre_pad_size + size, PAGE_SZ));
 }

+static void llext_manager_detached_map(void __sparse_cache *vma, size_t size, uint32_t flags)


Would this ever fail ? do we need to return an int ?

arch_mem_map returns void so it cant fail in a way we could handle, inside there is only one check for size but it will lead to panic anyway

if (size == 0) { LOG_ERR("Cannot map physical memory at 0x%08X: invalid " "zero size", (uint32_t)phys); k_panic(); }

lyakh · 2025-03-13T08:08:07Z

src/library_manager/llext_manager.c

+	/* Use cached virtual address */
+	uintptr_t va = POINTER_TO_UINT(sys_cache_cached_ptr_get(aligned_vma));
+
+	arch_mem_map(aligned_vma, va, ALIGN_UP(pre_pad_size + size, PAGE_SZ), flags);


In principle you don't have to page-align the size, arch_mem_map() does it already, but doesn't hurt too much

src/library_manager/llext_manager.c

lyakh · 2025-03-13T08:44:36Z

src/library_manager/llext_manager.c

+						  void __sparse_cache *vma,
+						  size_t size)
+{
+	unsigned int i;


let's at least place this whole function under #if CONFIG_MMU. And I'm also considering making this simpler and faster by reducing the generality and making it more restricted to our use-case: only handle 2 detached sections - executable .cold and data .coldrodata. I'd add two fields to struct lib_manager_module to identify those two sections, e.g. store their indices. Like in llext_manager_load_data_from_storage() check shdr.sh_flags to see if it's code or data and assign respective index. Then here just do

llext_get_section_info(ldr, ext, mctx->cold_idx, &shdr, &s_region, &s_offset); llext_get_region_info(ldr, ext, s_region, NULL, &region_addr, NULL); llext_manager_detached_unmap(...);

and similar for mctx->coldrodata_idx. Yes, you'd need to pass mctx to this function too

That was my plan at first, but then I realized we can simply check sections the same way we do at the beginning and make it much more generic: whatever we find, we will also find later to unmap.

added ifdef for MMU

abonislawski · 2025-03-17T15:49:28Z

Rebased (all required PRs have been merged).

lyakh · 2025-03-20T07:31:11Z

no CI PTL results... let's re-test

lyakh · 2025-03-20T07:31:19Z

SOFCI TEST

lyakh · 2025-03-20T07:33:34Z

src/library_manager/llext_manager.c

+		if (s_region != region)
+			continue;
+
+		/* unmap detached sections (will be outside requested VMA area) */


if you have to update this PR, better replace "unmap" with "restore default flags"

Lets do this incrementally - CI passing

Spoke too soon - I reloaded and can see a CI timeout on some tests on PTL. Will rerun to be sure.

@lgirdwood ~~those IPC timeouts on PTL are also present in "main," seem to be caused by xruns. So, unrelated to this PR, but have to be fixed~~
UPDATE: sorry, I might have been wrong, upon closer examination these failures do look different

It was unstable with many timeouts in different tests each run, with reverted 'ipc: move most functions to run from DRAM' its now stable in linux CI, lets firstly merge base PTL DRAM enablement and later stabilize IPC

@abonislawski ok, so you're referring to jenkins failures, not to your local or "Intel Internal" tests. One of the failing jenkins runs was https://sof-ci.01.org/sofpr/PR9893/build11697/devicetest/index.html. In it we see the firmware running into an error:

[00:00:02.029,105] <err> ipc: ipc_comp_free: ipc_comp_free(): comp id: 0x3 state is 5 cannot be freed [00:00:02.029,115] <err> ipc: ipc_pipeline_free: ipc_pipeline_free(): module free () failed [00:00:02.029,128] <err> ipc: ipc_cmd: ipc4: FW_GEN_MSG failed with err 12

So that should be debuggable?

Yes, only linux CI, not a problem for another one in both configurations. This specific fail with pass in most runs and I really dont want to debug all of this, we need DRAM for modules on platforms with DRAM functionality

Actually I think reverting the DRAM IPC commit only hides the problem. I'd propose a test: update to the Zephyr version as in the PR (use the first commit as is), disable LLEXT and enable cold storage on PTL and run that through the CI. I expect that to pass. Then enable modules but remove all __cold attributes from them - that should pass too. So I suspect only when you enable __cold code in LLEXT modules, then your flag updating code breaks it. So maybe we still have some page overlaps? You're updating flags for sections, that shouldn't be touched?

unfortunately this scenario with __cold removed in src/audio also failed

lgirdwood · 2025-03-20T09:57:38Z

SOFCI TEST

lgirdwood · 2025-03-20T14:50:08Z

No results for PTL - retest

lgirdwood · 2025-03-20T14:50:15Z

SOFCI TEST

abonislawski · 2025-03-23T21:37:16Z

SOFCI TEST

lyakh

This revert wasn't here when I was approving this PR IIRC. Let's check why it's needed now

This patch will set permissions for detached sections if MMU is enabled Signed-off-by: Adrian Bonislawski <adrian.bonislawski@intel.com>

Enable CONFIG_COLD_STORE_EXECUTE_DRAM for ACE 3.0 Signed-off-by: Adrian Bonislawski <adrian.bonislawski@intel.com>

This reverts commit 0e53393. Signed-off-by: Adrian Bonislawski <adrian.bonislawski@intel.com>

lyakh · 2025-03-25T09:25:23Z

let's keep this test result - it was a rare all green PTL test https://sof-ci.01.org/sofpr/PR9893/build11747/devicetest/index.html

abonislawski · 2025-03-25T11:12:56Z

SOFCI TEST

lgirdwood · 2025-03-25T13:55:15Z

Test results are good.

schedule to re-apply later

abonislawski requested review from dbaluta, iuliana-prodan, kv2019i, lbetlej, lgirdwood, lyakh, mmaka1 and plbossart as code owners March 12, 2025 17:22

lgirdwood reviewed Mar 12, 2025

View reviewed changes

lgirdwood approved these changes Mar 12, 2025

View reviewed changes

lyakh reviewed Mar 13, 2025

View reviewed changes

abonislawski force-pushed the ptl_dram_llext branch 2 times, most recently from d7f87aa to b0e601e Compare March 17, 2025 15:46

abonislawski force-pushed the ptl_dram_llext branch 3 times, most recently from 432d36e to d4fdfda Compare March 19, 2025 11:38

lgirdwood approved these changes Mar 19, 2025

View reviewed changes

lyakh approved these changes Mar 20, 2025

View reviewed changes

abonislawski force-pushed the ptl_dram_llext branch from d4fdfda to 2bbefab Compare March 24, 2025 07:18

abonislawski requested review from bardliao, marcinszkudlinski and pblaszko as code owners March 24, 2025 07:18

lyakh previously requested changes Mar 24, 2025

View reviewed changes

abonislawski added 3 commits March 25, 2025 10:15

llext_manager: map detached sections for mmu

4a3b768

This patch will set permissions for detached sections if MMU is enabled Signed-off-by: Adrian Bonislawski <adrian.bonislawski@intel.com>

boards: ace30: enable cold store DRAM execution

d5b61a0

Enable CONFIG_COLD_STORE_EXECUTE_DRAM for ACE 3.0 Signed-off-by: Adrian Bonislawski <adrian.bonislawski@intel.com>

Revert "ipc: move most functions to run from DRAM"

4c44e99

This reverts commit 0e53393. Signed-off-by: Adrian Bonislawski <adrian.bonislawski@intel.com>

abonislawski force-pushed the ptl_dram_llext branch from 2bbefab to 4c44e99 Compare March 25, 2025 09:15

lgirdwood approved these changes Mar 25, 2025

View reviewed changes

lgirdwood merged commit 2b4d0ea into thesofproject:main Mar 25, 2025
44 of 49 checks passed

llext: add support for PTL DRAM detached sections with MMU enabled #9893

llext: add support for PTL DRAM detached sections with MMU enabled #9893

Uh oh!

Conversation

abonislawski commented Mar 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abonislawski commented Mar 17, 2025

Uh oh!

lyakh commented Mar 20, 2025

Uh oh!

lyakh commented Mar 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lyakh Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abonislawski Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgirdwood commented Mar 20, 2025

Uh oh!

lgirdwood commented Mar 20, 2025

Uh oh!

lgirdwood commented Mar 20, 2025

Uh oh!

abonislawski commented Mar 23, 2025

Uh oh!

lyakh left a comment

Choose a reason for hiding this comment

Uh oh!

lyakh commented Mar 25, 2025

Uh oh!

abonislawski commented Mar 25, 2025

Uh oh!

lgirdwood commented Mar 25, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

abonislawski commented Mar 12, 2025 •

edited

Loading

lyakh Mar 21, 2025 •

edited

Loading

abonislawski Mar 25, 2025 •

edited

Loading