-
Notifications
You must be signed in to change notification settings - Fork 349
Move virtual region creation from Zephyr to SOF, add virtual memory regions protection #10124
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces virtual memory regions for the SOF (Sound Open Firmware) project, specifically adding support for two types of virtual regions: shared heap regions and loadable library regions. The changes establish a new attribute-based system for categorizing memory regions and implement initialization code for both virtual heap and loadable library memory regions.
Key changes:
- Introduces new virtual region attribute constants to replace hardcoded values
- Adds virtual memory region initialization for both shared heap and loadable libraries
- Updates board configurations to support multiple virtual regions
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| zephyr/include/sof/lib/regions_mm.h | Defines new virtual region attribute constants |
| zephyr/lib/regions_mm.c | Updates region matching to use new attribute constant |
| zephyr/lib/alloc.c | Adds virtual memory region initialization for shared heap |
| src/library_manager/llext_manager_dram.c | Implements virtual region initialization for loadable libraries |
| zephyr/Kconfig | Adds configuration for virtual heap region size |
| src/library_manager/Kconfig | Adds configuration for library region size |
| app/boards/*.conf | Updates board configurations to support 2 virtual regions |
Comments suppressed due to low confidence (1)
src/library_manager/Kconfig
Outdated
| should be set by this option. | ||
|
|
||
| config LIBRARY_REGION_SIZE | ||
| hex "Base address for memory, dedicated to loadable modules" |
Copilot
AI
Jul 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The description is incorrect - this config defines the size of the region, not the base address. It should read 'Size of memory region dedicated to loadable modules'.
| hex "Base address for memory, dedicated to loadable modules" | |
| hex "Size of memory region dedicated to loadable modules" |
lgirdwood
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just needs the base address/size Kconfig fixes as per copilot.
9f34d35 to
ccaa42f
Compare
| /* add a region for loadable libraries */ | ||
| ret = adsp_add_virtual_memory_region(CONFIG_LIBRARY_BASE_ADDRESS, | ||
| CONFIG_LIBRARY_REGION_SIZE, | ||
| VIRTUAL_REGION_LLEXT_LIBRARIES_ATTR); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return adsp_add_virtual_memory_region(...);;-)
|
@marcinszkudlinski some CI build error |
yep, Zephyr PR must go first |
ccaa42f to
da254f5
Compare
|
(rebase only) |
da254f5 to
98701fc
Compare
|
|
@marcinszkudlinski looks like we are blocked on the west update atm as its failing fuzzer. @serhiy-katsyuba-intel is bisecting which Zephyr commit breaks. Btw, can you check internal CI. Thanks! |
| - name: zephyr | ||
| repo-path: zephyr | ||
| revision: c99605126cd9cce5684ddd9ad56aed5292004867 | ||
| revision: 8843fd9fe1800164fbb5221d95d1eba7d0699458 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't this breaking bisection? If so, it should be merged with the changes that switch SOF over to the new API
jsarha
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIU LGTM
kv2019i
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marcinszkudlinski can you check this is bisect safe w.r.t. zephyr update?
| repo-path: tomlc99 | ||
| path: sof/tools/rimage/tomlc99 | ||
| revision: e3a03f5ec7d8d33be705c5ce8a632d998ce9b4d1 | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could have the usual commit style for west.yml updates.
| #include <rimage/sof/user/manifest.h> | ||
| #include <module/module/api_ver.h> | ||
|
|
||
| #include <adsp_memory_regions.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the west update needs to come before this, or bisect is broken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the west update needs to come before this, or bisect is broken.
@kv2019i I think it's the opposite - moving to the new Zephyr option without updating SOF to it seems to break bisection to me #10124 (comment)
kv2019i
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style issue remains with the west.yml commit, but not a blocker, approving.
lyakh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bisection seems broken to me, please clarify
|
@lyakh bisect is fine Calling of the functions in question (adsp_add_virtual_memory_region and adsp_mm_get_unused_l2_start_aligned) is already in place, it just do nothing because creation of regions used to be hard-coded in Zephyr. Following commits remove the WA and make use of new Zephyr's features. |
@marcinszkudlinski aha, thanks, and it also works before the fourth commit in your PR? 0a5d62e |
|
yes, it will. default MM_DRV_INTEL_VIRTUAL_REGION_COUNT=1 so a single region will be created |
|
internal CI: couple of ADSP_IPC_MOD_NOT_INITIALIZED, checking... |
98701fc to
8e7191f
Compare
|
if a module had no .bss + .data region there llext loaded still was trying to do mapping for virtual address = 0 and size = 0. FIX: Condition added |
Update to Zephyr commit 8843fd9fe18 Samples: Fix ID for VCNL4040 sensor sample Signed-off-by: Marcin Szkudlinski <marcin.szkudlinski@intel.com>
remove all workarounds for virtual memory region creation: - stubs for zephyr functions - using first memory region without checking attributes Signed-off-by: Marcin Szkudlinski <marcin.szkudlinski@intel.com>
Add a _safe check, verify if virtual memory being mapped for virtual heap fits into the region it belongs Signed-off-by: Marcin Szkudlinski <marcin.szkudlinski@intel.com>
This commit adds initialization of virtual memory region dedicated for LLEXT loadable modules. Signed-off-by: Marcin Szkudlinski <marcin.szkudlinski@intel.com>
Add a _safe check, verify if virtual memory being mapped for LLEXT module fits into the region it belongs Signed-off-by: Marcin Szkudlinski <marcin.szkudlinski@intel.com>
8e7191f to
6cdf069
Compare
|
cosmetic: extra empty line removed, zephyr update commit description changed |

This PR moves creation of virtual memory regions from Zephyr to SOF. ZEphyr is still keeping a table with regions and ensures no regions overlapping
it also creates a new region for loadable modules.
FUTURE: add checking if a module being loaded fits into the region
it is also a fix for viretual memory overlaps - when a platform had 5 cores (PTL) the shared memory regions overlaps witj loadable modules area
this PR must be mergerd after zephyrproject-rtos/zephyr#93334