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

Ftpm #7054

Closed
wants to merge 27 commits into from
Closed

Ftpm #7054

wants to merge 27 commits into from

Conversation

jenswi-linaro
Copy link
Contributor

@jenswi-linaro jenswi-linaro commented Sep 25, 2024

Imports OP-TEE fTPM TA into optee_os/ta/ftpm folder.
Based on https://github.com/microsoft/ms-tpm-20-ref and
https://github.com/zeschg/ms-tpm-20-ref.git branch feat_add_tee_crypto

Depends on OP-TEE/build#780 and OP-TEE/manifest#293

@jenswi-linaro jenswi-linaro marked this pull request as draft September 25, 2024 09:33
Copy link
Contributor Author

@jenswi-linaro jenswi-linaro 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 the commit "ftpm: import TEE crypto API wrappers"

Comment on lines 160 to 162
# define tpmHashStateCopy_SHA1 memcpy
# define tpmHashStateExport_SHA1 memcpy
# define tpmHashStateImport_SHA1 memcpy
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will not work as intended since only a TEE_OperationHandle is stored in the hash state. We'll be copying a pointer.
The GP TEE Internal Core API doesn't have a function to import or export the hash state. That's a problem since the TPM implementation seems to depend on this.

We will most likely need to use a different implementation for hashes.

@etienne-lms
Copy link
Contributor

Comments on the preliminary patches.

Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com> for commit
"mk/subdir.mk: refactor process-subdir-{srcs-y,gensrcs-helper}",
"ta_dev_kit.mk: use spec-srcs and spec-out-dir",
"mk: introduce global-incdirs_ext-y".

Some questions around commit "mk/subdir.mk: introduce srcs_ext-y and srcs_ext_base-y".

Copy link
Contributor

@jforissier jforissier left a comment

Choose a reason for hiding this comment

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

For "ta_dev_kit.mk: use spec-srcs and spec-out-dir":
Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

Copy link
Contributor

@jforissier jforissier left a comment

Choose a reason for hiding this comment

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

For "mk: introduce global-incdirs_ext-y":
Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>

Copy link
Contributor

@jforissier jforissier left a comment

Choose a reason for hiding this comment

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

For "mk/subdir.mk: introduce srcs_ext-y and srcs_ext_base-y":

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

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.

I forgot to post my comments. Here they are...

mk/subdir.mk Outdated

