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.

Sign up for GitHub

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

Jump to bottom

Updates to ELE driver on i.MX platforms #7268

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Updates to ELE driver on i.MX platforms #7268

wants to merge 13 commits into from

Conversation

sahilnxp
Copy link
Contributor

@sahilnxp sahilnxp commented Feb 8, 2025 •
edited
Loading

This PR is doing multiple changes in ELE driver wrt

  • Moving of ELE driver in a separate folder under driver/crypto/ele
  • Introduction of Memory management functions to be used in driver
  • Multiple command formats changes
  • Enablement of TRUST_MU on i.MX91 and i.MX93
  • Get random number from ELE

I have put all these changes in a single PR.
Please let me know if need to be break it into multiple PRs

CC: @saschahauer
#7256

@sahilnxp sahilnxp force-pushed the github_master_ele_update branch 2 times, most recently from ad1a28e to 45d3bf0 Compare February 9, 2025 11:21
saschahauer
saschahauer reviewed Feb 10, 2025
View reviewed changes
sahilnxp and others added 4 commits February 12, 2025 19:58
@sahilnxp
drivers: ele: move ELE to a dedicated directory
289bd5c 
Created a new folder in core/drivers/crypto named ele
and moved ele.c in that folder.
This is done for making the base for further crypto driver
based on ELE.

Signed-off-by: Sahil Malhotra <[email protected]>
@sahilnxp
drivers: ele: getting common macros and functions in header file
0039ecf 
Taking out macros and functions from c file and put them in
header file for being used by the other files of crypto driver.

Signed-off-by: Sahil Malhotra <[email protected]>
@sahilnxp
drivers: ele: add memory management functions
bc5ed99 
Add memory management function

Signed-off-by: Sahil Malhotra <[email protected]>
@clementfaure @sahilnxp
drivers: ele: disable ASLR for imx8ulp
698ef85 
On imx8ulp, the RNG code from ELE is not available at resume. Disable
the ASLR feature and make it available for imx93 only.

Signed-off-by: Clement Faure <[email protected]>
Signed-off-by: Sahil Malhotra <[email protected]>
@sahilnxp sahilnxp force-pushed the github_master_ele_update branch 2 times, most recently from 0f196e5 to 1a488df Compare February 12, 2025 19:02
@sahilnxp
Copy link
Contributor Author

PR updated with comments addressed.

@sahilnxp sahilnxp force-pushed the github_master_ele_update branch from 1a488df to b589598 Compare February 14, 2025 11:05
@sahilnxp
Copy link
Contributor Author

Any more comments on this one ?

@jforissier
Copy link
Contributor

