subsys: sdfw services: add device information service.

[ayla-nordicsemi]

Enable reading device information (UUID for instance) as a service requested by a client and executed by the secure domain.

[CLAassistant]

All committers have signed the CLA.

[NordicBuilder]

CI Information

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

Inputs:

Sources:

sdk-nrf: PR head: 5f25abc9c0584ead37b7f90d470ddc050f2c3003

more details

sdk-nrf:

PR head: 5f25abc9c0584ead37b7f90d470ddc050f2c3003
merge base: 46924418b10722a4c4084220151f16b6ca72e3df
target head (main): 4989dd10e20f684616dda81e4b9bb41bea81154c
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 (13)

include
│  ├── sdfw
│  │  ├── sdfw_services
│  │  │  │ device_info_service.h
subsys
│  ├── sdfw_services
│  │  ├── services
│  │  │  ├── CMakeLists.txt
│  │  │  ├── Kconfig
│  │  │  ├── device_info
│  │  │  │  ├── CMakeLists.txt
│  │  │  │  ├── Kconfig
│  │  │  │  ├── device_info_service.c
│  │  │  │  ├── device_info_service.cddl
│  │  │  │  ├── zcbor_generated
│  │  │  │  │  ├── CMakeLists.txt
│  │  │  │  │  ├── device_info_service_decode.c
│  │  │  │  │  ├── device_info_service_decode.h
│  │  │  │  │  ├── device_info_service_encode.c
│  │  │  │  │  ├── device_info_service_encode.h
│  │  │  │  │  │ device_info_service_types.h

Outputs:

Toolchain

Version: b77d8c1312
Build docker image: docker-dtr.nordicsemi.no/sw-production/ncs-build:b77d8c1312_912848a074

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

• ◻️ Toolchain - Skipped: existing toolchain is used
• ✅ Build twister
  • sdk-nrf test count: 141
• ✅ Integration tests
  • ✅ test-sdk-dfu
  • ⚠️ 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-chip
  • 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-nrf_crypto
  • test-fw-nrfconnect-ps
  • test-fw-nrfconnect-rpc
  • test-fw-nrfconnect-rs
  • test-fw-nrfconnect-tfm
  • test-fw-nrfconnect-thread
  • test-fw-nrfconnect-zigbee
  • test-low-level
  • test-sdk-audio
  • test-sdk-find-my
  • test-sdk-mcuboot
  • test-sdk-pmic-samples
  • test-sdk-sidewalk
  • test-sdk-wifi
  • test-secdom-samples-public

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

[NordicBuilder]

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 Publish GitHub Action.

[parttimaa]

Is there some reason for having the bigger than or equal >= instead of equals ==?` I'd assume UUID is always fixed in length.

[ayla-nordicsemi]

the read buffer and its size are provided by the user, it is okay to provide bigger than necessary buffer. In any case, the API will ALWAYS copy only 16 bytes to the output since, as you mentioned, the UUID has a fixed size.

[jonathannilsen]

I guess uuid_words_count could also be omitted from the API then, since the length is known and fixed? Maybe it would be better to define a struct with a fixed length array member? For example:

struct ssf_device_info_uuid {
    uint8_t value[UUID_BYTES_LENGTH];
};

int ssf_device_info_get_uuid(struct ssf_device_info_uuid *uuid);

Or if you agree with my suggestion below (#19278 (comment)) of combining all the device info reads into one request, you could make a single struct for this, for example

struct ssf_device_info {
    uint8_t uuid[16];
    uint32_t type;
    uint8_t testimprint[32];
    uint32_t partno;
    uint32_t hwrevision;
    uint32_t productionrevision;
};

int ssf_device_info_get(struct ssf_device_info *info);

[ayla-nordicsemi]

done, I've reworked the generated files and the service API.
now the device info handler on the secure side returns all the OICR register content at once in the device_info structure, the device_info.h header offers an API that assumes the uuid_buffer size to be 16 bytes.

[nordicjm]

declare variables at top of functions

[ayla-nordicsemi]

fixed.

[jonathannilsen]

I guess uuid_words_count could also be omitted from the API then, since the length is known and fixed? Maybe it would be better to define a struct with a fixed length array member? For example:

struct ssf_device_info_uuid {
    uint8_t value[UUID_BYTES_LENGTH];
};

int ssf_device_info_get_uuid(struct ssf_device_info_uuid *uuid);

Or if you agree with my suggestion below (#19278 (comment)) of combining all the device info reads into one request, you could make a single struct for this, for example

struct ssf_device_info {
    uint8_t uuid[16];
    uint32_t type;
    uint8_t testimprint[32];
    uint32_t partno;
    uint32_t hwrevision;
    uint32_t productionrevision;
};

int ssf_device_info_get(struct ssf_device_info *info);

[jonathannilsen]

As mentioned above, I think it would be preferable if each of the returned data types had a well defined type in this schema. Currently the length of the responses is not enforced by the protocol, and the lengths are not actually defined anywhere except for in a macro internal to device_info_service.c.

Unless there is a good reason to read/write the individual parts of the data you could consider combining them in a single request - that might help keep the client code size low as well as reduce the IPC overhead if you need to request multiple of these in a row. I think that would be pretty reasonable since the full data would be 64 bytes total. For example:

device_info = [
    uuid: bstr .size 16,
    type: uint,
    testimprint: bstr .size 32,
    partno: uint,
    hwrevision: uint,
    productionrevision: uint,
]

[ayla-nordicsemi]

thank you for suggesting this !
it required some dipper changes to the handler / service files but it is now done.

[parttimaa]

I don't understand why we need to cast to u8 pointers as the C standard function API has them as void pointers?
Memcpy function call returns void pointer, so the return value could be explicitly casted to void to show the intent of programmer.
https://en.cppreference.com/w/c/string/byte/memcpy

But nevertheless this code probably works as is. Might be something that static code analysis would nag about.