ipc: add cache flushing and invalidation for IPC data #9630
Conversation
tmleman
requested review from
bardliao,
marcinszkudlinski,
pblaszko,
lgirdwood,
plbossart,
mmaka1,
lbetlej,
dbaluta and
kv2019i
as code owners
tmleman
force-pushed
the
topic/upstream/pr/ipc4/secondary_core_response
branch
from
ea10c12
to
a99c314
Compare
tmleman
requested review from
abonislawski,
softwarecki,
serhiy-katsyuba-intel and
gbernatxintel
| @@ -214,6 +214,9 @@ void ipc_msg_send(struct ipc_msg *msg, void *data, bool high_priority) | |||
| msg->tx_data != data) { | |||
| ret = memcpy_s(msg->tx_data, msg->tx_size, data, msg->tx_size); | |||
| 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). |
tmleman
force-pushed
the
topic/upstream/pr/ipc4/secondary_core_response
branch
2 times, most recently
from
de9a0d9
to
bdbe8ce
Compare
| @@ -72,9 +72,9 @@ struct ipc4_msg_data { | |||
| static struct ipc4_msg_data msg_data; | |||
|
|
|||
| /* 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.
tmleman
force-pushed
the
topic/upstream/pr/ipc4/secondary_core_response
branch
2 times, most recently
from
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>
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.