Skip to content

Conversation

@marcinszkudlinski
Copy link
Contributor

@marcinszkudlinski marcinszkudlinski commented Jul 18, 2025

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

Copy link

Copilot AI left a 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)

should be set by this option.

config LIBRARY_REGION_SIZE
hex "Base address for memory, dedicated to loadable modules"
Copy link

Copilot AI Jul 18, 2025

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'.

Suggested change
hex "Base address for memory, dedicated to loadable modules"
hex "Size of memory region dedicated to loadable modules"

Copilot uses AI. Check for mistakes.
@marcinszkudlinski marcinszkudlinski changed the title Virt regions Move virtual region creation from Zephyr to SOF Jul 18, 2025
Copy link
Member

@lgirdwood lgirdwood left a 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.

@marcinszkudlinski marcinszkudlinski changed the title Move virtual region creation from Zephyr to SOF Move virtual region creation from Zephyr to SOF, add virtual memory regions protection Jul 31, 2025
/* 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);
Copy link
Collaborator

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(...);;-)

@lgirdwood
Copy link
Member

@marcinszkudlinski some CI build error

@marcinszkudlinski
Copy link
Contributor Author

@marcinszkudlinski some CI build error

yep, Zephyr PR must go first

@marcinszkudlinski
Copy link
Contributor Author

(rebase only)

@marcinszkudlinski
Copy link
Contributor Author

  • zephyr with required change

@lgirdwood
Copy link
Member

@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
Copy link
Collaborator

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

Copy link
Contributor

@jsarha jsarha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIU LGTM

Copy link
Collaborator

@kv2019i kv2019i left a 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

Copy link
Collaborator

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>
Copy link
Collaborator

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so it does...
image

Copy link
Collaborator

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)

Copy link
Collaborator

@kv2019i kv2019i left a 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
lyakh previously requested changes Aug 18, 2025
Copy link
Collaborator

@lyakh lyakh left a 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

@marcinszkudlinski
Copy link
Contributor Author

@lyakh bisect is fine
the very first commit switches Zephyr to new version
That was the whole point of WA I put in SOF - to solve circular dependency SOF-Zephyr
at moment of zephyr's SHA is changed, the __weak marked functions from SOF will be replaced by new Zephyr's

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.

@lyakh
Copy link
Collaborator

lyakh commented Aug 18, 2025

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.

@marcinszkudlinski aha, thanks, and it also works before the fourth commit in your PR? 0a5d62e

@marcinszkudlinski
Copy link
Contributor Author

yes, it will. default MM_DRV_INTEL_VIRTUAL_REGION_COUNT=1 so a single region will be created

@marcinszkudlinski
Copy link
Contributor Author

internal CI: couple of ADSP_IPC_MOD_NOT_INITIALIZED, checking...

@lyakh lyakh dismissed their stale review August 18, 2025 12:45

question resolved, waiting for CI results

@marcinszkudlinski
Copy link
Contributor Author

marcinszkudlinski commented Aug 20, 2025

if a module had no .bss + .data region there llext loaded still was trying to do mapping for virtual address = 0 and size = 0.
Virtual region protection introduced in this PR was rejecting such call causing the whole module load process failure

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>
@marcinszkudlinski
Copy link
Contributor Author

cosmetic: extra empty line removed, zephyr update commit description changed

@kv2019i kv2019i merged commit 28a99f1 into thesofproject:main Aug 22, 2025
36 of 45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants