Hello Ahmad
> -----Original Message-----
> From: Ahmad Fatoum <a.fatoum@xxxxxxxxxxxxxx>
> Sent: Monday, November 11, 2024 5:52 PM
> To: Gaurav Jain <gaurav.jain@xxxxxxx>; Horia Geanta
> <horia.geanta@xxxxxxx>; Pankaj Gupta <pankaj.gupta@xxxxxxx>; Herbert
> Xu <herbert@xxxxxxxxxxxxxxxxxxx>; David S . Miller
> <davem@xxxxxxxxxxxxx>; Silvano Di Ninno <silvano.dininno@xxxxxxx>;
> Varun Sethi <V.Sethi@xxxxxxx>; Meenakshi Aggarwal
> <meenakshi.aggarwal@xxxxxxx>; Sahil Malhotra
> <sahil.malhotra@xxxxxxx>; Nikolaus Voss <nikolaus.voss@xxxxxxxxxxxxxxx>
> Cc: linux-crypto@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Pengutronix
> Kernel Team <kernel@xxxxxxxxxxxxxx>
> Subject: [EXT] Re: [PATCH] crypto: caam - use JobR's space to access page 0
> regs
>
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report
> this email' button
>
>
> Hello Guarav,
>
> Thanks for your patch.
>
> On 11.11.24 13:10, Gaurav Jain wrote:
> > Access to controller region is not permitted.
>
> It's permitted on most of the older SoCs. Please mention on which SoCs this
> is no longer true and which SoCs you tested your change on.
Yes, it is permitted on iMX6/7/8M SoCs but not on iMX8DXL/QM/QXP/8ULP.
>
> > use JobR's register space to access page 0 registers.
> >
> > Fixes: 6a83830f649a ("crypto: caam - warn if blob_gen key is
> > insecure")
>
> Did the CAAM even support any of the SoCs, where this doesn't work
> anymore back when the code was mainlined?
Yes, for all SECO/ELE based SoCs, CAAM page 0 is not accessible from Non secure world.
>
> > Signed-off-by: Gaurav Jain <gaurav.jain@xxxxxxx>
> > ---
> > drivers/crypto/caam/blob_gen.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/crypto/caam/blob_gen.c
> > b/drivers/crypto/caam/blob_gen.c index 87781c1534ee..079a22cc9f02
> > 100644
> > --- a/drivers/crypto/caam/blob_gen.c
> > +++ b/drivers/crypto/caam/blob_gen.c
> > @@ -2,6 +2,7 @@
> > /*
> > * Copyright (C) 2015 Pengutronix, Steffen Trumtrar
> <kernel@xxxxxxxxxxxxxx>
> > * Copyright (C) 2021 Pengutronix, Ahmad Fatoum
> > <kernel@xxxxxxxxxxxxxx>
> > + * Copyright 2024 NXP
> > */
> >
> > #define pr_fmt(fmt) "caam blob_gen: " fmt @@ -104,7 +105,7 @@ int
> > caam_process_blob(struct caam_blob_priv *priv,
> > }
> >
> > ctrlpriv = dev_get_drvdata(jrdev->parent);
> > - moo = FIELD_GET(CSTA_MOO, rd_reg32(&ctrlpriv->ctrl-
> >perfmon.status));
> > + moo = FIELD_GET(CSTA_MOO,
> > + rd_reg32(&ctrlpriv->jr[0]->perfmon.status));
>
> I believe your change is correct, but I would prefer that ctrlpriv gets a
> perfmon member that is initialized in caam_probe to either &ctrlpriv->ctrl-
> >perfmon.status or &ctrlpriv->jr[0]->perfmon.status and then the code here
> would just use &ctrlpriv->perfmon->status.
>
> This would simplify code not only here, but also in caam_ctrl_rng_init.
As already communicated by Horia, a separate patch is good to cover this.
Thanks
Gaurav
>
> Thanks,
> Ahmad
>
>
> > if (moo != CSTA_MOO_SECURE && moo != CSTA_MOO_TRUSTED)
> > dev_warn(jrdev,
> > "using insecure test key, enable HAB to use
> > unique device key!\n");
>
>
> --
> Pengutronix e.K. | |
> Steuerwalder Str. 21 |
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
> pengutronix.de%2F&data=05%7C02%7Cgaurav.jain%40nxp.com%7C758768
> 98a8044b366f4808dd024b7740%7C686ea1d3bc2b4c6fa92cd99c5c30163
> 5%7C0%7C0%7C638669245367988594%7CUnknown%7CTWFpbGZsb3d8e
> yJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIj
> oiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=aaQ65iMsvuHn3q
> 0bo5UU%2FYU7Fpyw3El7wNVHd%2BMNee0%3D&reserved=0 |
> 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
