nrf_security: cracen: Improve DCACHE handling #20419
Conversation
github-actions
bot
added
the
changelog-entry-required
CI InformationTo view the history of this post, clich the 'edited' button above Inputs:Sources:sdk-nrf: PR head: 87a880672b6fc8800949ba36a59c73e59237597f more detailssdk-nrf:
Github labels
List of changed files detected by CI (6)Outputs:ToolchainVersion: aedb4c0245 Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped;
|
| sys_cache_data_flush_range(&out_desc, sizeof(out_desc)); | ||
| sys_cache_data_flush_range(input, input_size); | ||
| sys_cache_data_flush_range(output, output_size); | ||
| sys_cache_data_flush_and_invd_range(in_descs, sizeof(in_descs)); |
There was a problem hiding this comment.
this is overkill, flush would be enough, only output needs to be invalidated and that is done once the sx_cmdma_check() returned ok
There was a problem hiding this comment.
If that's overkill why not continue using sys_cache_data_flush_range() where it's okay?
There was a problem hiding this comment.
Sorry yes that is the plan I just wanted some initial feedback on the changes in general. I used sys_cache_data_flush_and_invd_range as I wanted to avoid any optimization in a first pass to see if it makes CI pass when DCACHE is enabled. It still doesn't so I was hoping to get more eyes onto it first
There was a problem hiding this comment.
just noticed these aren't correct, they just flush the descriptors, not the buffers itself, needs to be reworked
| @@ -381,7 +381,9 @@ int sx_blkcipher_status(struct sxblkcipher *c) | |||
| } | |||
|
|
|||
| #if CONFIG_DCACHE | |||
| sys_cache_data_invd_range((void *)&c->extramem, sizeof(c->extramem)); | |||
There was a problem hiding this comment.
invalidate range should be avoided as it could lead to memory loss on that cacheline if something else on the line was written by the core, e.g. if extramem is somewhere on the stack
| sys_cache_data_flush_range(&out_desc, sizeof(out_desc)); | ||
| sys_cache_data_flush_range(input, input_size); | ||
| sys_cache_data_flush_range(output, output_size); | ||
| sys_cache_data_flush_and_invd_range(in_descs, sizeof(in_descs)); |
There was a problem hiding this comment.
If that's overkill why not continue using sys_cache_data_flush_range() where it's okay?
karstenkoenig
force-pushed
the
dcache_cracen
branch
3 times, most recently
from
976946a to
b0abce8
Compare
| @@ -474,7 +474,8 @@ int sx_aead_status(struct sxaead *c) | |||
| } | |||
|
|
|||
| #if CONFIG_DCACHE | |||
| sys_cache_data_invd_range((void *)&c->extramem, sizeof(c->extramem)); | |||
| sx_cmdma_outdescs_flush_and_invd_dcache(&c->dma); | |||
| sys_cache_data_flush_and_invd_range((void *)&c->extramem, sizeof(c->extramem)); | |||
There was a problem hiding this comment.
Before this patch it is my understanding that the driver would do this:
sx_cmdma_check(); // Wait for CRACEN to write to RAM
sys_cache_data_invd_range(); // Invalidate the datacache, as it is now outdated wrt. what CRACEN wrote to RAM.
sx_memdiff(c->expectedtag, c->extramem); // Read from RAM what CRACEN wrote
In this new version we will do this:
sx_cmdma_check(); // Wait for CRACEN to write to RAM
sys_cache_data_flush_and_invd_range(); // Write the affected cachelines to RAM, and then invalidate those cachelines
Won't the sys_cache_data_flush_and_invd_range overwrite what CRAECN just wrote to RAM? Or am I misreading this?
There was a problem hiding this comment.
Hmmm thanks for raising this to my attention again, I thought this would be perfect as sys_cache_data_flush_and_invd_range is the same as cache eviction and that might happen at any time anyways and makes sure all the data the core has written to gets properly stored. A cacheline can be shared between output buffer and other data structures from the core, and if the core wrote to that other data it isn't allowed to get lost either.
But there is still chance for failure. The granularity for our data cache isn't a full cacheline, but a data unit, so sharing a cache line between local data structures and an output buffer for dma can be ok as long as the output buffer is 32bit aligned and is a multiple of 32bits in size.
But if the output buffer and some other date share the same dataunit in a cacheline you can get a conflict, but then both operations (data_flush_and_invd_range and invd_range) are wrong, as they both lead to a data loss, as this will either throw away the dirty memory inside the cores data cache, or write out the full 32bit word on a flush overwriting part of the output buffer. Need to take another round through this, and also the client side then.
karstenkoenig
force-pushed
the
dcache_cracen
branch
from
b0abce8 to
97a56c8
Compare
Calling plain invalidate isn't always safe, as there might be pending writes in the cache line that just get thrown away. Just switch to full cache line evasion, so writeback + invalidate just to be safe. Also iterate over the out descriptors to make sure all the buffers are cache handled on completing of the cracen operations. Signed-off-by: Karsten Koenig <karsten.koenig@nordicsemi.no>
karstenkoenig
force-pushed
the
dcache_cracen
branch
from
97a56c8 to
87a8806
Compare
|
|
This pull request has been marked as stale because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 7 days. Note, that you can always re-open a closed pull request at any time. |
Calling plain invalidate isn't always safe, as there might be pending writes in the cache line that just get thrown away. Just switch to full cache line evasion, so writeback + invalidate just to be safe. Also iterate over the out descriptors to make sure all the buffers are cache handled on completing of the cracen operations.