Reviewing this makes me realize we might not have made the best choices regarding the file paths (sources and includes) for the crypto drivers. I think there is no need for a new include directory for each crypto IP, and having qualified paths in the #include <...> would be more clear. I mean #include <drivers/crypto/imx/ele.h rather than #include <imx_ele.h>. Keeping imx in the path because it is not obvious that ele is imx and also to better organize the code (imx/ele, imx/caam...).
So I would suggest the following paths under core/ (all sources under drivers, all public headers under include/drivers):

  • C files: drivers/crypto/imx/ele/ele.c, drivers/crypto/imx/ele/*.c etc.
  • Header(s): include/drivers/crypto/imx/ele.h, used as #include <drivers/crypto/imx/ele.h>
    ...instead of what is proposed here (all source under drivers, some public headers under include/drivers, some others under drivers/something/include):
  • C files: drivers/crypto/ele/ele.c, drivers/crypto/ele/*.c
  • Headers(s): drivers/crypto/ele/include/ele.h

Applying the same to CAAM: ./drivers/crypto/caam/include/caam_cipher.h could become ./include/drivers/crypto/caam/cipher.h thus avoiding the double caam in the path and the extra include folder. It would be used as #include <drivers/crypto/caam/cipher.h> instead of #include <caam_cipher.h>. Many C files could also drop the caam_ prefix (./drivers/crypto/caam/acipher/caam_rsa.c -> ./drivers/crypto/caam/acipher/rsa.c etc.).

I know this is mostly cosmetic but as new drivers are introduced the older ones serve as templates so if we're not comfortable with something we should change it, otherwise it will get worse with time.

What do others think? We could also decide to rework this later. This is all very much internal to the OP-TEE tree anyways,

One more related thing: why export utils_mem.h ? It seems it could be private to the ELE driver in drivers/crypto/imx/ele/ and used with a local #include "..." by ele.c etc. And memutils.h sounds better IMO.

@jenswi-linaro
Copy link
Contributor

The proposed paths look good to me. It might be best if you rename the files after this PR is merged.

@sahilnxp
Copy link
Contributor Author

Thanks @jenswi-linaro @jforissier
Overall, I agree with your suggestion.

As @jenswi-linaro said, I will take up this thing after this PR is merged.

BTW, apart from this any other comment on this PR?

jenswi-linaro
jenswi-linaro reviewed Feb 18, 2025
View reviewed changes
core/drivers/crypto/ele/ele.c Outdated
unsigned long plat_get_aslr_seed(void)
{
uint64_t timeout = timeout_init_us(10 * 1000);

Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this empty line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CI is reporting an error without new line: https://github.com/OP-TEE/optee_os/actions/runs/13407450824/job/37449890100?pr=7268#step:5:61

Copy link
Contributor Author

@sahilnxp sahilnxp Feb 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

BTW, can you please tell me how can I run this checkdiff script on my local machine ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jenswi-linaro please help me with this.

clementfaure and others added 9 commits February 19, 2025 07:05
@clementfaure @sahilnxp
drivers: ele: use the baseline API to retrieve the UID
552df47 
Use the baseline API instead of the HSM to retrieve the UID. These two
API calls are duplicates and the HSM call is soon deprecated.

Signed-off-by: Clement Faure <[email protected]>
Signed-off-by: Sahil Malhotra <[email protected]>
@clementfaure @sahilnxp
drivers: ele: allocate data in heap for HUK derivation
5781a4f 
Use the heap and the ELE memory allocator instead of using the stack.

Signed-off-by: Clement Faure <[email protected]>
Signed-off-by: Sahil Malhotra <[email protected]>
@sahilnxp
drivers: ele: update session open command parameters
92f2098 
Update session open command parameters to be compatible with
ELE FW API doc

Signed-off-by: Sahil Malhotra <[email protected]>
@sahilnxp
drivers: ele: add SAB init command
806ab84 
There has been addition of SAB init command for initializing
the Edgeleock enclave services.

Signed-off-by: Sahil Malhotra <[email protected]>
@sahilnxp
drivers: ele: enable TRUST MU in OP-TEE for i.MX93-EVK/i.MX91-EVK
7bf7879 
There is TRUST MU available on i.MX91 and i.MX93 platforms.

TRUST MU can be used to access some HW features of Edgelock Enclave which
Normal MU cannot, but for now it is configured to be used to communicate
with ELE FW.

So Kernel will use Normal MU and OP-TEE will use TRUST MU.

There is special setup needed to write to Trust MU.
* First for TRUST-MU we must write a valid command to TR0 before we can
  write any of the remaining registers, and TR15 is reserved for special
  USM command.
* The CMD field for TR0 is bits 31:26 and must be greater than
  the value of the watermark set in SCM_CR2[31:22]. Typically
  if you just set the MSB (bit 31) its enough.
* SIZE must be programmed in bits 19:16 of TR0, we cannot write
  TRn past the specified size in this bit field

Signed-off-by: Sahil Malhotra <[email protected]>
@sahilnxp
drivers: ele: rng: get random number from ELE
6fd4bdb 
TEE_GenerateRandom() supported by ELE get random command on imx93 & imx91.

Issues in the ELE FW have been found when both, secure and
non-secure worlds are communicating with ELE.

To prevent any issue, rely on RNG software in OPTEE. The compilation of
hw_get_random_bytes() is conditioned by CFG_WITH_SOFTWARE_PRNG.
Set CFG_WITH_SOFTWARE_PRNG=y by default.

With CFG_WITH_SOFTWARE_PRNG enabled in OP-TEE, ELE will not be used
in OP-TEE at runtime and Linux can access the ELE without conflicts.

Signed-off-by: Clement Faure <[email protected]>
Signed-off-by: Olivier Masse <[email protected]>
Signed-off-by: Sahil Malhotra <[email protected]>
@sahilnxp
core: imx: enable ELE by default
a9c284c 
Enable ELE by default on all ELE supported devices

Signed-off-by: Sahil Malhotra <[email protected]>
@sahilnxp
fixup! drivers: ele: add memory management functions
5392215 
fixup! drivers: ele: add memory management functions

Signed-off-by: Sahil Malhotra <[email protected]>
@sahilnxp
fixup! drivers: ele: rng: get random number from ELE
47d1880 
fixup! drivers: ele: rng: get random number from ELE

Signed-off-by: Sahil Malhotra <[email protected]>
@sahilnxp sahilnxp force-pushed the github_master_ele_update branch from b589598 to 47d1880 Compare February 19, 2025 07:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jenswi-linaro jenswi-linaro jenswi-linaro left review comments

@KarlK90 KarlK90 KarlK90 left review comments

@saschahauer saschahauer saschahauer left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@sahilnxp @jforissier @jenswi-linaro @KarlK90 @saschahauer @clementfaure