Do not Review: Move CRACEN mutexes to threading_alt.c #18140

frkv · 2024-10-23T15:48:00Z

This PR changes CRACEN to use Mbed TLS threading primitives instead of directly calling nrf_security_mutex APIs.

This PR is not ready for review and is only used to run tests for some complex ABI compliance issues with OpenThread

NordicBuilder · 2024-10-23T15:51:02Z

CI Information

^{To view the history of this post, clich the 'edited' button above}
Build number: 3

Inputs:

Sources:

sdk-nrf: PR head: 71dd95feb7450e89892ad688e90c3ff4a8d10400
oberon-psa-crypto: PR head: bd78478f740abcb27521a0d06b8afaef4142416a

more details

sdk-nrf:

PR head: 71dd95feb7450e89892ad688e90c3ff4a8d10400
merge base: 6418bf3b95e22fffca8ecc7205182ffc8a90b0d1
target head (main): 6418bf3b95e22fffca8ecc7205182ffc8a90b0d1
Diff

oberon-psa-crypto:

PR head: bd78478f740abcb27521a0d06b8afaef4142416a
merge base: b41e899e7302462eb952b0b6a7c6903e368fb395
target head (main): b41e899e7302462eb952b0b6a7c6903e368fb395
Diff

Github labels

Enabled	Name	Description
	ci-disabled	Disable the ci execution
	ci-all-test	Run all of ci, no test spec filtering will be done
	ci-force-downstream	Force execution of downstream even if twister fails
	ci-run-twister	Force run twister
	ci-run-zephyr-twister	Force run zephyr twister

List of changed files detected by CI (17)

modules
│  ├── crypto
│  │  ├── oberon-psa-crypto
│  │  │  ├── library
│  │  │  │  ├── psa_crypto.c
│  │  │  │  │ psa_crypto_slot_management.c
subsys
│  ├── nrf_security
│  │  ├── Kconfig.psa
│  │  ├── cmake
│  │  │  │ psa_crypto_want_config.cmake
│  │  ├── configs
│  │  │  ├── psa_crypto_config.h.template
│  │  │  │ psa_crypto_want_config.h.template
│  │  ├── src
│  │  │  ├── drivers
│  │  │  │  ├── cracen
│  │  │  │  │  ├── cracenpsa
│  │  │  │  │  │  ├── src
│  │  │  │  │  │  │  ├── cracen.c
│  │  │  │  │  │  │  ├── ctr_drbg.c
│  │  │  │  │  │  │  ├── key_management.c
│  │  │  │  │  │  │  ├── kmu.c
│  │  │  │  │  │  │  │ prng_pool.c
│  │  │  │  │  ├── silexpk
│  │  │  │  │  │  ├── target
│  │  │  │  │  │  │  ├── baremetal_ba414e_with_ik
│  │  │  │  │  │  │  │  │ pk_baremetal.c
│  │  │  │  │  ├── sxsymcrypt
│  │  │  │  │  │  ├── src
│  │  │  │  │  │  │  ├── platform
│  │  │  │  │  │  │  │  ├── baremetal
│  │  │  │  │  │  │  │  │  │ cmdma_hw.c
│  │  │  ├── threading
│  │  │  │  ├── include
│  │  │  │  │  │ threading_alt.h
│  │  │  │  ├── threading.cmake
│  │  │  │  │ threading_alt.c
west.yml

Outputs:

Toolchain

Version: add720b6d9
Build docker image: docker-dtr.nordicsemi.no/sw-production/ncs-build:add720b6d9_912848a074

Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped; ⚠️ Quarantine

◻️ Toolchain - Skipped: existing toolchain is used
✅ Build twister
- sdk-nrf test count: 1466
❌ Integration tests
- ✅ test-fw-nrfconnect-chip
- ✅ test-fw-nrfconnect-nrf-iot_cloud
- ❌ test-fw-nrfconnect-nrf_crypto
- ✅ test-fw-nrfconnect-tfm
- ✅ test-sdk-find-my
- ✅ test-sdk-sidewalk
- ✅ test-sdk-dfu

Disabled integration tests

- desktop52_verification
- doc-internal
- test_ble_nrf_config
- test-fw-nrfconnect-apps
- test-fw-nrfconnect-ble_mesh
- test-fw-nrfconnect-ble_samples
- test-fw-nrfconnect-boot
- test-fw-nrfconnect-fem
- test-fw-nrfconnect-nfc
- test-fw-nrfconnect-nrf-iot_libmodem-nrf
- test-fw-nrfconnect-nrf-iot_lwm2m
- test-fw-nrfconnect-nrf-iot_mosh
- test-fw-nrfconnect-nrf-iot_nrf_provisioning
- test-fw-nrfconnect-nrf-iot_positioning
- test-fw-nrfconnect-nrf-iot_samples
- test-fw-nrfconnect-nrf-iot_serial_lte_modem
- test-fw-nrfconnect-nrf-iot_thingy91
- test-fw-nrfconnect-nrf-iot_zephyr_lwm2m
- test-fw-nrfconnect-proprietary_esb
- test-fw-nrfconnect-ps
- test-fw-nrfconnect-rpc
- test-fw-nrfconnect-rs
- test-fw-nrfconnect-thread
- test-fw-nrfconnect-zigbee
- test-low-level
- test-sdk-audio
- test-sdk-mcuboot
- test-sdk-pmic-samples
- test-sdk-wifi

Note: This message is automatically posted and updated by the CI

tomi-font · 2024-10-24T07:02:47Z

subsys/nrf_security/src/drivers/cracen/cracenpsa/src/cracen.c

+	__ASSERT(mbedtls_mutex_lock(&cracen_mutex) == 0,
+		"cracen_mutex not initialized (lock)");


Separate asserts from the actual action, so that the action is still performed even when asserts are disabled.
e.g.:

Suggested change

__ASSERT(mbedtls_mutex_lock(&cracen_mutex) == 0,

"cracen_mutex not initialized (lock)");

const int lock_ret = mbedtls_mutex_lock(&cracen_mutex);

__ASSERT(lock_ret == 0, "cracen_mutex not initialized (lock)");

tomi-font · 2024-10-24T07:03:42Z

subsys/nrf_security/src/drivers/cracen/cracenpsa/src/cracen.c

+	__ASSERT(mbedtls_mutex_unlock(&cracen_mutex) == 0,
+		"cracen_mutex not initialized (unlock)");


Are asserts on unlock actually needed? Because your assert message suggests that no (they both only fail if the mutex is not initialized). If yes, update the assert message.

tomi-font · 2024-10-24T07:05:01Z

subsys/nrf_security/src/drivers/cracen/cracenpsa/src/cracen.c

@@ -24,7 +24,7 @@

 static int users;

-NRF_SECURITY_MUTEX_DEFINE(cracen_mutex);
+extern mbedtls_threading_mutex_t cracen_mutex;


Could those be declared in a header file instead?

tomi-font · 2024-10-24T07:05:33Z

subsys/nrf_security/src/drivers/cracen/cracenpsa/src/prng_pool.c

@@ -24,13 +24,16 @@ static uint32_t prng_pool[PRNG_POOL_SIZE];
 static uint32_t prng_pool_remaining;




Suggested change

tomi-font · 2024-10-24T07:06:27Z

subsys/nrf_security/src/drivers/cracen/cracenpsa/src/prng_pool.c

+
+

Too many newlines in here.

tomi-font · 2024-10-24T07:08:34Z

subsys/nrf_security/src/drivers/cracen/silexpk/target/baremetal_ba414e_with_ik/pk_baremetal.c

@@ -111,15 +112,19 @@ void sx_pk_wrreg(struct sx_regs *regs, uint32_t addr, uint32_t v)
 	printk("sx_pk_wrreg(addr=0x%x, sum=0x%x, val=0x%x);\r\n", addr, (uint32_t)p, v);
 #endif

+	wmb(); /* comment for compliance */


This seems weird to me. wmb() and rmb() are defined as inline functions. What did compliance say here exactly?

