Feature/patch caam acipher by cneveux · Pull Request #3136 · OP-TEE/optee_os

cneveux · 2019-07-17T13:01:10Z

Add the NXP CAAM drivers:
- Asymmetric ECC

This PR includes the PR #2962 and PR #3135. The latest commit "core:driver implementation NXP CAAM Driver - ECC" is added to the PR #3135

Add C inline function to execute assembly instruction WFE/SEV use in C code Signed-off-by: Cedric Neveux <[email protected]> Reviewed-by: Jens Wiklander <[email protected]> Acked-by: Jens Wiklander <[email protected]>

Add registers to handle CAAM clocks Signed-off-by: Cedric Neveux <[email protected]>

Add dt functions: - dt_get_irq get the interrupt number of a node - dt_node_offset_by_compatible_status find a node compatible with specified 'compatible' input and check if the node status correspond to the expected one - dt_disable_status disable the 'status' field of node's prop - dt_set_secure_status set 'secure-status' field of node's prop and disable the 'status' field in the same time Signed-off-by: Cedric Neveux <[email protected]>

To allow any driver or initialization function to change the DT, moved the DT pack operation at the end of the generic boot. Move from update_external_dt to release_external_dt Signed-off-by: Cedric Neveux <[email protected]> Acked-by: Jens Wiklander <[email protected]>

Add the NXP CAAM drivers: - Random generator (instantiation and random generation) - Hash Add a generic cryptographic driver interface connecting TEE Crypto generic APIs to HW driver interface The Generic Crypto Driver interface in the core/driver/crypto/crypto_api is implemented to be able to use an other HW driver that is not the CAAM driver. Signed-off-by: Cedric Neveux <[email protected]>

Add the NXP CAAM drivers: - Cipher (AES/DES/DES3) Add a generic Cipher cryptographic driver interface connecting TEE Crypto generic APIs to HW driver interface Signed-off-by: Cedric Neveux <[email protected]>

Add the NXP CAAM drivers: - Asymmetric ECC Add a generic ECC cryptographic driver interface connecting TEE Crypto generic APIs to HW driver interface Signed-off-by: Cedric Neveux <[email protected]>

etienne-lms

for commit "core:arm64 Add sev/wfe inline function":
Reviewed-by: Etienne Carriere <[email protected]> with minor comments in commit message:

Remove Acked-by: tag from Jens since his Reviewed-by: tag is provided.
Can remove heading spaces from description lines.

etienne-lms · 2019-07-30T14:36:07Z

 S:	Maintained
 F:	core/arch/arm/plat-imx/

+NXP (Freescale) i.MX family


Looks like this entry could be merge into previous:same target platforms.

etienne-lms · 2019-07-30T14:36:45Z

+/* SPDX-License-Identifier: BSD-2-Clause */
+/*
+ * Copyright 2017-2018 NXP
+ *


can remove this empty line.
s/2018/2019/

etienne-lms · 2019-07-30T14:39:19Z

+/* SPDX-License-Identifier: BSD-2-Clause */
+/*
+ * Copyright 2017-2018 NXP
+ *


can remove this empty line
s/2018/2019/

etienne-lms · 2019-07-30T14:39:55Z

+ */
+
+#ifndef __MX7_CCM_REGS_H__
+#define __MX7_CCM_REGS_H__


s/MX7_CCM_REGS_H/MX7_CRM_REGS_H/g

etienne-lms · 2019-07-30T14:43:15Z

+ */
+#define CCM_GPR0			0x0
+#define CCM_GPRx_OFFSET			0x10
+#define CCM_GPRx(idx)			((idx * CCM_GRPx_OFFSET) + CCM_GPR0)


suggest parentheses in CCM_GPRx() here and in other CCM_*x() macros below:

-#define CCM_GPRx(idx) ((idx * CCM_GRPx_OFFSET) + CCM_GPR0) +#define CCM_GPRx(idx) (((idx) * CCM_GRPx_OFFSET) + CCM_GPR0)

etienne-lms · 2019-07-30T15:19:19Z

