mem: workaround - remove MEM_REG_ATTR_SHARED_HEAP from SOF by marcinszkudlinski · Pull Request #10144 · thesofproject/sof

marcinszkudlinski · 2025-07-30T08:44:46Z

This PR does 2 things:

removes MEM_REG_ATTR_SHARED_HEAP, add function stubs and - to allow merging changes to zephyr
forces buffers location in the very first memory region. This is required because of conflict of virtual addresses with loadable modules in case the platform has 5 cores

marcinszkudlinski · 2025-07-30T11:20:32Z

Internal CI was green before update

I needed to add a commit allowing SOF to work without and with zephyr changes from zephyrproject-rtos/zephyr#93334

This PR forces buffers location in the very first memory region, preventing from conflict of virtual addresses with loadable modules in case of 5 cores platforms Signed-off-by: Marcin Szkudlinski <marcin.szkudlinski@intel.com>

marcinszkudlinski · 2025-07-30T11:25:25Z

...and incorrect commit description fix

Please review and merge ASAP

Copilot

Pull Request Overview

This PR implements a workaround to resolve virtual address conflicts with loadable modules on 5-core platforms by removing dependency on MEM_REG_ATTR_SHARED_HEAP and forcing buffer allocation in the first memory region.

Removes the search for MEM_REG_ATTR_SHARED_HEAP regions and uses the first available virtual memory region instead
Adds weak function stubs for future Zephyr integration compatibility
Introduces a new configuration option for virtual heap region size

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File	Description
zephyr/lib/regions_mm.c	Replaces heap region search logic with direct assignment to first virtual memory region
zephyr/lib/alloc.c	Adds weak stubs and virtual memory region initialization logic
zephyr/include/sof/lib/regions_mm.h	Defines new attribute constant for shared heap regions
zephyr/Kconfig	Adds configuration option for virtual heap region size

Copilot · 2025-07-30T11:26:19Z

 #include <sof/audio/pipeline.h>
 #include <sof/audio/component_ext.h>
 #include <sof/trace/trace.h>
+#include <zephyr/drivers/mm/mm_drv_intel_adsp_mtl_tlb.h>


Including a platform-specific driver header in generic allocation code reduces portability. Consider moving this include to a platform-specific file or using a more generic interface.
Suggested change
#include<zephyr/drivers/mm/mm_drv_intel_adsp_mtl_tlb.h>
// Platform-specific header moved to a dedicated file for Intel ADSP MTL-specific logic.

Copilot · 2025-07-30T11:26:19Z

 	},
 };

+/* WA Stubs begin


The abbreviation 'WA' should be spelled out as 'Workaround' for clarity in the comment.
Suggested change
/*WAStubsbegin
/*WorkaroundStubsbegin

Copilot · 2025-07-30T11:26:19Z

+{
+	return 0;
+}
+/* WA Stubs end */


The abbreviation 'WA' should be spelled out as 'Workaround' for clarity in the comment.
Suggested change
/* WA Stubs end */
/* Workaround Stubs end */

Copilot · 2025-07-30T11:26:19Z

+
+__weak
+uintptr_t adsp_mm_get_unused_l2_start_aligned(void)
+{
+	return 0;
+}
+


The weak stub function lacks documentation explaining its purpose, expected behavior when the real implementation is available, and return value meaning.
Suggested change
__weak
uintptr_tadsp_mm_get_unused_l2_start_aligned(void)
{
return0;
}
/**
* @briefWeakstubforretrievingthestartaddressofunusedL2memory, aligned.
*
*Thisfunctionisaplaceholderandshouldbeoverriddenbyaplatform-specific
*implementation. ItisexpectedtoreturnthestartingaddressofunusedL2memory,
*alignedtotherequiredboundary.
*
* @returnuintptr_tThealignedstartaddressofunusedL2memory. Returns0if
*noimplementationisprovided.
*/
__weak
uintptr_tadsp_mm_get_unused_l2_start_aligned(void)
{
return0;
}
/**
* @briefWeakstubforaddingavirtualmemoryregion.
*
*Thisfunctionisaplaceholderandshouldbeoverriddenbyaplatform-specific
*implementation. Itisexpectedtoaddavirtualmemoryregionwiththespecified
*address, size, andattributes.
*
* @paramregion_addressThestartingaddressofthevirtualmemoryregion.
* @paramregion_sizeThesizeofthevirtualmemoryregioninbytes.
* @paramattrAttributesforthevirtualmemoryregion (e.g., accesspermissions).
*
* @returnintReturns0ifsuccessful, oranerrorcodeiftheoperationfails.
*/

Copilot · 2025-07-30T11:26:20Z

+{
+	return 0;
+}
+


The weak stub function lacks documentation explaining its parameters, return values, and expected behavior when the real implementation becomes available.
Suggested change
/**
* @briefAddavirtualmemoryregiontothesystem.
*
*Thisweakstubfunctionisaplaceholderforaddingavirtualmemoryregion.
*Itisexpectedtobeoverriddenbyarealimplementationinthefuture.
*
* @paramregion_addressThestartingaddressofthememoryregion.
* @paramregion_sizeThesizeofthememoryregioninbytes.
* @paramattrAttributesforthememoryregion (e.g., accesspermissions).
*
* @return0onsuccess, oranerrorcodeindicatingfailure.
*/

This commit add creation of virtual memory region for heap Called functions are not yet present in Zephyr (Zephyr is creating regions by itself now), so the commit also contains weaked aliases of zephyr functions. Signed-off-by: Marcin Szkudlinski <marcin.szkudlinski@intel.com>

marcinszkudlinski · 2025-07-30T12:23:22Z

fix for compilation when CONFIG_VIRTUAL_HEAP=0 (TGL)

lyakh · 2025-07-30T14:22:05Z

@@ -49,10 +49,7 @@ struct vmh_heap {
 */
 struct vmh_heap *vmh_init_heap(const struct vmh_heap_config *cfg, bool allocating_continuously)


could you please explain - do you mean that VMH and LLEXT attempt to use the same addresses on PTL? Don't we have enough address space, cannot we just adjust CONFIG_LIBRARY_BASE_ADDRESS?

sure it would work too. Yet I also want to make the whole system more resistant to such problem in a future, and this commit is making 2 steps at the same time - 1) a workaround 2) removing circular dependency from Zephyr.
Both can be done by getting the 1st range from the table with no questions ask, and than - when Zephyr is ready - creating zones by call from SOF (instead of hard-coded zones in Zephyr as it is now) and checking if any mapping fits into dedicated zone.

@marcinszkudlinski well, I was just wondering - why create a somewhat complicated workaround to be later replaced with a proper solution, if we can fix the problem by just changing an address in platform configuration, while also still working on a proper solution

removing circular dependency from Zephyr.
Currently I cannot merge "proper solution" to Zephyr
https://github.com/zephyrproject-rtos/zephyr/actions/runs/16593737908/job/46935880948?pr=93334

Ack @marcinszkudlinski . So it would this is the way to go, this PR removes MEM_REG_ATTR_CORE_HEAP, Marcin can update SOF in Zephyr manifest, then do the change in Zephyr (zephyrproject-rtos/zephyr#93334), and then update Zephyr in SOF. Good to go @lyakh ? We have failures in quickbuild as long as we have this open.

marcinszkudlinski · 2025-07-31T11:04:00Z

can we proceed? This defect is blocking PTL CI

marcinszkudlinski force-pushed the virtual_heap_WA branch from 5820dfa to b20596bCompare July 30, 2025 11:19

mem: workaround - always use 1st virtual memory region
1a669d3
This PR forces buffers location in the very first memory region, preventing from conflict of virtual addresses with loadable modules in case of 5 cores platforms Signed-off-by: Marcin Szkudlinski <marcin.szkudlinski@intel.com>

marcinszkudlinski force-pushed the virtual_heap_WA branch from b20596b to 6dcc67cCompare July 30, 2025 11:24

marcinszkudlinski marked this pull request as ready for review July 30, 2025 11:25

CopilotAI review requested due to automatic review settings July 30, 2025 11:25

marcinszkudlinski requested review from abonislawski, dabekjakub, dbaluta, iuliana-prodan, kv2019i and lyakh as code owners July 30, 2025 11:25

CopilotAI reviewed Jul 30, 2025
View reviewed changes

serhiy-katsyuba-intel approved these changes Jul 30, 2025
View reviewed changes

marcinszkudlinski force-pushed the virtual_heap_WA branch from 6dcc67c to 189b654Compare July 30, 2025 12:20

kv2019i approved these changes Jul 30, 2025
View reviewed changes

lyakh reviewed Jul 30, 2025
View reviewed changes

lyakh approved these changes Jul 31, 2025
View reviewed changes

lgirdwood approved these changes Jul 31, 2025
View reviewed changes

lgirdwood merged commit 4f1980c into thesofproject:mainJul 31, 2025
36 of 45 checks passed

	#include<zephyr/drivers/mm/mm_drv_intel_adsp_mtl_tlb.h>
	// Platform-specific header moved to a dedicated file for Intel ADSP MTL-specific logic.

-__weak
-uintptr_tadsp_mm_get_unused_l2_start_aligned(void)
-{
-return0;
-}
+/**
+* @briefWeakstubforretrievingthestartaddressofunusedL2memory, aligned.
+*
+*Thisfunctionisaplaceholderandshouldbeoverriddenbyaplatform-specific
+*implementation. ItisexpectedtoreturnthestartingaddressofunusedL2memory,
+*alignedtotherequiredboundary.
+*
+* @returnuintptr_tThealignedstartaddressofunusedL2memory. Returns0if
+*noimplementationisprovided.
+*/
+__weak
+uintptr_tadsp_mm_get_unused_l2_start_aligned(void)
+{
+return0;
+}
+/**
+* @briefWeakstubforaddingavirtualmemoryregion.
+*
+*Thisfunctionisaplaceholderandshouldbeoverriddenbyaplatform-specific
+*implementation. Itisexpectedtoaddavirtualmemoryregionwiththespecified
+*address, size, andattributes.
+*
+* @paramregion_addressThestartingaddressofthevirtualmemoryregion.
+* @paramregion_sizeThesizeofthevirtualmemoryregioninbytes.
+* @paramattrAttributesforthevirtualmemoryregion (e.g., accesspermissions).
+*
+* @returnintReturns0ifsuccessful, oranerrorcodeiftheoperationfails.
+*/

+/**
+* @briefAddavirtualmemoryregiontothesystem.
+*
+*Thisweakstubfunctionisaplaceholderforaddingavirtualmemoryregion.
+*Itisexpectedtobeoverriddenbyarealimplementationinthefuture.
+*
+* @paramregion_addressThestartingaddressofthememoryregion.
+* @paramregion_sizeThesizeofthememoryregioninbytes.
+* @paramattrAttributesforthememoryregion (e.g., accesspermissions).
+*
+* @return0onsuccess, oranerrorcodeindicatingfailure.
+*/

		@@ -49,10 +49,7 @@ struct vmh_heap {
		*/
		struct vmh_heap vmh_init_heap(const struct vmh_heap_config cfg, bool allocating_continuously)

Uh oh!

Conversation

marcinszkudlinski commented Jul 30, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marcinszkudlinski commented Jul 30, 2025

Uh oh!

marcinszkudlinski commented Jul 30, 2025

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

CopilotAIJul 30, 2025

Choose a reason for hiding this comment

Uh oh!

CopilotAIJul 30, 2025

Choose a reason for hiding this comment

Uh oh!

CopilotAIJul 30, 2025

Choose a reason for hiding this comment

Uh oh!

CopilotAIJul 30, 2025

Choose a reason for hiding this comment

Uh oh!

CopilotAIJul 30, 2025

Choose a reason for hiding this comment

Uh oh!

marcinszkudlinski commented Jul 30, 2025

Uh oh!

lyakhJul 30, 2025

Choose a reason for hiding this comment

Uh oh!

marcinszkudlinskiJul 31, 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!

lyakhJul 31, 2025

Choose a reason for hiding this comment

Uh oh!

marcinszkudlinskiJul 31, 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!

kv2019iJul 31, 2025

Choose a reason for hiding this comment

Uh oh!

marcinszkudlinski commented Jul 31, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

marcinszkudlinski commented Jul 30, 2025•
edited
Loading

marcinszkudlinskiJul 31, 2025•
edited
Loading

marcinszkudlinskiJul 31, 2025•
edited
Loading