LL userspace ipc_msg_send() and send_resource_notif() syscalls and related changes - #10725
LL userspace ipc_msg_send() and send_resource_notif() syscalls and related changes#10725jsarha wants to merge 3 commits into
Conversation
9511db2 to
0382995
Compare
|
Hmmm, looks like something is crashintg this branch every now and then at GLB_CREATE_PIPELINE message. Not sure if its my changes or some other bug in the userspace support. I guess I need come up with new methods to debug issue. Can not proceed with getting CONFIG_HOST_DMA_IPC_POSITION_UPDATES=y to work while there is random crashes even without it. |
0382995 to
8812392
Compare
|
I just rebased the the relevant commits on top of V32 of #10558 and tested that they still work, and also that they do not break the normal functions of non LL user-space build. So I think these are ready for wider review and merge. |
There was a problem hiding this comment.
Pull request overview
This PR introduces Zephyr user-space (LL) syscall support for ipc_msg_send() and IPC4 send_resource_notif(), and refactors some IPC4 notification helpers while extending IPC message allocation to optionally use specific heaps/module allocators.
Changes:
- Added Zephyr syscall wrappers + marshalling for
ipc_msg_send()and IPC4send_resource_notif(). - Refactored IPC4 notification helpers into a separate compilation unit and added syscall-side argument validation.
- Extended IPC message initialization APIs to optionally allocate from a specified
k_heapand introduced module-allocator IPC msg init helpers.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| zephyr/CMakeLists.txt | Registers new syscall headers for ipc_msg_send and IPC4 notification syscalls. |
| src/trace/dma-trace.c | Updates ipc_msg_init() call to the new signature. |
| src/samples/audio/detect_test.c | Updates IPC msg init helpers to pass an explicit heap argument. |
| src/library_manager/lib_notification.c | Updates ipc_msg_init() call to the new signature. |
| src/ipc/ipc4/notification.c | Converts send_resource_notif() into a syscall-capable implementation + verifier. |
| src/ipc/ipc4/notification-user.c | Adds refactored, non-syscall IPC4 notification helper functions. |
| src/ipc/ipc4/CMakeLists.txt | Adds the new notification-user.c to the IPC4 build. |
| src/ipc/ipc-common.c | Adds heap-aware IPC msg allocation and syscall support for ipc_msg_send(). |
| src/include/sof/ipc/msg.h | Changes IPC msg init APIs to accept an optional heap; moves send decl to a new header. |
| src/include/sof/ipc/ipc_msg_send.h | Introduces a dedicated header for ipc_msg_send() (syscall-aware). |
| src/include/sof/audio/module_adapter/module/generic.h | Adds module-allocator IPC msg init helpers. |
| src/include/ipc4/notification.h | Declares send_resource_notif() as a syscall under CONFIG_SOF_USERSPACE_LL. |
| src/audio/tdfb/tdfb_ipc4.c | Switches IPC4 notification template allocation to module allocator helper. |
| src/audio/tdfb/tdfb_ipc3.c | Switches IPC3 IPC msg allocation to module allocator helper. |
| src/audio/sound_dose/sound_dose-ipc4.c | Switches IPC4 notification template allocation to module allocator helper. |
| src/audio/pipeline/pipeline-graph.c | Allocates pipeline IPC msg using the pipeline heap via the new API. |
| src/audio/module_adapter/module/cadence_ipc4.c | Switches IPC notification template allocation to module allocator helper. |
| src/audio/mfcc/mfcc_ipc4.c | Switches IPC4 notification template allocation to module allocator helper. |
| src/audio/host-zephyr.c | Allocates host position IPC msg using the host heap via the new API. |
| src/audio/host-legacy.c | Updates ipc_msg_init() call to the new signature. |
| src/audio/google/google_hotword_detect.c | Updates ipc_msg_init() call to the new signature. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| #else | ||
| void z_impl_ipc_msg_send(struct ipc_msg *msg, void *data, | ||
| bool high_priority); | ||
| #define ipc_msg_send z_impl_ipc_msg_send | ||
| #endif |
There was a problem hiding this comment.
This header, with the macro is also included in the c-file where the implementation is. So the macro will replace ipc_msg_send with z_impl_ipc_msg_send .
| struct ipc_msg *ipc_msg_w_ext_init(struct k_heap *heap, uint32_t header, | ||
| uint32_t extension, uint32_t size); | ||
|
|
There was a problem hiding this comment.
That is a clear bug. Will fix.
| void z_vrfy_ipc_msg_send(struct ipc_msg *msg, void *data, bool high_priority) | ||
| { | ||
| K_OOPS(K_SYSCALL_MEMORY_WRITE(msg, sizeof(*msg))); | ||
|
|
||
| z_vrfy_ipc_msg_send_check_data(msg, data); | ||
|
|
||
| z_impl_ipc_msg_send(msg, data, high_priority); | ||
| } |
There was a problem hiding this comment.
More the checks the better.
| static inline struct ipc_msg *mod_ipc_msg_w_ext_init(struct processing_module *mod, | ||
| uint32_t header, | ||
| uint32_t extension, | ||
| uint32_t size) | ||
| { |
8812392 to
fff3d44
Compare
|
Issues found by Copilot fixed. |
kv2019i
left a comment
There was a problem hiding this comment.
The syscall verify checks are tricky, a few comments on those (plus some minor stuff).
| #endif | ||
|
|
||
| #if defined(__ZEPHYR__) && defined(CONFIG_SOF_USERSPACE_LL) | ||
| #include <zephyr/syscalls/ipc_msg_list_remove.h> |
There was a problem hiding this comment.
Minor: this could be part of the first if branch as the condition is the same..
| #define ipc_msg_send z_impl_ipc_msg_send | ||
| #endif | ||
|
|
||
| #if defined(__ZEPHYR__) && defined(CONFIG_SOF_USERSPACE_LL) |
| @@ -0,0 +1,32 @@ | |||
| /* SPDX-License-Identifier: BSD-3-Clause | |||
| * | |||
| * Copyright(c) 2026 Intel Corporation. All rights reserved. | |||
There was a problem hiding this comment.
Drop the all rights reserved.
|
|
||
| z_vrfy_ipc_msg_send_check_data(msg, data); | ||
|
|
||
| z_impl_ipc_msg_send(msg, data, high_priority); |
There was a problem hiding this comment.
I think this needs addition checks on the embedded list. Kernel will call "(list_is_empty(&msg->list)" and we need to make sure this is safe any user has access to any non-NULL pointers.
There was a problem hiding this comment.
@kv2019i hm, what do we do with, e.g. syscalls from generic.c like mod_fast_get() - the contents of the user-writable struct processing_module *mod object is also trusted there.
There was a problem hiding this comment.
If we check that the the msg->list is not part of any list (e.g. list_is_empty(&msg->list)), then isn't that enough? No need to check if the user or has or has not access to where ever the next and prev pointers are pointing, the previous check has already failed.
| k_spinlock_key_t key; | ||
|
|
||
| key = k_spin_lock(&ipc->lock); | ||
| list_item_del(&msg->list); |
There was a problem hiding this comment.
I think same here, we can't trust msg->list contents and needs to be checked in z_vrfy()
There was a problem hiding this comment.
Yes, maybe best to go through the ipc->msg_list and find the item to be removed from there, and K_OOPS if its is not part of that list.
| * | ||
| * @param msg The IPC message to remove from the queue. | ||
| */ | ||
| #if defined(__ZEPHYR__) && defined(CONFIG_SOF_USERSPACE_LL) |
There was a problem hiding this comment.
let's use the same condition as in other syscalls - #if defined(__ZEPHYR__) && defined(CONFIG_SOF_FULL_ZEPHYR_APPLICATION) - having it as syscall doesn't hurt - if it isn't called from userspace it will anyway just resolve to a function call.
There was a problem hiding this comment.
Shouln't this be the same condition as in .c file, e.g. if c-file has ZEPHYR && CONFIG_USERSPACE, why not have the same in here?
| @@ -0,0 +1,30 @@ | |||
| /* SPDX-License-Identifier: BSD-3-Clause | |||
There was a problem hiding this comment.
do we really need this level of header granularity?
| } | ||
| EXPORT_SYMBOL(ipc_msg_send); | ||
|
|
||
| #ifdef CONFIG_SOF_USERSPACE_LL |
There was a problem hiding this comment.
...and just CONFIG_USERSPACE in .c
| { | ||
| K_OOPS(K_SYSCALL_MEMORY_WRITE(msg, sizeof(*msg))); | ||
|
|
||
| z_vrfy_ipc_msg_send_check_data(msg, data); |
There was a problem hiding this comment.
don't use the z_vrfy_ prefix for a normal function. Yes, z_vrfy_ipc_msg_send_check_data() should be fixed too.
There was a problem hiding this comment.
I do not follow this exactly. Yes, I can remove z_vrfy_ from z_vrfy_ipc_msg_send_check_data() or get rid of that completely, but isn't the practice that a syscall has a matching z_vrfy_*() for checking the arguments?
There was a problem hiding this comment.
@jsarha it isn't just a "practice," it's how the Zephyr machinery works. You declare __syscall x() in a header, you implement it as z_impl_x() and you verify userspace safety in z_vrfy_x() which in success case explicitly calls z_impl_x(). It isn't just a convention, it's how we have to implement syscalls. But ipc_msg_send_check_data() wasn't a syscall, z_vrfy_ipc_msg_send_check_data() was just a normal function, so I commented, that to avoid confusion it would be better not to use the z_vrfy_ prefix for functions, not directly verifying a syscall. Now it's gone, so it's ok now
|
|
||
| z_vrfy_ipc_msg_send_check_data(msg, data); | ||
|
|
||
| z_impl_ipc_msg_send(msg, data, high_priority); |
There was a problem hiding this comment.
@kv2019i hm, what do we do with, e.g. syscalls from generic.c like mod_fast_get() - the contents of the user-writable struct processing_module *mod object is also trusted there.
| uint32_t resource_type, void *data, uint32_t data_size); | ||
|
|
||
| bool z_impl_send_resource_notif(uint32_t resource_id, uint32_t event_type, | ||
| uint32_t resource_type, void *data, uint32_t data_size); |
|
|
||
| static enum sof_ipc4_resource_event_type dir_to_xrun_event(enum sof_ipc_stream_direction dir) | ||
| { | ||
| return (dir == SOF_IPC_STREAM_PLAYBACK) ? SOF_IPC4_GATEWAY_UNDERRUN_DETECTED : |
| #else | ||
| bool send_resource_notif(uint32_t resource_id, uint32_t event_type, | ||
| uint32_t resource_type, void *data, uint32_t data_size) | ||
| #endif |
There was a problem hiding this comment.
no need - as long as you use the same preprocessor conditionals as other currently existing syscalls
There was a problem hiding this comment.
I either need this or I need #define send_resource_notif z_impl_send_resource_notif in the header else branch. The later is maybe a bit cleaner.
| } | ||
|
|
||
| bool send_copier_gateway_xrun_notif_msg(uint32_t pipeline_id, enum sof_ipc_stream_direction dir) | ||
| #ifdef CONFIG_SOF_USERSPACE_LL |
fff3d44 to
69acc5a
Compare
|
All review comments now hopefully addressed. Its still not clear to me behind what ifdef, @lyakh , would you want to put these syscalls? They all are now just behind CONFIG_USERSPACE. Now that I need to deal with this code without Copilot I can see how horribly inconsistent code it can write. Something that does not strike the eye at first glance, but when going back and forth the series for multiple times, it looks like at each occasion it pick the solution by random, and hardly ever uses an existing pattern. Anyway, I have now tested these commits again on top of both LL-userspace feature branch and our mainline, with CONFIG_USERSPACE both n and y, and everything appears to work (also my hacks that force notification sending of different types). I hope do not have to do this again. |
comments addressed. Not "approving" to let @kv2019i verify whether userspace checking is now satisfactory
kv2019i
left a comment
There was a problem hiding this comment.
Looks good now. Limiting who can send IPCs would be nice to have, but we can do that in a follow-up as well. If you need to respin, please add at least a comment.
| /* If data != NULL and tx_size > 0, verify the data buffer */ | ||
| if (data && msg->tx_size > 0) | ||
| K_OOPS(K_SYSCALL_MEMORY_READ(data, msg->tx_size)); | ||
|
|
There was a problem hiding this comment.
Now that this (ability to call ipc_msg_send() from user) is under CONFIG_USERSPACE, we need an additinal check which user threads can send IPCs. I'd lean to only allow this in builds with CONFIG_SOF_USERSPACE_LL defined for now to limit the exposure.
| * @param high_priority True if a high priortity message. | ||
| */ | ||
| void ipc_msg_send(struct ipc_msg *msg, void *data, bool high_priority); | ||
|
|
There was a problem hiding this comment.
I should probably move the syscall declaration in this same place where the function declaration was.
| uint32_t resource_type, void *data, uint32_t data_size); | ||
| #define send_resource_notif z_impl_send_resource_notif | ||
|
|
||
| #endif /* CONFIG_SOF_USERSPACE_LL */ |
There was a problem hiding this comment.
This is inconsistent with the #ifdef , like the commit message, but will be Ok again when I go back to CONFIG_SOF_USERSPACE_LL ifdef.
4ccc83a to
b726b78
Compare
|
Fixed the minor issue I noticed myself and went back to ifdef CONFIG_SOF_USERSPACE_LL. |
| * | ||
| * @param msg The IPC message to remove from the queue. | ||
| */ | ||
| #ifdef CONFIG_SOF_USERSPACE_LL |
There was a problem hiding this comment.
@jsarha not sure why you reverted to this scheme. I still think the one used in other our syscalls is preferable
There was a problem hiding this comment.
@lyakh I think @jsarha is doing this to address my earlier concern "Now that this (ability to call ipc_msg_send() from user) is under CONFIG_USERSPACE, we need an additinal check which user threads can send IPCs.". I was actually thinking of just returning error in vrfy if not a CONFIG_SOF_USERSPACE_LL, but that's kind of wasted mem for code. So why not just disable the syscalls.
There was a problem hiding this comment.
Let's go with the pattern used in ipc4/handler.h . This should compile on all targets (include cmocka/testbench that don't understand Zephyr __syscall macros).
Make ipc_msg_send() a Zephyr system call so audio processing modules running in user-space LL threads can queue IPC messages (e.g. position updates, notifications) back to the host. The change takes effect only if CONFIG_SOF_USERSPACE_LL=y. Follows the same pattern used for ipc_msg_reply(): a dedicated header with __syscall declaration, z_impl/z_vrfy split, and syscall header registration in CMakeLists.txt. The verifier validates that the msg struct is writable (the implementation touches the list linkage) and that the data buffer, when provided, is readable up to msg->tx_size bytes. Add ipc_msg_list_remove() as a system call that acquires the IPC lock and removes a message from its list. This allows message freeing to work from userspace threads that cannot access kernel spinlocks directly. Update ipc_msg_free() in msg.h to use ipc_msg_list_remove() instead of directly accessing the IPC spinlock. Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>
Add an optional heap parameter to ipc_msg_w_ext_init() and ipc_msg_init() so callers can direct allocations to a specific heap. When the heap argument is NULL the existing rzalloc() path is used; when non-NULL, sof_heap_alloc()/sof_heap_free() are used instead. This allows IPC messages to be allocated from userspace-accessible heaps. For audio module contexts, introduce mod_ipc_msg_w_ext_init() and mod_ipc_msg_init() in generic.h. These use mod_zalloc()/ mod_free() for allocations that are automatically tracked and freed with the module lifecycle. ipc_msg_w_ext_init() is moved from a static inline in msg.h to a non-inline function in ipc-common.c due to the additional sof_heap_alloc dependency. Update all existing callers: - Module context callers (cadence, sound_dose, tdfb, mfcc) use the new mod_ipc_msg_*() variants and mod_ipc_msg_free(). - host-zephyr.c uses hd->heap, pipeline-graph.c uses the heap parameter from pipeline_new(). - Remaining kernel-context callers pass NULL for the default heap. Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>
Move user-facing notification functions (send_copier_gateway_xrun_notif_msg, send_gateway_xrun_notif_msg, send_mixer_underrun_notif_msg, send_process_data_error_notif_msg) to a new notification-user.c file so they can run in userspace. The send_resource_notif() function, which depends on the kernel-side notification pool and IPC message infrastructure, is converted to a Zephyr syscall. The implementation is renamed to z_impl_send_resource_notif() and remains in notification.c alongside is_notif_filtered_out() and ipc4_update_notification_mask(). The send_resource_notif() is converted to a system call only if CONFIG_SOF_USERSPACE_LL=y, without it the behaviour is same as befofe. A z_vrfy_send_resource_notif() handler is added to validate the user-provided data buffer and other parameters before forwarding to the kernel implementation. Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>
b726b78 to
f3dc0e8
Compare
FYI @kv2019i , @lyakh , @lrgirdwo
I still intent to make sure the position updates work.