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

core: fixup of transfer list entry overriding #6461

Merged
merged 2 commits into from
Nov 20, 2023

Conversation

raymo200915
Copy link
Contributor

Expand the data size of DTB transfer list entry to the max allocable size to reserve sufficient space for new nodes.
This fixes a potential issue that the amended DTB transfer entry overrides other entries followed by, when inserting new nodes.

core/arch/arm/kernel/boot.c Show resolved Hide resolved
core/arch/arm/kernel/boot.c Outdated Show resolved Hide resolved
core/arch/arm/kernel/boot.c Outdated Show resolved Hide resolved
core/arch/arm/kernel/boot.c Show resolved Hide resolved
@raymo200915
Copy link
Contributor Author

All comments have been addressed.

core/arch/arm/kernel/boot.c Outdated Show resolved Hide resolved
core/arch/arm/kernel/boot.c Show resolved Hide resolved
@raymo200915 raymo200915 force-pushed the firmware_handoff_fix_dtb_max_sz branch from 275b0ae to a8b3df1 Compare November 16, 2023 16:16
@raymo200915
Copy link
Contributor Author

Separated commit for DT changes.
Squashed fixup commits.
All comments have been addressed.

@raymo200915
Copy link
Contributor Author

CI / make check (QEMUv8, BTI+MTE+PAC) (pull_request) failed due to a connection timeout when downloading the toolchain, looks not related to this PR.

Downloading arm-gnu-toolchain-11.3.rel1-x86_64-arm-none-linux-gnueabihf ...
Downloading arm-gnu-toolchain-11.3.rel1-x86_64-aarch64-none-linux-gnu ...
curl: (56) OpenSSL SSL_read: Connection timed out, errno 110
Download failed
make: *** [toolchain.mk:68: aarch64] Error 1
Error: Process completed with exit code 2.

@jforissier
Copy link
Contributor

@raymo200915 hopefully a transient network issue, I have restarted the job, thanks for the heads up

Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

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

For commit "core: fixup of transfer list entry overriding":
commit message should also tell that when transfer list is used, the FDT max size is given by the TL max_size field, not by the configuration switch CFG_DTB_MAX_SIZE.

core/arch/arm/kernel/boot.c Outdated Show resolved Hide resolved
@raymo200915 raymo200915 force-pushed the firmware_handoff_fix_dtb_max_sz branch from a8b3df1 to 720b7e4 Compare November 17, 2023 15:35
@raymo200915
Copy link
Contributor Author

raymo200915 commented Nov 17, 2023

For commit "core: fixup of transfer list entry overriding":
commit message should also tell that when transfer list is used, the FDT max size is given by the TL max_size field, not by the > configuration switch CFG_DTB_MAX_SIZE.

Fixed.

@raymo200915
Copy link
Contributor Author

Updated commit message of "core: fixup of transfer list entry overriding".
Addressed review comments.

Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

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

Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com> (for bith commits) with below comments addressed for the commit message on commit "core: fixup of transfer list entry overriding":

Header line: "core: arm: fixup of transfer list entry overriding"

Since this is a fix, could you add a Fixes: tag related to the TL integration commit?

Swap the 2 last paragraph. The last one refers to the 1st one:

 Expand the data size of DTB transfer list entry to the max allocable
 size to reserve sufficient space for new nodes.
+This fixes a potential issue that the amended DTB transfer entry
+overrides other entries followed by, when inserting new nodes.
+
 When CFG_TRANSFER_LIST is enabled, instead of CFG_DTB_MAX_SIZE,
 the DTB max size will be given by a calculation of the remaining space
 in the transfer list mapped memory.
-This fixes a potential issue that the amended DTB transfer entry
-overrides other entries followed by, when inserting new nodes.

Thanks

@raymo200915 raymo200915 force-pushed the firmware_handoff_fix_dtb_max_sz branch from 720b7e4 to f2bb551 Compare November 17, 2023 20:27
@raymo200915
Copy link
Contributor Author

Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com> (for bith commits) with below comments addressed for the commit message on commit "core: fixup of transfer list entry overriding":

Header line: "core: arm: fixup of transfer list entry overriding"

Since this is a fix, could you add a Fixes: tag related to the TL integration commit?

Swap the 2 last paragraph. The last one refers to the 1st one:

 Expand the data size of DTB transfer list entry to the max allocable
 size to reserve sufficient space for new nodes.
+This fixes a potential issue that the amended DTB transfer entry
+overrides other entries followed by, when inserting new nodes.
+
 When CFG_TRANSFER_LIST is enabled, instead of CFG_DTB_MAX_SIZE,
 the DTB max size will be given by a calculation of the remaining space
 in the transfer list mapped memory.
-This fixes a potential issue that the amended DTB transfer entry
-overrides other entries followed by, when inserting new nodes.

Thanks

Done. Thanks @etienne-lms

@raymo200915
Copy link
Contributor Author

Commit message updated.
Review tags added.

@jenswi-linaro
Copy link
Contributor

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

Add argument to function init_external_dt() to allow callers to specify
the maximum size of external DTB to be initialized.

Signed-off-by: Raymond Mao <raymond.mao@linaro.org>
Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com>
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
Expand the data size of DTB transfer list entry to the max allocable
size to reserve sufficient space for new nodes.
This fixes a potential issue that the amended DTB transfer entry
overrides other entries followed by, when inserting new nodes.

When CFG_TRANSFER_LIST is enabled, instead of CFG_DTB_MAX_SIZE,
the DTB max size will be given by a calculation of the remaining space
in the transfer list mapped memory.

Fixes: 6676372 ("core: add support for transfer list")
Signed-off-by: Raymond Mao <raymond.mao@linaro.org>
Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com>
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
@raymo200915 raymo200915 force-pushed the firmware_handoff_fix_dtb_max_sz branch from f2bb551 to b4b4fd8 Compare November 20, 2023 14:53
@raymo200915
Copy link
Contributor Author

Thanks @jenswi-linaro , review tags added.

@jforissier jforissier merged commit ab3536f into OP-TEE:master Nov 20, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants