vpage / vregion: a minimal version with a test#10636
Open
lyakh wants to merge 3 commits intothesofproject:mainfrom
Open
vpage / vregion: a minimal version with a test#10636lyakh wants to merge 3 commits intothesofproject:mainfrom
lyakh wants to merge 3 commits intothesofproject:mainfrom
Conversation
lyakh
requested review from
abonislawski,
dbaluta,
iuliana-prodan,
kv2019i,
lbetlej,
lgirdwood,
mmaka1 and
plbossart
as code owners
Contributor
There was a problem hiding this comment.
Pull request overview
Introduces a minimal “virtual pages” allocator and “virtual regions” (partitioned into interim + lifetime allocators) for SOF on Zephyr/ACE, along with a basic boot test and the required Kconfig/build/board configuration plumbing.
Changes:
- Add
vpagecontiguous virtual-page allocator andvregionallocator built on top of it. - Wire new sources and configs into Zephyr build + Kconfig (including new virtual region attribute).
- Add a Ztest-based boot test for
vregion, and bump ACE board virtual region count.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
zephyr/test/vregion.c |
Adds a basic vregion allocator test under boot tests. |
zephyr/test/CMakeLists.txt |
Builds the new test when CONFIG_SOF_VREGIONS is enabled. |
zephyr/lib/vregion.c |
Implements vregion (interim k_heap + lifetime linear allocator). |
zephyr/lib/vpages.c |
Implements vpage allocator and registers its init via SYS_INIT. |
zephyr/lib/alloc.c |
Adjusts virtual heap bundle sizing/comments to match new region sizing. |
zephyr/include/sof/lib/vregion.h |
Public API for vregion. |
zephyr/include/sof/lib/vpages.h |
Public API for vpage. |
zephyr/include/sof/lib/regions_mm.h |
Adds VIRTUAL_REGION_VPAGES_ATTR. |
zephyr/Kconfig |
Adds SOF_VREGIONS, SOF_VPAGE_MAX_ALLOCS, SOF_ZEPHYR_VIRTUAL_PAGE_REGION_SIZE, and updates heap region sizing defaults/wording. |
zephyr/CMakeLists.txt |
Builds vpages.c/vregion.c when CONFIG_SOF_VREGIONS is enabled. |
app/boards/intel_adsp_ace40_nvls.conf |
Increases virtual region count to 3 for VPAGES region. |
app/boards/intel_adsp_ace40_nvl.conf |
Increases virtual region count to 3 for VPAGES region. |
app/boards/intel_adsp_ace30_wcl.conf |
Increases virtual region count to 3 for VPAGES region. |
app/boards/intel_adsp_ace30_ptl.conf |
Increases virtual region count to 3 for VPAGES region. |
app/boards/intel_adsp_ace20_lnl.conf |
Increases virtual region count to 3 for VPAGES region. |
app/boards/intel_adsp_ace15_mtpm.conf |
Increases virtual region count to 3 for VPAGES region. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
zephyr/lib/vregion.c
Outdated
Comment on lines
+155
to
+158
| LOG_INF("new at base %p size %#x pages %d struct embedded at %p", | ||
| (void *)vr->base, total_size, pages, (void *)vr); | ||
| LOG_DBG(" interim size %#x at %p", interim_size, (void *)vr->interim.heap.heap.init_mem); | ||
| LOG_DBG(" lifetime size %#x at %p", lifetime_size, (void *)vr->lifetime.base); |
There was a problem hiding this comment.
The LOG_INF/LOG_DBG calls here print total_size/interim_size/lifetime_size using %#x, but these are size_t. On 64-bit builds this can truncate and can fail with -Wformat. Prefer %zu or %#zx for sizes.
zephyr/lib/vregion.c
Outdated
Comment on lines
+174
to
+175
| LOG_DBG("destroy %p size %#x pages %d", (void *)vr->base, vr->size, vr->pages); | ||
| LOG_DBG(" lifetime used %d free count %d", vr->lifetime.used, vr->lifetime.free_count); |
There was a problem hiding this comment.
vregion_destroy() logs vr->size and vr->lifetime.used with %#x/%d, but these are size_t. Please update these format strings to the correct size_t specifiers to avoid -Wformat issues.
Add a simple virtual page allocator that uses the Zephyr memory mapping infrastructure to allocate pages from a virtual region and map them to physical pages. Due to simplicity, the number of allocations is limited to CONFIG_SOF_VPAGE_MAX_ALLOCS Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com> Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Add support for per pipeline and per module virtual memory regions. The intention is to provide a single virtual memory region per pipeline or per DP module that can simplify module/pipeline memory management. The region takes advantage of the way pipeline and modules are constructed, destroyed and used during their lifetimes. 1) memory tracking - 1 pointer/size per pipeline or DP module. 2) memory read/write/execute permissions 3) cache utilization. Modules and pipelines will allocate from their region only and this will be abstracted via the allocation APIs. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com> Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Add a boot-time test for basic vregion functionality. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
kv2019i
reviewed
Mar 24, 2026
Collaborator
kv2019i
left a comment
kv2019i
left a comment
There was a problem hiding this comment.
One staging related comment inline. Otherwise looks good and addition of a small test case is great.
| hex "Size in bytes of virtual memory region for virtual heap shared for all cores" | ||
| depends on MM_DRV_INTEL_ADSP_MTL_TLB | ||
| default 0x100000 | ||
| default 0xc0000 |
Collaborator
There was a problem hiding this comment.
Any downsides to this change when done now? I mean this is enable the feature in many builds by default, but the vregions are not actually used yet. I wonder if these Kconfig changes should be done in the PR when vregion is actually used in pipelines?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a slightly simplified version of #10281 with an added test