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.

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

tests: Bluetooth: Mesh: use psa secure storage in mesh bsim tests #82319

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

alxelax
Copy link
Collaborator

@alxelax alxelax commented Nov 29, 2024

PR:

  1. Removes PSA ITS emulator in mesh bsim tests and enables secure storage
  2. Changes dependencies in secure storage to avoid circular dependencies if secure storage users select the same features

Copy link
Collaborator

@tomi-font tomi-font left a comment

Choose a reason for hiding this comment

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

Might also be worth a migration guide entry to indicate that it's now necessary to select the affected Kconfig options for the secure storage subsystem.

subsys/secure_storage/Kconfig.its_store Outdated Show resolved Hide resolved
subsys/secure_storage/Kconfig Outdated Show resolved Hide resolved
@@ -12,7 +12,7 @@ config SECURE_STORAGE_ITS_STORE_IMPLEMENTATION_SETTINGS
&& $(dt_node_has_compat,$(dt_node_parent,$(DT_STORAGE_PARTITION)),fixed-partitions)
imply FLASH_MAP
imply NVS
select SETTINGS
depends on SETTINGS
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will require changes to enable this (and FLASH) in the existing tests/samples under tests/subsys/secure_storage/ and samples/psa/.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd say rather add this as part of the already existing depends on, or split it into parts (e.g. putting FLASH_HAS_DRIVER_ENABLED separately), for consistency.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The Kconfig options still need to be enabled in the samples, I think.

Commit removes PSA ITS emulator and enables usage of
the PSA crypto storage instead.

Signed-off-by: Aleksandr Khromykh <[email protected]>
@alxelax alxelax force-pushed the remove_psa_its_emulator branch from 63bf345 to 8321d0b Compare December 3, 2024 10:51
@zephyrbot zephyrbot added the area: Samples Samples label Dec 3, 2024
@zephyrbot zephyrbot requested review from kartben and nashif December 3, 2024 10:52
@alxelax alxelax force-pushed the remove_psa_its_emulator branch from 8321d0b to 1bb8e85 Compare December 3, 2024 11:30
@zephyrbot zephyrbot added the Release Notes To be mentioned in the release notes label Dec 3, 2024
@alxelax alxelax force-pushed the remove_psa_its_emulator branch from 1bb8e85 to a1685a0 Compare December 3, 2024 11:46
samples/psa/its/overlay-secure_storage.conf Show resolved Hide resolved
@@ -12,7 +12,7 @@ config SECURE_STORAGE_ITS_STORE_IMPLEMENTATION_SETTINGS
&& $(dt_node_has_compat,$(dt_node_parent,$(DT_STORAGE_PARTITION)),fixed-partitions)
imply FLASH_MAP
imply NVS
select SETTINGS
depends on SETTINGS
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd say rather add this as part of the already existing depends on, or split it into parts (e.g. putting FLASH_HAS_DRIVER_ENABLED separately), for consistency.

doc/releases/migration-guide-4.1.rst Outdated Show resolved Hide resolved
doc/releases/migration-guide-4.1.rst Outdated Show resolved Hide resolved
doc/releases/migration-guide-4.1.rst Outdated Show resolved Hide resolved
* The :kconfig:option:`CONFIG_SECURE_STORAGE_ITS_STORE_IMPLEMENTATION_SETTINGS` symbol no longer implies the
:kconfig:option:`CONFIG_FLASH_MAP` and :kconfig:option:`CONFIG_NVS` Kconfig options.
It either no longer selects :kconfig:option:`CONFIG_SETTINGS` Kconfig option.
Platforms using Secure storage must explicitly enable :kconfig:option:`CONFIG_FLASH_MAP`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Using Secure storage with the settings-based ITS store module.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

didn't get this. How the final text should look like?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually I think it can be made as simple as the following as we previously refer to CONFIG_SECURE_STORAGE_ITS_STORE_IMPLEMENTATION_SETTINGS:

