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: arm: runtime check for CE #6645

Merged
merged 7 commits into from
Feb 27, 2024
Merged

Conversation

igoropaniuk
Copy link
Contributor

Add runtime check during boot for Crypto Extensions if CFG_CRYPTO_WITH_CE=y.

@igoropaniuk igoropaniuk force-pushed the ce_check branch 5 times, most recently from 90df954 to effbf49 Compare January 27, 2024 21:39
@igoropaniuk igoropaniuk force-pushed the ce_check branch 3 times, most recently from 018564a to d4cf61c Compare January 28, 2024 08:28
@jforissier
Copy link
Contributor

"core: arm: add masks for ID_ISAR5 fields":

Reviewed-by: Jerome Forissier <[email protected]>

core/arch/arm/kernel/boot.c Outdated Show resolved Hide resolved
core/arch/arm/kernel/boot.c Outdated Show resolved Hide resolved
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.

Comments on "core: arm: add helper for reading isar5".

core/arch/arm/include/arm32.h Outdated Show resolved Hide resolved
core/arch/arm/include/arm64.h Outdated Show resolved Hide resolved
@igoropaniuk igoropaniuk force-pushed the ce_check branch 2 times, most recently from 559adf6 to a52c9df Compare January 29, 2024 15:04
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.

"lib: libutee: define read function for id_isar5" should be "core: arm64: define read function for id_isar5". With that, this commit is:

Reviewed-by: Jerome Forissier <[email protected]>

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.

Typo in commit message for "core: arm64: add masks for ID_AA64ISAR0_EL1 fields":

-RDM    -   [31:68]
+RDM    -   [31:28]

For commit "core: arm: kernel: add runtime check for CE": maybe explicitly say in the commit message for core will panic if configuration enables an Arm CE feature that the hardware does not support.

Aside the minor comments:
Acked-by: Etienne Carriere <[email protected]>

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

@jforissier @jenswi-linaro @etienne-lms addressed all comments, applied Etienne's A-b tag

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.

Some typos i've missed.

core/arch/arm/include/arm64.h Outdated 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
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 "core: arm64: add masks for ID_AA64ISAR0_EL1 fields" with Etienne's comment addressed:
    Acked-by: Jerome Forissier <[email protected]>
  • For "core: arm: add helper functions for checking CE support":
    Reviewed-by: Jerome Forissier <[email protected]>

More comments below.

core/arch/arm/include/arm.h 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 Outdated Show resolved Hide resolved
@igoropaniuk igoropaniuk force-pushed the ce_check branch 2 times, most recently from 6a2d5ac to d195d5c Compare February 21, 2024 14:01
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.

  • Commit "core: arm: rewrite feat_crc32_implemented":
    Add parentheses after function name: feat_crc32_implemented() in subject and commit description.
    With that:
    Reviewed-by: Jerome Forissier <[email protected]>
  • Commit "core: arm64: remove ID_AA64ISAR0_EL1 macros":
    Reviewed-by: Jerome Forissier <[email protected]>
  • Commit "core: arm: add check in aarch32 for feat_crc32_implemented":
    Add missing parentheses, then:
    Reviewed-by: Jerome Forissier <[email protected]>
  • Commit "core: arm: add helper functions for checking CE support":
    Reviewed-by: Jerome Forissier <[email protected]>

Plus one comment on the last commit. Thanks!

core/arch/arm/kernel/boot.c Show resolved Hide resolved
@jforissier
Copy link
Contributor

For "core: arm: kernel: add runtime check for CE":
Reviewed-by: Jerome Forissier <[email protected]>

@igoropaniuk
Copy link
Contributor Author

@jforissier tags applied

@igoropaniuk igoropaniuk force-pushed the ce_check branch 2 times, most recently from b170fc4 to 895ed02 Compare February 22, 2024 10:53
@igoropaniuk
Copy link
Contributor Author

rebased onto the latest master

@igoropaniuk
Copy link
Contributor Author

@jenswi-linaro @jforissier @etienne-lms any additional comments/objections?

