Add pta self test for firmware handoff by raymo200915 · Pull Request #7352 · OP-TEE/optee_os

raymo200915 · 2025-04-04T14:27:29Z

Fix a pointer casting bug in transfer_list_add().

Add self tests for transfer list.
Adapt CFG_TRANSFER_LIST with its dependencies and add CFG_TRANSFER_LIST_TEST.

Xtest entry is added at:
OP-TEE/optee_test#788

Build with Transfer List enabled is added at:
OP-TEE/build#821

jforissier · 2025-04-09T07:39:34Z

For "core: kernel: fix bug in transfer_list_add" with the fixup:
Please add parentheses in the commit subject: transfer_list_add(), then:
Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>

jforissier · 2025-04-09T07:41:39Z

For commit "core: pta: add self tests for transfer list" with the fixups and Jens' comment addressed:
Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

etienne-lms · 2025-04-09T12:57:34Z

+	    tl->alignment != TRANSFER_LIST_INIT_MAX_ALIGN ||
+	    tl->size != sizeof(*tl) ||
+	    tl->max_size != TEST_TL_MAX_SIZE ||
+	    tl->flags != TL_FLAGS_HAS_CHECKSUM) {


Also check transfer_list_verify_checksum()?

etienne-lms · 2025-04-09T13:41:51Z

+$(eval $(call cfg-enable-all-depends,CFG_TRANSFER_LIST, \
+	 CFG_DT CFG_EXTERNAL_DT CFG_MAP_EXT_DT_SECURE))
+
+CFG_TRANSFER_LIST_TEST ?= $(CFG_TRANSFER_LIST)


A short description would be nice. Maybe tell that CFG_ENABLE_EMBEDDED_TESTS needs to be enabled to have the test embedded.

I guess you mean CFG_TEE_CORE_EMBED_INTERNAL_TESTS?

I can replace it with:

CFG_TRANSFER_LIST_TEST ?= $(call cfg-all-enabled,CFG_TRANSFER_LIST \ CFG_TEE_CORE_EMBED_INTERNAL_TESTS)

and move it under the line:

# Enable core self tests and related pseudo TAs CFG_TEE_CORE_EMBED_INTERNAL_TESTS ?= $(CFG_ENABLE_EMBEDDED_TESTS)

Then we don't need to add additional inline comments.

I guess you mean CFG_TEE_CORE_EMBED_INTERNAL_TESTS?

Yes, sorry.

I can replace it with:
(...)

Sound good to me, still with a short description for CFG_TRANSFER_LIST_TEST, like:
# Embed transfer list support self test when enabled

raymo200915 · 2025-04-09T20:09:55Z

All comments are addressed.
All commits are squashed and rebased.
Added tag Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org> to commit "core: kernel: fix bug in transfer_list_add()" and Acked-by: Jerome Forissier <jerome.forissier@linaro.org> to commit "core: pta: add self tests for transfer list"

jenswikl

Acked-by: Jens Wiklander <jens.wiklander@linaro.org>

etienne-lms

Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com> for commit
"core: kernel: fix bug in transfer_list_add()".

Nitpicking comment/question for commit "core: pta: add self tests for transfer list". Otherwise LGTM.

etienne-lms · 2025-04-10T08:19:14Z

+		tl_e = transfer_list_add(tl, tag_id, data_size, data);
+	else
+		tl_e = transfer_list_add_with_align(tl, tag_id, data_size, data,
+						    align);


Nitpicking: calling only transfer_list_add_with_align(..., align) should be enough.

Here is just to test different API entries.

etienne-lms · 2025-04-10T08:42:25Z

+	new_te_next = transfer_list_next(tl, tl_e);
+
+	/* skip the inserted void entry if it exists */
+	if (new_te_next->tag_id == TL_TAG_EMPTY)


Suggestion in case there are several empty slot + in case there is no next entry:

- if (new_te_next->tag_id == TL_TAG_EMPTY) + while (new_te_next && new_te_next->tag_id == TL_TAG_EMPTY)

Bythe way, how come new_te_next is not NULL when this function is called by transfer_list_tests() at line 215 to change TEST_TE3_ID data size.

Good question. When expanding the data region of an entry, it tries to end up with the address that fits the potential 'next' entry with the data alignment required, even when it is the last entry - That is the case you see in line 215, a void entry was added at the tail.
It is not bad but I think it is fair enough to check if it is the last one and avoid to filling the gap with the void entry.
I will add a small fix.

Fix the missing cast on the target address when doing memmove. Get the address of entry data via transfer_list_entry_data() instead of adding offset. Fixes: a122250 ("core: add transfer list API") Signed-off-by: Raymond Mao <raymond.mao@linaro.org> Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org> Acked-by: Jens Wiklander <jens.wiklander@linaro.org> Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com>

raymo200915 · 2025-04-10T18:40:04Z

All comments are addressed.
Tags added.
All commits are squashed and rebased.

jforissier

For commit "core: kernel: remove the last appended void transfer entry":

Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

transfer_list_set_data_size() appends a void entry for the following entries to meet the alignment requirement even when it is the last one, thus add a check before appending. Signed-off-by: Raymond Mao <raymond.mao@linaro.org> Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

Add self tests for transfer list. Adapt CFG_TRANSFER_LIST with its dependencies and add CFG_TRANSFER_LIST_TEST. Signed-off-by: Raymond Mao <raymond.mao@linaro.org> Acked-by: Jerome Forissier <jerome.forissier@linaro.org> Acked-by: Jens Wiklander <jens.wiklander@linaro.org>

raymo200915 · 2025-04-28T19:24:40Z

All tags are added.

jenswikl reviewed Apr 4, 2025

View reviewed changes

Comment thread core/kernel/transfer_list.c Outdated

jenswikl reviewed Apr 4, 2025

View reviewed changes

Comment thread mk/config.mk Outdated

Comment thread core/pta/tests/sub.mk Outdated

jenswikl reviewed Apr 7, 2025

View reviewed changes

Comment thread core/pta/tests/transfer_list.c Outdated

Comment thread core/pta/tests/transfer_list.c Outdated

Comment thread core/pta/tests/transfer_list.c Outdated

jforissier reviewed Apr 8, 2025

View reviewed changes

Comment thread core/pta/tests/invoke.c Outdated

raymo200915 force-pushed the fw_handoff_ut branch from d3a9cdb to 3e36539 Compare April 8, 2025 19:08

jenswikl reviewed Apr 9, 2025

View reviewed changes

Comment thread core/pta/tests/transfer_list.c Outdated

etienne-lms reviewed Apr 9, 2025

View reviewed changes

raymo200915 force-pushed the fw_handoff_ut branch from 2a51fbb to a62e15a Compare April 9, 2025 18:18

jenswikl reviewed Apr 10, 2025

View reviewed changes

etienne-lms reviewed Apr 10, 2025

View reviewed changes

raymo200915 force-pushed the fw_handoff_ut branch from a62e15a to ff5ac8c Compare April 10, 2025 16:31

jforissier reviewed Apr 28, 2025

View reviewed changes

jforissier mentioned this pull request Apr 28, 2025

qemu_v8: add build for enabling Firmware Handoff OP-TEE/build#821

Merged

raymo200915 added 2 commits April 28, 2025 12:22

raymo200915 force-pushed the fw_handoff_ut branch from ff5ac8c to 08d576f Compare April 28, 2025 19:22

jforissier mentioned this pull request May 5, 2025

ci: QEMUv8: check Firmware Handoff #7385

Merged

jforissier merged commit 76d920d into OP-TEE:master May 5, 2025
31 of 33 checks passed

Conversation

raymo200915 commented Apr 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jforissier commented Apr 9, 2025

Uh oh!

jforissier commented Apr 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raymo200915 Apr 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raymo200915 commented Apr 9, 2025

Uh oh!

jenswikl left a comment

Choose a reason for hiding this comment

Uh oh!

etienne-lms left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raymo200915 Apr 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raymo200915 commented Apr 10, 2025

Uh oh!

jforissier left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raymo200915 commented Apr 28, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

raymo200915 commented Apr 4, 2025 •

edited

Loading

raymo200915 Apr 9, 2025 •

edited

Loading

raymo200915 Apr 10, 2025 •

edited

Loading

jforissier left a comment •

edited

Loading