Securestorage #95799
Securestorage #95799
Conversation
zephyrbot
requested review from
anangl,
jaz1-nordic,
kl-cruz,
magp-nordic,
masz-nordic and
nika-nordic
| config GEN_UICR_SECURESTORAGE | ||
| bool "Enable UICR.SECURESTORAGE" | ||
| help |
There was a problem hiding this comment.
Should this have
depends on $(dt_nodelabel_enabled,secure_storage_partition)
or in some other way indicate dependence on the devicetree that is used by the cmake?
SebastianBoe
force-pushed
the
securestorage
branch
from
64cdd50 to
7ba1495
Compare
zephyrbot
added
area: Continuous Integration
area: Coding Guidelines
zephyrbot
requested review from
carlescufi,
fabiobaltieri,
jfischer-no,
kartben,
keith-zephyr,
nashif,
simhein and
stephanosio
SebastianBoe
force-pushed
the
securestorage
branch
from
7ba1495 to
1908a06
Compare
soc/nordic/common/uicr/gen_uicr.py
Outdated
| validate_secure_storage_partitions( | ||
| args.securestorage_address, | ||
| args.cpuapp_crypto_address, | ||
| args.cpuapp_crypto_size, | ||
| args.cpurad_crypto_address, | ||
| args.cpurad_crypto_size, | ||
| args.cpuapp_its_address, | ||
| args.cpuapp_its_size, | ||
| args.cpurad_its_address, | ||
| args.cpurad_its_size, | ||
| ) |
There was a problem hiding this comment.
Maybe validate_secure_storage_partitions should just take args directly? That would help avoid some boilerplate and potential for error with the order these are passed in here.
| else() | ||
| # Partition not found - set to zero | ||
| set(${output_address_var} 0 PARENT_SCOPE) | ||
| set(${output_size_var} 0 PARENT_SCOPE) | ||
| endif() |
There was a problem hiding this comment.
Tolerating this for any partition we use this function on instead of erroring out seems like it could lead to some hard to find bugs, e.g. if you misspelled 'periphconf_partition' then suddenly the python script might try to populate a periphconf at address 0 with size 0.
How about adding an argument to the function that lets us enable this fallback only for the partitions we want it on, or at least asserting outside the function that the nodes we wouldn't want this behavior for exist?
| The following device tree partitions are used in order: | ||
| - cpuapp_crypto_partition: Application processor crypto storage | ||
| - cpurad_crypto_partition: Radio core crypto storage | ||
| - cpuapp_its_partition: Application processor internal trusted storage | ||
| - cpurad_its_partition: Radio core internal trusted storage |
There was a problem hiding this comment.
The secure_storage_partition itself is also used
soc/nordic/common/uicr/gen_uicr.py
Outdated
| # Expected order: cpuapp_crypto_partition, cpurad_crypto_partition, | ||
| # cpuapp_its_partition, cpurad_its_partition | ||
| partitions = [ | ||
| (cpuapp_crypto_address, cpuapp_crypto_size, "cpuapp_crypto_partition"), | ||
| (cpurad_crypto_address, cpurad_crypto_size, "cpurad_crypto_partition"), | ||
| (cpuapp_its_address, cpuapp_its_size, "cpuapp_its_partition"), | ||
| (cpurad_its_address, cpurad_its_size, "cpurad_its_partition"), | ||
| ] |
There was a problem hiding this comment.
Could consider defining a simple class for these e.g. with dataclass / NamedTuple /namedtuple, that would avoid all the tuple unpacking below here as we could just use attribute access.
soc/nordic/common/uicr/gen_uicr.py
Outdated
| for i in range(len(present_partitions) - 1): | ||
| _, curr_addr, curr_size, curr_name = present_partitions[i] | ||
| _, next_addr, _________, next_name = present_partitions[i + 1] |
There was a problem hiding this comment.
Could be a use case for itertools.pairwise maybe?
SebastianBoe
force-pushed
the
securestorage
branch
2 times, most recently
from
07a4bee to
cbc86e0
Compare
Update the default memory map to include a `secure_storage_partition`, which is divided into at most four subpartitions. These will be used to configure UICR.SECURESTORAGE, if enabled. Signed-off-by: Sebastian Bøe <[email protected]>
Add UICR.SECURESTORAGE configuration based on device tree partitions. Validates partition layout and populates size fields in 1KB units. Handles missing partitions gracefully. Signed-off-by: Sebastian Bøe <[email protected]>
SebastianBoe
force-pushed
the
securestorage
branch
from
cbc86e0 to
0c1789d
Compare
|
Add UICR.SECURESTORAGE configuration based on device tree partitions.
Validates partition layout and populates size fields in 1KB units.
Handles missing partitions gracefully.
Update the default memory map to include a
secure_storage_partition,which is divided into at most four subpartitions. These will be used to
configure UICR.SECURESTORAGE, if enabled.