@jforissier
Copy link
Contributor

@jenswi-linaro @jforissier @etienne-lms any additional comments/objections?

The Xen CI job has failed, I have restarted it but I suspect there might be an issue with the access to the config registers...

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.

Look all good to me.
My R-b tag for the commits I've not given a review tag yet, FWIW (@jforissier, please merge staright, you've already acked these and that's far enough)

@jforissier
Copy link
Contributor

@etienne-lms I will give @jenswi-linaro the opportunity to comment further since he has reviewed already.

@jenswi-linaro
Copy link
Contributor

Acked-by: Jens Wiklander <[email protected]>

Add masks for obtaining Crypto Extensions support status from
ID_ISAR5_EL1 register:

Algo       Bits
CRC32  -   [19:16]
SHA2   -   [15:12]
SHA1   -   [11:8]
AES    -   [7:4]

For additional details check ARM Architecture Reference Manual
for ARMv8-A architecture profile.
D10.2.66 ID_ISAR5_EL1, AArch32 Instruction Set Attribute Register 5

Reviewed-by: Jerome Forissier <[email protected]>
Acked-by: Etienne Carriere <[email protected]>
Acked-by: Jens Wiklander <[email protected]>
Signed-off-by: Igor Opaniuk <[email protected]>
Add masks for obtaining Crypto Extensions support status from
ID_AA64ISAR0_EL1 register:

Algo       Bits
SM4    -   [43:40]
SM3    -   [39:36]
SHA3   -   [35:32]
RDM    -   [31:28]
TME    -   [27:24]
Atomic -   [23:20]
CRC32  -   [19:16]
SHA2   -   [15:12]
SHA1   -   [11:8]
AES    -   [7:4]

For additional details check ARM Architecture Reference Manual
for ARMv8-A architecture profile.
ID_AA64ISAR0_EL1, AArch64 Instruction Set Attribute Register 0.

Acked-by: Etienne Carriere <[email protected]>
Acked-by: Jerome Forissier <[email protected]>
Acked-by: Jens Wiklander <[email protected]>
Signed-off-by: Igor Opaniuk <[email protected]>
Rewrite check in feat_crc32_implementedfor for ARM64.

Reviewed-by: Jerome Forissier <[email protected]>
Acked-by: Jens Wiklander <[email protected]>
Signed-off-by: Igor Opaniuk <[email protected]>
Remove old definitions for ID_AA64ISAR0_EL1 CRC32 bitmask
and shift.

Reviewed-by: Jerome Forissier <[email protected]>
Acked-by: Jens Wiklander <[email protected]>
Signed-off-by: Igor Opaniuk <[email protected]>
Add support for checking CRC32 HW instruction in aarch32.

Reviewed-by: Jerome Forissier <[email protected]>
Acked-by: Jens Wiklander <[email protected]>
Signed-off-by: Igor Opaniuk <[email protected]>
Add helper functions for checking implementation of SHA1, SHA256,
SHA512, SHA3, SM3, SM4 instructions.

Acked-by: Etienne Carriere <[email protected]>
Reviewed-by: Jerome Forissier <[email protected]>
Acked-by: Jens Wiklander <[email protected]>
Signed-off-by: Igor Opaniuk <[email protected]>
Add runtime check during boot for supported ARMv8 Crypto Extensions.
Core will panic if configuration enables an ARMv8 CE feature
that the hardware does not support.

Link: OP-TEE#6631
Acked-by: Etienne Carriere <[email protected]>
Reviewed-by: Jerome Forissier <[email protected]>
Acked-by: Jens Wiklander <[email protected]>
Signed-off-by: Igor Opaniuk <[email protected]>
@igoropaniuk
Copy link
Contributor Author

igoropaniuk commented Feb 27, 2024

@jenswi-linaro tag applied, thanks

@jforissier
Copy link
Contributor

Thanks @igoropaniuk!

@jforissier jforissier merged commit 7f124eb into OP-TEE:master Feb 27, 2024
7 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.

4 participants