[EXT] RE: [PATCH v5 01/16] crypto/fsl: Add support for CAAM Job ring driver model

ZHIZHIKIN Andrey andrey.zhizhikin at leica-geosystems.com
Tue Nov 23 10:11:25 CET 2021 

Hello Gaurav,

> -----Original Message-----
> From: Gaurav Jain <gaurav.jain at nxp.com>
> Sent: Tuesday, November 23, 2021 8:22 AM
> To: ZHIZHIKIN Andrey <andrey.zhizhikin at leica-geosystems.com>; u-
> boot at lists.denx.de
> Cc: Stefano Babic <sbabic at denx.de>; Fabio Estevam <festevam at gmail.com>; Peng Fan
> <peng.fan at nxp.com>; Simon Glass <sjg at chromium.org>; Priyanka Jain
> <priyanka.jain at nxp.com>; Ye Li <ye.li at nxp.com>; Horia Geanta
> <horia.geanta at nxp.com>; Ji Luo <ji.luo at nxp.com>; Franck Lenormand
> <franck.lenormand at nxp.com>; Silvano Di Ninno <silvano.dininno at nxp.com>; Sahil
> Malhotra <sahil.malhotra at nxp.com>; Pankaj Gupta <pankaj.gupta at nxp.com>; Varun
> Sethi <V.Sethi at nxp.com>; dl-uboot-imx <uboot-imx at nxp.com>; Shengzhou Liu
> <shengzhou.liu at nxp.com>; Mingkai Hu <mingkai.hu at nxp.com>; Rajesh Bhagat
> <rajesh.bhagat at nxp.com>; Meenakshi Aggarwal <meenakshi.aggarwal at nxp.com>; Wasim
> Khan <wasim.khan at nxp.com>; Alison Wang <alison.wang at nxp.com>; Pramod Kumar
> <pramod.kumar_1 at nxp.com>; Andy Tang <andy.tang at nxp.com>; Adrian Alonso
> <adrian.alonso at nxp.com>; Vladimir Oltean <olteanv at gmail.com>; Michael Walle
> <michael at walle.cc>
> Subject: RE: [EXT] RE: [PATCH v5 01/16] crypto/fsl: Add support for CAAM Job ring
> driver model
> 
> 
> Hello Andrey
> 

[snip]

> > >
> > > HAB is a code that is part of the ROM code which set the JR DID for all
> i.mx8M.
> > > I took the dumps on SPL boot which actually shows the JR DID set by HAB.
> > > Dump taken by you on kernel boot does not show the values set by ROM.
> > > IMX8MM
> > > JR0DID_MS = 0x8011
> > > JR1DID_MS = 0x8011
> > > JR2DID_MS = 0x0
> > >
> > > IMX8MN
> > > JR0DID_MS = 0x8011
> > > JR1DID_MS = 0x8011
> > > JR2DID_MS = 0x0
> > >
> > > IMX8MP
> > > JR0DID_MS = 0x8011
> > > JR1DID_MS = 0x8011
> > > JR2DID_MS = 0x0
> >
> > This is an interesting piece of information, thanks a lot for the readout! So
> it
> > does look like that BootROM on all derivatives reserves JR0 and JR1 at the
> > beginning, letting the ATF to release only JR1 to NS world...
> >
> > Does IMX8MQ have the same reservation as well?
> >
> > > >
> > > > > With New implementation CAAM is enabled for i.MX8M derivative. Any
> > > > > JR whose DID is written in ATF, can be used in Uboot.
> > > > > JR0 is reserved for HAB so JR1 will be used for all i.MX8M derivatives.
> > > > >
> > > > > >
> > > > > > I'm wondering about several points here:
> > > > > > 1. Why does current implementation on have this reservation done
> > > > > > on imx8mm and
> > > > > >    where does this happen? None of the code pieces suggests that
> > > > > > it is
> > > > done in
> > > > > >    U-Boot, is it performed in BootROM?
> > > > >
> > > > > I cannot see if current implementation(SPL/Uboot) has reservation
> > > > > done for imx8mm.
> > > > > In ATF, we are reserving the JR0.
> > > >
> > > > I was not able to identify which part of ATF code is responsible to
> > > > program JR0DID_MS on imx8mm, the only thing I saw was the part where
> > > > the JR0 is held in S World *if* the JR0DID_MS is set to 0x8011. Can
> > > > you point out where is this performed in ATF code?
> > > >
> > > > If it is not in the ATF, then my question above still stands: which
> > > > component (HW or SW) programs JR0DID_MS, and why is it only done on
> > > > imx8mm derivative?
> > > HAB which is part of the ROM code sets the JR DID for all i.mx8M.
> > > >
> > > > >
> > > > > > 2. What is the intention of having JR0 reserved for all
> > > > > > derivatives? Is
> > > this
> > > > > >    the part of a bigger change that stretches across different
> > > > > > SW
> > > > components
> > > > > >    (e.g. ATF, OP-TEE, etc.)? If that is the case - then a more detailed
> > > > > >    description would be appreciated here.
> > > > > >
> > > > > > ATF code already accounts for this reservation in commit:
> > > > > > a83a7c65e ("TEE-639 plat: imx8m: Do not release JR0 to NS if HAB
> > > > > > is using it") [1], but there is no description on why is this required
> though.
> > > > > >
> > > > > > If this is required for HAB feature, then the question is:
> > > > > > should it be kept in
> > > > > S
> > > > > > World when U-Boot starts, or SPL can release it after the binary
> > > > > > is verified
> > > > > and
> > > > > > crypto facilities are not in use anymore?
> > > > >
> > > > > Commit: a83a7c65e reserves JR0 for HAB and not released to NS but
> > > > > JR1,
> > > > > JR2 are released to NS.
> > > >
> > > > Then I believe this change should be in-sync with ATF
> > > > implementation, because of the fact that your change can have any
> > > > arbitrary JR to be held in S World.
> > > >
> > > > What would happen if for example JR1 is programmed with TZ_OWN, but
> > > > ATF releases it to NS world? Can it be used by Kernel afterwards? Or
> > > > should the node be disabled here so that Kernel does not even see JR1
> during
> > boot?
> > > >
> > > Since JR0 is marked as disabled in DT, so SPL is only accessing single
> > > job ring and setting the JR1 DID as 0x8011.
> > > After SPL boots successfully, ATF is releasing JR1 and JR2 to NS by
> > > modifying the JRDID_MS as 0x1.
> > > Uboot is also accessing single jobring which is JR1.
> > > JR0 is only reserved for secure boot.
> >
> > Is it safe to assume that JR1 is then accessible from both S and NS Worlds?
> >
> > If that is the case, then that would actually mean that JRx status on DT should
> be
> > set as following:
> >
> > &sec_jr0 {
> >         status = "disabled";
> >         secure-status = "okay";
> > };
> >
> > &sec_jr1 {
> >         secure-status = "okay";
> > };
> >
> > &sec_jr2 {
> >         secure-status = "disabled";
> > };
> >
> > This would effectively mean:
> > JR0 - S-only,
> > JR1 - visible in both
> > JR2 - NS-only
> >
> > Please note, that as this configuration is applicable to both Kernel and U-Boot
> -
> > the above block should be defined in Kernel DT for all i.MX8M derivatives, and
> > picked up with the next U-Boot DTB re-sync.
> >
> > As I'm working on V3 for CAAM clean-up in the Kernel [1] - I can submit those
> > configuration changes, but I would need a confirmation from you if this is an
> > expected JR configuration, and whether IMX8MQ have the same setup.
> >
> IMX8MQ has same values.
> JR0DID_MS = 0x8011
> JR1DID_MS = 0x8011
> JR2DID_MS = 0x0
> 
> For now we are only reserving JR0 for secure boot. JR1 DID is later modified in
> ATF to 0x1. JR2 can be used by OPTEE which is secure and can set the DID before
> accessing the JR2.
> Setting secure-status as disabled for JR2 could break OPTEE.

I see no trouble here, as OPTEE does set the "secure-status" by itself if the
resource should be exclusively reserved in S World via dt_enable_secure_status()
call. What OPTEE does check is the "status" binding to identify which JR is
available, and setting secure-status = "disabled" does not imply status = "disabled".
JR device inquiry and reservation is done in caam_hal_cfg_get_jobring_dt() call,
see [1]. 

> "secure-status" property is not used in uboot CAAM driver code so how this is
> going to affect the caam driver working in SPL/Uboot?

The above snippet I proposed should be introduced in the Kernel DT, and then picked
up by U-Boot a the re-sync. It would not affect the U-Boot in any way, since the
"secure-status" property is not processed in it.

ATF uses register readout to identify which JR is held in S World, there is no impact
there as well.

OPTEE uses internal functions to set the proper secure-status, so it is beneficial to
Introduce DT bindings that it sets.

Kernel currently does not look at "secure-status" as well as U-Boot, and I'm not sure
if it is relevant for the moment.

Moreover, above snippet does reflect how the SW entities are seeing HW configurations
which comes out of the reset, isn't it?

> I am not sure about the kernel caam driver how secure-status is processed. For
> kernel JR configuration I cannot confirm.
> I would suggest to take the opinion from kernel caam maintainers as well.

I guess Horia can comment here regarding the above proposed status.

> 
> > >
> > > > So far, ATF only examines the JR0DID_MS content, and not all the others...
> > > >
> > > > > HAB uses JR0 for secure boot on all i.MX8M derivatives. Uboot
> > > > > calls HAB API for authenticating kernel.
> > > >
> > > > This implies then that the JR0 is permanently held in S World and
> > > > stays there for entire device powercycle and cannot be reclaimed in NS
> > World?
> > > Yes JR0 is held in S world.
> > >
> > >  In this
> > > > case, the DT node should be completely removed from DTB file so no
> > > > SW entity can even see it (as it is in a total possession of HW
> mechanisms).
> > > >
> > > We can consider this change after this patch series is merged.
> > > Currently I have disabled the JR0 in device tree.
> >
> > I guess with the proposed DT configuration this point would be covered as well,
> > isn't it? There would be no need to remove the node, as it would be marked
> > disabled in NS and enabled in S Worlds. I believe it is better to set the
> status as I
> > proposed, because that information in DT is transparent for everyone (removing
> > node raises questions regarding HW availability to me).
> 
> CAAM driver is used in spl, atf, optee, uboot, kernel.
> Spl and uboot can work with JR1 only. For other components it will be good to
> have their opinion.
> 
> >
> > >
> > > > >
> > > > > >
> > > > > > > +       jrdid_ms = JRDID_MS_TZ_OWN | JRDID_MS_PRIM_TZ |
> > > > > > > + JRDID_MS_PRIM_DID;
> > > > > >
> > > > > > What is the intention of setting JRDID_MS_PRIM_TZ? Isn't setting
> > > > > > JRDID_MS_TZ_OWN would be sufficient here?
> > > > >
> > > > > PRIM_TZ bit is set to 1 to indicate that only SecureWorld can
> > > > > access registers in that Job Ring's register page
> > > >
> > > > But would it not be enough just to set TZ_OWN? If I read SRM
> > > > correct: only TZ_OWN is enough to hold the JR in S World.
> > > >
> > > HAB is also setting 0x8011 as JR DID. It is better to be in sync with HAB.
> >
> > Do you know what is the reason for HAB to set PRIM_TZ bit? Is there any
> > specific reason for this?
> 
> To restrict JR register page access to Secure World, PRIM_TZ bit is set.
> So later in ATF we can decide which JobRing to release to NS.
> 
> Regards
> Gaurav Jain
> >
> > >
> > > Regards
> > > Gaurav Jain
> > >
> > > > >
> > > >
> > > > [snip]
> > > >
> > > > > >
> > > >
> > > > -- andrey
> >
> > -- andrey
> >

-- andrey

Link: [1]: https://github.com/OP-TEE/optee_os/blob/fd140f7eebbbee0c80f681b8bc1aad4b81f60194/core/drivers/crypto/caam/hal/common/hal_cfg_dt.c#L93

More information about the U-Boot mailing list