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

Add OTP hardware key support to plat-ti #1492

Merged
merged 3 commits into from
Apr 24, 2017
Merged

Conversation

glneo
Copy link
Contributor

@glneo glneo commented Apr 21, 2017

No description provided.

Copy link
Contributor

@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.

With our without my comment addressed:
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

@@ -161,6 +165,8 @@ void init_sec_mon(unsigned long nsec_entry)
nsec_ctx->mode_regs.und_lr = plat_boot_args->nsec_ctx.und_lr;
nsec_ctx->mon_lr = plat_boot_args->nsec_ctx.mon_lr;
nsec_ctx->mon_spsr = plat_boot_args->nsec_ctx.mon_spsr;

memcpy(&plat_huk[0], &plat_boot_args->huk[0], sizeof(plat_boot_args->huk));
Copy link
Contributor

Choose a reason for hiding this comment

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

This &array[0] style looks a bit funny, why not use just array instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If that is your preference then it makes no difference to me, updated.

@@ -15,6 +15,7 @@ $(call force,CFG_PL310_LOCKED,y)
$(call force,CFG_SECURE_TIME_SOURCE_REE,y)
arm32-platform-cpuarch := cortex-a9
else
$(call force,CFG_OTP_SUPPORT,y)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpicking: should this be CFG_OTP_SUPPORT ?= y instead? (so that the option may be disabled at build time: make ... CFG_OTP_SUPPORT=n). Now, if you think it doesn't make much sense, I'm perfectly OK with force.

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 think your suggestion makes more sense, updated.

memcpy(&hwkey->data[0], &plat_huk[0], sizeof(hwkey->data));
}

/* TODO */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A patch such as this one [0] would remove the need for this temporary tee_otp_get_die_id clone.

[0] 32dce12

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed. I'll pick up the first three patches from #1318.

Copy link
Contributor

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.

Thanks, Updated to take advantage of this change.

@@ -15,6 +15,7 @@ $(call force,CFG_PL310_LOCKED,y)
$(call force,CFG_SECURE_TIME_SOURCE_REE,y)
arm32-platform-cpuarch := cortex-a9
else
CFG_OTP_SUPPORT ?= y
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for changing my mind ;) but in fact I think this should just go. CFG_OTP_SUPPORT does not appear anywhere else now, and you already have the plat_huk[] array populated unconditionally, so it is a bit weird not to use it. Let's make things as simple as possible. OK?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh no scratch that, you have multiple platform flavors. Sorry.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hum. All flavors work the same so yeah, I think CFG_OTP_SUPPORT is useless.
Time to stop reviewing code and leave the office now ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, when AM43xx gets support for this then we can safely drop CFG_OTP_SUPPORT.

@jforissier
Copy link
Contributor

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

Currently the non-secure context is passed in from our initial secure
software as part of the OP-TEE load process. This passed-in data will
not only contain the non-secure context but also any additional data we
may need to give to OP-TEE. Rename these structures and group the context
data into a struct for future expansion.

Signed-off-by: Andrew F. Davis <afd@ti.com>
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>
Some TI platforms pass the HUK to OP-TEE via a secure memory stack. Read
and store this key for later use. On platforms without CFG_OTP_SUPPORT
this key is ignored.

Signed-off-by: Andrew F. Davis <afd@ti.com>
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>
On DRA7xx/AM57xx the initial secure software will pass OP-TEE a
Hardware Unique Key (HUK), use this key when requested.

Signed-off-by: Andrew F. Davis <afd@ti.com>
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>
@jforissier jforissier merged commit 75c6da9 into OP-TEE:master Apr 24, 2017
@glneo glneo deleted the otp-support branch April 25, 2017 19:38
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.

3 participants