+
+	while (fnode != -FDT_ERR_NOTFOUND) {
+		node_status = _fdt_get_status(fdt, fnode);
+		if ((node_status & status) == status) {


I think here status is expected to be an input value, not specifically both a mask and a value.

Here the status can be a defined with multiple bits hence it's used as mask first to extract expected status bit and then status is used to check result of the bit mask.

Think looks strange. Check which behavior you would expect when calling:

dt_node_offset_by_compatible_status(fdt, DT_STATUS_OK_NSEC, &node, &compat) ?

dt_node_offset_by_compatible_status(fdt, DT_STATUS_OK_NSEC | DT_STATUS_OK_SEC, &node, &compat) ?

Ok, today status a bit defined with a single bit hence it will work with if (node_status & status)

Question is what to differentiate between:
status = okay / secure-status = okay
status = disabled / secure-status = okay
status = okay / secure-status = disabled
I think a straight compare seems the best way for caller to get target status value.

status is the value used in linux
secure-status is used in OPTEE
refer to _fdt_get_status
If we want to have property only allocated in Secure, we have to ensure that status is disabled and secure-status is okay

Then you can call with status set to DT_STATUS_OK_SEC.
One may use this function with DT_STATUS_OK_SEC or with DT_STATUS_OK_SEC | DT_STATUS_OK_SEC for some other means.

By the way: do you really expect several to find several compatible node but one 1 with a non matching status value?

Why not? I'm not doing the Linux DTB.

By the way: do you really expect several to find several compatible node but one 1 with a non matching status value?

Why not? I'm not doing the Linux DTB.

That's fine. I was just curious.

The util function you're implementing can be used by others platforms/drivers. It should be clear what's its purpose is, hence a clear description of what is expected (in value and behavior) for argument status.

etienne-lms · 2019-07-30T15:20:06Z

+paddr_t dt_node_offset_by_compatible_status(void *fdt, int status, int *node,
+						const char *compatible)
+{
+	paddr_t offset = 0;


Prefer DT_INFO_INVALID_REG as invalid value.

etienne-lms · 2019-07-30T15:20:46Z

+
+	prop = fdt_getprop(fdt, node, "status", &len);
+	if (!prop) {
+		/* Status is not available, just add it */


can remove this comment.

etienne-lms · 2019-07-30T15:22:22Z

+{
+	if (dt_disable_status(fdt, node)) {
+		EMSG("Unable to disable Normal Status");
+		return (-1);


minor: return -1;

etienne-lms · 2019-07-30T15:24:02Z

+	 * If the property is not present, it will be added
+	 * otherwise it will be modified to be "okay"
+	 */
+	if (fdt_appendprop_string(fdt, node, "secure-status", "okay"))


Looks like this sequence could be used in dt_disable_status(), no?

no, the dt_disable_status sets the property status and the dt_set_secure_status sets the property secure-status used in the function _fdt_get_status.

I agree. My point is, why not using the following implementation?

int dt_disable_status(void *fdt, int node) { if (fdt_appendprop_string(fdt, node, "status", "disabled")) return -1; return 0; } int dt_set_secure_status(void *fdt, int node) { if (dt_disable_status(fdt, node)) return -1; if (fdt_appendprop_string(fdt, node, "secure-status", "okay")) return -1; return 0; }

After test it seems that the appendprop is not the correct function to use in both cases.
The appendprop appends the new status value to the current existing property and doesn't modify it.
Hence I'm keeping the current dt_disable_status function that seems to be better in the sense that the property status is added if not existing otherwise it modify it to be something else than okay or ok. When the property size is not changed, we are not spending time in recalculating the overall DT offsets.
I'll change the dt_set_secure_status to dt_enable_secure_status and use the function fdt_setprop_string in place of the fdt_appendprop_string. In this case we want to have the secure-status set to okay to enable it.

etienne-lms

Few comments for commit "plat-imx Add CRM registers mapping". Check also commit message:

Minor suggested change: s/plat-imx /plat-imx: /
Can remove heading spaces from description lines.

etienne-lms

for commit "core:dt add DT function to read/modify DT"

etienne-lms

For commit "core:kernel move DT pack at the end of boot':
Reviewed-by: Etienne Carriere <[email protected]> with minor comments.

etienne-lms · 2019-07-30T15:25:36Z

 CFG_MMAP_REGIONS ?= 24
+
+# Almost all platforms include CAAM HW Modules, except the
+# one force to be disabled


s/force/forced/

etienne-lms · 2019-07-30T15:31:34Z

+	/*
+	 * Close DT after all drivers load
+	 * It allows drivers load to modify the DT
+	 */


can remove this comment.

etienne-lms · 2019-07-30T15:33:22Z


 static void release_external_dt(void)
 {
+	int ret;


could you update this to init the local variable? (https://optee.readthedocs.io/general/coding_standards.html?highlight=All%20variables%20shall%20be%20initialized)

cneveux · 2019-07-31T08:03:38Z

@etienne-lms
I pushed the reviewed commit into the first PR #2962. I would appreciate if this first PR #2962 could be integrated then I'll update the other PR related to the CAAM driver after.

vchong mentioned this pull request Jul 17, 2019

Feature/patch caam cipher #3135

Closed

cneveux added 5 commits July 22, 2019 08:17

core:arm64 Add sev/wfe inline function

f23d8a7

Add C inline function to execute assembly instruction WFE/SEV use in C code Signed-off-by: Cedric Neveux <[email protected]> Reviewed-by: Jens Wiklander <[email protected]> Acked-by: Jens Wiklander <[email protected]>

plat-imx Add CRM registers mapping

4f92c31

Add registers to handle CAAM clocks Signed-off-by: Cedric Neveux <[email protected]>

cneveux force-pushed the feature/patch_caam_acipher branch from f809268 to 91563db Compare July 24, 2019 09:44

core:driver implementation NXP CAAM Driver - Cipher

10b0c01

Add the NXP CAAM drivers: - Cipher (AES/DES/DES3) Add a generic Cipher cryptographic driver interface connecting TEE Crypto generic APIs to HW driver interface Signed-off-by: Cedric Neveux <[email protected]>

cneveux force-pushed the feature/patch_caam_acipher branch 2 times, most recently from b59f0a3 to a151559 Compare July 24, 2019 11:44

core:driver implementation NXP CAAM Driver - ECC

a151559

Add the NXP CAAM drivers: - Asymmetric ECC Add a generic ECC cryptographic driver interface connecting TEE Crypto generic APIs to HW driver interface Signed-off-by: Cedric Neveux <[email protected]>

This was referenced Jul 24, 2019

Feature/patch caam huk #3149

Closed

Feature/patch caam rsa #3152

Closed

etienne-lms reviewed Jul 30, 2019

View reviewed changes

cneveux closed this Aug 28, 2019

cneveux deleted the feature/patch_caam_acipher branch October 13, 2020 12:57

Conversation

cneveux commented Jul 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

etienne-lms left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

etienne-lms left a comment

Choose a reason for hiding this comment

Uh oh!

etienne-lms left a comment

Choose a reason for hiding this comment

Uh oh!

etienne-lms left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cneveux commented Jul 31, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cneveux commented Jul 17, 2019 •

edited

Loading