[EXT] RE: [PATCH v5 01/16] crypto/fsl: Add support for CAAM Job ring driver model
ZHIZHIKIN Andrey
andrey.zhizhikin at leica-geosystems.com
Wed Nov 17 21:19:22 CET 2021
Hello Gaurav,
> -----Original Message-----
> From: ZHIZHIKIN Andrey
> Sent: Wednesday, November 17, 2021 2:03 PM
> To: Gaurav Jain <gaurav.jain at nxp.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>
> Subject: RE: [EXT] RE: [PATCH v5 01/16] crypto/fsl: Add support for CAAM Job ring
> driver model
>
> Hello Gaurav,
>
> > -----Original Message-----
> > From: Gaurav Jain <gaurav.jain at nxp.com>
> > Sent: Wednesday, November 17, 2021 12:26 PM
> > 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>
> > Subject: RE: [EXT] RE: [PATCH v5 01/16] crypto/fsl: Add support for CAAM Job
> ring
> > driver model
> >
> >
> > Hello Andrey
> >
> > > -----Original Message-----
> > > From: ZHIZHIKIN Andrey <andrey.zhizhikin at leica-geosystems.com>
> > > Sent: Tuesday, November 16, 2021 9:24 PM
> > > To: Gaurav Jain <gaurav.jain at nxp.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>
> > > Subject: [EXT] RE: [PATCH v5 01/16] crypto/fsl: Add support for CAAM Job ring
> > > driver model
> > >
> > > Caution: EXT Email
> > >
> > > Hello Gaurav,
> > >
> > > > -----Original Message-----
> > > > From: U-Boot <u-boot-bounces at lists.denx.de> On Behalf Of Gaurav Jain
> > > > Sent: Monday, November 15, 2021 8:00 AM
> > > > To: 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>; NXP i . MX U-Boot Team <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>; Tang Yuantian <andy.tang at nxp.com>; Adrian
> > > > Alonso <adrian.alonso at nxp.com>; Vladimir Oltean <olteanv at gmail.com>;
> > > > Gaurav Jain <gaurav.jain at nxp.com>
> > > > Subject: [PATCH v5 01/16] crypto/fsl: Add support for CAAM Job ring
> > > > driver model
> > > >
> > > >
> > > > added device tree support for job ring driver.
> > > > sec is initialized based on job ring information processed from device
> > > > tree.
> > > >
> > > > Signed-off-by: Gaurav Jain <gaurav.jain at nxp.com>
> > > > Reviewed-by: Ye Li <ye.li at nxp.com>
> > > > ---
> > > > cmd/Kconfig | 1 +
> > > > drivers/crypto/fsl/Kconfig | 7 +
> > > > drivers/crypto/fsl/Makefile | 4 +-
> > > > drivers/crypto/fsl/jr.c | 316 +++++++++++++++++++++++-------------
> > > > drivers/crypto/fsl/jr.h | 14 ++
> > > > 5 files changed, 232 insertions(+), 110 deletions(-)
> > > >
>
> [snip]
>
> > > > sec_out32(&sec->mcfgr, mcr);
> > > > +#if defined(CONFIG_SPL_BUILD) && defined(CONFIG_IMX8M)
> > >
> > > This would effectively reserve the JR0 on _all_ i.MX8M derivatives is S
> World.
> > This code is to set any JR DID in SPL so that the job ring can be configured.
> >
> > >
> > > Current implementation only has JR0 reserved in S World on imx8mm derivative,
> > > but this new addition extends this to imx8mn, imx8mp and imx8mq.
> > Current implementation do not initialize CAAM for i.MX8M derivatives. It is not
> > based on driver model approach and only using JR0.
>
> OK, but then I do not have on explanation on why do I see following results from
> reading JRaDID_MS registers on imx8m derivatives:
> - imx8mm:
> JR0DID_MS = 0x8011
> JR1DID_MS = 0x0
> JR2DID_MS = 0x0
> - imx8mn:
> JR0DID_MS = 0x0
> JR1DID_MS = 0x0
> JR2DID_MS = 0x0
> - imx8mp:
> JR0DID_MS = 0x0
> JR1DID_MS = 0x0
> JR2DID_MS = 0x0
I'd have to correct the readout above, I've posted the data which was not 100% accurate.
Here is the actual one:
- imx8mm:
JR0DID_MS = 0x8011
JR1DID_MS = 0x1
JR2DID_MS = 0x1
- imx8mn:
JR0DID_MS = 0x1
JR1DID_MS = 0x1
JR2DID_MS = 0x1
- imx8mp:
JR0DID_MS = 0x0
JR1DID_MS = 0x0
JR2DID_MS = 0x0
It does suggests the following:
- Mini does have JR0 reserved in S World, JR1 and JR2 are released
to NS World with DID programmed. According to the new logic in
the patch - this should allow Mini to utilize HAB feature.
- Nano does have all JR released in NS World, which suggests that
HAB is not available for it, correct?
- Plus does not have DID programmed in *any* JR devices, which
fails the RNG initialization during Kernel start since DEC0
cannot be initialized, but it is required to prime RNG via
direct descriptor execution in DEC0. This means that all Crypto
facilities are currently unavailable on Plus, correct? Does
any of patches in this series suggests the fix for this? Is there
simply power missing?
I would appreciate if you can comment on the rest of my points as they are still opened.
>
> This readout is taken at Kernel boot, and it clearly shows that only JR0 has
> TZ_OWN, PRIM_TZ and PRIM_DID bits set, and it is only done on imx8mm.
>
> > 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?
>
> >
> > > 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?
>
> 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? 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).
>
> >
> > >
> > > > + 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.
>
> >
>
> [snip]
>
> > >
>
> -- andrey
Cc: Michael Walle
-- andrey
More information about the U-Boot
mailing list