userspace: reduce fast-get scope and simplify preprocessor conditionals#10549
userspace: reduce fast-get scope and simplify preprocessor conditionals#10549lyakh wants to merge 2 commits intothesofproject:mainfrom
Conversation
lyakh
changed the title
lyakh
requested review from
LaurentiuM1234,
abonislawski,
dbaluta,
iuliana-prodan,
kv2019i,
lbetlej,
lgirdwood,
marcinszkudlinski,
mmaka1,
pblaszko and
plbossart
as code owners
There was a problem hiding this comment.
Pull request overview
This pull request optimizes the fast-get functionality and simplifies preprocessor conditionals for userspace isolation in the SOF (Sound Open Firmware) Zephyr integration.
Changes:
- Restricts fast-get implementation compilation to when .cold/.coldrodata sections are actually created
- Adds inline stub functions for fast-get when the full implementation isn't needed
- Simplifies userspace preprocessor conditionals by moving them to call sites
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| zephyr/CMakeLists.txt | Conditionally compiles fast-get.c only when CONFIG_COLD_STORE_EXECUTE_DRAM is enabled |
| src/include/sof/lib/fast-get.h | Adds preprocessor logic to provide inline stub implementations when fast-get isn't needed |
| src/schedule/zephyr_dp_schedule_thread.c | Moves k_thread_access_grant and scheduler_dp_grant calls inside CONFIG_USERSPACE guard |
| src/schedule/zephyr_dp_schedule.c | Removes CONFIG_USERSPACE preprocessor guard from scheduler_dp_grant function body |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| add_subdirectory(../src/module module_unused_install/) | ||
|
|
||
| if(CONFIG_COLD_STORE_EXECUTE_DRAM) | ||
| zephyr_library_sources_ifdef(CONFIG_FAST_GET lib/fast-get.c) |
There was a problem hiding this comment.
The compilation of fast-get.c now depends on both CONFIG_COLD_STORE_EXECUTE_DRAM and CONFIG_FAST_GET being enabled. However, the header file (src/include/sof/lib/fast-get.h lines 15-17) declares the real fast_get/fast_put functions based on CONFIG_COLD_STORE_EXECUTE_DRAM and other conditions, without checking CONFIG_FAST_GET. This could lead to a linking error if CONFIG_COLD_STORE_EXECUTE_DRAM is enabled but CONFIG_FAST_GET is not, since the header would declare the functions but the implementation wouldn't be compiled. Consider either: 1) removing the CONFIG_FAST_GET check here if CONFIG_FAST_GET is always enabled when CONFIG_COLD_STORE_EXECUTE_DRAM is true, or 2) updating the header to also check CONFIG_FAST_GET.
| zephyr_library_sources_ifdef(CONFIG_FAST_GET lib/fast-get.c) | |
| zephyr_library_sources(lib/fast-get.c) |
|
SOFCI TEST |
| # SOF module interface functions | ||
| add_subdirectory(../src/module module_unused_install/) | ||
|
|
||
| if(CONFIG_COLD_STORE_EXECUTE_DRAM) |
There was a problem hiding this comment.
Why just not to add the dependency to FAST_GET kconfig?
depends on COLD_STORE_EXECUTE_DRAM
There was a problem hiding this comment.
the current solution only affects this CMakeLists.txt, while yours would affect all build variants, using that Kconfig. But I'm not sure there are any such other build variants. Even ztest doesn't seem to use it. So, yes, that might work too. Let me try. Thanks
There was a problem hiding this comment.
@softwarecki actually no, I don't think it would work - because of
sof/src/audio/module_adapter/module/generic.c
Line 342 in 53f713a
There was a problem hiding this comment.
@lyakh I think you mentioned wrong Adrian...
@abonislawski
Kernel threads have access to all the memory, no need to additionally grant access to them. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
fast-get is only needed when DRAM is used, i.e. when CONFIG_COLD_STORE_EXECUTE_DRAM is selectef. It's also unneeded in modules when CONFIG_LLEXT_TYPE_ELF_SHAREDLIB is used, i.e. in gcc builds, because also in those builds no .cold and .coldrodata sections are created. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
3 participants
two simple commits: