Master+imx caam fix #2986

0xB0D · 2019-05-05T00:06:54Z

This series

Adds a simple set of routines to set the CAAM job-rings to normal world
Calls that routine for i.MX6 and i.MX7 processors.

This is not a full fledged CAAM driver, which both NXP and Microsoft have written but, not thus far upstreamed code for. This is simply about setting the default permissions of the CAAM.

The expectation is that a full CAAM driver will assign some job-rings to secure and some job-rings to normal world and will communicate job-ring ownership via DTB settings.

Setting job-ring ownership when transitioning from secure to non-secure world is best done in OPTEE rather than u-boot and so if this code is applied in OP-TEE the next step is to stop setting job-ring permissions in u-boot.

0xB0D · 2019-05-05T00:07:29Z

@MrVan

MrVan · 2019-05-05T01:53:19Z

core/arch/arm/plat-imx/imx_caam.c

+	int i;
+
+	for (i = 0; i < CAAM_NUM_JOB_RINGS; i++) {
+		reg = io_read32(&caam->jr[i].jrmidr_ms);


Setting all Job rings to NS should not be a good idea, secure world also need one when caam is supported in secure world

But..

If you want to use the CAAM in secure world you would run the driver NXP is upstreaming in #2962

If you don't want to run the CAAM in secure world, then the default as given here would still apply.

MrVan · 2019-05-05T01:54:34Z

core/arch/arm/plat-imx/sub.mk

@@ -1,7 +1,7 @@
 global-incdirs-y += .
 srcs-y += main.c

-srcs-$(CFG_MX6)$(CFG_MX7) += mmdc.c imx-common.c
+srcs-$(CFG_MX6)$(CFG_MX7) += mmdc.c imx-common.c imx_caam.c


I prefer a build CFG for this new function.

sahilnxp · 2019-05-05T15:52:51Z

There is a bigger version of this feature in #2962
Please have a look.

0xB0D · 2019-05-06T13:22:58Z

@sahilnxp
I see that. If you don't want to run the CAAM inside of OP-TEE, i.e. if you are just using it in Linux non-secure, then I think we still need a default set of permissions.
@MrVan @sahilnxp do you agree ?

MrVan · 2019-05-09T12:53:52Z

Reviewed-by: Peng Fan <peng.fan@nxp.com>

Emantor · 2019-05-09T13:39:56Z

core/arch/arm/plat-imx/imx_caam.h

+#if defined(CFG_MX6ULL)
+#define CAAM_NUM_JOB_RINGS		0
+#else
+#define CAAM_NUM_JOB_RINGS		4


At least the Security Reference Manuals I have for i.MX6Q and i.MX6UL document different numbers of job rings. Maybe @MrVan can clarify whether it is okay to assume four jobrings for all devices.

ah damn.
I thought I probably wasn't getting full coverage on job-ring numbers - let me review the two you have flagged - thanks.

Actually I believe four is the correct number.

https://github.com/u-boot/u-boot/blob/master/include/fsl_sec.h - line 84
https://github.com/u-boot/u-boot/blob/master/drivers/crypto/fsl/jr.c - line 636

The address-space maps 4 job-ring queues. You're right that not all processors implement 4 queues but the address space offset is valid.

It's up to the CAAM drivers - and their DTS configs to describe which runtime job-rings are used.

4 is the right value here though.

Emantor · 2019-05-09T13:41:13Z

core/arch/arm/plat-imx/imx6ul.c

@@ -32,4 +33,5 @@ static void init_csu(void)
 void plat_cpu_reset_late(void)
 {
 	init_csu();
+	init_caam();


Any reason why this is done in plat_cpu_reset_late? At least for this case the earliest user is the linux driver, in that case it can be moved to normal platform init.

No reason at all. I located it with csu_init() because it seemed like the right place.
I can move it elsewhere - I don't think there's any barrier to that.

plat_cpu_reset_late happens before the console is initialized, but since your code does not print anyway this is a minor nit on my part. If you go the route of implementing this into the driver subdir, this may be the correct place afterall, since the full CAAM implementation (if I remember correctly) should be initialized in plat_cpu_reset_late anyway. Take this with a grain of salt, I have not looked deep into the code yet.

sdininno · 2019-05-09T18:03:58Z

@bryanodonoghue ,
if you want to still go trough this pull request, Would you like to go through driver interface instead?
you can add driver_init() macro in the imx_caam.c file, and just enabling the compilation for the imx that actually have a CAAM IP (like for 6ULL that does not)
this way you avoid adding thing in the plat_cpu_reset_late function as well and adding a new imx_caam.h file just to call your function.
you may also want to add your file to the drivers folder that was created not that long ago by @Emantor .
Also, fyi you will face issues for devices that do not retain CAAM configuration during low power mode (suspend to ram). This is the case for i.MX7D (and probably 6ul as well) .
this will be addressed in the other effort that tries to bring a full fledge CAAM driver.

thanks,
Silvano

0xB0D · 2019-05-10T09:46:28Z

@sdininno @Emantor

OK, let me look at your full CAAM driver with a view to making it provide a default set of permissions irrespective of whether a platform wants runtime OP-TEE CAAM ...

0xB0D · 2019-05-27T15:26:46Z

Latest drop.

Adds RB @MrVan as indicated
Defines the # of job-rings as 4 as-is done in u-boot
Doesn't integrate with core:driver implementation NXP CAAM Driver #2962

On point # 3 I think the right thing to-do is to set default CAAM permissions irrespective of whether or not the CAAM driver will be used in OP-TEE - i.e. we should not require compilation of the CAAM driver to setup the job-rings for Linux.

When/if the CAAM driver makes it into upstream there may be an opportunity to rationalize the code but, in the meantime I'd like to move on with fixing the job-ring permissions, so that I can post an update to u-boot to remove job-ring ownership assignment there.

0xB0D · 2019-05-27T15:31:04Z

Update.
I finally figured out what was going wrong with Travis CI

It wants
// SPDX in .c files but
/* SPDX */ in .h files

:)

jbech-linaro · 2019-05-28T12:07:32Z

@bryanodonoghue , I've manually restarted IBART once, but it is still failing. I cannot see that the changes in this patch has anything to do with the errors, probably just a rebase of master that is needed. Could you please do that and force push.

When we transition to secure-world certain parts of the CAAM become opaque to normal world. This patch adds a simple routine to set CAAM job-ring permissions to normal-world by default. Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> Reviewed-by: Peng Fan <peng.fan@nxp.com>

This patch adds a call to set CAAM job-ring permissions for i.MX6 and i.MX7 processors. Since the iMX6ULL does not have a CAAM it will be skipped but, all other i.MX6 and i.MX7 SoCs will have their default CAAM job-ring permissions set to normal-world. Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> Reviewed-by: Peng Fan <peng.fan@nxp.com>

jforissier · 2019-06-05T15:33:27Z

This PR causes an error in the Shippable build:

...
 _make PLATFORM=imx-mx6ulevk
core/arch/arm/plat-imx/imx_caam.c: In function 'init_caam':
core/arch/arm/plat-imx/imx_caam.c:16:31: error: initialization from incompatible pointer type [-Werror=incompatible-pointer-types]
  struct imx_caam_ctrl *caam = (struct caam_ctrl *)CAAM_BASE;
                               ^
cc1: all warnings being treated as errors
mk/compile.mk:147: recipe for target 'out/core/arch/arm/plat-imx/imx_caam.o' failed
make: *** [out/core/arch/arm/plat-imx/imx_caam.o] Error 1
make: *** Waiting for unfinished jobs....

(why has Shippable not run during the CI step? Weird...)

jbech-linaro · 2019-06-05T16:16:00Z

(why has Shippable not run during the CI step? Weird...)

I've seen many weird thing on GitHub last couple of days, comments not being posted, takes time PRs to update after patch has been sent and so on. So made some issue at the GitHub side.

jbech-linaro · 2019-06-05T16:16:31Z

@bryanodonoghue would you mind having a look at it and send a fix?

jforissier · 2019-06-05T16:25:08Z

@bryanodonoghue an intermediate cast to (uintptr_t) should do the trick, we have such patterns already.

0xB0D · 2019-06-05T16:59:20Z

I'll post an update now

0xB0D · 2019-06-05T17:14:55Z

Well there's an interesting typo
struct **imx_caam_ctrl ***caam = (**struct caam_ctrl ***)
how did that ever even compile...

edit: yep... turns out it is possible to forward declare a struct in a cast.... so even this junk would compile

struct imx_caam_ctrl *caam = (struct larry *) CAAM_BASE;

0xB0D · 2019-06-05T17:25:41Z

#3074

0xB0D force-pushed the master+imx_caam_fix branch 3 times, most recently from 677056b to a8c39ae Compare May 5, 2019 12:55

MrVan reviewed May 5, 2019

View reviewed changes

0xB0D force-pushed the master+imx_caam_fix branch from a8c39ae to b006097 Compare May 8, 2019 17:37

Emantor reviewed May 9, 2019

View reviewed changes

0xB0D force-pushed the master+imx_caam_fix branch from b006097 to 014c7d9 Compare May 27, 2019 15:23

0xB0D force-pushed the master+imx_caam_fix branch from 014c7d9 to 1b1f0c2 Compare May 27, 2019 15:29

0xB0D added 2 commits May 29, 2019 00:29

0xB0D force-pushed the master+imx_caam_fix branch from 1b1f0c2 to e3c73a4 Compare May 28, 2019 23:30

jbech-linaro merged commit f142f6f into OP-TEE:master Jun 3, 2019

0xB0D deleted the master+imx_caam_fix branch June 4, 2019 09:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Master+imx caam fix #2986

Master+imx caam fix #2986

0xB0D commented May 5, 2019

0xB0D commented May 5, 2019

MrVan May 5, 2019

0xB0D May 6, 2019

MrVan May 5, 2019

sahilnxp commented May 5, 2019

0xB0D commented May 6, 2019

MrVan commented May 9, 2019

Emantor May 9, 2019

0xB0D May 9, 2019

0xB0D May 27, 2019

Emantor May 9, 2019

0xB0D May 9, 2019

Emantor May 10, 2019 •

edited

Loading

sdininno commented May 9, 2019 •

edited

Loading

0xB0D commented May 10, 2019

0xB0D commented May 27, 2019 •

edited

Loading

0xB0D commented May 27, 2019

jbech-linaro commented May 28, 2019

jforissier commented Jun 5, 2019

jbech-linaro commented Jun 5, 2019

jbech-linaro commented Jun 5, 2019

jforissier commented Jun 5, 2019

0xB0D commented Jun 5, 2019

0xB0D commented Jun 5, 2019 •

edited

Loading

0xB0D commented Jun 5, 2019

Master+imx caam fix #2986

Master+imx caam fix #2986

Conversation

0xB0D commented May 5, 2019

0xB0D commented May 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sahilnxp commented May 5, 2019

0xB0D commented May 6, 2019

MrVan commented May 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Emantor May 10, 2019 • edited Loading

Choose a reason for hiding this comment

sdininno commented May 9, 2019 • edited Loading

0xB0D commented May 10, 2019

0xB0D commented May 27, 2019 • edited Loading

0xB0D commented May 27, 2019

jbech-linaro commented May 28, 2019

jforissier commented Jun 5, 2019

jbech-linaro commented Jun 5, 2019

jbech-linaro commented Jun 5, 2019

jforissier commented Jun 5, 2019

0xB0D commented Jun 5, 2019

0xB0D commented Jun 5, 2019 • edited Loading

0xB0D commented Jun 5, 2019

Emantor May 10, 2019 •

edited

Loading

sdininno commented May 9, 2019 •

edited

Loading

0xB0D commented May 27, 2019 •

edited

Loading

0xB0D commented Jun 5, 2019 •

edited

Loading