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@xxxxxxxxxxxxxxxxxxxx>
---
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