-This commit ensures that the check if we are calling from ISR and/or kernel is used for CRACEN (which doesn't allow mutex-interaction). -This commit checks for failures to lock with asserts -Change cracen to use mbedtls_mutex_lock/unlock -Change cracen mutexs init to happen in threading_alt.c through the post-kernel SYS_INIT. Signed-off-by: Frank Audun Kvamtrø <[email protected]>

-Add memory barrier wmb() and rmb() around writes and reads for pk module (IKG) for cracen. Signed-off-by: Frank Audun Kvamtrø <[email protected]>

-This commit moves checks to ensure we aren't in pre_kernel/ISR into its own function to ensure that this can be compiled in TF-M which will be the case for CRACEN enabled devices. -Adding static function is_pre_kernel_or_isr which is checks if TF-M and MultiThreading is enabled Signed-off-by: Frank Audun Kvamtrø <[email protected]>

-This commit adds PSA_CRYPTO_THREAD_SAFE as a configuration that filters thread-safety enablement for PSA crypto APIs (front-end). -This commit adds a version of oberon-psa-crypto which supports PSA_CRYPTO_THREAD_SAFE (by updating the manifest) -This commit adds the Kconfig MBEDTLS_PSA_CRYPTO_DISABLE_THREAD_SAFETY which is used to ensure PSA_CRYPTO_THREAD_SAFE can be disabled if there is a wish to not enable thread-safety for the PSA crypto front-end APIs (separated from thread-safety for legacy Mbed TLS APIs and HW accelerated drivers. -This update includes a manifest-update to ensure that this change is bisectable. -Enable Mbed TLS threading APIs in nrf_security regardless if MBEDTLS_THREADING_C is enabled so they can be shared with cracen. -Add external prototypes for mbedtls_mutex_xxxx APIs that is used regardless if threading is actually enabled Note: This commit is done to try to resolve an ABI compliance issue with pre-compiled OpenThread libraries. Signed-off-by: Frank Audun Kvamtrø <[email protected]>

NordicBuilder · 2024-10-24T09:32:53Z

The following west manifest projects have been modified in this Pull Request:

Name	Old Revision	New Revision	Diff
oberon-psa-crypto	nrfconnect/sdk-oberon-psa-crypto@`b41e899` (main)	nrfconnect/sdk-oberon-psa-crypto#16	nrfconnect/sdk-oberon-psa-crypto#16/files

Note: This message is automatically posted and updated by the Manifest GitHub Action.

NordicBuilder · 2024-10-24T09:48:18Z

You can find the documentation preview for this PR at this link. It will be updated about 10 minutes after the documentation build succeeds.

Note: This comment is automatically posted by the Documentation Publishing GitHub Action.

github-actions · 2024-11-24T00:37:30Z

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.

frkv requested a review from a team as a code owner October 23, 2024 15:48

github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Oct 23, 2024

frkv force-pushed the mutex_move_to_threading branch from d8c120c to 92f399b Compare October 24, 2024 06:45

tomi-font requested changes Oct 24, 2024

View reviewed changes

frkv added 4 commits October 24, 2024 11:21

crypto: cracen: Ensure that register reads/writes are flushed for PK

0bbe275

-Add memory barrier wmb() and rmb() around writes and reads for pk module (IKG) for cracen. Signed-off-by: Frank Audun Kvamtrø <[email protected]>

frkv force-pushed the mutex_move_to_threading branch from 92f399b to 71dd95f Compare October 24, 2024 09:32

frkv requested review from a team as code owners October 24, 2024 09:32

github-actions bot added the manifest label Oct 24, 2024

NordicBuilder added manifest-oberon-psa-crypto DNM labels Oct 24, 2024

frkv changed the title ~~Move CRACEN mutexes to threading_alt.c~~ Do not Review: Move CRACEN mutexes to threading_alt.c Oct 24, 2024

github-actions bot added the Stale label Nov 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not Review: Move CRACEN mutexes to threading_alt.c #18140

Do not Review: Move CRACEN mutexes to threading_alt.c #18140

frkv commented Oct 23, 2024 •

edited

Loading

NordicBuilder commented Oct 23, 2024 •

edited

Loading

tomi-font Oct 24, 2024

tomi-font Oct 24, 2024

tomi-font Oct 24, 2024

tomi-font Oct 24, 2024

tomi-font Oct 24, 2024

tomi-font Oct 24, 2024

NordicBuilder commented Oct 24, 2024

NordicBuilder commented Oct 24, 2024

github-actions bot commented Nov 24, 2024

		__ASSERT(mbedtls_mutex_lock(&cracen_mutex) == 0,
		"cracen_mutex not initialized (lock)");

		__ASSERT(mbedtls_mutex_unlock(&cracen_mutex) == 0,
		"cracen_mutex not initialized (unlock)");

		@@ -24,13 +24,16 @@ static uint32_t prng_pool[PRNG_POOL_SIZE];
		static uint32_t prng_pool_remaining;

Do not Review: Move CRACEN mutexes to threading_alt.c #18140

Are you sure you want to change the base?

Do not Review: Move CRACEN mutexes to threading_alt.c #18140

Conversation

frkv commented Oct 23, 2024 • edited Loading

NordicBuilder commented Oct 23, 2024 • edited Loading

CI Information

Inputs:

Sources:

Github labels

Outputs:

Toolchain

Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped; ⚠️ Quarantine

tomi-font Oct 24, 2024

Choose a reason for hiding this comment

tomi-font Oct 24, 2024

Choose a reason for hiding this comment

tomi-font Oct 24, 2024

Choose a reason for hiding this comment

tomi-font Oct 24, 2024

Choose a reason for hiding this comment

tomi-font Oct 24, 2024

Choose a reason for hiding this comment

tomi-font Oct 24, 2024

Choose a reason for hiding this comment

NordicBuilder commented Oct 24, 2024

NordicBuilder commented Oct 24, 2024

github-actions bot commented Nov 24, 2024

frkv commented Oct 23, 2024 •

edited

Loading

NordicBuilder commented Oct 23, 2024 •

edited

Loading