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

LTC refactor sub.mk #6786

Merged
merged 2 commits into from
Apr 24, 2024
Merged

LTC refactor sub.mk #6786

merged 2 commits into from
Apr 24, 2024

Conversation

jenswi-linaro
Copy link
Contributor

No description provided.

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.

Comment for commit "core: ltc: merge sub.mk's into a single sub.mk".

I see that some build directives are removed (-Wno-unused-variable, -Wno-unused-parameter). As for my review comment on Wno-declaration-after-statement, I'm fine with the removal but i think it should be stated in the commit message.

This change discards use of _CFG_CORE_LTC_CIPHER, _CFG_CORE_LTC_AUTHENC and _CFG_CORE_LTC_MAC. I suggest to also remove them from core/crypto.mk.

@@ -1,8 +1,6 @@
global-incdirs-y += include
global-incdirs-y += src/headers

cflags-lib-y += -Wno-declaration-after-statement
Copy link
Contributor

Choose a reason for hiding this comment

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

This change remove this global libtomcrypt build directive. I'm fine with that but I think it should be stated in the commit message.

cppflags-mpi_desc.c-y += -DMBEDTLS_ALLOW_PRIVATE_ACCESS
endif

srcs-y += tomcrypt.c

srcs-$(_CFG_CORE_LTC_AES_DESC) += src/ciphers/aes/aes.c
Copy link
Contributor

Choose a reason for hiding this comment

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

Prior this change, this source file was built only when _CFG_CORE_LTC_AES_ACCEL is disabled (core/lib/libtomcrypt/src/ciphers/aes/sub.mk).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll fix up "core: ltc: merge sub.mk's into a single sub.mk" to keep this, and then clean it up in "core: ltc: sub.mk: reorganize and simplify"


srcs-$(_CFG_CORE_LTC_ED25519) += src/pk/ec25519/ec25519_crypto_ctx.c
srcs-$(_CFG_CORE_LTC_ED25519) += src/pk/ec25519/ec25519_export.c
srcs-$(_CFG_CORE_LTC_ED25519) += src/pk/ec25519/tweetnacl.c
Copy link
Contributor

Choose a reason for hiding this comment

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

These 3 were built when at least one of _CFG_CORE_LTC_X25519 and _CFG_CORE_LTC_ED25519 was enabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, I'll fixup the commit to use _CFG_CORE_LTC_EC25519 for this.

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: ltc: sub.mk: reorganize and simplify":
Looks all good to me. Could apply my R-b tag but i'd rather wait for your feedback on my review comments on the 1st commit and maybe have a second look.

@jenswi-linaro
Copy link
Contributor Author

Update. I had to do it as a force push.

@jenswi-linaro
Copy link
Contributor Author

I have prepared an update of LTC that I'll create a PR for once this is merged

@jforissier
Copy link
Contributor

Nice!

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

@jenswi-linaro
Copy link
Contributor Author

Tag applied

@jenswi-linaro
Copy link
Contributor Author

@etienne-lms do you have any more 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.

All good to me.
Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com>

Merge all sub.mk's below core/lib/libtomcrypt/src at the end of
core/lib/libtomcrypt/sub.mk.

It gives an easier overview of what is compiled, but it also makes it
easier when syncing core/lib/libtomcrypt/src with LTC upstream since
it's out of the way.

Unused sub.mk's are removed.

Removes the now unused _CFG_CORE_LTC_CIPHER, _CFG_CORE_LTC_AUTHENC and
_CFG_CORE_LTC_MAC from core/crypto.mk.

The global LTC build flag -Wno-declaration-after-statement is removed and
only supplied to the few source files that need it.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>
Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com>
Reorganize the LTC sub.mk by collecting configuration and files in
groups by algorithm or feature.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>
Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com>
@jenswi-linaro
Copy link
Contributor Author

Tag applied.

@jforissier jforissier merged commit 5c4fcb7 into OP-TEE:master Apr 24, 2024
7 checks passed
@jenswi-linaro jenswi-linaro deleted the ltc_sub_mk branch April 24, 2024 11:25
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

3 participants