define process-subdir-srcs_ext-y
ifneq ($(filter /%,$(1)),)
$$(error Absolute path not supported for srcs_ext-y: $(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

closing parenthesis missing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I'll fix.

@@ -123,9 +144,13 @@ endif
# Process files in current directory
$$(foreach g, $$(gensrcs-y), $$(eval $$(call process-subdir-gensrcs-y,$$(g))))
$$(foreach s, $$(srcs-y), $$(eval $$(call process-subdir-srcs-y,$$(s))))
$$(foreach s, $$(srcs_ext-y), $$(eval $$(call \
process-subdir-srcs_ext-y,$$(s),$$(firstword $$(srcs_ext_base-y)))))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you use firstword on external source base directory?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is the best action if srcs_ext_base-y has more than one word? Error out or ignore it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder. It would be a programming error, indeed, and expended sources would not be build likely leading to some linkage error.
I guess ignoring it here is enough.

# $1 is local source file name
# $2 is output file name
# $3 is tree source file name
oname-$(sm)-$3 := $$(if $$(oname-$1-y),$(out-dir)/$(base-prefix)/$$(oname-$1-y),$2)
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is oname-<file-name>-y defined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's supposed to be set in the sub.mk file if used.

Copy link
Contributor

@etienne-lms etienne-lms Oct 18, 2024

Choose a reason for hiding this comment

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

Not easy to guess. If there was an example in the inline description above, it could maybe help.
(edited)

Copy link
Contributor

Choose a reason for hiding this comment

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

Discard my last comment. I missed to look at the commit message that defines how oname-<filename>-y should be used.

Copy link
Contributor

@jforissier jforissier left a comment

Choose a reason for hiding this comment

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

"mk/subdir.mk: refactor process-subdir-{srcs-y,gensrcs-helper}":

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

Copy link
Contributor

@b49020 b49020 left a comment

Choose a reason for hiding this comment

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

Just few overall code import comments.

ta/ftpm/Makefile Outdated

O ?= ../out/fTPM
WOLF_ROOT := ../../../../external/wolfssl/
TPM_ROOT := ../../../../
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose these are just leftovers, can be dropped?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I'll fix.


/* The TAFs ID implemented in this TA */
#define TA_FTPM_SUBMIT_COMMAND (0)
#define TA_FTPM_EMULATE_PPI (1)
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAICS, TA_FTPM_EMULATE_PPI isn't being used from Linux kernel driver point of view. Also, it seems that Physical Presence Interface (PPI) specification currently implemented for ACPI based systems. So we should rather drop corresponding source code import until we see a real use-case upstream for OP-TEE based DT systems.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The primary target is OP-TEE based DT systems and the only thing we can realistically test for. However, I'm not so keen on removing features from fTPM since this is supposed to be the new upstream for the OP-TEE TA replacing the old deprecated location.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree and the git history of unmodified fTPM source import should be easily retrievable. The features which can't be tested and an interface is provided to normal world looks like a security concern to me. To be able to deploy fTPM for production use-cases, we really need to keep required bits enabled. At the very least you can disable this unused fTPM invoke command.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense, I'll add a configuration variable.

@jenswi-linaro
Copy link
Contributor Author

We need to find a way of testing that this code works as intended. Both for the current use case, but also that the TPM does what it should with other currently unused interfaces or features.

@b49020
Copy link
Contributor

b49020 commented Oct 11, 2024

We need to find a way of testing that this code works as intended. Both for the current use case, but also that the TPM does what it should with other currently unused interfaces or features.

Agree, I think measured boot is something we should at-least enable for our Qemu setup in the CI based on fTPM. Apart from that we can write basic user-space tests using tpm2-tools to perform signing/verification. There is also tpm2-selftest (https://manpages.debian.org/testing/tpm2-tools/tpm2_selftest.1.en.html) which can be really handy for testing fTPM.

We can also create tests for tpm2 provider for openssl (https://github.com/tpm2-software/tpm2-openssl) which will directly relate to real world use-cases.

@jenswi-linaro
Copy link
Contributor Author

Thanks, I was looking for something like tpm2_selftest.

@jenswi-linaro
Copy link
Contributor Author

tpm2_selftest is passing and with logging enabled, it excercises the bignum implementation which is my greatest concern. I think we have what's needed to prepare for a proper review. My plan is as follows:

  1. apply tags already provided
  2. drop the hash part from "ftpm: import TEE crypto API wrappers"
  3. refactor "ftpm: import TEE crypto API wrappers" to better follow our coding style
  4. fix TODOs etc in "ftpm: import TEE crypto API wrappers"

This should make "ftpm: import TEE crypto API wrappers" easier to review.

Remove the custom alignment from TEE_BigIntFMMContext.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Enable SHA-384 and SHA-512 support for user TAs.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Moves the common parts of the two make macros process-subdir-srcs-y and
process-subdir-gensrcs-helper into a new macro, process-file-vars.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com>
Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>
The commit cfa34d9 ("Add support for compiling in-tree TAs") added
spec-srcs and spec-out-dir for special handling of user_ta_header.c when
compiling in-tree TAs.

However, these variables are just as relevant for out-of-tree TAs
compiled via ta/mk/ta_dev_kit.mk. So as a simplification switch to use
spec-srcs and spec-out-dir in that file too.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com>
Acked-by: Jerome Forissier <jerome.forissier@linaro.org>
Introduce the global-incdirs_ext-y variable to deal with including
header files from outside of this git (optee_os.git).

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com>
Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>
Introduce two new variables srcs_ext-y and srcs_ext_base-y to deal with
compiling source code outside of this git (optee_os.git).

srcs_ext_base-y assigns the root directory of the external source files
to compile. srcs_ext-y works as srcs-y except that it's relative to the
$(srcs_ext_base-y) directory.

Introduce the per source file variable oname-<file name>-y to override
the default output object file name. This helps to shorten and make a
more sane name for the output object file name when the source file is
outside optee_os source tree, for instance, a third-party library.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Acked-by: Jerome Forissier <jerome.forissier@linaro.org>
Import initial source from https://github.com/microsoft/ms-tpm-20-ref

copy initial source code from Samples/ARM32-FirmwareTPM/optee_ta/fTPM
commit e9fc7b89d865 ("Fix conflicting types for `ReadVarBytes`. (OP-TEE#102)")

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Import TEE crypto API wrappers from
https://github.com/zeschg/ms-tpm-20-ref/ from commit ea7f4b3c3f82
("feat: exchange wolfcrypt backend with op-tee crypto api") with the
exception of src/crypt/tee/TpmToTEEHash.c and
include/TEE/TpmToTEEHash.h.

Samples/ARM32-FirmwareTPM/optee_ta/fTPM/reference/include/TpmProfile.h to
ta/ftpm/reference/include/TpmProfile.h

Source directory TPMCmd/tpm to destination directory ta/ftpm
src/crypt/tee/TpmToTEEMath.c    -> tee/TpmToTEEMath.c
src/crypt/tee/TpmToTEESupport.c -> tee/TpmToTEESupport.c
src/crypt/tee/TpmToTEESym.c     -> tee/TpmToTEESym.c
include/TEE/TpmToTEEMath.h      -> include/TEE/TpmToTEEMath.h
include/TEE/TpmToTEESym.h       -> include/TEE/TpmToTEESym.h

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Refactors the imported TEE crypto API wrappers to better follow the
OP-TEE coding style.

On touched files, set Microsoft copyright year to 2018 based on git
history in the reference source. Add Linaro copyright.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Add and use hashlib wrappers for MbedTLS. Disabling ALG_SM3_256 since
it's not supported by MbedTLS.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Build the in-tree fTPM TA if CFG_MS_TPM_20_REF is supplied as a
non-empty path.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Add TA_FLAG_DEVICE_ENUM_TEE_STORAGE_PRIVATE to TA_FLAGS to enumerate the
TA once secure storage is available.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Removes the redundant or invalid s_NV* declarations

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
_plat__Fail() is declared with a __noreturn since it's guaranteed to not
return, but it only calls TEE_Panic() which doesn't have the same
attribute. TEE_Panic() does indeed never return so add a while(true)
after the TEE_Panic() so silence the warning.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Fix conflicting types for _plat__NvMemoryWrite() by adding the return
type used in the declaration. _plat__NvMemoryWrite() is updated to
always return TRUE since that's the expectation of success in NvWrite in
TPMCmd/tpm/src/subsystem/NvReserved.c in the reference implementation
(ms-tpm-20-ref).

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Remove the mismatching _plat__Signal_PowerOn() prototype from fTPM.h.
The real prototype is in ta/ftpm/platform/include/Platform_fp.h.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Remove the redefinition of TA_ALL_PARAM_TYPE() from fTPM.c, it's
originally defined in ta/ftpm/include/fTPM.h.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Move user_ta_header_defines.h to the common include directory for fTPM.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
lib/libutils/isoc/include/ctype.h already declares toupper() and
tolower() so remove the redundant declarations from RuntimeSupport.h.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Separate TA_FTPM_UUID and the command IDs into ftpm_ta.h. Update
user_ta_header_defines.h to include ftpm_ta.h instead of the old fTPM.h
to minimize the include file dependencies.

Set Microsoft copyright year to 2018 based on git history in the
reference source. Add missing SPDX-License-Identifier and Linaro
copyright.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Remove TpmProfile.h overriding the version in the reference source.

The old TpmProfile.h included a few .h files not included in the
reference TpmProfile.h, so add the missing includes the affected source
files.

Add SPDX-License-Identifier and add Linaro copyright for all modified
files. Set Microsoft copyright year based on git history in the
reference source.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Updates the fTPM sub.mk file to make it compile the source files.
Many warnings are disabled

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Remove the unused file RuntimeSupport.c.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Remove the now unused wolfssl crypto wrapper.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Remove the now unused sub.mk files.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Update the fTPM Makefile to support building externally. Remove WolfSSL
leftovers.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Add the configuration variable CFG_FTPM_EMULATE_PPI, default disabled,
to control whether PPI emulation is enabled.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
@jenswi-linaro
Copy link
Contributor Author

I've applied previous tags and squashed in updates.
The commit "ftpm: fixup TEE crypto API wrappers" is causing a bit of churn. We want to review the result of that fixup, but not necessarily the commit being fixed up. It might be interesting to see how the original code looked, but perhaps it's enough to look in https://github.com/zeschg/ms-tpm-20-ref/.
Should I squash "ftpm: fixup TEE crypto API wrappers" into "ftpm: import TEE crypto API wrappers" or keep it as is?

The commit "ta: ftpm: import initial source" is quite large and it's not reasonable to review it in detail. This code has been used for fTPM until now so we should be able to rely on previous reviews in that repository.

* The copyright in this software is being made available under the BSD
* License, included below. This software may be subject to other third
* party and contributor rights, including patent rights, and no such
* rights are granted under this license.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just drop the license text which is covered by SPDX-* here and other places? The copyrights should be retained though.

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 don't know if that's possible. The last time I checked, it deviated slightly from the license by SPDX-License-Identifier: BSD-2-Clause.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, we should have clarity regarding any deviation from standard BSD-2-Clause license with the authors. Otherwise it will be hard for users to understand licensing requirements.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IANAL but for reasonable and practical purposes this is a BSD-2-Clause. I don't know what

 * The copyright in this software is being made available under the BSD
 * License, included below. This software may be subject to other third
 * party and contributor rights, including patent rights, and no such
 * rights are granted under this license.

is supposed to add. Perhaps it's only a lawyer trying to motivate its existence by causing troubles like this.
However, removing the license below affects the meaning of this text.

I propose that we keep this for now and if there's interest later we can try to remove it. fTPM has already been used with this license so we're not changing anything.

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting and I see a corresponding patent already filed by Microsoft here: https://patents.google.com/patent/WO2013019369A1/en. IANAL too but this license text infers that there ins't any royalty free grant provided which is usually provided by a BSD-2-Clause Plus Patent License (https://spdx.org/licenses/BSD-2-Clause-Patent.html).

So I am not sure if bundling fTPM source within optee-os repo enforces that patent restriction on optee-os source/binary distribution as well. IMHO, we really need to seek lawyer's advice or consult with Microsoft. However, if we create a separate fTPM TA repo within OP-TEE github project then optee-os source may remain independent of any such restrictions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting and I see a corresponding patent already filed by Microsoft here: https://patents.google.com/patent/WO2013019369A1/en. IANAL too but this license text infers that there ins't any royalty free grant provided which is usually provided by a BSD-2-Clause Plus Patent License (https://spdx.org/licenses/BSD-2-Clause-Patent.html).

My interpretation of the quoted text is the opposite. They emphasize that they don't grant patent rights. But that's already implied by the BSD-2-Clause license, so the text is generally redundant.

So I am not sure if bundling fTPM source within optee-os repo enforces that patent restriction on optee-os source/binary distribution as well. IMHO, we really need to seek lawyer's advice or consult with Microsoft.

I'd say that it's not our problem.

However, if we create a separate fTPM TA repo within OP-TEE github project then optee-os source may remain independent of any such restrictions.

It does not make a difference from a license point of view if files are in the same repository.

Copy link
Contributor

Choose a reason for hiding this comment

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

My interpretation of the quoted text is the opposite.

Lets leave that interpretation for lawyers or better consult with Microsoft for clarification. @jan-kiszka might have some clarity on this since they may be enabling fTPM in their products.

So I am not sure if bundling fTPM source within optee-os repo enforces that patent restriction on optee-os source/binary distribution as well. IMHO, we really need to seek lawyer's advice or consult with Microsoft.

I'd say that it's not our problem.

It will become a problem for downstream (like OE distro etc.) consumers of optee-os where just a version uprev will add a possible license restriction even if they aren't enabling fTPM. We at least need to update optee-os license accordingly such that it becomes known to consumers.

However, if we create a separate fTPM TA repo within OP-TEE github project then optee-os source may remain independent of any such restrictions.

It does not make a difference from a license point of view if files are in the same repository.

It will make a difference for consumers who choose to not enable fTPM in their products due to license constraints.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will become a problem for downstream (like OE distro etc.) consumers of optee-os where just a version uprev will add a possible license restriction even if they aren't enabling fTPM. We at least need to update optee-os license accordingly such that it becomes known to consumers.

The main OP-TEE license is BSD-2-Clause, but there are exceptions. We don't have a uniform license that can be updated. We would in particular not want to add this since it only causes trouble when trying to classify the license we are just experiencing.

However, if we create a separate fTPM TA repo within OP-TEE github project then optee-os source may remain independent of any such restrictions.

It does not make a difference from a license point of view if files are in the same repository.

It will make a difference for consumers who choose to not enable fTPM in their products due to license constraints.

Why? If the files aren't used why does it matter if they exist?

Copy link
Contributor

Choose a reason for hiding this comment

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

I found an interesting read about "Patents in Open Source" here [1], please have a look at it. I hope it helps to clarify licensing concerns I am trying to raise here.

[1] https://google.github.io/opencasebook/patents/#express-but-non-specific-licenses

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is in my view only a concern for those using fTPM and there's nothing we can do about it other than to inform about the state. It's not the code that is the problem, but rather what it implements. Another fTPM implementation would also be affected by that patent.

@b49020
Copy link
Contributor

b49020 commented Oct 18, 2024

Should I squash "ftpm: fixup TEE crypto API wrappers" into "ftpm: import TEE crypto API wrappers" or keep it as is?

Yeah that makes sense to me.

The commit "ta: ftpm: import initial source" is quite large and it's not reasonable to review it in detail. This code has been used for fTPM until now so we should be able to rely on previous reviews in that repository.

That's reasonable, it's going to take time to make this fTPM import production ready with more stress testing and catching any corner cases. But we should go ahead with the initial merge after fixing obvious things.

@b49020
Copy link
Contributor

b49020 commented Oct 18, 2024

BTW, do you plan to add CI tests to OP-TEE CI for fTPM? tpm2_selftest can be a good candidate to start with.

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.

Regarding preparatory commits

  • Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com> for commits
    "tee_api_types.h: default alignment for TEE_BigIntFMMContext" and
    "libmbedtls: mbedtls_config_uta.h: enable SHA-384 and SHA-512 support".

  • Still few comments for commit "mk/subdir.mk: introduce srcs_ext-y and srcs_ext_base-y".

@@ -123,9 +144,13 @@ endif
# Process files in current directory
$$(foreach g, $$(gensrcs-y), $$(eval $$(call process-subdir-gensrcs-y,$$(g))))
$$(foreach s, $$(srcs-y), $$(eval $$(call process-subdir-srcs-y,$$(s))))
$$(foreach s, $$(srcs_ext-y), $$(eval $$(call \
process-subdir-srcs_ext-y,$$(s),$$(firstword $$(srcs_ext_base-y)))))
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder. It would be a programming error, indeed, and expended sources would not be build likely leading to some linkage error.
I guess ignoring it here is enough.

# $1 is local source file name
# $2 is output file name
# $3 is tree source file name
oname-$(sm)-$3 := $$(if $$(oname-$1-y),$(out-dir)/$(base-prefix)/$$(oname-$1-y),$2)
Copy link
Contributor

@etienne-lms etienne-lms Oct 18, 2024

Choose a reason for hiding this comment

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

Not easy to guess. If there was an example in the inline description above, it could maybe help.
(edited)

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 commit
"mk/subdir.mk: introduce srcs_ext-y and srcs_ext_base-y".

# $1 is local source file name
# $2 is output file name
# $3 is tree source file name
oname-$(sm)-$3 := $$(if $$(oname-$1-y),$(out-dir)/$(base-prefix)/$$(oname-$1-y),$2)
Copy link
Contributor

Choose a reason for hiding this comment

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

Discard my last comment. I missed to look at the commit message that defines how oname-<filename>-y should be used.

@jenswi-linaro
Copy link
Contributor Author

BTW, do you plan to add CI tests to OP-TEE CI for fTPM? tpm2_selftest can be a good candidate to start with.

We should do something to interact with fTPM. It seems that it does some self-testing when being probed so just checking for the device might be enough. On the other hand, we may want to have the tpm2-tools in the rootfs regardless so we might as well use tpm2_selftest.

@jenswi-linaro
Copy link
Contributor Author

We have decided to keep fTPM in a separate git instead. #7090 and OP-TEE/optee_ftpm#1 replace this PR.

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.

4 participants