* [PATCH] crypto: caam - check jr permissions before probing @ 2021-11-04 16:21 Andrey Zhizhikin 2021-11-05 1:20 ` Michael Walle 2021-11-11 16:45 ` [PATCH v2 0/2] CAAM Driver: re-factor and dynamic JR availability check Andrey Zhizhikin 0 siblings, 2 replies; 36+ messages in thread From: Andrey Zhizhikin @ 2021-11-04 16:21 UTC (permalink / raw) To: horia.geanta, pankaj.gupta, herbert, davem, iuliana.prodan, michael, linux-crypto, linux-kernel Cc: Andrey Zhizhikin Job Rings can be set to be exclusively used by TrustZone which makes the access to those rings only possible from Secure World. This access separation is defined by setting bits in CAAM JRxDID_MS register. Once reserved to be owned by TrustZone, this Job Ring becomes unavailable for the Kernel. This reservation is performed early in the boot process, even before the Kernel starts, which leads to unavailability of the HW at the probing stage. Moreover, the reservation can be done for any Job Ring and is not under control of the Kernel. The only condition that remains is: at least one Job Ring should be made available for the NS world. Current implementation lists Job Rings as child nodes of CAAM driver, and tries to perform probing on those regardless of whether JR HW is accessible or not. This leads to the following error while probing: [ 1.509894] caam 30900000.crypto: job rings = 3, qi = 0 [ 1.525201] caam_jr 30901000.jr: failed to flush job ring 0 [ 1.525214] caam_jr: probe of 30901000.jr failed with error -5 Implement a dynamic mechanism to identify which Job Ring is actually marked as owned by TrustZone, and fail the probing of those child nodes with -ENODEV. Signed-off-by: Andrey Zhizhikin <andrey.zhizhikin@leica-geosystems.com> --- drivers/crypto/caam/ctrl.c | 18 ++++++++++++------ drivers/crypto/caam/intern.h | 1 + drivers/crypto/caam/jr.c | 17 +++++++++++++++++ drivers/crypto/caam/regs.h | 8 +++++--- 4 files changed, 35 insertions(+), 9 deletions(-) diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c index ca0361b2dbb0..c48506a02340 100644 --- a/drivers/crypto/caam/ctrl.c +++ b/drivers/crypto/caam/ctrl.c @@ -805,12 +805,18 @@ static int caam_probe(struct platform_device *pdev) for_each_available_child_of_node(nprop, np) if (of_device_is_compatible(np, "fsl,sec-v4.0-job-ring") || of_device_is_compatible(np, "fsl,sec4.0-job-ring")) { - ctrlpriv->jr[ring] = (struct caam_job_ring __iomem __force *) - ((__force uint8_t *)ctrl + - (ring + JR_BLOCK_NUMBER) * - BLOCK_OFFSET - ); - ctrlpriv->total_jobrs++; + /* Check if the JR is not owned exclusively by TZ */ + if (!((rd_reg32(&ctrl->jr_mid[ring].liodn_ms)) & + (MSTRID_LOCK_TZ_OWN | MSTRID_LOCK_PRIM_TZ))) { + ctrlpriv->jr[ring] = (struct caam_job_ring __iomem __force *) + ((__force uint8_t *)ctrl + + (ring + JR_BLOCK_NUMBER) * + BLOCK_OFFSET + ); + /* Indicate that this JR is available for NS */ + ctrlpriv->jobr_ns_flags |= BIT(ring); + ctrlpriv->total_jobrs++; + } ring++; } diff --git a/drivers/crypto/caam/intern.h b/drivers/crypto/caam/intern.h index 7d45b21bd55a..2919af55dbfe 100644 --- a/drivers/crypto/caam/intern.h +++ b/drivers/crypto/caam/intern.h @@ -91,6 +91,7 @@ struct caam_drv_private { * or from register-based version detection code */ u8 total_jobrs; /* Total Job Rings in device */ + u8 jobr_ns_flags; /* Flags for each Job Ring if it is not owned by TZ*/ u8 qi_present; /* Nonzero if QI present in device */ u8 mc_en; /* Nonzero if MC f/w is active */ int secvio_irq; /* Security violation interrupt number */ diff --git a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c index 7f2b1101f567..a260981e0843 100644 --- a/drivers/crypto/caam/jr.c +++ b/drivers/crypto/caam/jr.c @@ -511,10 +511,27 @@ static int caam_jr_probe(struct platform_device *pdev) struct device_node *nprop; struct caam_job_ring __iomem *ctrl; struct caam_drv_private_jr *jrpriv; + struct caam_drv_private *caamctrlpriv; static int total_jobrs; struct resource *r; int error; + /* Before we proceed with the JR device probing, verify + * that the job ring itself is available to Non-Secure world. + * If the JR is owned exclusively by TZ - we should not even + * process it further. + */ + caamctrlpriv = dev_get_drvdata(pdev->dev.parent); + if (!(caamctrlpriv->jobr_ns_flags & BIT(total_jobrs))) { + /* This job ring is marked to be exclusively used by TZ, + * do not proceed with probing as the HW block is inaccessible. + * Increment total seen JR devices since it is used as the index + * into verification and fail probing for this node. + */ + total_jobrs++; + return -ENODEV; + } + jrdev = &pdev->dev; jrpriv = devm_kzalloc(jrdev, sizeof(*jrpriv), GFP_KERNEL); if (!jrpriv) diff --git a/drivers/crypto/caam/regs.h b/drivers/crypto/caam/regs.h index 3738625c0250..8ade617f9e65 100644 --- a/drivers/crypto/caam/regs.h +++ b/drivers/crypto/caam/regs.h @@ -445,10 +445,12 @@ struct caam_perfmon { }; /* LIODN programming for DMA configuration */ -#define MSTRID_LOCK_LIODN 0x80000000 -#define MSTRID_LOCK_MAKETRUSTED 0x00010000 /* only for JR masterid */ +#define MSTRID_LOCK_LIODN BIT(31) +#define MSTRID_LOCK_MAKETRUSTED BIT(16) /* only for JR: masterid */ +#define MSTRID_LOCK_TZ_OWN BIT(15) /* only for JR: owned by TZ */ +#define MSTRID_LOCK_PRIM_TZ BIT(4) /* only for JR: Primary TZ */ -#define MSTRID_LIODN_MASK 0x0fff +#define MSTRID_LIODN_MASK GENMASK(11, 0) struct masterid { u32 liodn_ms; /* lock and make-trusted control bits */ u32 liodn_ls; /* LIODN for non-sequence and seq access */ base-commit: 8a796a1dfca2780321755033a74bca2bbe651680 -- 2.25.1 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH] crypto: caam - check jr permissions before probing 2021-11-04 16:21 [PATCH] crypto: caam - check jr permissions before probing Andrey Zhizhikin @ 2021-11-05 1:20 ` Michael Walle 2021-11-05 10:34 ` ZHIZHIKIN Andrey 2021-11-11 16:45 ` [PATCH v2 0/2] CAAM Driver: re-factor and dynamic JR availability check Andrey Zhizhikin 1 sibling, 1 reply; 36+ messages in thread From: Michael Walle @ 2021-11-05 1:20 UTC (permalink / raw) To: Andrey Zhizhikin Cc: horia.geanta, pankaj.gupta, herbert, davem, iuliana.prodan, linux-crypto, linux-kernel Hi Andrey, Am 2021-11-04 17:21, schrieb Andrey Zhizhikin: > Job Rings can be set to be exclusively used by TrustZone which makes > the > access to those rings only possible from Secure World. This access > separation is defined by setting bits in CAAM JRxDID_MS register. Once > reserved to be owned by TrustZone, this Job Ring becomes unavailable > for > the Kernel. This reservation is performed early in the boot process, > even before the Kernel starts, which leads to unavailability of the HW > at the probing stage. Moreover, the reservation can be done for any Job > Ring and is not under control of the Kernel. The only condition that > remains is: at least one Job Ring should be made available for the NS > world. Where is that written down? > Current implementation lists Job Rings as child nodes of CAAM driver, > and tries to perform probing on those regardless of whether JR HW is > accessible or not. > > This leads to the following error while probing: > [ 1.509894] caam 30900000.crypto: job rings = 3, qi = 0 > [ 1.525201] caam_jr 30901000.jr: failed to flush job ring 0 > [ 1.525214] caam_jr: probe of 30901000.jr failed with error -5 > > Implement a dynamic mechanism to identify which Job Ring is actually > marked as owned by TrustZone, and fail the probing of those child nodes > with -ENODEV. > > Signed-off-by: Andrey Zhizhikin <andrey.zhizhikin@leica-geosystems.com> > --- > drivers/crypto/caam/ctrl.c | 18 ++++++++++++------ > drivers/crypto/caam/intern.h | 1 + > drivers/crypto/caam/jr.c | 17 +++++++++++++++++ > drivers/crypto/caam/regs.h | 8 +++++--- > 4 files changed, 35 insertions(+), 9 deletions(-) > > diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c > index ca0361b2dbb0..c48506a02340 100644 > --- a/drivers/crypto/caam/ctrl.c > +++ b/drivers/crypto/caam/ctrl.c > @@ -805,12 +805,18 @@ static int caam_probe(struct platform_device > *pdev) > for_each_available_child_of_node(nprop, np) > if (of_device_is_compatible(np, "fsl,sec-v4.0-job-ring") || > of_device_is_compatible(np, "fsl,sec4.0-job-ring")) { > - ctrlpriv->jr[ring] = (struct caam_job_ring __iomem __force *) > - ((__force uint8_t *)ctrl + > - (ring + JR_BLOCK_NUMBER) * > - BLOCK_OFFSET > - ); > - ctrlpriv->total_jobrs++; > + /* Check if the JR is not owned exclusively by TZ */ > + if (!((rd_reg32(&ctrl->jr_mid[ring].liodn_ms)) & > + (MSTRID_LOCK_TZ_OWN | MSTRID_LOCK_PRIM_TZ))) { what is the PRIM_TZ bit? I don't see it in my reference manual (which is for the LS1028A). Can't we just do a if (rd_reg32(&ctrl->jr_mid[ring].liodn_ms) & (MSTRID_LOCK_TZ_OWN | MSTRID_LOCK_PRIM_TZ)) continue; > + ctrlpriv->jr[ring] = (struct caam_job_ring __iomem __force *) > + ((__force uint8_t *)ctrl + > + (ring + JR_BLOCK_NUMBER) * > + BLOCK_OFFSET > + ); This isn't really used at all. Can we drop "jr" from struct caam_drv_private altogether? See also below. > + /* Indicate that this JR is available for NS */ > + ctrlpriv->jobr_ns_flags |= BIT(ring); > + ctrlpriv->total_jobrs++; as well as this? > + } > ring++; > } > > diff --git a/drivers/crypto/caam/intern.h > b/drivers/crypto/caam/intern.h > index 7d45b21bd55a..2919af55dbfe 100644 > --- a/drivers/crypto/caam/intern.h > +++ b/drivers/crypto/caam/intern.h > @@ -91,6 +91,7 @@ struct caam_drv_private { > * or from register-based version detection code > */ > u8 total_jobrs; /* Total Job Rings in device */ > + u8 jobr_ns_flags; /* Flags for each Job Ring if it is not owned by > TZ*/ > u8 qi_present; /* Nonzero if QI present in device */ > u8 mc_en; /* Nonzero if MC f/w is active */ > int secvio_irq; /* Security violation interrupt number */ > diff --git a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c > index 7f2b1101f567..a260981e0843 100644 > --- a/drivers/crypto/caam/jr.c > +++ b/drivers/crypto/caam/jr.c > @@ -511,10 +511,27 @@ static int caam_jr_probe(struct platform_device > *pdev) > struct device_node *nprop; > struct caam_job_ring __iomem *ctrl; > struct caam_drv_private_jr *jrpriv; > + struct caam_drv_private *caamctrlpriv; > static int total_jobrs; ugh. > struct resource *r; > int error; > > + /* Before we proceed with the JR device probing, verify > + * that the job ring itself is available to Non-Secure world. > + * If the JR is owned exclusively by TZ - we should not even > + * process it further. > + */ > + caamctrlpriv = dev_get_drvdata(pdev->dev.parent); > + if (!(caamctrlpriv->jobr_ns_flags & BIT(total_jobrs))) { Is anything preventing from total_jobrs getting big? Can't we get rid of this static variable somehow? Before this commit, it was just used for messages. Can we check the TZ bit here instead of doing that bitflags dance? nitpick: in caam there are no netdev comments. So multiline comments look like: /* * this is a comment. */ > + /* This job ring is marked to be exclusively used by TZ, > + * do not proceed with probing as the HW block is inaccessible. > + * Increment total seen JR devices since it is used as the index > + * into verification and fail probing for this node. > + */ > + total_jobrs++; > + return -ENODEV; > + } > + > jrdev = &pdev->dev; > jrpriv = devm_kzalloc(jrdev, sizeof(*jrpriv), GFP_KERNEL); > if (!jrpriv) > diff --git a/drivers/crypto/caam/regs.h b/drivers/crypto/caam/regs.h > index 3738625c0250..8ade617f9e65 100644 > --- a/drivers/crypto/caam/regs.h > +++ b/drivers/crypto/caam/regs.h > @@ -445,10 +445,12 @@ struct caam_perfmon { > }; > > /* LIODN programming for DMA configuration */ > -#define MSTRID_LOCK_LIODN 0x80000000 > -#define MSTRID_LOCK_MAKETRUSTED 0x00010000 /* only for JR masterid */ > +#define MSTRID_LOCK_LIODN BIT(31) > +#define MSTRID_LOCK_MAKETRUSTED BIT(16) /* only for JR: masterid */ > +#define MSTRID_LOCK_TZ_OWN BIT(15) /* only for JR: owned by TZ */ > +#define MSTRID_LOCK_PRIM_TZ BIT(4) /* only for JR: Primary TZ */ can't find that one. in general, does these marcros match with your reference manual? Which one do you have? for me the bits are named: LICID[31], AMTD[16], TZ[15] and SDID[11:0] also the register is called JRnICID. I wonder if the LS1028A has a newer/older CAAM version. according to the device tree (fsl-ls1028a.dtsi) the sec is v5.0 (and also compatible with v4.0). If you have a look at the RM it says 7.0 (at least the MAJ_VER in SECVID_MS is 7 accoring to the manual; i haven't checked on the hardware for now. Horia, can you shed some light here. > -#define MSTRID_LIODN_MASK 0x0fff > +#define MSTRID_LIODN_MASK GENMASK(11, 0) > struct masterid { > u32 liodn_ms; /* lock and make-trusted control bits */ > u32 liodn_ls; /* LIODN for non-sequence and seq access */ > > base-commit: 8a796a1dfca2780321755033a74bca2bbe651680 -- -michael ^ permalink raw reply [flat|nested] 36+ messages in thread
* RE: [PATCH] crypto: caam - check jr permissions before probing 2021-11-05 1:20 ` Michael Walle @ 2021-11-05 10:34 ` ZHIZHIKIN Andrey 2021-11-05 23:16 ` Michael Walle 0 siblings, 1 reply; 36+ messages in thread From: ZHIZHIKIN Andrey @ 2021-11-05 10:34 UTC (permalink / raw) To: Michael Walle Cc: horia.geanta, pankaj.gupta, herbert, davem, iuliana.prodan, linux-crypto, linux-kernel Hello Michael, > -----Original Message----- > From: Michael Walle <michael@walle.cc> > Sent: Friday, November 5, 2021 2:21 AM > To: ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com> > Cc: horia.geanta@nxp.com; pankaj.gupta@nxp.com; > herbert@gondor.apana.org.au; davem@davemloft.net; > iuliana.prodan@nxp.com; linux-crypto@vger.kernel.org; linux- > kernel@vger.kernel.org > Subject: Re: [PATCH] crypto: caam - check jr permissions before probing > > > Hi Andrey, > > Am 2021-11-04 17:21, schrieb Andrey Zhizhikin: > > Job Rings can be set to be exclusively used by TrustZone which makes > > the access to those rings only possible from Secure World. This access > > separation is defined by setting bits in CAAM JRxDID_MS register. Once > > reserved to be owned by TrustZone, this Job Ring becomes unavailable > > for the Kernel. This reservation is performed early in the boot > > process, even before the Kernel starts, which leads to unavailability > > of the HW at the probing stage. Moreover, the reservation can be done > > for any Job Ring and is not under control of the Kernel. The only > > condition that remains is: at least one Job Ring should be made > > available for the NS world. > > Where is that written down? If you refer to the condition statement: this is implied as without any JR initialized - it would not be possible to register a single algo. This stemmed out from the discussion of the patch in U-Boot (see [1]), where the it was indicated that JR0 is reserved for HAB on imx8m series. The naïve solution proposed was to just disable the child node, but I decided to look for a dynamic possibility to recognize those reserved JR nodes, hence this patch came out. > > > Current implementation lists Job Rings as child nodes of CAAM driver, > > and tries to perform probing on those regardless of whether JR HW is > > accessible or not. > > > > This leads to the following error while probing: > > [ 1.509894] caam 30900000.crypto: job rings = 3, qi = 0 > > [ 1.525201] caam_jr 30901000.jr: failed to flush job ring 0 > > [ 1.525214] caam_jr: probe of 30901000.jr failed with error -5 > > > > Implement a dynamic mechanism to identify which Job Ring is actually > > marked as owned by TrustZone, and fail the probing of those child > > nodes with -ENODEV. > > > > Signed-off-by: Andrey Zhizhikin > > <andrey.zhizhikin@leica-geosystems.com> > > --- > > drivers/crypto/caam/ctrl.c | 18 ++++++++++++------ > > drivers/crypto/caam/intern.h | 1 + > > drivers/crypto/caam/jr.c | 17 +++++++++++++++++ > > drivers/crypto/caam/regs.h | 8 +++++--- > > 4 files changed, 35 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c > > index ca0361b2dbb0..c48506a02340 100644 > > --- a/drivers/crypto/caam/ctrl.c > > +++ b/drivers/crypto/caam/ctrl.c > > @@ -805,12 +805,18 @@ static int caam_probe(struct platform_device > > *pdev) > > for_each_available_child_of_node(nprop, np) > > if (of_device_is_compatible(np, "fsl,sec-v4.0-job-ring") || > > of_device_is_compatible(np, "fsl,sec4.0-job-ring")) { > > - ctrlpriv->jr[ring] = (struct caam_job_ring __iomem __force *) > > - ((__force uint8_t *)ctrl + > > - (ring + JR_BLOCK_NUMBER) * > > - BLOCK_OFFSET > > - ); > > - ctrlpriv->total_jobrs++; > > + /* Check if the JR is not owned exclusively by TZ */ > > + if (!((rd_reg32(&ctrl->jr_mid[ring].liodn_ms)) & > > + (MSTRID_LOCK_TZ_OWN | > > + MSTRID_LOCK_PRIM_TZ))) { > > what is the PRIM_TZ bit? I don't see it in my reference manual (which is for the > LS1028A). See my comment below regarding this bit. > > Can't we just do a > if (rd_reg32(&ctrl->jr_mid[ring].liodn_ms) & > (MSTRID_LOCK_TZ_OWN | MSTRID_LOCK_PRIM_TZ)) > continue; Yes, this would work as well. I'll address this is V2. > > > + ctrlpriv->jr[ring] = (struct caam_job_ring __iomem __force *) > > + ((__force uint8_t *)ctrl + > > + (ring + JR_BLOCK_NUMBER) * > > + BLOCK_OFFSET > > + ); > > This isn't really used at all. Can we drop "jr" from struct caam_drv_private > altogether? See also below. Agree. I was contemplating to remove this, as the caam_jr does its own devm_ioremap in the probe and does not use what caam driver passes along. But I decided not to address this with this patch, since this is not related to the issue this change addresses. In general, this driver needs a bit of TLC, and I can take care of those points (together with ctrlpriv->total_jobrs) in a separate series, unless there are strong objections arises that this cleanup should come along this patch. > > > + /* Indicate that this JR is available for NS */ > > + ctrlpriv->jobr_ns_flags |= BIT(ring); > > + ctrlpriv->total_jobrs++; > > as well as this? Noted. > > > + } > > ring++; > > } > > > > diff --git a/drivers/crypto/caam/intern.h > > b/drivers/crypto/caam/intern.h index 7d45b21bd55a..2919af55dbfe 100644 > > --- a/drivers/crypto/caam/intern.h > > +++ b/drivers/crypto/caam/intern.h > > @@ -91,6 +91,7 @@ struct caam_drv_private { > > * or from register-based version detection code > > */ > > u8 total_jobrs; /* Total Job Rings in device */ > > + u8 jobr_ns_flags; /* Flags for each Job Ring if it is not owned by > > TZ*/ > > u8 qi_present; /* Nonzero if QI present in device */ > > u8 mc_en; /* Nonzero if MC f/w is active */ > > int secvio_irq; /* Security violation interrupt number */ > > diff --git a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c index > > 7f2b1101f567..a260981e0843 100644 > > --- a/drivers/crypto/caam/jr.c > > +++ b/drivers/crypto/caam/jr.c > > @@ -511,10 +511,27 @@ static int caam_jr_probe(struct platform_device > > *pdev) > > struct device_node *nprop; > > struct caam_job_ring __iomem *ctrl; > > struct caam_drv_private_jr *jrpriv; > > + struct caam_drv_private *caamctrlpriv; > > static int total_jobrs; > > ugh. Yes, I've seen that. At first, I even wanted to replace it with the ctrlpriv->total_jobrs, but decided not to do it in order to keep this patch focused. > > > struct resource *r; > > int error; > > > > + /* Before we proceed with the JR device probing, verify > > + * that the job ring itself is available to Non-Secure world. > > + * If the JR is owned exclusively by TZ - we should not even > > + * process it further. > > + */ > > + caamctrlpriv = dev_get_drvdata(pdev->dev.parent); > > + if (!(caamctrlpriv->jobr_ns_flags & BIT(total_jobrs))) { > > Is anything preventing from total_jobrs getting big? Can't we get rid of this static > variable somehow? Before this commit, it was just used for messages. I guess not, the only limitation we have is the HW. Current imx8m mini does have 3 Job Rings, and plus added one more. Since I do not have any Layerscape HW - I cannot really tell if the number of instantiated Job Rings there is bigger than 8 and would grow in the future. I had a local implementation version with set_bit/test_bit variant, perhaps that one would be more appropriate here. If it's OK - I can do that one and push it in V2. > Can we check the TZ bit here instead of doing that bitflags dance? Honestly, I had this implementation locally as well, but decided to go ahead with "the dance" in order not to access the registers of another module from this one. Besides, the JR node loop in present caam_probe() got me tripped, as it does an early lookup and analysis of JR child nodes and I found it a right place to analyze and record the JR availability. > > nitpick: in caam there are no netdev comments. So multiline comments look like: > /* > * this is a comment. > */ Noted, will be addressed in V2. > > > + /* This job ring is marked to be exclusively used by TZ, > > + * do not proceed with probing as the HW block is inaccessible. > > + * Increment total seen JR devices since it is used as the index > > + * into verification and fail probing for this node. > > + */ > > + total_jobrs++; > > + return -ENODEV; > > + } > > + > > jrdev = &pdev->dev; > > jrpriv = devm_kzalloc(jrdev, sizeof(*jrpriv), GFP_KERNEL); > > if (!jrpriv) > > diff --git a/drivers/crypto/caam/regs.h b/drivers/crypto/caam/regs.h > > index 3738625c0250..8ade617f9e65 100644 > > --- a/drivers/crypto/caam/regs.h > > +++ b/drivers/crypto/caam/regs.h > > @@ -445,10 +445,12 @@ struct caam_perfmon { }; > > > > /* LIODN programming for DMA configuration */ > > -#define MSTRID_LOCK_LIODN 0x80000000 > > -#define MSTRID_LOCK_MAKETRUSTED 0x00010000 /* only for JR > masterid */ > > +#define MSTRID_LOCK_LIODN BIT(31) > > +#define MSTRID_LOCK_MAKETRUSTED BIT(16) /* only for JR: masterid */ > > +#define MSTRID_LOCK_TZ_OWN BIT(15) /* only for JR: owned by TZ */ > > +#define MSTRID_LOCK_PRIM_TZ BIT(4) /* only for JR: Primary TZ */ > > can't find that one. > > in general, does these marcros match with your reference manual? Which one do > you have? I'm working on the imx8m mini, which has a v4.0 of CAAM, and this bit is defined in JR[0:2]DID_MS registers. The map looks like following: LDID[31], USE_OUT[30], PRIM_ICID[29:19], LAMTD[17], AMTD[16], TZ_OWN[15], SDID_MS[14:5], PRIM_TZ[4], PRIM_DID[3:0] Perhaps, there is a deviation here between what is instantiated in LS vs i.MX series. At this point, I would also be interested to hear back from NXP on this. > > for me the bits are named: > LICID[31], AMTD[16], TZ[15] and SDID[11:0] also the register is called JRnICID. > > I wonder if the LS1028A has a newer/older CAAM version. > according to the device tree (fsl-ls1028a.dtsi) the sec is v5.0 (and also compatible > with v4.0). If you have a look at the RM it says 7.0 (at least the MAJ_VER in > SECVID_MS is 7 accoring to the manual; i haven't checked on the hardware for > now. I've checked the imx8m mini HW, and it has reported: Major: 4 Minor: 1 Era: 9 I believe that the LS family has a newer version of CAAM instantiated, which can be the reason on those register content deviations. > > Horia, can you shed some light here. +1 > > > -#define MSTRID_LIODN_MASK 0x0fff > > +#define MSTRID_LIODN_MASK GENMASK(11, 0) > > struct masterid { > > u32 liodn_ms; /* lock and make-trusted control bits */ > > u32 liodn_ls; /* LIODN for non-sequence and seq access */ > > > > base-commit: 8a796a1dfca2780321755033a74bca2bbe651680 > > -- > -michael Link: [1]: http://patchwork.ozlabs.org/project/uboot/patch/20211026065554.29009-4-gaurav.jain@nxp.com/ -- andrey ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] crypto: caam - check jr permissions before probing 2021-11-05 10:34 ` ZHIZHIKIN Andrey @ 2021-11-05 23:16 ` Michael Walle 2021-11-08 10:24 ` ZHIZHIKIN Andrey 0 siblings, 1 reply; 36+ messages in thread From: Michael Walle @ 2021-11-05 23:16 UTC (permalink / raw) To: ZHIZHIKIN Andrey Cc: horia.geanta, pankaj.gupta, herbert, davem, iuliana.prodan, linux-crypto, linux-kernel Hi Andrey, Am 2021-11-05 11:34, schrieb ZHIZHIKIN Andrey: >> -----Original Message----- >> From: Michael Walle <michael@walle.cc> >> Sent: Friday, November 5, 2021 2:21 AM >> To: ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com> >> Cc: horia.geanta@nxp.com; pankaj.gupta@nxp.com; >> herbert@gondor.apana.org.au; davem@davemloft.net; >> iuliana.prodan@nxp.com; linux-crypto@vger.kernel.org; linux- >> kernel@vger.kernel.org >> Subject: Re: [PATCH] crypto: caam - check jr permissions before >> probing >> >> >> Hi Andrey, >> >> Am 2021-11-04 17:21, schrieb Andrey Zhizhikin: >> > Job Rings can be set to be exclusively used by TrustZone which makes >> > the access to those rings only possible from Secure World. This access >> > separation is defined by setting bits in CAAM JRxDID_MS register. Once >> > reserved to be owned by TrustZone, this Job Ring becomes unavailable >> > for the Kernel. This reservation is performed early in the boot >> > process, even before the Kernel starts, which leads to unavailability >> > of the HW at the probing stage. Moreover, the reservation can be done >> > for any Job Ring and is not under control of the Kernel. The only >> > condition that remains is: at least one Job Ring should be made >> > available for the NS world. >> >> Where is that written down? > > If you refer to the condition statement: this is implied as without any > JR > initialized - it would not be possible to register a single algo. > > This stemmed out from the discussion of the patch in U-Boot (see [1]), > where the it was indicated that JR0 is reserved for HAB on imx8m > series. > > The naïve solution proposed was to just disable the child node, but I > decided to look for a dynamic possibility to recognize those reserved > JR > nodes, hence this patch came out. First, thank you for taking the extra step here. Does "reserved for HAB" mean TF-A is using it or is the BootROM already using it. TF-A is optional (so is HAB I guess?). So it might be possible to have jr0 in linux, right? If that is correct, the solution to disable the jr0 at compile time is wrong. Thus it has to be done at run time. Either by removing/disabling the node in u-boot or by not probing it. I'm not sure whats the correct solution here, though. >> > Current implementation lists Job Rings as child nodes of CAAM driver, >> > and tries to perform probing on those regardless of whether JR HW is >> > accessible or not. >> > >> > This leads to the following error while probing: >> > [ 1.509894] caam 30900000.crypto: job rings = 3, qi = 0 >> > [ 1.525201] caam_jr 30901000.jr: failed to flush job ring 0 >> > [ 1.525214] caam_jr: probe of 30901000.jr failed with error -5 >> > >> > Implement a dynamic mechanism to identify which Job Ring is actually >> > marked as owned by TrustZone, and fail the probing of those child >> > nodes with -ENODEV. >> > >> > Signed-off-by: Andrey Zhizhikin >> > <andrey.zhizhikin@leica-geosystems.com> >> > --- >> > drivers/crypto/caam/ctrl.c | 18 ++++++++++++------ >> > drivers/crypto/caam/intern.h | 1 + >> > drivers/crypto/caam/jr.c | 17 +++++++++++++++++ >> > drivers/crypto/caam/regs.h | 8 +++++--- >> > 4 files changed, 35 insertions(+), 9 deletions(-) >> > >> > diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c >> > index ca0361b2dbb0..c48506a02340 100644 >> > --- a/drivers/crypto/caam/ctrl.c >> > +++ b/drivers/crypto/caam/ctrl.c >> > @@ -805,12 +805,18 @@ static int caam_probe(struct platform_device >> > *pdev) >> > for_each_available_child_of_node(nprop, np) >> > if (of_device_is_compatible(np, "fsl,sec-v4.0-job-ring") || >> > of_device_is_compatible(np, "fsl,sec4.0-job-ring")) { >> > - ctrlpriv->jr[ring] = (struct caam_job_ring __iomem __force *) >> > - ((__force uint8_t *)ctrl + >> > - (ring + JR_BLOCK_NUMBER) * >> > - BLOCK_OFFSET >> > - ); >> > - ctrlpriv->total_jobrs++; >> > + /* Check if the JR is not owned exclusively by TZ */ >> > + if (!((rd_reg32(&ctrl->jr_mid[ring].liodn_ms)) & >> > + (MSTRID_LOCK_TZ_OWN | >> > + MSTRID_LOCK_PRIM_TZ))) { >> >> what is the PRIM_TZ bit? I don't see it in my reference manual (which >> is for the >> LS1028A). > > See my comment below regarding this bit. > >> >> Can't we just do a >> if (rd_reg32(&ctrl->jr_mid[ring].liodn_ms) & >> (MSTRID_LOCK_TZ_OWN | MSTRID_LOCK_PRIM_TZ)) >> continue; > > Yes, this would work as well. I'll address this is V2. > >> >> > + ctrlpriv->jr[ring] = (struct caam_job_ring __iomem __force *) >> > + ((__force uint8_t *)ctrl + >> > + (ring + JR_BLOCK_NUMBER) * >> > + BLOCK_OFFSET >> > + ); >> >> This isn't really used at all. Can we drop "jr" from struct >> caam_drv_private >> altogether? See also below. > > Agree. I was contemplating to remove this, as the caam_jr does its own > devm_ioremap in the probe > and does not use what caam driver passes along. But I decided not to > address this with this patch, > since this is not related to the issue this change addresses. > > In general, this driver needs a bit of TLC, and I can take care of > those points (together with > ctrlpriv->total_jobrs) in a separate series, unless there are strong > objections arises that this cleanup > should come along this patch. If you clean up later, probably most of this code is going away, no? Then whats the point in having this patch in the first place. Usually its the other way around. >> > + /* Indicate that this JR is available for NS */ >> > + ctrlpriv->jobr_ns_flags |= BIT(ring); >> > + ctrlpriv->total_jobrs++; >> >> as well as this? > > Noted. > >> >> > + } >> > ring++; >> > } >> > >> > diff --git a/drivers/crypto/caam/intern.h >> > b/drivers/crypto/caam/intern.h index 7d45b21bd55a..2919af55dbfe 100644 >> > --- a/drivers/crypto/caam/intern.h >> > +++ b/drivers/crypto/caam/intern.h >> > @@ -91,6 +91,7 @@ struct caam_drv_private { >> > * or from register-based version detection code >> > */ >> > u8 total_jobrs; /* Total Job Rings in device */ >> > + u8 jobr_ns_flags; /* Flags for each Job Ring if it is not owned by >> > TZ*/ >> > u8 qi_present; /* Nonzero if QI present in device */ >> > u8 mc_en; /* Nonzero if MC f/w is active */ >> > int secvio_irq; /* Security violation interrupt number */ >> > diff --git a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c index >> > 7f2b1101f567..a260981e0843 100644 >> > --- a/drivers/crypto/caam/jr.c >> > +++ b/drivers/crypto/caam/jr.c >> > @@ -511,10 +511,27 @@ static int caam_jr_probe(struct platform_device >> > *pdev) >> > struct device_node *nprop; >> > struct caam_job_ring __iomem *ctrl; >> > struct caam_drv_private_jr *jrpriv; >> > + struct caam_drv_private *caamctrlpriv; >> > static int total_jobrs; >> >> ugh. > > Yes, I've seen that. At first, I even wanted to replace it with the > ctrlpriv->total_jobrs, > but decided not to do it in order to keep this patch focused. Having the total_jobrs (and using it for anything else than messages) static will create an unnecessary dependency on the correct probe order. >> > struct resource *r; >> > int error; >> > >> > + /* Before we proceed with the JR device probing, verify >> > + * that the job ring itself is available to Non-Secure world. >> > + * If the JR is owned exclusively by TZ - we should not even >> > + * process it further. >> > + */ >> > + caamctrlpriv = dev_get_drvdata(pdev->dev.parent); >> > + if (!(caamctrlpriv->jobr_ns_flags & BIT(total_jobrs))) { >> >> Is anything preventing from total_jobrs getting big? Can't we get rid >> of this static >> variable somehow? Before this commit, it was just used for messages. > > I guess not, the only limitation we have is the HW. Current imx8m mini > does have > 3 Job Rings, and plus added one more. Since I do not have any > Layerscape HW - > I cannot really tell if the number of instantiated Job Rings there is > bigger than 8 > and would grow in the future. For now the LS1028A has 4. > I had a local implementation version with set_bit/test_bit variant, > perhaps that > one would be more appropriate here. If it's OK - I can do that one and > push it in V2. > >> Can we check the TZ bit here instead of doing that bitflags dance? > > Honestly, I had this implementation locally as well, but decided to go > ahead with > "the dance" in order not to access the registers of another module > from this one. Ahh I didn't notice that the register was part of the parent. Meh. > Besides, the JR node loop in present caam_probe() got me tripped, as it > does an > early lookup and analysis of JR child nodes and I found it a right > place to analyze > and record the JR availability. I see. But we should really communicate whether the child should return ENODEV in a different way. IMHO this static thing is really fragile. >> >> nitpick: in caam there are no netdev comments. So multiline comments >> look like: >> /* >> * this is a comment. >> */ > > Noted, will be addressed in V2. > >> >> > + /* This job ring is marked to be exclusively used by TZ, >> > + * do not proceed with probing as the HW block is inaccessible. >> > + * Increment total seen JR devices since it is used as the index >> > + * into verification and fail probing for this node. >> > + */ >> > + total_jobrs++; >> > + return -ENODEV; >> > + } >> > + >> > jrdev = &pdev->dev; >> > jrpriv = devm_kzalloc(jrdev, sizeof(*jrpriv), GFP_KERNEL); >> > if (!jrpriv) >> > diff --git a/drivers/crypto/caam/regs.h b/drivers/crypto/caam/regs.h >> > index 3738625c0250..8ade617f9e65 100644 >> > --- a/drivers/crypto/caam/regs.h >> > +++ b/drivers/crypto/caam/regs.h >> > @@ -445,10 +445,12 @@ struct caam_perfmon { }; >> > >> > /* LIODN programming for DMA configuration */ >> > -#define MSTRID_LOCK_LIODN 0x80000000 >> > -#define MSTRID_LOCK_MAKETRUSTED 0x00010000 /* only for JR >> masterid */ >> > +#define MSTRID_LOCK_LIODN BIT(31) >> > +#define MSTRID_LOCK_MAKETRUSTED BIT(16) /* only for JR: masterid */ >> > +#define MSTRID_LOCK_TZ_OWN BIT(15) /* only for JR: owned by TZ */ >> > +#define MSTRID_LOCK_PRIM_TZ BIT(4) /* only for JR: Primary TZ */ >> >> can't find that one. >> >> in general, does these marcros match with your reference manual? Which >> one do >> you have? > > I'm working on the imx8m mini, which has a v4.0 of CAAM, and this bit > is defined in > JR[0:2]DID_MS registers. > > The map looks like following: > LDID[31], USE_OUT[30], PRIM_ICID[29:19], LAMTD[17], AMTD[16], > TZ_OWN[15], SDID_MS[14:5], PRIM_TZ[4], PRIM_DID[3:0] > > Perhaps, there is a deviation here between what is instantiated in LS > vs i.MX series. > > At this point, I would also be interested to hear back from NXP on > this. Probably, but that would also mean we'd have to distiguish between these too. You're checking PRIM_TZ which is SDID on the LS1028A (which is an arbitrary number if I understand it correctly). So the check above might actually trigger although it shouldn't. That said, whats PRIM_TZ? When is it set? >> for me the bits are named: >> LICID[31], AMTD[16], TZ[15] and SDID[11:0] also the register is called >> JRnICID. >> >> I wonder if the LS1028A has a newer/older CAAM version. >> according to the device tree (fsl-ls1028a.dtsi) the sec is v5.0 (and >> also compatible >> with v4.0). If you have a look at the RM it says 7.0 (at least the >> MAJ_VER in >> SECVID_MS is 7 accoring to the manual; i haven't checked on the >> hardware for >> now. > > I've checked the imx8m mini HW, and it has reported: > Major: 4 > Minor: 1 > Era: 9 > > I believe that the LS family has a newer version of CAAM instantiated, > which can > be the reason on those register content deviations. probably, but as said above, we'd then need to distiguish between both. If you need to check PRIM_TZ which I haven't fully understood for now. > >> >> Horia, can you shed some light here. > > +1 > >> >> > -#define MSTRID_LIODN_MASK 0x0fff >> > +#define MSTRID_LIODN_MASK GENMASK(11, 0) >> > struct masterid { >> > u32 liodn_ms; /* lock and make-trusted control bits */ >> > u32 liodn_ls; /* LIODN for non-sequence and seq access */ >> > >> > base-commit: 8a796a1dfca2780321755033a74bca2bbe651680 >> >> -- >> -michael > > Link: [1]: > http://patchwork.ozlabs.org/project/uboot/patch/20211026065554.29009-4-gaurav.jain@nxp.com/ > > -- andrey -- -michael ^ permalink raw reply [flat|nested] 36+ messages in thread
* RE: [PATCH] crypto: caam - check jr permissions before probing 2021-11-05 23:16 ` Michael Walle @ 2021-11-08 10:24 ` ZHIZHIKIN Andrey 2021-11-08 16:13 ` Michael Walle 0 siblings, 1 reply; 36+ messages in thread From: ZHIZHIKIN Andrey @ 2021-11-08 10:24 UTC (permalink / raw) To: Michael Walle Cc: horia.geanta, pankaj.gupta, herbert, davem, iuliana.prodan, linux-crypto, linux-kernel Hello Michael, > -----Original Message----- > From: Michael Walle <michael@walle.cc> > Sent: Saturday, November 6, 2021 12:17 AM > To: ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com> > Cc: horia.geanta@nxp.com; pankaj.gupta@nxp.com; > herbert@gondor.apana.org.au; davem@davemloft.net; > iuliana.prodan@nxp.com; linux-crypto@vger.kernel.org; linux- > kernel@vger.kernel.org > Subject: Re: [PATCH] crypto: caam - check jr permissions before probing > > > Hi Andrey, > > Am 2021-11-05 11:34, schrieb ZHIZHIKIN Andrey: > >> -----Original Message----- > >> From: Michael Walle <michael@walle.cc> > >> Sent: Friday, November 5, 2021 2:21 AM > >> To: ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com> > >> Cc: horia.geanta@nxp.com; pankaj.gupta@nxp.com; > >> herbert@gondor.apana.org.au; davem@davemloft.net; > >> iuliana.prodan@nxp.com; linux-crypto@vger.kernel.org; linux- > >> kernel@vger.kernel.org > >> Subject: Re: [PATCH] crypto: caam - check jr permissions before > >> probing > >> > >> > >> Hi Andrey, > >> > >> Am 2021-11-04 17:21, schrieb Andrey Zhizhikin: > >> > Job Rings can be set to be exclusively used by TrustZone which > >> > makes the access to those rings only possible from Secure World. > >> > This access separation is defined by setting bits in CAAM JRxDID_MS > >> > register. Once reserved to be owned by TrustZone, this Job Ring > >> > becomes unavailable for the Kernel. This reservation is performed > >> > early in the boot process, even before the Kernel starts, which > >> > leads to unavailability of the HW at the probing stage. Moreover, > >> > the reservation can be done for any Job Ring and is not under > >> > control of the Kernel. The only condition that remains is: at least > >> > one Job Ring should be made available for the NS world. > >> > >> Where is that written down? > > > > If you refer to the condition statement: this is implied as without > > any JR initialized - it would not be possible to register a single > > algo. > > > > This stemmed out from the discussion of the patch in U-Boot (see [1]), > > where the it was indicated that JR0 is reserved for HAB on imx8m > > series. > > > > The naïve solution proposed was to just disable the child node, but I > > decided to look for a dynamic possibility to recognize those reserved > > JR nodes, hence this patch came out. > > First, thank you for taking the extra step here. Does "reserved for HAB" > mean TF-A is using it or is the BootROM already using it. TF-A is optional (so is HAB > I guess?). So it might be possible to have jr0 in linux, right? If that is correct, the > solution to disable the jr0 at compile time is wrong. From what I've seen in the U-Boot and ATF code, it seems that the JR0 is reserved by BootROM. When the execution reaches the ATF (after SPL) those bits are already set which concludes that the reservation is done quite early. Since current U-Boot does not have any support for CAAM integrated yet (patchset is under review) - it makes me believe that the reservation is done in BootROM. It is correct though: if the JR is not reserved - then it is accessible in Linux, hence compile-time disabling does not looked like a good solution to me. > > Thus it has to be done at run time. Either by removing/disabling the node in u- > boot or by not probing it. I'm not sure whats the correct solution here, though. I was looking at various possibilities here (including OF_DYNAMIC), but could not find any elegant solution that would cover this case so far. I'd continue to experiment to see what would be the most appropriate here. > > >> > Current implementation lists Job Rings as child nodes of CAAM > >> > driver, and tries to perform probing on those regardless of whether > >> > JR HW is accessible or not. > >> > > >> > This leads to the following error while probing: > >> > [ 1.509894] caam 30900000.crypto: job rings = 3, qi = 0 > >> > [ 1.525201] caam_jr 30901000.jr: failed to flush job ring 0 > >> > [ 1.525214] caam_jr: probe of 30901000.jr failed with error -5 > >> > > >> > Implement a dynamic mechanism to identify which Job Ring is > >> > actually marked as owned by TrustZone, and fail the probing of > >> > those child nodes with -ENODEV. > >> > > >> > Signed-off-by: Andrey Zhizhikin > >> > <andrey.zhizhikin@leica-geosystems.com> > >> > --- > >> > drivers/crypto/caam/ctrl.c | 18 ++++++++++++------ > >> > drivers/crypto/caam/intern.h | 1 + > >> > drivers/crypto/caam/jr.c | 17 +++++++++++++++++ > >> > drivers/crypto/caam/regs.h | 8 +++++--- > >> > 4 files changed, 35 insertions(+), 9 deletions(-) > >> > > >> > diff --git a/drivers/crypto/caam/ctrl.c > >> > b/drivers/crypto/caam/ctrl.c index ca0361b2dbb0..c48506a02340 > >> > 100644 > >> > --- a/drivers/crypto/caam/ctrl.c > >> > +++ b/drivers/crypto/caam/ctrl.c > >> > @@ -805,12 +805,18 @@ static int caam_probe(struct platform_device > >> > *pdev) > >> > for_each_available_child_of_node(nprop, np) > >> > if (of_device_is_compatible(np, "fsl,sec-v4.0-job-ring") || > >> > of_device_is_compatible(np, "fsl,sec4.0-job-ring")) { > >> > - ctrlpriv->jr[ring] = (struct caam_job_ring __iomem __force *) > >> > - ((__force uint8_t *)ctrl + > >> > - (ring + JR_BLOCK_NUMBER) * > >> > - BLOCK_OFFSET > >> > - ); > >> > - ctrlpriv->total_jobrs++; > >> > + /* Check if the JR is not owned exclusively by TZ */ > >> > + if (!((rd_reg32(&ctrl->jr_mid[ring].liodn_ms)) & > >> > + (MSTRID_LOCK_TZ_OWN | > >> > + MSTRID_LOCK_PRIM_TZ))) { > >> > >> what is the PRIM_TZ bit? I don't see it in my reference manual (which > >> is for the LS1028A). > > > > See my comment below regarding this bit. > > > >> > >> Can't we just do a > >> if (rd_reg32(&ctrl->jr_mid[ring].liodn_ms) & > >> (MSTRID_LOCK_TZ_OWN | MSTRID_LOCK_PRIM_TZ)) > >> continue; > > > > Yes, this would work as well. I'll address this is V2. > > > >> > >> > + ctrlpriv->jr[ring] = (struct caam_job_ring __iomem __force > *) > >> > + ((__force uint8_t *)ctrl + > >> > + (ring + JR_BLOCK_NUMBER) * > >> > + BLOCK_OFFSET > >> > + ); > >> > >> This isn't really used at all. Can we drop "jr" from struct > >> caam_drv_private altogether? See also below. > > > > Agree. I was contemplating to remove this, as the caam_jr does its own > > devm_ioremap in the probe and does not use what caam driver passes > > along. But I decided not to address this with this patch, since this > > is not related to the issue this change addresses. > > > > In general, this driver needs a bit of TLC, and I can take care of > > those points (together with > > ctrlpriv->total_jobrs) in a separate series, unless there are strong > > objections arises that this cleanup > > should come along this patch. > > If you clean up later, probably most of this code is going away, no? > Then whats the point in having this patch in the first place. Usually its the other > way around. True, this is what I've realized once I looked at the implementation again. I would include the clean-up and re-spin this patch as a series which would contain it as well. Thanks for pointing it out! > > >> > + /* Indicate that this JR is available for NS */ > >> > + ctrlpriv->jobr_ns_flags |= BIT(ring); > >> > + ctrlpriv->total_jobrs++; > >> > >> as well as this? > > > > Noted. > > > >> > >> > + } > >> > ring++; > >> > } > >> > > >> > diff --git a/drivers/crypto/caam/intern.h > >> > b/drivers/crypto/caam/intern.h index 7d45b21bd55a..2919af55dbfe > >> > 100644 > >> > --- a/drivers/crypto/caam/intern.h > >> > +++ b/drivers/crypto/caam/intern.h > >> > @@ -91,6 +91,7 @@ struct caam_drv_private { > >> > * or from register-based version detection code > >> > */ > >> > u8 total_jobrs; /* Total Job Rings in device */ > >> > + u8 jobr_ns_flags; /* Flags for each Job Ring if it is not owned by > >> > TZ*/ > >> > u8 qi_present; /* Nonzero if QI present in device */ > >> > u8 mc_en; /* Nonzero if MC f/w is active */ > >> > int secvio_irq; /* Security violation interrupt number */ > >> > diff --git a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c > >> > index > >> > 7f2b1101f567..a260981e0843 100644 > >> > --- a/drivers/crypto/caam/jr.c > >> > +++ b/drivers/crypto/caam/jr.c > >> > @@ -511,10 +511,27 @@ static int caam_jr_probe(struct > >> > platform_device > >> > *pdev) > >> > struct device_node *nprop; > >> > struct caam_job_ring __iomem *ctrl; > >> > struct caam_drv_private_jr *jrpriv; > >> > + struct caam_drv_private *caamctrlpriv; > >> > static int total_jobrs; > >> > >> ugh. > > > > Yes, I've seen that. At first, I even wanted to replace it with the > > ctrlpriv->total_jobrs, > > but decided not to do it in order to keep this patch focused. > > Having the total_jobrs (and using it for anything else than messages) static will > create an unnecessary dependency on the correct probe order. Yes, I've stumbled upon this logical problem myself as well. I'd decided that this should go, and would replace it with the option to use IRBAR_JRx as the indexing, since it has the address aligned and can be used as a bit index. - For LS1028A it would look like: IRBAR_JR[ring_id] >> 16 - For i.MX8M series it would be: IRBAR_JR[ring_id] >> 12 As those offsets are defined in the HW, they can be reliably used as indexing parameter to interrogate the CAAM if the JR is reserved for TZ or not. In addition, in order not to access the caam_ctrl register set from caam_jr driver, I'd still prefer to use a bitmask and compare the bits set against that new indexing. This would allow the driver to get rid of that static total_jobrs part at all. I would appreciate the community opinion on the approach above whether it is plausible and does not violate any kernel rules. > > >> > struct resource *r; > >> > int error; > >> > > >> > + /* Before we proceed with the JR device probing, verify > >> > + * that the job ring itself is available to Non-Secure world. > >> > + * If the JR is owned exclusively by TZ - we should not even > >> > + * process it further. > >> > + */ > >> > + caamctrlpriv = dev_get_drvdata(pdev->dev.parent); > >> > + if (!(caamctrlpriv->jobr_ns_flags & BIT(total_jobrs))) { > >> > >> Is anything preventing from total_jobrs getting big? Can't we get rid > >> of this static variable somehow? Before this commit, it was just used > >> for messages. > > > > I guess not, the only limitation we have is the HW. Current imx8m mini > > does have > > 3 Job Rings, and plus added one more. Since I do not have any > > Layerscape HW - I cannot really tell if the number of instantiated Job > > Rings there is bigger than 8 and would grow in the future. > > For now the LS1028A has 4. I had a look at the LS1028A SRM, and can confirm it does indeed have 4 JR. > > > I had a local implementation version with set_bit/test_bit variant, > > perhaps that one would be more appropriate here. If it's OK - I can do > > that one and push it in V2. > > > >> Can we check the TZ bit here instead of doing that bitflags dance? > > > > Honestly, I had this implementation locally as well, but decided to go > > ahead with "the dance" in order not to access the registers of another > > module from this one. > > Ahh I didn't notice that the register was part of the parent. Meh. > > > Besides, the JR node loop in present caam_probe() got me tripped, as > > it does an early lookup and analysis of JR child nodes and I found it > > a right place to analyze and record the JR availability. > > I see. But we should really communicate whether the child should return > ENODEV in a different way. IMHO this static thing is really fragile. If I go with the indexing option described above - this should be solved. > > >> > >> nitpick: in caam there are no netdev comments. So multiline comments > >> look like: > >> /* > >> * this is a comment. > >> */ > > > > Noted, will be addressed in V2. > > > >> > >> > + /* This job ring is marked to be exclusively used by TZ, > >> > + * do not proceed with probing as the HW block is inaccessible. > >> > + * Increment total seen JR devices since it is used as the index > >> > + * into verification and fail probing for this node. > >> > + */ > >> > + total_jobrs++; > >> > + return -ENODEV; > >> > + } > >> > + > >> > jrdev = &pdev->dev; > >> > jrpriv = devm_kzalloc(jrdev, sizeof(*jrpriv), GFP_KERNEL); > >> > if (!jrpriv) > >> > diff --git a/drivers/crypto/caam/regs.h > >> > b/drivers/crypto/caam/regs.h index 3738625c0250..8ade617f9e65 > >> > 100644 > >> > --- a/drivers/crypto/caam/regs.h > >> > +++ b/drivers/crypto/caam/regs.h > >> > @@ -445,10 +445,12 @@ struct caam_perfmon { }; > >> > > >> > /* LIODN programming for DMA configuration */ > >> > -#define MSTRID_LOCK_LIODN 0x80000000 > >> > -#define MSTRID_LOCK_MAKETRUSTED 0x00010000 /* only for JR > >> masterid */ > >> > +#define MSTRID_LOCK_LIODN BIT(31) > >> > +#define MSTRID_LOCK_MAKETRUSTED BIT(16) /* only for JR: masterid > */ > >> > +#define MSTRID_LOCK_TZ_OWN BIT(15) /* only for JR: owned by TZ > */ > >> > +#define MSTRID_LOCK_PRIM_TZ BIT(4) /* only for JR: Primary TZ */ > >> > >> can't find that one. > >> > >> in general, does these marcros match with your reference manual? > >> Which one do you have? > > > > I'm working on the imx8m mini, which has a v4.0 of CAAM, and this bit > > is defined in JR[0:2]DID_MS registers. > > > > The map looks like following: > > LDID[31], USE_OUT[30], PRIM_ICID[29:19], LAMTD[17], AMTD[16], > > TZ_OWN[15], SDID_MS[14:5], PRIM_TZ[4], PRIM_DID[3:0] > > > > Perhaps, there is a deviation here between what is instantiated in LS > > vs i.MX series. > > > > At this point, I would also be interested to hear back from NXP on > > this. > > Probably, but that would also mean we'd have to distiguish between these too. > You're checking PRIM_TZ which is SDID on the LS1028A (which is an arbitrary > number if I understand it correctly). So the check above might actually trigger > although it shouldn't. It's maybe the opposite though: on the LS1028A if the TZ is set, then NS would read SDID as all 0's. This presents the problem that PRIM_TZ bit defined for i.MX8M series would read as 0 on LS series and fail the reservation check. At this point I'd really like someone from NXP to comment on it, specifically: is it enough to just check the TZ bit for all families to recognize that JR is reserved for usage in Secure world only? > > That said, whats PRIM_TZ? When is it set? It is set together with TZ_OWN early at the boot and is used for several purposes, namely: to derive SDID_MS (it is done dynamically), and also to indicate that the access to that JR registers (config, interrupt, buffers, etc.) are only possible from Secure World. > > >> for me the bits are named: > >> LICID[31], AMTD[16], TZ[15] and SDID[11:0] also the register is > >> called JRnICID. > >> > >> I wonder if the LS1028A has a newer/older CAAM version. > >> according to the device tree (fsl-ls1028a.dtsi) the sec is v5.0 (and > >> also compatible with v4.0). If you have a look at the RM it says 7.0 > >> (at least the MAJ_VER in SECVID_MS is 7 accoring to the manual; i > >> haven't checked on the hardware for now. > > > > I've checked the imx8m mini HW, and it has reported: > > Major: 4 > > Minor: 1 > > Era: 9 > > > > I believe that the LS family has a newer version of CAAM instantiated, > > which can be the reason on those register content deviations. > > probably, but as said above, we'd then need to distiguish between both. If you > need to check PRIM_TZ which I haven't fully understood for now. Correct, unless somebody from NXP could confirm that checking only TZ bit is sufficient to understand that JR is reserved. In this case PRIM_TZ does not need to be checked on i.MX8M series. > > > > >> > >> Horia, can you shed some light here. > > > > +1 > > > >> > >> > -#define MSTRID_LIODN_MASK 0x0fff > >> > +#define MSTRID_LIODN_MASK GENMASK(11, 0) > >> > struct masterid { > >> > u32 liodn_ms; /* lock and make-trusted control bits */ > >> > u32 liodn_ls; /* LIODN for non-sequence and seq access */ > >> > > >> > base-commit: 8a796a1dfca2780321755033a74bca2bbe651680 > >> > >> -- > >> -michael > > > > Link: [1]: http://patchwork.ozlabs.org/project/uboot/patch/20211026065554.29009-4-gaurav.jain@nxp.com/ > > > > -- andrey > > -- > -michael ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] crypto: caam - check jr permissions before probing 2021-11-08 10:24 ` ZHIZHIKIN Andrey @ 2021-11-08 16:13 ` Michael Walle 2021-11-10 9:33 ` ZHIZHIKIN Andrey 0 siblings, 1 reply; 36+ messages in thread From: Michael Walle @ 2021-11-08 16:13 UTC (permalink / raw) To: ZHIZHIKIN Andrey Cc: horia.geanta, pankaj.gupta, herbert, davem, iuliana.prodan, linux-crypto, linux-kernel Hi Andrey, >> First, thank you for taking the extra step here. Does "reserved for >> HAB" >> mean TF-A is using it or is the BootROM already using it. TF-A is >> optional (so is HAB >> I guess?). So it might be possible to have jr0 in linux, right? If >> that is correct, the >> solution to disable the jr0 at compile time is wrong. > > From what I've seen in the U-Boot and ATF code, it seems that the JR0 > is reserved > by BootROM. When the execution reaches the ATF (after SPL) those bits > are already > set which concludes that the reservation is done quite early. Since > current U-Boot > does not have any support for CAAM integrated yet (patchset is under > review) - > it makes me believe that the reservation is done in BootROM. Ok. I guess we have to wait for an answer from NXP. But it strikes as odd that it there is no Secure World, you'll loose one job ring. > It is correct though: if the JR is not reserved - then it is > accessible in Linux, hence > compile-time disabling does not looked like a good solution to me. Mh, I had a closer look at the IMX8M SRM (I don't have one for the IMX8MM yet). It looks like secure world can reassign the Job Ring to non-secure world though (unless LDID is set). If that is the case I think, deciding at probe time if a job ring is available is not correct; as it can be reassigned later. So maybe u-boot (or TF-A) should mark that node as disabled after all. If the BootROM is actually already assigning this to secure world (and setting the lock bit LDID). Then it can also be removed from the linux dtsi, because there is no way it can be assigned to linux anyways. .. >> >> > diff --git a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c >> >> > index >> >> > 7f2b1101f567..a260981e0843 100644 >> >> > --- a/drivers/crypto/caam/jr.c >> >> > +++ b/drivers/crypto/caam/jr.c >> >> > @@ -511,10 +511,27 @@ static int caam_jr_probe(struct >> >> > platform_device >> >> > *pdev) >> >> > struct device_node *nprop; >> >> > struct caam_job_ring __iomem *ctrl; >> >> > struct caam_drv_private_jr *jrpriv; >> >> > + struct caam_drv_private *caamctrlpriv; >> >> > static int total_jobrs; >> >> >> >> ugh. >> > >> > Yes, I've seen that. At first, I even wanted to replace it with the >> > ctrlpriv->total_jobrs, >> > but decided not to do it in order to keep this patch focused. >> >> Having the total_jobrs (and using it for anything else than messages) >> static will >> create an unnecessary dependency on the correct probe order. > > Yes, I've stumbled upon this logical problem myself as well. > > I'd decided that this should go, and would replace it with the option > to use > IRBAR_JRx as the indexing, since it has the address aligned and can be > used as a bit index. > - For LS1028A it would look like: IRBAR_JR[ring_id] >> 16 > - For i.MX8M series it would be: IRBAR_JR[ring_id] >> 12 > > As those offsets are defined in the HW, they can be reliably used as > indexing parameter > to interrogate the CAAM if the JR is reserved for TZ or not. > > In addition, in order not to access the caam_ctrl register set from > caam_jr driver, I'd still > prefer to use a bitmask and compare the bits set against that new > indexing. This would > allow the driver to get rid of that static total_jobrs part at all. > > I would appreciate the community opinion on the approach above whether > it is plausible > and does not violate any kernel rules. Will try to follow you here later. .. >> >> in general, does these marcros match with your reference manual? >> >> Which one do you have? >> > >> > I'm working on the imx8m mini, which has a v4.0 of CAAM, and this bit >> > is defined in JR[0:2]DID_MS registers. >> > >> > The map looks like following: >> > LDID[31], USE_OUT[30], PRIM_ICID[29:19], LAMTD[17], AMTD[16], >> > TZ_OWN[15], SDID_MS[14:5], PRIM_TZ[4], PRIM_DID[3:0] >> > >> > Perhaps, there is a deviation here between what is instantiated in LS >> > vs i.MX series. >> > >> > At this point, I would also be interested to hear back from NXP on >> > this. >> >> Probably, but that would also mean we'd have to distiguish between >> these too. >> You're checking PRIM_TZ which is SDID on the LS1028A (which is an >> arbitrary >> number if I understand it correctly). So the check above might >> actually trigger >> although it shouldn't. > > It's maybe the opposite though: on the LS1028A if the TZ is set, then > NS would > read SDID as all 0's. This presents the problem that PRIM_TZ bit > defined for i.MX8M > series would read as 0 on LS series and fail the reservation check. I don't think you have to take the PRIM_TZ bit into account. PRIM_TZ=1 implies OWN_TZ=1. (I'm not sure what PRIM_TZ=0 and OWN_TZ=1 is good for though). But as mentioned above, I'm not convinced that deciding at probe time is the solution here. > At this point I'd really like someone from NXP to comment on it, > specifically: is it enough > to just check the TZ bit for all families to recognize that JR is > reserved for usage in > Secure world only? yep. >> >> That said, whats PRIM_TZ? When is it set? > > It is set together with TZ_OWN early at the boot and is used for > several purposes, namely: > to derive SDID_MS (it is done dynamically), and also to indicate that > the access to that JR > registers (config, interrupt, buffers, etc.) are only possible from > Secure World. Thanks, I also read the SRM for this bit, right now. -michael ^ permalink raw reply [flat|nested] 36+ messages in thread
* RE: [PATCH] crypto: caam - check jr permissions before probing 2021-11-08 16:13 ` Michael Walle @ 2021-11-10 9:33 ` ZHIZHIKIN Andrey 2021-11-12 18:55 ` Michael Walle 0 siblings, 1 reply; 36+ messages in thread From: ZHIZHIKIN Andrey @ 2021-11-10 9:33 UTC (permalink / raw) To: Michael Walle Cc: horia.geanta, pankaj.gupta, herbert, davem, iuliana.prodan, linux-crypto, linux-kernel Hello Michael, > -----Original Message----- > From: Michael Walle <michael@walle.cc> > Sent: Monday, November 8, 2021 5:13 PM > To: ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com> > Cc: horia.geanta@nxp.com; pankaj.gupta@nxp.com; > herbert@gondor.apana.org.au; davem@davemloft.net; > iuliana.prodan@nxp.com; linux-crypto@vger.kernel.org; linux- > kernel@vger.kernel.org > Subject: Re: [PATCH] crypto: caam - check jr permissions before probing > > > Hi Andrey, > > >> First, thank you for taking the extra step here. Does "reserved for > >> HAB" > >> mean TF-A is using it or is the BootROM already using it. TF-A is > >> optional (so is HAB > >> I guess?). So it might be possible to have jr0 in linux, right? If > >> that is correct, the > >> solution to disable the jr0 at compile time is wrong. > > > > From what I've seen in the U-Boot and ATF code, it seems that the JR0 > > is reserved > > by BootROM. When the execution reaches the ATF (after SPL) those bits > > are already > > set which concludes that the reservation is done quite early. Since > > current U-Boot > > does not have any support for CAAM integrated yet (patchset is under > > review) - > > it makes me believe that the reservation is done in BootROM. > > Ok. I guess we have to wait for an answer from NXP. But it strikes as > odd that it there is no Secure World, you'll loose one job ring. From HW perspective, the JR is not lost - it is just assigned to S world. This also provides an opportunity (at least for i.MX8M series) to issue transactions and create Trusted descriptors in S world for NS world. This is achieved by 2 sets of ICID/DID pairs, and this is where USE_OUT bit is actually used. This however is missing on the LS series (SRM does not state this is implemented), which leaves LS with only one ICID/DID pair per ring. From OS perspective however, I would totally agree - the JR is indeed lost, even if there is no software running that required S world. The only description of process for control transfer from S to NS world I was able to find is described in LS1028A SRM section 12.2.2.3, which only details ring user re-assignment, but it does not detail whether TZ_OWN can participate in this process (set or reset), and this is also similar for i.MX8M family. > > > It is correct though: if the JR is not reserved - then it is > > accessible in Linux, hence > > compile-time disabling does not looked like a good solution to me. > > Mh, I had a closer look at the IMX8M SRM (I don't have one for the > IMX8MM yet). It looks like secure world can reassign the Job Ring > to non-secure world though (unless LDID is set). If that is the > case I think, deciding at probe time if a job ring is available is > not correct; as it can be reassigned later. That's exactly the culprit here: the LDID is not set on the JR reserved. This makes it possible for the code executing in S to transfer the JR to NS. Practically, I do not see that this would happen though, as even the NXP proposed to disable the node at compile time, which gives me an indication that the transfer was never planned. This is however a dangerous assumption I have to admit, and in the general case - this transfer can occur. Moreover, from what I read in the SRM of both i.MX8MM and LS1028A - there is no lock that is imposed on TZ_OWN bit by setting the LDID (or LICID for LS family). Would it be possible for you to tell which section of SRM provides a description of the JR transfer you mentioned above? As for probing of the JR node, then I believe it is rather the opposite: deciding whether the JR is available during probing provides an opportunity to bind the device later on when it would be released from S to NS world (provided that this would ever occur). However, keeping in mind that there is no HW mechanism to indicate that the JR appears in NS world after the boot and the user transfer should be done manually by some other SW entity - later bind can also be performed there. The only difference to the current proposed solution would be to examine the CAAM control register instead of the flag from JR while probing, and this is what I'm currently testing on my end. > > So maybe u-boot (or TF-A) should mark that node as disabled after > all. If the U-Boot implementation would come up with similar dynamic recognition - then it would not be necessary to disable the node there as well. This would also ensure that if in later derivatives or ATF code updates another JR would be reserved as well - there would be no need to change and align DTB to it, as it would be dynamically recognized. > > If the BootROM is actually already assigning this to secure world > (and setting the lock bit LDID). Then it can also be removed from > the linux dtsi, because there is no way it can be assigned to linux > anyways. As I've indicated above: the LDID bit is not set on this JR. > > .. > > >> >> > diff --git a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c > >> >> > index > >> >> > 7f2b1101f567..a260981e0843 100644 > >> >> > --- a/drivers/crypto/caam/jr.c > >> >> > +++ b/drivers/crypto/caam/jr.c > >> >> > @@ -511,10 +511,27 @@ static int caam_jr_probe(struct > >> >> > platform_device > >> >> > *pdev) > >> >> > struct device_node *nprop; > >> >> > struct caam_job_ring __iomem *ctrl; > >> >> > struct caam_drv_private_jr *jrpriv; > >> >> > + struct caam_drv_private *caamctrlpriv; > >> >> > static int total_jobrs; > >> >> > >> >> ugh. > >> > > >> > Yes, I've seen that. At first, I even wanted to replace it with the > >> > ctrlpriv->total_jobrs, > >> > but decided not to do it in order to keep this patch focused. > >> > >> Having the total_jobrs (and using it for anything else than messages) > >> static will > >> create an unnecessary dependency on the correct probe order. > > > > Yes, I've stumbled upon this logical problem myself as well. > > > > I'd decided that this should go, and would replace it with the option > > to use > > IRBAR_JRx as the indexing, since it has the address aligned and can be > > used as a bit index. > > - For LS1028A it would look like: IRBAR_JR[ring_id] >> 16 > > - For i.MX8M series it would be: IRBAR_JR[ring_id] >> 12 > > > > As those offsets are defined in the HW, they can be reliably used as > > indexing parameter > > to interrogate the CAAM if the JR is reserved for TZ or not. > > > > In addition, in order not to access the caam_ctrl register set from > > caam_jr driver, I'd still > > prefer to use a bitmask and compare the bits set against that new > > indexing. This would > > allow the driver to get rid of that static total_jobrs part at all. > > > > I would appreciate the community opinion on the approach above whether > > it is plausible > > and does not violate any kernel rules. > > Will try to follow you here later. I'm now working on a patchset that would supersede this one, and would include the dynamic indexing based on the JR address instead of that static variable used. This would also allow to re-order JR nodes inside the DTS and do not rely on the order of appearance. > > .. > > >> >> in general, does these marcros match with your reference manual? > >> >> Which one do you have? > >> > > >> > I'm working on the imx8m mini, which has a v4.0 of CAAM, and this bit > >> > is defined in JR[0:2]DID_MS registers. > >> > > >> > The map looks like following: > >> > LDID[31], USE_OUT[30], PRIM_ICID[29:19], LAMTD[17], AMTD[16], > >> > TZ_OWN[15], SDID_MS[14:5], PRIM_TZ[4], PRIM_DID[3:0] > >> > > >> > Perhaps, there is a deviation here between what is instantiated in LS > >> > vs i.MX series. > >> > > >> > At this point, I would also be interested to hear back from NXP on > >> > this. > >> > >> Probably, but that would also mean we'd have to distiguish between > >> these too. > >> You're checking PRIM_TZ which is SDID on the LS1028A (which is an > >> arbitrary > >> number if I understand it correctly). So the check above might > >> actually trigger > >> although it shouldn't. > > > > It's maybe the opposite though: on the LS1028A if the TZ is set, then > > NS would > > read SDID as all 0's. This presents the problem that PRIM_TZ bit > > defined for i.MX8M > > series would read as 0 on LS series and fail the reservation check. > > I don't think you have to take the PRIM_TZ bit into account. PRIM_TZ=1 > implies OWN_TZ=1. (I'm not sure what PRIM_TZ=0 and OWN_TZ=1 is good > for though). But as mentioned above, I'm not convinced that deciding > at probe time is the solution here. From what I read, PRIM_TZ bit is mixed into the SDID and also "locks" JR register interface to S world. Setting PRIM_TZ=0 and TZ_OWN=1 has primarily an influence of SDID construction, this is outlined in JRsDID_MS register description. > > > At this point I'd really like someone from NXP to comment on it, > > specifically: is it enough > > to just check the TZ bit for all families to recognize that JR is > > reserved for usage in > > Secure world only? > > yep. I've compared both i.MX8M and LS family SRMs, and looks like the OWN_TZ bit is the only unification point here that can be verified. I 'm currently testing the implementation where only that bit is checked and so far I have good results. I would post a V2 as a series and supersede this patch, where only that check would be included. > > >> > >> That said, whats PRIM_TZ? When is it set? > > > > It is set together with TZ_OWN early at the boot and is used for > > several purposes, namely: > > to derive SDID_MS (it is done dynamically), and also to indicate that > > the access to that JR > > registers (config, interrupt, buffers, etc.) are only possible from > > Secure World. > > Thanks, I also read the SRM for this bit, right now. > > -michael ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] crypto: caam - check jr permissions before probing 2021-11-10 9:33 ` ZHIZHIKIN Andrey @ 2021-11-12 18:55 ` Michael Walle 2021-11-15 9:38 ` ZHIZHIKIN Andrey 0 siblings, 1 reply; 36+ messages in thread From: Michael Walle @ 2021-11-12 18:55 UTC (permalink / raw) To: ZHIZHIKIN Andrey Cc: horia.geanta, pankaj.gupta, herbert, davem, iuliana.prodan, linux-crypto, linux-kernel Hi Andrey, Am 2021-11-10 10:33, schrieb ZHIZHIKIN Andrey: >> -----Original Message----- >> From: Michael Walle <michael@walle.cc> >> >> First, thank you for taking the extra step here. Does "reserved for >> >> HAB" >> >> mean TF-A is using it or is the BootROM already using it. TF-A is >> >> optional (so is HAB >> >> I guess?). So it might be possible to have jr0 in linux, right? If >> >> that is correct, the >> >> solution to disable the jr0 at compile time is wrong. >> > >> > From what I've seen in the U-Boot and ATF code, it seems that the JR0 >> > is reserved >> > by BootROM. When the execution reaches the ATF (after SPL) those bits >> > are already >> > set which concludes that the reservation is done quite early. Since >> > current U-Boot >> > does not have any support for CAAM integrated yet (patchset is under >> > review) - >> > it makes me believe that the reservation is done in BootROM. >> >> Ok. I guess we have to wait for an answer from NXP. But it strikes as >> odd that it there is no Secure World, you'll loose one job ring. > > From HW perspective, the JR is not lost - it is just assigned to S > world. I said its lost if there is no Secure World (which IMHO is still a valid case). I mean if its already the BootROM which assign it (unconditionally) and there will be no secure world later in the boot process, then its lost. > This also provides an opportunity (at least for i.MX8M series) to issue > transactions and create Trusted descriptors in S world for NS world. > This is achieved by 2 sets of ICID/DID pairs, and this is where USE_OUT > bit is actually used. This however is missing on the LS series (SRM > does > not state this is implemented), which leaves LS with only one ICID/DID > pair per ring. But this would also be possible if the JR is only acquired later by TF-A (or optee), no? > From OS perspective however, I would totally agree - the JR is indeed > lost, even if there is no software running that required S world. > > The only description of process for control transfer from S to NS world > I was able to find is described in LS1028A SRM section 12.2.2.3, which > only details ring user re-assignment, but it does not detail whether > TZ_OWN can participate in this process (set or reset), and this is also > similar for i.MX8M family. > >> >> > It is correct though: if the JR is not reserved - then it is >> > accessible in Linux, hence >> > compile-time disabling does not looked like a good solution to me. >> >> Mh, I had a closer look at the IMX8M SRM (I don't have one for the >> IMX8MM yet). It looks like secure world can reassign the Job Ring >> to non-secure world though (unless LDID is set). If that is the >> case I think, deciding at probe time if a job ring is available is >> not correct; as it can be reassigned later. > > That's exactly the culprit here: the LDID is not set on the JR > reserved. > > This makes it possible for the code executing in S to transfer the JR > to NS. > Practically, I do not see that this would happen though, as even the > NXP > proposed to disable the node at compile time, which gives me an > indication > that the transfer was never planned. This is however a dangerous > assumption > I have to admit, and in the general case - this transfer can occur. > > Moreover, from what I read in the SRM of both i.MX8MM and LS1028A - > there is no lock that is imposed on TZ_OWN bit by setting the LDID (or > LICID > for LS family). I've noticed that too, but then assumed that because TZ_PRIM=1 implies TZ_OWN=1 and the lock bit will lock TZ_PRIM then TZ_OWN must also be 1. But that's not the case for LS SoCs. > Would it be possible for you to tell which section of SRM provides a > description > of the JR transfer you mentioned above? I don't have access to the IMX8M SEC right now. If memory serves correctly, I just saw that on an overview. But I just had a look at the LS1028ASECRM, and there is "SEC's Job Ring interface can be independently assigned (and re-assigned) to different users." (ch 12.2). > As for probing of the JR node, then I believe it is rather the > opposite: > deciding whether the JR is available during probing provides an > opportunity to > bind the device later on when it would be released from S to NS world > (provided that this would ever occur). However, keeping in mind that > there is > no HW mechanism to indicate that the JR appears in NS world after the > boot > and the user transfer should be done manually by some other SW entity - > later > bind can also be performed there. Sure, but it will also be the other way around, no? Like S world can "steal" the JR from NS world. And thats what I'm worried about. > The only difference to the current proposed solution would be to > examine the > CAAM control register instead of the flag from JR while probing, and > this is what > I'm currently testing on my end. > >> >> So maybe u-boot (or TF-A) should mark that node as disabled after >> all. > > If the U-Boot implementation would come up with similar dynamic > recognition - > then it would not be necessary to disable the node there as well. > > This would also ensure that if in later derivatives or ATF code updates > another > JR would be reserved as well - there would be no need to change and > align DTB > to it, as it would be dynamically recognized. To be clear, I don't talk about dynamic behavior here. Just try to determine where the JR should be disabled/removed from the DT. And I'm assuming a static partition of the JRs between S and NS world. To recap, NXP suggested to disabled it in the SoC dtsi in u-boot. This depends on whether the BootROM actually assignes it to S worlds and there is no way for u-boot to regain access (assuming that u-boot or u-boot SPL will be started in EL3). If it is not possible to reassign it to NS world, then the JR should be disabled in linux and u-boot DTs. If there is a chance to regain control and that there might be no TF-A at all, then statically disable the JR in u-boot is wrong. Instead it should be determined at runtime (again just static partitioning, no dynamic reassignment). I had a fixup at runtime of the DT (both the active DT in u-boot as well as the DT passed from u-boot to linux) in mind. Check the TZ_OWN bit and remove/disable the JR. There is also an ongoing discussion where and how the DT will be passed to u-boot and linux. So I might be the case that TF-A will allocate one JR to itself and just pass u-boot the DT where that JR is disabled or removed. Which might also fit the "fixup" in u-boot. >> If the BootROM is actually already assigning this to secure world >> (and setting the lock bit LDID). Then it can also be removed from >> the linux dtsi, because there is no way it can be assigned to linux >> anyways. > > As I've indicated above: the LDID bit is not set on this JR. Ok, then u-boot should be able to reset the JR to its defaults if its started in EL3 (and there is no TF-A at all), right? >> >> >> > diff --git a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c >> >> >> > index >> >> >> > 7f2b1101f567..a260981e0843 100644 >> >> >> > --- a/drivers/crypto/caam/jr.c >> >> >> > +++ b/drivers/crypto/caam/jr.c >> >> >> > @@ -511,10 +511,27 @@ static int caam_jr_probe(struct >> >> >> > platform_device >> >> >> > *pdev) >> >> >> > struct device_node *nprop; >> >> >> > struct caam_job_ring __iomem *ctrl; >> >> >> > struct caam_drv_private_jr *jrpriv; >> >> >> > + struct caam_drv_private *caamctrlpriv; >> >> >> > static int total_jobrs; >> >> >> >> >> >> ugh. >> >> > >> >> > Yes, I've seen that. At first, I even wanted to replace it with the >> >> > ctrlpriv->total_jobrs, >> >> > but decided not to do it in order to keep this patch focused. >> >> >> >> Having the total_jobrs (and using it for anything else than messages) >> >> static will >> >> create an unnecessary dependency on the correct probe order. >> > >> > Yes, I've stumbled upon this logical problem myself as well. >> > >> > I'd decided that this should go, and would replace it with the option >> > to use >> > IRBAR_JRx as the indexing, since it has the address aligned and can be >> > used as a bit index. >> > - For LS1028A it would look like: IRBAR_JR[ring_id] >> 16 >> > - For i.MX8M series it would be: IRBAR_JR[ring_id] >> 12 >> > >> > As those offsets are defined in the HW, they can be reliably used as >> > indexing parameter >> > to interrogate the CAAM if the JR is reserved for TZ or not. >> > >> > In addition, in order not to access the caam_ctrl register set from >> > caam_jr driver, I'd still >> > prefer to use a bitmask and compare the bits set against that new >> > indexing. This would >> > allow the driver to get rid of that static total_jobrs part at all. >> > >> > I would appreciate the community opinion on the approach above whether >> > it is plausible >> > and does not violate any kernel rules. >> >> Will try to follow you here later. > > I'm now working on a patchset that would supersede this one, and would > include the dynamic indexing based on the JR address instead of that > static > variable used. This would also allow to re-order JR nodes inside the > DTS and > do not rely on the order of appearance. > >> >> .. >> >> >> >> in general, does these marcros match with your reference manual? >> >> >> Which one do you have? >> >> > >> >> > I'm working on the imx8m mini, which has a v4.0 of CAAM, and this bit >> >> > is defined in JR[0:2]DID_MS registers. >> >> > >> >> > The map looks like following: >> >> > LDID[31], USE_OUT[30], PRIM_ICID[29:19], LAMTD[17], AMTD[16], >> >> > TZ_OWN[15], SDID_MS[14:5], PRIM_TZ[4], PRIM_DID[3:0] >> >> > >> >> > Perhaps, there is a deviation here between what is instantiated in LS >> >> > vs i.MX series. >> >> > >> >> > At this point, I would also be interested to hear back from NXP on >> >> > this. >> >> >> >> Probably, but that would also mean we'd have to distiguish between >> >> these too. >> >> You're checking PRIM_TZ which is SDID on the LS1028A (which is an >> >> arbitrary >> >> number if I understand it correctly). So the check above might >> >> actually trigger >> >> although it shouldn't. >> > >> > It's maybe the opposite though: on the LS1028A if the TZ is set, then >> > NS would >> > read SDID as all 0's. This presents the problem that PRIM_TZ bit >> > defined for i.MX8M >> > series would read as 0 on LS series and fail the reservation check. >> >> I don't think you have to take the PRIM_TZ bit into account. PRIM_TZ=1 >> implies OWN_TZ=1. (I'm not sure what PRIM_TZ=0 and OWN_TZ=1 is good >> for though). But as mentioned above, I'm not convinced that deciding >> at probe time is the solution here. > > From what I read, PRIM_TZ bit is mixed into the SDID and also "locks" > JR > register interface to S world. Setting PRIM_TZ=0 and TZ_OWN=1 has > primarily an influence of SDID construction, this is outlined in > JRsDID_MS > register description. > >> >> > At this point I'd really like someone from NXP to comment on it, >> > specifically: is it enough >> > to just check the TZ bit for all families to recognize that JR is >> > reserved for usage in >> > Secure world only? >> >> yep. > > I've compared both i.MX8M and LS family SRMs, and looks like the > OWN_TZ bit is the only unification point here that can be verified. > > I 'm currently testing the implementation where only that bit is > checked and so far I have good results. I would post a V2 as a series > and supersede this patch, where only that check would be included. > >> >> >> >> >> That said, whats PRIM_TZ? When is it set? >> > >> > It is set together with TZ_OWN early at the boot and is used for >> > several purposes, namely: >> > to derive SDID_MS (it is done dynamically), and also to indicate that >> > the access to that JR >> > registers (config, interrupt, buffers, etc.) are only possible from >> > Secure World. >> >> Thanks, I also read the SRM for this bit, right now. >> >> -michael -- -michael ^ permalink raw reply [flat|nested] 36+ messages in thread
* RE: [PATCH] crypto: caam - check jr permissions before probing 2021-11-12 18:55 ` Michael Walle @ 2021-11-15 9:38 ` ZHIZHIKIN Andrey 2021-11-15 11:40 ` Michael Walle 0 siblings, 1 reply; 36+ messages in thread From: ZHIZHIKIN Andrey @ 2021-11-15 9:38 UTC (permalink / raw) To: Michael Walle Cc: horia.geanta, pankaj.gupta, herbert, davem, iuliana.prodan, linux-crypto, linux-kernel Hello Michael, > -----Original Message----- > From: Michael Walle <michael@walle.cc> > Sent: Friday, November 12, 2021 7:56 PM > To: ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com> > Cc: horia.geanta@nxp.com; pankaj.gupta@nxp.com; > herbert@gondor.apana.org.au; davem@davemloft.net; > iuliana.prodan@nxp.com; linux-crypto@vger.kernel.org; linux- > kernel@vger.kernel.org > Subject: Re: [PATCH] crypto: caam - check jr permissions before probing > > Hi Andrey, > > Am 2021-11-10 10:33, schrieb ZHIZHIKIN Andrey: > >> -----Original Message----- > >> From: Michael Walle <michael@walle.cc> > >> >> First, thank you for taking the extra step here. Does "reserved > >> >> for HAB" > >> >> mean TF-A is using it or is the BootROM already using it. TF-A is > >> >> optional (so is HAB I guess?). So it might be possible to have jr0 > >> >> in linux, right? If that is correct, the solution to disable the > >> >> jr0 at compile time is wrong. > >> > > >> > From what I've seen in the U-Boot and ATF code, it seems that the > >> > JR0 is reserved by BootROM. When the execution reaches the ATF > >> > (after SPL) those bits are already set which concludes that the > >> > reservation is done quite early. Since current U-Boot does not have > >> > any support for CAAM integrated yet (patchset is under > >> > review) - > >> > it makes me believe that the reservation is done in BootROM. > >> > >> Ok. I guess we have to wait for an answer from NXP. But it strikes as > >> odd that it there is no Secure World, you'll loose one job ring. > > > > From HW perspective, the JR is not lost - it is just assigned to S > > world. > > I said its lost if there is no Secure World (which IMHO is still a valid case). I mean if > its already the BootROM which assign it > (unconditionally) > and there will be no secure world later in the boot process, then its lost. Agree, in this case it can be considered as a total loss of JR for NS World. > > > This also provides an opportunity (at least for i.MX8M series) to > > issue transactions and create Trusted descriptors in S world for NS world. > > This is achieved by 2 sets of ICID/DID pairs, and this is where > > USE_OUT bit is actually used. This however is missing on the LS series > > (SRM does not state this is implemented), which leaves LS with only > > one ICID/DID pair per ring. > > But this would also be possible if the JR is only acquired later by TF-A (or optee), > no? If I read the SRM correct, then the answer is rather no. There is a clear separation that is done between 2 worlds, and they use the JR independently. The i.MX8M series however adds support to issue transactions from S world on behalf of both S and NS worlds by utilizing the second pair of ICID/DID, which LS family does not have. > > > From OS perspective however, I would totally agree - the JR is indeed > > lost, even if there is no software running that required S world. > > > > The only description of process for control transfer from S to NS > > world I was able to find is described in LS1028A SRM section 12.2.2.3, > > which only details ring user re-assignment, but it does not detail > > whether TZ_OWN can participate in this process (set or reset), and > > this is also similar for i.MX8M family. > > > >> > >> > It is correct though: if the JR is not reserved - then it is > >> > accessible in Linux, hence compile-time disabling does not looked > >> > like a good solution to me. > >> > >> Mh, I had a closer look at the IMX8M SRM (I don't have one for the > >> IMX8MM yet). It looks like secure world can reassign the Job Ring to > >> non-secure world though (unless LDID is set). If that is the case I > >> think, deciding at probe time if a job ring is available is not > >> correct; as it can be reassigned later. > > > > That's exactly the culprit here: the LDID is not set on the JR > > reserved. > > > > This makes it possible for the code executing in S to transfer the JR > > to NS. > > Practically, I do not see that this would happen though, as even the > > NXP proposed to disable the node at compile time, which gives me an > > indication that the transfer was never planned. This is however a > > dangerous assumption I have to admit, and in the general case - this > > transfer can occur. > > > > Moreover, from what I read in the SRM of both i.MX8MM and LS1028A - > > there is no lock that is imposed on TZ_OWN bit by setting the LDID (or > > LICID for LS family). > > I've noticed that too, but then assumed that because TZ_PRIM=1 implies > TZ_OWN=1 and the lock bit will lock TZ_PRIM then TZ_OWN must also be 1. > But that's not the case for LS SoCs. Correct, if PRIM_TZ is set and locked - then JR cannot be migrated to NS world until next POR. This is an indirect lock implication I suppose. > > > Would it be possible for you to tell which section of SRM provides a > > description of the JR transfer you mentioned above? > > I don't have access to the IMX8M SEC right now. If memory serves correctly, I just > saw that on an overview. But I just had a look at the LS1028ASECRM, and there is > "SEC's Job Ring interface can be independently assigned (and re-assigned) to > different users." (ch 12.2). I've seen that in both SRMs, and the description is pretty much the same. What is not written in this section is the transitions between S <-> NS worlds, and this is the piece of information of the interest here... :( Perhaps, NXP can step up here to comment on this transitions mechanism. > > > As for probing of the JR node, then I believe it is rather the > > opposite: > > deciding whether the JR is available during probing provides an > > opportunity to bind the device later on when it would be released from > > S to NS world (provided that this would ever occur). However, keeping > > in mind that there is no HW mechanism to indicate that the JR appears > > in NS world after the boot and the user transfer should be done > > manually by some other SW entity - later bind can also be performed > > there. > > Sure, but it will also be the other way around, no? Like S world can "steal" the JR > from NS world. And thats what I'm worried about. This is actually the key point, and I neglected this a bit completely unintentionally. If the software entity running in S World would like to reclaim the JR from Kernel, then it can do so at any given point of time. This for sure should be carefully crafted according to "Ring user (re-)assignment procedure" described in SRM, but what this would mean in practice for the Kernel is: any crypto operation running inside that JR would either complete or be abrupted. Once S World entity would reclaim the JR, the NS Word software entity should unbind the JR and stop using it while submitting operations for CAAM Algos. There is a conceptual problem here with this scenario, namely: S World should notify the NS World that a particular resource (JR in our case) is reserved and should not be used at all. AFAIK this mechanism is not present, and until it is not there - there would be no chance to realize that the JR is gone. Please note, that this patch (and consecutive V2 series) are not addressing above problem and was never intended to. The scenario you're talking about is a part of a bigger task, which is separate from what this patch covers. Just to make it clear: this patch (and consecutive V2 series) tried to address the functionality to dynamically identify which JR is not available for NS World at the Kernel boot and mark them accordingly. This allows that different derivatives that have CAAM HW IP to have any JR reserved, and would not require no changes in DTB to have a node disabled. There are several key components running in S World before Kernel (BootROM, SPL, ATF, OP-TEE) and they all can have JR reserved. If any of those software instances reserve the JR - then currently it should disable it in the DTB as well. This patch allows them not to do so, and moving the identification logic into the Kernel to dynamically figure out which resources are there to use. > > > The only difference to the current proposed solution would be to > > examine the CAAM control register instead of the flag from JR while > > probing, and this is what I'm currently testing on my end. > > > >> > >> So maybe u-boot (or TF-A) should mark that node as disabled after > >> all. > > > > If the U-Boot implementation would come up with similar dynamic > > recognition - then it would not be necessary to disable the node there > > as well. > > > > This would also ensure that if in later derivatives or ATF code > > updates another JR would be reserved as well - there would be no need > > to change and align DTB to it, as it would be dynamically recognized. > > To be clear, I don't talk about dynamic behavior here. Just try to determine > where the JR should be disabled/removed from the DT. And I'm assuming a static > partition of the JRs between S and NS world. As I've written above, I believe it would be hard to rely on static partitioning between S and NS Worlds, as we have several S World agents in the game before Kernel starts. They either should have a clean contract to establish this partitioning, or Kernel should be able to see which of those JRs are not available. This patch addresses the later since we do not have any rules regarding the partitioning contract. > > To recap, NXP suggested to disabled it in the SoC dtsi in u-boot. This depends on > whether the BootROM actually assignes it to S worlds and there is no way for u- > boot to regain access (assuming that u-boot or u-boot SPL will be started in EL3). > If it is not possible to reassign it to NS world, then the JR should be disabled in > linux and u-boot DTs. If there is a chance to regain control and that there might > be no TF-A at all, then statically disable the JR in u-boot is wrong. Instead it should > be determined at runtime (again just static partitioning, no dynamic > reassignment). Just to add: this proposal was done for i.MX8M Mini SoC only, which does not cover all other derivatives implementing CAAM. I assume that if we go with DTB approach - all other derivatives should be revised and reserve nodes appropriate. > > I had a fixup at runtime of the DT (both the active DT in u-boot as well as the DT > passed from u-boot to linux) in mind. Check the TZ_OWN bit and remove/disable > the JR. > > There is also an ongoing discussion where and how the DT will be passed to u- > boot and linux. So I might be the case that TF-A will allocate one JR to itself and > just pass u-boot the DT where that JR is disabled or removed. Which might also fit > the "fixup" in u-boot. Yes, but in this case - all derivatives should have this done, right? I'm not sure how this can be enforced, and also not sure how to keep this up in the future... > > >> If the BootROM is actually already assigning this to secure world > >> (and setting the lock bit LDID). Then it can also be removed from the > >> linux dtsi, because there is no way it can be assigned to linux > >> anyways. > > > > As I've indicated above: the LDID bit is not set on this JR. > > Ok, then u-boot should be able to reset the JR to its defaults if its started in EL3 > (and there is no TF-A at all), right? It can, if the CAAM driver finally lands in U-Boot and this functionality is implemented in that driver. So far, both of those is not covered... What I've just seen in V5 patch series for CAAM support in U-Boot [1], there is a dynamic reservation provisioned in SPL for any arbitrary JR number. Therefore, I believe this patch makes total sense to isolate Kernel and verify which JR is available at boot. > > >> >> >> > diff --git a/drivers/crypto/caam/jr.c > >> >> >> > b/drivers/crypto/caam/jr.c index > >> >> >> > 7f2b1101f567..a260981e0843 100644 > >> >> >> > --- a/drivers/crypto/caam/jr.c > >> >> >> > +++ b/drivers/crypto/caam/jr.c > >> >> >> > @@ -511,10 +511,27 @@ static int caam_jr_probe(struct > >> >> >> > platform_device > >> >> >> > *pdev) > >> >> >> > struct device_node *nprop; > >> >> >> > struct caam_job_ring __iomem *ctrl; > >> >> >> > struct caam_drv_private_jr *jrpriv; > >> >> >> > + struct caam_drv_private *caamctrlpriv; > >> >> >> > static int total_jobrs; > >> >> >> > >> >> >> ugh. > >> >> > > >> >> > Yes, I've seen that. At first, I even wanted to replace it with > >> >> > the > >> >> > ctrlpriv->total_jobrs, > >> >> > but decided not to do it in order to keep this patch focused. > >> >> > >> >> Having the total_jobrs (and using it for anything else than > >> >> messages) static will create an unnecessary dependency on the > >> >> correct probe order. > >> > > >> > Yes, I've stumbled upon this logical problem myself as well. > >> > > >> > I'd decided that this should go, and would replace it with the > >> > option to use IRBAR_JRx as the indexing, since it has the address > >> > aligned and can be used as a bit index. > >> > - For LS1028A it would look like: IRBAR_JR[ring_id] >> 16 > >> > - For i.MX8M series it would be: IRBAR_JR[ring_id] >> 12 > >> > > >> > As those offsets are defined in the HW, they can be reliably used > >> > as indexing parameter to interrogate the CAAM if the JR is reserved > >> > for TZ or not. > >> > > >> > In addition, in order not to access the caam_ctrl register set from > >> > caam_jr driver, I'd still prefer to use a bitmask and compare the > >> > bits set against that new indexing. This would allow the driver to > >> > get rid of that static total_jobrs part at all. > >> > > >> > I would appreciate the community opinion on the approach above > >> > whether it is plausible and does not violate any kernel rules. > >> > >> Will try to follow you here later. > > > > I'm now working on a patchset that would supersede this one, and would > > include the dynamic indexing based on the JR address instead of that > > static variable used. This would also allow to re-order JR nodes > > inside the DTS and do not rely on the order of appearance. > > > >> > >> .. > >> > >> >> >> in general, does these marcros match with your reference manual? > >> >> >> Which one do you have? > >> >> > > >> >> > I'm working on the imx8m mini, which has a v4.0 of CAAM, and > >> >> > this bit is defined in JR[0:2]DID_MS registers. > >> >> > > >> >> > The map looks like following: > >> >> > LDID[31], USE_OUT[30], PRIM_ICID[29:19], LAMTD[17], AMTD[16], > >> >> > TZ_OWN[15], SDID_MS[14:5], PRIM_TZ[4], PRIM_DID[3:0] > >> >> > > >> >> > Perhaps, there is a deviation here between what is instantiated > >> >> > in LS vs i.MX series. > >> >> > > >> >> > At this point, I would also be interested to hear back from NXP > >> >> > on this. > >> >> > >> >> Probably, but that would also mean we'd have to distiguish between > >> >> these too. > >> >> You're checking PRIM_TZ which is SDID on the LS1028A (which is an > >> >> arbitrary number if I understand it correctly). So the check above > >> >> might actually trigger although it shouldn't. > >> > > >> > It's maybe the opposite though: on the LS1028A if the TZ is set, > >> > then NS would read SDID as all 0's. This presents the problem that > >> > PRIM_TZ bit defined for i.MX8M series would read as 0 on LS series > >> > and fail the reservation check. > >> > >> I don't think you have to take the PRIM_TZ bit into account. > >> PRIM_TZ=1 implies OWN_TZ=1. (I'm not sure what PRIM_TZ=0 and > OWN_TZ=1 > >> is good for though). But as mentioned above, I'm not convinced that > >> deciding at probe time is the solution here. > > > > From what I read, PRIM_TZ bit is mixed into the SDID and also "locks" > > JR > > register interface to S world. Setting PRIM_TZ=0 and TZ_OWN=1 has > > primarily an influence of SDID construction, this is outlined in > > JRsDID_MS register description. > > > >> > >> > At this point I'd really like someone from NXP to comment on it, > >> > specifically: is it enough > >> > to just check the TZ bit for all families to recognize that JR is > >> > reserved for usage in Secure world only? > >> > >> yep. > > > > I've compared both i.MX8M and LS family SRMs, and looks like the > > OWN_TZ bit is the only unification point here that can be verified. > > > > I 'm currently testing the implementation where only that bit is > > checked and so far I have good results. I would post a V2 as a series > > and supersede this patch, where only that check would be included. > > > >> > >> >> > >> >> That said, whats PRIM_TZ? When is it set? > >> > > >> > It is set together with TZ_OWN early at the boot and is used for > >> > several purposes, namely: > >> > to derive SDID_MS (it is done dynamically), and also to indicate > >> > that the access to that JR registers (config, interrupt, buffers, > >> > etc.) are only possible from Secure World. > >> > >> Thanks, I also read the SRM for this bit, right now. > >> > >> -michael > > -- > -michael Link: [1]: http://patchwork.ozlabs.org/project/uboot/patch/20211115070014.17586-2-gaurav.jain@nxp.com/ -- andrey ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] crypto: caam - check jr permissions before probing 2021-11-15 9:38 ` ZHIZHIKIN Andrey @ 2021-11-15 11:40 ` Michael Walle 0 siblings, 0 replies; 36+ messages in thread From: Michael Walle @ 2021-11-15 11:40 UTC (permalink / raw) To: ZHIZHIKIN Andrey Cc: horia.geanta, pankaj.gupta, herbert, davem, iuliana.prodan, linux-crypto, linux-kernel >> > As for probing of the JR node, then I believe it is rather the >> > opposite: >> > deciding whether the JR is available during probing provides an >> > opportunity to bind the device later on when it would be released from >> > S to NS world (provided that this would ever occur). However, keeping >> > in mind that there is no HW mechanism to indicate that the JR appears >> > in NS world after the boot and the user transfer should be done >> > manually by some other SW entity - later bind can also be performed >> > there. >> >> Sure, but it will also be the other way around, no? Like S world can >> "steal" the JR >> from NS world. And thats what I'm worried about. > > This is actually the key point, and I neglected this a bit completely > unintentionally. > > If the software entity running in S World would like to reclaim the JR > from Kernel, > then it can do so at any given point of time. This for sure should be > carefully crafted > according to "Ring user (re-)assignment procedure" described in SRM, > but what this > would mean in practice for the Kernel is: any crypto operation running > inside that JR > would either complete or be abrupted. Once S World entity would reclaim > the JR, > the NS Word software entity should unbind the JR and stop using it > while submitting > operations for CAAM Algos. > > There is a conceptual problem here with this scenario, namely: S World > should notify > the NS World that a particular resource (JR in our case) is reserved > and should not be > used at all. AFAIK this mechanism is not present, and until it is not > there - there > would be no chance to realize that the JR is gone. > > Please note, that this patch (and consecutive V2 series) are not > addressing above > problem and was never intended to. The scenario you're talking about > is a part of > a bigger task, which is separate from what this patch covers. I didn't imply that it should, I just questioning wether we should blindly trust the DT (and that would mean someone, i.e. u-boot have to dynamically patch it) or we we shouldn't trust it and the kernel should figure it out on its own (this patch). Do you know if there are any drivers in linux doing the latter? > Just to make it clear: this patch (and consecutive V2 series) tried to > address the > functionality to dynamically identify which JR is not available for NS > World at the > Kernel boot and mark them accordingly. This allows that different > derivatives that > have CAAM HW IP to have any JR reserved, and would not require no > changes in > DTB to have a node disabled. > > There are several key components running in S World before Kernel > (BootROM, SPL, > ATF, OP-TEE) and they all can have JR reserved. If any of those > software instances > reserve the JR - then currently it should disable it in the DTB as > well. This patch allows > them not to do so, and moving the identification logic into the Kernel > to dynamically > figure out which resources are there to use. And this is exactly what I'm worried about (or lets say unsure). Whether we shouldn't trust the DT. >> > The only difference to the current proposed solution would be to >> > examine the CAAM control register instead of the flag from JR while >> > probing, and this is what I'm currently testing on my end. >> > >> >> >> >> So maybe u-boot (or TF-A) should mark that node as disabled after >> >> all. >> > >> > If the U-Boot implementation would come up with similar dynamic >> > recognition - then it would not be necessary to disable the node there >> > as well. >> > >> > This would also ensure that if in later derivatives or ATF code >> > updates another JR would be reserved as well - there would be no need >> > to change and align DTB to it, as it would be dynamically recognized. >> >> To be clear, I don't talk about dynamic behavior here. Just try to >> determine >> where the JR should be disabled/removed from the DT. And I'm assuming >> a static >> partition of the JRs between S and NS world. > > As I've written above, I believe it would be hard to rely on static > partitioning > between S and NS Worlds, as we have several S World agents in the game > before Kernel starts. They either should have a clean contract to > establish this > partitioning, or Kernel should be able to see which of those JRs are > not available. > This patch addresses the later since we do not have any rules regarding > the > partitioning contract. But isn't the contract the device tree? >> To recap, NXP suggested to disabled it in the SoC dtsi in u-boot. This >> depends on >> whether the BootROM actually assignes it to S worlds and there is no >> way for u- >> boot to regain access (assuming that u-boot or u-boot SPL will be >> started in EL3). >> If it is not possible to reassign it to NS world, then the JR should >> be disabled in >> linux and u-boot DTs. If there is a chance to regain control and that >> there might >> be no TF-A at all, then statically disable the JR in u-boot is wrong. >> Instead it should >> be determined at runtime (again just static partitioning, no dynamic >> reassignment). > > Just to add: this proposal was done for i.MX8M Mini SoC only, which > does not cover > all other derivatives implementing CAAM. > > I assume that if we go with DTB approach - all other derivatives > should be revised > and reserve nodes appropriate. Yes, and I disagree how this is implemented in u-boot right now (or is proposed) because its yet again the simple and fast solution. I still think this should be done at runtime and the node should be disabled if it looks like its not available. that is, it should do the same as you are doing here. If there is some software in S world taking over the JR sometime later, then the code in u-boot has to be revised (or the device tree for this particular board has to be adapted). >> I had a fixup at runtime of the DT (both the active DT in u-boot as >> well as the DT >> passed from u-boot to linux) in mind. Check the TZ_OWN bit and >> remove/disable >> the JR. >> >> There is also an ongoing discussion where and how the DT will be >> passed to u- >> boot and linux. So I might be the case that TF-A will allocate one JR >> to itself and >> just pass u-boot the DT where that JR is disabled or removed. Which >> might also fit >> the "fixup" in u-boot. > > Yes, but in this case - all derivatives should have this done, right? with derivatives you mean SoCs implementing CAAM? I thought of something along the following: ofnode_foreach_compatible_node(node, "fsl,sec-v4.0-job-ring") { if (caam_jr_is_unavailable()) ofnode_set_enabled(node, false); } in the common dt_fixup for the SoC. So yes, this should be called for all the SoCs which could have a CAAM. And it should also probably be done for the control DT in u-boot. > I'm not sure how > this can be enforced, and also not sure how to keep this up in the > future... I can't follow you here. >> >> If the BootROM is actually already assigning this to secure world >> >> (and setting the lock bit LDID). Then it can also be removed from the >> >> linux dtsi, because there is no way it can be assigned to linux >> >> anyways. >> > >> > As I've indicated above: the LDID bit is not set on this JR. >> >> Ok, then u-boot should be able to reset the JR to its defaults if its >> started in EL3 >> (and there is no TF-A at all), right? > > It can, if the CAAM driver finally lands in U-Boot and this > functionality is > implemented in that driver. So far, both of those is not covered... > > What I've just seen in V5 patch series for CAAM support in U-Boot [1], > there is a dynamic reservation provisioned in SPL for any arbitrary JR > number. > Therefore, I believe this patch makes total sense to isolate Kernel and > verify > which JR is available at boot. I still have to look at v5. but the one mail I got didn't look very promising, as it says "we go with the "-u-boot.dtsi" approach and just one jobring statically. -michael ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v2 0/2] CAAM Driver: re-factor and dynamic JR availability check 2021-11-04 16:21 [PATCH] crypto: caam - check jr permissions before probing Andrey Zhizhikin 2021-11-05 1:20 ` Michael Walle @ 2021-11-11 16:45 ` Andrey Zhizhikin 2021-11-11 16:46 ` [PATCH v2 1/2] crypto: caam - convert to use capabilities Andrey Zhizhikin ` (2 more replies) 1 sibling, 3 replies; 36+ messages in thread From: Andrey Zhizhikin @ 2021-11-11 16:45 UTC (permalink / raw) To: horia.geanta, pankaj.gupta, herbert, davem, iuliana.prodan, michael, linux-crypto, linux-kernel Cc: Andrey Zhizhikin This patchset is a follow-up work, which was originated with patch [1]. During the review of originally proposed solution, it has been discovered that a certain level of clean-up and re-factoring would be needed for CAAM Driver to remove static variables in JR probing functions, and some bits of CAAM driver code that is used for debug output purposes only. New solution is proposed in this series, and provides following enhancements to the CAAM Driver infrastructure: - CAAM Driver uses new bitmap to indicate capabilities, which has been previously accomplished by various assorted variables in caam_ctrl_priv structure - JR node parsing is made independent from the order of appearance in the DTB, indexing is now based on the address provided by the "reg" property - CAAM Driver checks the presence on JR nodes in DTB, and matches the access to JR HW units in its internal registers. If the JR HW unit is marked to be used by TrustZone - it would be marked appropriate and further probing of JR device would be stopped at the very early stage. Changelog with respect to original patch [1] is recorded in the updated version of the patch, which is included in this series. Link: [1]: https://lore.kernel.org/lkml/20211104162114.2019509-1-andrey.zhizhikin@leica-geosystems.com/ Andrey Zhizhikin (2): crypto: caam - convert to use capabilities crypto: caam - check jr permissions before probing drivers/crypto/caam/caamalg_qi.c | 2 +- drivers/crypto/caam/ctrl.c | 110 ++++++++++++++++++++++--------- drivers/crypto/caam/intern.h | 18 ++--- drivers/crypto/caam/jr.c | 33 ++++++++-- drivers/crypto/caam/regs.h | 9 ++- 5 files changed, 122 insertions(+), 50 deletions(-) base-commit: ad8be4fa6e8149ba6ea21fdf0089e8254437b3c8 -- 2.25.1 ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v2 1/2] crypto: caam - convert to use capabilities 2021-11-11 16:45 ` [PATCH v2 0/2] CAAM Driver: re-factor and dynamic JR availability check Andrey Zhizhikin @ 2021-11-11 16:46 ` Andrey Zhizhikin 2021-11-12 19:22 ` Michael Walle 2021-11-11 16:46 ` [PATCH v2 2/2] crypto: caam - check jr permissions before probing Andrey Zhizhikin 2021-12-07 23:02 ` [PATCH v3 0/2] CAAM Driver: re-factor and set proper JR status Andrey Zhizhikin 2 siblings, 1 reply; 36+ messages in thread From: Andrey Zhizhikin @ 2021-11-11 16:46 UTC (permalink / raw) To: horia.geanta, pankaj.gupta, herbert, davem, iuliana.prodan, michael, linux-crypto, linux-kernel Cc: Andrey Zhizhikin CAAM driver contains several variables, which are used for indication that ertail capabilities are detected during initial probing of the device. They are defined as u8, but mainly used as boolean variables to identify capabillities. Clean-up all assorted variables, collect them into one bitfield value which encodes capabilities as bit, and use them in the execution flow instead. Signed-off-by: Andrey Zhizhikin <andrey.zhizhikin@leica-geosystems.com> --- Changes in V2: No change, this patch is newly introduced drivers/crypto/caam/caamalg_qi.c | 2 +- drivers/crypto/caam/ctrl.c | 49 ++++++++++++++++++-------------- drivers/crypto/caam/intern.h | 16 +++++------ drivers/crypto/caam/regs.h | 2 -- 4 files changed, 36 insertions(+), 33 deletions(-) diff --git a/drivers/crypto/caam/caamalg_qi.c b/drivers/crypto/caam/caamalg_qi.c index 189a7438b29c..372a319e8434 100644 --- a/drivers/crypto/caam/caamalg_qi.c +++ b/drivers/crypto/caam/caamalg_qi.c @@ -2610,7 +2610,7 @@ int caam_qi_algapi_init(struct device *ctrldev) bool registered = false; /* Make sure this runs only on (DPAA 1.x) QI */ - if (!priv->qi_present || caam_dpaa2) + if (!(priv->caam_caps | CAAM_CAPS_QI_PRESENT) || caam_dpaa2) return 0; /* diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c index ca0361b2dbb0..7a14a69d89c7 100644 --- a/drivers/crypto/caam/ctrl.c +++ b/drivers/crypto/caam/ctrl.c @@ -100,7 +100,7 @@ static inline int run_descriptor_deco0(struct device *ctrldev, u32 *desc, int i; - if (ctrlpriv->virt_en == 1 || + if ((ctrlpriv->caam_caps & CAAM_CAPS_VIRT_ENABLED) || /* * Apparently on i.MX8M{Q,M,N,P} it doesn't matter if virt_en == 1 * and the following steps should be performed regardless @@ -169,7 +169,7 @@ static inline int run_descriptor_deco0(struct device *ctrldev, u32 *desc, *status = rd_reg32(&deco->op_status_hi) & DECO_OP_STATUS_HI_ERR_MASK; - if (ctrlpriv->virt_en == 1) + if (ctrlpriv->caam_caps & CAAM_CAPS_VIRT_ENABLED) clrsetbits_32(&ctrl->deco_rsr, DECORSR_JR0, 0); /* Mark the DECO as free */ @@ -622,7 +622,6 @@ static int caam_probe(struct platform_device *pdev) struct dentry *dfs_root; u32 scfgr, comp_params; u8 rng_vid; - int pg_size; int BLOCK_OFFSET = 0; bool pr_support = false; @@ -666,11 +665,12 @@ static int caam_probe(struct platform_device *pdev) else caam_ptr_sz = sizeof(u32); caam_dpaa2 = !!(comp_params & CTPR_MS_DPAA2); - ctrlpriv->qi_present = !!(comp_params & CTPR_MS_QI_MASK); + ctrlpriv->caam_caps |= (!!(comp_params & CTPR_MS_QI_MASK)) ? + CAAM_CAPS_QI_PRESENT : 0; #ifdef CONFIG_CAAM_QI /* If (DPAA 1.x) QI present, check whether dependencies are available */ - if (ctrlpriv->qi_present && !caam_dpaa2) { + if ((ctrlpriv->caam_caps & CAAM_CAPS_QI_PRESENT) && !caam_dpaa2) { ret = qman_is_probed(); if (!ret) { return -EPROBE_DEFER; @@ -692,11 +692,14 @@ static int caam_probe(struct platform_device *pdev) /* Allocating the BLOCK_OFFSET based on the supported page size on * the platform */ - pg_size = (comp_params & CTPR_MS_PG_SZ_MASK) >> CTPR_MS_PG_SZ_SHIFT; - if (pg_size == 0) - BLOCK_OFFSET = PG_SIZE_4K; + ctrlpriv->caam_caps |= + (!!((comp_params & CTPR_MS_PG_SZ_MASK) >> CTPR_MS_PG_SZ_SHIFT)) ? + CAAM_CAPS_64K_PAGESIZE : 0; + + if (ctrlpriv->caam_caps & CAAM_CAPS_64K_PAGESIZE) + BLOCK_OFFSET = SZ_64K; else - BLOCK_OFFSET = PG_SIZE_64K; + BLOCK_OFFSET = SZ_4K; ctrlpriv->ctrl = (struct caam_ctrl __iomem __force *)ctrl; ctrlpriv->assure = (struct caam_assurance __iomem __force *) @@ -711,11 +714,11 @@ static int caam_probe(struct platform_device *pdev) /* Get the IRQ of the controller (for security violations only) */ ctrlpriv->secvio_irq = irq_of_parse_and_map(nprop, 0); np = of_find_compatible_node(NULL, NULL, "fsl,qoriq-mc"); - ctrlpriv->mc_en = !!np; + ctrlpriv->caam_caps |= (!!np) ? CAAM_CAPS_MC_ENABLED : 0; of_node_put(np); #ifdef CONFIG_FSL_MC_BUS - if (ctrlpriv->mc_en) { + if (ctrlpriv->caam_caps & CAAM_CAPS_MC_ENABLED) { struct fsl_mc_version *mc_version; mc_version = fsl_mc_get_version(); @@ -732,7 +735,7 @@ static int caam_probe(struct platform_device *pdev) * In case of SoCs with Management Complex, MC f/w performs * the configuration. */ - if (!ctrlpriv->mc_en) + if (!(ctrlpriv->caam_caps & CAAM_CAPS_MC_ENABLED)) clrsetbits_32(&ctrl->mcr, MCFGR_AWCACHE_MASK, MCFGR_AWCACHE_CACH | MCFGR_AWCACHE_BUFF | MCFGR_WDENABLE | MCFGR_LARGE_BURST); @@ -745,7 +748,6 @@ static int caam_probe(struct platform_device *pdev) */ scfgr = rd_reg32(&ctrl->scfgr); - ctrlpriv->virt_en = 0; if (comp_params & CTPR_MS_VIRT_EN_INCL) { /* VIRT_EN_INCL = 1 & VIRT_EN_POR = 1 or * VIRT_EN_INCL = 1 & VIRT_EN_POR = 0 & SCFGR_VIRT_EN = 1 @@ -753,14 +755,14 @@ static int caam_probe(struct platform_device *pdev) if ((comp_params & CTPR_MS_VIRT_EN_POR) || (!(comp_params & CTPR_MS_VIRT_EN_POR) && (scfgr & SCFGR_VIRT_EN))) - ctrlpriv->virt_en = 1; + ctrlpriv->caam_caps |= CAAM_CAPS_VIRT_ENABLED; } else { /* VIRT_EN_INCL = 0 && VIRT_EN_POR_VALUE = 1 */ if (comp_params & CTPR_MS_VIRT_EN_POR) - ctrlpriv->virt_en = 1; + ctrlpriv->caam_caps |= CAAM_CAPS_VIRT_ENABLED; } - if (ctrlpriv->virt_en == 1) + if (ctrlpriv->caam_caps & CAAM_CAPS_VIRT_ENABLED) clrsetbits_32(&ctrl->jrstart, 0, JRSTART_JR0_START | JRSTART_JR1_START | JRSTART_JR2_START | JRSTART_JR3_START); @@ -785,7 +787,7 @@ static int caam_probe(struct platform_device *pdev) caam_debugfs_init(ctrlpriv, dfs_root); /* Check to see if (DPAA 1.x) QI present. If so, enable */ - if (ctrlpriv->qi_present && !caam_dpaa2) { + if ((ctrlpriv->caam_caps & CAAM_CAPS_QI_PRESENT) && !caam_dpaa2) { ctrlpriv->qi = (struct caam_queue_if __iomem __force *) ((__force uint8_t *)ctrl + BLOCK_OFFSET * QI_BLOCK_NUMBER @@ -810,12 +812,13 @@ static int caam_probe(struct platform_device *pdev) (ring + JR_BLOCK_NUMBER) * BLOCK_OFFSET ); - ctrlpriv->total_jobrs++; ring++; + ctrlpriv->caam_caps |= BIT(ring); } /* If no QI and no rings specified, quit and go home */ - if ((!ctrlpriv->qi_present) && (!ctrlpriv->total_jobrs)) { + if (!(ctrlpriv->caam_caps & CAAM_CAPS_QI_PRESENT) && + (hweight_long(ctrlpriv->caam_caps & CAAM_CAPS_JOBRS_MASK) == 0)) { dev_err(dev, "no queues configured, terminating\n"); return -ENOMEM; } @@ -832,7 +835,8 @@ static int caam_probe(struct platform_device *pdev) * already instantiated, do RNG instantiation * In case of SoCs with Management Complex, RNG is managed by MC f/w. */ - if (!(ctrlpriv->mc_en && pr_support) && rng_vid >= 4) { + if (!((ctrlpriv->caam_caps & CAAM_CAPS_MC_ENABLED) && pr_support) && + rng_vid >= 4) { ctrlpriv->rng4_sh_init = rd_reg32(&ctrl->r4tst[0].rdsta); /* @@ -900,8 +904,9 @@ static int caam_probe(struct platform_device *pdev) /* Report "alive" for developer to see */ dev_info(dev, "device ID = 0x%016llx (Era %d)\n", caam_id, ctrlpriv->era); - dev_info(dev, "job rings = %d, qi = %d\n", - ctrlpriv->total_jobrs, ctrlpriv->qi_present); + dev_info(dev, "job rings = %ld, qi = %s\n", + hweight_long(ctrlpriv->caam_caps & CAAM_CAPS_JOBRS_MASK), + (ctrlpriv->caam_caps & CAAM_CAPS_QI_PRESENT) ? "yes" : "no"); ret = devm_of_platform_populate(dev); if (ret) diff --git a/drivers/crypto/caam/intern.h b/drivers/crypto/caam/intern.h index 7d45b21bd55a..37f0b93c7087 100644 --- a/drivers/crypto/caam/intern.h +++ b/drivers/crypto/caam/intern.h @@ -86,15 +86,15 @@ struct caam_drv_private { struct iommu_domain *domain; - /* - * Detected geometry block. Filled in from device tree if powerpc, - * or from register-based version detection code - */ - u8 total_jobrs; /* Total Job Rings in device */ - u8 qi_present; /* Nonzero if QI present in device */ - u8 mc_en; /* Nonzero if MC f/w is active */ + unsigned long caam_caps; /* CAAM Module capabilities */ + +#define CAAM_CAPS_QI_PRESENT BIT(0) /* Queue Manager interface (QI) implemented */ +#define CAAM_CAPS_JOBRS_MASK GENMASK(15, 1) /* Job Ring is available in NS World */ +#define CAAM_CAPS_MC_ENABLED BIT(16) /* Management Complex is enabled (F/W is active) */ +#define CAAM_CAPS_VIRT_ENABLED BIT(17) /* Virtualization enabled */ +#define CAAM_CAPS_64K_PAGESIZE BIT(18) /* CAAM register page size (64KB if set, 4KB if unset) */ + int secvio_irq; /* Security violation interrupt number */ - int virt_en; /* Virtualization enabled in CAAM */ int era; /* CAAM Era (internal HW revision) */ #define RNG4_MAX_HANDLES 2 diff --git a/drivers/crypto/caam/regs.h b/drivers/crypto/caam/regs.h index 3738625c0250..186e76e6a3e7 100644 --- a/drivers/crypto/caam/regs.h +++ b/drivers/crypto/caam/regs.h @@ -1023,6 +1023,4 @@ struct caam_deco { #define ASSURE_BLOCK_NUMBER 6 #define QI_BLOCK_NUMBER 7 #define DECO_BLOCK_NUMBER 8 -#define PG_SIZE_4K 0x1000 -#define PG_SIZE_64K 0x10000 #endif /* REGS_H */ -- 2.25.1 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/2] crypto: caam - convert to use capabilities 2021-11-11 16:46 ` [PATCH v2 1/2] crypto: caam - convert to use capabilities Andrey Zhizhikin @ 2021-11-12 19:22 ` Michael Walle 2021-11-15 9:45 ` ZHIZHIKIN Andrey 0 siblings, 1 reply; 36+ messages in thread From: Michael Walle @ 2021-11-12 19:22 UTC (permalink / raw) To: Andrey Zhizhikin Cc: horia.geanta, pankaj.gupta, herbert, davem, iuliana.prodan, linux-crypto, linux-kernel Am 2021-11-11 17:46, schrieb Andrey Zhizhikin: > CAAM driver contains several variables, which are used for indication > that ertail capabilities are detected during initial probing of the > device. They are defined as u8, but mainly used as boolean variables to > identify capabillities. > > Clean-up all assorted variables, collect them into one bitfield value > which encodes capabilities as bit, and use them in the execution flow > instead. > > Signed-off-by: Andrey Zhizhikin <andrey.zhizhikin@leica-geosystems.com> > --- > Changes in V2: No change, this patch is newly introduced > > drivers/crypto/caam/caamalg_qi.c | 2 +- > drivers/crypto/caam/ctrl.c | 49 ++++++++++++++++++-------------- > drivers/crypto/caam/intern.h | 16 +++++------ > drivers/crypto/caam/regs.h | 2 -- > 4 files changed, 36 insertions(+), 33 deletions(-) > > diff --git a/drivers/crypto/caam/caamalg_qi.c > b/drivers/crypto/caam/caamalg_qi.c > index 189a7438b29c..372a319e8434 100644 > --- a/drivers/crypto/caam/caamalg_qi.c > +++ b/drivers/crypto/caam/caamalg_qi.c > @@ -2610,7 +2610,7 @@ int caam_qi_algapi_init(struct device *ctrldev) > bool registered = false; > > /* Make sure this runs only on (DPAA 1.x) QI */ > - if (!priv->qi_present || caam_dpaa2) > + if (!(priv->caam_caps | CAAM_CAPS_QI_PRESENT) || caam_dpaa2) Typo? if (!(priv->caam_caps & CAAM_CAPS_QI_PRESENT) || caam_dpaa2) > return 0; > > /* > diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c > index ca0361b2dbb0..7a14a69d89c7 100644 > --- a/drivers/crypto/caam/ctrl.c > +++ b/drivers/crypto/caam/ctrl.c > @@ -100,7 +100,7 @@ static inline int run_descriptor_deco0(struct > device *ctrldev, u32 *desc, > int i; > > > - if (ctrlpriv->virt_en == 1 || > + if ((ctrlpriv->caam_caps & CAAM_CAPS_VIRT_ENABLED) || > /* > * Apparently on i.MX8M{Q,M,N,P} it doesn't matter if virt_en == > 1 > * and the following steps should be performed regardless > @@ -169,7 +169,7 @@ static inline int run_descriptor_deco0(struct > device *ctrldev, u32 *desc, > *status = rd_reg32(&deco->op_status_hi) & > DECO_OP_STATUS_HI_ERR_MASK; > > - if (ctrlpriv->virt_en == 1) > + if (ctrlpriv->caam_caps & CAAM_CAPS_VIRT_ENABLED) > clrsetbits_32(&ctrl->deco_rsr, DECORSR_JR0, 0); > > /* Mark the DECO as free */ > @@ -622,7 +622,6 @@ static int caam_probe(struct platform_device *pdev) > struct dentry *dfs_root; > u32 scfgr, comp_params; > u8 rng_vid; > - int pg_size; > int BLOCK_OFFSET = 0; > bool pr_support = false; > > @@ -666,11 +665,12 @@ static int caam_probe(struct platform_device > *pdev) > else > caam_ptr_sz = sizeof(u32); > caam_dpaa2 = !!(comp_params & CTPR_MS_DPAA2); > - ctrlpriv->qi_present = !!(comp_params & CTPR_MS_QI_MASK); > + ctrlpriv->caam_caps |= (!!(comp_params & CTPR_MS_QI_MASK)) ? > + CAAM_CAPS_QI_PRESENT : 0; "!!" is superfluous here. and the braces, too. But IMHO this is easier on the eye: if (comp_params & CTPR_MS_QI_MASK) ctrl->priv->caam_cap |= CAAM_CAPS_QI_PRESENT; > #ifdef CONFIG_CAAM_QI > /* If (DPAA 1.x) QI present, check whether dependencies are available > */ > - if (ctrlpriv->qi_present && !caam_dpaa2) { > + if ((ctrlpriv->caam_caps & CAAM_CAPS_QI_PRESENT) && !caam_dpaa2) { > ret = qman_is_probed(); > if (!ret) { > return -EPROBE_DEFER; > @@ -692,11 +692,14 @@ static int caam_probe(struct platform_device > *pdev) > /* Allocating the BLOCK_OFFSET based on the supported page size on > * the platform > */ > - pg_size = (comp_params & CTPR_MS_PG_SZ_MASK) >> CTPR_MS_PG_SZ_SHIFT; > - if (pg_size == 0) > - BLOCK_OFFSET = PG_SIZE_4K; > + ctrlpriv->caam_caps |= > + (!!((comp_params & CTPR_MS_PG_SZ_MASK) >> CTPR_MS_PG_SZ_SHIFT)) ? > + CAAM_CAPS_64K_PAGESIZE : 0; same here as above. if (comp_params & CTPR_MS_PG_SZ_MASK) ctrl->priv->caam_cap |= CAAM_CAPS_64K_PAGESIZE; Assuming that SZ_MASK and SZ_SHIFT fits together ;) > + > + if (ctrlpriv->caam_caps & CAAM_CAPS_64K_PAGESIZE) > + BLOCK_OFFSET = SZ_64K; > else > - BLOCK_OFFSET = PG_SIZE_64K; > + BLOCK_OFFSET = SZ_4K; btw.. that all uppercase BLOCK_OFFSET looks super odd. Can we get rid of that too? I haven't checked but pg_size didn't make any sense before, did it? At least if SZ_MASK has more than one bit. > ctrlpriv->ctrl = (struct caam_ctrl __iomem __force *)ctrl; > ctrlpriv->assure = (struct caam_assurance __iomem __force *) > @@ -711,11 +714,11 @@ static int caam_probe(struct platform_device > *pdev) > /* Get the IRQ of the controller (for security violations only) */ > ctrlpriv->secvio_irq = irq_of_parse_and_map(nprop, 0); > np = of_find_compatible_node(NULL, NULL, "fsl,qoriq-mc"); > - ctrlpriv->mc_en = !!np; > + ctrlpriv->caam_caps |= (!!np) ? CAAM_CAPS_MC_ENABLED : 0; if (np) ctrlpriv->caam_caps |= CAAM_CAPS_MC_ENABLED; > of_node_put(np); > > #ifdef CONFIG_FSL_MC_BUS > - if (ctrlpriv->mc_en) { > + if (ctrlpriv->caam_caps & CAAM_CAPS_MC_ENABLED) { > struct fsl_mc_version *mc_version; > > mc_version = fsl_mc_get_version(); > @@ -732,7 +735,7 @@ static int caam_probe(struct platform_device *pdev) > * In case of SoCs with Management Complex, MC f/w performs > * the configuration. > */ > - if (!ctrlpriv->mc_en) > + if (!(ctrlpriv->caam_caps & CAAM_CAPS_MC_ENABLED)) > clrsetbits_32(&ctrl->mcr, MCFGR_AWCACHE_MASK, > MCFGR_AWCACHE_CACH | MCFGR_AWCACHE_BUFF | > MCFGR_WDENABLE | MCFGR_LARGE_BURST); > @@ -745,7 +748,6 @@ static int caam_probe(struct platform_device *pdev) > */ > scfgr = rd_reg32(&ctrl->scfgr); > > - ctrlpriv->virt_en = 0; > if (comp_params & CTPR_MS_VIRT_EN_INCL) { > /* VIRT_EN_INCL = 1 & VIRT_EN_POR = 1 or > * VIRT_EN_INCL = 1 & VIRT_EN_POR = 0 & SCFGR_VIRT_EN = 1 > @@ -753,14 +755,14 @@ static int caam_probe(struct platform_device > *pdev) > if ((comp_params & CTPR_MS_VIRT_EN_POR) || > (!(comp_params & CTPR_MS_VIRT_EN_POR) && > (scfgr & SCFGR_VIRT_EN))) > - ctrlpriv->virt_en = 1; > + ctrlpriv->caam_caps |= CAAM_CAPS_VIRT_ENABLED; at first sight it looked like a wrong indendation, but it the old code was wrong. > } else { > /* VIRT_EN_INCL = 0 && VIRT_EN_POR_VALUE = 1 */ > if (comp_params & CTPR_MS_VIRT_EN_POR) > - ctrlpriv->virt_en = 1; > + ctrlpriv->caam_caps |= CAAM_CAPS_VIRT_ENABLED; > } > > - if (ctrlpriv->virt_en == 1) > + if (ctrlpriv->caam_caps & CAAM_CAPS_VIRT_ENABLED) > clrsetbits_32(&ctrl->jrstart, 0, JRSTART_JR0_START | > JRSTART_JR1_START | JRSTART_JR2_START | > JRSTART_JR3_START); > @@ -785,7 +787,7 @@ static int caam_probe(struct platform_device *pdev) > caam_debugfs_init(ctrlpriv, dfs_root); > > /* Check to see if (DPAA 1.x) QI present. If so, enable */ > - if (ctrlpriv->qi_present && !caam_dpaa2) { > + if ((ctrlpriv->caam_caps & CAAM_CAPS_QI_PRESENT) && !caam_dpaa2) { > ctrlpriv->qi = (struct caam_queue_if __iomem __force *) > ((__force uint8_t *)ctrl + > BLOCK_OFFSET * QI_BLOCK_NUMBER > @@ -810,12 +812,13 @@ static int caam_probe(struct platform_device > *pdev) > (ring + JR_BLOCK_NUMBER) * > BLOCK_OFFSET > ); > - ctrlpriv->total_jobrs++; > ring++; > + ctrlpriv->caam_caps |= BIT(ring); I think this deserves an own macro. At the moment you assume that the lower bits are for the rings, right? I'd like to see something like ctrlpriv->caam_caps |= JR_PRESENT(ring); then have #define JR_PRESENT_MASK GENMASK(7, 0) #define JR_PRESENT(x) (BIT(x) & JR_PRESENT_MASK) together with all the other bits in caam_caps. Or something along that. I guess you got the idea. > } > > /* If no QI and no rings specified, quit and go home */ > - if ((!ctrlpriv->qi_present) && (!ctrlpriv->total_jobrs)) { > + if (!(ctrlpriv->caam_caps & CAAM_CAPS_QI_PRESENT) && > + (hweight_long(ctrlpriv->caam_caps & CAAM_CAPS_JOBRS_MASK) == 0)) > { > dev_err(dev, "no queues configured, terminating\n"); > return -ENOMEM; > } > @@ -832,7 +835,8 @@ static int caam_probe(struct platform_device *pdev) > * already instantiated, do RNG instantiation > * In case of SoCs with Management Complex, RNG is managed by MC f/w. > */ > - if (!(ctrlpriv->mc_en && pr_support) && rng_vid >= 4) { > + if (!((ctrlpriv->caam_caps & CAAM_CAPS_MC_ENABLED) && pr_support) && > + rng_vid >= 4) { > ctrlpriv->rng4_sh_init = > rd_reg32(&ctrl->r4tst[0].rdsta); > /* > @@ -900,8 +904,9 @@ static int caam_probe(struct platform_device *pdev) > /* Report "alive" for developer to see */ > dev_info(dev, "device ID = 0x%016llx (Era %d)\n", caam_id, > ctrlpriv->era); > - dev_info(dev, "job rings = %d, qi = %d\n", > - ctrlpriv->total_jobrs, ctrlpriv->qi_present); > + dev_info(dev, "job rings = %ld, qi = %s\n", > + hweight_long(ctrlpriv->caam_caps & CAAM_CAPS_JOBRS_MASK), > + (ctrlpriv->caam_caps & CAAM_CAPS_QI_PRESENT) ? "yes" : "no"); > > ret = devm_of_platform_populate(dev); > if (ret) > diff --git a/drivers/crypto/caam/intern.h > b/drivers/crypto/caam/intern.h > index 7d45b21bd55a..37f0b93c7087 100644 > --- a/drivers/crypto/caam/intern.h > +++ b/drivers/crypto/caam/intern.h > @@ -86,15 +86,15 @@ struct caam_drv_private { > > struct iommu_domain *domain; > > - /* > - * Detected geometry block. Filled in from device tree if powerpc, > - * or from register-based version detection code > - */ > - u8 total_jobrs; /* Total Job Rings in device */ > - u8 qi_present; /* Nonzero if QI present in device */ > - u8 mc_en; /* Nonzero if MC f/w is active */ > + unsigned long caam_caps; /* CAAM Module capabilities */ > + > +#define CAAM_CAPS_QI_PRESENT BIT(0) /* Queue Manager interface (QI) > implemented */ > +#define CAAM_CAPS_JOBRS_MASK GENMASK(15, 1) /* Job Ring is available > in NS World */ ok I see you already have something like that. See above. That BIT() in the code above should go away. > +#define CAAM_CAPS_MC_ENABLED BIT(16) /* Management Complex is enabled > (F/W is active) */ > +#define CAAM_CAPS_VIRT_ENABLED BIT(17) /* Virtualization enabled */ > +#define CAAM_CAPS_64K_PAGESIZE BIT(18) /* CAAM register page size > (64KB if set, 4KB if unset) */ > + > int secvio_irq; /* Security violation interrupt number */ > - int virt_en; /* Virtualization enabled in CAAM */ > int era; /* CAAM Era (internal HW revision) */ > > #define RNG4_MAX_HANDLES 2 > diff --git a/drivers/crypto/caam/regs.h b/drivers/crypto/caam/regs.h > index 3738625c0250..186e76e6a3e7 100644 > --- a/drivers/crypto/caam/regs.h > +++ b/drivers/crypto/caam/regs.h > @@ -1023,6 +1023,4 @@ struct caam_deco { > #define ASSURE_BLOCK_NUMBER 6 > #define QI_BLOCK_NUMBER 7 > #define DECO_BLOCK_NUMBER 8 > -#define PG_SIZE_4K 0x1000 > -#define PG_SIZE_64K 0x10000 nice ;) > #endif /* REGS_H */ Otherwise, I really like this cleanup. Thanks, -michael ^ permalink raw reply [flat|nested] 36+ messages in thread
* RE: [PATCH v2 1/2] crypto: caam - convert to use capabilities 2021-11-12 19:22 ` Michael Walle @ 2021-11-15 9:45 ` ZHIZHIKIN Andrey 0 siblings, 0 replies; 36+ messages in thread From: ZHIZHIKIN Andrey @ 2021-11-15 9:45 UTC (permalink / raw) To: Michael Walle Cc: horia.geanta, pankaj.gupta, herbert, davem, iuliana.prodan, linux-crypto, linux-kernel Hello Michael, First of, thanks a lot for thorough review! > -----Original Message----- > From: Michael Walle <michael@walle.cc> > Sent: Friday, November 12, 2021 8:23 PM > To: ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com> > Cc: horia.geanta@nxp.com; pankaj.gupta@nxp.com; > herbert@gondor.apana.org.au; davem@davemloft.net; > iuliana.prodan@nxp.com; linux-crypto@vger.kernel.org; linux- > kernel@vger.kernel.org > Subject: Re: [PATCH v2 1/2] crypto: caam - convert to use capabilities > > > Am 2021-11-11 17:46, schrieb Andrey Zhizhikin: > > CAAM driver contains several variables, which are used for indication > > that ertail capabilities are detected during initial probing of the > > device. They are defined as u8, but mainly used as boolean variables to > > identify capabillities. > > > > Clean-up all assorted variables, collect them into one bitfield value > > which encodes capabilities as bit, and use them in the execution flow > > instead. > > > > Signed-off-by: Andrey Zhizhikin <andrey.zhizhikin@leica-geosystems.com> > > --- > > Changes in V2: No change, this patch is newly introduced > > > > drivers/crypto/caam/caamalg_qi.c | 2 +- > > drivers/crypto/caam/ctrl.c | 49 ++++++++++++++++++-------------- > > drivers/crypto/caam/intern.h | 16 +++++------ > > drivers/crypto/caam/regs.h | 2 -- > > 4 files changed, 36 insertions(+), 33 deletions(-) > > > > diff --git a/drivers/crypto/caam/caamalg_qi.c > > b/drivers/crypto/caam/caamalg_qi.c > > index 189a7438b29c..372a319e8434 100644 > > --- a/drivers/crypto/caam/caamalg_qi.c > > +++ b/drivers/crypto/caam/caamalg_qi.c > > @@ -2610,7 +2610,7 @@ int caam_qi_algapi_init(struct device *ctrldev) > > bool registered = false; > > > > /* Make sure this runs only on (DPAA 1.x) QI */ > > - if (!priv->qi_present || caam_dpaa2) > > + if (!(priv->caam_caps | CAAM_CAPS_QI_PRESENT) || caam_dpaa2) > > Typo? > if (!(priv->caam_caps & CAAM_CAPS_QI_PRESENT) || caam_dpaa2) True, good catch! Would address in V3. > > > > return 0; > > > > /* > > diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c > > index ca0361b2dbb0..7a14a69d89c7 100644 > > --- a/drivers/crypto/caam/ctrl.c > > +++ b/drivers/crypto/caam/ctrl.c > > @@ -100,7 +100,7 @@ static inline int run_descriptor_deco0(struct > > device *ctrldev, u32 *desc, > > int i; > > > > > > - if (ctrlpriv->virt_en == 1 || > > + if ((ctrlpriv->caam_caps & CAAM_CAPS_VIRT_ENABLED) || > > /* > > * Apparently on i.MX8M{Q,M,N,P} it doesn't matter if virt_en == > > 1 > > * and the following steps should be performed regardless > > @@ -169,7 +169,7 @@ static inline int run_descriptor_deco0(struct > > device *ctrldev, u32 *desc, > > *status = rd_reg32(&deco->op_status_hi) & > > DECO_OP_STATUS_HI_ERR_MASK; > > > > - if (ctrlpriv->virt_en == 1) > > + if (ctrlpriv->caam_caps & CAAM_CAPS_VIRT_ENABLED) > > clrsetbits_32(&ctrl->deco_rsr, DECORSR_JR0, 0); > > > > /* Mark the DECO as free */ > > @@ -622,7 +622,6 @@ static int caam_probe(struct platform_device *pdev) > > struct dentry *dfs_root; > > u32 scfgr, comp_params; > > u8 rng_vid; > > - int pg_size; > > int BLOCK_OFFSET = 0; > > bool pr_support = false; > > > > @@ -666,11 +665,12 @@ static int caam_probe(struct platform_device > > *pdev) > > else > > caam_ptr_sz = sizeof(u32); > > caam_dpaa2 = !!(comp_params & CTPR_MS_DPAA2); > > - ctrlpriv->qi_present = !!(comp_params & CTPR_MS_QI_MASK); > > + ctrlpriv->caam_caps |= (!!(comp_params & CTPR_MS_QI_MASK)) ? > > + CAAM_CAPS_QI_PRESENT : 0; > > "!!" is superfluous here. and the braces, too. But IMHO this is > easier on the eye: > if (comp_params & CTPR_MS_QI_MASK) > ctrl->priv->caam_cap |= CAAM_CAPS_QI_PRESENT; Would replace and push in V3. > > > > #ifdef CONFIG_CAAM_QI > > /* If (DPAA 1.x) QI present, check whether dependencies are available > > */ > > - if (ctrlpriv->qi_present && !caam_dpaa2) { > > + if ((ctrlpriv->caam_caps & CAAM_CAPS_QI_PRESENT) && !caam_dpaa2) { > > ret = qman_is_probed(); > > if (!ret) { > > return -EPROBE_DEFER; > > @@ -692,11 +692,14 @@ static int caam_probe(struct platform_device > > *pdev) > > /* Allocating the BLOCK_OFFSET based on the supported page size on > > * the platform > > */ > > - pg_size = (comp_params & CTPR_MS_PG_SZ_MASK) >> > CTPR_MS_PG_SZ_SHIFT; > > - if (pg_size == 0) > > - BLOCK_OFFSET = PG_SIZE_4K; > > + ctrlpriv->caam_caps |= > > + (!!((comp_params & CTPR_MS_PG_SZ_MASK) >> > CTPR_MS_PG_SZ_SHIFT)) ? > > + CAAM_CAPS_64K_PAGESIZE : 0; > > same here as above. > > if (comp_params & CTPR_MS_PG_SZ_MASK) > ctrl->priv->caam_cap |= CAAM_CAPS_64K_PAGESIZE; > > Assuming that SZ_MASK and SZ_SHIFT fits together ;) Correct, comes in V3. > > > + > > + if (ctrlpriv->caam_caps & CAAM_CAPS_64K_PAGESIZE) > > + BLOCK_OFFSET = SZ_64K; > > else > > - BLOCK_OFFSET = PG_SIZE_64K; > > + BLOCK_OFFSET = SZ_4K; > > btw.. that all uppercase BLOCK_OFFSET looks super odd. Can we get > rid of that too? I haven't checked but pg_size didn't make any > sense before, did it? At least if SZ_MASK has more than one bit. This unfortunately be needed, since IM8M and LS family have different page sizes and therefore - different base address offsets. I would rename it to match the meaning of this variable. > > > ctrlpriv->ctrl = (struct caam_ctrl __iomem __force *)ctrl; > > ctrlpriv->assure = (struct caam_assurance __iomem __force *) > > @@ -711,11 +714,11 @@ static int caam_probe(struct platform_device > > *pdev) > > /* Get the IRQ of the controller (for security violations only) */ > > ctrlpriv->secvio_irq = irq_of_parse_and_map(nprop, 0); > > np = of_find_compatible_node(NULL, NULL, "fsl,qoriq-mc"); > > - ctrlpriv->mc_en = !!np; > > + ctrlpriv->caam_caps |= (!!np) ? CAAM_CAPS_MC_ENABLED : 0; > > if (np) > ctrlpriv->caam_caps |= CAAM_CAPS_MC_ENABLED; Noted, comes in V3. > > > of_node_put(np); > > > > #ifdef CONFIG_FSL_MC_BUS > > - if (ctrlpriv->mc_en) { > > + if (ctrlpriv->caam_caps & CAAM_CAPS_MC_ENABLED) { > > struct fsl_mc_version *mc_version; > > > > mc_version = fsl_mc_get_version(); > > @@ -732,7 +735,7 @@ static int caam_probe(struct platform_device *pdev) > > * In case of SoCs with Management Complex, MC f/w performs > > * the configuration. > > */ > > - if (!ctrlpriv->mc_en) > > + if (!(ctrlpriv->caam_caps & CAAM_CAPS_MC_ENABLED)) > > clrsetbits_32(&ctrl->mcr, MCFGR_AWCACHE_MASK, > > MCFGR_AWCACHE_CACH | MCFGR_AWCACHE_BUFF | > > MCFGR_WDENABLE | MCFGR_LARGE_BURST); > > @@ -745,7 +748,6 @@ static int caam_probe(struct platform_device *pdev) > > */ > > scfgr = rd_reg32(&ctrl->scfgr); > > > > - ctrlpriv->virt_en = 0; > > if (comp_params & CTPR_MS_VIRT_EN_INCL) { > > /* VIRT_EN_INCL = 1 & VIRT_EN_POR = 1 or > > * VIRT_EN_INCL = 1 & VIRT_EN_POR = 0 & SCFGR_VIRT_EN = 1 > > @@ -753,14 +755,14 @@ static int caam_probe(struct platform_device > > *pdev) > > if ((comp_params & CTPR_MS_VIRT_EN_POR) || > > (!(comp_params & CTPR_MS_VIRT_EN_POR) && > > (scfgr & SCFGR_VIRT_EN))) > > - ctrlpriv->virt_en = 1; > > + ctrlpriv->caam_caps |= CAAM_CAPS_VIRT_ENABLED; > > at first sight it looked like a wrong indendation, but it the old > code was wrong. checkpatch.pl --strict forced my hand to align it. ;) > > > } else { > > /* VIRT_EN_INCL = 0 && VIRT_EN_POR_VALUE = 1 */ > > if (comp_params & CTPR_MS_VIRT_EN_POR) > > - ctrlpriv->virt_en = 1; > > + ctrlpriv->caam_caps |= CAAM_CAPS_VIRT_ENABLED; > > } > > > > - if (ctrlpriv->virt_en == 1) > > + if (ctrlpriv->caam_caps & CAAM_CAPS_VIRT_ENABLED) > > clrsetbits_32(&ctrl->jrstart, 0, JRSTART_JR0_START | > > JRSTART_JR1_START | JRSTART_JR2_START | > > JRSTART_JR3_START); > > @@ -785,7 +787,7 @@ static int caam_probe(struct platform_device *pdev) > > caam_debugfs_init(ctrlpriv, dfs_root); > > > > /* Check to see if (DPAA 1.x) QI present. If so, enable */ > > - if (ctrlpriv->qi_present && !caam_dpaa2) { > > + if ((ctrlpriv->caam_caps & CAAM_CAPS_QI_PRESENT) && !caam_dpaa2) { > > ctrlpriv->qi = (struct caam_queue_if __iomem __force *) > > ((__force uint8_t *)ctrl + > > BLOCK_OFFSET * QI_BLOCK_NUMBER > > @@ -810,12 +812,13 @@ static int caam_probe(struct platform_device > > *pdev) > > (ring + JR_BLOCK_NUMBER) * > > BLOCK_OFFSET > > ); > > - ctrlpriv->total_jobrs++; > > ring++; > > + ctrlpriv->caam_caps |= BIT(ring); > > I think this deserves an own macro. At the moment you assume that > the lower bits are for the rings, right? I'd like to see something > like > ctrlpriv->caam_caps |= JR_PRESENT(ring); > > then have > #define JR_PRESENT_MASK GENMASK(7, 0) > #define JR_PRESENT(x) (BIT(x) & JR_PRESENT_MASK) > together with all the other bits in caam_caps. Or something > along that. I guess you got the idea. True, would come up with a separate macro for this. > > > > } > > > > /* If no QI and no rings specified, quit and go home */ > > - if ((!ctrlpriv->qi_present) && (!ctrlpriv->total_jobrs)) { > > + if (!(ctrlpriv->caam_caps & CAAM_CAPS_QI_PRESENT) && > > + (hweight_long(ctrlpriv->caam_caps & CAAM_CAPS_JOBRS_MASK) == 0)) > > { > > dev_err(dev, "no queues configured, terminating\n"); > > return -ENOMEM; > > } > > @@ -832,7 +835,8 @@ static int caam_probe(struct platform_device *pdev) > > * already instantiated, do RNG instantiation > > * In case of SoCs with Management Complex, RNG is managed by MC f/w. > > */ > > - if (!(ctrlpriv->mc_en && pr_support) && rng_vid >= 4) { > > + if (!((ctrlpriv->caam_caps & CAAM_CAPS_MC_ENABLED) && pr_support) > && > > + rng_vid >= 4) { > > ctrlpriv->rng4_sh_init = > > rd_reg32(&ctrl->r4tst[0].rdsta); > > /* > > @@ -900,8 +904,9 @@ static int caam_probe(struct platform_device *pdev) > > /* Report "alive" for developer to see */ > > dev_info(dev, "device ID = 0x%016llx (Era %d)\n", caam_id, > > ctrlpriv->era); > > - dev_info(dev, "job rings = %d, qi = %d\n", > > - ctrlpriv->total_jobrs, ctrlpriv->qi_present); > > + dev_info(dev, "job rings = %ld, qi = %s\n", > > + hweight_long(ctrlpriv->caam_caps & CAAM_CAPS_JOBRS_MASK), > > + (ctrlpriv->caam_caps & CAAM_CAPS_QI_PRESENT) ? "yes" : "no"); > > > > ret = devm_of_platform_populate(dev); > > if (ret) > > diff --git a/drivers/crypto/caam/intern.h > > b/drivers/crypto/caam/intern.h > > index 7d45b21bd55a..37f0b93c7087 100644 > > --- a/drivers/crypto/caam/intern.h > > +++ b/drivers/crypto/caam/intern.h > > @@ -86,15 +86,15 @@ struct caam_drv_private { > > > > struct iommu_domain *domain; > > > > - /* > > - * Detected geometry block. Filled in from device tree if powerpc, > > - * or from register-based version detection code > > - */ > > - u8 total_jobrs; /* Total Job Rings in device */ > > - u8 qi_present; /* Nonzero if QI present in device */ > > - u8 mc_en; /* Nonzero if MC f/w is active */ > > + unsigned long caam_caps; /* CAAM Module capabilities */ > > + > > +#define CAAM_CAPS_QI_PRESENT BIT(0) /* Queue Manager interface (QI) > > implemented */ > > +#define CAAM_CAPS_JOBRS_MASK GENMASK(15, 1) /* Job Ring is available > > in NS World */ > > ok I see you already have something like that. See above. That BIT() > in the code above should go away. > > > +#define CAAM_CAPS_MC_ENABLED BIT(16) /* Management Complex is > enabled > > (F/W is active) */ > > +#define CAAM_CAPS_VIRT_ENABLED BIT(17) /* Virtualization enabled */ > > +#define CAAM_CAPS_64K_PAGESIZE BIT(18) /* CAAM register page size > > (64KB if set, 4KB if unset) */ > > + > > int secvio_irq; /* Security violation interrupt number */ > > - int virt_en; /* Virtualization enabled in CAAM */ > > int era; /* CAAM Era (internal HW revision) */ > > > > #define RNG4_MAX_HANDLES 2 > > diff --git a/drivers/crypto/caam/regs.h b/drivers/crypto/caam/regs.h > > index 3738625c0250..186e76e6a3e7 100644 > > --- a/drivers/crypto/caam/regs.h > > +++ b/drivers/crypto/caam/regs.h > > @@ -1023,6 +1023,4 @@ struct caam_deco { > > #define ASSURE_BLOCK_NUMBER 6 > > #define QI_BLOCK_NUMBER 7 > > #define DECO_BLOCK_NUMBER 8 > > -#define PG_SIZE_4K 0x1000 > > -#define PG_SIZE_64K 0x10000 > > nice ;) > > > #endif /* REGS_H */ > > Otherwise, I really like this cleanup. Thanks a lot! This patch makes the driver more easy to follow. > > Thanks, > -michael -- andrey ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v2 2/2] crypto: caam - check jr permissions before probing 2021-11-11 16:45 ` [PATCH v2 0/2] CAAM Driver: re-factor and dynamic JR availability check Andrey Zhizhikin 2021-11-11 16:46 ` [PATCH v2 1/2] crypto: caam - convert to use capabilities Andrey Zhizhikin @ 2021-11-11 16:46 ` Andrey Zhizhikin 2021-11-12 21:17 ` Michael Walle 2021-12-07 23:02 ` [PATCH v3 0/2] CAAM Driver: re-factor and set proper JR status Andrey Zhizhikin 2 siblings, 1 reply; 36+ messages in thread From: Andrey Zhizhikin @ 2021-11-11 16:46 UTC (permalink / raw) To: horia.geanta, pankaj.gupta, herbert, davem, iuliana.prodan, michael, linux-crypto, linux-kernel Cc: Andrey Zhizhikin Job Rings can be set to be exclusively used by TrustZone which makes the access to those rings only possible from Secure World. This access separation is defined by setting bits in CAAM JRxDID_MS register. Once reserved to be owned by TrustZone, this Job Ring becomes unavailable for the Kernel. This reservation is performed early in the boot process, even before the Kernel starts, which leads to unavailability of the HW at the probing stage. Moreover, the reservation can be done for any Job Ring and is not under control of the Kernel. Current implementation lists Job Rings as child nodes of CAAM driver, and tries to perform probing on those regardless of whether JR HW is accessible or not. This leads to the following error while probing: [ 1.509894] caam 30900000.crypto: job rings = 3, qi = 0 [ 1.525201] caam_jr 30901000.jr: failed to flush job ring 0 [ 1.525214] caam_jr: probe of 30901000.jr failed with error -5 Implement a dynamic mechanism to identify which Job Ring is actually marked as owned by TrustZone, and fail the probing of those child nodes with -ENODEV. If the Job Ring is released from the Secure World at the later stage, then bind can be performed, which would check the Ring availability and proceed with probing if the Ring is released. Signed-off-by: Andrey Zhizhikin <andrey.zhizhikin@leica-geosystems.com> --- Changes in V2: - Create and export new method in CAAM control driver to verify JR validity based on the address supplied. - Re-work the logic JR driver to call CAAM control method instead of bit verification. This ensures the actual information retrival from CAAM module during JR probe. - Clean-up of internal static job indexes used during probing, new indexing is performed based on the address supplied in DTB node. drivers/crypto/caam/ctrl.c | 63 ++++++++++++++++++++++++++++++------ drivers/crypto/caam/intern.h | 2 ++ drivers/crypto/caam/jr.c | 33 ++++++++++++++++--- drivers/crypto/caam/regs.h | 7 ++-- 4 files changed, 87 insertions(+), 18 deletions(-) diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c index 7a14a69d89c7..ffe233f2c4ef 100644 --- a/drivers/crypto/caam/ctrl.c +++ b/drivers/crypto/caam/ctrl.c @@ -79,6 +79,42 @@ static void build_deinstantiation_desc(u32 *desc, int handle) append_jump(desc, JUMP_CLASS_CLASS1 | JUMP_TYPE_HALT); } +/* + * caam_ctrl_check_jr_perm - check if the job ring can be accessed + * from Non-Secure World. + * @ctrldev - pointer to CAAM control device + * @ring_addr - input address of Job Ring, which access should be verified + * + * Return: - 0 if Job Ring is available in NS World + * - 1 if Job Ring is reserved in the Secure World + */ +inline int caam_ctrl_check_jr_perm(struct device *ctrldev, u32 ring_addr) +{ + struct caam_drv_private *ctrlpriv = dev_get_drvdata(ctrldev); + struct caam_ctrl __iomem *ctrl = ctrlpriv->ctrl; + u32 ring_id; + + ring_id = ring_addr >> + ((ctrlpriv->caam_caps & CAAM_CAPS_64K_PAGESIZE) ? + 16 : 12); + /* + * Check if the JR is not owned exclusively by TZ, + * and update capabilities bitmap to indicate that + * the Job Ring is available. + * Note: Ring index is 0-based in the register map + */ + if (!((rd_reg32(&ctrl->jr_mid[ring_id - 1].liodn_ms)) & + MSTRID_LOCK_TZ_OWN)) { + ctrlpriv->caam_caps |= BIT(ring_id); + return 0; + } + + /* Job Ring is reserved, clear bit if is was set before */ + ctrlpriv->caam_caps &= ~BIT(ring_id); + return 1; +} +EXPORT_SYMBOL(caam_ctrl_check_jr_perm); + /* * run_descriptor_deco0 - runs a descriptor on DECO0, under direct control of * the software (no JR/QI used). @@ -612,7 +648,7 @@ static bool check_version(struct fsl_mc_version *mc_version, u32 major, /* Probe routine for CAAM top (controller) level */ static int caam_probe(struct platform_device *pdev) { - int ret, ring, gen_sk, ent_delay = RTSDCTL_ENT_DLY_MIN; + int ret, gen_sk, ent_delay = RTSDCTL_ENT_DLY_MIN; u64 caam_id; const struct soc_device_attribute *imx_soc_match; struct device *dev; @@ -803,20 +839,27 @@ static int caam_probe(struct platform_device *pdev) #endif } - ring = 0; for_each_available_child_of_node(nprop, np) if (of_device_is_compatible(np, "fsl,sec-v4.0-job-ring") || of_device_is_compatible(np, "fsl,sec4.0-job-ring")) { - ctrlpriv->jr[ring] = (struct caam_job_ring __iomem __force *) - ((__force uint8_t *)ctrl + - (ring + JR_BLOCK_NUMBER) * - BLOCK_OFFSET - ); - ring++; - ctrlpriv->caam_caps |= BIT(ring); + u32 ring_id; + /* + * Get register page to see the start address of CB + * This is used to index into the bitmap of available + * job rings and indicate if it is available in NS world. + */ + ret = of_property_read_u32(np, "reg", &ring_id); + if (ret) { + dev_err(dev, "failed to get register address for jobr\n"); + continue; + } + caam_ctrl_check_jr_perm(dev, ring_id); } - /* If no QI and no rings specified, quit and go home */ + /* + * If no QI, no rings specified or no rings available for NS - + * quit and go home + */ if (!(ctrlpriv->caam_caps & CAAM_CAPS_QI_PRESENT) && (hweight_long(ctrlpriv->caam_caps & CAAM_CAPS_JOBRS_MASK) == 0)) { dev_err(dev, "no queues configured, terminating\n"); diff --git a/drivers/crypto/caam/intern.h b/drivers/crypto/caam/intern.h index 37f0b93c7087..8d6a0cff556a 100644 --- a/drivers/crypto/caam/intern.h +++ b/drivers/crypto/caam/intern.h @@ -115,6 +115,8 @@ struct caam_drv_private { #endif }; +inline int caam_ctrl_check_jr_perm(struct device *ctrldev, u32 ring_addr); + #ifdef CONFIG_CRYPTO_DEV_FSL_CAAM_CRYPTO_API int caam_algapi_init(struct device *dev); diff --git a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c index 7f2b1101f567..e1bc1ce5515e 100644 --- a/drivers/crypto/caam/jr.c +++ b/drivers/crypto/caam/jr.c @@ -90,7 +90,7 @@ static int caam_reset_hw_jr(struct device *dev) if ((rd_reg32(&jrp->rregs->jrintstatus) & JRINT_ERR_HALT_MASK) != JRINT_ERR_HALT_COMPLETE || timeout == 0) { - dev_err(dev, "failed to flush job ring %d\n", jrp->ridx); + dev_err(dev, "failed to flush job ring %x\n", jrp->ridx); return -EIO; } @@ -101,7 +101,7 @@ static int caam_reset_hw_jr(struct device *dev) cpu_relax(); if (timeout == 0) { - dev_err(dev, "failed to reset job ring %d\n", jrp->ridx); + dev_err(dev, "failed to reset job ring %x\n", jrp->ridx); return -EIO; } @@ -489,7 +489,7 @@ static int caam_jr_init(struct device *dev) error = devm_request_irq(dev, jrp->irq, caam_jr_interrupt, IRQF_SHARED, dev_name(dev), dev); if (error) { - dev_err(dev, "can't connect JobR %d interrupt (%d)\n", + dev_err(dev, "can't connect JobR %x interrupt (%d)\n", jrp->ridx, jrp->irq); tasklet_kill(&jrp->irqtask); } @@ -511,10 +511,33 @@ static int caam_jr_probe(struct platform_device *pdev) struct device_node *nprop; struct caam_job_ring __iomem *ctrl; struct caam_drv_private_jr *jrpriv; - static int total_jobrs; + u32 ring_addr; struct resource *r; int error; + /* + * Get register page to see the start address of CB. + * Job Rings have their respective input base addresses + * defined in the register IRBAR_JRx. This address is + * present in the DT node and is aligned to the page + * size defined at CAAM compile time. + */ + if (of_property_read_u32(pdev->dev.of_node, "reg", &ring_addr)) { + dev_err(&pdev->dev, "failed to get register address for jobr\n"); + return -ENOMEM; + } + + if (caam_ctrl_check_jr_perm(pdev->dev.parent, ring_addr)) { + /* + * This job ring is marked to be exclusively used by TZ, + * do not proceed with probing as the HW block is inaccessible. + * Defer this device probing for later, it might be released + * into NS world. + */ + dev_info(&pdev->dev, "job ring is reserved in secure world\n"); + return -ENODEV; + } + jrdev = &pdev->dev; jrpriv = devm_kzalloc(jrdev, sizeof(*jrpriv), GFP_KERNEL); if (!jrpriv) @@ -523,7 +546,7 @@ static int caam_jr_probe(struct platform_device *pdev) dev_set_drvdata(jrdev, jrpriv); /* save ring identity relative to detection */ - jrpriv->ridx = total_jobrs++; + jrpriv->ridx = ring_addr; nprop = pdev->dev.of_node; /* Get configuration properties from device tree */ diff --git a/drivers/crypto/caam/regs.h b/drivers/crypto/caam/regs.h index 186e76e6a3e7..c4e8574bc570 100644 --- a/drivers/crypto/caam/regs.h +++ b/drivers/crypto/caam/regs.h @@ -445,10 +445,11 @@ struct caam_perfmon { }; /* LIODN programming for DMA configuration */ -#define MSTRID_LOCK_LIODN 0x80000000 -#define MSTRID_LOCK_MAKETRUSTED 0x00010000 /* only for JR masterid */ +#define MSTRID_LOCK_LIODN BIT(31) +#define MSTRID_LOCK_MAKETRUSTED BIT(16) /* only for JR: masterid */ +#define MSTRID_LOCK_TZ_OWN BIT(15) /* only for JR: owned by TZ */ -#define MSTRID_LIODN_MASK 0x0fff +#define MSTRID_LIODN_MASK GENMASK(11, 0) struct masterid { u32 liodn_ms; /* lock and make-trusted control bits */ u32 liodn_ls; /* LIODN for non-sequence and seq access */ -- 2.25.1 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH v2 2/2] crypto: caam - check jr permissions before probing 2021-11-11 16:46 ` [PATCH v2 2/2] crypto: caam - check jr permissions before probing Andrey Zhizhikin @ 2021-11-12 21:17 ` Michael Walle 2021-11-15 10:07 ` ZHIZHIKIN Andrey 0 siblings, 1 reply; 36+ messages in thread From: Michael Walle @ 2021-11-12 21:17 UTC (permalink / raw) To: Andrey Zhizhikin Cc: horia.geanta, pankaj.gupta, herbert, davem, iuliana.prodan, linux-crypto, linux-kernel Am 2021-11-11 17:46, schrieb Andrey Zhizhikin: > Job Rings can be set to be exclusively used by TrustZone which makes > the > access to those rings only possible from Secure World. This access > separation is defined by setting bits in CAAM JRxDID_MS register. Once > reserved to be owned by TrustZone, this Job Ring becomes unavailable > for > the Kernel. This reservation is performed early in the boot process, > even before the Kernel starts, which leads to unavailability of the HW > at the probing stage. Moreover, the reservation can be done for any Job > Ring and is not under control of the Kernel. > > Current implementation lists Job Rings as child nodes of CAAM driver, > and tries to perform probing on those regardless of whether JR HW is > accessible or not. > > This leads to the following error while probing: > [ 1.509894] caam 30900000.crypto: job rings = 3, qi = 0 > [ 1.525201] caam_jr 30901000.jr: failed to flush job ring 0 > [ 1.525214] caam_jr: probe of 30901000.jr failed with error -5 > > Implement a dynamic mechanism to identify which Job Ring is actually > marked as owned by TrustZone, and fail the probing of those child nodes > with -ENODEV. For other reviewers/maintainers: I'm still not sure this is the way to go. Instead one can let u-boot fix up the device tree and remove or disable the JR node if its not available. > If the Job Ring is released from the Secure World at the later stage, > then bind can be performed, which would check the Ring availability and > proceed with probing if the Ring is released. But what if its the other way around and S world will "steal" it from NS world. > > Signed-off-by: Andrey Zhizhikin <andrey.zhizhikin@leica-geosystems.com> > --- > Changes in V2: > - Create and export new method in CAAM control driver to verify JR > validity based on the address supplied. > - Re-work the logic JR driver to call CAAM control method instead of > bit > verification. This ensures the actual information retrival from CAAM > module during JR probe. > - Clean-up of internal static job indexes used during probing, new > indexing is performed based on the address supplied in DTB node. > > drivers/crypto/caam/ctrl.c | 63 ++++++++++++++++++++++++++++++------ > drivers/crypto/caam/intern.h | 2 ++ > drivers/crypto/caam/jr.c | 33 ++++++++++++++++--- > drivers/crypto/caam/regs.h | 7 ++-- > 4 files changed, 87 insertions(+), 18 deletions(-) > > diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c > index 7a14a69d89c7..ffe233f2c4ef 100644 > --- a/drivers/crypto/caam/ctrl.c > +++ b/drivers/crypto/caam/ctrl.c > @@ -79,6 +79,42 @@ static void build_deinstantiation_desc(u32 *desc, > int handle) > append_jump(desc, JUMP_CLASS_CLASS1 | JUMP_TYPE_HALT); > } > > +/* > + * caam_ctrl_check_jr_perm - check if the job ring can be accessed > + * from Non-Secure World. > + * @ctrldev - pointer to CAAM control device > + * @ring_addr - input address of Job Ring, which access should be > verified > + * > + * Return: - 0 if Job Ring is available in NS World > + * - 1 if Job Ring is reserved in the Secure World > + */ > +inline int caam_ctrl_check_jr_perm(struct device *ctrldev, u32 > ring_addr) inline? static int caam_ctrl_.. > +{ > + struct caam_drv_private *ctrlpriv = dev_get_drvdata(ctrldev); > + struct caam_ctrl __iomem *ctrl = ctrlpriv->ctrl; > + u32 ring_id; > + > + ring_id = ring_addr >> > + ((ctrlpriv->caam_caps & CAAM_CAPS_64K_PAGESIZE) ? > + 16 : 12); mh also here: if (ctrlpriv->caam_caps & CAAM_CAPS_64K_PAGESIZE) ring_id = ring_addr >> 16; else ring_id = ring_addr >> 12; Is there something to replace that "16" and "12" by the SZ_ macros, btw? > + /* > + * Check if the JR is not owned exclusively by TZ, > + * and update capabilities bitmap to indicate that > + * the Job Ring is available. > + * Note: Ring index is 0-based in the register map > + */ > + if (!((rd_reg32(&ctrl->jr_mid[ring_id - 1].liodn_ms)) & > + MSTRID_LOCK_TZ_OWN)) { > + ctrlpriv->caam_caps |= BIT(ring_id); See also the former patch, this should be a macro. > + return 0; > + } > + > + /* Job Ring is reserved, clear bit if is was set before */ > + ctrlpriv->caam_caps &= ~BIT(ring_id); > + return 1; > +} > +EXPORT_SYMBOL(caam_ctrl_check_jr_perm); no need for exporting this, no? > + > /* > * run_descriptor_deco0 - runs a descriptor on DECO0, under direct > control of > * the software (no JR/QI used). > @@ -612,7 +648,7 @@ static bool check_version(struct fsl_mc_version > *mc_version, u32 major, > /* Probe routine for CAAM top (controller) level */ > static int caam_probe(struct platform_device *pdev) > { > - int ret, ring, gen_sk, ent_delay = RTSDCTL_ENT_DLY_MIN; > + int ret, gen_sk, ent_delay = RTSDCTL_ENT_DLY_MIN; > u64 caam_id; > const struct soc_device_attribute *imx_soc_match; > struct device *dev; > @@ -803,20 +839,27 @@ static int caam_probe(struct platform_device > *pdev) > #endif > } > > - ring = 0; > for_each_available_child_of_node(nprop, np) > if (of_device_is_compatible(np, "fsl,sec-v4.0-job-ring") || > of_device_is_compatible(np, "fsl,sec4.0-job-ring")) { > - ctrlpriv->jr[ring] = (struct caam_job_ring __iomem __force *) > - ((__force uint8_t *)ctrl + > - (ring + JR_BLOCK_NUMBER) * > - BLOCK_OFFSET > - ); > - ring++; > - ctrlpriv->caam_caps |= BIT(ring); > + u32 ring_id; > + /* > + * Get register page to see the start address of CB > + * This is used to index into the bitmap of available > + * job rings and indicate if it is available in NS world. > + */ > + ret = of_property_read_u32(np, "reg", &ring_id); Not sure about this one, but I don't have any better idea. > + if (ret) { > + dev_err(dev, "failed to get register address for jobr\n"); > + continue; > + } > + caam_ctrl_check_jr_perm(dev, ring_id); What about if (!caam_ctrl_is_jr_available(dev, ring_id)) ctrlpriv->caam_caps &= ~BIT(ring_id); Again BIT() should be its own macro. > } > > - /* If no QI and no rings specified, quit and go home */ > + /* > + * If no QI, no rings specified or no rings available for NS - > + * quit and go home > + */ > if (!(ctrlpriv->caam_caps & CAAM_CAPS_QI_PRESENT) && > (hweight_long(ctrlpriv->caam_caps & CAAM_CAPS_JOBRS_MASK) == 0)) > { > dev_err(dev, "no queues configured, terminating\n"); > diff --git a/drivers/crypto/caam/intern.h > b/drivers/crypto/caam/intern.h > index 37f0b93c7087..8d6a0cff556a 100644 > --- a/drivers/crypto/caam/intern.h > +++ b/drivers/crypto/caam/intern.h > @@ -115,6 +115,8 @@ struct caam_drv_private { > #endif > }; > > +inline int caam_ctrl_check_jr_perm(struct device *ctrldev, u32 > ring_addr); > + > #ifdef CONFIG_CRYPTO_DEV_FSL_CAAM_CRYPTO_API > > int caam_algapi_init(struct device *dev); > diff --git a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c > index 7f2b1101f567..e1bc1ce5515e 100644 > --- a/drivers/crypto/caam/jr.c > +++ b/drivers/crypto/caam/jr.c > @@ -90,7 +90,7 @@ static int caam_reset_hw_jr(struct device *dev) > > if ((rd_reg32(&jrp->rregs->jrintstatus) & JRINT_ERR_HALT_MASK) != > JRINT_ERR_HALT_COMPLETE || timeout == 0) { > - dev_err(dev, "failed to flush job ring %d\n", jrp->ridx); > + dev_err(dev, "failed to flush job ring %x\n", jrp->ridx); mh? why changing this? > return -EIO; > } > > @@ -101,7 +101,7 @@ static int caam_reset_hw_jr(struct device *dev) > cpu_relax(); > > if (timeout == 0) { > - dev_err(dev, "failed to reset job ring %d\n", jrp->ridx); > + dev_err(dev, "failed to reset job ring %x\n", jrp->ridx); > return -EIO; > } > > @@ -489,7 +489,7 @@ static int caam_jr_init(struct device *dev) > error = devm_request_irq(dev, jrp->irq, caam_jr_interrupt, > IRQF_SHARED, > dev_name(dev), dev); > if (error) { > - dev_err(dev, "can't connect JobR %d interrupt (%d)\n", > + dev_err(dev, "can't connect JobR %x interrupt (%d)\n", > jrp->ridx, jrp->irq); > tasklet_kill(&jrp->irqtask); > } > @@ -511,10 +511,33 @@ static int caam_jr_probe(struct platform_device > *pdev) > struct device_node *nprop; > struct caam_job_ring __iomem *ctrl; > struct caam_drv_private_jr *jrpriv; > - static int total_jobrs; > + u32 ring_addr; > struct resource *r; > int error; > > + /* > + * Get register page to see the start address of CB. > + * Job Rings have their respective input base addresses > + * defined in the register IRBAR_JRx. This address is > + * present in the DT node and is aligned to the page > + * size defined at CAAM compile time. > + */ > + if (of_property_read_u32(pdev->dev.of_node, "reg", &ring_addr)) { > + dev_err(&pdev->dev, "failed to get register address for jobr\n"); > + return -ENOMEM; > + } > + > + if (caam_ctrl_check_jr_perm(pdev->dev.parent, ring_addr)) { With the change above, this will also have no bogus side effects on caam_caps. > + /* > + * This job ring is marked to be exclusively used by TZ, > + * do not proceed with probing as the HW block is inaccessible. > + * Defer this device probing for later, it might be released > + * into NS world. > + */ > + dev_info(&pdev->dev, "job ring is reserved in secure world\n"); > + return -ENODEV; > + } > + > jrdev = &pdev->dev; > jrpriv = devm_kzalloc(jrdev, sizeof(*jrpriv), GFP_KERNEL); > if (!jrpriv) > @@ -523,7 +546,7 @@ static int caam_jr_probe(struct platform_device > *pdev) > dev_set_drvdata(jrdev, jrpriv); > > /* save ring identity relative to detection */ > - jrpriv->ridx = total_jobrs++; > + jrpriv->ridx = ring_addr; > > nprop = pdev->dev.of_node; > /* Get configuration properties from device tree */ > diff --git a/drivers/crypto/caam/regs.h b/drivers/crypto/caam/regs.h > index 186e76e6a3e7..c4e8574bc570 100644 > --- a/drivers/crypto/caam/regs.h > +++ b/drivers/crypto/caam/regs.h > @@ -445,10 +445,11 @@ struct caam_perfmon { > }; > > /* LIODN programming for DMA configuration */ > -#define MSTRID_LOCK_LIODN 0x80000000 > -#define MSTRID_LOCK_MAKETRUSTED 0x00010000 /* only for JR masterid */ > +#define MSTRID_LOCK_LIODN BIT(31) > +#define MSTRID_LOCK_MAKETRUSTED BIT(16) /* only for JR: masterid */ > +#define MSTRID_LOCK_TZ_OWN BIT(15) /* only for JR: owned by TZ */ > > -#define MSTRID_LIODN_MASK 0x0fff > +#define MSTRID_LIODN_MASK GENMASK(11, 0) > struct masterid { > u32 liodn_ms; /* lock and make-trusted control bits */ > u32 liodn_ls; /* LIODN for non-sequence and seq access */ -- -michael ^ permalink raw reply [flat|nested] 36+ messages in thread
* RE: [PATCH v2 2/2] crypto: caam - check jr permissions before probing 2021-11-12 21:17 ` Michael Walle @ 2021-11-15 10:07 ` ZHIZHIKIN Andrey 2021-11-15 11:17 ` Michael Walle 2021-11-18 0:47 ` Horia Geantă 0 siblings, 2 replies; 36+ messages in thread From: ZHIZHIKIN Andrey @ 2021-11-15 10:07 UTC (permalink / raw) To: Michael Walle Cc: horia.geanta, pankaj.gupta, herbert, davem, iuliana.prodan, linux-crypto, linux-kernel Hello Michael, > -----Original Message----- > From: Michael Walle <michael@walle.cc> > Sent: Friday, November 12, 2021 10:18 PM > To: ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com> > Cc: horia.geanta@nxp.com; pankaj.gupta@nxp.com; > herbert@gondor.apana.org.au; davem@davemloft.net; > iuliana.prodan@nxp.com; linux-crypto@vger.kernel.org; linux- > kernel@vger.kernel.org > Subject: Re: [PATCH v2 2/2] crypto: caam - check jr permissions before probing > > > Am 2021-11-11 17:46, schrieb Andrey Zhizhikin: > > Job Rings can be set to be exclusively used by TrustZone which makes > > the access to those rings only possible from Secure World. This access > > separation is defined by setting bits in CAAM JRxDID_MS register. Once > > reserved to be owned by TrustZone, this Job Ring becomes unavailable > > for the Kernel. This reservation is performed early in the boot > > process, even before the Kernel starts, which leads to unavailability > > of the HW at the probing stage. Moreover, the reservation can be done > > for any Job Ring and is not under control of the Kernel. > > > > Current implementation lists Job Rings as child nodes of CAAM driver, > > and tries to perform probing on those regardless of whether JR HW is > > accessible or not. > > > > This leads to the following error while probing: > > [ 1.509894] caam 30900000.crypto: job rings = 3, qi = 0 > > [ 1.525201] caam_jr 30901000.jr: failed to flush job ring 0 > > [ 1.525214] caam_jr: probe of 30901000.jr failed with error -5 > > > > Implement a dynamic mechanism to identify which Job Ring is actually > > marked as owned by TrustZone, and fail the probing of those child > > nodes with -ENODEV. > > For other reviewers/maintainers: I'm still not sure this is the way to go. Instead > one can let u-boot fix up the device tree and remove or disable the JR node if its > not available. Just as further clarification: this patch is intended to accommodate for cases where JR is claimed in S world at the boot and not available for Kernel. It does not account for fully dynamic cases, where JRs can be reclaimed between S <-> NS Worlds during runtime. It rather accounts for situation when any arbitrary JR can be reserved by any software entity before Kernel starts without a need to disable nodes at compile time. Full dynamic recognition is a part of much bigger task and is out of scope here. > > > If the Job Ring is released from the Secure World at the later stage, > > then bind can be performed, which would check the Ring availability > > and proceed with probing if the Ring is released. > > But what if its the other way around and S world will "steal" it from NS world. This is not accounted for in this patch, as I do not know if there is any notification mechanism exists between S <-> NS world that would allow to share the status of JR. The implementation in this patch does provide a mechanism to perform a later bind, but does not have any mechanism to indicate when it should be done... > > > > > Signed-off-by: Andrey Zhizhikin > > <andrey.zhizhikin@leica-geosystems.com> > > --- > > Changes in V2: > > - Create and export new method in CAAM control driver to verify JR > > validity based on the address supplied. > > - Re-work the logic JR driver to call CAAM control method instead of > > bit > > verification. This ensures the actual information retrival from CAAM > > module during JR probe. > > - Clean-up of internal static job indexes used during probing, new > > indexing is performed based on the address supplied in DTB node. > > > > drivers/crypto/caam/ctrl.c | 63 ++++++++++++++++++++++++++++++------ > > drivers/crypto/caam/intern.h | 2 ++ > > drivers/crypto/caam/jr.c | 33 ++++++++++++++++--- > > drivers/crypto/caam/regs.h | 7 ++-- > > 4 files changed, 87 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c > > index 7a14a69d89c7..ffe233f2c4ef 100644 > > --- a/drivers/crypto/caam/ctrl.c > > +++ b/drivers/crypto/caam/ctrl.c > > @@ -79,6 +79,42 @@ static void build_deinstantiation_desc(u32 *desc, > > int handle) > > append_jump(desc, JUMP_CLASS_CLASS1 | JUMP_TYPE_HALT); } > > > > +/* > > + * caam_ctrl_check_jr_perm - check if the job ring can be accessed > > + * from Non-Secure World. > > + * @ctrldev - pointer to CAAM control device > > + * @ring_addr - input address of Job Ring, which access should be > > verified > > + * > > + * Return: - 0 if Job Ring is available in NS World > > + * - 1 if Job Ring is reserved in the Secure World > > + */ > > +inline int caam_ctrl_check_jr_perm(struct device *ctrldev, u32 > > ring_addr) > > inline? > static int caam_ctrl_.. > > > +{ > > + struct caam_drv_private *ctrlpriv = dev_get_drvdata(ctrldev); > > + struct caam_ctrl __iomem *ctrl = ctrlpriv->ctrl; > > + u32 ring_id; > > + > > + ring_id = ring_addr >> > > + ((ctrlpriv->caam_caps & CAAM_CAPS_64K_PAGESIZE) ? > > + 16 : 12); > > mh also here: > if (ctrlpriv->caam_caps & CAAM_CAPS_64K_PAGESIZE) > ring_id = ring_addr >> 16; > else > ring_id = ring_addr >> 12; > > Is there something to replace that "16" and "12" by the SZ_ macros, btw? Good point, I'd need to look into this further on with what this can be replaced. > > > + /* > > + * Check if the JR is not owned exclusively by TZ, > > + * and update capabilities bitmap to indicate that > > + * the Job Ring is available. > > + * Note: Ring index is 0-based in the register map > > + */ > > + if (!((rd_reg32(&ctrl->jr_mid[ring_id - 1].liodn_ms)) & > > + MSTRID_LOCK_TZ_OWN)) { > > + ctrlpriv->caam_caps |= BIT(ring_id); > > See also the former patch, this should be a macro. True, would be covered in V3. > > > + return 0; > > + } > > + > > + /* Job Ring is reserved, clear bit if is was set before */ > > + ctrlpriv->caam_caps &= ~BIT(ring_id); > > + return 1; > > +} > > +EXPORT_SYMBOL(caam_ctrl_check_jr_perm); > > no need for exporting this, no? Unfortunately, both CONFIG_CRYPTO_DEV_FSL_CAAM and CONFIG_CRYPTO_DEV_FSL_CAAM_JR are tristate. Setting both config options to "=m" fails to resolve caam_ctrl_check_jr_perm, therefore I had to export it. It strikes me odd however that CAAM can be compiled as module without CAAM_JR module at all. This would imply that DECO is used directly, which according to SRM is used for pure descriptor debug purposes and should never be used in production. I guess CRYPTO_DEV_FSL_CAAM _JR should be merged into CRYPTO_DEV_FSL_CAAM, so they at least comes together. In that case the export would not be necessary at all. I must admit I didn't find this a good solution, therefore any advise on a better solution here is highly appreciated. > > > + > > /* > > * run_descriptor_deco0 - runs a descriptor on DECO0, under direct > > control of > > * the software (no JR/QI used). > > @@ -612,7 +648,7 @@ static bool check_version(struct fsl_mc_version > > *mc_version, u32 major, > > /* Probe routine for CAAM top (controller) level */ static int > > caam_probe(struct platform_device *pdev) { > > - int ret, ring, gen_sk, ent_delay = RTSDCTL_ENT_DLY_MIN; > > + int ret, gen_sk, ent_delay = RTSDCTL_ENT_DLY_MIN; > > u64 caam_id; > > const struct soc_device_attribute *imx_soc_match; > > struct device *dev; > > @@ -803,20 +839,27 @@ static int caam_probe(struct platform_device > > *pdev) > > #endif > > } > > > > - ring = 0; > > for_each_available_child_of_node(nprop, np) > > if (of_device_is_compatible(np, "fsl,sec-v4.0-job-ring") || > > of_device_is_compatible(np, "fsl,sec4.0-job-ring")) { > > - ctrlpriv->jr[ring] = (struct caam_job_ring __iomem __force *) > > - ((__force uint8_t *)ctrl + > > - (ring + JR_BLOCK_NUMBER) * > > - BLOCK_OFFSET > > - ); > > - ring++; > > - ctrlpriv->caam_caps |= BIT(ring); > > + u32 ring_id; > > + /* > > + * Get register page to see the start address of CB > > + * This is used to index into the bitmap of available > > + * job rings and indicate if it is available in NS world. > > + */ > > + ret = of_property_read_u32(np, "reg", &ring_id); > > Not sure about this one, but I don't have any better idea. Similar approach is proposed in U-Boot series [1]. > > > > + if (ret) { > > + dev_err(dev, "failed to get register address for jobr\n"); > > + continue; > > + } > > + caam_ctrl_check_jr_perm(dev, ring_id); > > What about > if (!caam_ctrl_is_jr_available(dev, ring_id)) > ctrlpriv->caam_caps &= ~BIT(ring_id); > > Again BIT() should be its own macro. Yes, would replace it in V3. > > > } > > > > - /* If no QI and no rings specified, quit and go home */ > > + /* > > + * If no QI, no rings specified or no rings available for NS - > > + * quit and go home > > + */ > > if (!(ctrlpriv->caam_caps & CAAM_CAPS_QI_PRESENT) && > > (hweight_long(ctrlpriv->caam_caps & CAAM_CAPS_JOBRS_MASK) == > > 0)) { > > dev_err(dev, "no queues configured, terminating\n"); > > diff --git a/drivers/crypto/caam/intern.h > > b/drivers/crypto/caam/intern.h index 37f0b93c7087..8d6a0cff556a 100644 > > --- a/drivers/crypto/caam/intern.h > > +++ b/drivers/crypto/caam/intern.h > > @@ -115,6 +115,8 @@ struct caam_drv_private { #endif }; > > > > +inline int caam_ctrl_check_jr_perm(struct device *ctrldev, u32 > > ring_addr); > > + > > #ifdef CONFIG_CRYPTO_DEV_FSL_CAAM_CRYPTO_API > > > > int caam_algapi_init(struct device *dev); diff --git > > a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c index > > 7f2b1101f567..e1bc1ce5515e 100644 > > --- a/drivers/crypto/caam/jr.c > > +++ b/drivers/crypto/caam/jr.c > > @@ -90,7 +90,7 @@ static int caam_reset_hw_jr(struct device *dev) > > > > if ((rd_reg32(&jrp->rregs->jrintstatus) & JRINT_ERR_HALT_MASK) != > > JRINT_ERR_HALT_COMPLETE || timeout == 0) { > > - dev_err(dev, "failed to flush job ring %d\n", jrp->ridx); > > + dev_err(dev, "failed to flush job ring %x\n", > > + jrp->ridx); > > mh? why changing this? After the change, jrp->ridx would contain JR hex address instead of index, therefore I had to replace the debug output. > > > return -EIO; > > } > > > > @@ -101,7 +101,7 @@ static int caam_reset_hw_jr(struct device *dev) > > cpu_relax(); > > > > if (timeout == 0) { > > - dev_err(dev, "failed to reset job ring %d\n", jrp->ridx); > > + dev_err(dev, "failed to reset job ring %x\n", > > + jrp->ridx); > > return -EIO; > > } > > > > @@ -489,7 +489,7 @@ static int caam_jr_init(struct device *dev) > > error = devm_request_irq(dev, jrp->irq, caam_jr_interrupt, > > IRQF_SHARED, > > dev_name(dev), dev); > > if (error) { > > - dev_err(dev, "can't connect JobR %d interrupt (%d)\n", > > + dev_err(dev, "can't connect JobR %x interrupt (%d)\n", > > jrp->ridx, jrp->irq); > > tasklet_kill(&jrp->irqtask); > > } > > @@ -511,10 +511,33 @@ static int caam_jr_probe(struct platform_device > > *pdev) > > struct device_node *nprop; > > struct caam_job_ring __iomem *ctrl; > > struct caam_drv_private_jr *jrpriv; > > - static int total_jobrs; > > + u32 ring_addr; > > struct resource *r; > > int error; > > > > + /* > > + * Get register page to see the start address of CB. > > + * Job Rings have their respective input base addresses > > + * defined in the register IRBAR_JRx. This address is > > + * present in the DT node and is aligned to the page > > + * size defined at CAAM compile time. > > + */ > > + if (of_property_read_u32(pdev->dev.of_node, "reg", &ring_addr)) { > > + dev_err(&pdev->dev, "failed to get register address for jobr\n"); > > + return -ENOMEM; > > + } > > + > > + if (caam_ctrl_check_jr_perm(pdev->dev.parent, ring_addr)) { > > With the change above, this will also have no bogus side effects on caam_caps. > > > + /* > > + * This job ring is marked to be exclusively used by TZ, > > + * do not proceed with probing as the HW block is inaccessible. > > + * Defer this device probing for later, it might be released > > + * into NS world. > > + */ > > + dev_info(&pdev->dev, "job ring is reserved in secure world\n"); > > + return -ENODEV; > > + } > > + > > jrdev = &pdev->dev; > > jrpriv = devm_kzalloc(jrdev, sizeof(*jrpriv), GFP_KERNEL); > > if (!jrpriv) > > @@ -523,7 +546,7 @@ static int caam_jr_probe(struct platform_device > > *pdev) > > dev_set_drvdata(jrdev, jrpriv); > > > > /* save ring identity relative to detection */ > > - jrpriv->ridx = total_jobrs++; > > + jrpriv->ridx = ring_addr; > > > > nprop = pdev->dev.of_node; > > /* Get configuration properties from device tree */ diff --git > > a/drivers/crypto/caam/regs.h b/drivers/crypto/caam/regs.h index > > 186e76e6a3e7..c4e8574bc570 100644 > > --- a/drivers/crypto/caam/regs.h > > +++ b/drivers/crypto/caam/regs.h > > @@ -445,10 +445,11 @@ struct caam_perfmon { }; > > > > /* LIODN programming for DMA configuration */ > > -#define MSTRID_LOCK_LIODN 0x80000000 > > -#define MSTRID_LOCK_MAKETRUSTED 0x00010000 /* only for JR > masterid */ > > +#define MSTRID_LOCK_LIODN BIT(31) > > +#define MSTRID_LOCK_MAKETRUSTED BIT(16) /* only for JR: masterid */ > > +#define MSTRID_LOCK_TZ_OWN BIT(15) /* only for JR: owned by TZ */ > > > > -#define MSTRID_LIODN_MASK 0x0fff > > +#define MSTRID_LIODN_MASK GENMASK(11, 0) > > struct masterid { > > u32 liodn_ms; /* lock and make-trusted control bits */ > > u32 liodn_ls; /* LIODN for non-sequence and seq access */ > > -- > -michael Link: [1]: http://patchwork.ozlabs.org/project/uboot/patch/20211115070014.17586-2-gaurav.jain@nxp.com/ -- andrey ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 2/2] crypto: caam - check jr permissions before probing 2021-11-15 10:07 ` ZHIZHIKIN Andrey @ 2021-11-15 11:17 ` Michael Walle 2021-11-18 0:47 ` Horia Geantă 1 sibling, 0 replies; 36+ messages in thread From: Michael Walle @ 2021-11-15 11:17 UTC (permalink / raw) To: ZHIZHIKIN Andrey Cc: horia.geanta, pankaj.gupta, herbert, davem, iuliana.prodan, linux-crypto, linux-kernel >> > +EXPORT_SYMBOL(caam_ctrl_check_jr_perm); >> >> no need for exporting this, no? > > Unfortunately, both CONFIG_CRYPTO_DEV_FSL_CAAM and > CONFIG_CRYPTO_DEV_FSL_CAAM_JR are tristate. Setting both > config options to "=m" fails to resolve caam_ctrl_check_jr_perm, > therefore I had to export it. > > It strikes me odd however that CAAM can be compiled as module > without CAAM_JR module at all. This would imply that DECO is used > directly, which according to SRM is used for pure descriptor debug > purposes and should never be used in production. > > I guess CRYPTO_DEV_FSL_CAAM _JR should be merged into > CRYPTO_DEV_FSL_CAAM, so they at least comes together. In that > case the export would not be necessary at all. > > I must admit I didn't find this a good solution, therefore any advise > on a better solution here is highly appreciated. I see, and I'm too lazy at the moment to figure that out ;) but afaik new exports should be only EXPORT_SYMBOL_GPL(). >> > if ((rd_reg32(&jrp->rregs->jrintstatus) & JRINT_ERR_HALT_MASK) != >> > JRINT_ERR_HALT_COMPLETE || timeout == 0) { >> > - dev_err(dev, "failed to flush job ring %d\n", jrp->ridx); >> > + dev_err(dev, "failed to flush job ring %x\n", >> > + jrp->ridx); >> >> mh? why changing this? > > After the change, jrp->ridx would contain JR hex address instead of > index, > therefore I had to replace the debug output. ahh then, ridx should renamed accordingly, I suppose. -michael ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 2/2] crypto: caam - check jr permissions before probing 2021-11-15 10:07 ` ZHIZHIKIN Andrey 2021-11-15 11:17 ` Michael Walle @ 2021-11-18 0:47 ` Horia Geantă 2021-11-18 8:28 ` Michael Walle 1 sibling, 1 reply; 36+ messages in thread From: Horia Geantă @ 2021-11-18 0:47 UTC (permalink / raw) To: ZHIZHIKIN Andrey, Michael Walle Cc: Pankaj Gupta, herbert, davem, Iuliana Prodan, linux-crypto, linux-kernel, linux-imx On 11/15/2021 12:07 PM, ZHIZHIKIN Andrey wrote: > Hello Michael, > >> -----Original Message----- >> From: Michael Walle <michael@walle.cc> >> Sent: Friday, November 12, 2021 10:18 PM >> To: ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com> >> Cc: horia.geanta@nxp.com; pankaj.gupta@nxp.com; >> herbert@gondor.apana.org.au; davem@davemloft.net; >> iuliana.prodan@nxp.com; linux-crypto@vger.kernel.org; linux- >> kernel@vger.kernel.org >> Subject: Re: [PATCH v2 2/2] crypto: caam - check jr permissions before probing >> >> >> Am 2021-11-11 17:46, schrieb Andrey Zhizhikin: >>> Job Rings can be set to be exclusively used by TrustZone which makes >>> the access to those rings only possible from Secure World. This access >>> separation is defined by setting bits in CAAM JRxDID_MS register. Once >>> reserved to be owned by TrustZone, this Job Ring becomes unavailable >>> for the Kernel. This reservation is performed early in the boot >>> process, even before the Kernel starts, which leads to unavailability >>> of the HW at the probing stage. Moreover, the reservation can be done >>> for any Job Ring and is not under control of the Kernel. >>> >>> Current implementation lists Job Rings as child nodes of CAAM driver, >>> and tries to perform probing on those regardless of whether JR HW is >>> accessible or not. >>> >>> This leads to the following error while probing: >>> [ 1.509894] caam 30900000.crypto: job rings = 3, qi = 0 >>> [ 1.525201] caam_jr 30901000.jr: failed to flush job ring 0 >>> [ 1.525214] caam_jr: probe of 30901000.jr failed with error -5 >>> >>> Implement a dynamic mechanism to identify which Job Ring is actually >>> marked as owned by TrustZone, and fail the probing of those child >>> nodes with -ENODEV. >> >> For other reviewers/maintainers: I'm still not sure this is the way to go. Instead >> one can let u-boot fix up the device tree and remove or disable the JR node if its >> not available. > > Just as further clarification: this patch is intended to accommodate for cases where > JR is claimed in S world at the boot and not available for Kernel. It does not account > for fully dynamic cases, where JRs can be reclaimed between S <-> NS Worlds > during runtime. It rather accounts for situation when any arbitrary JR can be reserved > by any software entity before Kernel starts without a need to disable nodes at > compile time. > I prefer f/w to fix the DT before passing it to the kernel, either by adding the "secure-status" property (set explicitly to "disabled") or by removing the job ring node(s) that are reserved. OP-TEE already uses the first option. We should probably pick this up. The reason I am supporting relying on DT and consequently avoiding registers is that accessing page 0 in the caam register space from Non-secure world should be avoided when caam is managed by Secure world (e.g. OP-TEE) or a Secure Enclave (e.g. SECO). Unfortunately support for HW-enforced access control for caam register space is not that great / fine-grained, with the exception of more recent parts like i.MX8MP and i.MX8ULP. Horia ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 2/2] crypto: caam - check jr permissions before probing 2021-11-18 0:47 ` Horia Geantă @ 2021-11-18 8:28 ` Michael Walle 2021-11-18 10:08 ` ZHIZHIKIN Andrey 0 siblings, 1 reply; 36+ messages in thread From: Michael Walle @ 2021-11-18 8:28 UTC (permalink / raw) To: Horia Geantă Cc: ZHIZHIKIN Andrey, Pankaj Gupta, herbert, davem, Iuliana Prodan, linux-crypto, linux-kernel, linux-imx Hi Horia, >>>> Job Rings can be set to be exclusively used by TrustZone which makes >>>> the access to those rings only possible from Secure World. This >>>> access >>>> separation is defined by setting bits in CAAM JRxDID_MS register. >>>> Once >>>> reserved to be owned by TrustZone, this Job Ring becomes unavailable >>>> for the Kernel. This reservation is performed early in the boot >>>> process, even before the Kernel starts, which leads to >>>> unavailability >>>> of the HW at the probing stage. Moreover, the reservation can be >>>> done >>>> for any Job Ring and is not under control of the Kernel. >>>> >>>> Current implementation lists Job Rings as child nodes of CAAM >>>> driver, >>>> and tries to perform probing on those regardless of whether JR HW is >>>> accessible or not. >>>> >>>> This leads to the following error while probing: >>>> [ 1.509894] caam 30900000.crypto: job rings = 3, qi = 0 >>>> [ 1.525201] caam_jr 30901000.jr: failed to flush job ring 0 >>>> [ 1.525214] caam_jr: probe of 30901000.jr failed with error -5 >>>> >>>> Implement a dynamic mechanism to identify which Job Ring is actually >>>> marked as owned by TrustZone, and fail the probing of those child >>>> nodes with -ENODEV. >>> >>> For other reviewers/maintainers: I'm still not sure this is the way >>> to go. Instead >>> one can let u-boot fix up the device tree and remove or disable the >>> JR node if its >>> not available. >> >> Just as further clarification: this patch is intended to accommodate >> for cases where >> JR is claimed in S world at the boot and not available for Kernel. It >> does not account >> for fully dynamic cases, where JRs can be reclaimed between S <-> NS >> Worlds >> during runtime. It rather accounts for situation when any arbitrary JR >> can be reserved >> by any software entity before Kernel starts without a need to disable >> nodes at >> compile time. >> > I prefer f/w to fix the DT before passing it to the kernel, > either by adding the "secure-status" property (set explicitly to > "disabled") > or by removing the job ring node(s) that are reserved. > OP-TEE already uses the first option. We should probably pick this up. Ah, nice: Documentation/devicetree/bindings/arm/secure.txt If I understand this correctly, if optee reserves a JR it will set the secure-status to okay and status to disabled. (There is still a missing link, how u-boot will then be passed this modified device tree, I might miss something here.) But what about the HAB, if I understand Andrey correct, then JR0 will already be marked as "S world only" (or at least no EL3 program will release it again). To me it looks like then either JR0 should be (1) hardcoded to secure-status = "okay", status = "disabled", or (2) u-boot SPL (or TF-A) should return it to NS world (and optee might take it over again). > The reason I am supporting relying on DT and consequently avoiding > registers > is that accessing page 0 in the caam register space from Non-secure > world > should be avoided when caam is managed by Secure world (e.g. OP-TEE) > or a Secure Enclave (e.g. SECO). > > Unfortunately support for HW-enforced access control for caam register > space > is not that great / fine-grained, with the exception of more recent > parts > like i.MX8MP and i.MX8ULP. -michael ^ permalink raw reply [flat|nested] 36+ messages in thread
* RE: [PATCH v2 2/2] crypto: caam - check jr permissions before probing 2021-11-18 8:28 ` Michael Walle @ 2021-11-18 10:08 ` ZHIZHIKIN Andrey 2021-11-18 10:11 ` Michael Walle 0 siblings, 1 reply; 36+ messages in thread From: ZHIZHIKIN Andrey @ 2021-11-18 10:08 UTC (permalink / raw) To: Michael Walle, Horia Geantă Cc: Pankaj Gupta, herbert, davem, Iuliana Prodan, linux-crypto, linux-kernel, linux-imx, Gaurav Jain Hello Horia/Michael, I'd reply here to both of you since your answers are complementing each other. I've also Cc: Gaurav here as he is working on CAAM support in U-Boot so this discussion is relevant for him as well I suppose. > -----Original Message----- > From: Michael Walle <michael@walle.cc> > Sent: Thursday, November 18, 2021 9:29 AM > To: Horia Geantă <horia.geanta@nxp.com> > Cc: ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com>; Pankaj Gupta > <pankaj.gupta@nxp.com>; herbert@gondor.apana.org.au; davem@davemloft.net; Iuliana > Prodan <iuliana.prodan@nxp.com>; linux-crypto@vger.kernel.org; linux- > kernel@vger.kernel.org; linux-imx <linux-imx@nxp.com> > Subject: Re: [PATCH v2 2/2] crypto: caam - check jr permissions before probing > > > Hi Horia, > > >>>> Job Rings can be set to be exclusively used by TrustZone which makes > >>>> the access to those rings only possible from Secure World. This > >>>> access > >>>> separation is defined by setting bits in CAAM JRxDID_MS register. > >>>> Once > >>>> reserved to be owned by TrustZone, this Job Ring becomes unavailable > >>>> for the Kernel. This reservation is performed early in the boot > >>>> process, even before the Kernel starts, which leads to > >>>> unavailability > >>>> of the HW at the probing stage. Moreover, the reservation can be > >>>> done > >>>> for any Job Ring and is not under control of the Kernel. > >>>> > >>>> Current implementation lists Job Rings as child nodes of CAAM > >>>> driver, > >>>> and tries to perform probing on those regardless of whether JR HW is > >>>> accessible or not. > >>>> > >>>> This leads to the following error while probing: > >>>> [ 1.509894] caam 30900000.crypto: job rings = 3, qi = 0 > >>>> [ 1.525201] caam_jr 30901000.jr: failed to flush job ring 0 > >>>> [ 1.525214] caam_jr: probe of 30901000.jr failed with error -5 > >>>> > >>>> Implement a dynamic mechanism to identify which Job Ring is actually > >>>> marked as owned by TrustZone, and fail the probing of those child > >>>> nodes with -ENODEV. > >>> > >>> For other reviewers/maintainers: I'm still not sure this is the way > >>> to go. Instead > >>> one can let u-boot fix up the device tree and remove or disable the > >>> JR node if its > >>> not available. > >> > >> Just as further clarification: this patch is intended to accommodate > >> for cases where > >> JR is claimed in S world at the boot and not available for Kernel. It > >> does not account > >> for fully dynamic cases, where JRs can be reclaimed between S <-> NS > >> Worlds > >> during runtime. It rather accounts for situation when any arbitrary JR > >> can be reserved > >> by any software entity before Kernel starts without a need to disable > >> nodes at > >> compile time. > >> > > I prefer f/w to fix the DT before passing it to the kernel, > > either by adding the "secure-status" property (set explicitly to > > "disabled") > > or by removing the job ring node(s) that are reserved. According to the DT bindings doc mentioned by Michael below, it would not be needed to remove the node. Setting status = "disabled"; secure-status = "okay" should be enough to reserve JR node in S World permanently. It would also serve the purpose to indicate that the HW do provide the correct total amount of JRs, and just some of then are not available to be used in NS World. > > OP-TEE already uses the first option. We should probably pick this up. Agree. I would drop the register access from this patch and follow-up with DT node approach. > > Ah, nice: > Documentation/devicetree/bindings/arm/secure.txt Good point, thanks for the doc guidance! This does provide a clear layout on how the DT node should be crafted! > > If I understand this correctly, if optee reserves a JR it will set the > secure-status to okay and status to disabled. (There is still a missing > link, how u-boot will then be passed this modified device tree, I might > miss something here.) I need to look at how OP-TEE does things here, but if they just set secure-status = "okay" - then the JR should be visible in both worlds. > > But what about the HAB, if I understand Andrey correct, then JR0 will > already be marked as "S world only" (or at least no EL3 program will > release it again). It's a good point, which is still unclear: can JR0 be reclaimed back after HAB is finished? Or should it stay in S-only world? > To me it looks like then either JR0 should be > (1) hardcoded to secure-status = "okay", status = "disabled", or (2) > u-boot SPL (or TF-A) should return it to NS world (and optee might > take it over again). If the answer to HAB question is: it should stay in S World, then I'd suggest to go with (1) as it presents the opportunity to define the initial state of JR0 in deterministic state, without loosing the information that HW does indeed have it implemented (node is present, but permanently disabled). Later reclamation with this combination is also possible. What I would propose in addition here as well, is to define the secure-status = "disabled" for all JR nodes on all derivatives, to have the status set consistently. If later reclamation for any arbitrary JR from NS to S world is needed - this property can be adjusted accordingly by SW entity. Same thing should be done in U-Boot I suppose. > > > The reason I am supporting relying on DT and consequently avoiding > > registers > > is that accessing page 0 in the caam register space from Non-secure > > world > > should be avoided when caam is managed by Secure world (e.g. OP-TEE) > > or a Secure Enclave (e.g. SECO). Understood, this is a valid point that I was missing. I'd re-work this to use DT bindings and push it in V3. I guess there would not be much left of this patch once I'd use DT approach, so I'd re-spin the series to include DT bindings instead. JR driver clean-up to remove static JR counter :| would go into the first one then. > > > > Unfortunately support for HW-enforced access control for caam register > > space > > is not that great / fine-grained, with the exception of more recent > > parts > > like i.MX8MP and i.MX8ULP. Are there any particular distinct differences on those derivatives that should be taken into account here with respect of JR reservation? I might include those as well in this patch series if they are not that significant, otherwise would try to address them separately. > > -michael Thanks to all of you for commenting here! -- andrey ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 2/2] crypto: caam - check jr permissions before probing 2021-11-18 10:08 ` ZHIZHIKIN Andrey @ 2021-11-18 10:11 ` Michael Walle 0 siblings, 0 replies; 36+ messages in thread From: Michael Walle @ 2021-11-18 10:11 UTC (permalink / raw) To: ZHIZHIKIN Andrey Cc: Horia Geantă, Pankaj Gupta, herbert, davem, Iuliana Prodan, linux-crypto, linux-kernel, linux-imx, Gaurav Jain Am 2021-11-18 11:08, schrieb ZHIZHIKIN Andrey: > I guess there would not be much left of this patch once I'd use DT > approach, > so I'd re-spin the series to include DT bindings instead. JR driver > clean-up > to remove static JR counter :| would go into the first one then. I really like the cleanup though! Esp. the removing of the static variable. -michael ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v3 0/2] CAAM Driver: re-factor and set proper JR status 2021-11-11 16:45 ` [PATCH v2 0/2] CAAM Driver: re-factor and dynamic JR availability check Andrey Zhizhikin 2021-11-11 16:46 ` [PATCH v2 1/2] crypto: caam - convert to use capabilities Andrey Zhizhikin 2021-11-11 16:46 ` [PATCH v2 2/2] crypto: caam - check jr permissions before probing Andrey Zhizhikin @ 2021-12-07 23:02 ` Andrey Zhizhikin 2021-12-07 23:02 ` [PATCH v3 1/2] crypto: caam - convert to use capabilities Andrey Zhizhikin ` (2 more replies) 2 siblings, 3 replies; 36+ messages in thread From: Andrey Zhizhikin @ 2021-12-07 23:02 UTC (permalink / raw) To: linux-kernel Cc: robh+dt, shawnguo, michael, s.hauer, kernel, festevam, linux-imx, horia.geanta, pankaj.gupta, herbert, davem, l.stach, qiangqing.zhang, peng.fan, alice.guo, aford173, frieder.schrempf, krzk, shengjiu.wang, gregkh, ping.bai, daniel.baluta, jun.li, marex, thunder.leizhen, martink, leonard.crestez, hongxing.zhu, agx, devicetree, linux-arm-kernel, linux-crypto, op-tee, Andrey Zhizhikin This V3 series covers points uncovered during the review of the previous series, one major point being that register readout should not be used for dynamic JR availability check due to its unreliability. Instead, JR should have a proper status set in FDT which indicates the availability of the ring in NS-World. This status is aligned with what BootROM code configures, and can be modified by all actors in the boot chain. Therefore, patch in V2 series that was handling the dynamic JR availability check is dropped in this series and replaced by the patch which sets proper DT status for JR nodes. Andrey Zhizhikin (2): crypto: caam - convert to use capabilities arm64: dts: imx8m: define proper status for caam jr arch/arm64/boot/dts/freescale/imx8mm.dtsi | 4 + arch/arm64/boot/dts/freescale/imx8mn.dtsi | 4 + arch/arm64/boot/dts/freescale/imx8mp.dtsi | 4 + arch/arm64/boot/dts/freescale/imx8mq.dtsi | 4 + drivers/crypto/caam/caamalg_qi.c | 2 +- drivers/crypto/caam/ctrl.c | 115 ++++++++++++++-------- drivers/crypto/caam/intern.h | 20 ++-- drivers/crypto/caam/jr.c | 19 +++- drivers/crypto/caam/regs.h | 2 - 9 files changed, 122 insertions(+), 52 deletions(-) base-commit: 04fe99a8d936d46a310ca61b8b63dc270962bf01 -- 2.25.1 ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v3 1/2] crypto: caam - convert to use capabilities 2021-12-07 23:02 ` [PATCH v3 0/2] CAAM Driver: re-factor and set proper JR status Andrey Zhizhikin @ 2021-12-07 23:02 ` Andrey Zhizhikin 2021-12-07 23:02 ` [PATCH v3 2/2] arm64: dts: imx8m: define proper status for caam jr Andrey Zhizhikin 2022-01-06 10:56 ` [PATCH v3 0/2] CAAM Driver: re-factor and set proper JR status ZHIZHIKIN Andrey 2 siblings, 0 replies; 36+ messages in thread From: Andrey Zhizhikin @ 2021-12-07 23:02 UTC (permalink / raw) To: linux-kernel Cc: robh+dt, shawnguo, michael, s.hauer, kernel, festevam, linux-imx, horia.geanta, pankaj.gupta, herbert, davem, l.stach, qiangqing.zhang, peng.fan, alice.guo, aford173, frieder.schrempf, krzk, shengjiu.wang, gregkh, ping.bai, daniel.baluta, jun.li, marex, thunder.leizhen, martink, leonard.crestez, hongxing.zhu, agx, devicetree, linux-arm-kernel, linux-crypto, op-tee, Andrey Zhizhikin CAAM driver contains several variables, which are used for indication that certain capabilities are detected during initial probing of the device. They are defined as u8, but mainly used as boolean variables to identify capabillities. CAAM JR driver code also contains the static variable in the probe method, which is used to derive the JR index value and does not respect how JRs are implemented in the HW. Clean-up all assorted variables, collect them into one bitfield value which encodes capabilities as bit, and use them in the execution flow instead. Replace static indexing in JR driver with index derived from "reg" binding, as it reflects the actual JR number in the HW. Signed-off-by: Andrey Zhizhikin <andrey.zhizhikin@leica-geosystems.com> --- Changes in V3: - Address review comments from V2 series, replace inline if statements with explicit ones - Add more helper macros - Merge change done in separate patch for JR indexing into here - Change caam_ctrl_check_jr_perm() from register readout to status binding check - Do not export caam_ctrl_check_jr_perm() anymore, drop the declaration from header - Remove more local variables in probe() and replace them with capabilities readout drivers/crypto/caam/caamalg_qi.c | 2 +- drivers/crypto/caam/ctrl.c | 115 ++++++++++++++++++++----------- drivers/crypto/caam/intern.h | 20 +++--- drivers/crypto/caam/jr.c | 19 ++++- drivers/crypto/caam/regs.h | 2 - 5 files changed, 106 insertions(+), 52 deletions(-) diff --git a/drivers/crypto/caam/caamalg_qi.c b/drivers/crypto/caam/caamalg_qi.c index 189a7438b29c..1b7cdae28290 100644 --- a/drivers/crypto/caam/caamalg_qi.c +++ b/drivers/crypto/caam/caamalg_qi.c @@ -2610,7 +2610,7 @@ int caam_qi_algapi_init(struct device *ctrldev) bool registered = false; /* Make sure this runs only on (DPAA 1.x) QI */ - if (!priv->qi_present || caam_dpaa2) + if (!(priv->caam_caps & CAAM_CAPS_QI_PRESENT) || caam_dpaa2) return 0; /* diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c index ca0361b2dbb0..37c0c6af1137 100644 --- a/drivers/crypto/caam/ctrl.c +++ b/drivers/crypto/caam/ctrl.c @@ -79,6 +79,36 @@ static void build_deinstantiation_desc(u32 *desc, int handle) append_jump(desc, JUMP_CLASS_CLASS1 | JUMP_TYPE_HALT); } +/* + * caam_ctrl_check_jr_perm - check if the job ring can be accessed + * from Non-Secure World. + * @np - pointer to JR device node + * + * Return: - 0 if Job Ring is reserved in the Secure World + * - 1 if Job Ring is available in NS World + */ +static inline int caam_ctrl_check_jr_perm(const struct device_node *np) +{ + struct property *p; + + /* read "status" property first */ + p = of_find_property(np, "status", NULL); + if (p && (!strncmp(p->value, "disabled", p->length))) { + /* + * "status" is set to "disabled", which would imply that the JR + * is not available for NS World. All other possible combination + * of "status" and "secure-status" would rather indicate that JR + * is either available in NS-only, or in both S and NS Worlds. + * In a later case, we indicate that this JR can be used by the + * Kernel since the resource is shared. + * For details, see: + * Documentation/devicetree/bindings/arm/secure.txt + */ + return 0; + } + return 1; +} + /* * run_descriptor_deco0 - runs a descriptor on DECO0, under direct control of * the software (no JR/QI used). @@ -100,7 +130,7 @@ static inline int run_descriptor_deco0(struct device *ctrldev, u32 *desc, int i; - if (ctrlpriv->virt_en == 1 || + if ((ctrlpriv->caam_caps & CAAM_CAPS_VIRT_ENABLED) || /* * Apparently on i.MX8M{Q,M,N,P} it doesn't matter if virt_en == 1 * and the following steps should be performed regardless @@ -169,7 +199,7 @@ static inline int run_descriptor_deco0(struct device *ctrldev, u32 *desc, *status = rd_reg32(&deco->op_status_hi) & DECO_OP_STATUS_HI_ERR_MASK; - if (ctrlpriv->virt_en == 1) + if (ctrlpriv->caam_caps & CAAM_CAPS_VIRT_ENABLED) clrsetbits_32(&ctrl->deco_rsr, DECORSR_JR0, 0); /* Mark the DECO as free */ @@ -612,7 +642,7 @@ static bool check_version(struct fsl_mc_version *mc_version, u32 major, /* Probe routine for CAAM top (controller) level */ static int caam_probe(struct platform_device *pdev) { - int ret, ring, gen_sk, ent_delay = RTSDCTL_ENT_DLY_MIN; + int ret, gen_sk, ent_delay = RTSDCTL_ENT_DLY_MIN; u64 caam_id; const struct soc_device_attribute *imx_soc_match; struct device *dev; @@ -622,8 +652,6 @@ static int caam_probe(struct platform_device *pdev) struct dentry *dfs_root; u32 scfgr, comp_params; u8 rng_vid; - int pg_size; - int BLOCK_OFFSET = 0; bool pr_support = false; ctrlpriv = devm_kzalloc(&pdev->dev, sizeof(*ctrlpriv), GFP_KERNEL); @@ -666,11 +694,12 @@ static int caam_probe(struct platform_device *pdev) else caam_ptr_sz = sizeof(u32); caam_dpaa2 = !!(comp_params & CTPR_MS_DPAA2); - ctrlpriv->qi_present = !!(comp_params & CTPR_MS_QI_MASK); + if (comp_params & CTPR_MS_QI_MASK) + ctrlpriv->caam_caps |= CAAM_CAPS_QI_PRESENT; #ifdef CONFIG_CAAM_QI /* If (DPAA 1.x) QI present, check whether dependencies are available */ - if (ctrlpriv->qi_present && !caam_dpaa2) { + if ((ctrlpriv->caam_caps & CAAM_CAPS_QI_PRESENT) && !caam_dpaa2) { ret = qman_is_probed(); if (!ret) { return -EPROBE_DEFER; @@ -689,33 +718,29 @@ static int caam_probe(struct platform_device *pdev) } #endif - /* Allocating the BLOCK_OFFSET based on the supported page size on - * the platform - */ - pg_size = (comp_params & CTPR_MS_PG_SZ_MASK) >> CTPR_MS_PG_SZ_SHIFT; - if (pg_size == 0) - BLOCK_OFFSET = PG_SIZE_4K; - else - BLOCK_OFFSET = PG_SIZE_64K; + /* Allocate control blocks based on the CAAM supported page size */ + if (comp_params & CTPR_MS_PG_SZ_MASK) + ctrlpriv->caam_caps |= CAAM_CAPS_64K_PAGESIZE; ctrlpriv->ctrl = (struct caam_ctrl __iomem __force *)ctrl; ctrlpriv->assure = (struct caam_assurance __iomem __force *) ((__force uint8_t *)ctrl + - BLOCK_OFFSET * ASSURE_BLOCK_NUMBER + CAAM_CAPS_PG_SZ(ctrlpriv->caam_caps) * ASSURE_BLOCK_NUMBER ); ctrlpriv->deco = (struct caam_deco __iomem __force *) ((__force uint8_t *)ctrl + - BLOCK_OFFSET * DECO_BLOCK_NUMBER + CAAM_CAPS_PG_SZ(ctrlpriv->caam_caps) * DECO_BLOCK_NUMBER ); /* Get the IRQ of the controller (for security violations only) */ ctrlpriv->secvio_irq = irq_of_parse_and_map(nprop, 0); np = of_find_compatible_node(NULL, NULL, "fsl,qoriq-mc"); - ctrlpriv->mc_en = !!np; + if (np) + ctrlpriv->caam_caps |= CAAM_CAPS_MC_ENABLED; of_node_put(np); #ifdef CONFIG_FSL_MC_BUS - if (ctrlpriv->mc_en) { + if (ctrlpriv->caam_caps & CAAM_CAPS_MC_ENABLED) { struct fsl_mc_version *mc_version; mc_version = fsl_mc_get_version(); @@ -732,7 +757,7 @@ static int caam_probe(struct platform_device *pdev) * In case of SoCs with Management Complex, MC f/w performs * the configuration. */ - if (!ctrlpriv->mc_en) + if (!(ctrlpriv->caam_caps & CAAM_CAPS_MC_ENABLED)) clrsetbits_32(&ctrl->mcr, MCFGR_AWCACHE_MASK, MCFGR_AWCACHE_CACH | MCFGR_AWCACHE_BUFF | MCFGR_WDENABLE | MCFGR_LARGE_BURST); @@ -745,7 +770,6 @@ static int caam_probe(struct platform_device *pdev) */ scfgr = rd_reg32(&ctrl->scfgr); - ctrlpriv->virt_en = 0; if (comp_params & CTPR_MS_VIRT_EN_INCL) { /* VIRT_EN_INCL = 1 & VIRT_EN_POR = 1 or * VIRT_EN_INCL = 1 & VIRT_EN_POR = 0 & SCFGR_VIRT_EN = 1 @@ -753,14 +777,14 @@ static int caam_probe(struct platform_device *pdev) if ((comp_params & CTPR_MS_VIRT_EN_POR) || (!(comp_params & CTPR_MS_VIRT_EN_POR) && (scfgr & SCFGR_VIRT_EN))) - ctrlpriv->virt_en = 1; + ctrlpriv->caam_caps |= CAAM_CAPS_VIRT_ENABLED; } else { /* VIRT_EN_INCL = 0 && VIRT_EN_POR_VALUE = 1 */ if (comp_params & CTPR_MS_VIRT_EN_POR) - ctrlpriv->virt_en = 1; + ctrlpriv->caam_caps |= CAAM_CAPS_VIRT_ENABLED; } - if (ctrlpriv->virt_en == 1) + if (ctrlpriv->caam_caps & CAAM_CAPS_VIRT_ENABLED) clrsetbits_32(&ctrl->jrstart, 0, JRSTART_JR0_START | JRSTART_JR1_START | JRSTART_JR2_START | JRSTART_JR3_START); @@ -785,10 +809,10 @@ static int caam_probe(struct platform_device *pdev) caam_debugfs_init(ctrlpriv, dfs_root); /* Check to see if (DPAA 1.x) QI present. If so, enable */ - if (ctrlpriv->qi_present && !caam_dpaa2) { + if ((ctrlpriv->caam_caps & CAAM_CAPS_QI_PRESENT) && !caam_dpaa2) { ctrlpriv->qi = (struct caam_queue_if __iomem __force *) ((__force uint8_t *)ctrl + - BLOCK_OFFSET * QI_BLOCK_NUMBER + CAAM_CAPS_PG_SZ(ctrlpriv->caam_caps) * QI_BLOCK_NUMBER ); /* This is all that's required to physically enable QI */ wr_reg32(&ctrlpriv->qi->qi_control_lo, QICTL_DQEN); @@ -801,21 +825,32 @@ static int caam_probe(struct platform_device *pdev) #endif } - ring = 0; for_each_available_child_of_node(nprop, np) if (of_device_is_compatible(np, "fsl,sec-v4.0-job-ring") || of_device_is_compatible(np, "fsl,sec4.0-job-ring")) { - ctrlpriv->jr[ring] = (struct caam_job_ring __iomem __force *) - ((__force uint8_t *)ctrl + - (ring + JR_BLOCK_NUMBER) * - BLOCK_OFFSET - ); - ctrlpriv->total_jobrs++; - ring++; + u32 ring_id; + /* + * Get register page to see the start address of CB + * This is used to index into the bitmap of available + * job rings and indicate if it is available in NS world. + */ + ret = of_property_read_u32(np, "reg", &ring_id); + if (ret) { + dev_err(dev, "failed to get register address for jobr\n"); + continue; + } + ring_id = ring_id >> CAAM_CAPS_PG_SHIFT(ctrlpriv->caam_caps); + + if (caam_ctrl_check_jr_perm(np)) + ctrlpriv->caam_caps |= CAAM_CAPS_JR_PRESENT(ring_id); } - /* If no QI and no rings specified, quit and go home */ - if ((!ctrlpriv->qi_present) && (!ctrlpriv->total_jobrs)) { + /* + * If no QI, no rings specified or no rings available for NS - + * quit and go home + */ + if (!(ctrlpriv->caam_caps & CAAM_CAPS_QI_PRESENT) && + (hweight_long(ctrlpriv->caam_caps & CAAM_CAPS_JOBRS_MASK) == 0)) { dev_err(dev, "no queues configured, terminating\n"); return -ENOMEM; } @@ -832,7 +867,8 @@ static int caam_probe(struct platform_device *pdev) * already instantiated, do RNG instantiation * In case of SoCs with Management Complex, RNG is managed by MC f/w. */ - if (!(ctrlpriv->mc_en && pr_support) && rng_vid >= 4) { + if (!((ctrlpriv->caam_caps & CAAM_CAPS_MC_ENABLED) && pr_support) && + rng_vid >= 4) { ctrlpriv->rng4_sh_init = rd_reg32(&ctrl->r4tst[0].rdsta); /* @@ -900,8 +936,9 @@ static int caam_probe(struct platform_device *pdev) /* Report "alive" for developer to see */ dev_info(dev, "device ID = 0x%016llx (Era %d)\n", caam_id, ctrlpriv->era); - dev_info(dev, "job rings = %d, qi = %d\n", - ctrlpriv->total_jobrs, ctrlpriv->qi_present); + dev_info(dev, "job rings = %ld, qi = %s\n", + hweight_long(ctrlpriv->caam_caps & CAAM_CAPS_JOBRS_MASK), + (ctrlpriv->caam_caps & CAAM_CAPS_QI_PRESENT) ? "yes" : "no"); ret = devm_of_platform_populate(dev); if (ret) diff --git a/drivers/crypto/caam/intern.h b/drivers/crypto/caam/intern.h index 7d45b21bd55a..37fa9db461c7 100644 --- a/drivers/crypto/caam/intern.h +++ b/drivers/crypto/caam/intern.h @@ -86,15 +86,19 @@ struct caam_drv_private { struct iommu_domain *domain; - /* - * Detected geometry block. Filled in from device tree if powerpc, - * or from register-based version detection code - */ - u8 total_jobrs; /* Total Job Rings in device */ - u8 qi_present; /* Nonzero if QI present in device */ - u8 mc_en; /* Nonzero if MC f/w is active */ + unsigned long caam_caps; /* CAAM Module capabilities */ + +#define CAAM_CAPS_QI_PRESENT BIT(0) /* Queue Manager interface (QI) implemented */ +#define CAAM_CAPS_JOBRS_MASK GENMASK(15, 1) /* Job Ring is available in NS World */ +#define CAAM_CAPS_MC_ENABLED BIT(16) /* Management Complex is enabled (F/W is active) */ +#define CAAM_CAPS_VIRT_ENABLED BIT(17) /* Virtualization enabled */ +#define CAAM_CAPS_64K_PAGESIZE BIT(18) /* CAAM register page size (64KB if set, 4KB if unset) */ + +#define CAAM_CAPS_JR_PRESENT(id) (BIT(id) & CAAM_CAPS_JOBRS_MASK) +#define CAAM_CAPS_PG_SHIFT(caps) (((caps) & CAAM_CAPS_64K_PAGESIZE) ? 16 : 12) +#define CAAM_CAPS_PG_SZ(caps) (1 << CAAM_CAPS_PG_SHIFT(caps)) + int secvio_irq; /* Security violation interrupt number */ - int virt_en; /* Virtualization enabled in CAAM */ int era; /* CAAM Era (internal HW revision) */ #define RNG4_MAX_HANDLES 2 diff --git a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c index 7f2b1101f567..e218d4ae604c 100644 --- a/drivers/crypto/caam/jr.c +++ b/drivers/crypto/caam/jr.c @@ -511,10 +511,25 @@ static int caam_jr_probe(struct platform_device *pdev) struct device_node *nprop; struct caam_job_ring __iomem *ctrl; struct caam_drv_private_jr *jrpriv; - static int total_jobrs; + struct caam_drv_private *caamctrlpriv; + u32 ring_idx; struct resource *r; int error; + /* + * Get register page to see the start address of CB. + * Job Rings have their respective input base addresses + * defined in the register IRBAR_JRx. This address is + * present in the DT node and is aligned to the page + * size defined at CAAM compile time. + */ + if (of_property_read_u32(pdev->dev.of_node, "reg", &ring_idx)) { + dev_err(&pdev->dev, "failed to get register address for jobr\n"); + return -ENOMEM; + } + caamctrlpriv = dev_get_drvdata(pdev->dev.parent); + ring_idx = ring_idx >> CAAM_CAPS_PG_SHIFT(caamctrlpriv->caam_caps); + jrdev = &pdev->dev; jrpriv = devm_kzalloc(jrdev, sizeof(*jrpriv), GFP_KERNEL); if (!jrpriv) @@ -523,7 +538,7 @@ static int caam_jr_probe(struct platform_device *pdev) dev_set_drvdata(jrdev, jrpriv); /* save ring identity relative to detection */ - jrpriv->ridx = total_jobrs++; + jrpriv->ridx = ring_idx; nprop = pdev->dev.of_node; /* Get configuration properties from device tree */ diff --git a/drivers/crypto/caam/regs.h b/drivers/crypto/caam/regs.h index 3738625c0250..186e76e6a3e7 100644 --- a/drivers/crypto/caam/regs.h +++ b/drivers/crypto/caam/regs.h @@ -1023,6 +1023,4 @@ struct caam_deco { #define ASSURE_BLOCK_NUMBER 6 #define QI_BLOCK_NUMBER 7 #define DECO_BLOCK_NUMBER 8 -#define PG_SIZE_4K 0x1000 -#define PG_SIZE_64K 0x10000 #endif /* REGS_H */ -- 2.25.1 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v3 2/2] arm64: dts: imx8m: define proper status for caam jr 2021-12-07 23:02 ` [PATCH v3 0/2] CAAM Driver: re-factor and set proper JR status Andrey Zhizhikin 2021-12-07 23:02 ` [PATCH v3 1/2] crypto: caam - convert to use capabilities Andrey Zhizhikin @ 2021-12-07 23:02 ` Andrey Zhizhikin 2022-01-06 11:26 ` Rouven Czerwinski 2022-01-06 10:56 ` [PATCH v3 0/2] CAAM Driver: re-factor and set proper JR status ZHIZHIKIN Andrey 2 siblings, 1 reply; 36+ messages in thread From: Andrey Zhizhikin @ 2021-12-07 23:02 UTC (permalink / raw) To: linux-kernel Cc: robh+dt, shawnguo, michael, s.hauer, kernel, festevam, linux-imx, horia.geanta, pankaj.gupta, herbert, davem, l.stach, qiangqing.zhang, peng.fan, alice.guo, aford173, frieder.schrempf, krzk, shengjiu.wang, gregkh, ping.bai, daniel.baluta, jun.li, marex, thunder.leizhen, martink, leonard.crestez, hongxing.zhu, agx, devicetree, linux-arm-kernel, linux-crypto, op-tee, Andrey Zhizhikin CAAM JR nodes are configured by BootROM and are used by various software entities during the boot process before they reach the Kernel. Default BootROM configuration have JR0 and JR1 reserved for S-only access, while JR2 is generally available for both S and NS access. HAB feature of i.MX8M family does require that JR0 is reserved exclusively in S-only world, while JR1 and JR2 are both released to NS-World. OP-TEE can later reclaim the JR2 via dt_enable_secure_status() call, and modify the DID to hold it in S-World only. The above setup has been discovered during review of CAAM patchset presented to U-Boot integration [1], and does not correspond to the status on jr nodes in FDT. This missing status settings leads to the following error message during jr node probing: [ 1.509894] caam 30900000.crypto: job rings = 3, qi = 0 [ 1.525201] caam_jr 30901000.jr: failed to flush job ring 0 [ 1.525214] caam_jr: probe of 30901000.jr failed with error -5 JR register readout after BootROM execution shows the following values: JR0DID_MS = 0x8011 JR1DID_MS = 0x8011 JR2DID_MS = 0x0 This shows that JR0 and JR1 have TZ_OWN bit set, which marks them to be reserved for S-World, while JR2 remains accessible from NS-World. Provide the correct status for JR nodes in imx8m derivatives, which have a following meaning: - JR0: S-only - JR1: visible in both - JR2: NS-only Note, that JR2 is initially marked to be NS-only which does correspond to DID readout when OP-TEE is not present. Once present, OP-TEE will reclaim the JR2 and set both "status" and "secure-status" to claim JR2 for S-only access. Signed-off-by: Andrey Zhizhikin <andrey.zhizhikin@leica-geosystems.com> Link: [1]: https://lore.kernel.org/u-boot/AM6PR06MB4691FC905FE5658BE4B15C11A6609@AM6PR06MB4691.eurprd06.prod.outlook.com/ --- Changes in V3: - No change, new patch introduced arch/arm64/boot/dts/freescale/imx8mm.dtsi | 4 ++++ arch/arm64/boot/dts/freescale/imx8mn.dtsi | 4 ++++ arch/arm64/boot/dts/freescale/imx8mp.dtsi | 4 ++++ arch/arm64/boot/dts/freescale/imx8mq.dtsi | 4 ++++ 4 files changed, 16 insertions(+) diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi index 5b9c2cca9ac4..51465974c4ea 100644 --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi @@ -914,18 +914,22 @@ sec_jr0: jr@1000 { compatible = "fsl,sec-v4.0-job-ring"; reg = <0x1000 0x1000>; interrupts = <GIC_SPI 105 IRQ_TYPE_LEVEL_HIGH>; + status = "disabled"; + secure-status = "okay"; }; sec_jr1: jr@2000 { compatible = "fsl,sec-v4.0-job-ring"; reg = <0x2000 0x1000>; interrupts = <GIC_SPI 106 IRQ_TYPE_LEVEL_HIGH>; + secure-status = "okay"; }; sec_jr2: jr@3000 { compatible = "fsl,sec-v4.0-job-ring"; reg = <0x3000 0x1000>; interrupts = <GIC_SPI 114 IRQ_TYPE_LEVEL_HIGH>; + secure-status = "disabled"; }; }; diff --git a/arch/arm64/boot/dts/freescale/imx8mn.dtsi b/arch/arm64/boot/dts/freescale/imx8mn.dtsi index ba23b416b5e6..e5edf14319b1 100644 --- a/arch/arm64/boot/dts/freescale/imx8mn.dtsi +++ b/arch/arm64/boot/dts/freescale/imx8mn.dtsi @@ -808,18 +808,22 @@ sec_jr0: jr@1000 { compatible = "fsl,sec-v4.0-job-ring"; reg = <0x1000 0x1000>; interrupts = <GIC_SPI 105 IRQ_TYPE_LEVEL_HIGH>; + status = "disabled"; + secure-status = "okay"; }; sec_jr1: jr@2000 { compatible = "fsl,sec-v4.0-job-ring"; reg = <0x2000 0x1000>; interrupts = <GIC_SPI 106 IRQ_TYPE_LEVEL_HIGH>; + secure-status = "okay"; }; sec_jr2: jr@3000 { compatible = "fsl,sec-v4.0-job-ring"; reg = <0x3000 0x1000>; interrupts = <GIC_SPI 114 IRQ_TYPE_LEVEL_HIGH>; + secure-status = "disabled"; }; }; diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi index 977783784342..3c23bf5c3910 100644 --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi @@ -661,18 +661,22 @@ sec_jr0: jr@1000 { compatible = "fsl,sec-v4.0-job-ring"; reg = <0x1000 0x1000>; interrupts = <GIC_SPI 105 IRQ_TYPE_LEVEL_HIGH>; + status = "disabled"; + secure-status = "okay"; }; sec_jr1: jr@2000 { compatible = "fsl,sec-v4.0-job-ring"; reg = <0x2000 0x1000>; interrupts = <GIC_SPI 106 IRQ_TYPE_LEVEL_HIGH>; + secure-status = "okay"; }; sec_jr2: jr@3000 { compatible = "fsl,sec-v4.0-job-ring"; reg = <0x3000 0x1000>; interrupts = <GIC_SPI 114 IRQ_TYPE_LEVEL_HIGH>; + secure-status = "disabled"; }; }; diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi b/arch/arm64/boot/dts/freescale/imx8mq.dtsi index 95d8b95d6120..16c4c9110ce7 100644 --- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi +++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi @@ -999,18 +999,22 @@ sec_jr0: jr@1000 { compatible = "fsl,sec-v4.0-job-ring"; reg = <0x1000 0x1000>; interrupts = <GIC_SPI 105 IRQ_TYPE_LEVEL_HIGH>; + status = "disabled"; + secure-status = "okay"; }; sec_jr1: jr@2000 { compatible = "fsl,sec-v4.0-job-ring"; reg = <0x2000 0x1000>; interrupts = <GIC_SPI 106 IRQ_TYPE_LEVEL_HIGH>; + secure-status = "okay"; }; sec_jr2: jr@3000 { compatible = "fsl,sec-v4.0-job-ring"; reg = <0x3000 0x1000>; interrupts = <GIC_SPI 114 IRQ_TYPE_LEVEL_HIGH>; + secure-status = "disabled"; }; }; -- 2.25.1 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH v3 2/2] arm64: dts: imx8m: define proper status for caam jr 2021-12-07 23:02 ` [PATCH v3 2/2] arm64: dts: imx8m: define proper status for caam jr Andrey Zhizhikin @ 2022-01-06 11:26 ` Rouven Czerwinski 2022-01-06 14:08 ` ZHIZHIKIN Andrey 0 siblings, 1 reply; 36+ messages in thread From: Rouven Czerwinski @ 2022-01-06 11:26 UTC (permalink / raw) To: Andrey Zhizhikin, linux-kernel Cc: peng.fan, ping.bai, alice.guo, agx, frieder.schrempf, leonard.crestez, festevam, marex, herbert, horia.geanta, aford173, krzk, linux-imx, devicetree, hongxing.zhu, s.hauer, pankaj.gupta, robh+dt, thunder.leizhen, martink, daniel.baluta, linux-arm-kernel, gregkh, shengjiu.wang, qiangqing.zhang, michael, op-tee, linux-crypto, kernel, jun.li, shawnguo, davem, l.stach Hi Andrey, On Wed, 2021-12-08 at 00:02 +0100, Andrey Zhizhikin wrote: > CAAM JR nodes are configured by BootROM and are used by various software > entities during the boot process before they reach the Kernel. > > Default BootROM configuration have JR0 and JR1 reserved for S-only > access, while JR2 is generally available for both S and NS access. HAB > feature of i.MX8M family does require that JR0 is reserved exclusively > in S-only world, while JR1 and JR2 are both released to NS-World. OP-TEE > can later reclaim the JR2 via dt_enable_secure_status() call, and modify > the DID to hold it in S-World only. > > The above setup has been discovered during review of CAAM patchset > presented to U-Boot integration [1], and does not correspond to the > status on jr nodes in FDT. > > This missing status settings leads to the following error message during > jr node probing: > [ 1.509894] caam 30900000.crypto: job rings = 3, qi = 0 > [ 1.525201] caam_jr 30901000.jr: failed to flush job ring 0 > [ 1.525214] caam_jr: probe of 30901000.jr failed with error -5 > > JR register readout after BootROM execution shows the following values: > JR0DID_MS = 0x8011 > JR1DID_MS = 0x8011 > JR2DID_MS = 0x0 > > This shows that JR0 and JR1 have TZ_OWN bit set, which marks them to be > reserved for S-World, while JR2 remains accessible from NS-World. > > Provide the correct status for JR nodes in imx8m derivatives, which have > a following meaning: > - JR0: S-only > - JR1: visible in both > - JR2: NS-only > > Note, that JR2 is initially marked to be NS-only which does correspond > to DID readout when OP-TEE is not present. Once present, OP-TEE will > reclaim the JR2 and set both "status" and "secure-status" to claim JR2 > for S-only access. While I can understand that you want to fix your use case for when HAB is enabled, note that this is disabling JR0 in the none-HAB case as well. IMO this should be handled correctly by the bootloader and/or OP- TEE. The default upstream configuration for OP-TEE is to not use the CAAM at runtime as well, since linux runtime PM disablement of the CAAM will lock up OP-TEE when it tries to access the CAAM. Kind regards, Rouven Czerwinski > > Signed-off-by: Andrey Zhizhikin <andrey.zhizhikin@leica-geosystems.com> > Link: [1]: https://lore.kernel.org/u-boot/AM6PR06MB4691FC905FE5658BE4B15C11A6609@AM6PR06MB4691.eurprd06.prod.outlook.com/ > --- > Changes in V3: > - No change, new patch introduced > > arch/arm64/boot/dts/freescale/imx8mm.dtsi | 4 ++++ > arch/arm64/boot/dts/freescale/imx8mn.dtsi | 4 ++++ > arch/arm64/boot/dts/freescale/imx8mp.dtsi | 4 ++++ > arch/arm64/boot/dts/freescale/imx8mq.dtsi | 4 ++++ > 4 files changed, 16 insertions(+) > > diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi > index 5b9c2cca9ac4..51465974c4ea 100644 > --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi > +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi > @@ -914,18 +914,22 @@ sec_jr0: jr@1000 { > compatible = "fsl,sec-v4.0-job-ring"; > reg = <0x1000 0x1000>; > interrupts = <GIC_SPI 105 IRQ_TYPE_LEVEL_HIGH>; > + status = "disabled"; > + secure-status = "okay"; > }; > > sec_jr1: jr@2000 { > compatible = "fsl,sec-v4.0-job-ring"; > reg = <0x2000 0x1000>; > interrupts = <GIC_SPI 106 IRQ_TYPE_LEVEL_HIGH>; > + secure-status = "okay"; > }; > > sec_jr2: jr@3000 { > compatible = "fsl,sec-v4.0-job-ring"; > reg = <0x3000 0x1000>; > interrupts = <GIC_SPI 114 IRQ_TYPE_LEVEL_HIGH>; > + secure-status = "disabled"; > }; > }; > > diff --git a/arch/arm64/boot/dts/freescale/imx8mn.dtsi b/arch/arm64/boot/dts/freescale/imx8mn.dtsi > index ba23b416b5e6..e5edf14319b1 100644 > --- a/arch/arm64/boot/dts/freescale/imx8mn.dtsi > +++ b/arch/arm64/boot/dts/freescale/imx8mn.dtsi > @@ -808,18 +808,22 @@ sec_jr0: jr@1000 { > compatible = "fsl,sec-v4.0-job-ring"; > reg = <0x1000 0x1000>; > interrupts = <GIC_SPI 105 IRQ_TYPE_LEVEL_HIGH>; > + status = "disabled"; > + secure-status = "okay"; > }; > > sec_jr1: jr@2000 { > compatible = "fsl,sec-v4.0-job-ring"; > reg = <0x2000 0x1000>; > interrupts = <GIC_SPI 106 IRQ_TYPE_LEVEL_HIGH>; > + secure-status = "okay"; > }; > > sec_jr2: jr@3000 { > compatible = "fsl,sec-v4.0-job-ring"; > reg = <0x3000 0x1000>; > interrupts = <GIC_SPI 114 IRQ_TYPE_LEVEL_HIGH>; > + secure-status = "disabled"; > }; > }; > > diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > index 977783784342..3c23bf5c3910 100644 > --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi > +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > @@ -661,18 +661,22 @@ sec_jr0: jr@1000 { > compatible = "fsl,sec-v4.0-job-ring"; > reg = <0x1000 0x1000>; > interrupts = <GIC_SPI 105 IRQ_TYPE_LEVEL_HIGH>; > + status = "disabled"; > + secure-status = "okay"; > }; > > sec_jr1: jr@2000 { > compatible = "fsl,sec-v4.0-job-ring"; > reg = <0x2000 0x1000>; > interrupts = <GIC_SPI 106 IRQ_TYPE_LEVEL_HIGH>; > + secure-status = "okay"; > }; > > sec_jr2: jr@3000 { > compatible = "fsl,sec-v4.0-job-ring"; > reg = <0x3000 0x1000>; > interrupts = <GIC_SPI 114 IRQ_TYPE_LEVEL_HIGH>; > + secure-status = "disabled"; > }; > }; > > diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi b/arch/arm64/boot/dts/freescale/imx8mq.dtsi > index 95d8b95d6120..16c4c9110ce7 100644 > --- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi > +++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi > @@ -999,18 +999,22 @@ sec_jr0: jr@1000 { > compatible = "fsl,sec-v4.0-job-ring"; > reg = <0x1000 0x1000>; > interrupts = <GIC_SPI 105 IRQ_TYPE_LEVEL_HIGH>; > + status = "disabled"; > + secure-status = "okay"; > }; > > sec_jr1: jr@2000 { > compatible = "fsl,sec-v4.0-job-ring"; > reg = <0x2000 0x1000>; > interrupts = <GIC_SPI 106 IRQ_TYPE_LEVEL_HIGH>; > + secure-status = "okay"; > }; > > sec_jr2: jr@3000 { > compatible = "fsl,sec-v4.0-job-ring"; > reg = <0x3000 0x1000>; > interrupts = <GIC_SPI 114 IRQ_TYPE_LEVEL_HIGH>; > + secure-status = "disabled"; > }; > }; > -- Pengutronix e.K. | Rouven Czerwinski | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | ^ permalink raw reply [flat|nested] 36+ messages in thread
* RE: [PATCH v3 2/2] arm64: dts: imx8m: define proper status for caam jr 2022-01-06 11:26 ` Rouven Czerwinski @ 2022-01-06 14:08 ` ZHIZHIKIN Andrey 2022-01-07 9:46 ` Rouven Czerwinski 0 siblings, 1 reply; 36+ messages in thread From: ZHIZHIKIN Andrey @ 2022-01-06 14:08 UTC (permalink / raw) To: Rouven Czerwinski, linux-kernel Cc: peng.fan, ping.bai, alice.guo, agx, frieder.schrempf, leonard.crestez, festevam, marex, herbert, horia.geanta, aford173, krzk, linux-imx, devicetree, hongxing.zhu, s.hauer, pankaj.gupta, robh+dt, thunder.leizhen, martink, daniel.baluta, linux-arm-kernel, gregkh, shengjiu.wang, qiangqing.zhang, michael, op-tee, linux-crypto, kernel, jun.li, shawnguo, davem, l.stach Hello Rouven, > -----Original Message----- > From: Rouven Czerwinski <r.czerwinski@pengutronix.de> > Sent: Thursday, January 6, 2022 12:27 PM > To: ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com>; linux- > kernel@vger.kernel.org > Cc: peng.fan@nxp.com; ping.bai@nxp.com; alice.guo@nxp.com; agx@sigxcpu.org; > frieder.schrempf@kontron.de; leonard.crestez@nxp.com; festevam@gmail.com; > marex@denx.de; herbert@gondor.apana.org.au; horia.geanta@nxp.com; > aford173@gmail.com; krzk@kernel.org; linux-imx@nxp.com; > devicetree@vger.kernel.org; hongxing.zhu@nxp.com; s.hauer@pengutronix.de; > pankaj.gupta@nxp.com; robh+dt@kernel.org; thunder.leizhen@huawei.com; > martink@posteo.de; daniel.baluta@nxp.com; linux-arm-kernel@lists.infradead.org; > gregkh@linuxfoundation.org; shengjiu.wang@nxp.com; qiangqing.zhang@nxp.com; > michael@walle.cc; op-tee@lists.trustedfirmware.org; linux-crypto@vger.kernel.org; > kernel@pengutronix.de; jun.li@nxp.com; shawnguo@kernel.org; davem@davemloft.net; > l.stach@pengutronix.de > Subject: Re: [PATCH v3 2/2] arm64: dts: imx8m: define proper status for caam jr > > Hi Andrey, > > On Wed, 2021-12-08 at 00:02 +0100, Andrey Zhizhikin wrote: > > CAAM JR nodes are configured by BootROM and are used by various software > > entities during the boot process before they reach the Kernel. > > > > Default BootROM configuration have JR0 and JR1 reserved for S-only > > access, while JR2 is generally available for both S and NS access. HAB > > feature of i.MX8M family does require that JR0 is reserved exclusively > > in S-only world, while JR1 and JR2 are both released to NS-World. OP-TEE > > can later reclaim the JR2 via dt_enable_secure_status() call, and modify > > the DID to hold it in S-World only. > > > > The above setup has been discovered during review of CAAM patchset > > presented to U-Boot integration [1], and does not correspond to the > > status on jr nodes in FDT. > > > > This missing status settings leads to the following error message during > > jr node probing: > > [ 1.509894] caam 30900000.crypto: job rings = 3, qi = 0 > > [ 1.525201] caam_jr 30901000.jr: failed to flush job ring 0 > > [ 1.525214] caam_jr: probe of 30901000.jr failed with error -5 > > > > JR register readout after BootROM execution shows the following values: > > JR0DID_MS = 0x8011 > > JR1DID_MS = 0x8011 > > JR2DID_MS = 0x0 > > > > This shows that JR0 and JR1 have TZ_OWN bit set, which marks them to be > > reserved for S-World, while JR2 remains accessible from NS-World. > > > > Provide the correct status for JR nodes in imx8m derivatives, which have > > a following meaning: > > - JR0: S-only > > - JR1: visible in both > > - JR2: NS-only > > > > Note, that JR2 is initially marked to be NS-only which does correspond > > to DID readout when OP-TEE is not present. Once present, OP-TEE will > > reclaim the JR2 and set both "status" and "secure-status" to claim JR2 > > for S-only access. > > While I can understand that you want to fix your use case for when HAB > is enabled, note that this is disabling JR0 in the none-HAB case as > well. This is not totally correct, as this patch does address the reservation of JR0 by BootROM in both HAB and non-HAB configurations. My current setup does not include HAB functionality enabled, and I still do observe boot errors that are listed in commit message. This is due to the fact that the BootROM does not release JR0 to NS-World regardless of whether HAB is enabled or not. This has been discussed in the U-Boot thread I provided the link in the patch. This patch does rather bring the correct HW module description as seeing from Linux. > IMO this should be handled correctly by the bootloader and/or OP- > TEE. The default upstream configuration for OP-TEE is to not use the > CAAM at runtime as well, since linux runtime PM disablement of the CAAM > will lock up OP-TEE when it tries to access the CAAM. If by handling you mean releasing JR0 reservation - then yes, it should be done by either SPL or TF-A as they do run in S World. In such a case, DTB bindings need to be adapted further according to the new state. Until this done - this patch does provide a correct state of HW to the Kernel. > > Kind regards, > Rouven Czerwinski > > > > > Signed-off-by: Andrey Zhizhikin <andrey.zhizhikin@leica-geosystems.com> > > Link: [1]: https://lore.kernel.org/u-boot/AM6PR06MB4691FC905FE5658BE4B15C11A6609@AM6PR06MB4691.eurprd06.prod.outlook.com/ > > --- > > Changes in V3: > > - No change, new patch introduced > > > > arch/arm64/boot/dts/freescale/imx8mm.dtsi | 4 ++++ > > arch/arm64/boot/dts/freescale/imx8mn.dtsi | 4 ++++ > > arch/arm64/boot/dts/freescale/imx8mp.dtsi | 4 ++++ > > arch/arm64/boot/dts/freescale/imx8mq.dtsi | 4 ++++ > > 4 files changed, 16 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi > b/arch/arm64/boot/dts/freescale/imx8mm.dtsi > > index 5b9c2cca9ac4..51465974c4ea 100644 > > --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi > > +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi > > @@ -914,18 +914,22 @@ sec_jr0: jr@1000 { > > compatible = "fsl,sec-v4.0-job-ring"; > > reg = <0x1000 0x1000>; > > interrupts = <GIC_SPI 105 > IRQ_TYPE_LEVEL_HIGH>; > > + status = "disabled"; > > + secure-status = "okay"; > > }; > > > > sec_jr1: jr@2000 { > > compatible = "fsl,sec-v4.0-job-ring"; > > reg = <0x2000 0x1000>; > > interrupts = <GIC_SPI 106 > IRQ_TYPE_LEVEL_HIGH>; > > + secure-status = "okay"; > > }; > > > > sec_jr2: jr@3000 { > > compatible = "fsl,sec-v4.0-job-ring"; > > reg = <0x3000 0x1000>; > > interrupts = <GIC_SPI 114 > IRQ_TYPE_LEVEL_HIGH>; > > + secure-status = "disabled"; > > }; > > }; > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mn.dtsi > b/arch/arm64/boot/dts/freescale/imx8mn.dtsi > > index ba23b416b5e6..e5edf14319b1 100644 > > --- a/arch/arm64/boot/dts/freescale/imx8mn.dtsi > > +++ b/arch/arm64/boot/dts/freescale/imx8mn.dtsi > > @@ -808,18 +808,22 @@ sec_jr0: jr@1000 { > > compatible = "fsl,sec-v4.0-job-ring"; > > reg = <0x1000 0x1000>; > > interrupts = <GIC_SPI 105 > IRQ_TYPE_LEVEL_HIGH>; > > + status = "disabled"; > > + secure-status = "okay"; > > }; > > > > sec_jr1: jr@2000 { > > compatible = "fsl,sec-v4.0-job-ring"; > > reg = <0x2000 0x1000>; > > interrupts = <GIC_SPI 106 > IRQ_TYPE_LEVEL_HIGH>; > > + secure-status = "okay"; > > }; > > > > sec_jr2: jr@3000 { > > compatible = "fsl,sec-v4.0-job-ring"; > > reg = <0x3000 0x1000>; > > interrupts = <GIC_SPI 114 > IRQ_TYPE_LEVEL_HIGH>; > > + secure-status = "disabled"; > > }; > > }; > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi > b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > index 977783784342..3c23bf5c3910 100644 > > --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > @@ -661,18 +661,22 @@ sec_jr0: jr@1000 { > > compatible = "fsl,sec-v4.0-job-ring"; > > reg = <0x1000 0x1000>; > > interrupts = <GIC_SPI 105 > IRQ_TYPE_LEVEL_HIGH>; > > + status = "disabled"; > > + secure-status = "okay"; > > }; > > > > sec_jr1: jr@2000 { > > compatible = "fsl,sec-v4.0-job-ring"; > > reg = <0x2000 0x1000>; > > interrupts = <GIC_SPI 106 > IRQ_TYPE_LEVEL_HIGH>; > > + secure-status = "okay"; > > }; > > > > sec_jr2: jr@3000 { > > compatible = "fsl,sec-v4.0-job-ring"; > > reg = <0x3000 0x1000>; > > interrupts = <GIC_SPI 114 > IRQ_TYPE_LEVEL_HIGH>; > > + secure-status = "disabled"; > > }; > > }; > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi > b/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > index 95d8b95d6120..16c4c9110ce7 100644 > > --- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > +++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > @@ -999,18 +999,22 @@ sec_jr0: jr@1000 { > > compatible = "fsl,sec-v4.0-job-ring"; > > reg = <0x1000 0x1000>; > > interrupts = <GIC_SPI 105 > IRQ_TYPE_LEVEL_HIGH>; > > + status = "disabled"; > > + secure-status = "okay"; > > }; > > > > sec_jr1: jr@2000 { > > compatible = "fsl,sec-v4.0-job-ring"; > > reg = <0x2000 0x1000>; > > interrupts = <GIC_SPI 106 > IRQ_TYPE_LEVEL_HIGH>; > > + secure-status = "okay"; > > }; > > > > sec_jr2: jr@3000 { > > compatible = "fsl,sec-v4.0-job-ring"; > > reg = <0x3000 0x1000>; > > interrupts = <GIC_SPI 114 > IRQ_TYPE_LEVEL_HIGH>; > > + secure-status = "disabled"; > > }; > > }; > > > > -- > Pengutronix e.K. | Rouven Czerwinski | > Steuerwalder Str. 21 | http://www.pengutronix.de/ | > 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | -- andrey ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v3 2/2] arm64: dts: imx8m: define proper status for caam jr 2022-01-06 14:08 ` ZHIZHIKIN Andrey @ 2022-01-07 9:46 ` Rouven Czerwinski 2022-01-07 10:40 ` ZHIZHIKIN Andrey 2022-01-07 11:47 ` Michael Walle 0 siblings, 2 replies; 36+ messages in thread From: Rouven Czerwinski @ 2022-01-07 9:46 UTC (permalink / raw) To: ZHIZHIKIN Andrey, linux-kernel Cc: peng.fan, ping.bai, alice.guo, agx, krzk, leonard.crestez, festevam, marex, herbert, horia.geanta, daniel.baluta, frieder.schrempf, linux-imx, devicetree, hongxing.zhu, s.hauer, pankaj.gupta, robh+dt, thunder.leizhen, martink, aford173, linux-arm-kernel, gregkh, shengjiu.wang, qiangqing.zhang, michael, op-tee, linux-crypto, kernel, l.stach, shawnguo, davem, jun.li On Thu, 2022-01-06 at 14:08 +0000, ZHIZHIKIN Andrey wrote: > Hello Rouven, > > > -----Original Message----- > > From: Rouven Czerwinski <r.czerwinski@pengutronix.de> > > Sent: Thursday, January 6, 2022 12:27 PM > > To: ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com>; linux- > > kernel@vger.kernel.org > > Cc: peng.fan@nxp.com; ping.bai@nxp.com; alice.guo@nxp.com; agx@sigxcpu.org; > > frieder.schrempf@kontron.de; leonard.crestez@nxp.com; festevam@gmail.com; > > marex@denx.de; herbert@gondor.apana.org.au; horia.geanta@nxp.com; > > aford173@gmail.com; krzk@kernel.org; linux-imx@nxp.com; > > devicetree@vger.kernel.org; hongxing.zhu@nxp.com; s.hauer@pengutronix.de; > > pankaj.gupta@nxp.com; robh+dt@kernel.org; thunder.leizhen@huawei.com; > > martink@posteo.de; daniel.baluta@nxp.com; linux-arm-kernel@lists.infradead.org; > > gregkh@linuxfoundation.org; shengjiu.wang@nxp.com; qiangqing.zhang@nxp.com; > > michael@walle.cc; op-tee@lists.trustedfirmware.org; linux-crypto@vger.kernel.org; > > kernel@pengutronix.de; jun.li@nxp.com; shawnguo@kernel.org; davem@davemloft.net; > > l.stach@pengutronix.de > > Subject: Re: [PATCH v3 2/2] arm64: dts: imx8m: define proper status for caam jr > > > > Hi Andrey, > > > > On Wed, 2021-12-08 at 00:02 +0100, Andrey Zhizhikin wrote: > > > CAAM JR nodes are configured by BootROM and are used by various software > > > entities during the boot process before they reach the Kernel. > > > > > > Default BootROM configuration have JR0 and JR1 reserved for S-only > > > access, while JR2 is generally available for both S and NS access. HAB > > > feature of i.MX8M family does require that JR0 is reserved exclusively > > > in S-only world, while JR1 and JR2 are both released to NS-World. OP-TEE > > > can later reclaim the JR2 via dt_enable_secure_status() call, and modify > > > the DID to hold it in S-World only. > > > > > > The above setup has been discovered during review of CAAM patchset > > > presented to U-Boot integration [1], and does not correspond to the > > > status on jr nodes in FDT. > > > > > > This missing status settings leads to the following error message during > > > jr node probing: > > > [ 1.509894] caam 30900000.crypto: job rings = 3, qi = 0 > > > [ 1.525201] caam_jr 30901000.jr: failed to flush job ring 0 > > > [ 1.525214] caam_jr: probe of 30901000.jr failed with error -5 > > > > > > JR register readout after BootROM execution shows the following values: > > > JR0DID_MS = 0x8011 > > > JR1DID_MS = 0x8011 > > > JR2DID_MS = 0x0 > > > > > > This shows that JR0 and JR1 have TZ_OWN bit set, which marks them to be > > > reserved for S-World, while JR2 remains accessible from NS-World. > > > > > > Provide the correct status for JR nodes in imx8m derivatives, which have > > > a following meaning: > > > - JR0: S-only > > > - JR1: visible in both > > > - JR2: NS-only > > > > > > Note, that JR2 is initially marked to be NS-only which does correspond > > > to DID readout when OP-TEE is not present. Once present, OP-TEE will > > > reclaim the JR2 and set both "status" and "secure-status" to claim JR2 > > > for S-only access. > > > > While I can understand that you want to fix your use case for when HAB > > is enabled, note that this is disabling JR0 in the none-HAB case as > > well. > > This is not totally correct, as this patch does address the reservation of > JR0 by BootROM in both HAB and non-HAB configurations. My current setup does > not include HAB functionality enabled, and I still do observe boot errors > that are listed in commit message. This is due to the fact that the BootROM > does not release JR0 to NS-World regardless of whether HAB is enabled or not. > This has been discussed in the U-Boot thread I provided the link in the patch. > > This patch does rather bring the correct HW module description as seeing > from Linux. I took a look into i.MX8MQ, the bootrom indeed sets 0x8011 for JR0 & JR1: JR0:0000000000008011 JR1:0000000000008011 JR2:0000000000000000 TF-A >CAAM JR0:0000000000000001 JR1:0000000000000001 JR2:0000000000000001 However at least the upstream TF-A reconfigures the DIDs to 1 for all i.MX8M* derivates. So while it is technically correct to change the DT values as you describe, the downstream TF-A and upstream TF-A seem to differ in their configuration. I also think it's a bad move to hardcode the JR configuration to the boot ROM config, since AFAIK i.MX8M* can not be run without TF-A. And IMO the upstream kernel should follow what the upstream TF-A does in this case. > > > IMO this should be handled correctly by the bootloader and/or OP- > > TEE. The default upstream configuration for OP-TEE is to not use the > > CAAM at runtime as well, since linux runtime PM disablement of the CAAM > > will lock up OP-TEE when it tries to access the CAAM. > > If by handling you mean releasing JR0 reservation - then yes, it should be > done by either SPL or TF-A as they do run in S World. In such a case, DTB > bindings need to be adapted further according to the new state. Until this > done - this patch does provide a correct state of HW to the Kernel. Upstream TF-A simply releases all JRs to the normal world, matching the current DTB description. Kind Regards, Rouven Czerwinski > > > > Kind regards, > > Rouven Czerwinski > > > > > > > > Signed-off-by: Andrey Zhizhikin <andrey.zhizhikin@leica-geosystems.com> > > > Link: [1]: https://lore.kernel.org/u-boot/AM6PR06MB4691FC905FE5658BE4B15C11A6609@AM6PR06MB4691.eurprd06.prod.outlook.com/ > > > --- > > > Changes in V3: > > > - No change, new patch introduced > > > > > > arch/arm64/boot/dts/freescale/imx8mm.dtsi | 4 ++++ > > > arch/arm64/boot/dts/freescale/imx8mn.dtsi | 4 ++++ > > > arch/arm64/boot/dts/freescale/imx8mp.dtsi | 4 ++++ > > > arch/arm64/boot/dts/freescale/imx8mq.dtsi | 4 ++++ > > > 4 files changed, 16 insertions(+) > > > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi > > b/arch/arm64/boot/dts/freescale/imx8mm.dtsi > > > index 5b9c2cca9ac4..51465974c4ea 100644 > > > --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi > > > +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi > > > @@ -914,18 +914,22 @@ sec_jr0: jr@1000 { > > > compatible = "fsl,sec-v4.0-job-ring"; > > > reg = <0x1000 0x1000>; > > > interrupts = <GIC_SPI 105 > > IRQ_TYPE_LEVEL_HIGH>; > > > + status = "disabled"; > > > + secure-status = "okay"; > > > }; > > > > > > sec_jr1: jr@2000 { > > > compatible = "fsl,sec-v4.0-job-ring"; > > > reg = <0x2000 0x1000>; > > > interrupts = <GIC_SPI 106 > > IRQ_TYPE_LEVEL_HIGH>; > > > + secure-status = "okay"; > > > }; > > > > > > sec_jr2: jr@3000 { > > > compatible = "fsl,sec-v4.0-job-ring"; > > > reg = <0x3000 0x1000>; > > > interrupts = <GIC_SPI 114 > > IRQ_TYPE_LEVEL_HIGH>; > > > + secure-status = "disabled"; > > > }; > > > }; > > > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mn.dtsi > > b/arch/arm64/boot/dts/freescale/imx8mn.dtsi > > > index ba23b416b5e6..e5edf14319b1 100644 > > > --- a/arch/arm64/boot/dts/freescale/imx8mn.dtsi > > > +++ b/arch/arm64/boot/dts/freescale/imx8mn.dtsi > > > @@ -808,18 +808,22 @@ sec_jr0: jr@1000 { > > > compatible = "fsl,sec-v4.0-job-ring"; > > > reg = <0x1000 0x1000>; > > > interrupts = <GIC_SPI 105 > > IRQ_TYPE_LEVEL_HIGH>; > > > + status = "disabled"; > > > + secure-status = "okay"; > > > }; > > > > > > sec_jr1: jr@2000 { > > > compatible = "fsl,sec-v4.0-job-ring"; > > > reg = <0x2000 0x1000>; > > > interrupts = <GIC_SPI 106 > > IRQ_TYPE_LEVEL_HIGH>; > > > + secure-status = "okay"; > > > }; > > > > > > sec_jr2: jr@3000 { > > > compatible = "fsl,sec-v4.0-job-ring"; > > > reg = <0x3000 0x1000>; > > > interrupts = <GIC_SPI 114 > > IRQ_TYPE_LEVEL_HIGH>; > > > + secure-status = "disabled"; > > > }; > > > }; > > > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > > index 977783784342..3c23bf5c3910 100644 > > > --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > > +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > > @@ -661,18 +661,22 @@ sec_jr0: jr@1000 { > > > compatible = "fsl,sec-v4.0-job-ring"; > > > reg = <0x1000 0x1000>; > > > interrupts = <GIC_SPI 105 > > IRQ_TYPE_LEVEL_HIGH>; > > > + status = "disabled"; > > > + secure-status = "okay"; > > > }; > > > > > > sec_jr1: jr@2000 { > > > compatible = "fsl,sec-v4.0-job-ring"; > > > reg = <0x2000 0x1000>; > > > interrupts = <GIC_SPI 106 > > IRQ_TYPE_LEVEL_HIGH>; > > > + secure-status = "okay"; > > > }; > > > > > > sec_jr2: jr@3000 { > > > compatible = "fsl,sec-v4.0-job-ring"; > > > reg = <0x3000 0x1000>; > > > interrupts = <GIC_SPI 114 > > IRQ_TYPE_LEVEL_HIGH>; > > > + secure-status = "disabled"; > > > }; > > > }; > > > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > b/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > > index 95d8b95d6120..16c4c9110ce7 100644 > > > --- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > > +++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi > > > @@ -999,18 +999,22 @@ sec_jr0: jr@1000 { > > > compatible = "fsl,sec-v4.0-job-ring"; > > > reg = <0x1000 0x1000>; > > > interrupts = <GIC_SPI 105 > > IRQ_TYPE_LEVEL_HIGH>; > > > + status = "disabled"; > > > + secure-status = "okay"; > > > }; > > > > > > sec_jr1: jr@2000 { > > > compatible = "fsl,sec-v4.0-job-ring"; > > > reg = <0x2000 0x1000>; > > > interrupts = <GIC_SPI 106 > > IRQ_TYPE_LEVEL_HIGH>; > > > + secure-status = "okay"; > > > }; > > > > > > sec_jr2: jr@3000 { > > > compatible = "fsl,sec-v4.0-job-ring"; > > > reg = <0x3000 0x1000>; > > > interrupts = <GIC_SPI 114 > > IRQ_TYPE_LEVEL_HIGH>; > > > + secure-status = "disabled"; > > > }; > > > }; > > > > > > > -- > > Pengutronix e.K. | Rouven Czerwinski | > > Steuerwalder Str. 21 | http://www.pengutronix.de/ | > > 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > > > -- andrey -- Pengutronix e.K. | Rouven Czerwinski | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | ^ permalink raw reply [flat|nested] 36+ messages in thread
* RE: [PATCH v3 2/2] arm64: dts: imx8m: define proper status for caam jr 2022-01-07 9:46 ` Rouven Czerwinski @ 2022-01-07 10:40 ` ZHIZHIKIN Andrey 2022-01-07 11:55 ` Rouven Czerwinski 2022-01-07 11:47 ` Michael Walle 1 sibling, 1 reply; 36+ messages in thread From: ZHIZHIKIN Andrey @ 2022-01-07 10:40 UTC (permalink / raw) To: Rouven Czerwinski, linux-kernel Cc: peng.fan, ping.bai, alice.guo, agx, krzk, leonard.crestez, festevam, marex, herbert, horia.geanta, daniel.baluta, frieder.schrempf, linux-imx, devicetree, hongxing.zhu, s.hauer, pankaj.gupta, robh+dt, thunder.leizhen, martink, aford173, linux-arm-kernel, gregkh, shengjiu.wang, qiangqing.zhang, michael, op-tee, linux-crypto, kernel, l.stach, shawnguo, davem, jun.li Hello Rouven, > -----Original Message----- > From: Rouven Czerwinski <r.czerwinski@pengutronix.de> > Sent: Friday, January 7, 2022 10:46 AM > To: ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com>; linux- > kernel@vger.kernel.org > Cc: peng.fan@nxp.com; ping.bai@nxp.com; alice.guo@nxp.com; agx@sigxcpu.org; > krzk@kernel.org; leonard.crestez@nxp.com; festevam@gmail.com; marex@denx.de; > herbert@gondor.apana.org.au; horia.geanta@nxp.com; daniel.baluta@nxp.com; > frieder.schrempf@kontron.de; linux-imx@nxp.com; devicetree@vger.kernel.org; > hongxing.zhu@nxp.com; s.hauer@pengutronix.de; pankaj.gupta@nxp.com; > robh+dt@kernel.org; thunder.leizhen@huawei.com; martink@posteo.de; > aford173@gmail.com; linux-arm-kernel@lists.infradead.org; > gregkh@linuxfoundation.org; shengjiu.wang@nxp.com; qiangqing.zhang@nxp.com; > michael@walle.cc; op-tee@lists.trustedfirmware.org; linux-crypto@vger.kernel.org; > kernel@pengutronix.de; l.stach@pengutronix.de; shawnguo@kernel.org; > davem@davemloft.net; jun.li@nxp.com > Subject: Re: [PATCH v3 2/2] arm64: dts: imx8m: define proper status for caam jr > > > On Thu, 2022-01-06 at 14:08 +0000, ZHIZHIKIN Andrey wrote: > > Hello Rouven, > > > > > -----Original Message----- > > > From: Rouven Czerwinski <r.czerwinski@pengutronix.de> > > > Sent: Thursday, January 6, 2022 12:27 PM > > > To: ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com>; linux- > > > kernel@vger.kernel.org > > > Cc: peng.fan@nxp.com; ping.bai@nxp.com; alice.guo@nxp.com; agx@sigxcpu.org; > > > frieder.schrempf@kontron.de; leonard.crestez@nxp.com; festevam@gmail.com; > > > marex@denx.de; herbert@gondor.apana.org.au; horia.geanta@nxp.com; > > > aford173@gmail.com; krzk@kernel.org; linux-imx@nxp.com; > > > devicetree@vger.kernel.org; hongxing.zhu@nxp.com; s.hauer@pengutronix.de; > > > pankaj.gupta@nxp.com; robh+dt@kernel.org; thunder.leizhen@huawei.com; > > > martink@posteo.de; daniel.baluta@nxp.com; linux-arm- > kernel@lists.infradead.org; > > > gregkh@linuxfoundation.org; shengjiu.wang@nxp.com; qiangqing.zhang@nxp.com; > > > michael@walle.cc; op-tee@lists.trustedfirmware.org; linux- > crypto@vger.kernel.org; > > > kernel@pengutronix.de; jun.li@nxp.com; shawnguo@kernel.org; > davem@davemloft.net; > > > l.stach@pengutronix.de > > > Subject: Re: [PATCH v3 2/2] arm64: dts: imx8m: define proper status for caam > jr > > > > > > Hi Andrey, > > > > > > On Wed, 2021-12-08 at 00:02 +0100, Andrey Zhizhikin wrote: > > > > CAAM JR nodes are configured by BootROM and are used by various software > > > > entities during the boot process before they reach the Kernel. > > > > > > > > Default BootROM configuration have JR0 and JR1 reserved for S-only > > > > access, while JR2 is generally available for both S and NS access. HAB > > > > feature of i.MX8M family does require that JR0 is reserved exclusively > > > > in S-only world, while JR1 and JR2 are both released to NS-World. OP-TEE > > > > can later reclaim the JR2 via dt_enable_secure_status() call, and modify > > > > the DID to hold it in S-World only. > > > > > > > > The above setup has been discovered during review of CAAM patchset > > > > presented to U-Boot integration [1], and does not correspond to the > > > > status on jr nodes in FDT. > > > > > > > > This missing status settings leads to the following error message during > > > > jr node probing: > > > > [ 1.509894] caam 30900000.crypto: job rings = 3, qi = 0 > > > > [ 1.525201] caam_jr 30901000.jr: failed to flush job ring 0 > > > > [ 1.525214] caam_jr: probe of 30901000.jr failed with error -5 > > > > > > > > JR register readout after BootROM execution shows the following values: > > > > JR0DID_MS = 0x8011 > > > > JR1DID_MS = 0x8011 > > > > JR2DID_MS = 0x0 > > > > > > > > This shows that JR0 and JR1 have TZ_OWN bit set, which marks them to be > > > > reserved for S-World, while JR2 remains accessible from NS-World. > > > > > > > > Provide the correct status for JR nodes in imx8m derivatives, which have > > > > a following meaning: > > > > - JR0: S-only > > > > - JR1: visible in both > > > > - JR2: NS-only > > > > > > > > Note, that JR2 is initially marked to be NS-only which does correspond > > > > to DID readout when OP-TEE is not present. Once present, OP-TEE will > > > > reclaim the JR2 and set both "status" and "secure-status" to claim JR2 > > > > for S-only access. > > > > > > While I can understand that you want to fix your use case for when HAB > > > is enabled, note that this is disabling JR0 in the none-HAB case as > > > well. > > > > This is not totally correct, as this patch does address the reservation of > > JR0 by BootROM in both HAB and non-HAB configurations. My current setup does > > not include HAB functionality enabled, and I still do observe boot errors > > that are listed in commit message. This is due to the fact that the BootROM > > does not release JR0 to NS-World regardless of whether HAB is enabled or not. > > This has been discussed in the U-Boot thread I provided the link in the patch. > > > > This patch does rather bring the correct HW module description as seeing > > from Linux. > > I took a look into i.MX8MQ, the bootrom indeed sets 0x8011 for JR0 & > JR1: > JR0:0000000000008011 > JR1:0000000000008011 > JR2:0000000000000000 > TF-A > >CAAM > JR0:0000000000000001 > JR1:0000000000000001 > JR2:0000000000000001 > > However at least the upstream TF-A reconfigures the DIDs to 1 for all > i.MX8M* derivates. So while it is technically correct to change the DT > values as you describe, the downstream TF-A and upstream TF-A seem to > differ in their configuration. I also think it's a bad move to hardcode > the JR configuration to the boot ROM config, since AFAIK i.MX8M* can > not be run without TF-A. And IMO the upstream kernel should follow what > the upstream TF-A does in this case. This is indeed an interesting piece of information, thanks a lot! From what I understood in previous discussions for this series here in the Kernel, and on U-Boot ML: JR0 is required to be held reserved in S-World for HAB to operate, and this is clearly communicated by NXP in [1]. If my understanding is correct, then upstream TF-A either does not support or breaks the HAB feature. I've been following the build documentation in U-Boot, where the downstream TF-A is listed as build prequisites [2] without any mentioning of upstream, hence I received a readout that matched the BootROM "1-to-1". I believe that if the information from NXP regarding JR0 reservation for HAB is correct (and I have no reasons to doubt it), then upstream TF-A should be adapted in order for HAB feature to work, and in that case this patch brings correct HW state description as seeing from Linux. And in contrary, if the upstream TF-A is not adjusted to include HAB support - then applying this patch would effectively just "remove" one JR device, still keeping 2 additional available nodes for HW crypto operations in Kernel, with added benefit that both upstream and downstream TF-A can be used during the boot without seeing issues later in the Kernel. > > > > > > IMO this should be handled correctly by the bootloader and/or OP- > > > TEE. The default upstream configuration for OP-TEE is to not use the > > > CAAM at runtime as well, since linux runtime PM disablement of the CAAM > > > will lock up OP-TEE when it tries to access the CAAM. > > > > If by handling you mean releasing JR0 reservation - then yes, it should be > > done by either SPL or TF-A as they do run in S World. In such a case, DTB > > bindings need to be adapted further according to the new state. Until this > > done - this patch does provide a correct state of HW to the Kernel. > > Upstream TF-A simply releases all JRs to the normal world, matching the > current DTB description. > > Kind Regards, > Rouven Czerwinski > > [snip] Regards, Andrey Link: [1]: https://lore.kernel.org/u-boot/VI1PR04MB5342C8C6E651EC2CC4477EB5E79A9@VI1PR04MB5342.eurprd04.prod.outlook.com/ Link: [2]: https://source.denx.de/u-boot/u-boot/-/blob/master/doc/board/nxp/imx8mm_evk.rst ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v3 2/2] arm64: dts: imx8m: define proper status for caam jr 2022-01-07 10:40 ` ZHIZHIKIN Andrey @ 2022-01-07 11:55 ` Rouven Czerwinski 2022-01-08 20:48 ` ZHIZHIKIN Andrey 0 siblings, 1 reply; 36+ messages in thread From: Rouven Czerwinski @ 2022-01-07 11:55 UTC (permalink / raw) To: ZHIZHIKIN Andrey, linux-kernel Cc: peng.fan, ping.bai, alice.guo, agx, krzk, leonard.crestez, festevam, marex, herbert, horia.geanta, daniel.baluta, frieder.schrempf, linux-imx, devicetree, hongxing.zhu, s.hauer, pankaj.gupta, robh+dt, thunder.leizhen, martink, aford173, linux-arm-kernel, gregkh, shengjiu.wang, qiangqing.zhang, michael, op-tee, linux-crypto, kernel, l.stach, shawnguo, davem, jun.li On Fri, 2022-01-07 at 10:40 +0000, ZHIZHIKIN Andrey wrote: > Hello Rouven, > > > -----Original Message----- > > From: Rouven Czerwinski <r.czerwinski@pengutronix.de> > > Sent: Friday, January 7, 2022 10:46 AM > > To: ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com>; linux- > > kernel@vger.kernel.org > > Cc: peng.fan@nxp.com; ping.bai@nxp.com; alice.guo@nxp.com; agx@sigxcpu.org; > > krzk@kernel.org; leonard.crestez@nxp.com; festevam@gmail.com; marex@denx.de; > > herbert@gondor.apana.org.au; horia.geanta@nxp.com; daniel.baluta@nxp.com; > > frieder.schrempf@kontron.de; linux-imx@nxp.com; devicetree@vger.kernel.org; > > hongxing.zhu@nxp.com; s.hauer@pengutronix.de; pankaj.gupta@nxp.com; > > robh+dt@kernel.org; thunder.leizhen@huawei.com; martink@posteo.de; > > aford173@gmail.com; linux-arm-kernel@lists.infradead.org; > > gregkh@linuxfoundation.org; shengjiu.wang@nxp.com; qiangqing.zhang@nxp.com; > > michael@walle.cc; op-tee@lists.trustedfirmware.org; linux-crypto@vger.kernel.org; > > kernel@pengutronix.de; l.stach@pengutronix.de; shawnguo@kernel.org; > > davem@davemloft.net; jun.li@nxp.com > > Subject: Re: [PATCH v3 2/2] arm64: dts: imx8m: define proper status for caam jr > > > > > > On Thu, 2022-01-06 at 14:08 +0000, ZHIZHIKIN Andrey wrote: > > > Hello Rouven, > > > > > > > -----Original Message----- > > > > From: Rouven Czerwinski <r.czerwinski@pengutronix.de> > > > > Sent: Thursday, January 6, 2022 12:27 PM > > > > To: ZHIZHIKIN Andrey <andrey.zhizhikin@leica-geosystems.com>; linux- > > > > kernel@vger.kernel.org > > > > Cc: peng.fan@nxp.com; ping.bai@nxp.com; alice.guo@nxp.com; agx@sigxcpu.org; > > > > frieder.schrempf@kontron.de; leonard.crestez@nxp.com; festevam@gmail.com; > > > > marex@denx.de; herbert@gondor.apana.org.au; horia.geanta@nxp.com; > > > > aford173@gmail.com; krzk@kernel.org; linux-imx@nxp.com; > > > > devicetree@vger.kernel.org; hongxing.zhu@nxp.com; s.hauer@pengutronix.de; > > > > pankaj.gupta@nxp.com; robh+dt@kernel.org; thunder.leizhen@huawei.com; > > > > martink@posteo.de; daniel.baluta@nxp.com; linux-arm- > > kernel@lists.infradead.org; > > > > gregkh@linuxfoundation.org; shengjiu.wang@nxp.com; qiangqing.zhang@nxp.com; > > > > michael@walle.cc; op-tee@lists.trustedfirmware.org; linux- > > crypto@vger.kernel.org; > > > > kernel@pengutronix.de; jun.li@nxp.com; shawnguo@kernel.org; > > davem@davemloft.net; > > > > l.stach@pengutronix.de > > > > Subject: Re: [PATCH v3 2/2] arm64: dts: imx8m: define proper status for caam > > jr > > > > > > > > Hi Andrey, > > > > > > > > On Wed, 2021-12-08 at 00:02 +0100, Andrey Zhizhikin wrote: > > > > > CAAM JR nodes are configured by BootROM and are used by various software > > > > > entities during the boot process before they reach the Kernel. > > > > > > > > > > Default BootROM configuration have JR0 and JR1 reserved for S-only > > > > > access, while JR2 is generally available for both S and NS access. HAB > > > > > feature of i.MX8M family does require that JR0 is reserved exclusively > > > > > in S-only world, while JR1 and JR2 are both released to NS-World. OP-TEE > > > > > can later reclaim the JR2 via dt_enable_secure_status() call, and modify > > > > > the DID to hold it in S-World only. > > > > > > > > > > The above setup has been discovered during review of CAAM patchset > > > > > presented to U-Boot integration [1], and does not correspond to the > > > > > status on jr nodes in FDT. > > > > > > > > > > This missing status settings leads to the following error message during > > > > > jr node probing: > > > > > [ 1.509894] caam 30900000.crypto: job rings = 3, qi = 0 > > > > > [ 1.525201] caam_jr 30901000.jr: failed to flush job ring 0 > > > > > [ 1.525214] caam_jr: probe of 30901000.jr failed with error -5 > > > > > > > > > > JR register readout after BootROM execution shows the following values: > > > > > JR0DID_MS = 0x8011 > > > > > JR1DID_MS = 0x8011 > > > > > JR2DID_MS = 0x0 > > > > > > > > > > This shows that JR0 and JR1 have TZ_OWN bit set, which marks them to be > > > > > reserved for S-World, while JR2 remains accessible from NS-World. > > > > > > > > > > Provide the correct status for JR nodes in imx8m derivatives, which have > > > > > a following meaning: > > > > > - JR0: S-only > > > > > - JR1: visible in both > > > > > - JR2: NS-only > > > > > > > > > > Note, that JR2 is initially marked to be NS-only which does correspond > > > > > to DID readout when OP-TEE is not present. Once present, OP-TEE will > > > > > reclaim the JR2 and set both "status" and "secure-status" to claim JR2 > > > > > for S-only access. > > > > > > > > While I can understand that you want to fix your use case for when HAB > > > > is enabled, note that this is disabling JR0 in the none-HAB case as > > > > well. > > > > > > This is not totally correct, as this patch does address the reservation of > > > JR0 by BootROM in both HAB and non-HAB configurations. My current setup does > > > not include HAB functionality enabled, and I still do observe boot errors > > > that are listed in commit message. This is due to the fact that the BootROM > > > does not release JR0 to NS-World regardless of whether HAB is enabled or not. > > > This has been discussed in the U-Boot thread I provided the link in the patch. > > > > > > This patch does rather bring the correct HW module description as seeing > > > from Linux. > > > > I took a look into i.MX8MQ, the bootrom indeed sets 0x8011 for JR0 & > > JR1: > > JR0:0000000000008011 > > JR1:0000000000008011 > > JR2:0000000000000000 > > TF-A > > > CAAM > > JR0:0000000000000001 > > JR1:0000000000000001 > > JR2:0000000000000001 > > > > However at least the upstream TF-A reconfigures the DIDs to 1 for all > > i.MX8M* derivates. So while it is technically correct to change the DT > > values as you describe, the downstream TF-A and upstream TF-A seem to > > differ in their configuration. I also think it's a bad move to hardcode > > the JR configuration to the boot ROM config, since AFAIK i.MX8M* can > > not be run without TF-A. And IMO the upstream kernel should follow what > > the upstream TF-A does in this case. > > This is indeed an interesting piece of information, thanks a lot! > > From what I understood in previous discussions for this series here in > the Kernel, and on U-Boot ML: JR0 is required to be held reserved in > S-World for HAB to operate, and this is clearly communicated by NXP in [1]. > If my understanding is correct, then upstream TF-A either does not support > or breaks the HAB feature. Yes, upstream TF-A does not implement the NXP specific SIPs to communicate with the ROM code to do further image authentication. Thats also the reason why all JRs are released to normal world, there is no possible HAB use after TF-A has started. > I've been following the build documentation in U-Boot, where the downstream > TF-A is listed as build prequisites [2] without any mentioning of upstream, > hence I received a readout that matched the BootROM "1-to-1". Yes, since the downstream TF-A is required to authenticate further images. Aside from this the bootrom HAB interface on i.MX8MQ was broken in interesting ways, I had to implement parsing of the HAB status SRAM area by hand within barebox. > I believe that if the information from NXP regarding JR0 reservation for > HAB is correct (and I have no reasons to doubt it), then upstream TF-A > should be adapted in order for HAB feature to work, and in that case this > patch brings correct HW state description as seeing from Linux. JR0 for HAB in S-World makes sense to me, otherwise the bootrom will probably refuse to work with the JR. > And in contrary, if the upstream TF-A is not adjusted to include HAB > support - then applying this patch would effectively just "remove" one > JR device, still keeping 2 additional available nodes for HW crypto > operations in Kernel, with added benefit that both upstream and > downstream TF-A can be used during the boot without seeing issues later > in the Kernel. Even with the downstream TF-A there is no reason to keep JR0 asigned to the secure world, unless you are running OP-TEE. JR0 assignement for HAB shouldn't be required after the bootloader has run, unless you want to HAB authenticate images from a running Linux kernel. The reason NXP hardcodes the configuration downstream is of course to support HAB & OP-TEE, but this is still not a reason to hardcode this assignement into the kernel device tree. They probably also hardcode it in their downstream kernel versions. I can understand that it seems easier to hardcode this in the kernel, but as I said before, if you are running OP-TEE you need to adjust the DT anyway since nodes need to be added and you might as well adjust the jobring configuration than. Kind regards, Rouven > > > > > > > > > > IMO this should be handled correctly by the bootloader and/or OP- > > > > TEE. The default upstream configuration for OP-TEE is to not use the > > > > CAAM at runtime as well, since linux runtime PM disablement of the CAAM > > > > will lock up OP-TEE when it tries to access the CAAM. > > > > > > If by handling you mean releasing JR0 reservation - then yes, it should be > > > done by either SPL or TF-A as they do run in S World. In such a case, DTB > > > bindings need to be adapted further according to the new state. Until this > > > done - this patch does provide a correct state of HW to the Kernel. > > > > Upstream TF-A simply releases all JRs to the normal world, matching the > > current DTB description. > > > > Kind Regards, > > Rouven Czerwinski > > > > > [snip] > > Regards, > Andrey > > Link: [1]: https://lore.kernel.org/u-boot/VI1PR04MB5342C8C6E651EC2CC4477EB5E79A9@VI1PR04MB5342.eurprd04.prod.outlook.com/ > Link: [2]: https://source.denx.de/u-boot/u-boot/-/blob/master/doc/board/nxp/imx8mm_evk.rst -- Pengutronix e.K. | Rouven Czerwinski | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | ^ permalink raw reply [flat|nested] 36+ messages in thread
* RE: [PATCH v3 2/2] arm64: dts: imx8m: define proper status for caam jr 2022-01-07 11:55 ` Rouven Czerwinski @ 2022-01-08 20:48 ` ZHIZHIKIN Andrey 0 siblings, 0 replies; 36+ messages in thread From: ZHIZHIKIN Andrey @ 2022-01-08 20:48 UTC (permalink / raw) To: Rouven Czerwinski, linux-kernel Cc: peng.fan, ping.bai, alice.guo, agx, krzk, leonard.crestez, festevam, marex, herbert, horia.geanta, daniel.baluta, frieder.schrempf, linux-imx, devicetree, hongxing.zhu, s.hauer, pankaj.gupta, robh+dt, thunder.leizhen, martink, aford173, linux-arm-kernel, gregkh, shengjiu.wang, qiangqing.zhang, michael, op-tee, linux-crypto, kernel, l.stach, shawnguo, davem, jun.li Hello Rouven, > -----Original Message----- > From: Rouven Czerwinski <r.czerwinski@pengutronix.de> > Sent: Friday, January 7, 2022 12:56 PM [snip] > > Yes, upstream TF-A does not implement the NXP specific SIPs to > communicate with the ROM code to do further image authentication. > Thats also the reason why all JRs are released to normal world, there > is no possible HAB use after TF-A has started. > > > I've been following the build documentation in U-Boot, where the downstream > > TF-A is listed as build prequisites [2] without any mentioning of upstream, > > hence I received a readout that matched the BootROM "1-to-1". > > Yes, since the downstream TF-A is required to authenticate further > images. > > Aside from this the bootrom HAB interface on i.MX8MQ was broken in > interesting ways, I had to implement parsing of the HAB status SRAM > area by hand within barebox. > > > I believe that if the information from NXP regarding JR0 reservation for > > HAB is correct (and I have no reasons to doubt it), then upstream TF-A > > should be adapted in order for HAB feature to work, and in that case this > > patch brings correct HW state description as seeing from Linux. > > JR0 for HAB in S-World makes sense to me, otherwise the bootrom will > probably refuse to work with the JR. > > > And in contrary, if the upstream TF-A is not adjusted to include HAB > > support - then applying this patch would effectively just "remove" one > > JR device, still keeping 2 additional available nodes for HW crypto > > operations in Kernel, with added benefit that both upstream and > > downstream TF-A can be used during the boot without seeing issues later > > in the Kernel. > > Even with the downstream TF-A there is no reason to keep JR0 asigned to > the secure world, unless you are running OP-TEE. JR0 assignement for > HAB shouldn't be required after the bootloader has run, unless you want > to HAB authenticate images from a running Linux kernel. Then this probably should be communicated in U-Boot as there is a patchset proposed in U-Boot, and one of the patches in that series [1] disabled JR0 as well. Once merged - the JR0 is not going to be available for Linux, regardless of the fact that TF-A would set DIDs to 0x1. > > The reason NXP hardcodes the configuration downstream is of course to > support HAB & OP-TEE, but this is still not a reason to hardcode this > assignement into the kernel device tree. They probably also hardcode it > in their downstream kernel versions. Actually, I've checked the downstream NXP Kernel version, and none of the Job Ring nodes (including JR0) are disabled there. > > I can understand that it seems easier to hardcode this in the kernel, > but as I said before, if you are running OP-TEE you need to adjust the > DT anyway since nodes need to be added and you might as well adjust the > jobring configuration than. My first version of this patch has been targeting dynamic register readout to check if DID is set for either S or NS Worlds, but that was rejected due to unreliable readout in HW. Hence this attempt to statically disable node was made. Please note, that there are combinations out there which do have HAB, TF-A but no OP-TEE. In that case patching DT is not an option, and would cause the probing error at boot. Frankly speaking, I'm not sure how to proceed with this further... Clearly, there is an issue that JR devices are not available in certain combination of SW entities that are setting different permissions on JR: upstream TF-A does not do any reservation but does not support HAB (and this is aligned with current DT nodes description); downstream TF-A does perform reservation and support HAB, but does not release JR0 to NS-World causing error on the boot at JR probing. Since those 2 combinations are orthogonal - the only solution that I see (including the patch proposed in U-Boot ML) is to reserve the JR0 for all combinations, loosing it in Linux but covering both TF-A (HAB and non-HAB) at the same time. If you have any other suggestions here - I'm fully opened to receive those! > > Kind regards, > Rouven > > Regards, Andrey Link: [1]: https://lore.kernel.org/u-boot/20211207074129.10955-3-gaurav.jain@nxp.com/ ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v3 2/2] arm64: dts: imx8m: define proper status for caam jr 2022-01-07 9:46 ` Rouven Czerwinski 2022-01-07 10:40 ` ZHIZHIKIN Andrey @ 2022-01-07 11:47 ` Michael Walle 2022-01-07 11:58 ` Lucas Stach 1 sibling, 1 reply; 36+ messages in thread From: Michael Walle @ 2022-01-07 11:47 UTC (permalink / raw) To: Rouven Czerwinski Cc: ZHIZHIKIN Andrey, linux-kernel, peng.fan, ping.bai, alice.guo, agx, krzk, leonard.crestez, festevam, marex, herbert, horia.geanta, daniel.baluta, frieder.schrempf, linux-imx, devicetree, hongxing.zhu, s.hauer, pankaj.gupta, robh+dt, thunder.leizhen, martink, aford173, linux-arm-kernel, gregkh, shengjiu.wang, qiangqing.zhang, op-tee, linux-crypto, kernel, l.stach, shawnguo, davem, jun.li Hi Rouven, Am 2022-01-07 10:46, schrieb Rouven Czerwinski: > .. since AFAIK i.MX8M* can not be run without TF-A. Are you sure? There probably aren't any boards out there without TF-A, but why shouldn't it work without it? -michael ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v3 2/2] arm64: dts: imx8m: define proper status for caam jr 2022-01-07 11:47 ` Michael Walle @ 2022-01-07 11:58 ` Lucas Stach 2022-01-07 12:05 ` Michael Walle 0 siblings, 1 reply; 36+ messages in thread From: Lucas Stach @ 2022-01-07 11:58 UTC (permalink / raw) To: Michael Walle, Rouven Czerwinski Cc: ZHIZHIKIN Andrey, linux-kernel, peng.fan, ping.bai, alice.guo, agx, krzk, leonard.crestez, festevam, marex, herbert, horia.geanta, daniel.baluta, frieder.schrempf, linux-imx, devicetree, hongxing.zhu, s.hauer, pankaj.gupta, robh+dt, thunder.leizhen, martink, aford173, linux-arm-kernel, gregkh, shengjiu.wang, qiangqing.zhang, op-tee, linux-crypto, kernel, shawnguo, davem, jun.li Am Freitag, dem 07.01.2022 um 12:47 +0100 schrieb Michael Walle: > Hi Rouven, > > Am 2022-01-07 10:46, schrieb Rouven Czerwinski: > > .. since AFAIK i.MX8M* can not be run without TF-A. > > Are you sure? There probably aren't any boards out there > without TF-A, but why shouldn't it work without it? PSCI, i.e. the only means to start the secondary CPUs, is implemented in TF-A, so it's very unlikely that anyone would want to run a system without TF-A. Also quite a bit of the lowlevel SoC initialization is implemented in TF-A. Regards, Lucas ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v3 2/2] arm64: dts: imx8m: define proper status for caam jr 2022-01-07 11:58 ` Lucas Stach @ 2022-01-07 12:05 ` Michael Walle 0 siblings, 0 replies; 36+ messages in thread From: Michael Walle @ 2022-01-07 12:05 UTC (permalink / raw) To: Lucas Stach Cc: Rouven Czerwinski, ZHIZHIKIN Andrey, linux-kernel, peng.fan, ping.bai, alice.guo, agx, krzk, leonard.crestez, festevam, marex, herbert, horia.geanta, daniel.baluta, frieder.schrempf, linux-imx, devicetree, hongxing.zhu, s.hauer, pankaj.gupta, robh+dt, thunder.leizhen, martink, aford173, linux-arm-kernel, gregkh, shengjiu.wang, qiangqing.zhang, op-tee, linux-crypto, kernel, shawnguo, davem, jun.li Am 2022-01-07 12:58, schrieb Lucas Stach: > Am Freitag, dem 07.01.2022 um 12:47 +0100 schrieb Michael Walle: >> Hi Rouven, >> >> Am 2022-01-07 10:46, schrieb Rouven Czerwinski: >> > .. since AFAIK i.MX8M* can not be run without TF-A. >> >> Are you sure? There probably aren't any boards out there >> without TF-A, but why shouldn't it work without it? > > PSCI, i.e. the only means to start the secondary CPUs, is implemented > in TF-A, so it's very unlikely that anyone would want to run a system > without TF-A. Also quite a bit of the lowlevel SoC initialization is > implemented in TF-A. Doesn't mean u-boot cannot implement PSCI; actually you doesn't need it at all, you can still use spin tables. I just keep hearing the same arguments for the LS1028A SoC and yet there is one board without TF-A ;) Anyway, I admit it's rather unlikely. -michael ^ permalink raw reply [flat|nested] 36+ messages in thread
* RE: [PATCH v3 0/2] CAAM Driver: re-factor and set proper JR status 2021-12-07 23:02 ` [PATCH v3 0/2] CAAM Driver: re-factor and set proper JR status Andrey Zhizhikin 2021-12-07 23:02 ` [PATCH v3 1/2] crypto: caam - convert to use capabilities Andrey Zhizhikin 2021-12-07 23:02 ` [PATCH v3 2/2] arm64: dts: imx8m: define proper status for caam jr Andrey Zhizhikin @ 2022-01-06 10:56 ` ZHIZHIKIN Andrey 2022-01-07 2:36 ` Herbert Xu 2 siblings, 1 reply; 36+ messages in thread From: ZHIZHIKIN Andrey @ 2022-01-06 10:56 UTC (permalink / raw) To: herbert, linux-kernel Cc: robh+dt, shawnguo, michael, s.hauer, kernel, festevam, linux-imx, horia.geanta, pankaj.gupta, davem, l.stach, qiangqing.zhang, peng.fan, alice.guo, aford173, frieder.schrempf, krzk, shengjiu.wang, gregkh, ping.bai, daniel.baluta, jun.li, marex, thunder.leizhen, martink, leonard.crestez, hongxing.zhu, agx, devicetree, linux-arm-kernel, linux-crypto, op-tee Hello Herbert, Gentle ping on this V3. I see that in Patchwork this series state is set to "Deferred". Is there anything missing here to proceed further? > -----Original Message----- > From: Andrey Zhizhikin <andrey.zhizhikin@leica-geosystems.com> > Sent: Wednesday, December 8, 2021 12:02 AM > To: linux-kernel@vger.kernel.org > Cc: robh+dt@kernel.org; shawnguo@kernel.org; michael@walle.cc; > s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com; linux- > imx@nxp.com; horia.geanta@nxp.com; pankaj.gupta@nxp.com; > herbert@gondor.apana.org.au; davem@davemloft.net; l.stach@pengutronix.de; > qiangqing.zhang@nxp.com; peng.fan@nxp.com; alice.guo@nxp.com; aford173@gmail.com; > frieder.schrempf@kontron.de; krzk@kernel.org; shengjiu.wang@nxp.com; > gregkh@linuxfoundation.org; ping.bai@nxp.com; daniel.baluta@nxp.com; > jun.li@nxp.com; marex@denx.de; thunder.leizhen@huawei.com; martink@posteo.de; > leonard.crestez@nxp.com; hongxing.zhu@nxp.com; agx@sigxcpu.org; > devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux- > crypto@vger.kernel.org; op-tee@lists.trustedfirmware.org; Andrey Zhizhikin > <andrey.zhizhikin@leica-geosystems.com> > Subject: [PATCH v3 0/2] CAAM Driver: re-factor and set proper JR status > > This V3 series covers points uncovered during the review of the previous > series, one major point being that register readout should not be used > for dynamic JR availability check due to its unreliability. > > Instead, JR should have a proper status set in FDT which indicates the > availability of the ring in NS-World. This status is aligned with what > BootROM code configures, and can be modified by all actors in the boot > chain. > > Therefore, patch in V2 series that was handling the dynamic JR > availability check is dropped in this series and replaced by the patch > which sets proper DT status for JR nodes. > > Andrey Zhizhikin (2): > crypto: caam - convert to use capabilities > arm64: dts: imx8m: define proper status for caam jr > > arch/arm64/boot/dts/freescale/imx8mm.dtsi | 4 + > arch/arm64/boot/dts/freescale/imx8mn.dtsi | 4 + > arch/arm64/boot/dts/freescale/imx8mp.dtsi | 4 + > arch/arm64/boot/dts/freescale/imx8mq.dtsi | 4 + > drivers/crypto/caam/caamalg_qi.c | 2 +- > drivers/crypto/caam/ctrl.c | 115 ++++++++++++++-------- > drivers/crypto/caam/intern.h | 20 ++-- > drivers/crypto/caam/jr.c | 19 +++- > drivers/crypto/caam/regs.h | 2 - > 9 files changed, 122 insertions(+), 52 deletions(-) > > > base-commit: 04fe99a8d936d46a310ca61b8b63dc270962bf01 > -- > 2.25.1 > -- andrey ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v3 0/2] CAAM Driver: re-factor and set proper JR status 2022-01-06 10:56 ` [PATCH v3 0/2] CAAM Driver: re-factor and set proper JR status ZHIZHIKIN Andrey @ 2022-01-07 2:36 ` Herbert Xu 0 siblings, 0 replies; 36+ messages in thread From: Herbert Xu @ 2022-01-07 2:36 UTC (permalink / raw) To: ZHIZHIKIN Andrey Cc: linux-kernel, robh+dt, shawnguo, michael, s.hauer, kernel, festevam, linux-imx, horia.geanta, pankaj.gupta, davem, l.stach, qiangqing.zhang, peng.fan, alice.guo, aford173, frieder.schrempf, krzk, shengjiu.wang, gregkh, ping.bai, daniel.baluta, jun.li, marex, thunder.leizhen, martink, leonard.crestez, hongxing.zhu, agx, devicetree, linux-arm-kernel, linux-crypto, op-tee On Thu, Jan 06, 2022 at 10:56:12AM +0000, ZHIZHIKIN Andrey wrote: > Hello Herbert, > > Gentle ping on this V3. I see that in Patchwork this series state is set to "Deferred". > > Is there anything missing here to proceed further? Please get the caam driver maintainer to review and ack the patch series. Thanks, -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 36+ messages in thread
end of thread, other threads:[~2022-01-08 20:48 UTC | newest] Thread overview: 36+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-11-04 16:21 [PATCH] crypto: caam - check jr permissions before probing Andrey Zhizhikin 2021-11-05 1:20 ` Michael Walle 2021-11-05 10:34 ` ZHIZHIKIN Andrey 2021-11-05 23:16 ` Michael Walle 2021-11-08 10:24 ` ZHIZHIKIN Andrey 2021-11-08 16:13 ` Michael Walle 2021-11-10 9:33 ` ZHIZHIKIN Andrey 2021-11-12 18:55 ` Michael Walle 2021-11-15 9:38 ` ZHIZHIKIN Andrey 2021-11-15 11:40 ` Michael Walle 2021-11-11 16:45 ` [PATCH v2 0/2] CAAM Driver: re-factor and dynamic JR availability check Andrey Zhizhikin 2021-11-11 16:46 ` [PATCH v2 1/2] crypto: caam - convert to use capabilities Andrey Zhizhikin 2021-11-12 19:22 ` Michael Walle 2021-11-15 9:45 ` ZHIZHIKIN Andrey 2021-11-11 16:46 ` [PATCH v2 2/2] crypto: caam - check jr permissions before probing Andrey Zhizhikin 2021-11-12 21:17 ` Michael Walle 2021-11-15 10:07 ` ZHIZHIKIN Andrey 2021-11-15 11:17 ` Michael Walle 2021-11-18 0:47 ` Horia Geantă 2021-11-18 8:28 ` Michael Walle 2021-11-18 10:08 ` ZHIZHIKIN Andrey 2021-11-18 10:11 ` Michael Walle 2021-12-07 23:02 ` [PATCH v3 0/2] CAAM Driver: re-factor and set proper JR status Andrey Zhizhikin 2021-12-07 23:02 ` [PATCH v3 1/2] crypto: caam - convert to use capabilities Andrey Zhizhikin 2021-12-07 23:02 ` [PATCH v3 2/2] arm64: dts: imx8m: define proper status for caam jr Andrey Zhizhikin 2022-01-06 11:26 ` Rouven Czerwinski 2022-01-06 14:08 ` ZHIZHIKIN Andrey 2022-01-07 9:46 ` Rouven Czerwinski 2022-01-07 10:40 ` ZHIZHIKIN Andrey 2022-01-07 11:55 ` Rouven Czerwinski 2022-01-08 20:48 ` ZHIZHIKIN Andrey 2022-01-07 11:47 ` Michael Walle 2022-01-07 11:58 ` Lucas Stach 2022-01-07 12:05 ` Michael Walle 2022-01-06 10:56 ` [PATCH v3 0/2] CAAM Driver: re-factor and set proper JR status ZHIZHIKIN Andrey 2022-01-07 2:36 ` Herbert Xu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).