[U-Boot,2/4] crypto/fsl: Use __sec_set_jr_context_normal
diff mbox series

Message ID 20190423101948.24898-3-bryan.odonoghue@linaro.org
State Changes Requested
Delegated to: Stefano Babic
Headers show
Series
  • imx: Implement job-ring context switching
Related show

Commit Message

Bryan O'Donoghue April 23, 2019, 10:19 a.m. UTC
Use __sec_set_jr_context_normal() to set job-ring ownership rather than the
current in-line array walk.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 drivers/crypto/fsl/jr.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

Comments

Breno Matheus Lima April 25, 2019, 3:24 a.m. UTC | #1
Hi Bryan,

Em ter, 23 de abr de 2019 às 07:20, Bryan O'Donoghue
<bryan.odonoghue@linaro.org> escreveu:
>
> Use __sec_set_jr_context_normal() to set job-ring ownership rather than the
> current in-line array walk.
>
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  drivers/crypto/fsl/jr.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/drivers/crypto/fsl/jr.c b/drivers/crypto/fsl/jr.c
> index 7b13aa4a61..65982b8369 100644
> --- a/drivers/crypto/fsl/jr.c
> +++ b/drivers/crypto/fsl/jr.c
> @@ -616,7 +616,6 @@ int sec_init_idx(uint8_t sec_idx)
>  {
>         ccsr_sec_t *sec = (void *)SEC_ADDR(sec_idx);
>         uint32_t mcr = sec_in32(&sec->mcfgr);
> -       uint32_t jrown_ns;
>         int i;

We may also need to remove this variable otherwise we get build warning below:

drivers/crypto/fsl/jr.c: In function 'sec_init_idx':
drivers/crypto/fsl/jr.c:625:6: warning: unused variable 'i' [-Wunused-variable]
  int i;
      ^

Thanks for submitting this patch set.

I couldn't get encrypted boot working in my first attempt, doing the
exact same procedure with commit 22191ac35344 ("drivers/crypto/fsl:
assign job-rings to non-TrustZone") reverted works fine.

I will take a better look in your patch set and let you know if I find
something.

Best Regards,
Breno Matheus Lima
Bryan O'Donoghue April 30, 2019, 1:28 a.m. UTC | #2
On 25/04/2019 04:24, Breno Matheus Lima wrote:
> I couldn't get encrypted boot working in my first attempt, doing the
> exact same procedure with commit 22191ac35344 ("drivers/crypto/fsl:
> assign job-rings to non-TrustZone") reverted works fine.

Hi Breno,

I noticed another patch from you re: dek blob, does that address this 
issue for you are is this still a live thing ?

If you are running in secure-world, and the BootROM dek blob stuff 
validates job-ring ownership it _should_ be possible to flip the 
ownership bits to what the BootROM expects and then back again.

If its not working, presumably its because we aren't flipping ownership 
at the right time.

Maybe better to set permissions to secure-world while we are in u-boot 
and then switch to normal world before we hand over to the next boot phase.

---
bod
Bryan O'Donoghue April 30, 2019, 8:13 a.m. UTC | #3
On 30/04/2019 02:28, Bryan O'Donoghue wrote:
> 
> 
> On 25/04/2019 04:24, Breno Matheus Lima wrote:
>> I couldn't get encrypted boot working in my first attempt, doing the
>> exact same procedure with commit 22191ac35344 ("drivers/crypto/fsl:
>> assign job-rings to non-TrustZone") reverted works fine.
> 
> Hi Breno,
> 
> I noticed another patch from you re: dek blob, does that address this 
> issue for you are is this still a live thing ?
> 
> If you are running in secure-world, and the BootROM dek blob stuff 
> validates job-ring ownership it _should_ be possible to flip the 
> ownership bits to what the BootROM expects and then back again.
> 
> If its not working, presumably its because we aren't flipping ownership 
> at the right time.

It occurred to me after I went to bed.

The right thing to do is leave the BootROM settings up until we hand-off 
and then set the required post-boot settings.

Something I reckon can be ~easily done in some sort of architectural 
handover preparation function.

I'll spin that patchset.

---
bod
Breno Matheus Lima April 30, 2019, 4:06 p.m. UTC | #4
Hi Bryan,

Em ter, 30 de abr de 2019 às 05:13, Bryan O'Donoghue
<bryan.odonoghue@linaro.org> escreveu:
>
>
>
> On 30/04/2019 02:28, Bryan O'Donoghue wrote:
> >
> >
> > On 25/04/2019 04:24, Breno Matheus Lima wrote:
> >> I couldn't get encrypted boot working in my first attempt, doing the
> >> exact same procedure with commit 22191ac35344 ("drivers/crypto/fsl:
> >> assign job-rings to non-TrustZone") reverted works fine.
> >
> > Hi Breno,
> >
> > I noticed another patch from you re: dek blob, does that address this
> > issue for you are is this still a live thing ?

No, the patch I have recently submitted does not address the JR
TrustZone issue we are currently seeing with DEK blob decapsulation at
ROM level. I was not following AN12056 when I tried so I couldn't see
this other issue at first moment.

> >
> > If you are running in secure-world, and the BootROM dek blob stuff
> > validates job-ring ownership it _should_ be possible to flip the
> > ownership bits to what the BootROM expects and then back again.
> >
> > If its not working, presumably its because we aren't flipping ownership
> > at the right time.
>
> It occurred to me after I went to bed.
>
> The right thing to do is leave the BootROM settings up until we hand-off
> and then set the required post-boot settings.
>
> Something I reckon can be ~easily done in some sort of architectural
> handover preparation function.
>
> I'll spin that patchset.

Thanks for preparing a second version for this patchset, I see that
you have also replied to my other e-mail in "[PATCH 1/4] crypto/fsl:
Introduce API to save/restore job-ring context".

Your new proposal looks fine to me, I can test again.

Thanks,
Breno Lima

Patch
diff mbox series

diff --git a/drivers/crypto/fsl/jr.c b/drivers/crypto/fsl/jr.c
index 7b13aa4a61..65982b8369 100644
--- a/drivers/crypto/fsl/jr.c
+++ b/drivers/crypto/fsl/jr.c
@@ -616,7 +616,6 @@  int sec_init_idx(uint8_t sec_idx)
 {
 	ccsr_sec_t *sec = (void *)SEC_ADDR(sec_idx);
 	uint32_t mcr = sec_in32(&sec->mcfgr);
-	uint32_t jrown_ns;
 	int i;
 	int ret = 0;
 
@@ -674,11 +673,7 @@  int sec_init_idx(uint8_t sec_idx)
 #endif
 
 	/* Set ownership of job rings to non-TrustZone mode by default */
-	for (i = 0; i < ARRAY_SIZE(sec->jrliodnr); i++) {
-		jrown_ns = sec_in32(&sec->jrliodnr[i].ms);
-		jrown_ns |= JROWN_NS | JRMID_NS;
-		sec_out32(&sec->jrliodnr[i].ms, jrown_ns);
-	}
+	__sec_set_jr_context_normal(sec_idx);
 
 	ret = jr_init(sec_idx);
 	if (ret < 0) {