Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nrf_security: cracen: Improve DCACHE handling #20419

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

nrf_security: cracen: Improve DCACHE handling #20419

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion subsys/nrf_security/src/drivers/cracen/sxsymcrypt/src/aead.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

#endif

if (!(c->dma.dmamem.cfg & c->cfg->ctxsave) && c->expectedtag != NULL) {
Expand Down
15 changes: 8 additions & 7 deletions subsys/nrf_security/src/drivers/cracen/sxsymcrypt/src/blkcipher.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,6 @@ void sx_blkcipher_free(struct sxblkcipher *c)
sx_cmdma_release_hw(&c->dma);
}


static int sx_blkcipher_hw_reserve(struct sxblkcipher *c)
{
int err = SX_OK;
Expand Down Expand Up @@ -149,8 +148,7 @@ static int sx_blkcipher_create_aesxts(struct sxblkcipher *c, const struct sxkeyr
mode = CMDMA_BLKCIPHER_MODE_SET(BLKCIPHER_MODEID_XTS);
keyszfld = 0;

sx_cmdma_newcmd(&c->dma, c->descs,
KEYREF_AES_HWKEY_CONF(key1->cfg) | mode | keyszfld,
sx_cmdma_newcmd(&c->dma, c->descs, KEYREF_AES_HWKEY_CONF(key1->cfg) | mode | keyszfld,
c->cfg->dmatags->cfg);
if (KEYREF_IS_USR(key1)) {
ADD_CFGDESC(c->dma, key1->key, key1->sz, c->cfg->dmatags->key);
Expand Down Expand Up @@ -240,8 +238,7 @@ int sx_blkcipher_create_aesctr_dec(struct sxblkcipher *c, const struct sxkeyref
return sx_blkcipher_create_aes_ba411(c, key, iv, &ba411ctrcfg, ba411ctrcfg.decr);
}

int sx_blkcipher_create_aesecb_enc(struct sxblkcipher *c, const struct sxkeyref *key
)
int sx_blkcipher_create_aesecb_enc(struct sxblkcipher *c, const struct sxkeyref *key)
{
return sx_blkcipher_create_aes_ba411(c, key, NULL, &ba411ecbcfg, CM_CFG_ENCRYPT);
}
Expand Down Expand Up @@ -381,7 +378,9 @@ int sx_blkcipher_status(struct sxblkcipher *c)
}

#if CONFIG_DCACHE
sys_cache_data_invd_range((void *)&c->extramem, sizeof(c->extramem));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

sx_cmdma_outdescs_flush_and_invd_dcache(&c->dma);
/* extramem is often a target for out descriptors, this might not be needed anymore */
sys_cache_data_flush_and_invd_range((void *)&c->extramem, sizeof(c->extramem));
#endif

sx_blkcipher_free(c);
Expand Down Expand Up @@ -435,6 +434,8 @@ int sx_blkcipher_ecb_simple(uint8_t *key, size_t key_size, uint8_t *input, size_
#if CONFIG_DCACHE
sys_cache_data_flush_range(in_descs, sizeof(in_descs));
sys_cache_data_flush_range(&out_desc, sizeof(out_desc));
sys_cache_data_flush_range(&cmd, sizeof(cmd));
sys_cache_data_flush_range(key, key_size);
sys_cache_data_flush_range(input, input_size);
sys_cache_data_flush_range(output, output_size);
#endif
Expand All @@ -451,7 +452,7 @@ int sx_blkcipher_ecb_simple(uint8_t *key, size_t key_size, uint8_t *input, size_
sx_cmdma_release_hw(NULL);

#if CONFIG_DCACHE
sys_cache_data_invd_range(output, output_size);
sys_cache_data_flush_and_invd_range(output, output_size);
#endif

return r;
Expand Down
21 changes: 18 additions & 3 deletions subsys/nrf_security/src/drivers/cracen/sxsymcrypt/src/cmdma.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,17 +28,19 @@ static void sx_cmdma_finalize_descs(struct sxdesc *start, struct sxdesc *end)

for (d = start; d < end; d++) {
#ifdef DMA_FIFO_ADDR
if (d->addr == (char *)DMA_FIFO_ADDR)
if (d->addr == (char *)DMA_FIFO_ADDR) {
d->sz |= DMA_CONST_ADDR;
}
#endif
d->next = d + 1;
}
end->next = DMA_LAST_DESCRIPTOR;
end->dmatag |= DMATAG_LAST;
end->sz |= DMA_REALIGN;
#ifdef DMA_FIFO_ADDR
if (end->addr == (char *)DMA_FIFO_ADDR)
if (end->addr == (char *)DMA_FIFO_ADDR) {
end->sz |= DMA_CONST_ADDR;
}
#endif
}

Expand All @@ -52,14 +54,16 @@ void sx_cmdma_start(struct sx_dmactl *dma, size_t privsz, struct sxdesc *indescs
#ifdef CONFIG_DCACHE
m = (struct sxdesc *)(dma->mapped + sizeof(struct sx_dmaslot));
for (; m != DMA_LAST_DESCRIPTOR; m = m->next) {
sys_cache_data_flush_and_invd_range(m, sizeof(*m));
sys_cache_data_flush_and_invd_range(m->addr, m->sz & DMA_SZ_MASK);
}
m = (struct sxdesc *)(dma->mapped + offsetof(struct sx_dmaslot, outdescs));
for (; m != DMA_LAST_DESCRIPTOR; m = m->next) {
sys_cache_data_flush_and_invd_range(m, sizeof(*m));
sys_cache_data_flush_and_invd_range(m->addr, m->sz & DMA_SZ_MASK);
}

sys_cache_data_flush_range((void *)&dma->dmamem, sizeof(dma->dmamem) + privsz);
sys_cache_data_flush_and_invd_range((void *)&dma->dmamem, sizeof(dma->dmamem) + privsz);
#endif

m = (struct sxdesc *)(dma->mapped + sizeof(struct sx_dmaslot));
Expand All @@ -70,6 +74,17 @@ void sx_cmdma_start(struct sx_dmactl *dma, size_t privsz, struct sxdesc *indescs
sx_wrreg(REG_START, REG_START_ALL);
}

#ifdef CONFIG_DCACHE
void sx_cmdma_outdescs_flush_and_invd_dcache(const struct sx_dmactl *dma)
{
struct sxdesc *m = (struct sxdesc *)(dma->mapped + offsetof(struct sx_dmaslot, outdescs));

for (; m != DMA_LAST_DESCRIPTOR; m = m->next) {
sys_cache_data_flush_and_invd_range(m->addr, m->sz & DMA_SZ_MASK);
}
}
#endif

int sx_cmdma_check(void)
{
uint32_t r = 0xFF;
Expand Down
29 changes: 17 additions & 12 deletions subsys/nrf_security/src/drivers/cracen/sxsymcrypt/src/cmdma.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
#define DMATAG_CONFIG(offset) ((1 << 4) | (offset << 8))

/* can be 0, 1 or 2 */
#define DMATAG_DATATYPE(x) (x << 6)
#define DMATAG_DATATYPE(x) (x << 6)

#define DMATAG_DATATYPE_HEADER (1 << 6)
#define DMATAG_DATATYPE_REFERENCE (3 << 6)
Expand Down Expand Up @@ -106,17 +106,17 @@
(dmactl).d++; \
} while (0)

#define ADD_INDESC_BITS(dmactl, baddr, bsz, tag, msk, bitsz)\
do {\
size_t bitmask = (msk << 3) | 0x7;\
size_t validbitsz = bitsz & bitmask;\
if (validbitsz == 0)\
validbitsz = bitmask + 1;\
uint32_t asz = ALIGN_SZA(bsz, msk);\
(dmactl).d->addr = sx_map_usrdatain((char *)(baddr), bsz);\
(dmactl).d->sz = asz | DMA_REALIGN;\
(dmactl).d->dmatag = tag | DMATAG_IGN((validbitsz - 1)); \
(dmactl).d++;\
#define ADD_INDESC_BITS(dmactl, baddr, bsz, tag, msk, bitsz) \
do { \
size_t bitmask = (msk << 3) | 0x7; \
size_t validbitsz = bitsz & bitmask; \
if (validbitsz == 0) \

Check warning on line 113 in subsys/nrf_security/src/drivers/cracen/sxsymcrypt/src/cmdma.h

View workflow job for this annotation

GitHub Actions / Run compliance checks on patch series (PR)

BRACES 

subsys/nrf_security/src/drivers/cracen/sxsymcrypt/src/cmdma.h:113  braces {} are required around if/while/for/else
validbitsz = bitmask + 1; \
uint32_t asz = ALIGN_SZA(bsz, msk); \
(dmactl).d->addr = sx_map_usrdatain((char *)(baddr), bsz); \
(dmactl).d->sz = asz | DMA_REALIGN; \
(dmactl).d->dmatag = tag | DMATAG_IGN((validbitsz - 1)); \
(dmactl).d++; \
} while (0)

#define SET_LAST_DESC_IGN(dmactl, bsz, msk) \
Expand Down Expand Up @@ -163,6 +163,11 @@
/** Start input/fetcher DMA at indescs and output/pusher DMA at outdescs */
void sx_cmdma_start(struct sx_dmactl *dma, size_t privsz, struct sxdesc *indescs);

#ifdef CONFIG_DCACHE
/** Flush and invalidate the buffers for the output descriptors */
void sx_cmdma_outdescs_flush_and_invd_dcache(const struct sx_dmactl *dma);
#endif

/** Return how the DMA is doing.
*
* Possible return values are:
Expand Down
5 changes: 3 additions & 2 deletions subsys/nrf_security/src/drivers/cracen/sxsymcrypt/src/hash.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,7 @@ static int start_hash_hw(struct sxhash *c)
{
sx_cmdma_start(&c->dma, sizeof(c->descs) + sizeof(c->extramem), c->descs);

return 0;
return SX_OK;
}

int sx_hash_save_state(struct sxhash *c)
Expand Down Expand Up @@ -289,7 +289,8 @@ int sx_hash_status(struct sxhash *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));
#endif

sx_hash_free(c);
Expand Down
3 changes: 2 additions & 1 deletion subsys/nrf_security/src/drivers/cracen/sxsymcrypt/src/mac.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,8 @@ int sx_mac_status(struct sxmac *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));
#endif

sx_mac_free(c);
Expand Down
Loading