Suggested change
Platforms using Secure storage must explicitly enable :kconfig:option:`CONFIG_FLASH_MAP`,
Platforms using it must now explicitly enable :kconfig:option:`CONFIG_FLASH_MAP`,

doc/releases/release-notes-4.1.rst Outdated Show resolved Hide resolved
@alxelax alxelax force-pushed the remove_psa_its_emulator branch from a1685a0 to e3ee4e9 Compare December 3, 2024 15:07
@alxelax alxelax force-pushed the remove_psa_its_emulator branch 3 times, most recently from e0c5700 to 30d9af5 Compare December 4, 2024 15:21
The commit changes dependencies on SETTINGS and FLASH
to avoid circular dependencies if security storage
subsystem users select them too.

Signed-off-by: Aleksandr Khromykh <[email protected]>
@sjanc
Copy link
Collaborator

sjanc commented Dec 6, 2024

#AutoPTS run zephyr nrf52 MESH/NODE/KR/BV-01-C DFUM/SR/FD/BV-01-C

@codecoup-tester
Copy link

Scheduled PR #82319 (comment), board: nrf52, estimated start time: 14:43:31, test case count: 2, estimated duration: 0:04:49

Test cases to be runDFUM/SR/FD/BV-01-C
MESH/NODE/KR/BV-01-C

@codecoup-tester
Copy link

AutoPTS Bot results:

Failed tests (1)MESH MESH/NODE/KR/BV-01-C FAIL
Successful tests (1)DFUM DFUM/SR/FD/BV-01-C PASS

Comment on lines +11 to +12
filter: CONFIG_SECURE_STORAGE and CONFIG_SECURE_STORAGE_ITS_STORE_IMPLEMENTATION_SETTINGS
and CONFIG_FLASH_HAS_DRIVER_ENABLED
Copy link
Collaborator

Choose a reason for hiding this comment

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

CONFIG_SECURE_STORAGE_ITS_STORE_IMPLEMENTATION_SETTINGS takes care of CONFIG_FLASH_HAS_DRIVER_ENABLED:

Suggested change
filter: CONFIG_SECURE_STORAGE and CONFIG_SECURE_STORAGE_ITS_STORE_IMPLEMENTATION_SETTINGS
and CONFIG_FLASH_HAS_DRIVER_ENABLED
filter: CONFIG_SECURE_STORAGE and CONFIG_SECURE_STORAGE_ITS_STORE_IMPLEMENTATION_SETTINGS

Also, why change this at all?

Comment on lines +5 to +14
secure_storage.psa.crypto.secure_storage.settings:
filter: CONFIG_SECURE_STORAGE and CONFIG_SECURE_STORAGE_ITS_STORE_IMPLEMENTATION_SETTINGS
and CONFIG_FLASH_HAS_DRIVER_ENABLED
extra_args: EXTRA_CONF_FILE=overlay-secure_storage.conf;overlay-settings.conf
integration_platforms:
- native_sim
- nrf54l15dk/nrf54l15/cpuapp
secure_storage.psa.crypto.secure_storage.custom:
filter: CONFIG_SECURE_STORAGE and CONFIG_SECURE_STORAGE_ITS_STORE_IMPLEMENTATION_CUSTOM
and CONFIG_FLASH_HAS_DRIVER_ENABLED
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a copy/paste fail right?

@@ -12,7 +12,7 @@ config SECURE_STORAGE_ITS_STORE_IMPLEMENTATION_SETTINGS
&& $(dt_node_has_compat,$(dt_node_parent,$(DT_STORAGE_PARTITION)),fixed-partitions)
imply FLASH_MAP
imply NVS
select SETTINGS
depends on SETTINGS
Copy link
Collaborator

Choose a reason for hiding this comment

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

The Kconfig options still need to be enabled in the samples, I think.

Comment on lines +11 to +14
CONFIG_FLASH=y
CONFIG_FLASH_MAP=y
CONFIG_NVS=y
CONFIG_SETTINGS=y
Copy link
Collaborator

Choose a reason for hiding this comment

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

This shouldn't be here? Only in overlay-settings.conf.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants