ipc: add cache flushing and invalidation for IPC data#9630
Conversation
ea10c12 to
a99c314
Compare
kv2019i
left a comment
There was a problem hiding this comment.
Good catch, see comment inline.
| assert(!ret); | ||
| if (!cpu_is_primary(cpu_get_id())) | ||
| dcache_writeback_region((__sparse_force void __sparse_cache *)msg->tx_data, | ||
| msg->tx_size); |
There was a problem hiding this comment.
This is a good catch. I wonder if a better fix would be to just allocate tx_data as uncached.
There was a problem hiding this comment.
Possible, but it probably comes with a performance cost. It's hard for me to determine what is better. However, invalidating each message also seems problematic to me, and I considered adding some flag in the structure that would indicate that the payload should be invalidated before copying.
|
@tmleman any idea why we haven't seen these failures in CI? Is the contents of received IPCs never verified? |
It seems to me that this is because most of the large_config_get messages are processed on the main core, and the responses to messages that were sent to secondary cores only return a status informing whether the operation was successful (messages such as module_init or set_pipeline_state). |
de9a0d9 to
bdbe8ce
Compare
|
|
||
| /* fw sends a fw ipc message to send the status of the last host ipc message */ | ||
| static struct ipc_msg msg_reply = {0, 0, 0, 0, LIST_INIT(msg_reply.list)}; | ||
| static struct ipc_msg msg_reply = {0, 0, 0, 0, false, LIST_INIT(msg_reply.list)}; |
There was a problem hiding this comment.
are those static structures ever cleaned? If not, once is_shared is set to true it'll never be set to back false
There was a problem hiding this comment.
Thanks, that was a good catch. It seems to me that I managed to cover all cases now.
ee2bb7b to
e43f8c4
Compare
This patch addresses an issue with incorrect IPC responses due to the lack of cache flushing and invalidation on secondary cores. The following changes have been made: 1. Added cache writeback for IPC message data in `ipc_msg_send` when the current core is not the primary core. 2. Added cache invalidation for IPC message data in `ipc_prepare_to_send` before writing to the mailbox. These changes ensure that the IPC data is correctly synchronized between cores. Signed-off-by: Tomasz Leman <tomasz.m.leman@intel.com>
lgirdwood
left a comment
There was a problem hiding this comment.
CI passing with multicore tests.
kv2019i
left a comment
There was a problem hiding this comment.
I still wonder whether it would be overall better to just not use cached alloc for tx_data given IPC processing is not on the DSP hot path. OTOH, with the current approach (with cached used), this PR is needed.
This patch addresses an issue with incorrect IPC responses due to the lack of cache flushing and invalidation on secondary cores.
The following changes have been made:
ipc_msg_sendwhen the current core is not the primary core.ipc_prepare_to_sendbefore writing to the mailbox.These changes ensure that the IPC data is correctly synchronized between cores.